Re: [PATCH v1 4/4] perf tools: Add test for weight annotation

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Background thread in perf top continually overwrites the TUI user preference for displaying weight columns due to an unprotected data race.
--

--- Patch [4]: [PATCH v1 4/4] perf tools: Add test for weight annotation ---
commit 7947c9d694355b6721ccd7944dc41891a9968db4
Author: Andi Kleen <[email protected]>

perf tools: Add support for displaying weights in annotate

Add support for showing all the three possible per IP weights in
annotate. The weights are shown by defaults if any are non zero. This
is useful, especially with the new insn lat statistics, but also
for all the existing weights.

> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 2888ac3b7b926..32bd6e23f6fdd 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -827,6 +827,7 @@ static void perf_event__process_sample(const struct perf_tool *tool,
>  			top->vmlinux_warned = true;
>  		}
>  	}
> +	symbol_conf__set_weight_mode(&symbol_conf, sample);

[Severity: Medium]
Will this background sample processing thread concurrently overwrite the TUI
user preference for displaying weight columns?

The TUI thread modifies the global symbol_conf structure when the user attempts
to hide the weight columns. If the background thread is continually calling
symbol_conf__set_weight_mode() on every sample without locking, it appears the
user's preference could get immediately overridden, causing the TUI to flicker
or misalign columns.

[ ... ]

> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index e47a467750890..dada5fa6d29f2 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
[ ... ]
> @@ -1091,6 +1092,14 @@ static int annotate_browser__run(struct annotate_browser *browser,
>  				symbol_conf.show_total_period = true;
>  			annotation__update_column_widths(notes);
>  			continue;
> +		case 'w':
> +			symbol_conf.hide_weight = !symbol_conf.hide_weight;
> +			symbol_conf.weight_mask = symbol_conf.hide_weight ? 0 :
> +				annotation__histogram(notes, evsel)->weight_mask;

[Severity: Medium]
Could this unprotected update to the global symbol_conf.weight_mask race with
the background thread's read-modify-write operations?

[ ... ]

> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 94f9c8faeddab..4b04320e35ecb 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
[ ... ]
> +void symbol_conf__set_weight_mode(struct symbol_conf *c,
> +				  struct perf_sample *sample)
> +{
> +	if (sample->weight)
> +		c->weight_mask |= BIT(WEIGHT_WEIGHT);

[Severity: Medium]
Does this unprotected read-modify-write on the global symbol_conf structure
corrupt the state when called concurrently with the TUI thread?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4