Re: [PATCH] perf/x86/amd/uncore: Add group validation

[Ian Rogers]

On Tue, Jun 23, 2026 at 3:49 AM Sandipan Das <sandipan.das@xxxxxxx> wrote:
>
> The amd_uncore driver currently does not validate event groups and
> allows creation of groups with more events than the number of available
> hardware counters. Because of this, pmu->event_init() succeeds but
> counter assignment fails later in pmu->add() which returns -EBUSY once
> all counters are exhausted.
>
> Address this by introducing group validation in the pmu->event_init()
> path. Since the uncore PMUs have no per-event constraints and all
> counters of a PMU are interchangeable, validation is reduced to just
> counting the group members that target a PMU and ensuring that they fit
> within the available set of counters.
>
> Signed-off-by: Sandipan Das <sandipan.das@xxxxxxx>

This is great Sandipan! Thanks for addressing this! I'd been wondering
if in the perf tool if we could test hardware PMUs for not supporting
failing at open properly. This is a problem for weak groups, as used
by metrics, because they try to group all events and then break the
group when the open fails. I'd observed that AMD uncore events
supposedly opened but then failed during reading. I suspect other PMUs
also suffer this.

Peter mentioned a behavior in the past: opening events in a group in a
disabled state, with more events than counters, and then the software
enables and disables events in the group to control counter
allocation. The perf tool doesn't currently utilize this behavior but
I think it explains some of the Sashiko feedback.

Would it be possible to get a Fixes tag for stable backports?

Thanks,
Ian

> ---
>  arch/x86/events/amd/uncore.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
> index dbc00b6dd69e..9715cfb8cce3 100644
> --- a/arch/x86/events/amd/uncore.c
> +++ b/arch/x86/events/amd/uncore.c
> @@ -265,6 +265,29 @@ static void amd_uncore_del(struct perf_event *event, int flags)
>         hwc->idx = -1;
>  }
>
> +static bool amd_uncore_group_valid(struct perf_event *event)
> +{
> +       struct amd_uncore_pmu *pmu = event_to_amd_uncore_pmu(event);
> +       struct perf_event *leader = event->group_leader;
> +       struct perf_event *sibling;
> +       int counters = 0;
> +
> +       /* Ignore software events as they do not occupy hardware counters */
> +       if (leader->pmu == event->pmu && !is_software_event(leader))
> +               counters++;
> +
> +       for_each_sibling_event(sibling, leader) {
> +               if (sibling->pmu == event->pmu && !is_software_event(sibling))
> +                       counters++;
> +       }
> +
> +       /*
> +        * When pmu->event_init() is called, the event is yet to be linked to
> +        * its leader's sibling list, so it is counted separately
> +        */
> +       return (counters + 1) <= pmu->num_counters;
> +}
> +
>  static int amd_uncore_event_init(struct perf_event *event)
>  {
>         struct amd_uncore_pmu *pmu;
> @@ -282,6 +305,15 @@ static int amd_uncore_event_init(struct perf_event *event)
>         if (!ctx)
>                 return -ENODEV;
>
> +       /*
> +        * Ensure that all events in a group can be scheduled together so that
> +        * a failure can be reported at perf_event_open() time rather than
> +        * silently at pmu->add() time when no free counter is found
> +        */
> +       if (event->group_leader != event &&
> +           !amd_uncore_group_valid(event))
> +               return -EINVAL;
> +
>         /*
>          * NB and Last level cache counters (MSRs) are shared across all cores
>          * that share the same NB / Last level cache.  On family 16h and below,
> --
> 2.53.0
>