linux-kernel - 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]

Date:	Thu, 4 Jun 2009 10:44:38 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Xiao Guangrong <xiaoguangrong@...fujitsu.com>
cc:	Zhaolei <zhaolei@...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: [PATCH 1/3] ftrace: add tracepoint for timer

On Thu, 4 Jun 2009, Xiao Guangrong wrote:
> > void debug_and_trace_timer_activate(....)
> > {
> > 	debug_timer_activate(...);
> > 	trace_timer_activate(...);
> > }
> > 
> > in the timer code:
> > 
> > -      debug_timer_activate(...);
> > +      debug_and_trace_timer_activate(...);
> > 
> > So this does not change the order of functions at all, but it avoids
> > sprinkling the real code with tons of extra crap.
> > 
> 
> See below code:
> 
> static inline int
> __mod_timer(......)
> {
> 	......
> 	......
>

- 	debug_timer_activate(timer);
+	debug_and_trace_timer_activate(timer, expires);

  Does the trick nicely

> In addition, do you agree my humble opinion about not put __init_timer() and
> debug_timer_init() together? (can be found at:

No, I do not agree at all. I still see no value in those extra trace
points. As I showed you above you can hide all in extra arguments to a
combined debug_trace_xxx() function.

> http://marc.info/?l=linux-kernel&m=124399744614127&w=2)

>> But TRACE_EVENT not only be used in ftrace but also be used in
>> other probe module. Maybe detailed information of timer is
>> requisite in other probe funtion.  In this case, we must put
>> trace_timer_init() after __init_timer().

Please stop this handwaving about potential use cases. I still have
not seen a single technical argument which explains the benefit of
those extra trace points. If we follow your "maybe its needed" logic
then we have to add yet another dozen of tracepoints into the same
functions to gather all possible states which might be there.

Tracing is important but it needs to be done unintrusive to the code
and you have to apply common sense which information is really
valuable and necessary. Gathering random crap just because it might be
necessary at some point is useless.

Get your gear together, sit down and think hard about which
information a tracer or a probe needs to have to give us a useful
insight into a particular object or function.

As long as there is no technical convincing argument that the gathered
information is really useful I'm not going to apply any of those
patches to the code I maintain.

Thanks,

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