Re: [PATCH V2 2/8] perf/x86/intel/uncore: Fix refcnt and other cleanups

["Mi, Dapeng" <dapeng1.mi@linux.intel.com>]

On 6/2/2026 10:16 PM, Chen, Zide wrote:
>
> On 6/2/2026 4:52 AM, Mi, Dapeng wrote:
>> On 6/2/2026 1:01 AM, Zide Chen wrote:
>>> Fix typo UNCORE_BOX_FLAG_INITIATED to UNCORE_BOX_FLAG_INITIALIZED.
>>>
>>> Rename the 'id' parameter in uncore_box_{ref,unref}() to 'die' to
>>> reflect its actual meaning and be consistent with other functions.
>>>
>>> Remove the incorrect atomic_inc(&box->refcnt) from
>>> uncore_pci_pmu_register(): PCI boxes are not tracked by refcnt,
>>> and this call incorrectly increments it on a per-die basis.
>>>
>>> Signed-off-by: Zide Chen <zide.chen@intel.com>
>>> ---
>>> v2:
>>> - Don't rename pmu->activeboxes and keep its semantics because in
>>>   uncore_pci_remove() path, uncore_pci_pmu_unregister() won't be
>>>   called for non-active boxes.
>>> - Since pmu->activeboxes keeps it's name, don't need to rename
>>>   box->refcnt to box->cpu_refcnt.
>>> ---
>>>  arch/x86/events/intel/uncore.c | 11 +++++------
>>>  arch/x86/events/intel/uncore.h |  6 +++---
>>>  2 files changed, 8 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
>>> index b69b6a21d46b..d759888476c3 100644
>>> --- a/arch/x86/events/intel/uncore.c
>>> +++ b/arch/x86/events/intel/uncore.c
>>> @@ -1170,7 +1170,6 @@ static int uncore_pci_pmu_register(struct pci_dev *pdev,
>>>  	if (!box)
>>>  		return -ENOMEM;
>>>  
>>> -	atomic_inc(&box->refcnt);
>> I'm not sure if we should remove this. The
>> uncore_box_ref()/uncore_box_unref() are only called for MSR or MMIO type
>> uncore PMUs. For the uncore PMUs of PCI type, the box->refcnt is only
>> increased here. All the 3 kinds of uncore PMUs should keep consistent
>> behavior on the refcnt. 
> box->refcnt tracks how many CPUs are active within this die. Here it is
> incorrectly incremented on a per-die basis. Additionally, during the PCI
> uncore enumeration or teardown process, there is no per-CPU context, so
> box->refcnt is useless in PCI uncore.
>
> PCI uncore only needs pmu->activeboxes.

Per my understanding, the main aim of refcnt is to prevent the box
structure is freed unexpectedly, it doesn't directly couple with CPUs. It
just records how many users are using the the box regardless the users are
CPU or something else. For the uncore PMUs of MSR and MMIO type, CPUs can
be seen as the users, while for the uncore PMUs of PCI, I suppose the dies
(actually any cpu on the die) can be seen the users. 

If we delete this refcnt reference, then the box structure is under no
protection and the box structure could be freed by some accidents. 


>
>> Could we keep this and just decrease the refcnt in
>> uncore_pci_pmu_unregister()? Thanks.
>>
>>
>>>  	box->dieid = die;
>>>  	box->pci_dev = pdev;
>>>  	box->pmu = pmu;
>>> @@ -1518,7 +1517,7 @@ static void uncore_change_context(struct intel_uncore_type **uncores,
>>>  		uncore_change_type_ctx(*uncores, old_cpu, new_cpu);
>>>  }
>>>  
>>> -static void uncore_box_unref(struct intel_uncore_type **types, int id)
>>> +static void uncore_box_unref(struct intel_uncore_type **types, int die)
>>>  {
>>>  	struct intel_uncore_type *type;
>>>  	struct intel_uncore_pmu *pmu;
>>> @@ -1529,7 +1528,7 @@ static void uncore_box_unref(struct intel_uncore_type **types, int id)
>>>  		type = *types;
>>>  		pmu = type->pmus;
>>>  		for (i = 0; i < type->num_boxes; i++, pmu++) {
>>> -			box = pmu->boxes[id];
>>> +			box = pmu->boxes[die];
>>>  			if (box && box->cpu >= 0 && atomic_dec_return(&box->refcnt) == 0)
>>>  				uncore_box_exit(box);
>>>  		}
>>> @@ -1604,14 +1603,14 @@ static int allocate_boxes(struct intel_uncore_type **types,
>>>  }
>>>  
>>>  static int uncore_box_ref(struct intel_uncore_type **types,
>>> -			  int id, unsigned int cpu)
>>> +			  int die, unsigned int cpu)
>>>  {
>>>  	struct intel_uncore_type *type;
>>>  	struct intel_uncore_pmu *pmu;
>>>  	struct intel_uncore_box *box;
>>>  	int i, ret;
>>>  
>>> -	ret = allocate_boxes(types, id, cpu);
>>> +	ret = allocate_boxes(types, die, cpu);
>>>  	if (ret)
>>>  		return ret;
>>>  
>>> @@ -1619,7 +1618,7 @@ static int uncore_box_ref(struct intel_uncore_type **types,
>>>  		type = *types;
>>>  		pmu = type->pmus;
>>>  		for (i = 0; i < type->num_boxes; i++, pmu++) {
>>> -			box = pmu->boxes[id];
>>> +			box = pmu->boxes[die];
>>>  			if (box && box->cpu >= 0 && atomic_inc_return(&box->refcnt) == 1)
>>>  				uncore_box_init(box);
>>>  		}
>>> diff --git a/arch/x86/events/intel/uncore.h b/arch/x86/events/intel/uncore.h
>>> index c2e5ccb1d72c..bad5d8dec8e0 100644
>>> --- a/arch/x86/events/intel/uncore.h
>>> +++ b/arch/x86/events/intel/uncore.h
>>> @@ -185,7 +185,7 @@ struct intel_uncore_box {
>>>  #define CFL_UNC_CBO_7_PERFEVTSEL0		0xf70
>>>  #define CFL_UNC_CBO_7_PER_CTR0			0xf76
>>>  
>>> -#define UNCORE_BOX_FLAG_INITIATED		0
>>> +#define UNCORE_BOX_FLAG_INITIALIZED		0
>>>  /* event config registers are 8-byte apart */
>>>  #define UNCORE_BOX_FLAG_CTL_OFFS8		1
>>>  /* CFL 8th CBOX has different MSR space */
>>> @@ -559,7 +559,7 @@ static inline u64 uncore_read_counter(struct intel_uncore_box *box,
>>>  
>>>  static inline void uncore_box_init(struct intel_uncore_box *box)
>>>  {
>>> -	if (!test_and_set_bit(UNCORE_BOX_FLAG_INITIATED, &box->flags)) {
>>> +	if (!test_and_set_bit(UNCORE_BOX_FLAG_INITIALIZED, &box->flags)) {
>>>  		if (box->pmu->type->ops->init_box)
>>>  			box->pmu->type->ops->init_box(box);
>>>  	}
>>> @@ -567,7 +567,7 @@ static inline void uncore_box_init(struct intel_uncore_box *box)
>>>  
>>>  static inline void uncore_box_exit(struct intel_uncore_box *box)
>>>  {
>>> -	if (test_and_clear_bit(UNCORE_BOX_FLAG_INITIATED, &box->flags)) {
>>> +	if (test_and_clear_bit(UNCORE_BOX_FLAG_INITIALIZED, &box->flags)) {
>>>  		if (box->pmu->type->ops->exit_box)
>>>  			box->pmu->type->ops->exit_box(box);
>>>  	}
>