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

[Steven Rostedt]

On Sat, 04 Apr 2015 17:14:56 +0200
Paul Bolle <pebolle@xxxxxxxxxx> 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?

Some of the above looks to be set for clean up patches. Not all related
to this patch set.

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

You don't need to be part of a module to need module.h. You may just
need module info.

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

Technically yes. But its use is hidden within a macro, so perhaps it's
not necessary. Maybe it is to prevent include hell?

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

kallsyms.h was added to ftrace.h because ftrace.h had used
KSYM_NAME_LEN in the past, but has since removed that code but the
kallsyms.h was not removed (looks like clean up patches are needed).

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

Yeah, it should not rely on ftrace.h as that should have its headers
cleaned up. When changing code, it is easy to find what headers are
needed to build (it wont build without the right ones), but when
removing code, it's not easy to know what headers also need to be
removed, and that is where we end up with too many headers included.

I'm sure there's tools to help out with that.

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

Actually, the need for ftrace.h to stop including kallsyms ended with
commit 3f5ec13696fd "tracing/fastboot: move boot tracer structs and
funcs into their own header". And eventually, that code was also
removed when tracepoints made it obsolete.

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/