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

[Steven Rostedt <rostedt@goodmis.org>]

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);