From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752648AbbDDPPE (ORCPT ); Sat, 4 Apr 2015 11:15:04 -0400 Received: from lb2-smtp-cloud6.xs4all.net ([194.109.24.28]:60097 "EHLO lb2-smtp-cloud6.xs4all.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751998AbbDDPPB (ORCPT ); Sat, 4 Apr 2015 11:15:01 -0400 Message-ID: <1428160496.7898.132.camel@x220> Subject: Re: [PATCH v3 5/7] tracing: Add 'hist' event trigger command From: Paul Bolle To: Tom Zanussi 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 17:14:56 +0200 In-Reply-To: <9fe50519aa2cac1550b40a0e396dd721eff03574.1428072891.git.tom.zanussi@linux.intel.com> References: <9fe50519aa2cac1550b40a0e396dd721eff03574.1428072891.git.tom.zanussi@linux.intel.com> 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 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? 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 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? 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.