Re: [PATCH v8 16/17] perf symbol: Add setters for bitfields sharing a byte to avoid concurrent update issues

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unconditional acquisition of a global mutex in high-frequency per-sample fast paths causes severe performance regression.
- [High] Unlocked reads of bitfields concurrent with locked writes cause data races and undefined behavior.
- [Medium] The `symbol_bits_lock` mutex is leaked and potentially re-initialized if `symbol__init()` fails.
--

commit 587302f6555ca9ddcc54f4761c10004005eaa78f
Author: Ian Rogers <irogers@google.com>

perf symbol: Add setters for bitfields sharing a byte to avoid concurrent update issues

This commit introduces a global symbol_bits_lock and helper functions
to serialize updates to the ignore and annotate2 bitfields in struct symbol.
This aims to prevent lost updates caused by concurrent modifications to
bitfields sharing the same byte.

> diff --git a/tools/perf/builtin-kwork.c b/tools/perf/builtin-kwork.c
> index 9d3a4c779a41..7337ee956dc9 100644
> --- a/tools/perf/builtin-kwork.c
> +++ b/tools/perf/builtin-kwork.c
[ ... ]
> @@ -725,7 +725,7 @@ static void timehist_save_callchain(struct perf_kwork *kwork,
>  		if (sym) {
>  			if (!strcmp(sym->name, "__softirqentry_text_start") ||
>  			    !strcmp(sym->name, "__do_softirq"))
> -				sym->ignore = 1;
> +				symbol__set_ignore(sym, true);
>  		}
>  
>  		callchain_cursor_advance(cursor);

Since timehist_save_callchain is a per-sample fast path that can be called
millions of times per run, could this unconditional acquisition of a global
mutex cause severe lock contention and performance regression?

Would it be better to add a lockless check like if (!sym->ignore) to bypass
the lock once the flag is already set?

> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 555247568e7a..655e95f660c2 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
[ ... ]
> @@ -2371,7 +2371,7 @@ static void save_task_callchain(struct perf_sched *sched,
>  			if (!strcmp(sym->name, "schedule") ||
>  			    !strcmp(sym->name, "__schedule") ||
>  			    !strcmp(sym->name, "preempt_schedule"))
> -				sym->ignore = 1;
> +				symbol__set_ignore(sym, true);
>  		}

Could the same fast-path lock contention happen here during
save_task_callchain?

> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index a70066d17729..1238a0d6ce6e 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
[ ... ]
> @@ -345,6 +348,20 @@ void symbol__delete(struct symbol *sym)
>  	free(((void *)sym) - symbol_conf.priv_size);
>  }
>  
> +void symbol__set_ignore(struct symbol *sym, bool ignore)
> +{
> +	mutex_lock(&symbol_bits_lock);
> +	sym->ignore = ignore;
> +	mutex_unlock(&symbol_bits_lock);
> +}
> +
> +void symbol__set_annotate2(struct symbol *sym, bool annotate2)
> +{
> +	mutex_lock(&symbol_bits_lock);
> +	sym->annotate2 = annotate2;
> +	mutex_unlock(&symbol_bits_lock);
> +}

While the new symbol_bits_lock serializes writes to these bitfields, readers
of sym->ignore and sym->annotate2 remain unlocked throughout the codebase.

In C, adjacent bitfields share the same underlying memory location. Under the
C11 memory model, does reading a bitfield without synchronization while another
thread performs a read-modify-write on that same memory location constitute a
data race?

Could this lead to torn reads or undefined behavior during concurrent access?

> @@ -2415,6 +2432,8 @@ int symbol__init(struct perf_env *env)
>  	if (symbol_conf.initialized)
>  		return 0;
>  
> +	mutex_init(&symbol_bits_lock);
> +
>  	symbol_conf.priv_size = PERF_ALIGN(symbol_conf.priv_size, sizeof(u64));
>  
>  	symbol__elf_init();

If a subsequent initialization step fails, like setup_parallelism_bitmap,
this function returns -1 without calling mutex_destroy on the error paths.

Because symbol_conf.initialized remains false, a subsequent call to
symbol__exit will also return early and fail to clean the mutex up.

Additionally, if a caller attempts to retry symbol__init after a failure,
could this cause mutex_init to be invoked on an already-initialized mutex?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260502065935.291960-1-irogers@google.com?part=16