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

Xiao Guangrong
2009-07-07T09:28:20Z

Thomas Gleixner wrote:
> On Mon, 6 Jul 2009, Xiao Guangrong wrote:
>> Changelog v1->v2:
>> 1: Add jiffies info into timer
>> 2: Combine debugobjects and trace as Thomas's suggestion
>
> I like it that way. Some minor nitpicks below.
>
>> +TRACE_EVENT(timer_expire,
>> +
>> + TP_PROTO(struct timer_list *timer),
>> +
>> + TP_ARGS(timer),
>> +
>> + TP_STRUCT__entry(
>> + __field( void *, timer )
>> + __field( void *, function )
>
> Do we really need the function here? It does not change between
> start and expiry.
>

Yeah, I'll fix.

>> +TRACE_EVENT(timer_cancel,
>> +
>> + TP_PROTO(struct timer_list *timer),
>> +
>> + TP_ARGS(timer),
>> +
>> + TP_STRUCT__entry(
>> + __field( void *, timer )
>> + __field( void *, function )
>
> Ditto.
>

Will fix.

>> +
>> +static inline void debug_and_trace_timer_expire(struct timer_list *timer)
>
> Technically not really necessary. The debug code does not care about
> it.
>

Will fix.

>> timer = list_first_entry(head, struct timer_list,entry);
>> + debug_and_trace_timer_expire(timer);
>> fn = timer->function;
>> data = timer->data;
>>
>
> Can you please add the tracepoint which was suggested by Anton (IIRC)
> which allows us to measure the execution time of the callback function
> ? There is no need to dereference timer in the expiry tracepoint, we
> just store the pointer. So we can safely move the tracepoints just
> around the function call.
>
> trace_timer_expire(timer);
> fn(data);
> trace_timer_callback_done(timer);
>
> Or whatever fancy name you come up with.
>

OK, I'll fix it base on your suggestion.

trace_timer_expire/trace_timer_callback_done are already in Frederic' tree
but with different name, see below URL:
http://marc.info/?l=linux-kernel&m=124331396919301&w=2

I'll modify those tracepoint's name to match ours.

Thank you very much!

Xiao

>
> tglx
>
>