Re: [PATCH v3 3/4] ftrace: add tracepoint for hrtimer

[Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>]

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

> 
> 
>