Date:	Tue, 11 Mar 2014 11:40:23 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc:	"Frank Ch. Eigler" <fche@...hat.com>, linux-kernel@...r.kernel.org,
	Ingo Molnar <mingo@...nel.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Johannes Berg <johannes.berg@...el.com>
Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not
 set via debugfs

On Tue, 11 Mar 2014 15:06:22 +0000 (UTC)
Mathieu Desnoyers <mathieu.desnoyers@...icios.com> wrote:


> Let's step back and look at the overall picture here, along with the
> possible solutions that are available so far.
> 
> My intent is to let end users specify "I want to trace this specific
> tracepoint" from the tracer interface as long as the tracepoint probe
> provider module is loaded (the file with CREATE_TRACE_POINTS defined).
> For instance, the tracepoint call site can be within a driver loaded by
> the USB stack when a USB device is plugged in. While tracing is enabled,
> the user may want to plug in the said USB device to investigate what is
> the culprit of an issue he would be facing when the the device is plugged
> in.
> 
> There seems to be 2 elegant ways to achieve this while giving feedback to
> tracers about whether there are active tracepoint callsites or not:
> 
> 1) We add a trace_has_callsites_enabled() (or any better name) function to
>    tracepoint.h, which allows tracers to query whether a given tracepoint
>    name has any callsites enabled,

I don't find that one very elegant at all.

> 2) We change tracepoint_probe_register() so it unregisters the tracepoint
>    and return -ENODEV if there are no callsites enabled, and deal with
>    the use-case I explain above with module coming and going within the
>    tracer. This duplicates part of the tracepoint infrastructure into the
>    tracer though, which is why I was not so keen on going for this
>    solution.

I don't mind the above.

> 
> The other solution proposed by Steven (returning -ENODEV without
> unregistering the tracepoint) does not appear to be an elegant solution,
> as we discussed earlier in this thread. It kind of weird to have a negative
> value treated as an OK special case.

Right, but it was a compromise as you didn't like #2 earlier.

> 
> The other solution proposed by Steven in an earlier thread was to tie
> tracepoints very deeply with module loading infrastructure and add module
> parameters specifically to specify if tracepoint callsites need to be
> enabled on module load. This approach unfortunately expect that everyone
> interacts with module loading, as root, in a system-wide (no multi-session)

Who can load modules not as root?? That is utterly broken. As once you
can load a module, YOU ARE ROOT.
 
> fashion and is not suitable for the user-base we are targeting. I seems to
> be a user experience disaster IMHO.

For your case only. But it is normal operation for normal uses of Linux.

> 
> I'm OK as long as we have an elegant way forward. Ideally I would have
> prefered (1) to eliminate code duplication between tracers and tracepoint
> infrastructure (we have to reimplement a hash table similar to tracepoints
> within the tracer with solution (2)), but (2) technically works too.

Here's what I propose then. We implement 2 for now. You can "duplicate"
the code into your own work. Then we should be able to simplify the
tracepoint code as it no longer will have the requirement to enable
tracepoints that do not exist.

Also we can remove that tracepoint_probe_unregister_noupdate() as it
has no in-tree users. Which, you have been lucky that no one noticed
that yet, as that is a legitimate excuse to remove a function.

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