Re: [PATCH 1/3] ftrace: add tracepoint for timer

[Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>]

Thomas Gleixner wrote:

> No, this is _wrong_. You need to look at the full source. I added some
> extra comments
> 
> 	new_base = __get_cpu_var(tvec_bases);
> 
> 	if (base != new_base) {
> 
> + 	   	 /* current timer base is on a different CPU */
> 
> 		/*
> 		 * We are trying to schedule the timer on the local CPU.
> 		 * However we can't change timer's base while it is running,
> 		 * otherwise del_timer_sync() can't detect that the timer's
> 		 * handler yet has not finished. This also guarantees that
> 		 * the timer is serialized wrt itself.
> 		 */
> 		if (likely(base->running_timer != timer)) {
> 			/* See the comment in lock_timer_base() */
> 			timer_set_base(timer, NULL);
> 			spin_unlock(&base->lock);
> 			base = new_base;
> 			spin_lock(&base->lock);
> 			timer_set_base(timer, base);
> -		}
> +		} else
> +		  /*
> +		   * we know that that
> +		   * the callback is running on a different CPU and we need
> +		   * to keep base unchanged, so smp_processor_id() is
> +		   * telling you the wrong information.
> +		   */
> +	}
> 
>> We can not add the timer to the current CPUs by using add_timer_on(), selects
> 
>   We can add the timer to the current CPU by using add_timer_on() as well.
> 
>> the timer base in this function as below code:
>> 	struct tvec_base *base = per_cpu(tvec_bases, cpu);
>> In this case, We can know the timer is added to 'cpu'.
>>
>> So, I add trace_timer_start() in __mod_timer() and add_timer_on()in my patch.
> 
>   Still in the __mod_timer() case the tracing info can be wrong and
>   tracing wrong information is worse than tracing no information.
> 
>   Your patch could result in a trace which confuses the hell out of
>   people looking at it:
> 
>   ....... 0..... activate_timer on cpu 0
>   
>   some time later
> 
>   ....... 2..... expire timer on cpu 2
> 
>   And the guy who needs to analyse that trace would rip his hairs out
>   to find out how the timer moved from cpu 0 to cpu 2
>  
>> In hrtimer, all timer is added to the current CPU which can be getted by using
>> smp_processor_id() in probe function, so it not has 'cpu' argument in my patch.
> 
>   Wrong again. Read the code in switch_hrtimer_base(). It does the
>   same thing as the timer base selection logic in __mod_timer()
>  

Hi tglx: 

Thanks for you correct my mistake again.  :-) 

It's hard to detect which cpu the timer add, we can remove the 'cpu' parameter
in trace_timer_start() as your suggestion, like this:

TRACE_EVENT(timer_start,

	TP_PROTO(struct timer_list *timer),

	TP_ARGS(timer),

	TP_STRUCT__entry(
		__field( void *,	timer		)
		__field( void *,	function	)
		__field( unsigned long,	expires		)
	),
	......
}

>> In addition, we do better not put trace_timer_start() and debug_timer_activate
>> in one function, have two reasons:
>> 1: for trace_timer_start()'s logic, the timer start event is completed in 
>>    internal_add_timer(), in other words: the timer is not start before
>>    internal_add_timer().
> 
>  Oh well. So where is the difference of tracing it before or after the
>  list add happened ? That's complete irrelevant.
>  

Yes, maybe it's not important.

>> 2: as Zhaolei says in the last mail, the timer's data may changed after
>>    debug_timer_activate().
> 
>  Really ? What is going to change ? Nothing in the normal case, in the
>  case the timer is active then it is removed first. Also it depends on
>  how you do this:
> 
> void debug_and_trace_timer_activate(....)
> {
> 	debug_timer_activate(...);
> 	trace_timer_activate(...);
> }
> 
> in the timer code:
> 
> -      debug_timer_activate(...);
> +      debug_and_trace_timer_activate(...);
> 
> So this does not change the order of functions at all, but it avoids
> sprinkling the real code with tons of extra crap.
> 

See below code:

static inline int
__mod_timer(......)
{
	......
	......
	
	debug_timer_activate(timer);

	new_base = __get_cpu_var(tvec_bases);
	......
	......
	
	timer->expires = expires;	*
	internal_add_timer(base, timer);
	trace_timer_start(...)
	......
}
( this example is in Zhaolei's reply)

timer->expires can be changed at *, if we put trace_timer_start() and
debug_timer_activate() together, we can't get the right value of timer->expires.

In addition, do you agree my humble opinion about not put __init_timer() and
debug_timer_init() together? (can be found at:
http://marc.info/?l=linux-kernel&m=124399744614127&w=2)
If you agree with it, we do better to detach other event.

Thanks,
Xiao Guangrong

> Thanks,
> 
> 	tglx
> 
>