Re: [PATCH v7 3/4] 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 4 potential issue(s) to consider:
- [High] Data Race / UB on concurrent reads of bitfields
- [High] Architectural flaw and severe performance regression due to unconditional global lock
- [High] Subsystem rule violation: Event handler completeness missing in builtin-sched.c
- [Medium] Unbalanced lifecycle and resource leak on error paths in symbol__init()
--

commit eaf96d025426eab89c805716fc26dd2399ff6394
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 mutex to serialize modifications to bitfields
within the symbol structure. Helper functions are provided to update these
fields safely across concurrent threads.

> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 555247568e7a6..655e95f660c2f 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c

This isn't a bug in the current changes, but while reviewing builtin-sched.c,
it appears the sched tool registers the .mmap handler but misses .mmap2.

When a subcommand registers .mmap but not .mmap2, modern kernels generating
mmap2 events will have those events silently dropped. Is it possible this leaves
the machine vmlinux maps and process memory maps incomplete or null?

[ ... ]

> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index fd332db561575..e6a1f23634ec3 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);
> +}

Are concurrent reads left unprotected by this implementation?

While the lock protects the read-modify-write cycle of the bitfield updates
here, existing readers access these fields without holding the lock.
For example:

builtin-sched.c:save_task_callchain() {
    ...
    if (sym->ignore)
        ...
}

Under the C11 memory model, reading a bitfield while another thread
concurrently modifies an adjacent bitfield in the same byte constitutes
a data race. Could this result in torn reads and undefined behavior?

Additionally, is a global lock necessary for these updates?

Unconditional acquisition of a global lock on hot paths like
timehist_save_callchain() and save_task_callchain() might cause a performance
regression, especially since it is acquired even if the flag is already true.

On 64-bit systems, struct symbol has 3 bytes of internal padding before
the dso pointer. Converting the bitfields to separate bool or u8 fields
would allow them to occupy this padding. This layout would ensure each field
resides in a separate byte, allowing hardware byte-level atomicity to
eliminate false sharing and the need for locking entirely.

[ ... ]
> @@ -2398,6 +2415,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));

Could this skip destroying the mutex on error paths?

If setup_list() or other initializations fail later in symbol__init(),
the function returns -1 or jumps to error labels without calling
mutex_destroy(). If the caller retries initialization, calling mutex_init()
on an already-initialized mutex can result in undefined behavior.

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