[PATCH] trace: Add a free on close control mechanism for buffer_size_kb

[PATCH] trace: Add a free on close control mechanism for buffer_size_kb

[Vaibhav Nagarnaik]

The proc file entry buffer_size_kb is used to set the size of tracing
buffer. The memory to expand the buffer size is kernel memory. Consider
a use case where tracing is handled by a user space utility, which acts
as a gate keeper for tracing requests. In an OOM condition, tracing is
considered a low priority task and if the utility gets killed the ring
buffer memory cannot be released back to the kernel.

This patch adds an IOCTL on the buffer_size_kb file to set a boolean.
When this boolean is enabled, closing buffer_size_kb file will cause
tracing to stop and free up the ring buffer memory.

The user space process can then open the buffer_size_kb file to set the
new buffer size for tracing, enable the boolean through IOCTL and keep
the file open. Under OOM condition, if the process gets killed, the
kernel closes the file descriptor for buffer_size_kb. The release
handler stops the tracing and releases the kernel memory automatically.

Signed-off-by: Vaibhav Nagarnaik <vnagarnaik@google.com>
---
 include/linux/ftrace.h |    3 +
 kernel/trace/trace.c   |  135 ++++++++++++++++++++++++++++++++++++------------
 2 files changed, 105 insertions(+), 33 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index ca29e03..30c8a23 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -114,6 +114,9 @@ struct ftrace_func_command {
 					char *params, int enable);
 };
 
+/* enable/disable auto free ring buffer on file close */
+#define TRACE_RINGBUF_FREE_ON_CLOSE _IOW('t', 0x01, int)
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 
 int ftrace_arch_code_modify_prepare(void);
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index d38c16a..c676f17 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2817,6 +2817,42 @@ static int tracing_resize_ring_buffer(unsigned long size)
 	return ret;
 }
 
+static ssize_t tracing_buffer_resize_atomic(unsigned long size)
+{
+	int cpu, ret = 0;
+
+	mutex_lock(&trace_types_lock);
+
+	tracing_stop();
+
+	/* disable all cpu buffers */
+	for_each_tracing_cpu(cpu) {
+		if (global_trace.data[cpu])
+			atomic_inc(&global_trace.data[cpu]->disabled);
+		if (max_tr.data[cpu])
+			atomic_inc(&max_tr.data[cpu]->disabled);
+	}
+
+	if (size != global_trace.entries)
+		ret = tracing_resize_ring_buffer(size);
+
+	/* If check pages failed, return ENOMEM */
+	if (tracing_disabled)
+		ret = -ENOMEM;
+
+	for_each_tracing_cpu(cpu) {
+		if (global_trace.data[cpu])
+			atomic_dec(&global_trace.data[cpu]->disabled);
+		if (max_tr.data[cpu])
+			atomic_dec(&max_tr.data[cpu]->disabled);
+	}
+
+	tracing_start();
+	mutex_unlock(&trace_types_lock);
+
+	return ret;
+}
+
 
 /**
  * tracing_update_buffers - used by tracing facility to expand ring buffers
@@ -3399,11 +3435,37 @@ out_err:
 	goto out;
 }
 
+struct ftrace_entries_info {
+	struct trace_array	*tr;
+	int			free_buffer_on_close;
+};
+
+static int
+tracing_entries_open(struct inode *inode, struct file *filp)
+{
+	struct ftrace_entries_info *info;
+
+	if (tracing_disabled)
+		return -ENODEV;
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	info->tr = (struct trace_array *)inode->i_private;
+	info->free_buffer_on_close = 0;
+
+	filp->private_data = info;
+
+	return 0;
+}
+
 static ssize_t
 tracing_entries_read(struct file *filp, char __user *ubuf,
 		     size_t cnt, loff_t *ppos)
 {
-	struct trace_array *tr = filp->private_data;
+	struct ftrace_entries_info *info = filp->private_data;
+	struct trace_array *tr = info->tr;
 	char buf[96];
 	int r;
 
@@ -3425,7 +3487,7 @@ tracing_entries_write(struct file *filp, const char __user *ubuf,
 {
 	unsigned long val;
 	char buf[64];
-	int ret, cpu;
+	int ret;
 
 	if (cnt >= sizeof(buf))
 		return -EINVAL;
@@ -3443,46 +3505,50 @@ tracing_entries_write(struct file *filp, const char __user *ubuf,
 	if (!val)
 		return -EINVAL;
 
-	mutex_lock(&trace_types_lock);
+	/* value is in KB */
+	val <<= 10;
 
-	tracing_stop();
+	ret = tracing_buffer_resize_atomic(val);
+	if (ret < 0)
+		return ret;
 
-	/* disable all cpu buffers */
-	for_each_tracing_cpu(cpu) {
-		if (global_trace.data[cpu])
-			atomic_inc(&global_trace.data[cpu]->disabled);
-		if (max_tr.data[cpu])
-			atomic_inc(&max_tr.data[cpu]->disabled);
-	}
+	*ppos += cnt;
 
-	/* value is in KB */
-	val <<= 10;
+	return cnt;
+}
 
-	if (val != global_trace.entries) {
-		ret = tracing_resize_ring_buffer(val);
-		if (ret < 0) {
-			cnt = ret;
-			goto out;
-		}
+static long
+tracing_entries_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+	int ret = -ENOIOCTLCMD;
+	struct ftrace_entries_info *info = filp->private_data;
+
+	switch (cmd) {
+	case TRACE_RINGBUF_FREE_ON_CLOSE: {
+		info->free_buffer_on_close = !!arg;
+		ret = 0;
+		break;
+	}
 	}
 
-	*ppos += cnt;
+	return ret;
+}
 
-	/* If check pages failed, return ENOMEM */
-	if (tracing_disabled)
-		cnt = -ENOMEM;
- out:
-	for_each_tracing_cpu(cpu) {
-		if (global_trace.data[cpu])
-			atomic_dec(&global_trace.data[cpu]->disabled);
-		if (max_tr.data[cpu])
-			atomic_dec(&max_tr.data[cpu]->disabled);
+static int
+tracing_entries_release(struct inode *inode, struct file *filp)
+{
+	struct ftrace_entries_info *info = filp->private_data;
+
+	if (info->free_buffer_on_close) {
+		/* disable tracing */
+		tracing_off();
+		/* resize the ring buffer to 0 */
+		tracing_buffer_resize_atomic(0);
 	}
 
-	tracing_start();
-	mutex_unlock(&trace_types_lock);
+	kfree(info);
 
-	return cnt;
+	return 0;
 }
 
 static int mark_printk(const char *fmt, ...)
@@ -3624,9 +3690,12 @@ static const struct file_operations tracing_pipe_fops = {
 };
 
 static const struct file_operations tracing_entries_fops = {
-	.open		= tracing_open_generic,
+	.open		= tracing_entries_open,
 	.read		= tracing_entries_read,
 	.write		= tracing_entries_write,
+	.unlocked_ioctl	= tracing_entries_ioctl,
+	.compat_ioctl	= tracing_entries_ioctl,
+	.release	= tracing_entries_release,
 	.llseek		= generic_file_llseek,
 };
 
-- 
1.7.3.1

Re: [PATCH] trace: Add a free on close control mechanism for buffer_size_kb

[Vaibhav Nagarnaik]

All

Can you please take a look at this patch and provide your thoughts?


Thanks
Vaibhav Nagarnaik



On Fri, Apr 22, 2011 at 3:46 PM, Vaibhav Nagarnaik
<vnagarnaik@google.com> wrote:
> The proc file entry buffer_size_kb is used to set the size of tracing
> buffer. The memory to expand the buffer size is kernel memory. Consider
> a use case where tracing is handled by a user space utility, which acts
> as a gate keeper for tracing requests. In an OOM condition, tracing is
> considered a low priority task and if the utility gets killed the ring
> buffer memory cannot be released back to the kernel.
>
> This patch adds an IOCTL on the buffer_size_kb file to set a boolean.
> When this boolean is enabled, closing buffer_size_kb file will cause
> tracing to stop and free up the ring buffer memory.
>
> The user space process can then open the buffer_size_kb file to set the
> new buffer size for tracing, enable the boolean through IOCTL and keep
> the file open. Under OOM condition, if the process gets killed, the
> kernel closes the file descriptor for buffer_size_kb. The release
> handler stops the tracing and releases the kernel memory automatically.
>
> Signed-off-by: Vaibhav Nagarnaik <vnagarnaik@google.com>
> ---
>  include/linux/ftrace.h |    3 +
>  kernel/trace/trace.c   |  135 ++++++++++++++++++++++++++++++++++++------------
>  2 files changed, 105 insertions(+), 33 deletions(-)
>
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index ca29e03..30c8a23 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -114,6 +114,9 @@ struct ftrace_func_command {
>                                        char *params, int enable);
>  };
>
> +/* enable/disable auto free ring buffer on file close */
> +#define TRACE_RINGBUF_FREE_ON_CLOSE _IOW('t', 0x01, int)
> +
>  #ifdef CONFIG_DYNAMIC_FTRACE
>
>  int ftrace_arch_code_modify_prepare(void);
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index d38c16a..c676f17 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -2817,6 +2817,42 @@ static int tracing_resize_ring_buffer(unsigned long size)
>        return ret;
>  }
>
> +static ssize_t tracing_buffer_resize_atomic(unsigned long size)
> +{
> +       int cpu, ret = 0;
> +
> +       mutex_lock(&trace_types_lock);
> +
> +       tracing_stop();
> +
> +       /* disable all cpu buffers */
> +       for_each_tracing_cpu(cpu) {
> +               if (global_trace.data[cpu])
> +                       atomic_inc(&global_trace.data[cpu]->disabled);
> +               if (max_tr.data[cpu])
> +                       atomic_inc(&max_tr.data[cpu]->disabled);
> +       }
> +
> +       if (size != global_trace.entries)
> +               ret = tracing_resize_ring_buffer(size);
> +
> +       /* If check pages failed, return ENOMEM */
> +       if (tracing_disabled)
> +               ret = -ENOMEM;
> +
> +       for_each_tracing_cpu(cpu) {
> +               if (global_trace.data[cpu])
> +                       atomic_dec(&global_trace.data[cpu]->disabled);
> +               if (max_tr.data[cpu])
> +                       atomic_dec(&max_tr.data[cpu]->disabled);
> +       }
> +
> +       tracing_start();
> +       mutex_unlock(&trace_types_lock);
> +
> +       return ret;
> +}
> +
>
>  /**
>  * tracing_update_buffers - used by tracing facility to expand ring buffers
> @@ -3399,11 +3435,37 @@ out_err:
>        goto out;
>  }
>
> +struct ftrace_entries_info {
> +       struct trace_array      *tr;
> +       int                     free_buffer_on_close;
> +};
> +
> +static int
> +tracing_entries_open(struct inode *inode, struct file *filp)
> +{
> +       struct ftrace_entries_info *info;
> +
> +       if (tracing_disabled)
> +               return -ENODEV;
> +
> +       info = kzalloc(sizeof(*info), GFP_KERNEL);
> +       if (!info)
> +               return -ENOMEM;
> +
> +       info->tr = (struct trace_array *)inode->i_private;
> +       info->free_buffer_on_close = 0;
> +
> +       filp->private_data = info;
> +
> +       return 0;
> +}
> +
>  static ssize_t
>  tracing_entries_read(struct file *filp, char __user *ubuf,
>                     size_t cnt, loff_t *ppos)
>  {
> -       struct trace_array *tr = filp->private_data;
> +       struct ftrace_entries_info *info = filp->private_data;
> +       struct trace_array *tr = info->tr;
>        char buf[96];
>        int r;
>
> @@ -3425,7 +3487,7 @@ tracing_entries_write(struct file *filp, const char __user *ubuf,
>  {
>        unsigned long val;
>        char buf[64];
> -       int ret, cpu;
> +       int ret;
>
>        if (cnt >= sizeof(buf))
>                return -EINVAL;
> @@ -3443,46 +3505,50 @@ tracing_entries_write(struct file *filp, const char __user *ubuf,
>        if (!val)
>                return -EINVAL;
>
> -       mutex_lock(&trace_types_lock);
> +       /* value is in KB */
> +       val <<= 10;
>
> -       tracing_stop();
> +       ret = tracing_buffer_resize_atomic(val);
> +       if (ret < 0)
> +               return ret;
>
> -       /* disable all cpu buffers */
> -       for_each_tracing_cpu(cpu) {
> -               if (global_trace.data[cpu])
> -                       atomic_inc(&global_trace.data[cpu]->disabled);
> -               if (max_tr.data[cpu])
> -                       atomic_inc(&max_tr.data[cpu]->disabled);
> -       }
> +       *ppos += cnt;
>
> -       /* value is in KB */
> -       val <<= 10;
> +       return cnt;
> +}
>
> -       if (val != global_trace.entries) {
> -               ret = tracing_resize_ring_buffer(val);
> -               if (ret < 0) {
> -                       cnt = ret;
> -                       goto out;
> -               }
> +static long
> +tracing_entries_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +{
> +       int ret = -ENOIOCTLCMD;
> +       struct ftrace_entries_info *info = filp->private_data;
> +
> +       switch (cmd) {
> +       case TRACE_RINGBUF_FREE_ON_CLOSE: {
> +               info->free_buffer_on_close = !!arg;
> +               ret = 0;
> +               break;
> +       }
>        }
>
> -       *ppos += cnt;
> +       return ret;
> +}
>
> -       /* If check pages failed, return ENOMEM */
> -       if (tracing_disabled)
> -               cnt = -ENOMEM;
> - out:
> -       for_each_tracing_cpu(cpu) {
> -               if (global_trace.data[cpu])
> -                       atomic_dec(&global_trace.data[cpu]->disabled);
> -               if (max_tr.data[cpu])
> -                       atomic_dec(&max_tr.data[cpu]->disabled);
> +static int
> +tracing_entries_release(struct inode *inode, struct file *filp)
> +{
> +       struct ftrace_entries_info *info = filp->private_data;
> +
> +       if (info->free_buffer_on_close) {
> +               /* disable tracing */
> +               tracing_off();
> +               /* resize the ring buffer to 0 */
> +               tracing_buffer_resize_atomic(0);
>        }
>
> -       tracing_start();
> -       mutex_unlock(&trace_types_lock);
> +       kfree(info);
>
> -       return cnt;
> +       return 0;
>  }
>
>  static int mark_printk(const char *fmt, ...)
> @@ -3624,9 +3690,12 @@ static const struct file_operations tracing_pipe_fops = {
>  };
>
>  static const struct file_operations tracing_entries_fops = {
> -       .open           = tracing_open_generic,
> +       .open           = tracing_entries_open,
>        .read           = tracing_entries_read,
>        .write          = tracing_entries_write,
> +       .unlocked_ioctl = tracing_entries_ioctl,
> +       .compat_ioctl   = tracing_entries_ioctl,
> +       .release        = tracing_entries_release,
>        .llseek         = generic_file_llseek,
>  };
>
> --
> 1.7.3.1
>
>

Re: [PATCH] trace: Add a free on close control mechanism for buffer_size_kb

[Steven Rostedt]

Sorry, my mind has been elsewhere and I forgot about your patches.

On Fri, 2011-04-22 at 15:46 -0700, Vaibhav Nagarnaik wrote:
> The proc file entry buffer_size_kb is used to set the size of tracing
> buffer. The memory to expand the buffer size is kernel memory. Consider
> a use case where tracing is handled by a user space utility, which acts
> as a gate keeper for tracing requests. In an OOM condition, tracing is
> considered a low priority task and if the utility gets killed the ring
> buffer memory cannot be released back to the kernel.
> 
> This patch adds an IOCTL on the buffer_size_kb file to set a boolean.
> When this boolean is enabled, closing buffer_size_kb file will cause
> tracing to stop and free up the ring buffer memory.
> 
> The user space process can then open the buffer_size_kb file to set the
> new buffer size for tracing, enable the boolean through IOCTL and keep
> the file open. Under OOM condition, if the process gets killed, the
> kernel closes the file descriptor for buffer_size_kb. The release
> handler stops the tracing and releases the kernel memory automatically.
> 
> Signed-off-by: Vaibhav Nagarnaik <vnagarnaik@google.com>
> ---
>  include/linux/ftrace.h |    3 +
>  kernel/trace/trace.c   |  135 ++++++++++++++++++++++++++++++++++++------------
>  2 files changed, 105 insertions(+), 33 deletions(-)
> 
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index ca29e03..30c8a23 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -114,6 +114,9 @@ struct ftrace_func_command {
>  					char *params, int enable);
>  };
>  
> +/* enable/disable auto free ring buffer on file close */
> +#define TRACE_RINGBUF_FREE_ON_CLOSE _IOW('t', 0x01, int)
> +
>  #ifdef CONFIG_DYNAMIC_FTRACE
>  
>  int ftrace_arch_code_modify_prepare(void);
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index d38c16a..c676f17 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -2817,6 +2817,42 @@ static int tracing_resize_ring_buffer(unsigned long size)
>  	return ret;
>  }
>  
> +static ssize_t tracing_buffer_resize_atomic(unsigned long size)

Why call it "atomic"? Because you do the mutex? Anyway I hate the name.
It sounds like it can do it without preemption, which it clearly can't
do.

The rest looks fine to me.

-- Steve

> +{
> +	int cpu, ret = 0;
> +
> +	mutex_lock(&trace_types_lock);
> +
> +	tracing_stop();
> +
> +	/* disable all cpu buffers */
> +	for_each_tracing_cpu(cpu) {
> +		if (global_trace.data[cpu])
> +			atomic_inc(&global_trace.data[cpu]->disabled);
> +		if (max_tr.data[cpu])
> +			atomic_inc(&max_tr.data[cpu]->disabled);
> +	}
> +
> +	if (size != global_trace.entries)
> +		ret = tracing_resize_ring_buffer(size);
> +
> +	/* If check pages failed, return ENOMEM */
> +	if (tracing_disabled)
> +		ret = -ENOMEM;
> +
> +	for_each_tracing_cpu(cpu) {
> +		if (global_trace.data[cpu])
> +			atomic_dec(&global_trace.data[cpu]->disabled);
> +		if (max_tr.data[cpu])
> +			atomic_dec(&max_tr.data[cpu]->disabled);
> +	}
> +
> +	tracing_start();
> +	mutex_unlock(&trace_types_lock);
> +
> +	return ret;
> +}
> +
>  

Re: [PATCH] trace: Add a free on close control mechanism for buffer_size_kb

[Vaibhav Nagarnaik]

On Thu, Apr 28, 2011 at 4:32 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> Sorry, my mind has been elsewhere and I forgot about your patches.
>
> On Fri, 2011-04-22 at 15:46 -0700, Vaibhav Nagarnaik wrote:
>> The proc file entry buffer_size_kb is used to set the size of tracing
>> buffer. The memory to expand the buffer size is kernel memory. Consider
>> a use case where tracing is handled by a user space utility, which acts
>> as a gate keeper for tracing requests. In an OOM condition, tracing is
>> considered a low priority task and if the utility gets killed the ring
>> buffer memory cannot be released back to the kernel.
>>
>> This patch adds an IOCTL on the buffer_size_kb file to set a boolean.
>> When this boolean is enabled, closing buffer_size_kb file will cause
>> tracing to stop and free up the ring buffer memory.
>>
>> The user space process can then open the buffer_size_kb file to set the
>> new buffer size for tracing, enable the boolean through IOCTL and keep
>> the file open. Under OOM condition, if the process gets killed, the
>> kernel closes the file descriptor for buffer_size_kb. The release
>> handler stops the tracing and releases the kernel memory automatically.
>>
>> Signed-off-by: Vaibhav Nagarnaik <vnagarnaik@google.com>
>> ---
>>  include/linux/ftrace.h |    3 +
>>  kernel/trace/trace.c   |  135 ++++++++++++++++++++++++++++++++++++------------
>>  2 files changed, 105 insertions(+), 33 deletions(-)
>>
>> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
>> index ca29e03..30c8a23 100644
>> --- a/include/linux/ftrace.h
>> +++ b/include/linux/ftrace.h
>> @@ -114,6 +114,9 @@ struct ftrace_func_command {
>>                                       char *params, int enable);
>>  };
>>
>> +/* enable/disable auto free ring buffer on file close */
>> +#define TRACE_RINGBUF_FREE_ON_CLOSE _IOW('t', 0x01, int)
>> +
>>  #ifdef CONFIG_DYNAMIC_FTRACE
>>
>>  int ftrace_arch_code_modify_prepare(void);
>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> index d38c16a..c676f17 100644
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -2817,6 +2817,42 @@ static int tracing_resize_ring_buffer(unsigned long size)
>>       return ret;
>>  }
>>
>> +static ssize_t tracing_buffer_resize_atomic(unsigned long size)
>
> Why call it "atomic"? Because you do the mutex? Anyway I hate the name.
> It sounds like it can do it without preemption, which it clearly can't
> do.

I just wanted to note that it was a locked operation. I have changed the name
to tracing_buffer_resize_locked.

If you think it can be named better, I will change it.

>
> The rest looks fine to me.
>
Thanks for reviewing. I will send the updated patch.

> -- Steve
>
>> +{
>> +     int cpu, ret = 0;
>> +
>> +     mutex_lock(&trace_types_lock);
>> +
>> +     tracing_stop();
>> +
>> +     /* disable all cpu buffers */
>> +     for_each_tracing_cpu(cpu) {
>> +             if (global_trace.data[cpu])
>> +                     atomic_inc(&global_trace.data[cpu]->disabled);
>> +             if (max_tr.data[cpu])
>> +                     atomic_inc(&max_tr.data[cpu]->disabled);
>> +     }
>> +
>> +     if (size != global_trace.entries)
>> +             ret = tracing_resize_ring_buffer(size);
>> +
>> +     /* If check pages failed, return ENOMEM */
>> +     if (tracing_disabled)
>> +             ret = -ENOMEM;
>> +
>> +     for_each_tracing_cpu(cpu) {
>> +             if (global_trace.data[cpu])
>> +                     atomic_dec(&global_trace.data[cpu]->disabled);
>> +             if (max_tr.data[cpu])
>> +                     atomic_dec(&max_tr.data[cpu]->disabled);
>> +     }
>> +
>> +     tracing_start();
>> +     mutex_unlock(&trace_types_lock);
>> +
>> +     return ret;
>> +}
>> +
>>
>
>
>

Vaibhav Nagarnaik

[PATCH] trace: Add a free on close control mechanism for buffer_size_kb

[Vaibhav Nagarnaik]

The proc file entry buffer_size_kb is used to set the size of tracing
buffer. The memory to expand the buffer size is kernel memory. Consider
a use case where tracing is handled by a user space utility, which acts
as a gate keeper for tracing requests. In an OOM condition, tracing is
considered a low priority task and if the utility gets killed the ring
buffer memory cannot be released back to the kernel.

This patch adds an IOCTL on the buffer_size_kb file to set a boolean.
When this boolean is enabled, closing buffer_size_kb file will cause
tracing to stop and free up the ring buffer memory.

The user space process can then open the buffer_size_kb file to set the
new buffer size for tracing, enable the boolean through IOCTL and keep
the file open. Under OOM condition, if the process gets killed, the
kernel closes the file descriptor for buffer_size_kb. The release
handler stops the tracing and releases the kernel memory automatically.

Signed-off-by: Vaibhav Nagarnaik <vnagarnaik@google.com>
---
 include/linux/ftrace.h |    3 +
 kernel/trace/trace.c   |  135 ++++++++++++++++++++++++++++++++++++------------
 2 files changed, 105 insertions(+), 33 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index ca29e03..30c8a23 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -114,6 +114,9 @@ struct ftrace_func_command {
 					char *params, int enable);
 };
 
+/* enable/disable auto free ring buffer on file close */
+#define TRACE_RINGBUF_FREE_ON_CLOSE _IOW('t', 0x01, int)
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 
 int ftrace_arch_code_modify_prepare(void);
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index d38c16a..dcb0e2d 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2817,6 +2817,42 @@ static int tracing_resize_ring_buffer(unsigned long size)
 	return ret;
 }
 
+static ssize_t tracing_buffer_resize_locked(unsigned long size)
+{
+	int cpu, ret = 0;
+
+	mutex_lock(&trace_types_lock);
+
+	tracing_stop();
+
+	/* disable all cpu buffers */
+	for_each_tracing_cpu(cpu) {
+		if (global_trace.data[cpu])
+			atomic_inc(&global_trace.data[cpu]->disabled);
+		if (max_tr.data[cpu])
+			atomic_inc(&max_tr.data[cpu]->disabled);
+	}
+
+	if (size != global_trace.entries)
+		ret = tracing_resize_ring_buffer(size);
+
+	/* If check pages failed, return ENOMEM */
+	if (tracing_disabled)
+		ret = -ENOMEM;
+
+	for_each_tracing_cpu(cpu) {
+		if (global_trace.data[cpu])
+			atomic_dec(&global_trace.data[cpu]->disabled);
+		if (max_tr.data[cpu])
+			atomic_dec(&max_tr.data[cpu]->disabled);
+	}
+
+	tracing_start();
+	mutex_unlock(&trace_types_lock);
+
+	return ret;
+}
+
 
 /**
  * tracing_update_buffers - used by tracing facility to expand ring buffers
@@ -3399,11 +3435,37 @@ out_err:
 	goto out;
 }
 
+struct ftrace_entries_info {
+	struct trace_array	*tr;
+	int			free_buffer_on_close;
+};
+
+static int
+tracing_entries_open(struct inode *inode, struct file *filp)
+{
+	struct ftrace_entries_info *info;
+
+	if (tracing_disabled)
+		return -ENODEV;
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	info->tr = (struct trace_array *)inode->i_private;
+	info->free_buffer_on_close = 0;
+
+	filp->private_data = info;
+
+	return 0;
+}
+
 static ssize_t
 tracing_entries_read(struct file *filp, char __user *ubuf,
 		     size_t cnt, loff_t *ppos)
 {
-	struct trace_array *tr = filp->private_data;
+	struct ftrace_entries_info *info = filp->private_data;
+	struct trace_array *tr = info->tr;
 	char buf[96];
 	int r;
 
@@ -3425,7 +3487,7 @@ tracing_entries_write(struct file *filp, const char __user *ubuf,
 {
 	unsigned long val;
 	char buf[64];
-	int ret, cpu;
+	int ret;
 
 	if (cnt >= sizeof(buf))
 		return -EINVAL;
@@ -3443,46 +3505,50 @@ tracing_entries_write(struct file *filp, const char __user *ubuf,
 	if (!val)
 		return -EINVAL;
 
-	mutex_lock(&trace_types_lock);
+	/* value is in KB */
+	val <<= 10;
 
-	tracing_stop();
+	ret = tracing_buffer_resize_locked(val);
+	if (ret < 0)
+		return ret;
 
-	/* disable all cpu buffers */
-	for_each_tracing_cpu(cpu) {
-		if (global_trace.data[cpu])
-			atomic_inc(&global_trace.data[cpu]->disabled);
-		if (max_tr.data[cpu])
-			atomic_inc(&max_tr.data[cpu]->disabled);
-	}
+	*ppos += cnt;
 
-	/* value is in KB */
-	val <<= 10;
+	return cnt;
+}
 
-	if (val != global_trace.entries) {
-		ret = tracing_resize_ring_buffer(val);
-		if (ret < 0) {
-			cnt = ret;
-			goto out;
-		}
+static long
+tracing_entries_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+	int ret = -ENOIOCTLCMD;
+	struct ftrace_entries_info *info = filp->private_data;
+
+	switch (cmd) {
+	case TRACE_RINGBUF_FREE_ON_CLOSE: {
+		info->free_buffer_on_close = !!arg;
+		ret = 0;
+		break;
+	}
 	}
 
-	*ppos += cnt;
+	return ret;
+}
 
-	/* If check pages failed, return ENOMEM */
-	if (tracing_disabled)
-		cnt = -ENOMEM;
- out:
-	for_each_tracing_cpu(cpu) {
-		if (global_trace.data[cpu])
-			atomic_dec(&global_trace.data[cpu]->disabled);
-		if (max_tr.data[cpu])
-			atomic_dec(&max_tr.data[cpu]->disabled);
+static int
+tracing_entries_release(struct inode *inode, struct file *filp)
+{
+	struct ftrace_entries_info *info = filp->private_data;
+
+	if (info->free_buffer_on_close) {
+		/* disable tracing */
+		tracing_off();
+		/* resize the ring buffer to 0 */
+		tracing_buffer_resize_locked(0);
 	}
 
-	tracing_start();
-	mutex_unlock(&trace_types_lock);
+	kfree(info);
 
-	return cnt;
+	return 0;
 }
 
 static int mark_printk(const char *fmt, ...)
@@ -3624,9 +3690,12 @@ static const struct file_operations tracing_pipe_fops = {
 };
 
 static const struct file_operations tracing_entries_fops = {
-	.open		= tracing_open_generic,
+	.open		= tracing_entries_open,
 	.read		= tracing_entries_read,
 	.write		= tracing_entries_write,
+	.unlocked_ioctl	= tracing_entries_ioctl,
+	.compat_ioctl	= tracing_entries_ioctl,
+	.release	= tracing_entries_release,
 	.llseek		= generic_file_llseek,
 };
 
-- 
1.7.3.1

Re: [PATCH] trace: Add a free on close control mechanism for buffer_size_kb

[Steven Rostedt]

On Fri, 2011-04-29 at 12:45 -0700, Vaibhav Nagarnaik wrote:

> > Why call it "atomic"? Because you do the mutex? Anyway I hate the name.
> > It sounds like it can do it without preemption, which it clearly can't
> > do.
> 
> I just wanted to note that it was a locked operation. I have changed the name
> to tracing_buffer_resize_locked.
> 
> If you think it can be named better, I will change it.

Actually, the normal convention that I noticed that is used around the
kernel is the "normal" name is used when locking is applied. For
functions that do the same thing but does not lock, add two underscores
in front of the name:


void __foo(void)
{
	do_foo();
}

void foo(void)
{
	mutex_lock(&foo_lock);
	__foo();
	mutex_unlock(&foo_lock);
}

But if this has no real equivalent, just remove that name altogether.

Not to lock should be the exception, not locking.

-- Steve

Re: [PATCH] trace: Add a free on close control mechanism for buffer_size_kb

[Vaibhav Nagarnaik]

On Fri, Apr 29, 2011 at 1:01 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 2011-04-29 at 12:45 -0700, Vaibhav Nagarnaik wrote:
>
>> > Why call it "atomic"? Because you do the mutex? Anyway I hate the name.
>> > It sounds like it can do it without preemption, which it clearly can't
>> > do.
>>
>> I just wanted to note that it was a locked operation. I have changed the name
>> to tracing_buffer_resize_locked.
>>
>> If you think it can be named better, I will change it.
>
> Actually, the normal convention that I noticed that is used around the
> kernel is the "normal" name is used when locking is applied. For
> functions that do the same thing but does not lock, add two underscores
> in front of the name:
>
>
> void __foo(void)
> {
>        do_foo();
> }
>
> void foo(void)
> {
>        mutex_lock(&foo_lock);
>        __foo();
>        mutex_unlock(&foo_lock);
> }
>
> But if this has no real equivalent, just remove that name altogether.
>
> Not to lock should be the exception, not locking.

Ok. It makes sense. I now use __tracing_resize_ring_buffer to call the
unlocked version and tracing_resize_ring_buffer as a wrapper around it with
mutex locking.

I am sending the patch in a moment.

>
> -- Steve
>
>
>

Thanks

Vaibhav Nagarnaik

[PATCH] trace: Add a free on close control mechanism for buffer_size_kb

[Vaibhav Nagarnaik]

The proc file entry buffer_size_kb is used to set the size of tracing
buffer. The memory to expand the buffer size is kernel memory. Consider
a use case where tracing is handled by a user space utility, which acts
as a gate keeper for tracing requests. In an OOM condition, tracing is
considered a low priority task and if the utility gets killed the ring
buffer memory cannot be released back to the kernel.

This patch adds an IOCTL on the buffer_size_kb file to set a boolean.
When this boolean is enabled, closing buffer_size_kb file will cause
tracing to stop and free up the ring buffer memory.

The user space process can then open the buffer_size_kb file to set the
new buffer size for tracing, enable the boolean through IOCTL and keep
the file open. Under OOM condition, if the process gets killed, the
kernel closes the file descriptor for buffer_size_kb. The release
handler stops the tracing and releases the kernel memory automatically.

Signed-off-by: Vaibhav Nagarnaik <vnagarnaik@google.com>
---
 include/linux/ftrace.h |    3 +
 kernel/trace/trace.c   |  141 +++++++++++++++++++++++++++++++++++------------
 2 files changed, 108 insertions(+), 36 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index ca29e03..30c8a23 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -114,6 +114,9 @@ struct ftrace_func_command {
 					char *params, int enable);
 };
 
+/* enable/disable auto free ring buffer on file close */
+#define TRACE_RINGBUF_FREE_ON_CLOSE _IOW('t', 0x01, int)
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 
 int ftrace_arch_code_modify_prepare(void);
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index d38c16a..06f4458 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2765,7 +2765,7 @@ int tracer_init(struct tracer *t, struct trace_array *tr)
 	return t->init(tr);
 }
 
-static int tracing_resize_ring_buffer(unsigned long size)
+static int __tracing_resize_ring_buffer(unsigned long size)
 {
 	int ret;
 
@@ -2817,6 +2817,42 @@ static int tracing_resize_ring_buffer(unsigned long size)
 	return ret;
 }
 
+static ssize_t tracing_resize_ring_buffer(unsigned long size)
+{
+	int cpu, ret = 0;
+
+	mutex_lock(&trace_types_lock);
+
+	tracing_stop();
+
+	/* disable all cpu buffers */
+	for_each_tracing_cpu(cpu) {
+		if (global_trace.data[cpu])
+			atomic_inc(&global_trace.data[cpu]->disabled);
+		if (max_tr.data[cpu])
+			atomic_inc(&max_tr.data[cpu]->disabled);
+	}
+
+	if (size != global_trace.entries)
+		ret = __tracing_resize_ring_buffer(size);
+
+	/* If check pages failed, return ENOMEM */
+	if (tracing_disabled)
+		ret = -ENOMEM;
+
+	for_each_tracing_cpu(cpu) {
+		if (global_trace.data[cpu])
+			atomic_dec(&global_trace.data[cpu]->disabled);
+		if (max_tr.data[cpu])
+			atomic_dec(&max_tr.data[cpu]->disabled);
+	}
+
+	tracing_start();
+	mutex_unlock(&trace_types_lock);
+
+	return ret;
+}
+
 
 /**
  * tracing_update_buffers - used by tracing facility to expand ring buffers
@@ -2834,7 +2870,7 @@ int tracing_update_buffers(void)
 
 	mutex_lock(&trace_types_lock);
 	if (!ring_buffer_expanded)
-		ret = tracing_resize_ring_buffer(trace_buf_size);
+		ret = __tracing_resize_ring_buffer(trace_buf_size);
 	mutex_unlock(&trace_types_lock);
 
 	return ret;
@@ -2858,7 +2894,7 @@ static int tracing_set_tracer(const char *buf)
 	mutex_lock(&trace_types_lock);
 
 	if (!ring_buffer_expanded) {
-		ret = tracing_resize_ring_buffer(trace_buf_size);
+		ret = __tracing_resize_ring_buffer(trace_buf_size);
 		if (ret < 0)
 			goto out;
 		ret = 0;
@@ -3399,11 +3435,37 @@ out_err:
 	goto out;
 }
 
+struct ftrace_entries_info {
+	struct trace_array	*tr;
+	int			free_buffer_on_close;
+};
+
+static int
+tracing_entries_open(struct inode *inode, struct file *filp)
+{
+	struct ftrace_entries_info *info;
+
+	if (tracing_disabled)
+		return -ENODEV;
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	info->tr = (struct trace_array *)inode->i_private;
+	info->free_buffer_on_close = 0;
+
+	filp->private_data = info;
+
+	return 0;
+}
+
 static ssize_t
 tracing_entries_read(struct file *filp, char __user *ubuf,
 		     size_t cnt, loff_t *ppos)
 {
-	struct trace_array *tr = filp->private_data;
+	struct ftrace_entries_info *info = filp->private_data;
+	struct trace_array *tr = info->tr;
 	char buf[96];
 	int r;
 
@@ -3425,7 +3487,7 @@ tracing_entries_write(struct file *filp, const char __user *ubuf,
 {
 	unsigned long val;
 	char buf[64];
-	int ret, cpu;
+	int ret;
 
 	if (cnt >= sizeof(buf))
 		return -EINVAL;
@@ -3443,46 +3505,50 @@ tracing_entries_write(struct file *filp, const char __user *ubuf,
 	if (!val)
 		return -EINVAL;
 
-	mutex_lock(&trace_types_lock);
+	/* value is in KB */
+	val <<= 10;
 
-	tracing_stop();
+	ret = tracing_resize_ring_buffer(val);
+	if (ret < 0)
+		return ret;
 
-	/* disable all cpu buffers */
-	for_each_tracing_cpu(cpu) {
-		if (global_trace.data[cpu])
-			atomic_inc(&global_trace.data[cpu]->disabled);
-		if (max_tr.data[cpu])
-			atomic_inc(&max_tr.data[cpu]->disabled);
-	}
+	*ppos += cnt;
 
-	/* value is in KB */
-	val <<= 10;
+	return cnt;
+}
 
-	if (val != global_trace.entries) {
-		ret = tracing_resize_ring_buffer(val);
-		if (ret < 0) {
-			cnt = ret;
-			goto out;
-		}
+static long
+tracing_entries_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+	int ret = -ENOIOCTLCMD;
+	struct ftrace_entries_info *info = filp->private_data;
+
+	switch (cmd) {
+	case TRACE_RINGBUF_FREE_ON_CLOSE: {
+		info->free_buffer_on_close = !!arg;
+		ret = 0;
+		break;
+	}
 	}
 
-	*ppos += cnt;
+	return ret;
+}
 
-	/* If check pages failed, return ENOMEM */
-	if (tracing_disabled)
-		cnt = -ENOMEM;
- out:
-	for_each_tracing_cpu(cpu) {
-		if (global_trace.data[cpu])
-			atomic_dec(&global_trace.data[cpu]->disabled);
-		if (max_tr.data[cpu])
-			atomic_dec(&max_tr.data[cpu]->disabled);
+static int
+tracing_entries_release(struct inode *inode, struct file *filp)
+{
+	struct ftrace_entries_info *info = filp->private_data;
+
+	if (info->free_buffer_on_close) {
+		/* disable tracing */
+		tracing_off();
+		/* resize the ring buffer to 0 */
+		tracing_resize_ring_buffer(0);
 	}
 
-	tracing_start();
-	mutex_unlock(&trace_types_lock);
+	kfree(info);
 
-	return cnt;
+	return 0;
 }
 
 static int mark_printk(const char *fmt, ...)
@@ -3624,9 +3690,12 @@ static const struct file_operations tracing_pipe_fops = {
 };
 
 static const struct file_operations tracing_entries_fops = {
-	.open		= tracing_open_generic,
+	.open		= tracing_entries_open,
 	.read		= tracing_entries_read,
 	.write		= tracing_entries_write,
+	.unlocked_ioctl	= tracing_entries_ioctl,
+	.compat_ioctl	= tracing_entries_ioctl,
+	.release	= tracing_entries_release,
 	.llseek		= generic_file_llseek,
 };
 
-- 
1.7.3.1

Re: [PATCH] trace: Add a free on close control mechanism for buffer_size_kb

[Vaibhav Nagarnaik]

Hi Steven

Thanks for including my other patches in your tree. However, I did not
see this patch included in it.

Did you want any changes to it?


On Fri, Apr 29, 2011 at 4:16 PM, Vaibhav Nagarnaik
<vnagarnaik@google.com> wrote:
> The proc file entry buffer_size_kb is used to set the size of tracing
> buffer. The memory to expand the buffer size is kernel memory. Consider
> a use case where tracing is handled by a user space utility, which acts
> as a gate keeper for tracing requests. In an OOM condition, tracing is
> considered a low priority task and if the utility gets killed the ring
> buffer memory cannot be released back to the kernel.
>
> This patch adds an IOCTL on the buffer_size_kb file to set a boolean.
> When this boolean is enabled, closing buffer_size_kb file will cause
> tracing to stop and free up the ring buffer memory.
>
> The user space process can then open the buffer_size_kb file to set the
> new buffer size for tracing, enable the boolean through IOCTL and keep
> the file open. Under OOM condition, if the process gets killed, the
> kernel closes the file descriptor for buffer_size_kb. The release
> handler stops the tracing and releases the kernel memory automatically.
>
> Signed-off-by: Vaibhav Nagarnaik <vnagarnaik@google.com>
> ---
>  include/linux/ftrace.h |    3 +
>  kernel/trace/trace.c   |  141 +++++++++++++++++++++++++++++++++++------------
>  2 files changed, 108 insertions(+), 36 deletions(-)
>
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index ca29e03..30c8a23 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -114,6 +114,9 @@ struct ftrace_func_command {
>                                        char *params, int enable);
>  };
>
> +/* enable/disable auto free ring buffer on file close */
> +#define TRACE_RINGBUF_FREE_ON_CLOSE _IOW('t', 0x01, int)
> +
>  #ifdef CONFIG_DYNAMIC_FTRACE
>
>  int ftrace_arch_code_modify_prepare(void);
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index d38c16a..06f4458 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -2765,7 +2765,7 @@ int tracer_init(struct tracer *t, struct trace_array *tr)
>        return t->init(tr);
>  }
>
> -static int tracing_resize_ring_buffer(unsigned long size)
> +static int __tracing_resize_ring_buffer(unsigned long size)
>  {
>        int ret;
>
> @@ -2817,6 +2817,42 @@ static int tracing_resize_ring_buffer(unsigned long size)
>        return ret;
>  }
>
> +static ssize_t tracing_resize_ring_buffer(unsigned long size)
> +{
> +       int cpu, ret = 0;
> +
> +       mutex_lock(&trace_types_lock);
> +
> +       tracing_stop();
> +
> +       /* disable all cpu buffers */
> +       for_each_tracing_cpu(cpu) {
> +               if (global_trace.data[cpu])
> +                       atomic_inc(&global_trace.data[cpu]->disabled);
> +               if (max_tr.data[cpu])
> +                       atomic_inc(&max_tr.data[cpu]->disabled);
> +       }
> +
> +       if (size != global_trace.entries)
> +               ret = __tracing_resize_ring_buffer(size);
> +
> +       /* If check pages failed, return ENOMEM */
> +       if (tracing_disabled)
> +               ret = -ENOMEM;
> +
> +       for_each_tracing_cpu(cpu) {
> +               if (global_trace.data[cpu])
> +                       atomic_dec(&global_trace.data[cpu]->disabled);
> +               if (max_tr.data[cpu])
> +                       atomic_dec(&max_tr.data[cpu]->disabled);
> +       }
> +
> +       tracing_start();
> +       mutex_unlock(&trace_types_lock);
> +
> +       return ret;
> +}
> +
>
>  /**
>  * tracing_update_buffers - used by tracing facility to expand ring buffers
> @@ -2834,7 +2870,7 @@ int tracing_update_buffers(void)
>
>        mutex_lock(&trace_types_lock);
>        if (!ring_buffer_expanded)
> -               ret = tracing_resize_ring_buffer(trace_buf_size);
> +               ret = __tracing_resize_ring_buffer(trace_buf_size);
>        mutex_unlock(&trace_types_lock);
>
>        return ret;
> @@ -2858,7 +2894,7 @@ static int tracing_set_tracer(const char *buf)
>        mutex_lock(&trace_types_lock);
>
>        if (!ring_buffer_expanded) {
> -               ret = tracing_resize_ring_buffer(trace_buf_size);
> +               ret = __tracing_resize_ring_buffer(trace_buf_size);
>                if (ret < 0)
>                        goto out;
>                ret = 0;
> @@ -3399,11 +3435,37 @@ out_err:
>        goto out;
>  }
>
> +struct ftrace_entries_info {
> +       struct trace_array      *tr;
> +       int                     free_buffer_on_close;
> +};
> +
> +static int
> +tracing_entries_open(struct inode *inode, struct file *filp)
> +{
> +       struct ftrace_entries_info *info;
> +
> +       if (tracing_disabled)
> +               return -ENODEV;
> +
> +       info = kzalloc(sizeof(*info), GFP_KERNEL);
> +       if (!info)
> +               return -ENOMEM;
> +
> +       info->tr = (struct trace_array *)inode->i_private;
> +       info->free_buffer_on_close = 0;
> +
> +       filp->private_data = info;
> +
> +       return 0;
> +}
> +
>  static ssize_t
>  tracing_entries_read(struct file *filp, char __user *ubuf,
>                     size_t cnt, loff_t *ppos)
>  {
> -       struct trace_array *tr = filp->private_data;
> +       struct ftrace_entries_info *info = filp->private_data;
> +       struct trace_array *tr = info->tr;
>        char buf[96];
>        int r;
>
> @@ -3425,7 +3487,7 @@ tracing_entries_write(struct file *filp, const char __user *ubuf,
>  {
>        unsigned long val;
>        char buf[64];
> -       int ret, cpu;
> +       int ret;
>
>        if (cnt >= sizeof(buf))
>                return -EINVAL;
> @@ -3443,46 +3505,50 @@ tracing_entries_write(struct file *filp, const char __user *ubuf,
>        if (!val)
>                return -EINVAL;
>
> -       mutex_lock(&trace_types_lock);
> +       /* value is in KB */
> +       val <<= 10;
>
> -       tracing_stop();
> +       ret = tracing_resize_ring_buffer(val);
> +       if (ret < 0)
> +               return ret;
>
> -       /* disable all cpu buffers */
> -       for_each_tracing_cpu(cpu) {
> -               if (global_trace.data[cpu])
> -                       atomic_inc(&global_trace.data[cpu]->disabled);
> -               if (max_tr.data[cpu])
> -                       atomic_inc(&max_tr.data[cpu]->disabled);
> -       }
> +       *ppos += cnt;
>
> -       /* value is in KB */
> -       val <<= 10;
> +       return cnt;
> +}
>
> -       if (val != global_trace.entries) {
> -               ret = tracing_resize_ring_buffer(val);
> -               if (ret < 0) {
> -                       cnt = ret;
> -                       goto out;
> -               }
> +static long
> +tracing_entries_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +{
> +       int ret = -ENOIOCTLCMD;
> +       struct ftrace_entries_info *info = filp->private_data;
> +
> +       switch (cmd) {
> +       case TRACE_RINGBUF_FREE_ON_CLOSE: {
> +               info->free_buffer_on_close = !!arg;
> +               ret = 0;
> +               break;
> +       }
>        }
>
> -       *ppos += cnt;
> +       return ret;
> +}
>
> -       /* If check pages failed, return ENOMEM */
> -       if (tracing_disabled)
> -               cnt = -ENOMEM;
> - out:
> -       for_each_tracing_cpu(cpu) {
> -               if (global_trace.data[cpu])
> -                       atomic_dec(&global_trace.data[cpu]->disabled);
> -               if (max_tr.data[cpu])
> -                       atomic_dec(&max_tr.data[cpu]->disabled);
> +static int
> +tracing_entries_release(struct inode *inode, struct file *filp)
> +{
> +       struct ftrace_entries_info *info = filp->private_data;
> +
> +       if (info->free_buffer_on_close) {
> +               /* disable tracing */
> +               tracing_off();
> +               /* resize the ring buffer to 0 */
> +               tracing_resize_ring_buffer(0);
>        }
>
> -       tracing_start();
> -       mutex_unlock(&trace_types_lock);
> +       kfree(info);
>
> -       return cnt;
> +       return 0;
>  }
>
>  static int mark_printk(const char *fmt, ...)
> @@ -3624,9 +3690,12 @@ static const struct file_operations tracing_pipe_fops = {
>  };
>
>  static const struct file_operations tracing_entries_fops = {
> -       .open           = tracing_open_generic,
> +       .open           = tracing_entries_open,
>        .read           = tracing_entries_read,
>        .write          = tracing_entries_write,
> +       .unlocked_ioctl = tracing_entries_ioctl,
> +       .compat_ioctl   = tracing_entries_ioctl,
> +       .release        = tracing_entries_release,
>        .llseek         = generic_file_llseek,
>  };
>
> --
> 1.7.3.1
>
>



Vaibhav Nagarnaik

Re: [PATCH] trace: Add a free on close control mechanism for buffer_size_kb

[Steven Rostedt]

On Tue, 2011-05-24 at 11:37 -0700, Vaibhav Nagarnaik wrote:
> Hi Steven
> 
> Thanks for including my other patches in your tree. However, I did not
> see this patch included in it.
> 
> Did you want any changes to it?

There was a reason I didn't include this patch, but I don't remember
what it was ;)

I think I had to look at it deeper, but got too wrapped up in other
things, where as your other patches were easier to accept.

I'll try to get time to look at this patch in more depth again, and if
there's nothing wrong with it, I'll queue it up for my 2.6.41/2.8.1/3.1
queue.

It's too late to push it for this merge window.

Thanks,

-- Steve

Re: [PATCH] trace: Add a free on close control mechanism for buffer_size_kb

[Steven Rostedt]

On Fri, 2011-04-29 at 16:16 -0700, Vaibhav Nagarnaik wrote:

> +static ssize_t tracing_resize_ring_buffer(unsigned long size)
> +{
> +	int cpu, ret = 0;
> +
> +	mutex_lock(&trace_types_lock);
> +
> +	tracing_stop();
> +
> +	/* disable all cpu buffers */
> +	for_each_tracing_cpu(cpu) {
> +		if (global_trace.data[cpu])
> +			atomic_inc(&global_trace.data[cpu]->disabled);
> +		if (max_tr.data[cpu])
> +			atomic_inc(&max_tr.data[cpu]->disabled);
> +	}
> +
> +	if (size != global_trace.entries)
> +		ret = __tracing_resize_ring_buffer(size);
> +
> +	/* If check pages failed, return ENOMEM */
> +	if (tracing_disabled)
> +		ret = -ENOMEM;

This is incorrect. tracing_disabled is set from
__tracing_resize_ring_buffer() if the max buffer can't become the same
size as the global buffer.

What you should do is just check if ret is less than zero here.

	if (ret < 0)
		ret = -ENOMEM;

-- Steve


> +
> +	for_each_tracing_cpu(cpu) {
> +		if (global_trace.data[cpu])
> +			atomic_dec(&global_trace.data[cpu]->disabled);
> +		if (max_tr.data[cpu])
> +			atomic_dec(&max_tr.data[cpu]->disabled);
> +	}
> +
> +	tracing_start();
> +	mutex_unlock(&trace_types_lock);
> +
> +	return ret;
> +}
> +
>  

Re: [PATCH] trace: Add a free on close control mechanism for buffer_size_kb

[Vaibhav Nagarnaik]

On Wed, May 25, 2011 at 8:29 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 2011-04-29 at 16:16 -0700, Vaibhav Nagarnaik wrote:
>
>> +static ssize_t tracing_resize_ring_buffer(unsigned long size)
>> +{
>> +     int cpu, ret = 0;
>> +
>> +     mutex_lock(&trace_types_lock);
>> +
>> +     tracing_stop();
>> +
>> +     /* disable all cpu buffers */
>> +     for_each_tracing_cpu(cpu) {
>> +             if (global_trace.data[cpu])
>> +                     atomic_inc(&global_trace.data[cpu]->disabled);
>> +             if (max_tr.data[cpu])
>> +                     atomic_inc(&max_tr.data[cpu]->disabled);
>> +     }
>> +
>> +     if (size != global_trace.entries)
>> +             ret = __tracing_resize_ring_buffer(size);
>> +
>> +     /* If check pages failed, return ENOMEM */
>> +     if (tracing_disabled)
>> +             ret = -ENOMEM;
>
> This is incorrect. tracing_disabled is set from
> __tracing_resize_ring_buffer() if the max buffer can't become the same
> size as the global buffer.

That's right. Thanks for catching it, I didn't see that as I was trying
to follow the logic in tracing_entries_write() function.

>
> What you should do is just check if ret is less than zero here.
>
>        if (ret < 0)
>                ret = -ENOMEM;

Instead, I don't have to check/set the return value. I can just return
"ret" to the caller.

>
> -- Steve
>
>
>> +
>> +     for_each_tracing_cpu(cpu) {
>> +             if (global_trace.data[cpu])
>> +                     atomic_dec(&global_trace.data[cpu]->disabled);
>> +             if (max_tr.data[cpu])
>> +                     atomic_dec(&max_tr.data[cpu]->disabled);
>> +     }
>> +
>> +     tracing_start();
>> +     mutex_unlock(&trace_types_lock);
>> +
>> +     return ret;
>> +}
>> +
>>
>
>


Vaibhav Nagarnaik

[PATCH v2] trace: Add a free on close control mechanism for buffer_size_kb

[Vaibhav Nagarnaik]

The proc file entry buffer_size_kb is used to set the size of tracing
buffer. The memory to expand the buffer size is kernel memory. Consider
a use case where tracing is handled by a user space utility, which acts
as a gate keeper for tracing requests. In an OOM condition, tracing is
considered a low priority task and if the utility gets killed the ring
buffer memory cannot be released back to the kernel.

This patch adds an IOCTL on the buffer_size_kb file to set a boolean.
When this boolean is enabled, closing buffer_size_kb file will cause
tracing to stop and free up the ring buffer memory.

The user space process can then open the buffer_size_kb file to set the
new buffer size for tracing, enable the boolean through IOCTL and keep
the file open. Under OOM condition, if the process gets killed, the
kernel closes the file descriptor for buffer_size_kb. The release
handler stops the tracing and releases the kernel memory automatically.

Signed-off-by: Vaibhav Nagarnaik <vnagarnaik@google.com>
---
Changelog:
v2-v1
* Fixed the return value according to comments

 include/linux/ftrace.h |    3 +
 kernel/trace/trace.c   |  137 +++++++++++++++++++++++++++++++++++-------------
 2 files changed, 104 insertions(+), 36 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 9d88e1c..72a2f0d 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -127,6 +127,9 @@ struct ftrace_func_command {
 					char *params, int enable);
 };
 
+/* enable/disable auto free ring buffer on file close */
+#define TRACE_RINGBUF_FREE_ON_CLOSE _IOW('t', 0x01, int)
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 
 int ftrace_arch_code_modify_prepare(void);
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index ee9c921..6335e75 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2767,7 +2767,7 @@ int tracer_init(struct tracer *t, struct trace_array *tr)
 	return t->init(tr);
 }
 
-static int tracing_resize_ring_buffer(unsigned long size)
+static int __tracing_resize_ring_buffer(unsigned long size)
 {
 	int ret;
 
@@ -2819,6 +2819,38 @@ static int tracing_resize_ring_buffer(unsigned long size)
 	return ret;
 }
 
+static ssize_t tracing_resize_ring_buffer(unsigned long size)
+{
+	int cpu, ret = size;
+
+	mutex_lock(&trace_types_lock);
+
+	tracing_stop();
+
+	/* disable all cpu buffers */
+	for_each_tracing_cpu(cpu) {
+		if (global_trace.data[cpu])
+			atomic_inc(&global_trace.data[cpu]->disabled);
+		if (max_tr.data[cpu])
+			atomic_inc(&max_tr.data[cpu]->disabled);
+	}
+
+	if (size != global_trace.entries)
+		ret = __tracing_resize_ring_buffer(size);
+
+	for_each_tracing_cpu(cpu) {
+		if (global_trace.data[cpu])
+			atomic_dec(&global_trace.data[cpu]->disabled);
+		if (max_tr.data[cpu])
+			atomic_dec(&max_tr.data[cpu]->disabled);
+	}
+
+	tracing_start();
+	mutex_unlock(&trace_types_lock);
+
+	return ret;
+}
+
 
 /**
  * tracing_update_buffers - used by tracing facility to expand ring buffers
@@ -2836,7 +2868,7 @@ int tracing_update_buffers(void)
 
 	mutex_lock(&trace_types_lock);
 	if (!ring_buffer_expanded)
-		ret = tracing_resize_ring_buffer(trace_buf_size);
+		ret = __tracing_resize_ring_buffer(trace_buf_size);
 	mutex_unlock(&trace_types_lock);
 
 	return ret;
@@ -2860,7 +2892,7 @@ static int tracing_set_tracer(const char *buf)
 	mutex_lock(&trace_types_lock);
 
 	if (!ring_buffer_expanded) {
-		ret = tracing_resize_ring_buffer(trace_buf_size);
+		ret = __tracing_resize_ring_buffer(trace_buf_size);
 		if (ret < 0)
 			goto out;
 		ret = 0;
@@ -3409,11 +3441,37 @@ out_err:
 	goto out;
 }
 
+struct ftrace_entries_info {
+	struct trace_array	*tr;
+	int			free_buffer_on_close;
+};
+
+static int
+tracing_entries_open(struct inode *inode, struct file *filp)
+{
+	struct ftrace_entries_info *info;
+
+	if (tracing_disabled)
+		return -ENODEV;
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	info->tr = (struct trace_array *)inode->i_private;
+	info->free_buffer_on_close = 0;
+
+	filp->private_data = info;
+
+	return 0;
+}
+
 static ssize_t
 tracing_entries_read(struct file *filp, char __user *ubuf,
 		     size_t cnt, loff_t *ppos)
 {
-	struct trace_array *tr = filp->private_data;
+	struct ftrace_entries_info *info = filp->private_data;
+	struct trace_array *tr = info->tr;
 	char buf[96];
 	int r;
 
@@ -3435,7 +3493,7 @@ tracing_entries_write(struct file *filp, const char __user *ubuf,
 {
 	unsigned long val;
 	char buf[64];
-	int ret, cpu;
+	int ret;
 
 	if (cnt >= sizeof(buf))
 		return -EINVAL;
@@ -3453,46 +3511,50 @@ tracing_entries_write(struct file *filp, const char __user *ubuf,
 	if (!val)
 		return -EINVAL;
 
-	mutex_lock(&trace_types_lock);
+	/* value is in KB */
+	val <<= 10;
 
-	tracing_stop();
+	ret = tracing_resize_ring_buffer(val);
+	if (ret < 0)
+		return ret;
 
-	/* disable all cpu buffers */
-	for_each_tracing_cpu(cpu) {
-		if (global_trace.data[cpu])
-			atomic_inc(&global_trace.data[cpu]->disabled);
-		if (max_tr.data[cpu])
-			atomic_inc(&max_tr.data[cpu]->disabled);
-	}
+	*ppos += cnt;
 
-	/* value is in KB */
-	val <<= 10;
+	return cnt;
+}
 
-	if (val != global_trace.entries) {
-		ret = tracing_resize_ring_buffer(val);
-		if (ret < 0) {
-			cnt = ret;
-			goto out;
-		}
+static long
+tracing_entries_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+	int ret = -ENOIOCTLCMD;
+	struct ftrace_entries_info *info = filp->private_data;
+
+	switch (cmd) {
+	case TRACE_RINGBUF_FREE_ON_CLOSE: {
+		info->free_buffer_on_close = !!arg;
+		ret = 0;
+		break;
+	}
 	}
 
-	*ppos += cnt;
+	return ret;
+}
 
-	/* If check pages failed, return ENOMEM */
-	if (tracing_disabled)
-		cnt = -ENOMEM;
- out:
-	for_each_tracing_cpu(cpu) {
-		if (global_trace.data[cpu])
-			atomic_dec(&global_trace.data[cpu]->disabled);
-		if (max_tr.data[cpu])
-			atomic_dec(&max_tr.data[cpu]->disabled);
+static int
+tracing_entries_release(struct inode *inode, struct file *filp)
+{
+	struct ftrace_entries_info *info = filp->private_data;
+
+	if (info->free_buffer_on_close) {
+		/* disable tracing */
+		tracing_off();
+		/* resize the ring buffer to 0 */
+		tracing_resize_ring_buffer(0);
 	}
 
-	tracing_start();
-	mutex_unlock(&trace_types_lock);
+	kfree(info);
 
-	return cnt;
+	return 0;
 }
 
 static int mark_printk(const char *fmt, ...)
@@ -3634,9 +3696,12 @@ static const struct file_operations tracing_pipe_fops = {
 };
 
 static const struct file_operations tracing_entries_fops = {
-	.open		= tracing_open_generic,
+	.open		= tracing_entries_open,
 	.read		= tracing_entries_read,
 	.write		= tracing_entries_write,
+	.unlocked_ioctl	= tracing_entries_ioctl,
+	.compat_ioctl	= tracing_entries_ioctl,
+	.release	= tracing_entries_release,
 	.llseek		= generic_file_llseek,
 };
 
-- 
1.7.3.1

Re: [PATCH] trace: Add a free on close control mechanism for buffer_size_kb

[Steven Rostedt]

On Thu, 2011-05-26 at 11:00 -0700, Vaibhav Nagarnaik wrote:
> On Wed, May 25, 2011 at 8:29 PM, Steven Rostedt <rostedt@goodmis.org> wrote:

> >
> > What you should do is just check if ret is less than zero here.
> >
> >        if (ret < 0)
> >                ret = -ENOMEM;
> 
> Instead, I don't have to check/set the return value. I can just return
> "ret" to the caller.

Actually, ret could be -1, if we end up with a miss match in global vs
max buffers. But that is more of an anomaly. Maybe the solution there is
to have that return -ENOMEM as well?

-- Steve

Re: [PATCH] trace: Add a free on close control mechanism for buffer_size_kb

[Vaibhav Nagarnaik]

On Thu, May 26, 2011 at 11:23 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu, 2011-05-26 at 11:00 -0700, Vaibhav Nagarnaik wrote:
>> On Wed, May 25, 2011 at 8:29 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>
>> >
>> > What you should do is just check if ret is less than zero here.
>> >
>> >        if (ret < 0)
>> >                ret = -ENOMEM;
>>
>> Instead, I don't have to check/set the return value. I can just return
>> "ret" to the caller.
>
> Actually, ret could be -1, if we end up with a miss match in global vs
> max buffers. But that is more of an anomaly. Maybe the solution there is
> to have that return -ENOMEM as well?

Sure, I missed that. Corrected patch coming up!


>
> -- Steve
>
>
>

Vaibhav Nagarnaik

[PATCH v2] trace: Add a free on close control mechanism for buffer_size_kb

[Vaibhav Nagarnaik]

The proc file entry buffer_size_kb is used to set the size of tracing
buffer. The memory to expand the buffer size is kernel memory. Consider
a use case where tracing is handled by a user space utility, which acts
as a gate keeper for tracing requests. In an OOM condition, tracing is
considered a low priority task and if the utility gets killed the ring
buffer memory cannot be released back to the kernel.

This patch adds an IOCTL on the buffer_size_kb file to set a boolean.
When this boolean is enabled, closing buffer_size_kb file will cause
tracing to stop and free up the ring buffer memory.

The user space process can then open the buffer_size_kb file to set the
new buffer size for tracing, enable the boolean through IOCTL and keep
the file open. Under OOM condition, if the process gets killed, the
kernel closes the file descriptor for buffer_size_kb. The release
handler stops the tracing and releases the kernel memory automatically.

Signed-off-by: Vaibhav Nagarnaik <vnagarnaik@google.com>
---
Changelog:
v2-v1
* Fixed the return value in tracing_resize_ring_buffer()

 include/linux/ftrace.h |    3 +
 kernel/trace/trace.c   |  140 +++++++++++++++++++++++++++++++++++------------
 2 files changed, 107 insertions(+), 36 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 9d88e1c..72a2f0d 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -127,6 +127,9 @@ struct ftrace_func_command {
 					char *params, int enable);
 };
 
+/* enable/disable auto free ring buffer on file close */
+#define TRACE_RINGBUF_FREE_ON_CLOSE _IOW('t', 0x01, int)
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 
 int ftrace_arch_code_modify_prepare(void);
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index ee9c921..0824f55 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2767,7 +2767,7 @@ int tracer_init(struct tracer *t, struct trace_array *tr)
 	return t->init(tr);
 }
 
-static int tracing_resize_ring_buffer(unsigned long size)
+static int __tracing_resize_ring_buffer(unsigned long size)
 {
 	int ret;
 
@@ -2819,6 +2819,41 @@ static int tracing_resize_ring_buffer(unsigned long size)
 	return ret;
 }
 
+static ssize_t tracing_resize_ring_buffer(unsigned long size)
+{
+	int cpu, ret = size;
+
+	mutex_lock(&trace_types_lock);
+
+	tracing_stop();
+
+	/* disable all cpu buffers */
+	for_each_tracing_cpu(cpu) {
+		if (global_trace.data[cpu])
+			atomic_inc(&global_trace.data[cpu]->disabled);
+		if (max_tr.data[cpu])
+			atomic_inc(&max_tr.data[cpu]->disabled);
+	}
+
+	if (size != global_trace.entries)
+		ret = __tracing_resize_ring_buffer(size);
+
+	if (ret < 0)
+		ret = -ENOMEM;
+
+	for_each_tracing_cpu(cpu) {
+		if (global_trace.data[cpu])
+			atomic_dec(&global_trace.data[cpu]->disabled);
+		if (max_tr.data[cpu])
+			atomic_dec(&max_tr.data[cpu]->disabled);
+	}
+
+	tracing_start();
+	mutex_unlock(&trace_types_lock);
+
+	return ret;
+}
+
 
 /**
  * tracing_update_buffers - used by tracing facility to expand ring buffers
@@ -2836,7 +2871,7 @@ int tracing_update_buffers(void)
 
 	mutex_lock(&trace_types_lock);
 	if (!ring_buffer_expanded)
-		ret = tracing_resize_ring_buffer(trace_buf_size);
+		ret = __tracing_resize_ring_buffer(trace_buf_size);
 	mutex_unlock(&trace_types_lock);
 
 	return ret;
@@ -2860,7 +2895,7 @@ static int tracing_set_tracer(const char *buf)
 	mutex_lock(&trace_types_lock);
 
 	if (!ring_buffer_expanded) {
-		ret = tracing_resize_ring_buffer(trace_buf_size);
+		ret = __tracing_resize_ring_buffer(trace_buf_size);
 		if (ret < 0)
 			goto out;
 		ret = 0;
@@ -3409,11 +3444,37 @@ out_err:
 	goto out;
 }
 
+struct ftrace_entries_info {
+	struct trace_array	*tr;
+	int			free_buffer_on_close;
+};
+
+static int
+tracing_entries_open(struct inode *inode, struct file *filp)
+{
+	struct ftrace_entries_info *info;
+
+	if (tracing_disabled)
+		return -ENODEV;
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	info->tr = (struct trace_array *)inode->i_private;
+	info->free_buffer_on_close = 0;
+
+	filp->private_data = info;
+
+	return 0;
+}
+
 static ssize_t
 tracing_entries_read(struct file *filp, char __user *ubuf,
 		     size_t cnt, loff_t *ppos)
 {
-	struct trace_array *tr = filp->private_data;
+	struct ftrace_entries_info *info = filp->private_data;
+	struct trace_array *tr = info->tr;
 	char buf[96];
 	int r;
 
@@ -3435,7 +3496,7 @@ tracing_entries_write(struct file *filp, const char __user *ubuf,
 {
 	unsigned long val;
 	char buf[64];
-	int ret, cpu;
+	int ret;
 
 	if (cnt >= sizeof(buf))
 		return -EINVAL;
@@ -3453,46 +3514,50 @@ tracing_entries_write(struct file *filp, const char __user *ubuf,
 	if (!val)
 		return -EINVAL;
 
-	mutex_lock(&trace_types_lock);
+	/* value is in KB */
+	val <<= 10;
 
-	tracing_stop();
+	ret = tracing_resize_ring_buffer(val);
+	if (ret < 0)
+		return ret;
 
-	/* disable all cpu buffers */
-	for_each_tracing_cpu(cpu) {
-		if (global_trace.data[cpu])
-			atomic_inc(&global_trace.data[cpu]->disabled);
-		if (max_tr.data[cpu])
-			atomic_inc(&max_tr.data[cpu]->disabled);
-	}
+	*ppos += cnt;
 
-	/* value is in KB */
-	val <<= 10;
+	return cnt;
+}
 
-	if (val != global_trace.entries) {
-		ret = tracing_resize_ring_buffer(val);
-		if (ret < 0) {
-			cnt = ret;
-			goto out;
-		}
+static long
+tracing_entries_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+	int ret = -ENOIOCTLCMD;
+	struct ftrace_entries_info *info = filp->private_data;
+
+	switch (cmd) {
+	case TRACE_RINGBUF_FREE_ON_CLOSE: {
+		info->free_buffer_on_close = !!arg;
+		ret = 0;
+		break;
+	}
 	}
 
-	*ppos += cnt;
+	return ret;
+}
 
-	/* If check pages failed, return ENOMEM */
-	if (tracing_disabled)
-		cnt = -ENOMEM;
- out:
-	for_each_tracing_cpu(cpu) {
-		if (global_trace.data[cpu])
-			atomic_dec(&global_trace.data[cpu]->disabled);
-		if (max_tr.data[cpu])
-			atomic_dec(&max_tr.data[cpu]->disabled);
+static int
+tracing_entries_release(struct inode *inode, struct file *filp)
+{
+	struct ftrace_entries_info *info = filp->private_data;
+
+	if (info->free_buffer_on_close) {
+		/* disable tracing */
+		tracing_off();
+		/* resize the ring buffer to 0 */
+		tracing_resize_ring_buffer(0);
 	}
 
-	tracing_start();
-	mutex_unlock(&trace_types_lock);
+	kfree(info);
 
-	return cnt;
+	return 0;
 }
 
 static int mark_printk(const char *fmt, ...)
@@ -3634,9 +3699,12 @@ static const struct file_operations tracing_pipe_fops = {
 };
 
 static const struct file_operations tracing_entries_fops = {
-	.open		= tracing_open_generic,
+	.open		= tracing_entries_open,
 	.read		= tracing_entries_read,
 	.write		= tracing_entries_write,
+	.unlocked_ioctl	= tracing_entries_ioctl,
+	.compat_ioctl	= tracing_entries_ioctl,
+	.release	= tracing_entries_release,
 	.llseek		= generic_file_llseek,
 };
 
-- 
1.7.3.1