[PATCH] tracing: Make branch profiler counters atomic

[PATCH] tracing: Make branch profiler counters atomic

[nanshuaibo]

The branch profiler updates its static counters from arbitrary contexts. Concurrent updates can race and lose counts. KCSAN reports a data race between ftrace_likely_update() invocations.

Use relaxed compiler atomic operations for the counters. The profiler metadata is defined in compiler_types.h, before the kernel atomic API is available. The counters do not order accesses to other data.

Also use atomic loads when reading the counters for tracefs output and sorting, so readers do not race with atomic writers.

Tested on x86_64 QEMU/KVM with KCSAN and CONFIG_PROFILE_ANNOTATED_BRANCHES=y. The baseline reports the race in ftrace_likely_update(); it is not reported after this change.

Signed-off-by: nanshuaibo <nanshuaibo811@163.com>
---
 include/linux/compiler.h    | 14 ++++++++++--
 kernel/trace/trace_branch.c | 43 ++++++++++++++++++++++---------------
 2 files changed, 38 insertions(+), 19 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index cb2f6050b..7efbcd6aa 100644
--- 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)
+#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)
@@ -66,8 +76,8 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
 			.line = __LINE__,		\
 		};					\
 	(cond) ?					\
-		(__if_trace.miss_hit[1]++,1) :		\
-		(__if_trace.miss_hit[0]++,0);		\
+		(ftrace_branch_counter_inc(__if_trace.miss_hit[1]), 1) :\
+		(ftrace_branch_counter_inc(__if_trace.miss_hit[0]), 0);\
 })
 
 #endif /* CONFIG_PROFILE_ALL_BRANCHES */
diff --git a/kernel/trace/trace_branch.c b/kernel/trace/trace_branch.c
index d8e97ad79..36b61b5ed 100644
--- a/kernel/trace/trace_branch.c
+++ b/kernel/trace/trace_branch.c
@@ -73,7 +73,7 @@ probe_likely_condition(struct ftrace_likely_data *f, int val, int expect)
 
 	strscpy(entry->func, f->data.func);
 	strscpy(entry->file, p);
-	entry->constant = f->constant;
+	entry->constant = ftrace_branch_counter_read(f->constant);
 	entry->line = f->data.line;
 	entry->correct = val == expect;
 
@@ -202,7 +202,7 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
 
 	/* A constant is always correct */
 	if (is_constant) {
-		f->constant++;
+		ftrace_branch_counter_inc(f->constant);
 		val = expect;
 	}
 	/*
@@ -213,11 +213,10 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
 	 */
 	trace_likely_condition(f, val, expect);
 
-	/* FIXME: Make this atomic! */
 	if (val == expect)
-		f->data.correct++;
+		ftrace_branch_counter_inc(f->data.correct);
 	else
-		f->data.incorrect++;
+		ftrace_branch_counter_inc(f->data.incorrect);
 
 	user_access_restore(flags);
 }
@@ -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;
 }
@@ -282,7 +283,8 @@ static void branch_stat_show(struct seq_file *m,
 static int branch_stat_show_normal(struct seq_file *m,
 				   struct ftrace_branch_data *p, const char *f)
 {
-	seq_printf(m, "%8lu %8lu ",  p->correct, p->incorrect);
+	seq_printf(m, "%8lu %8lu ", ftrace_branch_counter_read(p->correct),
+		   ftrace_branch_counter_read(p->incorrect));
 	branch_stat_show(m, p, f);
 	return 0;
 }
@@ -291,18 +293,21 @@ static int annotate_branch_stat_show(struct seq_file *m, void *v)
 {
 	struct ftrace_likely_data *p = v;
 	const char *f;
+	unsigned long constant;
 	int l;
 
 	f = branch_stat_process_file(&p->data);
+	constant = ftrace_branch_counter_read(p->constant);
 
-	if (!p->constant)
+	if (!constant)
 		return branch_stat_show_normal(m, &p->data, f);
 
-	l = snprintf(NULL, 0, "/%lu", p->constant);
+	l = snprintf(NULL, 0, "/%lu", constant);
 	l = l > 8 ? 0 : 8 - l;
 
 	seq_printf(m, "%8lu/%lu %*lu ",
-		   p->data.correct, p->constant, l, p->data.incorrect);
+		   ftrace_branch_counter_read(p->data.correct), constant, l,
+		   ftrace_branch_counter_read(p->data.incorrect));
 	branch_stat_show(m, &p->data, f);
 	return 0;
 }
@@ -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;
 
-	if (a->incorrect < b->incorrect)
+	if (a_incorrect < b_incorrect)
 		return -1;
-	if (a->incorrect > b->incorrect)
+	if (a_incorrect > b_incorrect)
 		return 1;
 
 	/*
@@ -350,9 +359,9 @@ static int annotated_branch_stat_cmp(const void *p1, const void *p2)
 	 * first, we continue that by showing best (correct)
 	 * cases last.
 	 */
-	if (a->correct > b->correct)
+	if (a_correct > b_correct)
 		return -1;
-	if (a->correct < b->correct)
+	if (a_correct < b_correct)
 		return 1;
 
 	return 0;
-- 
2.43.0

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

[sashiko-bot]

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

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

[Steven Rostedt]

On Wed, 30 Sep 2026 11:07:28 +0800
nanshuaibo <nanshuaibo811@163.com> wrote:

> The branch profiler updates its static counters from arbitrary contexts. Concurrent updates can race and lose counts. KCSAN reports a data race between ftrace_likely_update() invocations.
> 
> Use relaxed compiler atomic operations for the counters. The profiler metadata is defined in compiler_types.h, before the kernel atomic API is available. The counters do not order accesses to other data.
> 
> Also use atomic loads when reading the counters for tracefs output and sorting, so readers do not race with atomic writers.
> 
> Tested on x86_64 QEMU/KVM with KCSAN and CONFIG_PROFILE_ANNOTATED_BRANCHES=y. The baseline reports the race in ftrace_likely_update(); it is not reported after this change.

FYI, Change log lines should be capped at 76 characters except for cut
and pasted output.

That said, NAK to the patch. The branch profile is a best effort and
known to be racy. It's to find where branches are most traveled, Their
exact numbers are not meaningful. No need for atomic operations.

-- Steve