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@xxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/