* [RFA][PATCH 0/4] tracing: Request for acks on fixing tracepoint code
@ 2014-02-26 19:01 Steven Rostedt
2014-02-26 19:01 ` [RFA][PATCH 1/4] tracing: Fix event header writeback.h to include tracepoint.h Steven Rostedt
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: Steven Rostedt @ 2014-02-26 19:01 UTC (permalink / raw)
To: linux-kernel
Cc: Ingo Molnar, Andrew Morton, Peter Zijlstra, Frederic Weisbecker,
Mathieu Desnoyers
[ 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.
Steven Rostedt (Red Hat) (4):
tracing: Fix event header writeback.h to include tracepoint.h
tracing: Fix event header migrate.h to include tracepoint.h
tracing/module: Remove include of tracepoint.h from module.h
tracing: Do not add event files for modules that fail tracepoints
----
include/linux/module.h | 1 -
include/linux/tracepoint.h | 7 +++++++
include/trace/events/migrate.h | 2 ++
include/trace/events/writeback.h | 1 +
kernel/trace/trace_events.c | 4 ++++
kernel/tracepoint.c | 2 +-
6 files changed, 23 insertions(+), 21 deletions(-)
^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFA][PATCH 1/4] tracing: Fix event header writeback.h to include tracepoint.h
2014-02-26 19:01 [RFA][PATCH 0/4] tracing: Request for acks on fixing tracepoint code Steven Rostedt
@ 2014-02-26 19:01 ` Steven Rostedt
2014-02-26 19:01 ` [RFA][PATCH 2/4] tracing: Fix event header migrate.h " Steven Rostedt
` (3 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Steven Rostedt @ 2014-02-26 19:01 UTC (permalink / raw)
To: linux-kernel
Cc: Ingo Molnar, Andrew Morton, Peter Zijlstra, Frederic Weisbecker,
Mathieu Desnoyers, Dave Chinner, Jens Axboe
[-- Attachment #1: 0001-tracing-Fix-event-header-writeback.h-to-include-trac.patch --]
[-- Type: text/plain, Size: 1013 bytes --]
[ Request for Ack ]
From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
The trace event headers are required to include tracepoint.h. The only reason
they worked now is because module.h included tracepoint.h, and that will soon
change. (And the change will be going to the stable tree as well)
Fixes: 455b2864686d "writeback: Initial tracing support"
Cc: stable@vger.kernel.org # 2.6.36+
Cc: Dave Chinner <dchinner@redhat.com>
Cc: Jens Axboe <jaxboe@fusionio.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
include/trace/events/writeback.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/trace/events/writeback.h b/include/trace/events/writeback.h
index c7bbbe7..309a086 100644
--- a/include/trace/events/writeback.h
+++ b/include/trace/events/writeback.h
@@ -4,6 +4,7 @@
#if !defined(_TRACE_WRITEBACK_H) || defined(TRACE_HEADER_MULTI_READ)
#define _TRACE_WRITEBACK_H
+#include <linux/tracepoint.h>
#include <linux/backing-dev.h>
#include <linux/writeback.h>
--
1.8.5.3
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [RFA][PATCH 2/4] tracing: Fix event header migrate.h to include tracepoint.h
2014-02-26 19:01 [RFA][PATCH 0/4] tracing: Request for acks on fixing tracepoint code Steven Rostedt
2014-02-26 19:01 ` [RFA][PATCH 1/4] tracing: Fix event header writeback.h to include tracepoint.h Steven Rostedt
@ 2014-02-26 19:01 ` Steven Rostedt
2014-02-28 11:22 ` Mel Gorman
2014-02-26 19:01 ` [RFA][PATCH 3/4] tracing/module: Remove include of tracepoint.h from module.h Steven Rostedt
` (2 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Steven Rostedt @ 2014-02-26 19:01 UTC (permalink / raw)
To: linux-kernel
Cc: Ingo Molnar, Andrew Morton, Peter Zijlstra, Frederic Weisbecker,
Mathieu Desnoyers, Mel Gorman
[-- Attachment #1: 0002-tracing-Fix-event-header-migrate.h-to-include-tracep.patch --]
[-- Type: text/plain, Size: 1024 bytes --]
[ Request for Ack ]
From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
The trace event headers are required to include tracepoint.h. The only reason
they worked now is because module.h included tracepoint.h, and that will soon
change. (And the change will be going to the stable tree as well)
Fixes: 7b2a2d4a18ff "mm: migrate: Add a tracepoint for migrate_pages"
Cc: stable@vger.kernel.org # 3.8+
Cc: Mel Gorman <mgorman@suse.de>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
include/trace/events/migrate.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/trace/events/migrate.h b/include/trace/events/migrate.h
index 3075ffb..4e4f2f8 100644
--- a/include/trace/events/migrate.h
+++ b/include/trace/events/migrate.h
@@ -4,6 +4,8 @@
#if !defined(_TRACE_MIGRATE_H) || defined(TRACE_HEADER_MULTI_READ)
#define _TRACE_MIGRATE_H
+#include <linux/tracepoint.h>
+
#define MIGRATE_MODE \
{MIGRATE_ASYNC, "MIGRATE_ASYNC"}, \
{MIGRATE_SYNC_LIGHT, "MIGRATE_SYNC_LIGHT"}, \
--
1.8.5.3
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [RFA][PATCH 3/4] tracing/module: Remove include of tracepoint.h from module.h
2014-02-26 19:01 [RFA][PATCH 0/4] tracing: Request for acks on fixing tracepoint code Steven Rostedt
2014-02-26 19:01 ` [RFA][PATCH 1/4] tracing: Fix event header writeback.h to include tracepoint.h Steven Rostedt
2014-02-26 19:01 ` [RFA][PATCH 2/4] tracing: Fix event header migrate.h " Steven Rostedt
@ 2014-02-26 19:01 ` Steven Rostedt
2014-02-27 3:13 ` Steven Rostedt
2014-02-26 19:01 ` [RFA][PATCH 4/4] tracing: Do not add event files for modules that fail tracepoints Steven Rostedt
2014-02-26 21:36 ` [RFA][PATCH 0/4] tracing: Request for acks on fixing tracepoint code Mathieu Desnoyers
4 siblings, 1 reply; 13+ messages in thread
From: Steven Rostedt @ 2014-02-26 19:01 UTC (permalink / raw)
To: linux-kernel
Cc: Ingo Molnar, Andrew Morton, Peter Zijlstra, Frederic Weisbecker,
Mathieu Desnoyers, Rusty Russell
[-- Attachment #1: 0003-tracing-module-Remove-include-of-tracepoint.h-from-m.patch --]
[-- Type: text/plain, Size: 952 bytes --]
[ Request for Ack ]
From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
There's nothing in the module.h header that requires tracepoint.h to be
included. Soon, tracepoint.h will require struct module, and will have to
include module.h. To avoid include hell, we need to remove the include of
tracepoint.h from module.h.
A stable patch will require this change, so include stable in the Cc.
Cc: stable@vger.kernel.org # 2.6.31+
Cc: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
include/linux/module.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index eaf60ff..6cc28d9 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -15,7 +15,6 @@
#include <linux/stringify.h>
#include <linux/kobject.h>
#include <linux/moduleparam.h>
-#include <linux/tracepoint.h>
#include <linux/export.h>
#include <linux/percpu.h>
--
1.8.5.3
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [RFA][PATCH 4/4] tracing: Do not add event files for modules that fail tracepoints
2014-02-26 19:01 [RFA][PATCH 0/4] tracing: Request for acks on fixing tracepoint code Steven Rostedt
` (2 preceding siblings ...)
2014-02-26 19:01 ` [RFA][PATCH 3/4] tracing/module: Remove include of tracepoint.h from module.h Steven Rostedt
@ 2014-02-26 19:01 ` Steven Rostedt
2014-02-26 21:36 ` [RFA][PATCH 0/4] tracing: Request for acks on fixing tracepoint code Mathieu Desnoyers
4 siblings, 0 replies; 13+ messages in thread
From: Steven Rostedt @ 2014-02-26 19:01 UTC (permalink / raw)
To: linux-kernel
Cc: Ingo Molnar, Andrew Morton, Peter Zijlstra, Frederic Weisbecker,
Mathieu Desnoyers, Rusty Russell
[-- Attachment #1: 0004-tracing-Do-not-add-event-files-for-modules-that-fail.patch --]
[-- Type: text/plain, Size: 2667 bytes --]
From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
If a module fails to add its tracepoints due to module tainting, do not
create the module event infrastructure in the debugfs directory. As the events
will not work and worse yet, they will silently fail, making the user wonder
why the events they enable do not display anything.
Having a warning on module load and the events not visible to the users
will make the cause of the problem much clearer.
Fixes: 6d723736e472 "tracing/events: add support for modules to TRACE_EVENT"
Cc: stable@vger.kernel.org # 2.6.31+
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
include/linux/tracepoint.h | 7 +++++++
kernel/trace/trace_events.c | 4 ++++
kernel/tracepoint.c | 2 +-
3 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index accc497..3cb2842 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -16,6 +16,7 @@
#include <linux/errno.h>
#include <linux/types.h>
+#include <linux/module.h>
#include <linux/rcupdate.h>
#include <linux/static_key.h>
@@ -60,6 +61,12 @@ struct tp_module {
unsigned int num_tracepoints;
struct tracepoint * const *tracepoints_ptrs;
};
+static inline bool trace_module_has_bad_taint(struct module *mod)
+{
+ return mod->taints & ~((1 << TAINT_OOT_MODULE) | (1 << TAINT_CRAP));
+}
+#else
+#define trace_module_has_bad_taint(mod) ({false;})
#endif /* CONFIG_MODULES */
struct tracepoint_iter {
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index e71ffd4..b2fee73 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -1777,6 +1777,10 @@ static void trace_module_add_events(struct module *mod)
{
struct ftrace_event_call **call, **start, **end;
+ /* Don't add infrastructure for mods without tracepoints */
+ if (trace_module_has_bad_taint(mod))
+ return;
+
start = mod->trace_events;
end = mod->trace_events + mod->num_trace_events;
diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index 8a5d5e95..f74e953 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -644,7 +644,7 @@ static int tracepoint_module_coming(struct module *mod)
* module headers (for forced load), to make sure we don't cause a crash.
* Staging and out-of-tree GPL modules are fine.
*/
- if (mod->taints & ~((1 << TAINT_OOT_MODULE) | (1 << TAINT_CRAP))) {
+ if (trace_module_has_bad_taint(mod)) {
pr_err("Module '%s' is tainted, disabling tracepoints\n",
mod->name);
return 0;
--
1.8.5.3
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [RFA][PATCH 0/4] tracing: Request for acks on fixing tracepoint code
2014-02-26 19:01 [RFA][PATCH 0/4] tracing: Request for acks on fixing tracepoint code Steven Rostedt
` (3 preceding siblings ...)
2014-02-26 19:01 ` [RFA][PATCH 4/4] tracing: Do not add event files for modules that fail tracepoints Steven Rostedt
@ 2014-02-26 21:36 ` Mathieu Desnoyers
2014-02-27 1:38 ` Steven Rostedt
4 siblings, 1 reply; 13+ messages in thread
From: Mathieu Desnoyers @ 2014-02-26 21:36 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Peter Zijlstra,
Frederic Weisbecker
----- Original Message -----
> From: "Steven Rostedt" <rostedt@goodmis.org>
> To: linux-kernel@vger.kernel.org
> Cc: "Ingo Molnar" <mingo@kernel.org>, "Andrew Morton" <akpm@linux-foundation.org>, "Peter Zijlstra"
> <peterz@infradead.org>, "Frederic Weisbecker" <fweisbec@gmail.com>, "Mathieu Desnoyers"
> <mathieu.desnoyers@efficios.com>
> 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.
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_*).
Thoughts ?
Thanks,
Mathieu
>
> Steven Rostedt (Red Hat) (4):
> tracing: Fix event header writeback.h to include tracepoint.h
> tracing: Fix event header migrate.h to include tracepoint.h
> tracing/module: Remove include of tracepoint.h from module.h
> tracing: Do not add event files for modules that fail tracepoints
>
> ----
> include/linux/module.h | 1 -
> include/linux/tracepoint.h | 7 +++++++
> include/trace/events/migrate.h | 2 ++
> include/trace/events/writeback.h | 1 +
> kernel/trace/trace_events.c | 4 ++++
> kernel/tracepoint.c | 2 +-
> 6 files changed, 23 insertions(+), 21 deletions(-)
>
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFA][PATCH 0/4] tracing: Request for acks on fixing tracepoint code
2014-02-26 21:36 ` [RFA][PATCH 0/4] tracing: Request for acks on fixing tracepoint code Mathieu Desnoyers
@ 2014-02-27 1:38 ` Steven Rostedt
2014-02-27 2:21 ` Mathieu Desnoyers
0 siblings, 1 reply; 13+ messages in thread
From: Steven Rostedt @ 2014-02-27 1:38 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Peter Zijlstra,
Frederic Weisbecker
On Wed, 26 Feb 2014 21:36:18 +0000 (UTC)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> ----- Original Message -----
> > From: "Steven Rostedt" <rostedt@goodmis.org>
> > To: linux-kernel@vger.kernel.org
> > Cc: "Ingo Molnar" <mingo@kernel.org>, "Andrew Morton" <akpm@linux-foundation.org>, "Peter Zijlstra"
> > <peterz@infradead.org>, "Frederic Weisbecker" <fweisbec@gmail.com>, "Mathieu Desnoyers"
> > <mathieu.desnoyers@efficios.com>
> > 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFA][PATCH 0/4] tracing: Request for acks on fixing tracepoint code
2014-02-27 1:38 ` Steven Rostedt
@ 2014-02-27 2:21 ` Mathieu Desnoyers
2014-02-27 2:43 ` Steven Rostedt
0 siblings, 1 reply; 13+ messages in thread
From: Mathieu Desnoyers @ 2014-02-27 2:21 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Peter Zijlstra,
Frederic Weisbecker
----- Original Message -----
> From: "Steven Rostedt" <rostedt@goodmis.org>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> Cc: linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@kernel.org>, "Andrew Morton" <akpm@linux-foundation.org>,
> "Peter Zijlstra" <peterz@infradead.org>, "Frederic Weisbecker" <fweisbec@gmail.com>
> Sent: Wednesday, February 26, 2014 8:38:53 PM
> Subject: Re: [RFA][PATCH 0/4] tracing: Request for acks on fixing tracepoint code
>
> On Wed, 26 Feb 2014 21:36:18 +0000 (UTC)
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>
> > ----- Original Message -----
> > > From: "Steven Rostedt" <rostedt@goodmis.org>
> > > To: linux-kernel@vger.kernel.org
> > > Cc: "Ingo Molnar" <mingo@kernel.org>, "Andrew Morton"
> > > <akpm@linux-foundation.org>, "Peter Zijlstra"
> > > <peterz@infradead.org>, "Frederic Weisbecker" <fweisbec@gmail.com>,
> > > "Mathieu Desnoyers"
> > > <mathieu.desnoyers@efficios.com>
> > > 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.
That is not the question. We don't care about how many times module.h is
included in the kernel, but rather what module.h itself includes and could
include in the future, throughout generic and arch-specific headers. If
someone want to add a tracepoint in a static inline function located within
a header file, they will need to include tracepoint.h. If tracepoint.h
happens to have a circular dependency on this header, there comes include
hell.
Arguing that it's OK to include headers within core instrumentation code
because they are themselves included pretty much everywhere is a paved way
to said include hell IMHO.
>
> >
> > 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.
I just recalled we already have things like "DECLARE_TRACE" and such in
tracepoint.h. I'm OK with trace_module_has_bad_taint() then.
>
> But as a compromise, I can move it to ftrace_event.h instead.
Since it will be used in tracepoint.c as well, which is a foundation of
ftrace_event, it would be bad coupling to make tracepoint.c include
ftrace_event.h (abstraction inversion). So I still think tracepoint.h
is the right place to put this, only not with the module.h dependency.
But perhaps I'm missing something. Why is it so important to you to make
this a static inline rather than a regular function call ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFA][PATCH 0/4] tracing: Request for acks on fixing tracepoint code
2014-02-27 2:21 ` Mathieu Desnoyers
@ 2014-02-27 2:43 ` Steven Rostedt
0 siblings, 0 replies; 13+ messages in thread
From: Steven Rostedt @ 2014-02-27 2:43 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Peter Zijlstra,
Frederic Weisbecker, Rusty Russell
On Thu, 27 Feb 2014 02:21:16 +0000 (UTC)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> That is not the question. We don't care about how many times module.h is
> included in the kernel, but rather what module.h itself includes and could
> include in the future, throughout generic and arch-specific headers. If
> someone want to add a tracepoint in a static inline function located within
> a header file, they will need to include tracepoint.h. If tracepoint.h
> happens to have a circular dependency on this header, there comes include
> hell.
Actually, we've been telling people not to do that. That is, we don't
allow for tracepoints to be in headers anymore. We've removed all of
the offenders.
>
> Arguing that it's OK to include headers within core instrumentation code
> because they are themselves included pretty much everywhere is a paved way
> to said include hell IMHO.
I find tracepoint.h far from a core instrumentation code. It's for
static tracepoints only. It's not like rcu or spinlock. It's about as
core as module.h is. I would argue that module.h is an even more core
header than tracepoint.h.
> >
> > But as a compromise, I can move it to ftrace_event.h instead.
>
> Since it will be used in tracepoint.c as well, which is a foundation of
> ftrace_event, it would be bad coupling to make tracepoint.c include
> ftrace_event.h (abstraction inversion). So I still think tracepoint.h
> is the right place to put this, only not with the module.h dependency.
>
> But perhaps I'm missing something. Why is it so important to you to make
> this a static inline rather than a regular function call ?
As it is a one liner check, it just seemed to fit as a static inline.
As this isn't in any fast path, I guess I could make it a normal
function call.
Fine, I'll do that instead. Then the module header change can go into
linux-next instead of mainline/stable.
-- Steve
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFA][PATCH 3/4] tracing/module: Remove include of tracepoint.h from module.h
2014-02-26 19:01 ` [RFA][PATCH 3/4] tracing/module: Remove include of tracepoint.h from module.h Steven Rostedt
@ 2014-02-27 3:13 ` Steven Rostedt
2014-03-17 2:34 ` Rusty Russell
0 siblings, 1 reply; 13+ messages in thread
From: Steven Rostedt @ 2014-02-27 3:13 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Peter Zijlstra,
Frederic Weisbecker, Mathieu Desnoyers, Rusty Russell
On Wed, 26 Feb 2014 14:01:43 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> [ Request for Ack ]
>
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
>
> There's nothing in the module.h header that requires tracepoint.h to be
> included. Soon, tracepoint.h will require struct module, and will have to
> include module.h. To avoid include hell, we need to remove the include of
> tracepoint.h from module.h.
>
> A stable patch will require this change, so include stable in the Cc.
>
> Cc: stable@vger.kernel.org # 2.6.31+
Hi Rusty,
This patch doesn't need to be stable, and can wait till v3.15. But I
have other patches that will break with this patch (headers that needed
to include tracepoint.h and not depend on a header chain to include it).
Can you give me your Acked-by for this, and I'll just add it to my 3.15
queue?
Thanks,
-- Steve
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
> include/linux/module.h | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index eaf60ff..6cc28d9 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -15,7 +15,6 @@
> #include <linux/stringify.h>
> #include <linux/kobject.h>
> #include <linux/moduleparam.h>
> -#include <linux/tracepoint.h>
> #include <linux/export.h>
>
> #include <linux/percpu.h>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFA][PATCH 2/4] tracing: Fix event header migrate.h to include tracepoint.h
2014-02-26 19:01 ` [RFA][PATCH 2/4] tracing: Fix event header migrate.h " Steven Rostedt
@ 2014-02-28 11:22 ` Mel Gorman
0 siblings, 0 replies; 13+ messages in thread
From: Mel Gorman @ 2014-02-28 11:22 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Peter Zijlstra,
Frederic Weisbecker, Mathieu Desnoyers
On Wed, Feb 26, 2014 at 02:01:42PM -0500, Steven Rostedt wrote:
> [ Request for Ack ]
>
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
>
> The trace event headers are required to include tracepoint.h. The only reason
> they worked now is because module.h included tracepoint.h, and that will soon
> change. (And the change will be going to the stable tree as well)
>
> Fixes: 7b2a2d4a18ff "mm: migrate: Add a tracepoint for migrate_pages"
> Cc: stable@vger.kernel.org # 3.8+
> Cc: Mel Gorman <mgorman@suse.de>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Acked-by: Mel Gorman <mgorman@suse.de>
--
Mel Gorman
SUSE Labs
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFA][PATCH 3/4] tracing/module: Remove include of tracepoint.h from module.h
2014-02-27 3:13 ` Steven Rostedt
@ 2014-03-17 2:34 ` Rusty Russell
2014-03-17 6:42 ` Steven Rostedt
0 siblings, 1 reply; 13+ messages in thread
From: Rusty Russell @ 2014-03-17 2:34 UTC (permalink / raw)
To: Steven Rostedt, Steven Rostedt
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Peter Zijlstra,
Frederic Weisbecker, Mathieu Desnoyers
Steven Rostedt <rostedt@goodmis.org> writes:
> On Wed, 26 Feb 2014 14:01:43 -0500
> Hi Rusty,
>
> This patch doesn't need to be stable, and can wait till v3.15. But I
> have other patches that will break with this patch (headers that needed
> to include tracepoint.h and not depend on a header chain to include it).
>
> Can you give me your Acked-by for this, and I'll just add it to my 3.15
> queue?
Cleaning up old mail, in case I didn't ack this:
Acked-by: Rusty Russell <rusty@rustcorp.com.au>
Cheers,
Rusty.
>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
>> ---
>> include/linux/module.h | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/include/linux/module.h b/include/linux/module.h
>> index eaf60ff..6cc28d9 100644
>> --- a/include/linux/module.h
>> +++ b/include/linux/module.h
>> @@ -15,7 +15,6 @@
>> #include <linux/stringify.h>
>> #include <linux/kobject.h>
>> #include <linux/moduleparam.h>
>> -#include <linux/tracepoint.h>
>> #include <linux/export.h>
>>
>> #include <linux/percpu.h>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFA][PATCH 3/4] tracing/module: Remove include of tracepoint.h from module.h
2014-03-17 2:34 ` Rusty Russell
@ 2014-03-17 6:42 ` Steven Rostedt
0 siblings, 0 replies; 13+ messages in thread
From: Steven Rostedt @ 2014-03-17 6:42 UTC (permalink / raw)
To: Rusty Russell
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Peter Zijlstra,
Frederic Weisbecker, Mathieu Desnoyers
On Mon, 2014-03-17 at 13:04 +1030, Rusty Russell wrote:
> Steven Rostedt <rostedt@goodmis.org> writes:
> > On Wed, 26 Feb 2014 14:01:43 -0500
> > Hi Rusty,
> >
> > This patch doesn't need to be stable, and can wait till v3.15. But I
> > have other patches that will break with this patch (headers that needed
> > to include tracepoint.h and not depend on a header chain to include it).
> >
> > Can you give me your Acked-by for this, and I'll just add it to my 3.15
> > queue?
>
> Cleaning up old mail, in case I didn't ack this:
>
> Acked-by: Rusty Russell <rusty@rustcorp.com.au>
Yep, you already gave me your ack. But I appreciate the double ack :-)
-- Steve
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-03-17 6:43 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-02-26 19:01 [RFA][PATCH 0/4] tracing: Request for acks on fixing tracepoint code Steven Rostedt
2014-02-26 19:01 ` [RFA][PATCH 1/4] tracing: Fix event header writeback.h to include tracepoint.h Steven Rostedt
2014-02-26 19:01 ` [RFA][PATCH 2/4] tracing: Fix event header migrate.h " Steven Rostedt
2014-02-28 11:22 ` Mel Gorman
2014-02-26 19:01 ` [RFA][PATCH 3/4] tracing/module: Remove include of tracepoint.h from module.h Steven Rostedt
2014-02-27 3:13 ` Steven Rostedt
2014-03-17 2:34 ` Rusty Russell
2014-03-17 6:42 ` Steven Rostedt
2014-02-26 19:01 ` [RFA][PATCH 4/4] tracing: Do not add event files for modules that fail tracepoints Steven Rostedt
2014-02-26 21:36 ` [RFA][PATCH 0/4] tracing: Request for acks on fixing tracepoint code Mathieu Desnoyers
2014-02-27 1:38 ` Steven Rostedt
2014-02-27 2:21 ` Mathieu Desnoyers
2014-02-27 2:43 ` Steven Rostedt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox