Re: [PATCH] perf topdown: Don't require config1==0 for a slots event

[Mi, Dapeng]

On 9/19/2026 4:13 AM, Arnaldo Carvalho de Melo wrote:
> On Wed, Sep 16, 2026 at 02:22:45PM -0700, Ian Rogers wrote:
>> On Wed, Sep 16, 2026 at 1:37 PM Zide Chen <zide.chen@xxxxxxxxx> wrote:
>>> arch_is_topdown_slots() requires config1 == 0, so it misidentified
>>> "cpu/slots,metrics_clear=1/" as not being the slots event, and an
>>> additional slots event gets inserted, which fails event scheduling
>>> since the group now needs two fixed counter 3.
>>>
>>>   $ perf stat -e "{cpu/slots,metrics_clear=1/,cpu/topdown-retiring/}" -- sleep 1
>>>   WARNING: events were regrouped to match PMUs
>>>        <not counted>      slots
>>>      <not supported>      cpu/slots,metrics_clear=1/
>>>        <not counted>      cpu/topdown-retiring/
>>>
>>> Drop the config1 == 0 check from arch_is_topdown_slots(). This is
>>> safe: config1 has no bearing on the event's identifying config value
>>> (TOPDOWN_SLOTS, 0x0400).
>>>
>>> The attr.type == PERF_TYPE_RAW check is what actually matters: only the
>>> core PMU is registered with PERF_TYPE_RAW, and on x86, 0x400 is
>>> guaranteed to be slots event on core CPU.
>>>
>>> arch_is_topdown_metrics() keeps its own config1 == 0 check, since
>>> genuine metrics events have no "metrics_clear" and config1 remains 0.
>>>
>>> Add a test verifying that an explicit "slots,metrics_clear=1" event in
>>> a group does not trigger an extra slots event injection.
>>>
>>> Fixes: 5b546de9cc17 ("perf topdown: Use attribute to see an event is a topdown metic or slots")
>>> Signed-off-by: Zide Chen <zide.chen@xxxxxxxxx>
>> Reviewed-by: Ian Rogers <irogers@xxxxxxxxxx>
>>
>> Note, there's a Sashiko warning about a potentially missing #include
>> for musl compatibility that maybe you or Arnaldo could address.
> Merging, will look add address that comment,

@Arnaldo, Zide posted the v2 patch to address the Sashiko comments.
https://lore.kernel.org/all/20260917181016.381994-1-zide.chen@xxxxxxxxx/

You may directly merge that patch. :) 

Thanks.


>
> Thanks,
>
> - Arnaldo
>  
>> Thanks,
>> Ian
>>
>>> ---
>>>  tools/perf/arch/x86/tests/topdown.c | 28 ++++++++++++++++++++++++++++
>>>  tools/perf/arch/x86/util/topdown.c  |  3 +--
>>>  2 files changed, 29 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/perf/arch/x86/tests/topdown.c b/tools/perf/arch/x86/tests/topdown.c
>>> index 2b6f47ce4932..b1ce9f83db00 100644
>>> --- a/tools/perf/arch/x86/tests/topdown.c
>>> +++ b/tools/perf/arch/x86/tests/topdown.c
>>> @@ -229,10 +229,38 @@ static int test__x86_topdown_slots_injection(struct test_suite *test __maybe_unu
>>>         return TEST_OK;
>>>  }
>>>
>>> +/*
>>> + * An explicit "slots,metrics_clear=1" event is still the slots event and
>>> + * must not cause an extra slots event to be injected into the group.
>>> + */
>>> +static int test__x86_topdown_metrics_clear(struct test_suite *test __maybe_unused,
>>> +                                          int subtest __maybe_unused)
>>> +{
>>> +       struct perf_pmu *pmu;
>>> +       char event_str[128];
>>> +       int ret;
>>> +
>>> +       if (!topdown_sys_has_perf_metrics())
>>> +               return TEST_OK;
>>> +
>>> +       pmu = perf_pmus__find_by_type(PERF_TYPE_RAW);
>>> +       if (!pmu || !perf_pmu__has_format(pmu, "metrics_clear"))
>>> +               return TEST_OK;
>>> +
>>> +       snprintf(event_str, sizeof(event_str),
>>> +                "{%s/slots,metrics_clear=1/,%s/topdown-retiring/}",
>>> +                pmu->name, pmu->name);
>>> +       ret = test_sort(event_str, 2, 1);
>>> +       TEST_ASSERT_EQUAL("explicit metrics_clear slots event isn't duplicated", ret, TEST_OK);
>>> +
>>> +       return TEST_OK;
>>> +}
>>> +
>>>  static struct test_case x86_topdown_tests[] = {
>>>         TEST_CASE("topdown events", x86_topdown),
>>>         TEST_CASE("topdown sorting", x86_topdown_sorting),
>>>         TEST_CASE("topdown slots injection", x86_topdown_slots_injection),
>>> +       TEST_CASE("topdown metrics_clear no extra slots", x86_topdown_metrics_clear),
>>>         { .name = NULL, }
>>>  };
>>>
>>> diff --git a/tools/perf/arch/x86/util/topdown.c b/tools/perf/arch/x86/util/topdown.c
>>> index bafd285119d7..64bc64e335a2 100644
>>> --- a/tools/perf/arch/x86/util/topdown.c
>>> +++ b/tools/perf/arch/x86/util/topdown.c
>>> @@ -37,8 +37,7 @@ bool topdown_sys_has_perf_metrics(void)
>>>  bool arch_is_topdown_slots(const struct evsel *evsel)
>>>  {
>>>         return evsel->core.attr.type == PERF_TYPE_RAW &&
>>> -              evsel->core.attr.config == TOPDOWN_SLOTS &&
>>> -              evsel->core.attr.config1 == 0;
>>> +              evsel->core.attr.config == TOPDOWN_SLOTS;
>>>  }
>>>
>>>  bool arch_is_topdown_metrics(const struct evsel *evsel)
>>> --
>>> 2.55.0
>>>