Re: [PATCH v2 3/9] perf/cxl: Fix the counter overflow delta fixup

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] A race condition between a lockless PMU read and the PMU overflow IRQ handler causes a full counter period to be erroneously added, massively double-counting the event.
--

--- Patch [3]: [PATCH v2 3/9] perf/cxl: Fix the counter overflow delta fixup ---
commit 72c8c960c955004fc0b667e90e3698f58a0d0ea6
Author: Dave Jiang <[email protected]>

perf/cxl: Fix the counter overflow delta fixup

This commit updates the CXL PMU counter overflow handling since the counters
freeze on overflow and are not reloaded. It modifies the delta fixup logic
to trigger when new_cnt is greater than or equal to prev_cnt, adding the
full period back to account for the wrap.

Closes: https://sashiko.dev/#/patchset/[email protected]?part=1

> diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
> index 3138514157cde..956da82025513 100644
> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c

[ ... ]

> @@ -698,12 +698,18 @@ static void __cxl_pmu_read(struct perf_event *event, bool overflow)
>  	} while (local64_cmpxchg(&hwc->prev_count, prev_cnt, new_cnt) != prev_cnt);
>  
>  	/*
> -	 * If we know an overflow occur then take that into account.
> -	 * Note counter is not reset as that would lose events
> +	 * The counter wraps to 0 and is never reloaded, so the masked unsigned
> +	 * subtraction only recovers a wrap when new_cnt < prev_cnt. On the
> +	 * overflow path new_cnt >= prev_cnt means the full period elapsed
> +	 * (prev_cnt is 0 from event_start(), or the residual left by an earlier
> +	 * overflow), so add the period back. mask + 1 is 2^counter_width, which
> +	 * evaluates to 0 for a 64-bit counter - the correct sum mod 2^64, and
> +	 * avoids the undefined 1 << 64.
>  	 */
> -	delta = (new_cnt - prev_cnt) & GENMASK_ULL(info->counter_width - 1, 0);
> -	if (overflow && delta < GENMASK_ULL(info->counter_width - 1, 0))
> -		delta += (1UL << info->counter_width);
> +	mask = GENMASK_ULL(info->counter_width - 1, 0);
> +	delta = (new_cnt - prev_cnt) & mask;
> +	if (overflow && new_cnt >= prev_cnt)
> +		delta += mask + 1;

[Severity: High]
This is a pre-existing issue, but can this lead to massive double-counting
if a concurrent lockless read races with the overflow handler?

If a userspace lockless PMU read, such as perf stat -I, happens after the
counter wraps and freezes, but before the delayed overflow IRQ handler
executes, the lockless reader will correctly compute the wrapped delta and
update hwc->prev_count to the frozen residual.

When the IRQ handler in __cxl_pmu_read() subsequently runs, both prev_cnt
and new_cnt will read the same frozen residual. The condition
new_cnt >= prev_cnt will evaluate to true since they are equal, causing the
handler to blindly add a full period (mask + 1) on top of the already
accounted wrap.

Could this be addressed by distinguishing between a full period elapsing and
a frozen residual being consumed by a concurrent lockless read?

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