Messages in this thread | | | Subject | Re: [PATCH v3 5/7] tracing: Add 'hist' event trigger command | From | Paul Bolle <> | Date | Sat, 04 Apr 2015 17:14:56 +0200 |
| |
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.
| |