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

[Hiraku Toyooka]

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@xxxxxxxxxxx>
> > Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> > Cc: Frederic Weisbecker <fweisbec@xxxxxxxxx>
> > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > ---
> >   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

--
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/