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

[Mi, Dapeng]

On 6/4/2026 11:41 PM, Chen, Zide wrote:
>
> On 6/3/2026 8:00 PM, Mi, Dapeng wrote:
>> On 6/3/2026 11:09 PM, Chen, Zide wrote:
>>> On 6/2/2026 8:13 PM, Mi, Dapeng wrote:
>>>> 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@xxxxxxxxx>
>>>>>>> ---
>>>>>>> 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. 
>>> No, box->refcnt is a refcnt for in-die CPUs, while pmu->activeboxes is a
>>> per-die count. They are clear and distinct. That's also why I tried to
>>> rename them to box->cpu_refcnt and pmu->die_refcnt in v1.
>>>
>>> If this line is not deleted, box->refcnt for PCI boxes is always <= 1,
>>> which is confusing and useless.
>>>
>>>  > If we delete this refcnt reference, then the box structure is under no
>>>> protection and the box structure could be freed by some accidents. 
>>> box->refcnt is not checked on any PCI boxes; Because there is one
>>> pmu->box[die] per die, it utilizes box->refcnt for MSR/MMIO boxes only:
>>>
>>> - box_ref(): first in-die CPU online — allocate and initialize the box.
>>> - box_unref(): last in-die CPU offline — tear down the box.
>>>
>>> Uncore PCI PMU enumeration and hot plug/unplug do not involve any CPU
>>> online/offline events, which is why box->refcnt is not used.
>> No, the refcnt is still useful even for pci type's uncore PMUs, especially
>> from the software's perspective. refcnt is the essential way to prevent the
>> unexpected free of the referenced structure. Although currently there would
>> be only one user for pci type's uncore PMUs and refcnt would be 1 in most
>> time, we can't ensure the code would always right in any time and no code
>> accidentally free the box. We'd better keep this refcnt mechanism to avoid
>> this situation. Thanks.
> pmu->activeboxes is the refcnt used by PCI PMUs, whose online/offline
> events operate on a per-die basis.
>
> box->refcnt is the refcnt used by MSR/MMIO PMUs, where online/offline
> events are per-CPU based.
>
> In this particular case (uncore_pci_pmu_register(), prior to the
> upcoming refactor), incrementing box->refcnt also increments
> pmu->activeboxes. This makes box->refcnt completely redundant for PCI
> PMUs and effectively a misuse of the per-cpu field for a per-die operation.
>
> If I’m missing any scenarios where PCI PMU boxes still require
> additional protection from box->refcnt, please help point them out.

IMO, activeboxes looks more like the reference count of PMU structure
instead of the box structure. Box->refcnt is actually the reference count
of  box structure. I know the refcnt could not matter too much for PCI
uncore PMUs since it has only one user right now, but we'd better keep the
identical refcnt behavior on all the MSR/MMIO/PCI uncore PMUs, this makes
the code clean and robust. 

Thanks.

>
>>>>>> 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);
>>>>>>>  	}