linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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-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

* 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: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-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  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-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

* [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

* 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

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