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

["Chen, Zide" <zide.chen@intel.com>]

On 5/12/2026 7:33 PM, sashiko-bot@kernel.org wrote:
> 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?

The additional power consumption occurs as soon as the MSR is written to
any value — not because the global control remains enabled.  So checking
n_active or disabling the global control when no events are active
wouldn't help.

> 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.

Again, for the power consumption concern, the only thing we need to do
is to postpone the write to the global control MSR (0x2ff0), and after
the first write, it's irrelevant.

Regarding the concern of the disable_box(), I'd like to address it
separately from this patch. The fix here is simply to defer the first
write until it's actually needed.


>> @@ -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,
>