linux-kernel - Re: Re: Re: [PATCH 1/3] ftrace: add tracepoint for timer

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: <4A239A92.60708@cn.fujitsu.com>
Date:	Mon, 01 Jun 2009 17:08:34 +0800
From:	Zhaolei <zhaolei@...fujitsu.com>
To:	Thomas Gleixner <tglx@...utronix.de>
CC:	Xiao Guangrong <xiaoguangrong@...fujitsu.com>, mingo@...e.hu,
	LKML <linux-kernel@...r.kernel.org>,
	kosaki.motohiro@...fujitsu.com,
	Steven Rostedt <rostedt@...dmis.org>, fweisbec@...il.com
Subject: Re: Re: Re: [PATCH 1/3] ftrace: add tracepoint for timer

Thomas Gleixner wrote:
> On Fri, 29 May 2009, Zhaolei wrote:
>> But, for trace_timer_start() in __mod_timer(), we need to put it after
>> timer->* changed.
> 
> Why ?

Hello, Thomas

Thanks for your review.

Please see my explain below.

> 
>>> +	TP_fast_assign(
>>> +		__entry->timer		= timer;
>>> +		__entry->function	= timer->function;
>>> +		__entry->expires	= timer->expires;
>>> +		__entry->cpu		= cpu;
> 
> Again, neither timer nor function nor expires will change when the
> timer is added, right ?
> 

Sorry for my poor English.
I don't means that internal_add_timer() will change timer->*.
My meaning is:
	timer->expires = expires; *
	internal_add_timer(base, timer);

*: timer->expires is changed here, so trace_timer_start() need to called below
   this line. If we call debug_timer_activate() and trace_timer_start() together,
   we need to move debug_timer_activate() to place below this line too.

> The only unknown at this point is cpu. See below.
> 
>> Nevertheless, it don't means we need separate trace_timer_start() and
>> debug_timer_activate(), because we can put move debug_timer_activate() below,
>> as:
>> -	debug_timer_activate(timer);
>> 	...
>>  	timer->expires = expires;
>>  	internal_add_timer(base, timer);
>> +	debug_timer_activate(timer);
> 
> No, you can not call it with the base->lock held.
> 
>> +	trace_timer_start(timer, smp_processor_id());
> 
> Also using smp_processor_id() here is wrong. We do not necessarily add
> the timer to the current CPUs timer wheel. See the code which selects
> the timer base. So this information is rather useless, because the
> tracer knows anyway  on which CPU we are running.
> 
> Unfortunately we do not have an easy way to figure out to which CPU
> the base belongs (except if it's the base of the current CPU). There
> is not much we can do about that. But OTOH, this is not a problem
> because we see when the timer expires on which CPU it was enqueued. So
> scrapping the cpu entry in the trace completely is not a big loss.

Indeed.
We can remove smp_processor_id() from trace_timer_start()'s argument.

Xiao Guangrong, author of this patch is in vacation these days, and will come
back recently.
Maybe we want to hear his opinion about this fix.

Thanks
Zhaolei

--
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/