Re: [PATCH v3 5/7] tracing: Add 'hist' event trigger command

[Tom Zanussi <tom.zanussi@linux.intel.com>]

On Sat, 2015-04-04 at 17:14 +0200, Paul Bolle wrote:
> What follows are a bunch of questions, and not really review remarks,
> triggered by the fact that <linux/module.h> is included here for reasons
> that were not really obvious when scanning the patch.
> 
> TL,DR:
> - why does trace_events_hist.c include <linux/module.h>?
> - why doesn't <linux/kallsyms.h> include <linux/module.h> instead?
> - why does <linux/ftrace.h> still include <linux/kallsyms.h>?
> - and why doesn't trace_events_hist.c include <linux/kallsyms.h>
>   directly instead?
> 
> Even shorter: shouldn't these files include the headers they need
> directly and not rely on other headers to pull them in?
> 

All good questions - not sure why linux/kallsyms.h doesn't include
module.h, which would solve the problem.

> On Fri, 2015-04-03 at 10:51 -0500, Tom Zanussi wrote:
> > --- a/kernel/trace/Kconfig
> > +++ b/kernel/trace/Kconfig
> 
> > +config HIST_TRIGGERS
> > +	bool "Histogram triggers"
> > +	depends on ARCH_HAVE_NMI_SAFE_CMPXCHG
> > +	help
> > +	  [...]
> 
> > --- a/kernel/trace/Makefile
> > +++ b/kernel/trace/Makefile
> 
> > +obj-$(CONFIG_HIST_TRIGGERS) += trace_events_hist.o
> 
> To make sure I'm parsing this Makefile correctly: trace_events_hist.o
> will never be part of a module, right?
> 

Right.

> > --- /dev/null
> > +++ b/kernel/trace/trace_events_hist.c
> 
> > +#include <linux/module.h>
> 
> When scanning this patch I wondered why this include was needed. Because
> this file will never be part of a module and I can't spot anything
> obviously module related in the code.
> 
> But deleting that include triggers errors when building
> trace_events_hist.o:
>     In file included from include/linux/ftrace.h:10:0,
>                      from kernel/trace/trace.h:12,
>                      from kernel/trace/trace_events_hist.c:30:
>     kernel/trace/trace_events_hist.c: In function ‘hist_trigger_stacktrace_print’:
>     include/linux/kallsyms.h:14:31: error: ‘MODULE_NAME_LEN’ undeclared (first use in this function)
>          2*(BITS_PER_LONG*3/10) + (MODULE_NAME_LEN - 1) + 1)
>                                    ^
>     kernel/trace/trace_events_hist.c:901:11: note: in expansion of macro ‘KSYM_SYMBOL_LEN’
>       char str[KSYM_SYMBOL_LEN];
>                ^
>     include/linux/kallsyms.h:14:31: note: each undeclared identifier is reported only once for each function it appears in
>          2*(BITS_PER_LONG*3/10) + (MODULE_NAME_LEN - 1) + 1)
>                                    ^
>     [...]
> 
> Looking into that I noticed that <linux/kallsyms.h> doesn't include
> <linux/module.h>, even though it uses MODULE_NAME_LEN. So shouldn't it
> include that header too?
> 
> The error I quoted above shows that <linux/kallsyms.h> is included
> indirectly (via "trace.h" and <linux/ftrace.h>). But <linux/ftrace.h>
> itself doesn't use anything from <linux/kallsyms.h>[0]. So I wonder why
> <linux/ftrace.h> still includes <linux/kallsyms.h>. Just so that other
> files can rely on it to be pulled in if they include <linux/ftrace.h>?
> 
> See for instance trace_events_hist.c, which I'm discussing here. It uses
> things like KSYM_SYMBOL_LEN, so shouldn't it include <linux/kallsyms.h>
> directly instead of relying of <linux/ftrace.h> to do so on its behalf?
> 
> 

Yes, so something like the below could be a start, which fixes up the
problem for trace, but misses a lot of potential others, such as this
obvious one from lib/vsprintf.c?:

  #include <linux/module.h>	/* for KSYM_SYMBOL_LEN */


----


[PATCH] kallsyms: Include module.h

KSYM_SYMBOL_LEN references MODULE_NAME_LEN, defined in module.h, so
have kallsyms.h include module.h instead of expecting kallsysms users
to do it.

Also fix up existing kallsyms users that apparently include module.h
for only that reason.

Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
---
 include/linux/ftrace.h           | 1 -
 include/linux/kallsyms.h         | 1 +
 kernel/trace/trace_events_hist.c | 2 +-
 kernel/trace/trace_kprobe.c      | 1 +
 kernel/trace/trace_output.c      | 1 +
 kernel/trace/trace_syscalls.c    | 2 +-
 6 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 1da6029..8554dd5 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -7,7 +7,6 @@
 #define _LINUX_FTRACE_H
 
 #include <linux/trace_clock.h>
-#include <linux/kallsyms.h>
 #include <linux/linkage.h>
 #include <linux/bitops.h>
 #include <linux/ptrace.h>
diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index 6883e19..e1550a4 100644
--- a/include/linux/kallsyms.h
+++ b/include/linux/kallsyms.h
@@ -8,6 +8,7 @@
 #include <linux/errno.h>
 #include <linux/kernel.h>
 #include <linux/stddef.h>
+#include <linux/module.h>
 
 #define KSYM_NAME_LEN 128
 #define KSYM_SYMBOL_LEN (sizeof("%s+%#lx/%#lx [%s]") + (KSYM_NAME_LEN - 1) + \
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 3d648ee..c290379 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -20,7 +20,7 @@
  * http://www.azulsystems.com/events/javaone_2007/2007_LockFreeHash.pdf
  */
 
-#include <linux/module.h>
+#include <linux/kallsyms.h>
 #include <linux/ctype.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 9ba3f43..39019ec 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -19,6 +19,7 @@
 
 #include <linux/module.h>
 #include <linux/uaccess.h>
+#include <linux/kallsyms.h>
 
 #include "trace_probe.h"
 
diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
index 692bf71..5ec310e 100644
--- a/kernel/trace/trace_output.c
+++ b/kernel/trace/trace_output.c
@@ -8,6 +8,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/ftrace.h>
+#include <linux/kallsyms.h>
 
 #include "trace_output.h"
 
diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
index d137e0a..72af617 100644
--- a/kernel/trace/trace_syscalls.c
+++ b/kernel/trace/trace_syscalls.c
@@ -3,7 +3,7 @@
 #include <linux/syscalls.h>
 #include <linux/slab.h>
 #include <linux/kernel.h>
-#include <linux/module.h>	/* for MODULE_NAME_LEN via KSYM_SYMBOL_LEN */
+#include <linux/kallsyms.h>
 #include <linux/ftrace.h>
 #include <linux/perf_event.h>
 #include <asm/syscall.h>
-- 
1.9.1



> Paul Bolle
> 
> [0] To be thorough: the need for <linux/ftrace.h> to include
> <linux/kallsyms.h> _for itself_ probably ended with commit 9c24624727f6
> ("KSYM_SYMBOL_LEN fixes"), which shipped in v2.6.28.
>