Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Mathieu Desnoyers]

----- Original Message -----
> From: "Steven Rostedt" <rostedt@xxxxxxxxxxx>
> To: "Frank Ch. Eigler" <fche@xxxxxxxxxx>
> Cc: "Mathieu Desnoyers" <mathieu.desnoyers@xxxxxxxxxxxx>, linux-kernel@xxxxxxxxxxxxxxx, "Ingo Molnar"
> <mingo@xxxxxxxxxx>, "Frederic Weisbecker" <fweisbec@xxxxxxxxx>, "Andrew Morton" <akpm@xxxxxxxxxxxxxxxxxxxx>,
> "Johannes Berg" <johannes.berg@xxxxxxxxx>
> Sent: Monday, March 10, 2014 10:58:20 PM
> Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs
> 
> On Mon, 10 Mar 2014 22:41:31 -0400
> fche@xxxxxxxxxx (Frank Ch. Eigler) wrote:
> 
> > 
> > Hi -
> > 
> > 
> > >> From: Steven Rostedt <rostedt@xxxxxxxxxxx>
> > >> 
> > >> Tracepoints were made to allow enabling a tracepoint in a module before
> > >> that
> > >> module was loaded. When a tracepoint is enabled and it does not exist,
> > >> the
> > >> name is stored and will be enabled when the tracepoint is created.
> > >> 
> > >> The problem with this approach is that when a tracepoint is enabled when
> > >> it expects to be there, it gives no warning that it does not exist.
> > 
> > So it is a deferred-activation kind of call, with no way of knowing
> > when or if the tracepoints will start coming in.  Why is that
> > supported at all, considering that clients could monitor modules
> > coming & going via the module_notifier chain, and update registration
> > at that time?
> 
> That's my argument.

So basically, all we'd have to do in LTTng is to add a hash table tracking the
tracepoint probes which are registered, but for which there are no
tracepoint call sites. Whenever registration of a probe would fail due to
-ENODEV (assuming we unregister the probe within tracepoint.c when we return
-ENODEV, as you initially proposed), we would put this probe in the hash table.
Upon module coming, we would iterate on the module's tracepoints and check
if any of those match the content of the hash table, and then register the
probe.

I guess I'd prefer that to the weird successful failure return value in
tracepoint.c.

Thanks,

Mathieu

> 
> > 
> > 
> > >> +	entry = get_tracepoint(name);
> > >> +	/* Make sure the entry was enabled */
> > >> +	if (!entry || !entry->enabled)
> > >> +		ret = -ENODEV;
> > 
> > For what it's worth, I agree with Mathieu that this sort of successful
> > failure result code is a problem for tracking what needs cleanup and
> > what doesn't.  (In systemtap's case, if this change gets merged, we'll
> > have to treat -ENODEV as if it were 0.)
> 
> Does systemtap enable tracepoints before they are created? That is, do
> you allow enabling of a tracepoint in a module that is not loaded yet?
> 
> If not, than you want this as an error.
> 
> -- Steve
> 

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