Re: [RESEND PATCH v4 11/11] perf/cxl: Clear stale overflow status before using a counter

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

Pre-existing issues:
- [High] Flawed overflow handling adds phantom counts to events when `cxl_pmu_read()` races with the overflow interrupt.
--

--- Patch [11]: [RESEND PATCH v4 11/11] perf/cxl: Clear stale overflow status before using a counter ---
commit 7eae9f25fd541ecbfdfed643b25d0495c2ec7444
Author: Dave Jiang <[email protected]>
perf/cxl: Clear stale overflow status before using a counter

This commit clears the overflow status register in cxl_pmu_event_start() and
cxl_pmu_probe(). This ensures that a counter handed from one event to the
next does not start with a previous owner's pending overflow, preventing
phantom counts from being added on handover.

Closes: https://lore.kernel.org/linux-cxl/anAVpO--i5heVfz9@MWDK4CY14F/

> diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c
> @@ -683,6 +683,15 @@ static void cxl_pmu_event_start(struct perf_event *event, int flags)
>  	 */
>  	cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_THRESHOLD_MSK,
>  			  cxl_pmu_config1_get_threshold(event));
> +
> +	/*
> +	 * Drop any overflow the previous owner of this counter left pending;
> +	 * cxl_pmu_event_stop() does not. Otherwise an interrupt latched over
> +	 * the handover gets charged to this event. Do it before arming Interrupt
> +	 * on Overflow below, and note RW1C leaves the other counters alone.
> +	 */
> +	writeq(BIT_ULL(hwc->idx), base + CXL_PMU_OVERFLOW_REG);

[Severity: High]
This is a pre-existing issue, but while we are addressing phantom counts from
stale overflows here, is there still a race condition between cxl_pmu_read()
and the overflow interrupt that can also result in phantom periods?

If the hardware counter wraps, it sets the overflow bit and raises an MSI.
Before the ISR can run, cxl_pmu_read() might be called (for instance, via a
system call or a periodic timer) while local IRQs are disabled:

cxl_pmu_read()
  __cxl_pmu_read(event, false)

This calculates the correct delta, adds it to the event count, and updates
prev_count to match new_cnt.

Once IRQs are re-enabled, the pending MSI fires. cxl_pmu_irq() runs and calls:

cxl_pmu_irq()
  __cxl_pmu_read(event, true)

Because prev_count was just updated, prev_count and new_cnt are now equal or
nearly equal. In __cxl_pmu_read(), we have:

	if (overflow && new_cnt >= prev_cnt)
		delta += mask + 1;

Since new_cnt is greater than or equal to prev_cnt and the overflow flag is
passed as true by the ISR, does this logic erroneously add a full mask + 1
phantom period to the performance counter?

[ ... ]

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