From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753107AbbDDUJO (ORCPT ); Sat, 4 Apr 2015 16:09:14 -0400 Received: from mga09.intel.com ([134.134.136.24]:19928 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752756AbbDDUJK (ORCPT ); Sat, 4 Apr 2015 16:09:10 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.11,523,1422950400"; d="scan'208";a="476802197" Message-ID: <1428178145.2610.13.camel@picadillo> Subject: Re: [PATCH v3 5/7] tracing: Add 'hist' event trigger command From: Tom Zanussi To: Paul Bolle Cc: rostedt@goodmis.org, masami.hiramatsu.pt@hitachi.com, namhyung@kernel.org, andi@firstfloor.org, linux-kernel@vger.kernel.org Date: Sat, 04 Apr 2015 15:09:05 -0500 In-Reply-To: <1428160496.7898.132.camel@x220> References: <9fe50519aa2cac1550b40a0e396dd721eff03574.1428072891.git.tom.zanussi@linux.intel.com> <1428160496.7898.132.camel@x220> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.10.4 (3.10.4-4.fc20) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 is included here for reasons > that were not really obvious when scanning the patch. > > TL,DR: > - why does trace_events_hist.c include ? > - why doesn't include instead? > - why does still include ? > - and why doesn't trace_events_hist.c include > 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 > > 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 doesn't include > , even though it uses MODULE_NAME_LEN. So shouldn't it > include that header too? > > The error I quoted above shows that is included > indirectly (via "trace.h" and ). But > itself doesn't use anything from [0]. So I wonder why > still includes . Just so that other > files can rely on it to be pulled in if they include ? > > See for instance trace_events_hist.c, which I'm discussing here. It uses > things like KSYM_SYMBOL_LEN, so shouldn't it include > directly instead of relying of 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 /* 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 --- 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 -#include #include #include #include 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 #include #include +#include #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 +#include #include #include #include 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 #include +#include #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 #include #include +#include #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 #include #include -#include /* for MODULE_NAME_LEN via KSYM_SYMBOL_LEN */ +#include #include #include #include -- 1.9.1 > Paul Bolle > > [0] To be thorough: the need for to include > _for itself_ probably ended with commit 9c24624727f6 > ("KSYM_SYMBOL_LEN fixes"), which shipped in v2.6.28. >