Re: [PATCH v3 7/9] perf/cxl: Unfreeze counters after handling an overflow interrupt

On Fri, Jul 31, 2026 at 04:28:25PM +0800, Dave Jiang wrote:
> The counters run with Freeze on Overflow set, so one overflow freezes every
> counter in the block (CXL r4.0 8.2.7.2.1). cxl_pmu_irq() reads the
> overflowed counters and clears the overflow status, but never unfreezes.
> Everything stays frozen until the next pmu_enable(), and events in that
> window are lost.
> 
> Unfreeze after clearing the status, unless the PMU has been disabled in the
> meantime. cxl_pmu_disable() freezes the block deliberately, and
> cxl_pmu_offline_cpu() migrates the context before it moves the interrupt's
> affinity, so an overflow can still land on the old CPU while the new one
> reprograms. Track the enabled state and leave the block frozen in that
> case. pmu_enable() unfreezes anyway.
> 
> Fixes: 5d7107c72796 ("perf: CXL Performance Monitoring Unit driver")
> Reported-by: [email protected]
> Closes: https://sashiko.dev/#/patchset/[email protected]?part=1
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Dave Jiang <[email protected]>
> ---
> v3:
> - Don't unfreeze if the PMU has been disabled (Robin, Jonathan). Review tag
>   dropped as the patch grew a hunk.
> - Robin also asked whether this can release a counter that overflowed after
>   the status read. It cannot. Global Freeze on Overflow stops every
>   non-free-running counter and they stay frozen until software unfreezes
>   them, so nothing the driver owns can advance, let alone newly overflow,
>   between the readq and here. Two counters overflowing together both show
>   up in the same pass. Only free-running counters move while frozen, and
>   the driver never programs those.
> - The enabled check is not a full exclusion, since the flag is read outside
>   any lock. Say so in the comment rather than implying otherwise. A lock
>   would buy nothing: pmu_disable() normally runs on the pinned CPU with
>   interrupts off, and in the one window where it does not - the migration
>   in cxl_pmu_offline_cpu() - event_stop() has already cleared Counter
>   Enable, so a stray unfreeze resumes nothing (sashiko-bot).
> ---
>  drivers/perf/cxl_pmu.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
> index 6fdc66a01fb6..580a75bc9210 100644
> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c
> @@ -108,6 +108,8 @@ struct cxl_pmu_info {
>  	bool filter_hdm;
>  	int msi_vec;
>  	int irq;
> +	/* Set between pmu_enable() and pmu_disable(), read by the IRQ handler */
> +	bool enabled;
>  };
>  
>  #define pmu_to_cxl_pmu_info(_pmu) container_of(_pmu, struct cxl_pmu_info, pmu)
> @@ -596,6 +598,7 @@ static void cxl_pmu_enable(struct pmu *pmu)
>  	void __iomem *base = info->base;
>  
>  	/* Can assume frozen at this stage */
> +	WRITE_ONCE(info->enabled, true);
>  	writeq(0, base + CXL_PMU_FREEZE_REG);
>  }
>  
> @@ -604,6 +607,7 @@ static void cxl_pmu_disable(struct pmu *pmu)
>  	struct cxl_pmu_info *info = pmu_to_cxl_pmu_info(pmu);
>  	void __iomem *base = info->base;
>  
> +	WRITE_ONCE(info->enabled, false);
>  	/*
>  	 * Whilst bits above number of counters are RsvdZ
>  	 * they are unlikely to be repurposed given
> @@ -803,6 +807,9 @@ static irqreturn_t cxl_pmu_irq(int irq, void *data)
>  
>  	writeq(overflowed, base + CXL_PMU_OVERFLOW_REG);
>  
> +	if (READ_ONCE(info->enabled))
> +		writeq(0, base + CXL_PMU_FREEZE_REG);
> +

The unfreeze looks right to me.
Maybe we should quote the corresponding part in spec as needed?
CXL 4.0 8.2.7.1.3 "CPMU Freeze (Offset 18h)", Table 8-183 - "Counter Unit
N remains frozen until explicitly unfrozen by software" .

I guess Robin's hotplug rework is solving the issue of cross-CPU check-then-act ?

--Richard



>  	return IRQ_HANDLED;
>  }
>  
> -- 
> 2.55.0
> 
>