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

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

----- Original Message -----
> From: "Steven Rostedt" <rostedt@goodmis.org>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> Cc: "Frank Ch. Eigler" <fche@redhat.com>, linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@kernel.org>, "Frederic
> Weisbecker" <fweisbec@gmail.com>, "Andrew Morton" <akpm@linux-foundation.org>, "Johannes Berg"
> <johannes.berg@intel.com>, "Linus Torvalds" <torvalds@linux-foundation.org>, "Peter Zijlstra"
> <peterz@infradead.org>, "Thomas Gleixner" <tglx@linutronix.de>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
> "lttng-dev" <lttng-dev@lists.lttng.org>, "Rusty Russell" <rusty@rustcorp.com.au>
> Sent: Wednesday, March 12, 2014 3:30:06 PM
> Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs
> 
> On Wed, 12 Mar 2014 14:58:02 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > 
> > Two modules should not have the same name. Is there any duplicate
> > tracepoints you are aware of. Namespace collisions in tracepoints
> > should be avoided, as that would cause people to trace things they did
> > not intend on tracing.
> > 
> > That should be a new patch as well. Enforce unique tracepoint names.
> 
> This may be why you are not understanding what I want. It's the way
> things are implemented today, which I believe are wrong. I see what you
> did. You have probes that are registered, and tracepoints that are
> where the code lies. You just add and remove probes from a hash list,
> and then you loop through all the tracepoints seeing if the iter->name
> matches a probe->name.
> 
> I'm fine with keeping the probe separate, but there really should be
> no more than just a one to one mapping between probes and tracepoints.
> Have the probe point to the matching tracepoint. The probe is
> registered, it enables the tracepoint static key, when it's ref count
> goes to zero, it disables the tracepoint static key. We can get rid of
> that loop then, as well as the duplicate names between probes and
> tracepoints.

Right there, this is not possible for a few reasons, namely:

- loop unrolling performed by the compiler can duplicate a tracepoint,
  even if it is only there once in the source code,
- inlining performed by the compiler may do the same,
- LTO, whenever it will start being used for the kernel, may do the same,
  and also spread call sites across modules.

There can be no 1 to 1 mapping between a probe function and a callsite
due to those compilers optimisations, even if we enforce the strictest
coding style rules possible on their use.

Thanks,

Mathieu


> 
> Here's the steps we should take:
> 
> 1) Prevent duplicate tracepoints. They are just namespace collisions
> that we already try to avoid. How to do this? We may need to add a
> hlist_node to the tracepoint structure, and keep them in a hash by name.
> Check for collisions when the name is added to the hash.
> 
> 2) Change the way tracepoints are enabled. Do not do a loop of all
> tracepoints, but instead have the first probe of a tracepoint enable
> it, and the last one to disable it. This would require a pointer from
> the probe to the tracepoint it represents. Again, it should not
> represent more than one.
> 
> 3) On module unload, it would be the responsibility of the user to
> unload all the tracepoints that may have been enabled for a module. We
> can add a mod pointer in the probe to make this easier, as well as to
> the tp_module structure.
> 
> The way tracepoints are today are to handle two completely different
> tracepoints with the same name. That should be avoided, and will make
> things much less complex.
> 
> Then you can easily handle the accounting of modules loading and
> unloading in your module, and the tracepoint code will match what the
> rest of the kernel does for resource management.
> 
> -- Steve
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com