Re: [PATCH 3/6] perf/x86/intel/uncore: Defer ADL global PMON enable to enable_box()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The deferred global PMON enablement permanently leaks the hardware power state after the first use, nullifying the intended power savings.
--

commit ce0046bfd8b6ed5388332db6e76ea45581716939
Author: Zide Chen <zide.chen@intel.com>

perf/x86/intel/uncore: Defer ADL global PMON enable to enable_box()

This commit drops adl_uncore_msr_init_box() and defers programming the
ADL_UNC_PERF_GLOBAL_CTL MSR to enable_box(). The goal is to save power by
ensuring the global control register is only enabled when a box is actually
used.

> diff --git a/arch/x86/events/intel/uncore_snb.c b/arch/x86/events/intel/uncore_snb.c
> index 3dbc6bacbd9de..edddd4f9ab5fc 100644
> --- a/arch/x86/events/intel/uncore_snb.c
> +++ b/arch/x86/events/intel/uncore_snb.c
[ ... ]
> -static void adl_uncore_msr_init_box(struct intel_uncore_box *box)
> -{
> -	if (box->pmu->pmu_idx == 0)
> -		wrmsrq(ADL_UNC_PERF_GLOBAL_CTL, SNB_UNC_GLOBAL_CTL_EN);
> -}
> -
>  static void adl_uncore_msr_enable_box(struct intel_uncore_box *box)
>  {
>  	wrmsrq(ADL_UNC_PERF_GLOBAL_CTL, SNB_UNC_GLOBAL_CTL_EN);

Does this unintentionally leave the hardware power state permanently enabled
after the first use?

The Linux perf core framework unconditionally calls perf_pmu_enable(), which
maps to uncore_pmu_enable() and executes enable_box(), at the end of PMU
transactions. This includes event removal sequences:

perf_pmu_disable()
   event->pmu->del()
   perf_pmu_enable()

Since adl_uncore_msr_enable_box() unconditionally writes
SNB_UNC_GLOBAL_CTL_EN, it seems the global PMON control is immediately
re-enabled and remains active even when zero events are in use, negating
the intended power savings.

Additionally, if a check like if (box->n_active) were added here, could
that silently break concurrent perf sessions on other uncore boxes?

Because disable_box() unconditionally clears the shared MSR during
transactions if pmu_idx == 0, failing to re-enable it would leave PMON
globally disabled for the entire die. Safely managing dynamic power for this
shared global MSR might require a package-level active reference count
across all boxes.

> @@ -587,7 +581,6 @@ static void adl_uncore_msr_exit_box(struct intel_uncore_box *box)
>  }
>  
>  static struct intel_uncore_ops adl_uncore_msr_ops = {
> -	.init_box	= adl_uncore_msr_init_box,
>  	.enable_box	= adl_uncore_msr_enable_box,
>  	.disable_box	= adl_uncore_msr_disable_box,
>  	.exit_box	= adl_uncore_msr_exit_box,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511230527.26096-1-zide.chen@intel.com?part=3