tracing: Request for acks on fixing tracepoint code

PATCH In review Steven Rostedt 4 participants 13 messages Feb 26, 2014 – Mar 17, 2014

Subsystems (3)

Tracing Page cache Module support

lore

Share & tools

lore

Steven Rostedt

author maintainer

2014-02-26 19:06 UTC #0 lore raw

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

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

Steven Rostedt

author maintainer

2014-02-26 19:08 UTC #1 lore raw

PATCH 1/4 tracing: Fix event header writeback.h to include tracepoint.h

[ Request for Ack ]

From: "Steven Rostedt (Red Hat)" <[email protected]>

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 [email protected] # 2.6.36+

Cc Dave Chinner <[email protected]>

Cc Jens Axboe <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
 include/trace/events/writeback.h | 1 +
 1 file changed, 1 insertion(+)

include/trace/events/writeback.h +1 -0

index c7bbbe7..309a086 100644
@@ -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>

Signature · 1 line

1.8.5.3

Steven Rostedt

author maintainer

2014-02-26 19:06 UTC #2 lore raw

PATCH 2/4 tracing: Fix event header migrate.h to include tracepoint.h

In reply to Steven Rostedt

[ Request for Ack ]

From: "Steven Rostedt (Red Hat)" <[email protected]>

Fixes 7b2a2d4a18ff "mm: migrate: Add a tracepoint for migrate_pages"

Cc [email protected] # 3.8+

Cc Mel Gorman <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
 include/trace/events/migrate.h | 2 ++
 1 file changed, 2 insertions(+)

include/trace/events/migrate.h +2 -0

index 3075ffb..4e4f2f8 100644
@@ -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"},		\

Signature · 1 line

1.8.5.3

Mel Gorman

Acked-by on v1 2/4

2014-02-28 11:23 UTC #3 lore raw

Re: [RFA][PATCH 2/4] tracing: Fix event header migrate.h to include tracepoint.h

Quoted 4 lines · Steven Rostedt

[ Request for Ack ]
From: "Steven Rostedt (Red Hat)" <[email protected]>

Quoted 8 lines

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 [email protected] # 3.8+
Cc Mel Gorman <[email protected]>
Signed-off-by Steven Rostedt <[email protected]>

Acked-by Mel Gorman <[email protected]>

Signature · 2 lines

Mel Gorman
SUSE Labs

Steven Rostedt

author maintainer

2014-02-26 19:06 UTC #4 lore raw

PATCH 3/4 tracing/module: Remove include of tracepoint.h from module.h

In reply to Steven Rostedt

[ Request for Ack ]

From: "Steven Rostedt (Red Hat)" <[email protected]>

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 [email protected] # 2.6.31+

Cc Rusty Russell <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
 include/linux/module.h | 1 -
 1 file changed, 1 deletion(-)

include/linux/module.h +0 -1

index eaf60ff..6cc28d9 100644
@@ -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>

Signature · 1 line

1.8.5.3

Steven Rostedt

author maintainer

2014-02-27 03:13 UTC #5 lore raw

Re: [RFA][PATCH 3/4] tracing/module: Remove include of tracepoint.h from module.h

Quoted 4 lines · On Wed, 26 Feb 2014 14:01:43 -0500 Steven Rostedt <[email protected]> wrote:

[ Request for Ack ]
From: "Steven Rostedt (Red Hat)" <[email protected]>

Quoted 8 lines

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 [email protected] # 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

Quoted 18 lines

Cc Rusty Russell <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
 include/linux/module.h | 1 -
 1 file changed, 1 deletion(-)

include/linux/module.h +0 -1

index eaf60ff..6cc28d9 100644
@@ -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>

Rusty Russell

Acked-by

2014-03-17 04:39 UTC #6 lore raw

Re: [RFA][PATCH 3/4] tracing/module: Remove include of tracepoint.h from module.h

Quoted 9 lines · Steven Rostedt

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 <[email protected]>

Cheers,
Rusty.

Quoted 18 lines

Cc Rusty Russell <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
 include/linux/module.h | 1 -
 1 file changed, 1 deletion(-)

include/linux/module.h +0 -1

index eaf60ff..6cc28d9 100644
@@ -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>

Steven Rostedt

author maintainer

2014-03-17 06:43 UTC #7 lore raw

Re: [RFA][PATCH 3/4] tracing/module: Remove include of tracepoint.h from module.h

Quoted 8 lines · Rusty Russell

Quoted 7 lines · Steven Rostedt
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).

Quoted 6 lines

Quoted 2 lines
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 <[email protected]>

Yep, you already gave me your ack. But I appreciate the double ack :-)

-- Steve

Steven Rostedt

author maintainer

2014-02-26 19:06 UTC #8 lore raw

PATCH 4/4 tracing: Do not add event files for modules that fail tracepoints

In reply to Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>

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 [email protected] # 2.6.31+

Cc Rusty Russell <[email protected]>

Cc Mathieu Desnoyers <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
 include/linux/tracepoint.h  | 7 +++++++
 kernel/trace/trace_events.c | 4 ++++
 kernel/tracepoint.c         | 2 +-
 3 files changed, 12 insertions(+), 1 deletion(-)

include/linux/tracepoint.h +7 -0

index accc497..3cb2842 100644
@@ -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 {

kernel/trace/trace_events.c +4 -0

index e71ffd4..b2fee73 100644
@@ -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;

kernel/tracepoint.c +1 -1

index 8a5d5e95..f74e953 100644
@@ -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;

Signature · 1 line

1.8.5.3

Mathieu Desnoyers

reviewer

2014-02-26 21:36 UTC #9 lore raw

In reply to Steven Rostedt

----- Original Message -----

Quoted 15 lines

From: "Steven Rostedt" <[email protected]>
To: [email protected]
Cc: "Ingo Molnar" <[email protected]>, "Andrew Morton" <[email protected]>, "Peter Zijlstra" <[email protected]>, "Frederic Weisbecker" <[email protected]>, "Mathieu Desnoyers" <[email protected]>
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.

Quoted 8 lines

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

Quoted 16 lines

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

Signature · 3 lines

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

Steven Rostedt

author maintainer

2014-02-27 01:38 UTC #10 lore raw

Quoted 25 lines · On Wed, 26 Feb 2014 21:36:18 +0000 (UTC) Mathieu Desnoyers…

----- Original Message -----
Quoted 15 lines
From: "Steven Rostedt" <[email protected]>
To: [email protected]
Cc: "Ingo Molnar" <[email protected]>, "Andrew Morton" <[email protected]>, "Peter Zijlstra" <[email protected]>, "Frederic Weisbecker" <[email protected]>, "Mathieu Desnoyers" <[email protected]>
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.
Quoted 8 lines
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.

Quoted 4 lines

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.

Quoted 8 lines

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

Mathieu Desnoyers

reviewer

2014-02-27 02:21 UTC #11 lore raw

----- Original Message -----

Quoted 39 lines

From: "Steven Rostedt" <[email protected]>
To: "Mathieu Desnoyers" <[email protected]>
Cc: [email protected], "Ingo Molnar" <[email protected]>, "Andrew Morton" <[email protected]>, "Peter Zijlstra" <[email protected]>, "Frederic Weisbecker" <[email protected]>
Sent: Wednesday, February 26, 2014 8:38:53 PM
Subject: Re: [RFA][PATCH 0/4] tracing: Request for acks on fixing tracepoint code
Quoted 29 lines · On Wed, 26 Feb 2014 21:36:18 +0000 (UTC) Mathieu Desnoyers…
----- Original Message -----
Quoted 27 lines
From: "Steven Rostedt" <[email protected]>
To: [email protected]
Cc: "Ingo Molnar" <[email protected]>, "Andrew Morton" <[email protected]>, "Peter Zijlstra" <[email protected]>, "Frederic Weisbecker" <[email protected]>, "Mathieu Desnoyers"
<[email protected]>
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.

Quoted 6 lines

Quoted 4 lines
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.

Quoted 8 lines

Quoted 8 lines
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.

Quoted 5 lines

Quoted 1 line
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.

Quoted 2 lines

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

Signature · 3 lines

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

Steven Rostedt

author maintainer

2014-02-27 02:43 UTC #12 lore raw

Quoted 7 lines · On Thu, 27 Feb 2014 02:21:16 +0000 (UTC) Mathieu Desnoyers…

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.

Quoted 4 lines

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.

Quoted 10 lines

Quoted 2 lines
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