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

["Chen, Zide" <zide.chen@intel.com>]

On 9/16/2026 6:00 PM, Ian Rogers wrote:
> On Wed, Sep 16, 2026 at 2:52 PM Chen, Zide <zide.chen@intel.com> wrote:
>>
>> On 9/16/2026 3:44 PM, sashiko-bot@kernel.org wrote:
>>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>>> - [Low] perf: missing explicit <stdio.h> and <string.h> inclusions
>>> --
>>>
>>> commit 197eea3115a0d065dc53e6f96ad2fd38fdb9bc99
>>> Author: Zide Chen <zide.chen@intel.com>
>>>
>>> perf topdown: Don't require config1==0 for a slots event
>>>
>>> This commit modifies the topdown slots architecture check to no longer require
>>> config1 == 0, fixing an issue where explicit slots events were duplicated in
>>> scheduling. It also adds a new test to verify that an explicit metrics_clear
>>> slots event doesn't trigger extra injections.
>>>
>>>> diff --git a/tools/perf/arch/x86/tests/topdown.c b/tools/perf/arch/x86/tests/topdown.c
>>>> index 2b6f47ce49324..b1ce9f83db00e 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);
>>>
>>> [Severity: Low]
>>> Does this new function test__x86_topdown_metrics_clear() need explicit header
>>> inclusions for musl compatibility?
>>>
>>> This code relies on an implicit declaration of <stdio.h> for snprintf().
>>> Missing POSIX headers can cause build failures when compiling the perf tool
>>> with strict libc implementations like musl instead of glibc.
>>>
>>> Should <stdio.h> (and potentially <string.h>) be explicitly included at the top
>>> of this file to prevent musl compilation failures?
>>
>> Not needed. tools/perf/util/pmu.h, along with debug.h (which is also
>> included), explicitly pulls in <stdio.h>.
>>
>> Additionally, this file already contains other calls to snprintf(). If
>> the required header dependency were missing, those existing uses would
>> already fail to build.
> 
> In general we want to avoid transitive includes as they are brittle.
> For example, if pmu.h and/or debug.h were changed, it might break
> musl.

OK, then I can add stdio.h and string.h to topdown.c.

> Thanks,
> Ian