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