Messages in this thread | | | Date | Fri, 21 Dec 2012 19:55:56 +0900 | From | Hiraku Toyooka <> | Subject | Re: [PATCH v3 -tip 2/4] tracing: replace static old_tracer with strcmp |
| |
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
| |