Message-ID: <20080703151238.GA3102@Krystal>
Date:	Thu, 3 Jul 2008 11:12:38 -0400
From:	Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>
To:	Masami Hiramatsu <mhiramat@...hat.com>
Cc:	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	Takashi Nishiie <t-nishiie@...css.fujitsu.com>,
	'Alexey Dobriyan' <adobriyan@...il.com>,
	'Peter Zijlstra' <peterz@...radead.org>,
	'Steven Rostedt' <rostedt@...dmis.org>,
	"'Frank Ch. Eigler'" <fche@...hat.com>,
	'Ingo Molnar' <mingo@...e.hu>,
	'LKML' <linux-kernel@...r.kernel.org>,
	'systemtap-ml' <systemtap@...rces.redhat.com>,
	'Hideo AOKI' <haoki@...hat.com>
Subject: Re: [RFC PATCH] Kernel Tracepoints

* Masami Hiramatsu (mhiramat@...hat.com) wrote:
> Hi Mathieu,
> 
> Mathieu Desnoyers wrote:
> > * Masami Hiramatsu (mhiramat@...hat.com) wrote:
> >> Mathieu Desnoyers wrote:
> >>> * Masami Hiramatsu (mhiramat@...hat.com) wrote:
> >>>  >
> >>>>> Implementation of kernel tracepoints. Inspired from the Linux Kernel Markers.
> >>>> What would you think redesigning markers on tracepoints? because most of the
> >>>> logic (scaning sections, multiple probe and activation) seems very similar
> >>>> to markers.
> >>>>
> >>> We could, although markers, because they use var args, allow to put the
> >>> iteration on the multi probe array out-of-line. Tracepoints cannot
> >>> afford this and the iteration must be done at the initial call-site.
> >>>
> >>> From what I see in your proposal, it's mostly to extract the if() call()
> >>> code from the inner __trace_mark() macro and to put it in a separate
> >>> macro, am I correct ? This would make the macro more readable.
> >> Sure, I think marker and tracepoint can share below functions;
> >> - definition of static local variables in specific sections
> > 
> > Given that we could want to keep activation of tracepoints and markers
> > separate (so they don't share the same namespace), declaring the static
> > variables in separated sections seems to make sense to me.
> 
> Sorry, I'm not sure what is "separate activation".
> As far as I can see, both tracepoints and markers are activated
> when its probe handlers are registered on each tracepoint/marker.
> Aren't it separated?
> 

Yes, it is separate. This is insured by the fact that markers and
tracepoints are declared in different sections. Therefore, even if they
have the same "name", they won't be used by each other.

> I did not mean integrating registering interfaces, but
> I think that they can share base(internal) functions.
> for example, both of them has XXX_update_range/_module_XXX_update etc.
> 

Hrm, but the sections and symbols on which these function iterate are
different. I am unsure it's worth trying to merge such tiny functions.

> IMHO, current code is not so good for maintenance. there are
> many code duplications (ex. kernel/module.c, I think
> that both of them (and imv too?) can share the code for
> handling its section and iterating entries). I'm not sure those
> duplications are acceptable.

Given it's only slmost one-liners, and that there is some ordering to
keep (markers and tracepoints must be updated before immediate values
because they might influence the immediate value state), I don't think
having a special section for these callbacks (a little bit like
initcalls, but for module load) is a good option.

> 
> >> - probe activation code (if() call())
> >> - multi probe handling
> > 
> > Hrm, the thing here is that because markers allow to do the iteration on
> > the multiple probe callbacks within an internal wrapper (instead of
> > doing it on-site as in the tracepoints), it allows to do some further
> > optimizations (less memory allocation and less pointer dereference in
> > the single probe case, not having to prepare the va_args in the
> > MARK_NOARGS case) which are only done because it does not have to add
> > code to the instrumentation site. However, tracepoints cannot have such
> > "generic" wrapper and we have to do the iteration on callbacks in the
> > code added to the instrumented object. Therefore, I keep it as small as
> > possible in terms of bytes of instructions.
> 
> OK, I see. So, __tracepoint_block() macro can specify handler function.
> what would you think about it?
> 

When I originally designed the markers, I tried to make sure there was
absolutely no code duplication until I discovered that trying to read a
huge amount of nested macros is just a pain starting from a certain
level. If we only save a few duplicated lines but end up tying up
markers and tracepoints, I am far from certain that it will make the
code more readable.

I'll post a tracepoint version with the modifications you proposed (it's
now placed earlier in the patchset), except the merge with markers.

Mathieu

> Thank you,
> 
> -- 
> Masami Hiramatsu
> 
> Software Engineer
> Hitachi Computer Products (America) Inc.
> Software Solutions Division
> 
> e-mail: mhiramat@...hat.com
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
--
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/

Powered by blists - more mailing lists