Re: [PATCH] tracing: Fix race of function probes counting

[Steven Rostedt]

On Thu, 20 Nov 2014 20:45:09 +0900
Masami Hiramatsu <masami.hiramatsu.pt@xxxxxxxxxxx> wrote:


> > Signed-off-by: Steven Rostedt <rostedt@xxxxxxxxxxx>
> 
> I have found a couple of typos, but basically, it looks OK.
> 
> Reviewed-by: Masami Hiramatsu <masami.hiramatsu.pt@xxxxxxxxxxx>

Grumble. I already pushed this to next so it's in my "no rebase" state.

But I can add another patch to do these updates.

> 
> > ---
> >  kernel/trace/trace_functions.c | 117 +++++++++++++++++++++++++++++++++--------
> >  1 file changed, 96 insertions(+), 21 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
> > index a8e0c7666164..973db52eb070 100644
> > --- a/kernel/trace/trace_functions.c
> > +++ b/kernel/trace/trace_functions.c
> > @@ -261,37 +261,74 @@ static struct tracer function_trace __tracer_data =
> >  };
> >  
> >  #ifdef CONFIG_DYNAMIC_FTRACE
> > -static int update_count(void **data)
> > +static void update_traceon_count(void **data, int on)
> 
> btw, why don't you use bool instead of int?

Because I didn't think of it ;-)

Yeah, bool would be better.

> 
> >  {
> > -	unsigned long *count = (long *)data;
> > +	long *count = (long *)data;
> > +	long old_count = *count;
> >  
> > -	if (!*count)
> > -		return 0;
> > +	/*
> > +	 * Tracing gets disabled (or enabled) once per count.
> > +	 * This function can be called at the same time on mulitple CPUs.
>                                                            ^^^^^^^^ multiple

You know, that is probably the biggest typo I make. My fingers do not
like to hit 't' before 'i' when I type "multiple". In fact, it did it
just then (I had to go back and correct it)!

> 
> > +	 * It is fine if both disable (or enable) tracing, as disabling
> > +	 * (or enabling) the second time doesn't do anything as the
> > +	 * state of the tracer is already disabled (or enabled).
> > +	 * What needs to be synchronized in this case is that the count
> > +	 * only gets decremented once, even if the tracer is disabled
> > +	 * (or enabled) twice, as the second one is really a nop.
> > +	 *
> > +	 * The memory barriers guarantee that we only decrement the
> > +	 * counter once. First the count is read to a local variable
> > +	 * and a read barrier is used to make sure that it is loaded
> > +	 * before checking if the tracer is in the state we want.
> > +	 * If the tracer is not in the state we want, then the count
> > +	 * is guaranteed to be the old count.
> > +	 *
> > +	 * Next the tracer is set to the state we want (disabled or enabled)
> > +	 * then a write memory barrier is used to make sure that
> > +	 * the new state is visible before changing the counter by
> > +	 * one minus the old counter. This guarantees that another CPU
> > +	 * executing this code will see the new state before seeing
> > +	 * the new counter value, and would not do anthing if the new
>                                                    ^^^^^^^anything?

OK, will fix.

Thanks,

-- Steve

> 
> > +	 * counter is seen.
> > +	 *
> > +	 * Note, there is no synchronization between this and a user
> > +	 * setting the tracing_on file. But we currently don't care
> > +	 * about that.
> > +	 */
> > +	if (!old_count)
> > +		return;
> >  
> > -	if (*count != -1)
> > -		(*count)--;
> > +	/* Make sure we see count before checking tracing state */
> > +	smp_rmb();
> >  
> > -	return 1;
> > +	if (on == !!tracing_is_on())
> > +		return;
> > +
> > +	if (on)
> > +		tracing_on();
> > +	else
> > +		tracing_off();
> > +
> > +	/* unlimited? */
> > +	if (old_count == -1)
> > +		return;
> > +
> > +	/* Make sure tracing state is visible before updating count */
> > +	smp_wmb();
> > +
> > +	*count = old_count - 1;
> >  }
> >  
> >  static void
> >  ftrace_traceon_count(unsigned long ip, unsigned long parent_ip, void **data)
> >  {
> > -	if (tracing_is_on())
> > -		return;
> > -
> > -	if (update_count(data))
> > -		tracing_on();
> > +	update_traceon_count(data, 1);
> >  }
> >  
> >  static void
> >  ftrace_traceoff_count(unsigned long ip, unsigned long parent_ip, void **data)
> >  {
> > -	if (!tracing_is_on())
> > -		return;
> > -
> > -	if (update_count(data))
> > -		tracing_off();
> > +	update_traceon_count(data, 0);
> >  }
> >  
> >  static void
> > @@ -330,11 +367,49 @@ ftrace_stacktrace(unsigned long ip, unsigned long parent_ip, void **data)
> >  static void
> >  ftrace_stacktrace_count(unsigned long ip, unsigned long parent_ip, void **data)
> >  {
> > -	if (!tracing_is_on())
> > -		return;
> > +	long *count = (long *)data;
> > +	long old_count;
> > +	long new_count;
> >  
> > -	if (update_count(data))
> > -		trace_dump_stack(STACK_SKIP);
> > +	/*
> > +	 * Stack traces should only execute the number of times the
> > +	 * user specified in the counter.
> > +	 */
> > +	do {
> > +
> > +		if (!tracing_is_on())
> > +			return;
> > +
> > +		old_count = *count;
> > +
> > +		if (!old_count)
> > +			return;
> > +
> > +		/* unlimited? */
> > +		if (old_count == -1) {
> > +			trace_dump_stack(STACK_SKIP);
> > +			return;
> > +		}
> > +
> > +		new_count = old_count - 1;
> > +		new_count = cmpxchg(count, old_count, new_count);
> > +		if (new_count == old_count)
> > +			trace_dump_stack(STACK_SKIP);
> > +
> > +	} while (new_count != old_count);
> > +}
> > +
> > +static int update_count(void **data)
> > +{
> > +	unsigned long *count = (long *)data;
> > +
> > +	if (!*count)
> > +		return 0;
> > +
> > +	if (*count != -1)
> > +		(*count)--;
> > +
> > +	return 1;
> >  }
> >  
> >  static void
> > 
> 
> 

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