Re: [PATCH] selftests: resctrl: ignore builds for unsupported architectures

[Shuah Khan <skhan@linuxfoundation.org>]

On 8/13/24 01:39, Ilpo Järvinen wrote:
> On Mon, 12 Aug 2024, Reinette Chatre wrote:
>> On 8/12/24 3:49 PM, Shuah Khan wrote:
>>> On 8/9/24 02:45, Ilpo Järvinen wrote:
>>>> Adding Maciej.
>>>>
>>>> On Fri, 9 Aug 2024, Muhammad Usama Anjum wrote:
>>>>> On 8/9/24 12:23 PM, Ilpo Järvinen wrote:
>>>>>> On Fri, 9 Aug 2024, Muhammad Usama Anjum wrote:
>>>>>>
>>>>>>> This test doesn't have support for other architectures. Altough
>>>>>>> resctrl
>>>>>>> is supported on x86 and ARM, but arch_supports_noncont_cat() shows
>>>>>>> that
>>>>>>> only x86 for AMD and Intel are supported by the test.
>>>>>>
>>>>>> One does not follow from the other. arch_supports_noncont_cat() is
>>>>>> only
>>>>>> small part of the tests so saying "This test" based on a small subset
>>>>>> of
>>>>>> all tests is bogus. Also, I don't see any reason why ARCH_ARM could
>>>>>> not be
>>>>>> added and arch_supports_noncont_cat() adapted accordingly.
>>>>> I'm not familiar with resctrl and the architectural part of it. Feel
>>>>> free to fix it and ignore this patch.
>>>>>
>>>>> If more things are missing than just adjusting
>>>>> arch_supports_noncont_cat(), the test should be turned off until proper
>>>>> support is added to the test.
>>>>>
>>>>>>> We get build
>>>>>>> errors when built for ARM and ARM64.
>>>>>>
>>>>>> As this seems the real reason, please quote any errors when you use
>>>>>> them
>>>>>> as justification so it can be reviewed if the reasoning is sound or
>>>>>> not.
>>>>>
>>>>>     CC       resctrl_tests
>>>>> In file included from resctrl.h:24,
>>>>>                    from cat_test.c:11:
>>>>> In function 'arch_supports_noncont_cat',
>>>>>       inlined from 'noncont_cat_run_test' at cat_test.c:323:6:
>>>>> ../kselftest.h:74:9: error: impossible constraint in 'asm'
>>>>>      74 |         __asm__ __volatile__ ("cpuid\n\t"
>>>>>          \
>>>>>         |         ^~~~~~~
>>>>> cat_test.c:301:17: note: in expansion of macro '__cpuid_count'
>>>>>     301 |                 __cpuid_count(0x10, 1, eax, ebx, ecx, edx);
>>>>>         |                 ^~~~~~~~~~~~~
>>>>> ../kselftest.h:74:9: error: impossible constraint in 'asm'
>>>>>      74 |         __asm__ __volatile__ ("cpuid\n\t"
>>>>>          \
>>>>>         |         ^~~~~~~
>>>>> cat_test.c:303:17: note: in expansion of macro '__cpuid_count'
>>>>>     303 |                 __cpuid_count(0x10, 2, eax, ebx, ecx, edx);
>>>>>         |                 ^~~~~~~~~~~~~
>>>>
>>>> Okay, so it's specific to lack of CPUID. This seems a kselftest common
>>>> level problem to me, since __cpuid_count() is provided in kselftest.h.
>>>>
>>>> Shuah (or others), what is the intended mechanism for selftests to know if
>>>> it can be used or not since as is, it's always defined?
>>> _cpuid_count() gets defined in ksefltest.h if it can't find it.
>>>
>>> As the comment says both gcc and cland probide __cpuid_count()
>>>
>>>     gcc cpuid.h provides __cpuid_count() since v4.4.
>>>     Clang/LLVM cpuid.h provides  __cpuid_count() since v3.4.0.
>>>
>>>>
>>>> I see some Makefiles use compile testing a trivial program to decide
>>>> whether
>>>> they build some x86_64 tests or not. Is that what should be done here too,
>>>> test if __cpuid_count() compiles or not (and then build some #ifdeffery
>>>> based on the result of that compile testing)?
>>>>
>>>
>>> These build errors need to be fixed instead of restricting the build> In
>>> some cases when the test can't be supported on an architecture then it is
>>> okay
>>> to suppress build. This is not a general solution to suppress build warnings
>>
>> While there is an effort to support Arm in resctrl [1], this is not currently
>> the case and the resctrl selftests as a consequence only support x86 with
>> built-in assumptions that a test runs on either AMD or Intel. After the kernel
>> gains support
>> for Arm more changes will be needed for the resctrl tests to support another
>> architecture
>> so I do think the most appropriate change to address this build failure is to
>> restrict
>> resctrl tests to x86.
> 
> While ARM lacks resctrl support at the moment (the patch BTW claims
> otherwise), this problem is general-level problem in selftests. When
> somebody includes kselftest.h, the header provided __cpuid_count() which
> seems to not be compilable on ARMs (even if the test itself would never
> call it on other than when running on Intel). Some #ifdeffery is necessary
> either in kselftest.h or in the test code.
> 
>>> I would recommend against adding suppress build code when it can be fixed.
>>
>> I expect after resctrl fs obtains support for Arm the resctrl selftests can be
>> updated to support it with more fine grained architectural checks than a
>> global
>> enable/disable needed at this time.
> 
> That won't help to a build failure. The build would fail on ARM even if
> there's some resctrl specific test for arch done by the test itself.

I see.
   
> 
>>> Let's investigate this problem to fix it properly. I don't see any arm and
>>> arm64
>>> maintainers and developers on this thread. It would be good to investigate
>>> to
>>> see if this can be fixed.
> 
> Yes, I was hoping there would be a general level solution which would
> provide e.g. HAS_CPUID_COUNT or an empty stub for __cpuid_count() or
> something along those lines.
Can we try to make this change?

thanks,
-- Shuah