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

[Paul Bolle <pebolle@tiscali.nl>]

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?

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?

> --- /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?


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.