Message-ID: <1272656885.9739.198.camel@gandalf.stny.rr.com>
Date:	Fri, 30 Apr 2010 15:48:05 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Mathieu Desnoyers <compudj@...stal.dyndns.org>
Cc:	linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...e.hu>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Peter Zijlstra <peterz@...radead.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Arnaldo Carvalho de Melo <acme@...hat.com>,
	Lai Jiangshan <laijs@...fujitsu.com>,
	Li Zefan <lizf@...fujitsu.com>,
	Masami Hiramatsu <mhiramat@...hat.com>,
	Christoph Hellwig <hch@....de>
Subject: Re: [PATCH 04/10][RFC] tracing: Remove per event trace registering

On Fri, 2010-04-30 at 15:06 -0400, Mathieu Desnoyers wrote:

> > If it is possible sure, but that's the point. Where do you add the
> > check? The typecast is in the C code that is constant for all trace
> > events.
> 
> You can add the call to the static inline type check directly within the
> generated probe function, right after the local variable declarations.

Well, one thing, the callback is not going to be the same as the
DECLARE_TRACE() because the prototype ends with "void *data", and the
function being called actually uses the type of that data.

We now will have:

	DEFINE_TRACE(mytracepoint, int myarg, myarg);

	void mycallback(int myarg, struct mystuct *mydata);

	register_trace_mytracepoint_data(mycallback, mydata)

There's no place in DEFINE_TRACE to be able to test the type of data
that is being passed back. I could make the calling function be:

	void mycallback(int myarg, void *data)
	{
		struct mystruct *mydata = data;
	[...]

Because the data is defined uniquely by the caller that registers a
callback. Each function can register its own data type.

> 
> > 
> > > 
> > > I also don't trust that these complex TRACE_EVENT() preprocessor macros
> > 
> > Thanks for your vote of confidence.
> 
> Please don't take this personally. As I said above, I include myself in
> the list of people I don't trust to write entirely bug-free code. I'm
> just saying that we should not overlook a possibility to detect more
> bugs automatically when we have one, especially if this results in no
> object code change.

The point being is that this is not about buggy code, but the fact that
the same data is being used in two places, you want to test to make sure
it is the same. I don't see how this helps.



> 
> > 
> > > will never ever have bugs. That's just doomed to happen one day or
> > > another. Again, call me paranoid if you like, but I think adding this
> > > type checking is justified.
> > 
> > Where do you add the typecheck?? As I said before, if the TRACE_EVENT()
> > macros are broken, then so will the typecheck, and it will not catch the
> > errors.
> > 
> > Sure the event macros can have bugs, but if it does then it will have
> > bugs for all. Because it is automated. If there is a bug, it wont be
> > because of a missed type being passed in, it would be because of one of
> > the extra macros we have that processes the same type incorrectly.
> > 
> > > 
> > > I am providing the type check implementation in a separate email. It
> > > will need to be extended to support the extra data parameter you plan to
> > > add.
> > 
> > I saw the patch, but how does it help?
> > 
> > I use "proto" to make the tracepoint and the callback, so I can add
> > somewhere this "check_trace_callback_type_##name(proto)", but if the
> > macros break somehow, that means proto changed between two references of
> > it, but what keeps proto from breaking at both callback creation and the
> > typecheck.
> > 
> > Basically, you are saying that somehow the argument "proto" can change
> > between two uses of it. I don't really see that happening, and I'm not
> > paranoid enough to think that's an issue. Adding checks that don't
> > really check anything, honestly I find a waste, and just more confusion
> > in the macros.
> 
> In the TRACE_EVENT() case, without the extra "void *data" argument,
> it is indeed checking that the "proto" of the callback you create is
> that same as the "proto" expected by the tracepoint call. However, given
> that you plan on adding other parameters besides "proto", then the added
> type-checking makes more and more sense.

But you can not test it! That's my point.

The first part of proto will be the same, and that's all we can test.
But the data parameter that the DECLARE_TRACE() is going to create will
be void *. Which means we can't test it. This is something that C lacks,
and we could test it in C++ if we did this with templates. The only way
to test it is at runtime with a magic number in the data field.

This is the same as the file->private data. You can't test it at build
time.

Let me explain this again:

	DECLARE_TRACE(name, proto, args);

Will call the function like:

	callback(args, data);

The callback will be at best:

	int callback(proto, void *data);


because the data being passed in is not defined yet. It is defined at
the point of the registering of the callback. You can have two callbacks
registered to the same tracepoint with two different types as the data
field.

So what is it that this check is testing?

-- Steve


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