Re: [RFA][PATCH 0/4] tracing: Request for acks on fixing tracepoint code

[Steven Rostedt]

On Wed, 26 Feb 2014 21:36:18 +0000 (UTC)
Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx> wrote:

> ----- Original Message -----
> > From: "Steven Rostedt" <rostedt@xxxxxxxxxxx>
> > To: linux-kernel@xxxxxxxxxxxxxxx
> > Cc: "Ingo Molnar" <mingo@xxxxxxxxxx>, "Andrew Morton" <akpm@xxxxxxxxxxxxxxxxxxxx>, "Peter Zijlstra"
> > <peterz@xxxxxxxxxxxxx>, "Frederic Weisbecker" <fweisbec@xxxxxxxxx>, "Mathieu Desnoyers"
> > <mathieu.desnoyers@xxxxxxxxxxxx>
> > Sent: Wednesday, February 26, 2014 2:01:40 PM
> > Subject: [RFA][PATCH 0/4] tracing: Request for acks on fixing tracepoint code
> > 
> > [ Request for Acks ]
> > 
> > Due to module tainting, we have tracepoints that silently do not work.
> > That will be solved another way. But the trace event infrastructure should
> > not be created for tainted modules. That is, the debugfs files should
> > not exist for them.
> > 
> > By moving the tracepoint module taint test into tracepoint.h, we can
> > reuse that same test when creating the module tracepoint events.
> > 
> > Note, I had to remove the tracepoint.h include from module.h as there
> > was nothing in module.h that required tracepoint.h, but this broke
> > a couple of event files (migrate.h and writeback.h) because they did
> > not include tracepoint.h, and were just lucky that it was included
> > by module.h.
> 
> When designing tracepoint.h, a lot of care went into making sure it did
> not have needless dependency on other headers, since this header is
> expected to be included into many other files and headers, thus posing
> a clear risk of becoming yet another root of an include dependency hell.

Well, module.h is included in many more.

> 
> While I agree on adding the API you propose, why made it a static inline ?
> This adds this dependency from tracepoint.h on module.h. Instead, we could
> just declare a symbol, and implement a tracepoint_module_has_bad_taint()
> within kernel/tracepoint.c. It should not be a fast path anyway, so I don't
> see the point it making it a static inline.
> 
> I also recommend sticking to the tracepoint_*() API (rather than trace_*).

Well, as this is now not just for tracepoints, but also used by the
trace_events, and because the name is already too big (but
descriptive), I rather not change it.

But as a compromise, I can move it to ftrace_event.h instead.

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