Re: [PATCH 3/9] perf/cxl: Drop bogus counter overflow fixup

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Removing the overflow fixup entirely causes the CXL PMU driver to silently drop a full period of events on overflow when the new counter skid is greater than or equal to the previous skid.
--

--- Patch [3]: [PATCH 3/9] perf/cxl: Drop bogus counter overflow fixup ---
commit 9b5168bc2e13d25590b1bb314d5cd8b527ba4f3e
Author: Dave Jiang <[email protected]>

perf/cxl: Drop bogus counter overflow fixup

This commit removes the overflow fixup in __cxl_pmu_read() and folds it into
cxl_pmu_read(). The stated rationale is that masked unsigned subtraction
already produces the correct delta across a wrap, and that the previous
fixup caused overcounting.

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..410c9162c8246 100644
> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c
[ ... ]
> @@ -698,21 +698,15 @@ 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 is not reset on overflow, and the unsigned subtraction
> +	 * masked to the counter width already yields the correct delta across a
> +	 * single wrap, so no overflow fixup is needed.
>  	 */
>  	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);

[Severity: Medium]
Does removing the overflow fixup entirely cause the driver to drop a full
period of events when new_cnt >= prev_cnt?

While standard modulo math handles wraps when the counter is reprogrammed to
a high value (ensuring new_cnt < prev_cnt), the CXL PMU does not reprogram
counters on overflow. They start at 0, wrap, and freeze at a small skid
value.

If a counter overflows, prev_cnt becomes skid1. On the next overflow,
new_cnt becomes skid2. If skid2 >= skid1 (which has a roughly 50% chance),
the masked subtraction only yields the tiny difference (skid2 - skid1),
completely missing the wrapped period.

Would it be more correct to keep an overflow fixup specifically for the
new_cnt >= prev_cnt case when an overflow occurs?

>  
>  	local64_add(delta, &event->count);
>  }

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