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

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

LGTM. Thanks.

Reviewed-by: Dapeng Mi <dapeng1.mi@linux.intel.com>

On 9/17/2026 4:27 AM, Zide Chen 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@intel.com>
> ---
>  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)