Re: [PATCH 1/7] perf/x86/intel/uncore: Rename refcount fields and other cleanups

[Ian Rogers]

On Tue, May 12, 2026 at 4:39 PM Zide Chen <zide.chen@xxxxxxxxx> 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.
>
> pmu->activeboxes is misleading because it does not decrement when the
> PMU fails to register.  Rename it to die_refcnt, reflecting that it
> counts the number of dies with a box present, regardless of whether
> the PMU is functioning.
>
> Rename box->refcnt to box->cpu_refcnt to clarify that it tracks the
> number of CPUs on the die with a corresponding PMU box present.
>
> Remove the incorrect atomic_inc(&box->refcnt) from
> uncore_pci_pmu_register(): PCI boxes are not tracked by cpu_refcnt,
> and this call incorrectly increments it on a per-die basis.
>
> Signed-off-by: Zide Chen <zide.chen@xxxxxxxxx>

Reviewed-by: Ian Rogers <irogers@xxxxxxxxxx>

Thanks,
Ian

> ---
>  arch/x86/events/intel/uncore.c | 21 +++++++++++----------
>  arch/x86/events/intel/uncore.h | 10 +++++-----
>  2 files changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
> index a7780c5cd419..012a7e081014 100644
> --- a/arch/x86/events/intel/uncore.c
> +++ b/arch/x86/events/intel/uncore.c
> @@ -1170,14 +1170,13 @@ static int uncore_pci_pmu_register(struct pci_dev *pdev,
>         if (!box)
>                 return -ENOMEM;
>
> -       atomic_inc(&box->refcnt);
>         box->dieid = die;
>         box->pci_dev = pdev;
>         box->pmu = pmu;
>         uncore_box_init(box);
>
>         pmu->boxes[die] = box;
> -       if (atomic_inc_return(&pmu->activeboxes) > 1)
> +       if (atomic_inc_return(&pmu->die_refcnt) > 1)
>                 return 0;
>
>         /* First active box registers the pmu */
> @@ -1249,7 +1248,7 @@ static void uncore_pci_pmu_unregister(struct intel_uncore_pmu *pmu, int die)
>         struct intel_uncore_box *box = pmu->boxes[die];
>
>         pmu->boxes[die] = NULL;
> -       if (atomic_dec_return(&pmu->activeboxes) == 0)
> +       if (atomic_dec_return(&pmu->die_refcnt) == 0)
>                 uncore_pmu_unregister(pmu);
>         uncore_box_exit(box);
>         kfree(box);
> @@ -1515,7 +1514,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;
> @@ -1526,8 +1525,9 @@ 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];
> -                       if (box && box->cpu >= 0 && atomic_dec_return(&box->refcnt) == 0)
> +                       box = pmu->boxes[die];
> +                       if (box && box->cpu >= 0 &&
> +                           atomic_dec_return(&box->cpu_refcnt) == 0)
>                                 uncore_box_exit(box);
>                 }
>         }
> @@ -1601,14 +1601,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;
>
> @@ -1616,8 +1616,9 @@ 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];
> -                       if (box && box->cpu >= 0 && atomic_inc_return(&box->refcnt) == 1)
> +                       box = pmu->boxes[die];
> +                       if (box && box->cpu >= 0 &&
> +                           atomic_inc_return(&box->cpu_refcnt) == 1)
>                                 uncore_box_init(box);
>                 }
>         }
> diff --git a/arch/x86/events/intel/uncore.h b/arch/x86/events/intel/uncore.h
> index c2e5ccb1d72c..7d4ef869d193 100644
> --- a/arch/x86/events/intel/uncore.h
> +++ b/arch/x86/events/intel/uncore.h
> @@ -147,7 +147,7 @@ struct intel_uncore_pmu {
>         char                            name[UNCORE_PMU_NAME_LEN];
>         int                             pmu_idx;
>         bool                            registered;
> -       atomic_t                        activeboxes;
> +       atomic_t                        die_refcnt;
>         cpumask_t                       cpu_mask;
>         struct intel_uncore_type        *type;
>         struct intel_uncore_box         **boxes;
> @@ -165,7 +165,7 @@ struct intel_uncore_box {
>         int n_events;
>         int cpu;        /* cpu to collect events */
>         unsigned long flags;
> -       atomic_t refcnt;
> +       atomic_t cpu_refcnt; /* Number of CPUs that have this box online */
>         struct perf_event *events[UNCORE_PMC_IDX_MAX];
>         struct perf_event *event_list[UNCORE_PMC_IDX_MAX];
>         struct event_constraint *event_constraint[UNCORE_PMC_IDX_MAX];
> @@ -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);
>         }
> --
> 2.54.0
>