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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4A641BFC.2050508@cn.fujitsu.com>
Date:	Mon, 20 Jul 2009 15:25:48 +0800
From:	Xiao Guangrong <xiaoguangrong@...fujitsu.com>
To:	Peter Zijlstra <peterz@...radead.org>
CC:	Ingo Molnar <mingo@...e.hu>, Thomas Gleixner <tglx@...utronix.de>,
	Steven Rostedt <rostedt@...dmis.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Zhaolei <zhaolei@...fujitsu.com>, kosaki.motohiro@...fujitsu.com,
	Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>,
	Anton Blanchard <anton@...ba.org>,
	LKML <linux-kernel@...r.kernel.org>
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

> 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/