* Peter Zijlstra <[email protected]> wrote:
> On Thu, Mar 07, 2019 at 01:18:41PM -0500, Steven Rostedt wrote: > > On Thu, 7 Mar 2019 09:45:35 -0800 > > Linus Torvalds <[email protected]> wrote: > > > > > On Thu, Mar 7, 2019 at 9:38 AM Peter Zijlstra <[email protected]> > > > wrote: > > > > > > > > Also; it seems to me that something PT, or maybe even simply: > > > > > > > > perf -e branches -e branch-misses > > > > > > > > would get you similar or sufficient information. > > > I currently have one of my engineers looking at the data and may be > > sending patches soon. It's basically an entry level way to get into > > kernel development. Note, no patch will be sent just because of the > > data from the profiling. The task is to look at and understand the > > code, and see if it can be optimized (with likely/unlikely or flow > > changes). It's a way to get a better understanding of the kernel in > > various locations. It is by no means "profiler said this, lets change > > it." All changes must be rational, and make sense. The profiler is only > > used to help find those places. > > Can't you just have those same engineers look at perf data? This seems > like a very expensive and convoluted way of getting something. So since no-one offered objections to using perf branch profiling instead (which method allows so much more than CONFIG_PROFILE_ALL_BRANCHES: such as profiling glibc and other user-space, or allowing to branch-profile the kernel is an uninstrumented form not distorted by CONFIG_PROFILE_ALL_BRANCHES code generation artifacts), lemme propose the attached patch to remove if-tracing. If the CONFIG_PROFILE_ALL_BRANCHES=y feature is required for anyone it can still be reverted privately or maintained out of tree - no need to burden the mainline kernel with this. I've build tested this and it Looks Perfect Here™. Thanks, Ingo =============================> >From 3f689ed8a1555aabead90e015a47aefddd2a4e25 Mon Sep 17 00:00:00 2001 From: Ingo Molnar <[email protected]> Date: Fri, 19 Apr 2019 11:42:43 +0200 Subject: [PATCH] compiler.h, tracing: Remove CONFIG_PROFILE_ALL_BRANCHES Redefining 'if' in compiler.h was a hideously wonderful hack back in 2008 when we merged it via: 2bcd521a684c: ("trace: profile all if conditionals") Meanwhile the 'wonderful' novelty part wore off a bit in that decade, and what remained is the 'hideous', mostly. ;-) Meanwhile #2: we also merged perf and hardware branch tracing capabilities (branch-miss events, but also BTS and aux hw tracing), which can collect similar data and so much more: $ perf -e branches -e branch-misses So let's remove this constant source of headaches for good. Anyone truly interested in this feature can revert this commit and/or maintain it out of tree - but the upstream pain isn't really worth it. Signed-off-by: Ingo Molnar <[email protected]> --- include/asm-generic/vmlinux.lds.h | 11 +---- include/linux/compiler.h | 24 ----------- kernel/trace/Kconfig | 17 -------- kernel/trace/trace_branch.c | 66 ----------------------------- tools/perf/tests/bpf-script-test-prologue.c | 9 ---- 5 files changed, 1 insertion(+), 126 deletions(-) diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h index f8f6f04c4453..9c477b2136c2 100644 --- a/include/asm-generic/vmlinux.lds.h +++ b/include/asm-generic/vmlinux.lds.h @@ -126,14 +126,6 @@ #define LIKELY_PROFILE() #endif -#ifdef CONFIG_PROFILE_ALL_BRANCHES -#define BRANCH_PROFILE() __start_branch_profile = .; \ - KEEP(*(_ftrace_branch)) \ - __stop_branch_profile = .; -#else -#define BRANCH_PROFILE() -#endif - #ifdef CONFIG_KPROBES #define KPROBE_BLACKLIST() . = ALIGN(8); \ __start_kprobe_blacklist = .; \ @@ -266,8 +258,7 @@ __start___verbose = .; \ KEEP(*(__verbose)) \ __stop___verbose = .; \ - LIKELY_PROFILE() \ - BRANCH_PROFILE() \ + LIKELY_PROFILE() \ TRACE_PRINTKS() \ BPF_RAW_TP() \ TRACEPOINT_STR() diff --git a/include/linux/compiler.h b/include/linux/compiler.h index d58aa0db05f9..c63105451c6a 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -48,30 +48,6 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val, # define unlikely(x) (__branch_check__(x, 0, __builtin_constant_p(x))) # endif -#ifdef CONFIG_PROFILE_ALL_BRANCHES -/* - * "Define 'is'", Bill Clinton - * "Define 'if'", Steven Rostedt - */ -#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) ) -#define __trace_if(cond) \ - if (__builtin_constant_p(!!(cond)) ? !!(cond) : \ - ({ \ - int ______r; \ - static struct ftrace_branch_data \ - __aligned(4) \ - __section("_ftrace_branch") \ - ______f = { \ - .func = __func__, \ - .file = __FILE__, \ - .line = __LINE__, \ - }; \ - ______r = !!(cond); \ - ______r ? ______f.miss_hit[1]++ : ______f.miss_hit[0]++;\ - ______r; \ - })) -#endif /* CONFIG_PROFILE_ALL_BRANCHES */ - #else # define likely(x) __builtin_expect(!!(x), 1) # define unlikely(x) __builtin_expect(!!(x), 0) diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig index 8bd1d6d001d7..169c34e0f16d 100644 --- a/kernel/trace/Kconfig +++ b/kernel/trace/Kconfig @@ -366,23 +366,6 @@ config PROFILE_ANNOTATED_BRANCHES Note: this will add a significant overhead; only turn this on if you need to profile the system's use of these macros. - -config PROFILE_ALL_BRANCHES - bool "Profile all if conditionals" if !FORTIFY_SOURCE - select TRACE_BRANCH_PROFILING - imply CC_DISABLE_WARN_MAYBE_UNINITIALIZED # avoid false positives - help - This tracer profiles all branch conditions. Every if () - taken in the kernel is recorded whether it hit or miss. - The results will be displayed in: - - /sys/kernel/debug/tracing/trace_stat/branch_all - - This option also enables the likely/unlikely profiler. - - This configuration, when enabled, will impose a great overhead - on the system. This should only be enabled when the system - is to be analyzed in much detail. endchoice config TRACING_BRANCHES diff --git a/kernel/trace/trace_branch.c b/kernel/trace/trace_branch.c index 3ea65cdff30d..be75301a9963 100644 --- a/kernel/trace/trace_branch.c +++ b/kernel/trace/trace_branch.c @@ -387,69 +387,3 @@ __init static int init_annotated_branch_stats(void) return 0; } fs_initcall(init_annotated_branch_stats); - -#ifdef CONFIG_PROFILE_ALL_BRANCHES - -extern unsigned long __start_branch_profile[]; -extern unsigned long __stop_branch_profile[]; - -static int all_branch_stat_headers(struct seq_file *m) -{ - seq_puts(m, " miss hit % " - " Function " - " File Line\n" - " ------- --------- - " - " -------- " - " ---- ----\n"); - return 0; -} - -static void *all_branch_stat_start(struct tracer_stat *trace) -{ - return __start_branch_profile; -} - -static void * -all_branch_stat_next(void *v, int idx) -{ - struct ftrace_branch_data *p = v; - - ++p; - - if ((void *)p >= (void *)__stop_branch_profile) - return NULL; - - return p; -} - -static int all_branch_stat_show(struct seq_file *m, void *v) -{ - struct ftrace_branch_data *p = v; - const char *f; - - f = branch_stat_process_file(p); - return branch_stat_show_normal(m, p, f); -} - -static struct tracer_stat all_branch_stats = { - .name = "branch_all", - .stat_start = all_branch_stat_start, - .stat_next = all_branch_stat_next, - .stat_headers = all_branch_stat_headers, - .stat_show = all_branch_stat_show -}; - -__init static int all_annotated_branch_stats(void) -{ - int ret; - - ret = register_stat_tracer(&all_branch_stats); - if (!ret) { - printk(KERN_WARNING "Warning: could not register " - "all branches stats\n"); - return 1; - } - return 0; -} -fs_initcall(all_annotated_branch_stats); -#endif /* CONFIG_PROFILE_ALL_BRANCHES */ diff --git a/tools/perf/tests/bpf-script-test-prologue.c b/tools/perf/tests/bpf-script-test-prologue.c index 43f1e16486f4..1f048bd89b0d 100644 --- a/tools/perf/tests/bpf-script-test-prologue.c +++ b/tools/perf/tests/bpf-script-test-prologue.c @@ -10,15 +10,6 @@ #include <uapi/linux/fs.h> -/* - * If CONFIG_PROFILE_ALL_BRANCHES is selected, - * 'if' is redefined after include kernel header. - * Recover 'if' for BPF object code. - */ -#ifdef if -# undef if -#endif - #define FMODE_READ 0x1 #define FMODE_WRITE 0x2
