Messages in this thread | | | Date | Mon, 20 Jul 2009 15:25:48 +0800 | From | Xiao Guangrong <> | Subject | Re: [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
> > >
| |