Re: [PATCH] tracing: Make branch profiler counters atomic

[sashiko-bot@kernel.org]

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 <nanshuaibo811@163.com>

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/20260930030728.1144418-1-nanshuaibo811@163.com?part=1