Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] tracing: severe performance regression from locked atomic instructions on branches - [Low] tracing: build failure on non-atomic archs due to compiler built-in atomics --
commit 9ef68553e3432c59dcd576b4a0f6c62d77db0cf7 Author: nanshuaibo <[email protected]> tracing: Make branch profiler counters atomic The branch profiler updates its static counters from arbitrary contexts. Concurrent updates can race and lose counts, resulting in KCSAN data race reports. This patch uses relaxed compiler atomic operations for the counters and atomic loads for reading the tracefs output. > diff --git a/include/linux/compiler.h b/include/linux/compiler.h > --- a/include/linux/compiler.h > +++ b/include/linux/compiler.h [ ... ] > @@ -14,6 +14,16 @@ > */ > void ftrace_likely_update(struct ftrace_likely_data *f, int val, > int expect, int is_constant); > + > +/* > + * Branch profiler data is defined in compiler_types.h, before the atomic API > + * is available. These counters are independent statistics and do not order > + * accesses to other data. > + */ > +#define ftrace_branch_counter_inc(counter) \ > + __atomic_fetch_add(&(counter), 1, __ATOMIC_RELAXED) [Severity: Low] Does directly invoking the GCC built-in here cause linking failures on certain architectures? On architectures without native lock-free atomic hardware instructions for unsigned long (like SPARC32), GCC lowers this built-in to a library call such as __atomic_fetch_add_4. Since the kernel does not link against libatomic, will this unconditionally fail to build at link time on those architectures when branch profiling is enabled? [Severity: Medium] Could turning this into an atomic instruction cause a severe performance regression? When CONFIG_PROFILE_ALL_BRANCHES or CONFIG_PROFILE_ANNOTATED_BRANCHES is enabled, using __atomic_fetch_add compels the compiler to emit locked instructions (e.g., LOCK XADD on x86) for every evaluated branch condition kernel-wide. Can this result in CPU pipeline stalls and cache-line bouncing? > +#define ftrace_branch_counter_read(counter) \ > + __atomic_load_n(&(counter), __ATOMIC_RELAXED) > #if defined(CONFIG_TRACE_BRANCH_PROFILING) \ > && !defined(DISABLE_BRANCH_PROFILING) && !defined(__CHECKER__) > #define likely_notrace(x) __builtin_expect(!!(x), 1) [ ... ] > diff --git a/kernel/trace/trace_branch.c b/kernel/trace/trace_branch.c > --- a/kernel/trace/trace_branch.c > +++ b/kernel/trace/trace_branch.c [ ... ] > @@ -239,13 +238,15 @@ static int annotated_branch_stat_headers(struct > seq_file *m) > > static inline long get_incorrect_percent(const struct ftrace_branch_data *p) > { > + unsigned long correct = ftrace_branch_counter_read(p->correct); > + unsigned long incorrect = ftrace_branch_counter_read(p->incorrect); > long percent; > > - if (p->correct) { > - percent = p->incorrect * 100; > - percent /= p->correct + p->incorrect; > + if (correct) { > + percent = incorrect * 100; > + percent /= correct + incorrect; > } else > - percent = p->incorrect ? 100 : -1; > + percent = incorrect ? 100 : -1; > > return percent; > } [ ... ] > @@ -329,6 +334,10 @@ static int annotated_branch_stat_cmp(const void *p1, > const void *p2) > { > const struct ftrace_branch_data *a = p1; > const struct ftrace_branch_data *b = p2; > + unsigned long a_incorrect = ftrace_branch_counter_read(a->incorrect); > + unsigned long b_incorrect = ftrace_branch_counter_read(b->incorrect); > + unsigned long a_correct = ftrace_branch_counter_read(a->correct); > + unsigned long b_correct = ftrace_branch_counter_read(b->correct); > > long percent_a, percent_b; > > @@ -340,9 +349,9 @@ static int annotated_branch_stat_cmp(const void *p1, > const void *p2) > if (percent_a > percent_b) > return 1; [Severity: Low] This is a pre-existing issue, but does this pattern create a race condition when computing these percentages? annotated_branch_stat_cmp() takes an initial snapshot of the lockless counters into local variables, but then calls get_incorrect_percent() which takes a second snapshot of the exact same counters. If the live counters change between these snapshots while user-space actively reads the debugfs trace stat file, the computed percentage may not match the fallback raw counts used for tie-breaking. Could this yield internally inconsistent sorting keys and violate the strict weak ordering required by the rbtree implementation? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
