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

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 29 Jul 2026 07:55:49 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Counters are configured with Freeze on Overflow and are never reloaded: on
> overflow the counter wraps to 0, counts on until the CPMU freezes, and
Hi Dave,

Thanks for looking at these.

Why would it count on if it froze?  I think the bot is tripping over the
fact we don't yet implement free running counters (and the other bug
about not unfreezing) for currently the ability to freeze on only some
counters (to do periodic sampling for instance).

The CXL CPMU spec is incredibly broad in what is supported, so maybe
we want to harden things anyway but I'm not sure the condition described
by most of this is real.

> retains that residual (CXL r4.0 8.2.7.2.3). So the masked subtraction in
> __cxl_pmu_read() only spans the wrap when new_cnt < prev_cnt.

It's been a long time so maybe I have how this was meant to work wrong.

There are two paths to __cxl_pmu_read()

1. We have freeze on overflow enabled so any counter that overflows results
in an interrupt. At that point all counters are frozen.
We then read only the counter that overflowed (which is 0) and that
will update the prev_cnt storage.  No chance of hitting the full wrap
around seen here.

2. An on demand read (polling) In this case the counter may
take any value, but because we have freeze on overflow it can't have
wrapped (as otherwise we'd have taken path 1).

So slightly fun question of why we have any wrapping control and I think
the answer is because the freeze on overflow isn't very specific in the
spec for whether it freezes on max value or 0.


> 
> The fixup keys off the delta rather than the operands:
> 
> 	delta = (new_cnt - prev_cnt) & GENMASK_ULL(counter_width - 1, 0);
> 	if (overflow && delta < GENMASK_ULL(counter_width - 1, 0))
> 		delta += (1UL << counter_width);
> 
> and is wrong both ways. After a mid-period read the subtraction already
> spans the wrap, but the guard holds for all but one delta value, so a
> second period is added and the event over-counts. After an earlier
> overflow read - or after event_start(), which leaves prev_count at 0 -
> prev_cnt is a small residual, and once the new residual is greater or
> equal only the difference between the two is counted and the period is
> lost, which for a residual of 0 means every period after the first.
> 'perf stat -I' hits the former, plain 'perf stat' the latter.
> 
> The shift is also undefined for counter_width == 64, and for >= 32 on
> 32-bit kernels where 1UL is 32 bits.

That bit seems valid.

> 
> Condition the fixup on new_cnt >= prev_cnt, the one case the masked
> subtraction cannot express, and use mask + 1 for the period: it is 0 for a
> 64-bit counter, the correct sum mod 2^64, and needs no shift. A
> userspace read landing between the wrap and the handler clearing the
> overflow status stays correct because the counter is frozen and cannot
> advance between the two reads.
> 
> Fixes: 5d7107c72796 ("perf: CXL Performance Monitoring Unit driver")
> Reported-by: sashiko-bot@kernel.org
> Closes: https://sashiko.dev/#/patchset/20260715191454.459673-1-dave@stgolabs.net?part=1
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v2:
> - Fix the fixup instead of deleting it. v1 dropped it on the rationale
>   that the masked subtraction already handles the wrap, which only holds
>   for new_cnt < prev_cnt; with freeze on overflow and no reload the
>   overflow path routinely sees new_cnt >= prev_cnt, where v1 silently
>   dropped a full period (sashiko-bot,
>   https://lore.kernel.org/linux-cxl/20260728211411.47B2D1F000E9@smtp.kernel.org/).
> - Keep the overflow argument and the __cxl_pmu_read()/cxl_pmu_read()
>   split; v1 folded them together once the argument went unused.
> - Use mask + 1 rather than 1ULL << counter_width so the 64-bit counter
>   case needs no special casing.
> ---
>  drivers/perf/cxl_pmu.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
> index 3138514157cd..956da8202551 100644
> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c
> @@ -690,7 +690,7 @@ static void __cxl_pmu_read(struct perf_event *event, bool overflow)
>  {
>  	struct cxl_pmu_info *info = pmu_to_cxl_pmu_info(event->pmu);
>  	struct hw_perf_event *hwc = &event->hw;
> -	u64 new_cnt, prev_cnt, delta;
> +	u64 new_cnt, prev_cnt, delta, mask;
>  
>  	do {
>  		prev_cnt = local64_read(&hwc->prev_count);
> @@ -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;
>  
>  	local64_add(delta, &event->count);
>  }