Re: [PATCH v9 17/18] 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 1 potential issue(s) to consider:
- [Medium] Performance regression due to `atomic_load` enforcing strict sequential consistency in frequently called `struct symbol` getters.
--

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

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

This commit addresses concurrent update issues by replacing bitfields in
struct symbol with a 16-bit atomic flags variable. It introduces atomic
accessors to read and modify these flags safely.

> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> --- a/tools/perf/util/symbol.h
> +++ b/tools/perf/util/symbol.h
[ ... ]
> @@ -77,6 +82,43 @@ struct symbol {
>  void symbol__delete(struct symbol *sym);
>  void symbols__delete(struct rb_root_cached *symbols);
>  
> +static inline u8 symbol__type(const struct symbol *sym)
> +{
> +	return (atomic_load(&sym->flags) & SYMBOL_FLAG_TYPE_MASK) >> SYMBOL_FLAG_TYPE_SHIFT;
> +}
> +
> +static inline u8 symbol__binding(const struct symbol *sym)
> +{
> +	return (atomic_load(&sym->flags) & SYMBOL_FLAG_BINDING_MASK) >> SYMBOL_FLAG_BINDING_SHIFT;
> +}
[ ... ]
> +static inline bool symbol__inlined(const struct symbol *sym)
> +{
> +	return (atomic_load(&sym->flags) & SYMBOL_FLAG_INLINED) != 0;
> +}

Could the use of atomic_load() here cause a performance regression on
weakly-ordered architectures?

By default, C11 atomic_load() implies memory_order_seq_cst. On architectures
like ARM64 or PowerPC, this generates memory barriers or load-acquire
instructions.

These getters are evaluated millions of times in extremely hot paths, such as
the qsort comparison function _sort__sym_cmp() in tools/perf/util/sort.c:

int64_t _sort__sym_cmp(struct symbol *sym_l, struct symbol *sym_r)
{
        ...
        if (symbol__inlined(sym_l) || symbol__inlined(sym_r)) {
                int ret = strcmp(sym_l->name, sym_r->name);
        ...
}

Emitting strict memory ordering instructions inside sorting loops prevents CPU
pipeline instruction reordering, which might significantly slow down sample
processing during perf report and perf top.

Since the read operations only require atomicity to prevent tearing, would it
be better to use atomic_load_explicit(&sym->flags, memory_order_relaxed)
instead?

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