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

[Vaibhav Nagarnaik <vnagarnaik@google.com>]

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