Re: [PATCH v7 4/4] perf symbol: Lazily compute idle and use a global lock for updates

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The new lazy evaluation helper `symbol__is_idle()` is never called, and the migration to a tri-state enum breaks existing boolean consumers of `sym->idle`.
- [High] Data race on lockless read of the `sym->idle` bitfield in `symbol__is_idle()`.
--

commit 2ef259ec3239aea58dbf7c1d14b3998396292f27
Author: Ian Rogers <irogers@google.com>

perf symbol: Lazily compute idle and use a global lock for updates

This patch moves the idle boolean computation into a lazy helper,
symbol__is_idle(), considering kernel version and architecture. It also
changes the idle field to a tri-state enum and protects updates with a
global lock.

Link: https://lore.kernel.org/lkml/20260219113850.354271-1-tmricht@linux.ibm.com/
Link: https://lore.kernel.org/lkml/20260323085255.98173-1-jameshongleiwang@126.com/

> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index e6a1f23634ec..8ec4b2836b44 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
[ ... ]
> @@ -384,17 +389,6 @@ void __symbols__insert(struct rb_root_cached *symbols,
>  	struct symbol *s;
>  	bool leftmost = true;
>  
> -	if (kernel) {
> -		const char *name = sym->name;
> -		/*
> -		 * ppc64 uses function descriptors and appends a '.' to the
> -		 * start of every instruction address. Remove it.
> -		 */
> -		if (name[0] == '.')
> -			name++;
> -		sym->idle = symbol__is_idle(name);
> -	}

Is the new symbol__is_idle() helper actually called anywhere to lazily
initialize this field?

It looks like the call was removed here, but the new function doesn't appear
to be invoked anywhere else by the consumers.

[ ... ]
> +bool symbol__is_idle(struct symbol *sym, const struct dso *dso, struct perf_env *env)
> +{
> +	static const char * const idle_symbols[] = {
[ ... ]
> +	};
> +	const char *name = sym->name;
> +	uint16_t e_machine = perf_env__e_machine(env, /*e_flags=*/NULL);
>  
> -	if (idle_symbols_list)
> -		return strlist__has_entry(idle_symbols_list, name);
> +	if (sym->idle)
> +		return sym->idle == SYMBOL_IDLE__IDLE;

Does reading the sym->idle bitfield here without holding symbol_bits_lock
cause a data race?

Since adjacent bitfields in struct symbol share the same memory location,
a concurrent update to them performs a read-modify-write cycle that could
race with this lockless read, potentially resulting in undefined behavior.

[ ... ]
> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> index 5d98d7e84d57..717d2f876d58 100644
> --- a/tools/perf/util/symbol.h
> +++ b/tools/perf/util/symbol.h
> @@ -43,6 +43,12 @@ Elf_Scn *elf_section_by_name(Elf *elf, GElf_Ehdr *ep,
>  			     GElf_Shdr *shp, const char *name, size_t *idx);
>  #endif
>  
> +enum symbol_idle_kind {
> +	SYMBOL_IDLE__UNKNOWN = 0,
> +	SYMBOL_IDLE__NOT_IDLE = 1,
> +	SYMBOL_IDLE__IDLE = 2,
> +};
> +
>  /**
>   * A symtab entry. When allocated this may be preceded by an annotation (see
>   * symbol__annotation) and/or a browser_index (see symbol__browser_index).
> @@ -58,8 +64,8 @@ struct symbol {
>  	u8		type:4;
>  	/** ELF binding type as defined for st_info. E.g. STB_WEAK or STB_GLOBAL. */
>  	u8		binding:4;
> -	/** Set true for kernel symbols of idle routines. */
> -	u8		idle:1;
> +	/** Cache for symbol__is_idle holding enum symbol_idle_kind values. */
> +	u8		idle:2;

Are there still places evaluating sym->idle directly as a boolean?

Since idle now defaults to SYMBOL_IDLE__UNKNOWN (0), a check like
!al.sym->idle would evaluate to true for all symbols initially, bypassing
existing filters.

If a non-idle symbol is later evaluated and set to SYMBOL_IDLE__NOT_IDLE (1),
wouldn't !al.sym->idle evaluate to false, causing valid symbols to be dropped
incorrectly?

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