Re: [PATCH v3 -tip 2/4] tracing: replace static old_tracer with strcmp

[Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>]

Hi,

(12/21/2012 12:04 PM), Steven Rostedt wrote:
> On Wed, 2012-12-19 at 16:02 +0900, Hiraku Toyooka wrote:
>> Currently, read functions for trace buffer use static "old_tracer"
>> for detecting changes of current tracer. This is because we can
>> assume that these functions are used from only one file ("trace").
>>
>> But we are adding snapshot feature for ftrace, then those functions
>> are called from two files. So we remove all static "old_tracer", and
>> replace those with string comparison between current and previous
>> tracers.
>
> I reworded what you wrote:
>

Thank you very much.

> ---
> Currently the trace buffer read functions use a static variable
> "old_tracer" for detecting if the current tracer changes. This was
> suitable for a single trace file ("trace"), but to add a snapshot
> feature that will use the same function for its file, a check against
> a static variable is not sufficient.
>
> To use the output functions for two different files, instead of storing
> the current tracer in a static variable, as the trace iterator
> descriptor
> contains a pointer to the original current tracer's name, that pointer
> can now be used to check if the current tracer has changed between
> different reads of the trace file.
> ---
>
> And I also realized a more efficient approach.
>
>>
>> Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Cc: Frederic Weisbecker <fweisbec@gmail.com>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>   kernel/trace/trace.c |   18 ++++++------------
>>   1 file changed, 6 insertions(+), 12 deletions(-)
>>
>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> index a8ce008..8d05a44 100644
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -1948,7 +1948,6 @@ void tracing_iter_reset(struct trace_iterator *iter, int cpu)
>>   static void *s_start(struct seq_file *m, loff_t *pos)
>>   {
>>   	struct trace_iterator *iter = m->private;
>> -	static struct tracer *old_tracer;
>>   	int cpu_file = iter->cpu_file;
>>   	void *p = NULL;
>>   	loff_t l = 0;
>> @@ -1956,10 +1955,9 @@ static void *s_start(struct seq_file *m, loff_t *pos)
>>
>>   	/* copy the tracer to avoid using a global lock all around */
>>   	mutex_lock(&trace_types_lock);
>> -	if (unlikely(old_tracer != current_trace && current_trace)) {
>> -		old_tracer = current_trace;
>> +	if (unlikely(current_trace &&
>> +		     strcmp(iter->trace->name, current_trace->name) != 0))
>>   		*iter->trace = *current_trace;
>
> As iter->trace is a copy of current_trace, it points to everything that
> the current_trace pointed to. As the current_trace->name is a pointer to
> a const char string that never changes, we don't need to do the strcmp()
> you can simply do a direct comparison:
>
> 	if (unlikely(current_trace && iter->trace->name != current_trace->name)) {
>

Yes. I'll update my patch to use this efficient way.

> I would add a comment about that too:
> 	/*
> 	 * iter->trace is a copy of current_trace, the pointer to the
> 	 * name may be used instead of a strcmp(), as iter->trace->name
> 	 * will point to the same string as current_trace->name.
> 	 */

OK. I'll include this comment in my patch.


Thanks,
Hiraku Toyooka