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...@hitachi.com>
> Cc: Steven Rostedt <rost...@goodmis.org>
> Cc: Frederic Weisbecker <fweis...@gmail.com>
> Cc: Ingo Molnar <mi...@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);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
