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

[Zhaolei <zhaolei@cn.fujitsu.com>]

Thomas Gleixner wrote:
> On Fri, 29 May 2009, Zhaolei wrote:
>> But, for trace_timer_start() in __mod_timer(), we need to put it after
>> timer->* changed.
> 
> Why ?

Hello, Thomas

Thanks for your review.

Please see my explain below.

> 
>>> +	TP_fast_assign(
>>> +		__entry->timer		= timer;
>>> +		__entry->function	= timer->function;
>>> +		__entry->expires	= timer->expires;
>>> +		__entry->cpu		= cpu;
> 
> Again, neither timer nor function nor expires will change when the
> timer is added, right ?
> 

Sorry for my poor English.
I don't means that internal_add_timer() will change timer->*.
My meaning is:
	timer->expires = expires; *
	internal_add_timer(base, timer);

*: timer->expires is changed here, so trace_timer_start() need to called below
   this line. If we call debug_timer_activate() and trace_timer_start() together,
   we need to move debug_timer_activate() to place below this line too.

> The only unknown at this point is cpu. See below.
> 
>> Nevertheless, it don't means we need separate trace_timer_start() and
>> debug_timer_activate(), because we can put move debug_timer_activate() below,
>> as:
>> -	debug_timer_activate(timer);
>> 	...
>>  	timer->expires = expires;
>>  	internal_add_timer(base, timer);
>> +	debug_timer_activate(timer);
> 
> No, you can not call it with the base->lock held.
> 
>> +	trace_timer_start(timer, smp_processor_id());
> 
> Also using smp_processor_id() here is wrong. We do not necessarily add
> the timer to the current CPUs timer wheel. See the code which selects
> the timer base. So this information is rather useless, because the
> tracer knows anyway  on which CPU we are running.
> 
> Unfortunately we do not have an easy way to figure out to which CPU
> the base belongs (except if it's the base of the current CPU). There
> is not much we can do about that. But OTOH, this is not a problem
> because we see when the timer expires on which CPU it was enqueued. So
> scrapping the cpu entry in the trace completely is not a big loss.

Indeed.
We can remove smp_processor_id() from trace_timer_start()'s argument.

Xiao Guangrong, author of this patch is in vacation these days, and will come
back recently.
Maybe we want to hear his opinion about this fix.

Thanks
Zhaolei