| Message ID | 20240809071059.265914-1-usama.anjum@collabora.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests: resctrl: ignore builds for unsupported architectures | expand |
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. > 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.
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); | ^~~~~~~~~~~~~
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? 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)?
On 8/9/24 1:45 AM, 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? > > 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)? > It is not obvious to me that resctrl needs those "trivial program" compile tests. For testing the target architecture ARCH seems appropriate. I do not think it is guaranteed that ARCH will always be set though so the Makefile may need an additional snippet to set ARCH to "uname -m" if it is not provided by environment, similar to what is done in other Makefiles. Reinette
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 I would recommend against adding suppress build code when it can be fixed. 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. thanks, -- Shuah
Hi Shuah, 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. > > 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. > > 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. Reinette [1] https://lore.kernel.org/lkml/20240802172853.22529-1-james.morse@arm.com/
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. > > 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.
On 8/12/24 18:05, Reinette Chatre wrote: > Hi Shuah, > > 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. > Sounds good to me. This would be good case for suppressing test build. >> >> 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. > >> >> 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. thanks, -- Shuah
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
diff --git a/tools/testing/selftests/resctrl/Makefile b/tools/testing/selftests/resctrl/Makefile index f408bd6bfc3d4..d5cf96315ef9b 100644 --- a/tools/testing/selftests/resctrl/Makefile +++ b/tools/testing/selftests/resctrl/Makefile @@ -3,7 +3,9 @@ CFLAGS = -g -Wall -O2 -D_FORTIFY_SOURCE=2 CFLAGS += $(KHDR_INCLUDES) +ifeq ($(ARCH),$(filter $(ARCH),x86 x86_64)) TEST_GEN_PROGS := resctrl_tests +endif LOCAL_HDRS += $(wildcard *.h)
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. We get build errors when built for ARM and ARM64. Hence add support in the Makefile to build this suite only for x86 and x86_64 architectures. Fixes: b733143cc455 ("selftests/resctrl: Make resctrl_tests run using kselftest framework") Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com> --- tools/testing/selftests/resctrl/Makefile | 2 ++ 1 file changed, 2 insertions(+)