* [PATCH][GIT PULL][v3.3] tracing: Add header wrappers event_headers_start.h and event_headers_end.h
@ 2012-01-16 22:57 Steven Rostedt
2012-01-17 9:54 ` Ingo Molnar
0 siblings, 1 reply; 31+ messages in thread
From: Steven Rostedt @ 2012-01-16 22:57 UTC (permalink / raw)
To: LKML; +Cc: Ingo Molnar, Andrew Morton, Frederic Weisbecker
Ingo,
This is actually a special pull request. This patch adds two new files
that are not used by anyone. The rational for this is, they will be
required for v3.4 and I want to make the transition in linux-next as
smooth as possible. Let me explain the situation.
There has been more and more cases where trace/events/*.h headers have
been showing up in normal header files. This is fine unless one of these
normal header files ends up included in another trace/events/*.h file.
What happens then, is when the CREATE_TRACE_POINTS is defined both
headers get evaluated. This means the C functions to create the
tracepoints are created for both the initial trace/events/*.h header, as
well as the one that got included by the normal header file. This makes
the build fail. We've already had to fix this up a few times to handle
these cases.
I added two header files:
include/trace/event_headers_start.h
include/trace/event_headers_end.h
These headers add some macro magic to handle the nested tracepoint event
headers that was described above.
The way this works is that all tracepoint event headers must include
these two headers around their other includes. For example trace/sched.h
will now have:
#include <trace/event_headers_start.h>
#include <linux/sched.h>
#include <linux/tracepoint.h>
#include <trace/event_headers_end.h>
I've already updated all the tracepoint headers inside the latest
kernel. I searched all headers with "TRACE_EVENT" in them to catch
headers outside of trace/events/ that define trace event headers.
The issue I see, and why I want this patch into 3.3 is that I have a
warning that will print if someone adds a new tracepoint event header
and doesn't add these files. If this warning goes into linux-next, and
someone adds a new tracepoint event header, they will start getting this
warning. The only way for them to stop it, is to include the above
wrappers. The problem is, the wrappers will not exist in the kernel that
gets pulled into linux-next, unless we push them now into 3.3.
Now if you feel this is too much and do not want to include files into
3.3 that are not being used, I can hold off the warning patch until 3.5,
and then we may have a mixture of files with and without these header
wrappers in 3.4.
What's your thoughts on this?
-- Steve
Please pull the latest tip/perf/urgent-3 tree, which can be found at:
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
tip/perf/urgent-3
Head SHA1: 8bc9bfdfd80d28b0902ee168b9b2c181e7e37c35
Steven Rostedt (1):
tracing: Add header wrappers event_headers_start.h and event_headers_end.h
----
include/trace/event_headers_end.h | 41 ++++++++++++++++++++
include/trace/event_headers_start.h | 71 +++++++++++++++++++++++++++++++++++
2 files changed, 112 insertions(+), 0 deletions(-)
---------------------------
commit 8bc9bfdfd80d28b0902ee168b9b2c181e7e37c35
Author: Steven Rostedt <srostedt@redhat.com>
Date: Mon Jan 16 17:00:19 2012 -0500
tracing: Add header wrappers event_headers_start.h and event_headers_end.h
As more tracepoints are created, more are being added inside of header
files to add tracepoints into static inline functions. If another tracepoint
header includes a header with a tracepoint header within it, it can break
the build.
The reason is that the tracepoint headers create their C functions when the
CREATE_TRACE_POINT macro is defined. But if the tracepoint header includes
another tracepoint header, the C functions for that second tracepoint header
will also be defined, causing the C functions to be defined in more than one
location.
To prevent this, two headers were created that must be wrapped around
all headers within tracepoint headers. These wrapper headers has some logic
to deal with the CREATE_TRACE_POINTS being set, and undefines it. The
second wrapper header will define it again.
This is only part of the solution. The other part is that all tracepoint headers
that are included within normal headers, must also wrap the TRACE_SYSTEM
macro define with #ifdef CONFIG_TRACE_POINTS. This second change will come
later.
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
diff --git a/include/trace/event_headers_end.h b/include/trace/event_headers_end.h
new file mode 100644
index 0000000..a5bd8f4
--- /dev/null
+++ b/include/trace/event_headers_end.h
@@ -0,0 +1,41 @@
+
+/* See event_headers_start.h for details. */
+
+#ifdef REENABLE_CREATE_TRACE_POINTS
+
+/* must be a better way to decrement a macro counter */
+
+# if REENABLE_CREATE_TRACE_POINTS == 0
+
+# elif REENABLE_CREATE_TRACE_POINTS == 1
+# define CREATE_TRACE_POINTS
+
+/*
+ * Keeping REENABLE_CREATE_TRACE_POINTS undefined
+ * will cause the rest of the #elif to fail.
+ * Set it to zero, it will act the same as undefined.
+ */
+# undef REENABLE_CREATE_TRACE_POINTS
+# define REENABLE_CREATE_TRACE_POINTS 0
+
+/*
+ * Would be nice to use elif here, but it seems that the
+ * above 'undef' or any redefining the
+ */
+# elif REENABLE_CREATE_TRACE_POINTS == 2
+# undef REENABLE_CREATE_TRACE_POINTS
+# define REENABLE_CREATE_TRACE_POINTS 1
+
+# elif REENABLE_CREATE_TRACE_POINTS == 3
+# undef REENABLE_CREATE_TRACE_POINTS
+# define REENABLE_CREATE_TRACE_POINTS 2
+
+# elif REENABLE_CREATE_TRACE_POINTS == 4
+# undef REENABLE_CREATE_TRACE_POINTS
+# define REENABLE_CREATE_TRACE_POINTS 3
+
+# else
+# error Bad REENABLE_CREATE_TRACE_POINTS number
+# endif
+
+#endif
diff --git a/include/trace/event_headers_start.h b/include/trace/event_headers_start.h
new file mode 100644
index 0000000..063df69
--- /dev/null
+++ b/include/trace/event_headers_start.h
@@ -0,0 +1,71 @@
+/*
+ * Because some trace/events/foo.h headers are included in normal headers,
+ * compiling can fail if one of these normal headers is included inside
+ * another trace/events/foo.h header. This is because when CREATE_TRACE_POINTS
+ * is defined, the trace/events/foo.h header file becomes activated to
+ * create the necessary C functions to use with tracepoints.
+ *
+ * The problem is that one trace/events/foo.h header will create the
+ * tracepoints for both included headers, and when the other header
+ * is called with CREATE_TRACE_POINTS, it will create duplicated functions
+ * and cause the build to fail.
+ *
+ * To allow other headers to be nested, we need to save the nesting level
+ * of these calls (REENABLE_CREATE_TRACE_POINTS) and disable the
+ * CREATE_TRACE_POINTS while the headers are nested.
+ *
+ * For this to work, all trace/events/foo.h headers should wrap all their
+ * headers with:
+ *
+ * #include <trace/event_headers_start.h>
+ * [...]
+ * #include <trace/event_headers_end.h>
+ *
+ * Unfortunately, macros are evaluate at the location of their use
+ * and not at the #define, so we need to create our own counter.
+ * We support up to 4 nested trace/events/foo.h headers, which should
+ * be way more than enough.
+ */
+#ifdef CREATE_TRACE_POINTS
+
+#undef CREATE_TRACE_POINTS
+
+#ifdef REENABLE_CREATE_TRACE_POINTS
+# if REENABLE_CREATE_TRACE_POINTS != 0
+# error Problem calculating REENABLE_CREATE_TRACE_POINTS
+# endif
+# undef REENABLE_CREATE_TRACE_POINTS
+#endif
+
+#define REENABLE_CREATE_TRACE_POINTS 1
+
+#else
+
+# ifdef REENABLE_CREATE_TRACE_POINTS
+/* Must be a better way to increment a macro counter */
+
+/*
+ * We allow four levels of nested event headers, which should
+ * be way more than enough.
+ */
+# if REENABLE_CREATE_TRACE_POINTS == 0
+/* Do nothing. */
+
+# elif REENABLE_CREATE_TRACE_POINTS == 1
+# undef REENABLE_CREATE_TRACE_POINTS
+# define REENABLE_CREATE_TRACE_POINTS 2
+
+# elif REENABLE_CREATE_TRACE_POINTS == 2
+# undef REENABLE_CREATE_TRACE_POINTS
+# define REENABLE_CREATE_TRACE_POINTS 3
+
+# elif REENABLE_CREATE_TRACE_POINTS == 3
+# undef REENABLE_CREATE_TRACE_POINTS
+# define REENABLE_CREATE_TRACE_POINTS 4
+
+# else
+# error Too many nested trace/events headers
+# endif
+# endif
+
+#endif
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH][GIT PULL][v3.3] tracing: Add header wrappers event_headers_start.h and event_headers_end.h
2012-01-16 22:57 [PATCH][GIT PULL][v3.3] tracing: Add header wrappers event_headers_start.h and event_headers_end.h Steven Rostedt
@ 2012-01-17 9:54 ` Ingo Molnar
2012-01-17 13:32 ` Steven Rostedt
0 siblings, 1 reply; 31+ messages in thread
From: Ingo Molnar @ 2012-01-17 9:54 UTC (permalink / raw)
To: Steven Rostedt; +Cc: LKML, Andrew Morton, Frederic Weisbecker
* Steven Rostedt <rostedt@goodmis.org> wrote:
>
> Ingo,
>
> This is actually a special pull request. This patch adds two new files
> that are not used by anyone. The rational for this is, they will be
> required for v3.4 and I want to make the transition in linux-next as
> smooth as possible. Let me explain the situation.
>
> There has been more and more cases where trace/events/*.h headers have
> been showing up in normal header files. This is fine unless one of these
> normal header files ends up included in another trace/events/*.h file.
> What happens then, is when the CREATE_TRACE_POINTS is defined both
> headers get evaluated. This means the C functions to create the
> tracepoints are created for both the initial trace/events/*.h header, as
> well as the one that got included by the normal header file. This makes
> the build fail. We've already had to fix this up a few times to handle
> these cases.
>
> I added two header files:
>
> include/trace/event_headers_start.h
> include/trace/event_headers_end.h
>
> These headers add some macro magic to handle the nested tracepoint event
> headers that was described above.
>
> The way this works is that all tracepoint event headers must include
> these two headers around their other includes. For example trace/sched.h
> will now have:
>
> #include <trace/event_headers_start.h>
> #include <linux/sched.h>
> #include <linux/tracepoint.h>
> #include <trace/event_headers_end.h>
>
> I've already updated all the tracepoint headers inside the latest
> kernel. I searched all headers with "TRACE_EVENT" in them to catch
> headers outside of trace/events/ that define trace event headers.
>
> The issue I see, and why I want this patch into 3.3 is that I have a
> warning that will print if someone adds a new tracepoint event header
> and doesn't add these files. If this warning goes into linux-next, and
> someone adds a new tracepoint event header, they will start getting this
> warning. The only way for them to stop it, is to include the above
> wrappers. The problem is, the wrappers will not exist in the kernel that
> gets pulled into linux-next, unless we push them now into 3.3.
>
> Now if you feel this is too much and do not want to include files into
> 3.3 that are not being used, I can hold off the warning patch until 3.5,
> and then we may have a mixture of files with and without these header
> wrappers in 3.4.
>
> What's your thoughts on this?
Hm, i don't really like the extra complexity - this code
*really* does not need any more complexity ...
How about the low-tech solution of adding some text between
'/* */' markers to warns that these headers should not be
included in ordinary headers?
The build error will pinpoint the bug anyway, it's not like the
kernel can be broken in any dangerous way.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH][GIT PULL][v3.3] tracing: Add header wrappers event_headers_start.h and event_headers_end.h
2012-01-17 9:54 ` Ingo Molnar
@ 2012-01-17 13:32 ` Steven Rostedt
2012-01-18 12:07 ` Ingo Molnar
0 siblings, 1 reply; 31+ messages in thread
From: Steven Rostedt @ 2012-01-17 13:32 UTC (permalink / raw)
To: Ingo Molnar; +Cc: LKML, Andrew Morton, Frederic Weisbecker
On Tue, 2012-01-17 at 10:54 +0100, Ingo Molnar wrote:
> * Steven Rostedt <rostedt@goodmis.org> wrote:
>
> Hm, i don't really like the extra complexity - this code
> *really* does not need any more complexity ...
I agree.
>
> How about the low-tech solution of adding some text between
> '/* */' markers to warns that these headers should not be
> included in ordinary headers?
The problem is that they currently are. For example:
include/linux/interrupt.h
has one to add a tracepoint in __raise_softirq_irqoff()
Which is fine, as long as no other tracepoint header includes
linux/interrupt.h.
There's also one in module.h.
>
> The build error will pinpoint the bug anyway, it's not like the
> kernel can be broken in any dangerous way.
OK, I'll put away these patches. For now we can try to keep all
tracepoints out of header files as much as possible. They really
shouldn't be in static inline functions anyway, because they tend to
bloat the kernel, as we saw when it was included in slab_def.h
-- Steve
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH][GIT PULL][v3.3] tracing: Add header wrappers event_headers_start.h and event_headers_end.h
2012-01-17 13:32 ` Steven Rostedt
@ 2012-01-18 12:07 ` Ingo Molnar
2012-01-18 17:56 ` Steven Rostedt
2012-02-17 13:46 ` [tip:perf/core] tracing/softirq: Move __raise_softirq_irqoff() out of header tip-bot for Steven Rostedt
0 siblings, 2 replies; 31+ messages in thread
From: Ingo Molnar @ 2012-01-18 12:07 UTC (permalink / raw)
To: Steven Rostedt; +Cc: LKML, Andrew Morton, Frederic Weisbecker
* Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 2012-01-17 at 10:54 +0100, Ingo Molnar wrote:
> > * Steven Rostedt <rostedt@goodmis.org> wrote:
> >
>
> > Hm, i don't really like the extra complexity - this code
> > *really* does not need any more complexity ...
>
> I agree.
>
> >
> > How about the low-tech solution of adding some text between
> > '/* */' markers to warns that these headers should not be
> > included in ordinary headers?
>
> The problem is that they currently are. For example:
>
> include/linux/interrupt.h
>
> has one to add a tracepoint in __raise_softirq_irqoff()
>
> Which is fine, as long as no other tracepoint header includes
> linux/interrupt.h.
Could we try to remove this one from the header?
I'd argue that __raise_softirq_irqoff() should not be inline -
that would solve a whole host of issues. Event tracing is
enabled in most distros so there's no real overhead argument to
be made here either - so it's probably a bit faster in fact to
have this uninlined. What do you think?
Thanks,
Ingo
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH][GIT PULL][v3.3] tracing: Add header wrappers event_headers_start.h and event_headers_end.h
2012-01-18 12:07 ` Ingo Molnar
@ 2012-01-18 17:56 ` Steven Rostedt
2012-01-22 22:59 ` Rusty Russell
2012-02-17 13:46 ` [tip:perf/core] tracing/softirq: Move __raise_softirq_irqoff() out of header tip-bot for Steven Rostedt
1 sibling, 1 reply; 31+ messages in thread
From: Steven Rostedt @ 2012-01-18 17:56 UTC (permalink / raw)
To: Ingo Molnar; +Cc: LKML, Andrew Morton, Frederic Weisbecker, Rusty Russell
On Wed, 2012-01-18 at 13:07 +0100, Ingo Molnar wrote:
> I'd argue that __raise_softirq_irqoff() should not be inline -
> that would solve a whole host of issues. Event tracing is
> enabled in most distros so there's no real overhead argument to
> be made here either - so it's probably a bit faster in fact to
> have this uninlined. What do you think?
Sure, I have no problem moving that out of the header. I don't think
raising a softirq is in that critical a path that it can't be a function
call.
There's only one trace/events header left which is in module.h. Perhaps
we can move __module_get() and try_module_get() out of the header. We
could just move the "if" part out.
extern void inc_module(struct module *module, unsigned long ip);
static inline void __module_get(struct module *module)
{
if (module) {
preempt_disable();
inc_module(module, _THIS_IP_);
preempt_enable();
}
}
static inline void try_module_get(struct module *module)
{
int ret = 1;
if (module) {
preempt_disable();
if (likely(module_is_live(module)))
inc_module(module, _THIS_IP_);
else
ret = 0;
preempt_enable();
}
return ret;
}
---
void inc_module(struct module *module, unsigned long ip)
{
__this_cpu_inc(module->refptr->incs);
trace_module_get(module, ip);
}
-- Steve
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH][GIT PULL][v3.3] tracing: Add header wrappers event_headers_start.h and event_headers_end.h
2012-01-18 17:56 ` Steven Rostedt
@ 2012-01-22 22:59 ` Rusty Russell
2012-01-26 2:41 ` [RFC][PATCH] tracing/module: Move tracepoint out of module.h Steven Rostedt
0 siblings, 1 reply; 31+ messages in thread
From: Rusty Russell @ 2012-01-22 22:59 UTC (permalink / raw)
To: Steven Rostedt, Ingo Molnar; +Cc: LKML, Andrew Morton, Frederic Weisbecker
On Wed, 18 Jan 2012 12:56:52 -0500, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Wed, 2012-01-18 at 13:07 +0100, Ingo Molnar wrote:
>
> > I'd argue that __raise_softirq_irqoff() should not be inline -
> > that would solve a whole host of issues. Event tracing is
> > enabled in most distros so there's no real overhead argument to
> > be made here either - so it's probably a bit faster in fact to
> > have this uninlined. What do you think?
>
> Sure, I have no problem moving that out of the header. I don't think
> raising a softirq is in that critical a path that it can't be a function
> call.
>
> There's only one trace/events header left which is in module.h. Perhaps
> we can move __module_get() and try_module_get() out of the header. We
> could just move the "if" part out.
Agreed. Since GCC should be able to eliminate that branch in almost all
cases, since it's usually a literal NULL or address of a (non-weak)
symbol.
Be interesting to see the before/after sizes with this out-of-line.
Thanks,
Rusty.
^ permalink raw reply [flat|nested] 31+ messages in thread
* [RFC][PATCH] tracing/module: Move tracepoint out of module.h
2012-01-22 22:59 ` Rusty Russell
@ 2012-01-26 2:41 ` Steven Rostedt
2012-01-26 2:45 ` Steven Rostedt
2012-01-26 10:28 ` Ingo Molnar
0 siblings, 2 replies; 31+ messages in thread
From: Steven Rostedt @ 2012-01-26 2:41 UTC (permalink / raw)
To: Rusty Russell; +Cc: Ingo Molnar, LKML, Andrew Morton, Frederic Weisbecker
On Mon, 2012-01-23 at 09:29 +1030, Rusty Russell wrote:
> > could just move the "if" part out.
>
> Agreed. Since GCC should be able to eliminate that branch in almost all
> cases, since it's usually a literal NULL or address of a (non-weak)
> symbol.
>
> Be interesting to see the before/after sizes with this out-of-line.
Using my default test config I got:
text data bss dec hex filename
7489488 2249584 9719808 19458880 128eb40 vmlinux-prepatch
7482458 2248048 9719808 19450314 128c9ca vmlinux-postpatch
An 8k savings!
Rusty,
Can you give my your Acked-by for this patch?
Thanks!
-- Steve
diff --git a/include/linux/module.h b/include/linux/module.h
index 3cb7839..b83a687 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -21,8 +21,6 @@
#include <linux/percpu.h>
#include <asm/module.h>
-#include <trace/events/module.h>
-
/* Not Yet Implemented */
#define MODULE_SUPPORTED_DEVICE(name)
@@ -438,6 +436,7 @@ unsigned int module_refcount(struct module *mod);
void __symbol_put(const char *symbol);
#define symbol_put(x) __symbol_put(MODULE_SYMBOL_PREFIX #x)
void symbol_put_addr(void *addr);
+void inc_module(struct module *module, unsigned long ip);
/* Sometimes we know we already have a refcount, and it's easier not
to handle the error case (which only happens with rmmod --wait). */
@@ -445,8 +444,7 @@ static inline void __module_get(struct module *module)
{
if (module) {
preempt_disable();
- __this_cpu_inc(module->refptr->incs);
- trace_module_get(module, _THIS_IP_);
+ inc_module(module, _THIS_IP_);
preempt_enable();
}
}
@@ -458,10 +456,9 @@ static inline int try_module_get(struct module *module)
if (module) {
preempt_disable();
- if (likely(module_is_live(module))) {
- __this_cpu_inc(module->refptr->incs);
- trace_module_get(module, _THIS_IP_);
- } else
+ if (likely(module_is_live(module)))
+ inc_module(module, _THIS_IP_);
+ else
ret = 0;
preempt_enable();
diff --git a/kernel/module.c b/kernel/module.c
index 178333c..83c22409 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -928,6 +928,13 @@ void module_put(struct module *module)
}
EXPORT_SYMBOL(module_put);
+void inc_module(struct module *module, unsigned long ip)
+{
+ __this_cpu_inc(module->refptr->incs);
+ trace_module_get(module, ip);
+}
+EXPORT_SYMBOL(inc_module);
+
#else /* !CONFIG_MODULE_UNLOAD */
static inline void print_unload_info(struct seq_file *m, struct module *mod)
{
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [RFC][PATCH] tracing/module: Move tracepoint out of module.h
2012-01-26 2:41 ` [RFC][PATCH] tracing/module: Move tracepoint out of module.h Steven Rostedt
@ 2012-01-26 2:45 ` Steven Rostedt
2012-01-26 10:28 ` Ingo Molnar
1 sibling, 0 replies; 31+ messages in thread
From: Steven Rostedt @ 2012-01-26 2:45 UTC (permalink / raw)
To: Rusty Russell; +Cc: Ingo Molnar, LKML, Andrew Morton, Frederic Weisbecker
On Wed, 2012-01-25 at 21:41 -0500, Steven Rostedt wrote:
> Can you give my your Acked-by for this patch?
s/my/me/
bah!
-- Steve
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC][PATCH] tracing/module: Move tracepoint out of module.h
2012-01-26 2:41 ` [RFC][PATCH] tracing/module: Move tracepoint out of module.h Steven Rostedt
2012-01-26 2:45 ` Steven Rostedt
@ 2012-01-26 10:28 ` Ingo Molnar
2012-01-26 13:52 ` Steven Rostedt
1 sibling, 1 reply; 31+ messages in thread
From: Ingo Molnar @ 2012-01-26 10:28 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Rusty Russell, LKML, Andrew Morton, Frederic Weisbecker
* Steven Rostedt <rostedt@goodmis.org> wrote:
> On Mon, 2012-01-23 at 09:29 +1030, Rusty Russell wrote:
> > > could just move the "if" part out.
> >
> > Agreed. Since GCC should be able to eliminate that branch in almost all
> > cases, since it's usually a literal NULL or address of a (non-weak)
> > symbol.
> >
> > Be interesting to see the before/after sizes with this out-of-line.
>
> Using my default test config I got:
>
> text data bss dec hex filename
> 7489488 2249584 9719808 19458880 128eb40 vmlinux-prepatch
> 7482458 2248048 9719808 19450314 128c9ca vmlinux-postpatch
>
> An 8k savings!
Lovely!
> @@ -445,8 +444,7 @@ static inline void __module_get(struct module *module)
> {
> if (module) {
> preempt_disable();
> - __this_cpu_inc(module->refptr->incs);
> - trace_module_get(module, _THIS_IP_);
> + inc_module(module, _THIS_IP_);
> preempt_enable();
> }
> }
> @@ -458,10 +456,9 @@ static inline int try_module_get(struct module *module)
> if (module) {
> preempt_disable();
>
> - if (likely(module_is_live(module))) {
> - __this_cpu_inc(module->refptr->incs);
> - trace_module_get(module, _THIS_IP_);
> - } else
> + if (likely(module_is_live(module)))
> + inc_module(module, _THIS_IP_);
> + else
> ret = 0;
How much more do we save if we move all of try_module_get() out
of line? It still seems a rather thick inline function with
preempt section and all. I'd *really* suggest that it should all
be uninlined.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC][PATCH] tracing/module: Move tracepoint out of module.h
2012-01-26 10:28 ` Ingo Molnar
@ 2012-01-26 13:52 ` Steven Rostedt
2012-01-26 13:55 ` Ingo Molnar
0 siblings, 1 reply; 31+ messages in thread
From: Steven Rostedt @ 2012-01-26 13:52 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Rusty Russell, LKML, Andrew Morton, Frederic Weisbecker
On Thu, 2012-01-26 at 11:28 +0100, Ingo Molnar wrote:
> How much more do we save if we move all of try_module_get() out
> of line? It still seems a rather thick inline function with
> preempt section and all. I'd *really* suggest that it should all
> be uninlined.
>
Here's the #'s
text data bss dec hex filename
7489488 2249584 9719808 19458880 128eb40 vmlinux-prepatch
text data bss dec hex filename
7482458 2248048 9719808 19450314 128c9ca vmlinux-postpatch
text data bss dec hex filename
7477393 2248080 9719808 19445281 128b621 vmlinux-updatedpatch
Thus we get an additional 5k it seems, for a total of 13k savings.
The below patch is what I used. Note, since the tracepoint requires the
instruction pointer, I created stub versions that call a "do_" version
of the function to pass in the local instruction counter.
Rusty, can you give your acked-by on that patch?
Thanks!
-- Steve
diff --git a/include/linux/module.h b/include/linux/module.h
index 3cb7839..2337f97 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -21,8 +21,6 @@
#include <linux/percpu.h>
#include <asm/module.h>
-#include <trace/events/module.h>
-
/* Not Yet Implemented */
#define MODULE_SUPPORTED_DEVICE(name)
@@ -439,34 +437,17 @@ void __symbol_put(const char *symbol);
#define symbol_put(x) __symbol_put(MODULE_SYMBOL_PREFIX #x)
void symbol_put_addr(void *addr);
-/* Sometimes we know we already have a refcount, and it's easier not
- to handle the error case (which only happens with rmmod --wait). */
+extern void do_module_get(struct module *module, int ip);
+extern int do_try_module_get(struct module *module, int ip);
+
static inline void __module_get(struct module *module)
{
- if (module) {
- preempt_disable();
- __this_cpu_inc(module->refptr->incs);
- trace_module_get(module, _THIS_IP_);
- preempt_enable();
- }
+ do_module_get(module, _THIS_IP_);
}
static inline int try_module_get(struct module *module)
{
- int ret = 1;
-
- if (module) {
- preempt_disable();
-
- if (likely(module_is_live(module))) {
- __this_cpu_inc(module->refptr->incs);
- trace_module_get(module, _THIS_IP_);
- } else
- ret = 0;
-
- preempt_enable();
- }
- return ret;
+ return do_try_module_get(module, _THIS_IP_);
}
extern void module_put(struct module *module);
diff --git a/kernel/module.c b/kernel/module.c
index 178333c..8955199 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -928,6 +928,42 @@ void module_put(struct module *module)
}
EXPORT_SYMBOL(module_put);
+static void inc_module(struct module *module, unsigned long ip)
+{
+ __this_cpu_inc(module->refptr->incs);
+ trace_module_get(module, ip);
+}
+
+/* Sometimes we know we already have a refcount, and it's easier not
+ to handle the error case (which only happens with rmmod --wait). */
+void do_module_get(struct module *module, int ip)
+{
+ if (module) {
+ preempt_disable();
+ inc_module(module, ip);
+ preempt_enable();
+ }
+}
+EXPORT_SYMBOL(do_module_get);
+
+int do_try_module_get(struct module *module, int ip)
+{
+ int ret = 1;
+
+ if (module) {
+ preempt_disable();
+
+ if (likely(module_is_live(module)))
+ inc_module(module, ip);
+ else
+ ret = 0;
+
+ preempt_enable();
+ }
+ return ret;
+}
+EXPORT_SYMBOL(do_try_module_get);
+
#else /* !CONFIG_MODULE_UNLOAD */
static inline void print_unload_info(struct seq_file *m, struct module *mod)
{
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [RFC][PATCH] tracing/module: Move tracepoint out of module.h
2012-01-26 13:52 ` Steven Rostedt
@ 2012-01-26 13:55 ` Ingo Molnar
2012-01-26 14:04 ` Steven Rostedt
0 siblings, 1 reply; 31+ messages in thread
From: Ingo Molnar @ 2012-01-26 13:55 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Rusty Russell, LKML, Andrew Morton, Frederic Weisbecker
* Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu, 2012-01-26 at 11:28 +0100, Ingo Molnar wrote:
>
> > How much more do we save if we move all of try_module_get() out
> > of line? It still seems a rather thick inline function with
> > preempt section and all. I'd *really* suggest that it should all
> > be uninlined.
> >
>
> Here's the #'s
>
> text data bss dec hex filename
> 7489488 2249584 9719808 19458880 128eb40 vmlinux-prepatch
> text data bss dec hex filename
> 7482458 2248048 9719808 19450314 128c9ca vmlinux-postpatch
> text data bss dec hex filename
> 7477393 2248080 9719808 19445281 128b621 vmlinux-updatedpatch
>
> Thus we get an additional 5k it seems, for a total of 13k savings.
>
> The below patch is what I used. Note, since the tracepoint
> requires the instruction pointer, I created stub versions that
> call a "do_" version of the function to pass in the local
> instruction counter.
Hm, why does this tracepoint include a flat RIP? It's pretty
superfluous as there's various other (orghogonal) ways to get at
the RIP.
If the field is needed for compatibility then lets set it to
zero at the tracepoint level and not burden the module.h
hotpath.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC][PATCH] tracing/module: Move tracepoint out of module.h
2012-01-26 13:55 ` Ingo Molnar
@ 2012-01-26 14:04 ` Steven Rostedt
2012-01-26 14:07 ` Steven Rostedt
2012-01-26 14:36 ` Steven Rostedt
0 siblings, 2 replies; 31+ messages in thread
From: Steven Rostedt @ 2012-01-26 14:04 UTC (permalink / raw)
To: Ingo Molnar
Cc: Rusty Russell, LKML, Andrew Morton, Frederic Weisbecker, Li Zefan
[ Added Li who added the original tracepoint ]
On Thu, 2012-01-26 at 14:55 +0100, Ingo Molnar wrote:
> * Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > On Thu, 2012-01-26 at 11:28 +0100, Ingo Molnar wrote:
> >
> > > How much more do we save if we move all of try_module_get() out
> > > of line? It still seems a rather thick inline function with
> > > preempt section and all. I'd *really* suggest that it should all
> > > be uninlined.
> > >
> >
> > Here's the #'s
> >
> > text data bss dec hex filename
> > 7489488 2249584 9719808 19458880 128eb40 vmlinux-prepatch
> > text data bss dec hex filename
> > 7482458 2248048 9719808 19450314 128c9ca vmlinux-postpatch
> > text data bss dec hex filename
> > 7477393 2248080 9719808 19445281 128b621 vmlinux-updatedpatch
> >
> > Thus we get an additional 5k it seems, for a total of 13k savings.
> >
> > The below patch is what I used. Note, since the tracepoint
> > requires the instruction pointer, I created stub versions that
> > call a "do_" version of the function to pass in the local
> > instruction counter.
>
> Hm, why does this tracepoint include a flat RIP? It's pretty
> superfluous as there's various other (orghogonal) ways to get at
> the RIP.
>
> If the field is needed for compatibility then lets set it to
> zero at the tracepoint level and not burden the module.h
> hotpath.
Perhaps I could use "__builtin_return_address(0)" instead. I'll try that
out. That way, we don't burden the module hotpath, and we keep the old
behavior.
-- Steve
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC][PATCH] tracing/module: Move tracepoint out of module.h
2012-01-26 14:04 ` Steven Rostedt
@ 2012-01-26 14:07 ` Steven Rostedt
2012-01-26 14:36 ` Steven Rostedt
1 sibling, 0 replies; 31+ messages in thread
From: Steven Rostedt @ 2012-01-26 14:07 UTC (permalink / raw)
To: Ingo Molnar
Cc: Rusty Russell, LKML, Andrew Morton, Frederic Weisbecker, Li Zefan
On Thu, 2012-01-26 at 09:04 -0500, Steven Rostedt wrote:
> Perhaps I could use "__builtin_return_address(0)" instead.
s/__builtin_return_address(0)/_RET_IP_/
-- Steve
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC][PATCH] tracing/module: Move tracepoint out of module.h
2012-01-26 14:04 ` Steven Rostedt
2012-01-26 14:07 ` Steven Rostedt
@ 2012-01-26 14:36 ` Steven Rostedt
2012-01-26 18:39 ` Ingo Molnar
2012-01-30 6:40 ` Li Zefan
1 sibling, 2 replies; 31+ messages in thread
From: Steven Rostedt @ 2012-01-26 14:36 UTC (permalink / raw)
To: Ingo Molnar
Cc: Rusty Russell, LKML, Andrew Morton, Frederic Weisbecker, Li Zefan
On Thu, 2012-01-26 at 09:04 -0500, Steven Rostedt wrote:
> [ Added Li who added the original tracepoint ]
>
> On Thu, 2012-01-26 at 14:55 +0100, Ingo Molnar wrote:
> > * Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > > On Thu, 2012-01-26 at 11:28 +0100, Ingo Molnar wrote:
> > >
> > > > How much more do we save if we move all of try_module_get() out
> > > > of line? It still seems a rather thick inline function with
> > > > preempt section and all. I'd *really* suggest that it should all
> > > > be uninlined.
> > > >
> > >
> > > Here's the #'s
> > >
> > > text data bss dec hex filename
> > > 7489488 2249584 9719808 19458880 128eb40 vmlinux-prepatch
> > > text data bss dec hex filename
> > > 7482458 2248048 9719808 19450314 128c9ca vmlinux-postpatch
> > > text data bss dec hex filename
> > > 7477393 2248080 9719808 19445281 128b621 vmlinux-updatedpatch
New numbers:
text data bss dec hex filename
7476509 2248048 9719808 19444365 128b28d vmlinux-updatedpatch2
Just 916 bytes savings more. But gets rid of the hot path changes.
New patch below: (and the IP still works)
-- Steve
diff --git a/include/linux/module.h b/include/linux/module.h
index 3cb7839..ef928b9 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -21,8 +21,6 @@
#include <linux/percpu.h>
#include <asm/module.h>
-#include <trace/events/module.h>
-
/* Not Yet Implemented */
#define MODULE_SUPPORTED_DEVICE(name)
@@ -439,36 +437,8 @@ void __symbol_put(const char *symbol);
#define symbol_put(x) __symbol_put(MODULE_SYMBOL_PREFIX #x)
void symbol_put_addr(void *addr);
-/* Sometimes we know we already have a refcount, and it's easier not
- to handle the error case (which only happens with rmmod --wait). */
-static inline void __module_get(struct module *module)
-{
- if (module) {
- preempt_disable();
- __this_cpu_inc(module->refptr->incs);
- trace_module_get(module, _THIS_IP_);
- preempt_enable();
- }
-}
-
-static inline int try_module_get(struct module *module)
-{
- int ret = 1;
-
- if (module) {
- preempt_disable();
-
- if (likely(module_is_live(module))) {
- __this_cpu_inc(module->refptr->incs);
- trace_module_get(module, _THIS_IP_);
- } else
- ret = 0;
-
- preempt_enable();
- }
- return ret;
-}
-
+extern void __module_get(struct module *module);
+extern int try_module_get(struct module *module);
extern void module_put(struct module *module);
#else /*!CONFIG_MODULE_UNLOAD*/
diff --git a/kernel/module.c b/kernel/module.c
index 178333c..c3dca5c 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -928,6 +928,38 @@ void module_put(struct module *module)
}
EXPORT_SYMBOL(module_put);
+/* Sometimes we know we already have a refcount, and it's easier not
+ to handle the error case (which only happens with rmmod --wait). */
+void __module_get(struct module *module)
+{
+ if (module) {
+ preempt_disable();
+ __this_cpu_inc(module->refptr->incs);
+ trace_module_get(module, _RET_IP_);
+ preempt_enable();
+ }
+}
+EXPORT_SYMBOL(__module_get);
+
+int try_module_get(struct module *module)
+{
+ int ret = 1;
+
+ if (module) {
+ preempt_disable();
+
+ if (likely(module_is_live(module))) {
+ __this_cpu_inc(module->refptr->incs);
+ trace_module_get(module, _RET_IP_);
+ } else
+ ret = 0;
+
+ preempt_enable();
+ }
+ return ret;
+}
+EXPORT_SYMBOL(try_module_get);
+
#else /* !CONFIG_MODULE_UNLOAD */
static inline void print_unload_info(struct seq_file *m, struct module *mod)
{
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [RFC][PATCH] tracing/module: Move tracepoint out of module.h
2012-01-26 14:36 ` Steven Rostedt
@ 2012-01-26 18:39 ` Ingo Molnar
2012-01-27 3:02 ` Rusty Russell
2012-01-30 6:40 ` Li Zefan
1 sibling, 1 reply; 31+ messages in thread
From: Ingo Molnar @ 2012-01-26 18:39 UTC (permalink / raw)
To: Steven Rostedt
Cc: Rusty Russell, LKML, Andrew Morton, Frederic Weisbecker, Li Zefan
* Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu, 2012-01-26 at 09:04 -0500, Steven Rostedt wrote:
> > [ Added Li who added the original tracepoint ]
> >
> > On Thu, 2012-01-26 at 14:55 +0100, Ingo Molnar wrote:
> > > * Steven Rostedt <rostedt@goodmis.org> wrote:
> > >
> > > > On Thu, 2012-01-26 at 11:28 +0100, Ingo Molnar wrote:
> > > >
> > > > > How much more do we save if we move all of try_module_get() out
> > > > > of line? It still seems a rather thick inline function with
> > > > > preempt section and all. I'd *really* suggest that it should all
> > > > > be uninlined.
> > > > >
> > > >
> > > > Here's the #'s
> > > >
> > > > text data bss dec hex filename
> > > > 7489488 2249584 9719808 19458880 128eb40 vmlinux-prepatch
> > > > text data bss dec hex filename
> > > > 7482458 2248048 9719808 19450314 128c9ca vmlinux-postpatch
> > > > text data bss dec hex filename
> > > > 7477393 2248080 9719808 19445281 128b621 vmlinux-updatedpatch
>
>
> New numbers:
>
> text data bss dec hex filename
> 7476509 2248048 9719808 19444365 128b28d vmlinux-updatedpatch2
>
> Just 916 bytes savings more. But gets rid of the hot path changes.
>
> New patch below: (and the IP still works)
Ok, i like this one best. Rusty, does it look good to you too?
Thanks,
Ingo
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC][PATCH] tracing/module: Move tracepoint out of module.h
2012-01-26 18:39 ` Ingo Molnar
@ 2012-01-27 3:02 ` Rusty Russell
2012-01-30 11:52 ` Steven Rostedt
0 siblings, 1 reply; 31+ messages in thread
From: Rusty Russell @ 2012-01-27 3:02 UTC (permalink / raw)
To: Ingo Molnar, Steven Rostedt
Cc: LKML, Andrew Morton, Frederic Weisbecker, Li Zefan
On Thu, 26 Jan 2012 19:39:46 +0100, Ingo Molnar <mingo@elte.hu> wrote:
> Ok, i like this one best. Rusty, does it look good to you too?
No, the if (module) test belongs in the inline wrapper (since gcc knows
that at compile time).
But the rest is good.
Thanks!
Rusty.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC][PATCH] tracing/module: Move tracepoint out of module.h
2012-01-27 3:02 ` Rusty Russell
@ 2012-01-30 11:52 ` Steven Rostedt
2012-01-30 17:28 ` Steven Rostedt
2012-01-31 3:58 ` Rusty Russell
0 siblings, 2 replies; 31+ messages in thread
From: Steven Rostedt @ 2012-01-30 11:52 UTC (permalink / raw)
To: Rusty Russell
Cc: Ingo Molnar, LKML, Andrew Morton, Frederic Weisbecker, Li Zefan
On Fri, 2012-01-27 at 13:32 +1030, Rusty Russell wrote:
> On Thu, 26 Jan 2012 19:39:46 +0100, Ingo Molnar <mingo@elte.hu> wrote:
> > Ok, i like this one best. Rusty, does it look good to you too?
>
> No, the if (module) test belongs in the inline wrapper (since gcc knows
> that at compile time).
For some reason though it still adds 5K when we keep the code as a
static inline. Note, my test config does have all the necessary modules
to boot the box as compiled in (not as modules). If necessary, I could
compile with a distro config and see what the differences are with that.
Rusty, the final decision is yours. If you believe that the added code
size is worth having the static inlines, then I'll go back to the
previous version that had that.
I'll compile with a distro config, and then take a look at the
differences of the vmlinux.
Thanks,
-- Steve
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC][PATCH] tracing/module: Move tracepoint out of module.h
2012-01-30 11:52 ` Steven Rostedt
@ 2012-01-30 17:28 ` Steven Rostedt
2012-01-31 3:58 ` Rusty Russell
1 sibling, 0 replies; 31+ messages in thread
From: Steven Rostedt @ 2012-01-30 17:28 UTC (permalink / raw)
To: Rusty Russell
Cc: Ingo Molnar, LKML, Andrew Morton, Frederic Weisbecker, Li Zefan
On Mon, 2012-01-30 at 06:52 -0500, Steven Rostedt wrote:
> On Fri, 2012-01-27 at 13:32 +1030, Rusty Russell wrote:
> > On Thu, 26 Jan 2012 19:39:46 +0100, Ingo Molnar <mingo@elte.hu> wrote:
> > > Ok, i like this one best. Rusty, does it look good to you too?
> >
> > No, the if (module) test belongs in the inline wrapper (since gcc knows
> > that at compile time).
>
> I'll compile with a distro config, and then take a look at the
> differences of the vmlinux.
>
I just finished, and here's my results:
I used the debian config config-3.0.0-1-amd64 against v3.2 kernel and
gcc 4.6.0.
text data bss dec hex filename
11124685 2028416 2510848 15663949 ef034d vmlinux.orig
11119568 2028416 2510848 15658832 eeef50 vmlinux.if
11117769 2028416 2510848 15657033 eee849 vmlinux.func
The first (.orig) is the current method.
The second (.if) is the functions still there, but the contents of the
if statement moved out of line into its own inc_module() function that
is put in module.c
The last (.func) is the entire function moved out of module.h into
module.c.
Savings of the inc_module() patch: 5117 bytes
Savings of the full out of line patch: 6916 bytes
That's another 1799 bytes in savings moving the functions out of inline.
I looked at why this was the case. I searched around, and it seems
mostly due to try_module_get() and not __module_get(). Because
try_module_get() is usually called with a pointer and not a constant:
fs/anon_inodes.c: anon_inode_getfile()
if (fops->owner && !try_module_get(fops->owner))
return ERR_PTR(-ENOENT);
To test this, I moved only try_module_get() from being inlined to a
function:
text data bss dec hex filename
11117895 2028416 2510848 15657159 eee8c7 vmlinux.if-func
This one has a savings of 6790 bytes (a differences of 126 bytes from
the full out-of-line version).
The extra 126 bytes came from:
fs/exec.c:set_binfmt(),
fs/filesystems.c:get_filesystem(),
drivers/dma/dmaengine.c:balance_ref_count(),
drivers/dma/dmaengine.c:dma_chan_get(),
drivers/tty/vt/vt.c:visual_init(),
drivers/tty/vt/vt.c:bind_con_driver(),
net/socket.c:sys_accept4(), net/socket.c:kernel_accept(),
net/ipv4/inet_timewait_sock.c:inet_twsk_alloc(),
net/sunrpc/svc.c:svc_set_num_threads(),
net/sunrpc/svc_xprt.c:svc_recv(),
and
net/sunrpc/auth_gss/gss_mech_switch.c:gss_mech_get()
All these call __module_get() from a pointer:
struct gss_api_mech *
gss_mech_get(struct gss_api_mech *gm)
{
__module_get(gm->gm_owner);
return gm;
}
where gcc can not optimize the if (module) out.
Rusty, which version do you want me to use?
Full out-of-line (which you don't seem to like).
Move just the contents of the if (module) out.
Move the contents of __module_get() if out, but out-of-line try_module_get()?
Below is the patch that moves the if (module) out of __module_get() and
makes try_module_get() into a normal function call (last choice above):
diff --git a/include/linux/module.h b/include/linux/module.h
index 3cb7839..41840be 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -21,8 +21,6 @@
#include <linux/percpu.h>
#include <asm/module.h>
-#include <trace/events/module.h>
-
/* Not Yet Implemented */
#define MODULE_SUPPORTED_DEVICE(name)
@@ -438,6 +436,7 @@ unsigned int module_refcount(struct module *mod);
void __symbol_put(const char *symbol);
#define symbol_put(x) __symbol_put(MODULE_SYMBOL_PREFIX #x)
void symbol_put_addr(void *addr);
+void inc_module(struct module *module, unsigned long ip);
/* Sometimes we know we already have a refcount, and it's easier not
to handle the error case (which only happens with rmmod --wait). */
@@ -445,30 +444,12 @@ static inline void __module_get(struct module
*module)
{
if (module) {
preempt_disable();
- __this_cpu_inc(module->refptr->incs);
- trace_module_get(module, _THIS_IP_);
- preempt_enable();
- }
-}
-
-static inline int try_module_get(struct module *module)
-{
- int ret = 1;
-
- if (module) {
- preempt_disable();
-
- if (likely(module_is_live(module))) {
- __this_cpu_inc(module->refptr->incs);
- trace_module_get(module, _THIS_IP_);
- } else
- ret = 0;
-
+ inc_module(module, _THIS_IP_);
preempt_enable();
}
- return ret;
}
+extern int try_module_get(struct module *module);
extern void module_put(struct module *module);
#else /*!CONFIG_MODULE_UNLOAD*/
diff --git a/kernel/module.c b/kernel/module.c
index 178333c..ca94dc9 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -928,6 +928,31 @@ void module_put(struct module *module)
}
EXPORT_SYMBOL(module_put);
+void inc_module(struct module *module, unsigned long ip)
+{
+ __this_cpu_inc(module->refptr->incs);
+ trace_module_get(module, ip);
+}
+EXPORT_SYMBOL(inc_module);
+
+int try_module_get(struct module *module)
+{
+ int ret = 1;
+
+ if (module) {
+ preempt_disable();
+
+ if (likely(module_is_live(module)))
+ inc_module(module, _RET_IP_);
+ else
+ ret = 0;
+
+ preempt_enable();
+ }
+ return ret;
+}
+EXPORT_SYMBOL(try_module_get);
+
#else /* !CONFIG_MODULE_UNLOAD */
static inline void print_unload_info(struct seq_file *m, struct module
*mod)
{
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [RFC][PATCH] tracing/module: Move tracepoint out of module.h
2012-01-30 11:52 ` Steven Rostedt
2012-01-30 17:28 ` Steven Rostedt
@ 2012-01-31 3:58 ` Rusty Russell
2012-01-31 12:20 ` Ingo Molnar
1 sibling, 1 reply; 31+ messages in thread
From: Rusty Russell @ 2012-01-31 3:58 UTC (permalink / raw)
To: Steven Rostedt
Cc: Ingo Molnar, LKML, Andrew Morton, Frederic Weisbecker, Li Zefan
On Mon, 30 Jan 2012 06:52:13 -0500, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 2012-01-27 at 13:32 +1030, Rusty Russell wrote:
> > On Thu, 26 Jan 2012 19:39:46 +0100, Ingo Molnar <mingo@elte.hu> wrote:
> > > Ok, i like this one best. Rusty, does it look good to you too?
> >
> > No, the if (module) test belongs in the inline wrapper (since gcc knows
> > that at compile time).
>
> For some reason though it still adds 5K when we keep the code as a
> static inline. Note, my test config does have all the necessary modules
> to boot the box as compiled in (not as modules). If necessary, I could
> compile with a distro config and see what the differences are with that.
>
> Rusty, the final decision is yours. If you believe that the added code
> size is worth having the static inlines, then I'll go back to the
> previous version that had that.
Wow, we're really bikeshedding this!
I was completely wrong with the "it's usually a constant" of course;
it's usually ->owner.
So let's just out-of-line the entire thing. I changed the type to bool
and s/_THIS_IP_/_RET_IP_/ -- is that sufficient?
Doesn't save me much here, though. What are your stats?
module: move __module_get and try_module_get() out of line.
With the preempt, tracepoint and everything, it's getting a bit
chubby.
Saves only 400 bytes of text here, but I don't do preempt or
tracepoints.
Before:
text data bss dec hex filename
5373459 399532 2514944 8287935 7e76bf vmlinux
After:
text data bss dec hex filename
5373071 399532 2514944 8287547 7e753b vmlinux
diff --git a/include/linux/module.h b/include/linux/module.h
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -21,8 +21,6 @@
#include <linux/percpu.h>
#include <asm/module.h>
-#include <trace/events/module.h>
-
/* Not Yet Implemented */
#define MODULE_SUPPORTED_DEVICE(name)
@@ -452,33 +450,11 @@ void symbol_put_addr(void *addr);
/* Sometimes we know we already have a refcount, and it's easier not
to handle the error case (which only happens with rmmod --wait). */
-static inline void __module_get(struct module *module)
-{
- if (module) {
- preempt_disable();
- __this_cpu_inc(module->refptr->incs);
- trace_module_get(module, _THIS_IP_);
- preempt_enable();
- }
-}
+extern void __module_get(struct module *module);
-static inline int try_module_get(struct module *module)
-{
- int ret = 1;
-
- if (module) {
- preempt_disable();
-
- if (likely(module_is_live(module))) {
- __this_cpu_inc(module->refptr->incs);
- trace_module_get(module, _THIS_IP_);
- } else
- ret = 0;
-
- preempt_enable();
- }
- return ret;
-}
+/* This is the Right Way to get a module: if it fails, it's being removed,
+ * so pretend it's not there. */
+extern bool try_module_get(struct module *module);
extern void module_put(struct module *module);
diff --git a/kernel/module.c b/kernel/module.c
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -903,6 +903,36 @@ static ssize_t show_refcnt(struct module
static struct module_attribute modinfo_refcnt =
__ATTR(refcnt, 0444, show_refcnt, NULL);
+void __module_get(struct module *module)
+{
+ if (module) {
+ preempt_disable();
+ __this_cpu_inc(module->refptr->incs);
+ trace_module_get(module, _RET_IP_);
+ preempt_enable();
+ }
+}
+EXPORT_SYMBOL(__module_get);
+
+bool try_module_get(struct module *module)
+{
+ bool ret = true;
+
+ if (module) {
+ preempt_disable();
+
+ if (likely(module_is_live(module))) {
+ __this_cpu_inc(module->refptr->incs);
+ trace_module_get(module, _RET_IP_);
+ } else
+ ret = false;
+
+ preempt_enable();
+ }
+ return ret;
+}
+EXPORT_SYMBOL(try_module_get);
+
void module_put(struct module *module)
{
if (module) {
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC][PATCH] tracing/module: Move tracepoint out of module.h
2012-01-31 3:58 ` Rusty Russell
@ 2012-01-31 12:20 ` Ingo Molnar
2012-01-31 12:50 ` Steven Rostedt
2012-02-01 1:10 ` Rusty Russell
0 siblings, 2 replies; 31+ messages in thread
From: Ingo Molnar @ 2012-01-31 12:20 UTC (permalink / raw)
To: Rusty Russell
Cc: Steven Rostedt, LKML, Andrew Morton, Frederic Weisbecker, Li Zefan
* Rusty Russell <rusty@rustcorp.com.au> wrote:
> On Mon, 30 Jan 2012 06:52:13 -0500, Steven Rostedt <rostedt@goodmis.org> wrote:
> > On Fri, 2012-01-27 at 13:32 +1030, Rusty Russell wrote:
> > > On Thu, 26 Jan 2012 19:39:46 +0100, Ingo Molnar <mingo@elte.hu> wrote:
> > > > Ok, i like this one best. Rusty, does it look good to you too?
> > >
> > > No, the if (module) test belongs in the inline wrapper (since gcc knows
> > > that at compile time).
> >
> > For some reason though it still adds 5K when we keep the
> > code as a static inline. Note, my test config does have all
> > the necessary modules to boot the box as compiled in (not as
> > modules). If necessary, I could compile with a distro config
> > and see what the differences are with that.
> >
> > Rusty, the final decision is yours. If you believe that the
> > added code size is worth having the static inlines, then
> > I'll go back to the previous version that had that.
>
> Wow, we're really bikeshedding this!
Hey, I don't think this is bikesheddig: we are shedding
*kilobytes* of code from pretty hot codepaths, that kind of
topic does deserve some detailed attention :-)
> I was completely wrong with the "it's usually a constant" of
> course; it's usually ->owner.
>
> So let's just out-of-line the entire thing. I changed the
> type to bool and s/_THIS_IP_/_RET_IP_/ -- is that sufficient?
>
> Doesn't save me much here, though. What are your stats?
I suspect it depends on inlining options in the .config plus on
event tracing?
> Saves only 400 bytes of text here, but I don't do preempt or
> tracepoints.
Most distro kernels do tracepoints so I guess that's where the
size delta comes from :-) In any case:
Acked-by: Ingo Molnar <mingo@elte.hu>
Thanks,
Ingo
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC][PATCH] tracing/module: Move tracepoint out of module.h
2012-01-31 12:20 ` Ingo Molnar
@ 2012-01-31 12:50 ` Steven Rostedt
2012-02-01 6:48 ` Rusty Russell
2012-02-01 1:10 ` Rusty Russell
1 sibling, 1 reply; 31+ messages in thread
From: Steven Rostedt @ 2012-01-31 12:50 UTC (permalink / raw)
To: Ingo Molnar
Cc: Rusty Russell, LKML, Andrew Morton, Frederic Weisbecker, Li Zefan
On Tue, 2012-01-31 at 13:20 +0100, Ingo Molnar wrote:
> > Saves only 400 bytes of text here, but I don't do preempt or
> > tracepoints.
>
> Most distro kernels do tracepoints so I guess that's where the
> size delta comes from :-) In any case:
Yes, having tracepoints inlined causes a lot of bloat. But I still did
get a 1799 bytes savings between moving the tracepoint out of line but
keeping the if, and totally moving the entire functions out of line. Not
sure what the discrepancy was there.
My last set of numbers came from the default 3.0.0 Debian config, which
probably adds more things that call these functions into the kernel
proper?
But anway, I think this bike shed is complete ;-)
>
> Acked-by: Ingo Molnar <mingo@elte.hu>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
-- Steve
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC][PATCH] tracing/module: Move tracepoint out of module.h
2012-01-31 12:50 ` Steven Rostedt
@ 2012-02-01 6:48 ` Rusty Russell
2012-02-01 13:27 ` Steven Rostedt
` (2 more replies)
0 siblings, 3 replies; 31+ messages in thread
From: Rusty Russell @ 2012-02-01 6:48 UTC (permalink / raw)
To: Steven Rostedt, Ingo Molnar
Cc: LKML, Andrew Morton, Frederic Weisbecker, Li Zefan
On Tue, 31 Jan 2012 07:50:30 -0500, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 2012-01-31 at 13:20 +0100, Ingo Molnar wrote:
>
> > > Saves only 400 bytes of text here, but I don't do preempt or
> > > tracepoints.
> >
> > Most distro kernels do tracepoints so I guess that's where the
> > size delta comes from :-) In any case:
>
> Yes, having tracepoints inlined causes a lot of bloat. But I still did
> get a 1799 bytes savings between moving the tracepoint out of line but
> keeping the if, and totally moving the entire functions out of line. Not
> sure what the discrepancy was there.
>
> My last set of numbers came from the default 3.0.0 Debian config, which
> probably adds more things that call these functions into the kernel
> proper?
Here's the results I get, using the Ubuntu 3.0.0 config (minus
CONFIG_DEBUG_INFO, and rest with Enter held down).
Applied,
Rusty.
From: Steven Rostedt <rostedt@goodmis.org>
Subject: module: move __module_get and try_module_get() out of line.
With the preempt, tracepoint and everything, it's getting a bit
chubby. For an Ubuntu-based config:
Before:
$ size -t `find * -name '*.ko'` | grep TOTAL
56199906 3870760 1606616 61677282 3ad1ee2 (TOTALS)
$ size vmlinux
text data bss dec hex filename
8509342 850368 3358720 12718430 c2115e vmlinux
After:
$ size -t `find * -name '*.ko'` | grep TOTAL
56183760 3867892 1606616 61658268 3acd49c (TOTALS)
$ size vmlinux
text data bss dec hex filename
8501842 849088 3358720 12709650 c1ef12 vmlinux
Acked-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> (made all out-of-line)
diff --git a/include/linux/module.h b/include/linux/module.h
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -21,8 +21,6 @@
#include <linux/percpu.h>
#include <asm/module.h>
-#include <trace/events/module.h>
-
/* Not Yet Implemented */
#define MODULE_SUPPORTED_DEVICE(name)
@@ -452,33 +450,11 @@ void symbol_put_addr(void *addr);
/* Sometimes we know we already have a refcount, and it's easier not
to handle the error case (which only happens with rmmod --wait). */
-static inline void __module_get(struct module *module)
-{
- if (module) {
- preempt_disable();
- __this_cpu_inc(module->refptr->incs);
- trace_module_get(module, _THIS_IP_);
- preempt_enable();
- }
-}
+extern void __module_get(struct module *module);
-static inline int try_module_get(struct module *module)
-{
- int ret = 1;
-
- if (module) {
- preempt_disable();
-
- if (likely(module_is_live(module))) {
- __this_cpu_inc(module->refptr->incs);
- trace_module_get(module, _THIS_IP_);
- } else
- ret = 0;
-
- preempt_enable();
- }
- return ret;
-}
+/* This is the Right Way to get a module: if it fails, it's being removed,
+ * so pretend it's not there. */
+extern bool try_module_get(struct module *module);
extern void module_put(struct module *module);
diff --git a/kernel/module.c b/kernel/module.c
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -903,6 +903,36 @@ static ssize_t show_refcnt(struct module
static struct module_attribute modinfo_refcnt =
__ATTR(refcnt, 0444, show_refcnt, NULL);
+void __module_get(struct module *module)
+{
+ if (module) {
+ preempt_disable();
+ __this_cpu_inc(module->refptr->incs);
+ trace_module_get(module, _RET_IP_);
+ preempt_enable();
+ }
+}
+EXPORT_SYMBOL(__module_get);
+
+bool try_module_get(struct module *module)
+{
+ bool ret = true;
+
+ if (module) {
+ preempt_disable();
+
+ if (likely(module_is_live(module))) {
+ __this_cpu_inc(module->refptr->incs);
+ trace_module_get(module, _RET_IP_);
+ } else
+ ret = false;
+
+ preempt_enable();
+ }
+ return ret;
+}
+EXPORT_SYMBOL(try_module_get);
+
void module_put(struct module *module)
{
if (module) {
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC][PATCH] tracing/module: Move tracepoint out of module.h
2012-02-01 6:48 ` Rusty Russell
@ 2012-02-01 13:27 ` Steven Rostedt
2012-02-01 13:49 ` Ingo Molnar
2012-03-29 4:22 ` Eric Dumazet
2 siblings, 0 replies; 31+ messages in thread
From: Steven Rostedt @ 2012-02-01 13:27 UTC (permalink / raw)
To: Rusty Russell
Cc: Ingo Molnar, LKML, Andrew Morton, Frederic Weisbecker, Li Zefan
On Wed, 2012-02-01 at 17:18 +1030, Rusty Russell wrote:
> Here's the results I get, using the Ubuntu 3.0.0 config (minus
> CONFIG_DEBUG_INFO, and rest with Enter held down).
>
> Applied,
> Rusty.
>
Thanks Rusty!
I originally had this in my own local git queue ready to be pushed to
Ingo after I get your Ack. But I think it is better if this change goes
through you. I'll take this change out and rebase, as none of my other
work depends on it.
Thanks again!
-- Steve
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC][PATCH] tracing/module: Move tracepoint out of module.h
2012-02-01 6:48 ` Rusty Russell
2012-02-01 13:27 ` Steven Rostedt
@ 2012-02-01 13:49 ` Ingo Molnar
2012-02-01 14:25 ` Steven Rostedt
2012-03-29 4:22 ` Eric Dumazet
2 siblings, 1 reply; 31+ messages in thread
From: Ingo Molnar @ 2012-02-01 13:49 UTC (permalink / raw)
To: Rusty Russell
Cc: Steven Rostedt, LKML, Andrew Morton, Frederic Weisbecker, Li Zefan
* Rusty Russell <rusty@rustcorp.com.au> wrote:
> Applied,
> Rusty.
>
> From: Steven Rostedt <rostedt@goodmis.org>
> Acked-by: Ingo Molnar <mingo@elte.hu>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> (made all out-of-line)
Steve's SOB seems to be missing.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC][PATCH] tracing/module: Move tracepoint out of module.h
2012-02-01 13:49 ` Ingo Molnar
@ 2012-02-01 14:25 ` Steven Rostedt
0 siblings, 0 replies; 31+ messages in thread
From: Steven Rostedt @ 2012-02-01 14:25 UTC (permalink / raw)
To: Ingo Molnar
Cc: Rusty Russell, LKML, Andrew Morton, Frederic Weisbecker, Li Zefan
On Wed, 2012-02-01 at 14:49 +0100, Ingo Molnar wrote:
> * Rusty Russell <rusty@rustcorp.com.au> wrote:
>
> > Applied,
> > Rusty.
> >
> > From: Steven Rostedt <rostedt@goodmis.org>
>
> > Acked-by: Ingo Molnar <mingo@elte.hu>
> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> (made all out-of-line)
>
> Steve's SOB seems to be missing.
Ah I may have never added it as it was originally just posted to get
Rusty's approval. So here it is:
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Thanks,
-- Steve
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC][PATCH] tracing/module: Move tracepoint out of module.h
2012-02-01 6:48 ` Rusty Russell
2012-02-01 13:27 ` Steven Rostedt
2012-02-01 13:49 ` Ingo Molnar
@ 2012-03-29 4:22 ` Eric Dumazet
2012-03-29 5:24 ` Rusty Russell
2 siblings, 1 reply; 31+ messages in thread
From: Eric Dumazet @ 2012-03-29 4:22 UTC (permalink / raw)
To: Rusty Russell
Cc: Steven Rostedt, Ingo Molnar, LKML, Andrew Morton,
Frederic Weisbecker, Li Zefan
Le mercredi 01 février 2012 à 17:18 +1030, Rusty Russell a écrit :
>
> +void __module_get(struct module *module)
> +{
> + if (module) {
> + preempt_disable();
> + __this_cpu_inc(module->refptr->incs);
> + trace_module_get(module, _RET_IP_);
> + preempt_enable();
> + }
> +}
> +EXPORT_SYMBOL(__module_get);
> +
Hi Rusty
I am wondering why preempt_disable()/preempt_enable() is needed in this
code.
this_cpu_inc(module->refptr->incs) is preempt safe...
diff --git a/kernel/module.c b/kernel/module.c
index 78ac6ec..7dc88b3 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -907,10 +907,8 @@ static struct module_attribute modinfo_refcnt =
void __module_get(struct module *module)
{
if (module) {
- preempt_disable();
- __this_cpu_inc(module->refptr->incs);
+ this_cpu_inc(module->refptr->incs);
trace_module_get(module, _RET_IP_);
- preempt_enable();
}
}
EXPORT_SYMBOL(__module_get);
@@ -920,15 +918,11 @@ bool try_module_get(struct module *module)
bool ret = true;
if (module) {
- preempt_disable();
-
if (likely(module_is_live(module))) {
- __this_cpu_inc(module->refptr->incs);
+ this_cpu_inc(module->refptr->incs);
trace_module_get(module, _RET_IP_);
} else
ret = false;
-
- preempt_enable();
}
return ret;
}
@@ -937,15 +931,13 @@ EXPORT_SYMBOL(try_module_get);
void module_put(struct module *module)
{
if (module) {
- preempt_disable();
smp_wmb(); /* see comment in module_refcount */
- __this_cpu_inc(module->refptr->decs);
+ this_cpu_inc(module->refptr->decs);
trace_module_put(module, _RET_IP_);
/* Maybe they're waiting for us to drop reference? */
if (unlikely(!module_is_live(module)))
wake_up_process(module->waiter);
- preempt_enable();
}
}
EXPORT_SYMBOL(module_put);
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [RFC][PATCH] tracing/module: Move tracepoint out of module.h
2012-03-29 4:22 ` Eric Dumazet
@ 2012-03-29 5:24 ` Rusty Russell
0 siblings, 0 replies; 31+ messages in thread
From: Rusty Russell @ 2012-03-29 5:24 UTC (permalink / raw)
To: Eric Dumazet
Cc: Steven Rostedt, Ingo Molnar, LKML, Andrew Morton,
Frederic Weisbecker, Li Zefan
On Thu, 29 Mar 2012 06:22:44 +0200, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le mercredi 01 février 2012 à 17:18 +1030, Rusty Russell a écrit :
>
> >
> > +void __module_get(struct module *module)
> > +{
> > + if (module) {
> > + preempt_disable();
> > + __this_cpu_inc(module->refptr->incs);
> > + trace_module_get(module, _RET_IP_);
> > + preempt_enable();
> > + }
> > +}
> > +EXPORT_SYMBOL(__module_get);
> > +
>
> Hi Rusty
>
> I am wondering why preempt_disable()/preempt_enable() is needed in this
> code.
>
> this_cpu_inc(module->refptr->incs) is preempt safe...
I agree, it's overkill here:
> @@ -907,10 +907,8 @@ static struct module_attribute modinfo_refcnt =
> void __module_get(struct module *module)
> {
> if (module) {
> - preempt_disable();
> - __this_cpu_inc(module->refptr->incs);
> + this_cpu_inc(module->refptr->incs);
> trace_module_get(module, _RET_IP_);
> - preempt_enable();
> }
> }
> EXPORT_SYMBOL(__module_get);
But this one is required:
> @@ -920,15 +918,11 @@ bool try_module_get(struct module *module)
> bool ret = true;
>
> if (module) {
> - preempt_disable();
> -
> if (likely(module_is_live(module))) {
> - __this_cpu_inc(module->refptr->incs);
> + this_cpu_inc(module->refptr->incs);
> trace_module_get(module, _RET_IP_);
> } else
> ret = false;
> -
> - preempt_enable();
> }
> return ret;
> }
This is to protect against module removal's stop_machine.
> @@ -937,15 +931,13 @@ EXPORT_SYMBOL(try_module_get);
> void module_put(struct module *module)
> {
> if (module) {
> - preempt_disable();
> smp_wmb(); /* see comment in module_refcount */
> - __this_cpu_inc(module->refptr->decs);
> + this_cpu_inc(module->refptr->decs);
>
> trace_module_put(module, _RET_IP_);
> /* Maybe they're waiting for us to drop reference? */
> if (unlikely(!module_is_live(module)))
> wake_up_process(module->waiter);
> - preempt_enable();
> }
> }
> EXPORT_SYMBOL(module_put);
Without the preempt disable, module can vanish immediately
after that increment.
Cheers,
Rusty.
--
How could I marry someone with more hair than me? http://baldalex.org
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC][PATCH] tracing/module: Move tracepoint out of module.h
2012-01-31 12:20 ` Ingo Molnar
2012-01-31 12:50 ` Steven Rostedt
@ 2012-02-01 1:10 ` Rusty Russell
2012-02-01 7:09 ` Ingo Molnar
1 sibling, 1 reply; 31+ messages in thread
From: Rusty Russell @ 2012-02-01 1:10 UTC (permalink / raw)
To: Ingo Molnar
Cc: Steven Rostedt, LKML, Andrew Morton, Frederic Weisbecker, Li Zefan
On Tue, 31 Jan 2012 13:20:16 +0100, Ingo Molnar <mingo@elte.hu> wrote:
> > Wow, we're really bikeshedding this!
>
> Hey, I don't think this is bikesheddig: we are shedding
> *kilobytes* of code from pretty hot codepaths, that kind of
> topic does deserve some detailed attention :-)
I agree, but we put the bloat in without this much discussion :)
I get the impression last I looked that most get/put aren't actually hot
paths (though it was designed to allow that, such as an inc per packet).
Which is an even better reason to out-of-line it...
> > Doesn't save me much here, though. What are your stats?
>
> I suspect it depends on inlining options in the .config plus on
> event tracing?
...
> Most distro kernels do tracepoints so I guess that's where the
> size delta comes from :-) In any case:
>
Yeah, I'm rebuilding an Ubuntu config kernel before and after to get
some more accurate stats on the final result for the commit message.
> Acked-by: Ingo Molnar <mingo@elte.hu>
Thanks,
Rusty.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC][PATCH] tracing/module: Move tracepoint out of module.h
2012-02-01 1:10 ` Rusty Russell
@ 2012-02-01 7:09 ` Ingo Molnar
0 siblings, 0 replies; 31+ messages in thread
From: Ingo Molnar @ 2012-02-01 7:09 UTC (permalink / raw)
To: Rusty Russell
Cc: Steven Rostedt, LKML, Andrew Morton, Frederic Weisbecker, Li Zefan
* Rusty Russell <rusty@rustcorp.com.au> wrote:
> On Tue, 31 Jan 2012 13:20:16 +0100, Ingo Molnar <mingo@elte.hu> wrote:
>
> > > Wow, we're really bikeshedding this!
> >
> > Hey, I don't think this is bikesheddig: we are shedding
> > *kilobytes* of code from pretty hot codepaths, that kind of
> > topic does deserve some detailed attention :-)
>
> I agree, but we put the bloat in without this much discussion
> :)
Hey, it was eventually noticed :-)
Would be nice if linux-next posted a daily before/after commit
bloatometer stats top list - unfortunately this would be pretty
time consuming to acquire due to the many rebasing Git trees.
With a proper Git workflow enforced it would be a few dozen
kernel builds per release, easily done on a fast enough box.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC][PATCH] tracing/module: Move tracepoint out of module.h
2012-01-26 14:36 ` Steven Rostedt
2012-01-26 18:39 ` Ingo Molnar
@ 2012-01-30 6:40 ` Li Zefan
1 sibling, 0 replies; 31+ messages in thread
From: Li Zefan @ 2012-01-30 6:40 UTC (permalink / raw)
To: Steven Rostedt
Cc: Ingo Molnar, Rusty Russell, LKML, Andrew Morton, Frederic Weisbecker
Steven Rostedt wrote:
> On Thu, 2012-01-26 at 09:04 -0500, Steven Rostedt wrote:
>> [ Added Li who added the original tracepoint ]
>>
>> On Thu, 2012-01-26 at 14:55 +0100, Ingo Molnar wrote:
>>> * Steven Rostedt <rostedt@goodmis.org> wrote:
>>>
>>>> On Thu, 2012-01-26 at 11:28 +0100, Ingo Molnar wrote:
>>>>
>>>>> How much more do we save if we move all of try_module_get() out
>>>>> of line? It still seems a rather thick inline function with
>>>>> preempt section and all. I'd *really* suggest that it should all
>>>>> be uninlined.
>>>>>
>>>>
>>>> Here's the #'s
>>>>
>>>> text data bss dec hex filename
>>>> 7489488 2249584 9719808 19458880 128eb40 vmlinux-prepatch
>>>> text data bss dec hex filename
>>>> 7482458 2248048 9719808 19450314 128c9ca vmlinux-postpatch
>>>> text data bss dec hex filename
>>>> 7477393 2248080 9719808 19445281 128b621 vmlinux-updatedpatch
>
>
> New numbers:
>
> text data bss dec hex filename
> 7476509 2248048 9719808 19444365 128b28d vmlinux-updatedpatch2
>
> Just 916 bytes savings more. But gets rid of the hot path changes.
>
> New patch below: (and the IP still works)
>
> -- Steve
>
This looks good to me. Thanks!
--
Li Zefan
^ permalink raw reply [flat|nested] 31+ messages in thread
* [tip:perf/core] tracing/softirq: Move __raise_softirq_irqoff() out of header
2012-01-18 12:07 ` Ingo Molnar
2012-01-18 17:56 ` Steven Rostedt
@ 2012-02-17 13:46 ` tip-bot for Steven Rostedt
1 sibling, 0 replies; 31+ messages in thread
From: tip-bot for Steven Rostedt @ 2012-02-17 13:46 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, hpa, mingo, rostedt, srostedt, tglx, mingo
Commit-ID: f069686e4bdc60a637d210ea3eea25fcdb82df88
Gitweb: http://git.kernel.org/tip/f069686e4bdc60a637d210ea3eea25fcdb82df88
Author: Steven Rostedt <srostedt@redhat.com>
AuthorDate: Wed, 25 Jan 2012 20:18:55 -0500
Committer: Steven Rostedt <rostedt@goodmis.org>
CommitDate: Fri, 3 Feb 2012 09:48:19 -0500
tracing/softirq: Move __raise_softirq_irqoff() out of header
The __raise_softirq_irqoff() contains a tracepoint. As tracepoints in headers
can cause issues, and not to mention, bloats the kernel when they are
in a static inline, it is best to move the function that contains the
tracepoint out of the header and into softirq.c.
Link: http://lkml.kernel.org/r/20120118120711.GB14863@elte.hu
Suggested-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
include/linux/interrupt.h | 7 +------
kernel/irq/chip.c | 2 ++
kernel/softirq.c | 6 ++++++
3 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index a64b00e..3f830e0 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -20,7 +20,6 @@
#include <linux/atomic.h>
#include <asm/ptrace.h>
#include <asm/system.h>
-#include <trace/events/irq.h>
/*
* These correspond to the IORESOURCE_IRQ_* defines in
@@ -456,11 +455,7 @@ asmlinkage void do_softirq(void);
asmlinkage void __do_softirq(void);
extern void open_softirq(int nr, void (*action)(struct softirq_action *));
extern void softirq_init(void);
-static inline void __raise_softirq_irqoff(unsigned int nr)
-{
- trace_softirq_raise(nr);
- or_softirq_pending(1UL << nr);
-}
+extern void __raise_softirq_irqoff(unsigned int nr);
extern void raise_softirq_irqoff(unsigned int nr);
extern void raise_softirq(unsigned int nr);
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index f7c543a..fc41824 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -16,6 +16,8 @@
#include <linux/interrupt.h>
#include <linux/kernel_stat.h>
+#include <trace/events/irq.h>
+
#include "internals.h"
/**
diff --git a/kernel/softirq.c b/kernel/softirq.c
index 4eb3a0f..06d4099 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -385,6 +385,12 @@ void raise_softirq(unsigned int nr)
local_irq_restore(flags);
}
+void __raise_softirq_irqoff(unsigned int nr)
+{
+ trace_softirq_raise(nr);
+ or_softirq_pending(1UL << nr);
+}
+
void open_softirq(int nr, void (*action)(struct softirq_action *))
{
softirq_vec[nr].action = action;
^ permalink raw reply related [flat|nested] 31+ messages in thread
end of thread, other threads:[~2012-03-29 5:51 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-16 22:57 [PATCH][GIT PULL][v3.3] tracing: Add header wrappers event_headers_start.h and event_headers_end.h Steven Rostedt
2012-01-17 9:54 ` Ingo Molnar
2012-01-17 13:32 ` Steven Rostedt
2012-01-18 12:07 ` Ingo Molnar
2012-01-18 17:56 ` Steven Rostedt
2012-01-22 22:59 ` Rusty Russell
2012-01-26 2:41 ` [RFC][PATCH] tracing/module: Move tracepoint out of module.h Steven Rostedt
2012-01-26 2:45 ` Steven Rostedt
2012-01-26 10:28 ` Ingo Molnar
2012-01-26 13:52 ` Steven Rostedt
2012-01-26 13:55 ` Ingo Molnar
2012-01-26 14:04 ` Steven Rostedt
2012-01-26 14:07 ` Steven Rostedt
2012-01-26 14:36 ` Steven Rostedt
2012-01-26 18:39 ` Ingo Molnar
2012-01-27 3:02 ` Rusty Russell
2012-01-30 11:52 ` Steven Rostedt
2012-01-30 17:28 ` Steven Rostedt
2012-01-31 3:58 ` Rusty Russell
2012-01-31 12:20 ` Ingo Molnar
2012-01-31 12:50 ` Steven Rostedt
2012-02-01 6:48 ` Rusty Russell
2012-02-01 13:27 ` Steven Rostedt
2012-02-01 13:49 ` Ingo Molnar
2012-02-01 14:25 ` Steven Rostedt
2012-03-29 4:22 ` Eric Dumazet
2012-03-29 5:24 ` Rusty Russell
2012-02-01 1:10 ` Rusty Russell
2012-02-01 7:09 ` Ingo Molnar
2012-01-30 6:40 ` Li Zefan
2012-02-17 13:46 ` [tip:perf/core] tracing/softirq: Move __raise_softirq_irqoff() out of header tip-bot for Steven Rostedt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).