lkml.org 
[lkml]   [2009]   [Jul]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 3/4] ftrace: add tracepoint for hrtimer
Hi Peter,

Thanks for your review.

Peter Zijlstra wrote:

>> +static inline void debug_and_trace_hrtimer_deactivate(struct hrtimer *timer)
>> +{
>> + debug_hrtimer_deactivate(timer);
>> + trace_hrtimer_cancel(timer);
>> +}
>
> I would argue that tracing is a form of debugging and you shouldn't need
> to mangle these names like that, simply leave them debug_*().
>

I think this makes sense. I'll fix it unless Thomas has objections.

>
>> @@ -1162,9 +1182,8 @@ static void __run_hrtimer(struct hrtimer *timer)
>> * the timer base.
>> */
>> spin_unlock(&cpu_base->lock);
>> - trace_hrtimer_entry(timer);
>> restart = fn(timer);
>> - trace_hrtimer_exit(timer, restart);
>> + trace_hrtimer_callback_done(timer);
>> spin_lock(&cpu_base->lock);
>>
>> /*
>
> Why bother introducing these tracepoints if you're going to remove them
> in the same patch-set?
>

Actually I'm renaming them but not removing them.

I can drop the first patch and merge it into the latter patches,
but that will lose the credit for Anton Blanchard

> Also, the below:
>
>> @@ -1275,6 +1294,7 @@ void hrtimer_interrupt(struct clock_event_device *dev)
>> break;
>> }
>>
>> + trace_hrtimer_expire(timer, basenow.tv64);
>> __run_hrtimer(timer);
>> }
>> base++;
>> @@ -1397,6 +1417,7 @@ void hrtimer_run_queues(void)
>> hrtimer_get_expires_tv64(timer))
>> break;
>>
>> + trace_hrtimer_expire(timer, base->softirq_time.tv64);
>> __run_hrtimer(timer);
>> }
>> spin_unlock(&cpu_base->lock);
>
> indicates you placed that tracepoint in the wrong place.
>
> Furthermore, I don't get why you want it there and not on the old
> _entry() site, because this adds all kinds of extra overhead and you
> loose the exact callback timings.
>

Yes, it's true, but the loose is only about 1 microsecond as I tested it.
Do you think it's acceptable or not?

If we put trace_hrtimer_expire() on the old _entry() site, then we can't
get the timestamps when hrtimer expires, which is used to calculate the
latency of hrtimer.

You can see the mail which I send to Thomas last week, can be found here:
http://marc.info/?l=linux-kernel&m=124762164322497&w=2)


Thanks,
Xiao

>
>
>


\
 
 \ /
  Last update: 2009-07-20 09:29    [W:0.074 / U:2.092 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site