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

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

On Sat, 2015-04-04 at 22:56 +0200, Paul Bolle wrote:
> Tom Zanussi schreef op za 04-04-2015 om 15:09 [-0500]:
> > [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>
> 
> The commit explanation is silent about this change. Are you sure you
> want to include it?
> 

Probably, but I'll mention in the changelog.  I'll submit a complete
patch with my updated patchset (after looking around for other includes
of module.h to add).

> > 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>
> 
> Note to self: needed for "struct module". But this file has two
> references to THIS_MODULE that look useless. 
> 
> >  #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>
> 
> Can't this be dropped too?
> 

Yeah, of course that's the whole point, which I missed here ;-)

Tom

> >  #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>
> 
> Thanks,
> 
> 
> Paul Bolle
>