Messages in this thread Patch in this message | | | Subject | Re: [PATCH v3 -tip 2/4] tracing: replace static old_tracer with strcmp | From | Steven Rostedt <> | Date | Thu, 20 Dec 2012 22:04:22 -0500 |
| |
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:
--- 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)) {
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. */
-- Steve
> - } > mutex_unlock(&trace_types_lock); > > atomic_inc(&trace_record_cmdline_disabled); > @@ -3481,7 +3479,6 @@ tracing_read_pipe(struct file *filp, char __user *ubuf, > size_t cnt, loff_t *ppos) > { > struct trace_iterator *iter = filp->private_data; > - static struct tracer *old_tracer; > ssize_t sret; > > /* return any leftover data */ > @@ -3493,10 +3490,9 @@ tracing_read_pipe(struct file *filp, char __user *ubuf, > > /* 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; > - } > mutex_unlock(&trace_types_lock); > > /* > @@ -3652,7 +3648,6 @@ static ssize_t tracing_splice_read_pipe(struct file *filp, > .ops = &tracing_pipe_buf_ops, > .spd_release = tracing_spd_release_pipe, > }; > - static struct tracer *old_tracer; > ssize_t ret; > size_t rem; > unsigned int i; > @@ -3662,10 +3657,9 @@ static ssize_t tracing_splice_read_pipe(struct file *filp, > > /* 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; > - } > mutex_unlock(&trace_types_lock); > > mutex_lock(&iter->mutex);
| |