This series first generalizes resctrl selftest non-contiguous CAT check to not assume non-AMD vendor implies Intel. Second, it improves selftests such that the use of __cpuid_count() does not lead into a build failure (happens at least on ARM).
While ARM does not currently support resctrl features, there's an ongoing work to enable resctrl support also for it on the kernel side. In any case, a common header such as kselftest.h should have a proper fallback in place for what it provides, thus it seems justified to fix this common level problem on the common level rather than e.g. disabling build for resctrl selftest for archs lacking resctrl support.
I've dropped reviewed and tested by tags from the last patch in v3 due to major changes into the makefile logic. So it would be helpful if Muhammad could retest with this version.
Acquiring ARCH in lib.mk will likely allow some cleanup into some subdirectory makefiles but that is left as future work because this series focuses in fixing cpuid/build.
v4: - New patch to reorder x86 selftest makefile to avoid clobbering CFLAGS (would cause __cpuid_count() related build fail otherwise)
v3: - Remove "empty" wording - Also cast input parameters to void - Initialize ARCH from uname -m if not set (this might allow cleaning up some other makefiles but that is left as future work)
v2: - Removed RFC from the last patch & added Fixes and tags - Fixed the error message's line splits - Noted down the reason for void casts in the stub
Ilpo Järvinen (4): selftests/resctrl: Generalize non-contiguous CAT check selftests/resctrl: Always initialize ecx to avoid build warnings selftests/x86: don't clobber CFLAGS kselftest: Provide __cpuid_count() stub on non-x86 archs
tools/testing/selftests/kselftest.h | 6 +++++ tools/testing/selftests/lib.mk | 6 +++++ tools/testing/selftests/resctrl/cat_test.c | 28 +++++++++++++--------- tools/testing/selftests/x86/Makefile | 4 +++- 4 files changed, 32 insertions(+), 12 deletions(-)
arch_supports_noncont_cat() checks if vendor is ARCH_AMD and if that is not true, ARCH_INTEL is assumed which might not be true either because get_vendor() can also return zero if neither AMD nor Intel is detected.
Generalize the vendor check using switch/case logic and return false for unknown vendors.
Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com Reviewed-by: Muhammad Usama Anjum usama.anjum@collabora.com --- tools/testing/selftests/resctrl/cat_test.c | 26 +++++++++++++--------- 1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index 742782438ca3..51a1cb6aac34 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -292,19 +292,25 @@ static bool arch_supports_noncont_cat(const struct resctrl_test *test) { unsigned int eax, ebx, ecx, edx;
- /* AMD always supports non-contiguous CBM. */ - if (get_vendor() == ARCH_AMD) + switch (get_vendor()) { + case ARCH_AMD: + /* AMD always supports non-contiguous CBM. */ return true;
- /* Intel support for non-contiguous CBM needs to be discovered. */ - if (!strcmp(test->resource, "L3")) - __cpuid_count(0x10, 1, eax, ebx, ecx, edx); - else if (!strcmp(test->resource, "L2")) - __cpuid_count(0x10, 2, eax, ebx, ecx, edx); - else - return false; + case ARCH_INTEL: + /* Intel support for non-contiguous CBM needs to be discovered. */ + if (!strcmp(test->resource, "L3")) + __cpuid_count(0x10, 1, eax, ebx, ecx, edx); + else if (!strcmp(test->resource, "L2")) + __cpuid_count(0x10, 2, eax, ebx, ecx, edx); + else + return false; + + return ((ecx >> 3) & 1);
- return ((ecx >> 3) & 1); + default: + return false; + } }
static int noncont_cat_run_test(const struct resctrl_test *test,
To avoid warnings when __cpuid_count() is an empty stub, always initialize ecx because it is used in the return statement.
Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com Reviewed-by: Muhammad Usama Anjum usama.anjum@collabora.com --- tools/testing/selftests/resctrl/cat_test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index 51a1cb6aac34..9882c5d19408 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -290,7 +290,7 @@ static int cat_run_test(const struct resctrl_test *test, const struct user_param
static bool arch_supports_noncont_cat(const struct resctrl_test *test) { - unsigned int eax, ebx, ecx, edx; + unsigned int eax, ebx, ecx = 0, edx;
switch (get_vendor()) { case ARCH_AMD:
The x86 selftests makefile clobbers CFLAGS preventing lib.mk from making the necessary adjustments into CFLAGS. This would lead to a build failure after upcoming change which wants to add -DHAVE_CPUID= into CFLAGS.
Reorder CFLAGS initialization in x86 selftest. Place the constant part of CFLAGS initialization before inclusion of lib.mk but leave adding KHDR_INCLUDES into CFLAGS into the existing position because KHDR_INCLUDES might be generated inside lib.mk.
Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com --- v4: - New patch
tools/testing/selftests/x86/Makefile | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/x86/Makefile b/tools/testing/selftests/x86/Makefile index 5c8757a25998..88a6576de92f 100644 --- a/tools/testing/selftests/x86/Makefile +++ b/tools/testing/selftests/x86/Makefile @@ -1,4 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 +CFLAGS := -O2 -g -std=gnu99 -pthread -Wall + all:
include ../lib.mk @@ -35,7 +37,7 @@ BINARIES_64 := $(TARGETS_C_64BIT_ALL:%=%_64) BINARIES_32 := $(patsubst %,$(OUTPUT)/%,$(BINARIES_32)) BINARIES_64 := $(patsubst %,$(OUTPUT)/%,$(BINARIES_64))
-CFLAGS := -O2 -g -std=gnu99 -pthread -Wall $(KHDR_INCLUDES) +CFLAGS += $(KHDR_INCLUDES)
# call32_from_64 in thunks.S uses absolute addresses. ifeq ($(CAN_BUILD_WITH_NOPIE),1)
Hi Ilpo,
On 9/3/24 7:45 AM, Ilpo Järvinen wrote:
The x86 selftests makefile clobbers CFLAGS preventing lib.mk from making the necessary adjustments into CFLAGS. This would lead to a build failure after upcoming change which wants to add -DHAVE_CPUID= into CFLAGS.
Reorder CFLAGS initialization in x86 selftest. Place the constant part of CFLAGS initialization before inclusion of lib.mk but leave adding KHDR_INCLUDES into CFLAGS into the existing position because KHDR_INCLUDES might be generated inside lib.mk.
Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com
v4:
New patch
tools/testing/selftests/x86/Makefile | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/x86/Makefile b/tools/testing/selftests/x86/Makefile index 5c8757a25998..88a6576de92f 100644 --- a/tools/testing/selftests/x86/Makefile +++ b/tools/testing/selftests/x86/Makefile @@ -1,4 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 +CFLAGS := -O2 -g -std=gnu99 -pthread -Wall
- all:
include ../lib.mk @@ -35,7 +37,7 @@ BINARIES_64 := $(TARGETS_C_64BIT_ALL:%=%_64) BINARIES_32 := $(patsubst %,$(OUTPUT)/%,$(BINARIES_32)) BINARIES_64 := $(patsubst %,$(OUTPUT)/%,$(BINARIES_64)) -CFLAGS := -O2 -g -std=gnu99 -pthread -Wall $(KHDR_INCLUDES) +CFLAGS += $(KHDR_INCLUDES) # call32_from_64 in thunks.S uses absolute addresses. ifeq ($(CAN_BUILD_WITH_NOPIE),1)
These changes are becoming less obvious to me. The first two red flags are: - Most other top level Makefiles assign KHDR_INCLUDES to CFLAGS *before* including lib.mk - What current Makefiles do matches the guidance from Documentation/dev-tools/kselftest.rst as per example (verbatim copy): CFLAGS = $(KHDR_INCLUDES) TEST_GEN_PROGS := close_range_test include ../lib.mk
Looking closer it is not clear to me why lib.mk sets KHDR_INCLUDES. As I understand the usage is intended to be: make TARGETS="target" -C tools/testing/selftests This means that it is tools/testing/selftests/Makefile that will run first and it ensures that KHDR_INCLUDES is set and supports the usage documented in Documentation/dev-tools/kselftest.rst
One usage that a change like in this patch could support is for users to be able to run "make" from within the test directory self ... but considering the current KHDR_INCLUDES custom this does not seem to be supported? (but we cannot know which tests/users rely on this behavior)
Looking further I also noticed that tools/testing/selftests/Makefile even sets ARCH (but does not export it). When considering the next patch it almost looks like what is missing is for tools/testing/selftests/Makefile to "export ARCH" ... but that potentially impacts the Makefiles that do manipulation of ARCH.
I initially started to look at this because of the resctrl impact, but I clearly am not familiar enough with the kselftest build files to understand the impacts nor provide guidance. I do hope the kselftest folks can help here.
Reinette
On Tue, 3 Sep 2024, Reinette Chatre wrote:
On 9/3/24 7:45 AM, Ilpo Järvinen wrote:
The x86 selftests makefile clobbers CFLAGS preventing lib.mk from making the necessary adjustments into CFLAGS. This would lead to a build failure after upcoming change which wants to add -DHAVE_CPUID= into CFLAGS.
Reorder CFLAGS initialization in x86 selftest. Place the constant part of CFLAGS initialization before inclusion of lib.mk but leave adding KHDR_INCLUDES into CFLAGS into the existing position because KHDR_INCLUDES might be generated inside lib.mk.
Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com
v4:
New patch
tools/testing/selftests/x86/Makefile | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/x86/Makefile b/tools/testing/selftests/x86/Makefile index 5c8757a25998..88a6576de92f 100644 --- a/tools/testing/selftests/x86/Makefile +++ b/tools/testing/selftests/x86/Makefile @@ -1,4 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 +CFLAGS := -O2 -g -std=gnu99 -pthread -Wall
- all: include ../lib.mk
@@ -35,7 +37,7 @@ BINARIES_64 := $(TARGETS_C_64BIT_ALL:%=%_64) BINARIES_32 := $(patsubst %,$(OUTPUT)/%,$(BINARIES_32)) BINARIES_64 := $(patsubst %,$(OUTPUT)/%,$(BINARIES_64)) -CFLAGS := -O2 -g -std=gnu99 -pthread -Wall $(KHDR_INCLUDES) +CFLAGS += $(KHDR_INCLUDES) # call32_from_64 in thunks.S uses absolute addresses. ifeq ($(CAN_BUILD_WITH_NOPIE),1)
These changes are becoming less obvious to me. The first two red flags are:
- Most other top level Makefiles assign KHDR_INCLUDES to CFLAGS *before* including lib.mk
Most toplevel makefiles assigned CFLAGS before including lib.mk so x86 selftest was an exception overwriting CFLAGS after including lib.mk. That looks like a real problem/bug and you don't seem to contest lib.mk having the right to alter CFLAGS. So I'm still convinced this patch is necessary independent of the cpuid/resctrl selftest issue.
I believe most of those Makefile are _buggy_ wrt. KHDR_INCLUDES but I don't care where $(KHDR_INCLUDES) goes, it's irrelevant for the problem I'm trying to solve which is the CFLAGS clobber. ...I just didn't want to add yet another problem by moving KHDR_INCLUDES before including lib.mk. I'm fine with leaving that can of worms for somebody else to sort out :-).
- What current Makefiles do matches the guidance from Documentation/dev-tools/kselftest.rst as per example (verbatim copy): CFLAGS = $(KHDR_INCLUDES) TEST_GEN_PROGS := close_range_test include ../lib.mk
I'm not objecting moving the entire CFLAGS line before including lib.mk in the x86 selftests makefile if that is what you'd want me to do?
Looking closer it is not clear to me why lib.mk sets KHDR_INCLUDES.
So are you suggesting the commit a52540522c95 ("selftests/landlock: Fix out-of-tree builds") added it for no reason? The commit message indicates it was added on purpose but I don't fully understand what "to other test collections when building in their directory" means.
As I understand the usage is intended to be: make TARGETS="target" -C tools/testing/selftests This means that it is tools/testing/selftests/Makefile that will run first and it ensures that KHDR_INCLUDES is set and supports the usage documented in Documentation/dev-tools/kselftest.rst
One usage that a change like in this patch could support is for users to be able to run "make" from within the test directory self ... but considering the current KHDR_INCLUDES custom this does not seem to be supported? (but we cannot know which tests/users rely on this behavior)
Perhaps "when building in their directory" is exactly that?
I don't doubt the makefiles are very full of problems.
Looking further I also noticed that tools/testing/selftests/Makefile even sets ARCH (but does not export it). When considering the next patch it almost looks like what is missing is for tools/testing/selftests/Makefile to "export ARCH" ... but that potentially impacts the Makefiles that do manipulation of ARCH.
The ARCH handling in various Makefiles is another mess.
I initially started to look at this because of the resctrl impact, but I clearly am not familiar enough with the kselftest build files to understand the impacts nor provide guidance. I do hope the kselftest folks can help here.
Luckily this seems to have caught Shuah attention now so hopefully we've soon some reasonable resolution to ARCH dependent building and code fragments so each selftest doesn't need to come up their own way of handling it. :-)
Building resctrl selftest fails on ARM because it uses __cpuid_count() that fails the build with error:
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); | ^~~~~~~~~~~~~
The resctrl selftest would run that code only on Intel CPUs but as is, the code cannot be build at all.
Define HAVE_CPUID in lib.mk based on ARCH (x86 or x86_64). If ARCH is not set, acquire it using uname -m.
Provide a stub for __cpuid_count() if HAVE_CPUID is not present to allow build to succeed. The stub casts its arguments to void to avoid causing "unused variable" or "set but not used" warnings.
Fixes: ae638551ab64 ("selftests/resctrl: Add non-contiguous CBMs CAT test") Reported-by: Muhammad Usama Anjum usama.anjum@collabora.com Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com Tested-by: Muhammad Usama Anjum usama.anjum@collabora.com Reviewed-by: Muhammad Usama Anjum usama.anjum@collabora.com --- v3: - Remove "empty" wording - Also cast input parameters to void - Initialize ARCH from uname -m if not set (this might allow cleaning up some other makefiles but that is left as future work) v2: - Removed RFC & added Fixes and Tested-by - Fixed the error message's line splits - Noted down the reason for void casts in the stub --- tools/testing/selftests/kselftest.h | 6 ++++++ tools/testing/selftests/lib.mk | 6 ++++++ 2 files changed, 12 insertions(+)
diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h index b8967b6e29d5..9c4bfbf107f1 100644 --- a/tools/testing/selftests/kselftest.h +++ b/tools/testing/selftests/kselftest.h @@ -70,10 +70,16 @@ * have __cpuid_count(). */ #ifndef __cpuid_count +#ifdef HAVE_CPUID #define __cpuid_count(level, count, a, b, c, d) \ __asm__ __volatile__ ("cpuid\n\t" \ : "=a" (a), "=b" (b), "=c" (c), "=d" (d) \ : "0" (level), "2" (count)) +#else +#define __cpuid_count(level, count, a, b, c, d) do { \ + (void)level; (void)count; (void)a; (void)b; (void)c; (void)d; \ +} while (0) +#endif #endif
/* define kselftest exit codes */ diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk index d6edcfcb5be8..8e3069926153 100644 --- a/tools/testing/selftests/lib.mk +++ b/tools/testing/selftests/lib.mk @@ -23,6 +23,8 @@ CLANG_TARGET_FLAGS_x86_64 := x86_64-linux-gnu
# Default to host architecture if ARCH is not explicitly given. ifeq ($(ARCH),) +ARCH := $(shell uname -m 2>/dev/null || echo not) +ARCH := $(shell echo $(ARCH) | sed -e s/i.86/x86/) CLANG_TARGET_FLAGS := $(shell $(CLANG) -print-target-triple) else CLANG_TARGET_FLAGS := $(CLANG_TARGET_FLAGS_$(ARCH)) @@ -199,6 +201,10 @@ clean: $(if $(TEST_GEN_MODS_DIR),clean_mods_dir) # Build with _GNU_SOURCE by default CFLAGS += -D_GNU_SOURCE=
+ifeq ($(ARCH),$(filter $(ARCH),x86 x86_64)) +CFLAGS += -DHAVE_CPUID= +endif + # Enables to extend CFLAGS and LDFLAGS from command line, e.g. # make USERCFLAGS=-Werror USERLDFLAGS=-static CFLAGS += $(USERCFLAGS)
On 9/3/24 08:45, Ilpo Järvinen wrote:
This series first generalizes resctrl selftest non-contiguous CAT check to not assume non-AMD vendor implies Intel. Second, it improves selftests such that the use of __cpuid_count() does not lead into a build failure (happens at least on ARM).
While ARM does not currently support resctrl features, there's an ongoing work to enable resctrl support also for it on the kernel side. In any case, a common header such as kselftest.h should have a proper fallback in place for what it provides, thus it seems justified to fix this common level problem on the common level rather than e.g. disabling build for resctrl selftest for archs lacking resctrl support.
I've dropped reviewed and tested by tags from the last patch in v3 due to major changes into the makefile logic. So it would be helpful if Muhammad could retest with this version.
Acquiring ARCH in lib.mk will likely allow some cleanup into some subdirectory makefiles but that is left as future work because this series focuses in fixing cpuid/build.
v4:
- New patch to reorder x86 selftest makefile to avoid clobbering CFLAGS (would cause __cpuid_count() related build fail otherwise)
I don't like the way this patch series is mushrooming. I am not convinced that changes to lib.mk and x86 Makefile are necessary.
I will take a look at this to see if this can be simplified.
thanks, -- Shuah
On Tue, 3 Sep 2024, Shuah Khan wrote:
On 9/3/24 08:45, Ilpo Järvinen wrote:
This series first generalizes resctrl selftest non-contiguous CAT check to not assume non-AMD vendor implies Intel. Second, it improves selftests such that the use of __cpuid_count() does not lead into a build failure (happens at least on ARM).
While ARM does not currently support resctrl features, there's an ongoing work to enable resctrl support also for it on the kernel side. In any case, a common header such as kselftest.h should have a proper fallback in place for what it provides, thus it seems justified to fix this common level problem on the common level rather than e.g. disabling build for resctrl selftest for archs lacking resctrl support.
I've dropped reviewed and tested by tags from the last patch in v3 due to major changes into the makefile logic. So it would be helpful if Muhammad could retest with this version.
Acquiring ARCH in lib.mk will likely allow some cleanup into some subdirectory makefiles but that is left as future work because this series focuses in fixing cpuid/build.
v4:
- New patch to reorder x86 selftest makefile to avoid clobbering CFLAGS (would cause __cpuid_count() related build fail otherwise)
I don't like the way this patch series is mushrooming. I am not convinced that changes to lib.mk and x86 Makefile are necessary.
I didn't like it either what I found from the various makefiles. I think there are many things done which conflict with what lib.mk seems to try to do.
I tried to ask in the first submission what test I should use in the header file as I'm not very familiar with how arch specific is done in userspace in the first place nor how it should be done within kselftest framework.
I will take a look at this to see if this can be simplified.
Sure, thanks.
On 9/4/24 06:18, Ilpo Järvinen wrote:
On Tue, 3 Sep 2024, Shuah Khan wrote:
On 9/3/24 08:45, Ilpo Järvinen wrote:
This series first generalizes resctrl selftest non-contiguous CAT check to not assume non-AMD vendor implies Intel. Second, it improves selftests such that the use of __cpuid_count() does not lead into a build failure (happens at least on ARM).
While ARM does not currently support resctrl features, there's an ongoing work to enable resctrl support also for it on the kernel side. In any case, a common header such as kselftest.h should have a proper fallback in place for what it provides, thus it seems justified to fix this common level problem on the common level rather than e.g. disabling build for resctrl selftest for archs lacking resctrl support.
I've dropped reviewed and tested by tags from the last patch in v3 due to major changes into the makefile logic. So it would be helpful if Muhammad could retest with this version.
Acquiring ARCH in lib.mk will likely allow some cleanup into some subdirectory makefiles but that is left as future work because this series focuses in fixing cpuid/build.
v4:
- New patch to reorder x86 selftest makefile to avoid clobbering CFLAGS (would cause __cpuid_count() related build fail otherwise)
I don't like the way this patch series is mushrooming. I am not convinced that changes to lib.mk and x86 Makefile are necessary.
I didn't like it either what I found from the various makefiles. I think there are many things done which conflict with what lib.mk seems to try to do.
Some of it by desig. lib.mk offers framework for common things. There are provisions to override like in the case of x86, powerpc. lib.mk tries to be flexible as well.
I tried to ask in the first submission what test I should use in the header file as I'm not very familiar with how arch specific is done in userspace in the first place nor how it should be done within kselftest framework.
Thoughts on cpuid:
- It is x86 specific. Moving this to kselftest.h was done to avoid duplicate. However now we are running into arm64/arm compile errors due to this which need addressing one way or the other.
I have some ideas on how to solve this - but I need answers to the following questions.
This is a question for you and Usama.
- Does resctrl run on arm64/arm and what's the output? - Can all other tests in resctrl other tests except noncont_cat_run_test? - If so send me the output.
thanks, -- Shuah
On Wed, 4 Sep 2024, Shuah Khan wrote:
On 9/4/24 06:18, Ilpo Järvinen wrote:
On Tue, 3 Sep 2024, Shuah Khan wrote:
On 9/3/24 08:45, Ilpo Järvinen wrote:
This series first generalizes resctrl selftest non-contiguous CAT check to not assume non-AMD vendor implies Intel. Second, it improves selftests such that the use of __cpuid_count() does not lead into a build failure (happens at least on ARM).
While ARM does not currently support resctrl features, there's an ongoing work to enable resctrl support also for it on the kernel side. In any case, a common header such as kselftest.h should have a proper fallback in place for what it provides, thus it seems justified to fix this common level problem on the common level rather than e.g. disabling build for resctrl selftest for archs lacking resctrl support.
I've dropped reviewed and tested by tags from the last patch in v3 due to major changes into the makefile logic. So it would be helpful if Muhammad could retest with this version.
Acquiring ARCH in lib.mk will likely allow some cleanup into some subdirectory makefiles but that is left as future work because this series focuses in fixing cpuid/build.
v4:
- New patch to reorder x86 selftest makefile to avoid clobbering CFLAGS (would cause __cpuid_count() related build fail otherwise)
I don't like the way this patch series is mushrooming. I am not convinced that changes to lib.mk and x86 Makefile are necessary.
I didn't like it either what I found from the various makefiles. I think there are many things done which conflict with what lib.mk seems to try to do.
Some of it by desig. lib.mk offers framework for common things. There are provisions to override like in the case of x86, powerpc. lib.mk tries to be flexible as well.
I tried to ask in the first submission what test I should use in the header file as I'm not very familiar with how arch specific is done in userspace in the first place nor how it should be done within kselftest framework.
Thoughts on cpuid:
- It is x86 specific. Moving this to kselftest.h was done to avoid duplicate. However now we are running into arm64/arm compile errors due to this which need addressing one way or the other.
I have some ideas on how to solve this - but I need answers to the following questions.
This is a question for you and Usama.
- Does resctrl run on arm64/arm and what's the output?
- Can all other tests in resctrl other tests except noncont_cat_run_test?
- If so send me the output.
Hi Shuah,
As mentioned in my coverletter above, resctrl does not currently support arm but there's an ongoing work to add arm support. On kernel side it requires major refactoring to move non-arch specific stuff out from arch/x86 so has (predictably) taken long time.
The resctrl selftests are mostly written in arch independent way (*) but there's also a way to limit a test only to CPUs from a particular vendor. And now this noncont_cat_run_test needs to use cpuid only on Intel CPUs (to read the supported flag), it's not needed even on AMD CPUs as they always support non-contiguous CAT bitmask.
So to summarize, it would be possible to disable resctrl test for non-x86 but it does not address the underlying problem with cpuid which will just come back later I think.
Alternatively, if there's some a good way in C code to do ifdeffery around that cpuid call, I could make that too, but I need to know which symbol to use for that ifdef.
(*) The cache topology may make some selftest unusable on new archs but not the selftest code itself.
On 9/4/24 06:54, Ilpo Järvinen wrote:
On Wed, 4 Sep 2024, Shuah Khan wrote:
On 9/4/24 06:18, Ilpo Järvinen wrote:
On Tue, 3 Sep 2024, Shuah Khan wrote:
On 9/3/24 08:45, Ilpo Järvinen wrote:
This series first generalizes resctrl selftest non-contiguous CAT check to not assume non-AMD vendor implies Intel. Second, it improves selftests such that the use of __cpuid_count() does not lead into a build failure (happens at least on ARM).
While ARM does not currently support resctrl features, there's an ongoing work to enable resctrl support also for it on the kernel side. In any case, a common header such as kselftest.h should have a proper fallback in place for what it provides, thus it seems justified to fix this common level problem on the common level rather than e.g. disabling build for resctrl selftest for archs lacking resctrl support.
I've dropped reviewed and tested by tags from the last patch in v3 due to major changes into the makefile logic. So it would be helpful if Muhammad could retest with this version.
Acquiring ARCH in lib.mk will likely allow some cleanup into some subdirectory makefiles but that is left as future work because this series focuses in fixing cpuid/build.
v4:
- New patch to reorder x86 selftest makefile to avoid clobbering CFLAGS (would cause __cpuid_count() related build fail otherwise)
I don't like the way this patch series is mushrooming. I am not convinced that changes to lib.mk and x86 Makefile are necessary.
I didn't like it either what I found from the various makefiles. I think there are many things done which conflict with what lib.mk seems to try to do.
Some of it by desig. lib.mk offers framework for common things. There are provisions to override like in the case of x86, powerpc. lib.mk tries to be flexible as well.
I tried to ask in the first submission what test I should use in the header file as I'm not very familiar with how arch specific is done in userspace in the first place nor how it should be done within kselftest framework.
Thoughts on cpuid:
- It is x86 specific. Moving this to kselftest.h was done to avoid duplicate. However now we are running into arm64/arm compile errors due to this which need addressing one way or the other.
I have some ideas on how to solve this - but I need answers to the following questions.
This is a question for you and Usama.
- Does resctrl run on arm64/arm and what's the output?
- Can all other tests in resctrl other tests except noncont_cat_run_test?
- If so send me the output.
Hi Shuah,
As mentioned in my coverletter above, resctrl does not currently support arm but there's an ongoing work to add arm support. On kernel side it requires major refactoring to move non-arch specific stuff out from arch/x86 so has (predictably) taken long time.
The resctrl selftests are mostly written in arch independent way (*) but there's also a way to limit a test only to CPUs from a particular vendor. And now this noncont_cat_run_test needs to use cpuid only on Intel CPUs (to read the supported flag), it's not needed even on AMD CPUs as they always support non-contiguous CAT bitmask.
So to summarize, it would be possible to disable resctrl test for non-x86 but it does not address the underlying problem with cpuid which will just come back later I think.
Alternatively, if there's some a good way in C code to do ifdeffery around that cpuid call, I could make that too, but I need to know which symbol to use for that ifdef.
(*) The cache topology may make some selftest unusable on new archs but not the selftest code itself.
I agree that suppressing resctrl build is not a solution. The real problem is is in defining __cpuid_count() in common code path.
I fixed it and send patch in. As I was testing I noticed the following on AMD platform:
- it ran the L3_NONCONT_CAT test which is expected.
# # Starting L3_NONCONT_CAT test ... # # Mounting resctrl to "/sys/fs/resctrl" # ARCH_AMD - supports non-contiguous CBM # # Write schema "L3:0=ff" to resctrl FS # # Write schema "L3:0=fc3f" to resctrl FS # ok 5 L3_NONCONT_CAT: test
- It went on to run L2_NONCONT_CAT - failed
# ok 6 # SKIP Hardware does not support L2_NONCONT_CAT or L2_NONCONT_CAT is disabled
Does it make sense to run both L3_NONCONT_CAT and L2_NONCONT_CAT on AMD? Maybe it is? resctrl checks L3 or L2 support on Intel.
Anyway - the problem is fixed now. Please review and test.
thanks, -- Shuah
Hi Shuah,
On 9/5/24 11:06 AM, Shuah Khan wrote:
On 9/4/24 06:54, Ilpo Järvinen wrote:
On Wed, 4 Sep 2024, Shuah Khan wrote:
On 9/4/24 06:18, Ilpo Järvinen wrote:
On Tue, 3 Sep 2024, Shuah Khan wrote:
On 9/3/24 08:45, Ilpo Järvinen wrote:
This series first generalizes resctrl selftest non-contiguous CAT check to not assume non-AMD vendor implies Intel. Second, it improves selftests such that the use of __cpuid_count() does not lead into a build failure (happens at least on ARM).
While ARM does not currently support resctrl features, there's an ongoing work to enable resctrl support also for it on the kernel side. In any case, a common header such as kselftest.h should have a proper fallback in place for what it provides, thus it seems justified to fix this common level problem on the common level rather than e.g. disabling build for resctrl selftest for archs lacking resctrl support.
I've dropped reviewed and tested by tags from the last patch in v3 due to major changes into the makefile logic. So it would be helpful if Muhammad could retest with this version.
Acquiring ARCH in lib.mk will likely allow some cleanup into some subdirectory makefiles but that is left as future work because this series focuses in fixing cpuid/build.
v4:
- New patch to reorder x86 selftest makefile to avoid clobbering CFLAGS
(would cause __cpuid_count() related build fail otherwise)
I don't like the way this patch series is mushrooming. I am not convinced that changes to lib.mk and x86 Makefile are necessary.
I didn't like it either what I found from the various makefiles. I think there are many things done which conflict with what lib.mk seems to try to do.
Some of it by desig. lib.mk offers framework for common things. There are provisions to override like in the case of x86, powerpc. lib.mk tries to be flexible as well.
I tried to ask in the first submission what test I should use in the header file as I'm not very familiar with how arch specific is done in userspace in the first place nor how it should be done within kselftest framework.
Thoughts on cpuid:
- It is x86 specific. Moving this to kselftest.h was done to avoid
duplicate. However now we are running into arm64/arm compile errors due to this which need addressing one way or the other.
I have some ideas on how to solve this - but I need answers to the following questions.
This is a question for you and Usama.
- Does resctrl run on arm64/arm and what's the output?
- Can all other tests in resctrl other tests except
noncont_cat_run_test?
- If so send me the output.
Hi Shuah,
As mentioned in my coverletter above, resctrl does not currently support arm but there's an ongoing work to add arm support. On kernel side it requires major refactoring to move non-arch specific stuff out from arch/x86 so has (predictably) taken long time.
The resctrl selftests are mostly written in arch independent way (*) but there's also a way to limit a test only to CPUs from a particular vendor. And now this noncont_cat_run_test needs to use cpuid only on Intel CPUs (to read the supported flag), it's not needed even on AMD CPUs as they always support non-contiguous CAT bitmask.
So to summarize, it would be possible to disable resctrl test for non-x86 but it does not address the underlying problem with cpuid which will just come back later I think.
Alternatively, if there's some a good way in C code to do ifdeffery around that cpuid call, I could make that too, but I need to know which symbol to use for that ifdef.
(*) The cache topology may make some selftest unusable on new archs but not the selftest code itself.
I agree that suppressing resctrl build is not a solution. The real problem is is in defining __cpuid_count() in common code path.
I fixed it and send patch in. As I was testing I noticed the following on AMD platform:
- it ran the L3_NONCONT_CAT test which is expected.
# # Starting L3_NONCONT_CAT test ... # # Mounting resctrl to "/sys/fs/resctrl" # ARCH_AMD - supports non-contiguous CBM # # Write schema "L3:0=ff" to resctrl FS # # Write schema "L3:0=fc3f" to resctrl FS # ok 5 L3_NONCONT_CAT: test
- It went on to run L2_NONCONT_CAT - failed
It is not intended to appear as a failure but instead just skipping of a test since the platform does not support the feature being tested.
# ok 6 # SKIP Hardware does not support L2_NONCONT_CAT or L2_NONCONT_CAT is disabled
The output looks as intended. When I run the test on an Intel system without L2 CAT the output looks the same.
Does it make sense to run both L3_NONCONT_CAT and L2_NONCONT_CAT on AMD? Maybe it is? resctrl checks L3 or L2 support on Intel.
The selftests test the features as exposed by the generic resctrl kernel subsystem instead of relying on its own inventory of what features need to be checked for which vendor. selftests will thus only test L3 or L2 if resctrl kernel subsystem indicates it is supported on underlying platform. Only afterwards may it use platform specific knowledge to help validate the feature. In this scenario resctrl indicated that L2 CAT is not supported by underlying platform and the test was skipped. It looks good to me.
Anyway - the problem is fixed now. Please review and test.
Thank you very much. Will do.
Reinette
On 9/5/24 14:43, Reinette Chatre wrote:
Hi Shuah,
On 9/5/24 11:06 AM, Shuah Khan wrote:
On 9/4/24 06:54, Ilpo Järvinen wrote:
On Wed, 4 Sep 2024, Shuah Khan wrote:
On 9/4/24 06:18, Ilpo Järvinen wrote:
On Tue, 3 Sep 2024, Shuah Khan wrote:
On 9/3/24 08:45, Ilpo Järvinen wrote: > This series first generalizes resctrl selftest non-contiguous CAT check > to not assume non-AMD vendor implies Intel. Second, it improves > selftests such that the use of __cpuid_count() does not lead into a > build failure (happens at least on ARM). > > While ARM does not currently support resctrl features, there's an > ongoing work to enable resctrl support also for it on the kernel side. > In any case, a common header such as kselftest.h should have a proper > fallback in place for what it provides, thus it seems justified to fix > this common level problem on the common level rather than e.g. > disabling build for resctrl selftest for archs lacking resctrl support. > > I've dropped reviewed and tested by tags from the last patch in v3 due > to major changes into the makefile logic. So it would be helpful if > Muhammad could retest with this version. > > Acquiring ARCH in lib.mk will likely allow some cleanup into some > subdirectory makefiles but that is left as future work because this > series focuses in fixing cpuid/build.
> > v4: > - New patch to reorder x86 selftest makefile to avoid clobbering CFLAGS > (would cause __cpuid_count() related build fail otherwise) > I don't like the way this patch series is mushrooming. I am not convinced that changes to lib.mk and x86 Makefile are necessary.
I didn't like it either what I found from the various makefiles. I think there are many things done which conflict with what lib.mk seems to try to do.
Some of it by desig. lib.mk offers framework for common things. There are provisions to override like in the case of x86, powerpc. lib.mk tries to be flexible as well.
I tried to ask in the first submission what test I should use in the header file as I'm not very familiar with how arch specific is done in userspace in the first place nor how it should be done within kselftest framework.
Thoughts on cpuid:
- It is x86 specific. Moving this to kselftest.h was done to avoid
duplicate. However now we are running into arm64/arm compile errors due to this which need addressing one way or the other.
I have some ideas on how to solve this - but I need answers to the following questions.
This is a question for you and Usama.
- Does resctrl run on arm64/arm and what's the output?
- Can all other tests in resctrl other tests except
noncont_cat_run_test?
- If so send me the output.
Hi Shuah,
As mentioned in my coverletter above, resctrl does not currently support arm but there's an ongoing work to add arm support. On kernel side it requires major refactoring to move non-arch specific stuff out from arch/x86 so has (predictably) taken long time.
The resctrl selftests are mostly written in arch independent way (*) but there's also a way to limit a test only to CPUs from a particular vendor. And now this noncont_cat_run_test needs to use cpuid only on Intel CPUs (to read the supported flag), it's not needed even on AMD CPUs as they always support non-contiguous CAT bitmask.
So to summarize, it would be possible to disable resctrl test for non-x86 but it does not address the underlying problem with cpuid which will just come back later I think.
Alternatively, if there's some a good way in C code to do ifdeffery around that cpuid call, I could make that too, but I need to know which symbol to use for that ifdef.
(*) The cache topology may make some selftest unusable on new archs but not the selftest code itself.
I agree that suppressing resctrl build is not a solution. The real problem is is in defining __cpuid_count() in common code path.
I fixed it and send patch in. As I was testing I noticed the following on AMD platform:
- it ran the L3_NONCONT_CAT test which is expected.
# # Starting L3_NONCONT_CAT test ... # # Mounting resctrl to "/sys/fs/resctrl" # ARCH_AMD - supports non-contiguous CBM # # Write schema "L3:0=ff" to resctrl FS # # Write schema "L3:0=fc3f" to resctrl FS # ok 5 L3_NONCONT_CAT: test
- It went on to run L2_NONCONT_CAT - failed
It is not intended to appear as a failure but instead just skipping of a test since the platform does not support the feature being tested.
# ok 6 # SKIP Hardware does not support L2_NONCONT_CAT or L2_NONCONT_CAT is disabled
The output looks as intended. When I run the test on an Intel system without L2 CAT the output looks the same.
Does it make sense to run both L3_NONCONT_CAT and L2_NONCONT_CAT on AMD? Maybe it is? resctrl checks L3 or L2 support on Intel.
The selftests test the features as exposed by the generic resctrl kernel subsystem instead of relying on its own inventory of what features need to be checked for which vendor. selftests will thus only test L3 or L2 if resctrl kernel subsystem indicates it is supported on underlying platform. Only afterwards may it use platform specific knowledge to help validate the feature. In this scenario resctrl indicated that L2 CAT is not supported by underlying platform and the test was skipped. It looks good to me.
Thanks for the detailed explanation. Sounds good to me.
thanks, -- Shuah
linux-kselftest-mirror@lists.linaro.org
-
Ilpo Järvinen -
Reinette Chatre -
Shuah Khan