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

On 7/28/26 2:14 PM, [email protected] wrote:
> 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.

Will fix in v2.

DJ

> --
> 
> --- 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);
>>  }
>