Non-contiguous CBM support for Intel CAT has been merged into the kernel with Commit 0e3cd31f6e90 ("x86/resctrl: Enable non-contiguous CBMs in Intel CAT") but there is no selftest that would validate if this feature works correctly. The selftest needs to verify if writing non-contiguous CBMs to the schemata file behaves as expected in comparison to the information about non-contiguous CBMs support.
The patch series is based on a rework of resctrl selftests that's currently in review [1]. The patch also implements a similar functionality presented in the bash script included in the cover letter of the original non-contiguous CBMs in Intel CAT series [3].
Changelog v3: - Rebase onto v4 of Ilpo's series [1]. - Split old patch 3/4 into two parts. One doing refactoring and one adding a new function. - Some changes to all the patches after Reinette's review.
Changelog v2: - Rebase onto v4 of Ilpo's series [2]. - Add two patches that prepare helpers for the new test. - Move Ilpo's patch that adds test grouping to this series. - Apply Ilpo's suggestion to the patch that adds a new test.
[1] https://lore.kernel.org/all/20231215150515.36983-1-ilpo.jarvinen@linux.intel... [2] https://lore.kernel.org/all/20231211121826.14392-1-ilpo.jarvinen@linux.intel... [3] https://lore.kernel.org/all/cover.1696934091.git.maciej.wieczor-retman@intel...
Older versions of this series: [v1] https://lore.kernel.org/all/20231109112847.432687-1-maciej.wieczor-retman@in... [v2] https://lore.kernel.org/all/cover.1702392177.git.maciej.wieczor-retman@intel...
Ilpo Järvinen (1): selftests/resctrl: Add test groups and name L3 CAT test L3_CAT
Maciej Wieczor-Retman (4): selftests/resctrl: Add helpers for the non-contiguous test selftests/resctrl: Split validate_resctrl_feature_request() selftests/resctrl: Add resource_info_file_exists() selftests/resctrl: Add non-contiguous CBMs CAT test
tools/testing/selftests/resctrl/cat_test.c | 84 +++++++++++++++- tools/testing/selftests/resctrl/cmt_test.c | 4 +- tools/testing/selftests/resctrl/mba_test.c | 4 +- tools/testing/selftests/resctrl/mbm_test.c | 6 +- tools/testing/selftests/resctrl/resctrl.h | 11 ++- .../testing/selftests/resctrl/resctrl_tests.c | 18 +++- tools/testing/selftests/resctrl/resctrlfs.c | 98 ++++++++++++++++--- 7 files changed, 199 insertions(+), 26 deletions(-)
From: Ilpo Järvinen ilpo.jarvinen@linux.intel.com
To select test to run -t parameter can be used. However, -t cat currently maps to L3 CAT test which will be confusing after more CAT related tests will be added.
Allow selecting tests as groups and call L3 CAT test "L3_CAT", "CAT" group will enable all CAT related tests.
Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com Signed-off-by: Maciej Wieczor-Retman maciej.wieczor-retman@intel.com --- Changelog v3: - Expand group description in struct comment (Reinette).
Changelog v2: - Move this patch from Ilpo's series here (Ilpo).
tools/testing/selftests/resctrl/cat_test.c | 3 ++- tools/testing/selftests/resctrl/resctrl.h | 3 +++ tools/testing/selftests/resctrl/resctrl_tests.c | 16 +++++++++++----- 3 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index 24af8310288a..39fc9303b8e8 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -295,7 +295,8 @@ static int cat_run_test(const struct resctrl_test *test, const struct user_param }
struct resctrl_test l3_cat_test = { - .name = "CAT", + .name = "L3_CAT", + .group = "CAT", .resource = "L3", .feature_check = test_resource_feature_check, .run_test = cat_run_test, diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index c52eaf46f24d..a1462029998e 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -65,6 +65,8 @@ struct user_params { /* * resctrl_test: resctrl test definition * @name: Test name + * @group: Test group - a common name for tests that share some characteristic + * (e.g., L3 CAT test belongs to the CAT group). Can be NULL * @resource: Resource to test (e.g., MB, L3, L2, etc.) * @vendor_specific: Bitmask for vendor-specific tests (can be 0 for universal tests) * @disabled: Test is disabled @@ -73,6 +75,7 @@ struct user_params { */ struct resctrl_test { const char *name; + const char *group; const char *resource; unsigned int vendor_specific; bool disabled; diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index 75fc49ba3efb..3044179ee6e9 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -65,11 +65,15 @@ static void cmd_help(void) printf("usage: resctrl_tests [-h] [-t test list] [-n no_of_bits] [-b benchmark_cmd [option]...]\n"); printf("\t-b benchmark_cmd [option]...: run specified benchmark for MBM, MBA and CMT\n"); printf("\t default benchmark is builtin fill_buf\n"); - printf("\t-t test list: run tests specified in the test list, "); + printf("\t-t test list: run tests/groups specified by the list, "); printf("e.g. -t mbm,mba,cmt,cat\n"); - printf("\t\tSupported tests:\n"); - for (i = 0; i < ARRAY_SIZE(resctrl_tests); i++) - printf("\t\t\t%s\n", resctrl_tests[i]->name); + printf("\t\tSupported tests (group):\n"); + for (i = 0; i < ARRAY_SIZE(resctrl_tests); i++) { + if (resctrl_tests[i]->group) + printf("\t\t\t%s (%s)\n", resctrl_tests[i]->name, resctrl_tests[i]->group); + else + printf("\t\t\t%s\n", resctrl_tests[i]->name); + } printf("\t-n no_of_bits: run cache tests using specified no of bits in cache bit mask\n"); printf("\t-p cpu_no: specify CPU number to run the test. 1 is default\n"); printf("\t-h: help\n"); @@ -199,7 +203,9 @@ int main(int argc, char **argv) bool found = false;
for (i = 0; i < ARRAY_SIZE(resctrl_tests); i++) { - if (!strcasecmp(token, resctrl_tests[i]->name)) { + if (!strcasecmp(token, resctrl_tests[i]->name) || + (resctrl_tests[i]->group && + !strcasecmp(token, resctrl_tests[i]->group))) { if (resctrl_tests[i]->disabled) tests++; resctrl_tests[i]->disabled = false;
The CAT non-contiguous selftests have to read the file responsible for reporting support of non-contiguous CBMs in kernel (resctrl). Then the test compares if that information matches what is reported by CPUID output.
Add a generic helper function to read an unsigned number from a file in /sys/fs/resctrl/info/<RESOURCE>/<FILE>.
Signed-off-by: Maciej Wieczor-Retman maciej.wieczor-retman@intel.com --- Changelog v3: - Rewrite patch message. - Add documentation and rewrote the function. (Reinette)
Changelog v2: - Add this patch.
tools/testing/selftests/resctrl/resctrl.h | 1 + tools/testing/selftests/resctrl/resctrlfs.c | 39 +++++++++++++++++++++ 2 files changed, 40 insertions(+)
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index a1462029998e..5116ea082d03 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -162,6 +162,7 @@ unsigned int count_contiguous_bits(unsigned long val, unsigned int *start); int get_full_cbm(const char *cache_type, unsigned long *mask); int get_mask_no_shareable(const char *cache_type, unsigned long *mask); int get_cache_size(int cpu_no, const char *cache_type, unsigned long *cache_size); +int resource_info_unsigned_get(const char *resource, const char *filename, unsigned int *val); void ctrlc_handler(int signum, siginfo_t *info, void *ptr); int signal_handler_register(void); void signal_handler_unregister(void); diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index 5750662cce57..cb5147c5f9a9 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -249,6 +249,45 @@ static int get_bit_mask(const char *filename, unsigned long *mask) return 0; }
+/* + * resource_info_unsigned_get - Read an unsigned value from a file in + * /sys/fs/resctrl/info/RESOURCE/FILENAME + * @resource: Resource name that matches directory names in + * /sys/fs/resctrl/info + * @filename: Filename of a file located in a directory specified with the + * 'resource' variable. + * @val: Variable where the read value is saved on success. + * + * Return: = 0 on success, < 0 on failure. On success the read value is saved into the 'val' + * variable. + */ +int resource_info_unsigned_get(const char *resource, const char *filename, + unsigned int *val) +{ + char reason[128], file_path[PATH_MAX]; + FILE *fp; + + snprintf(file_path, sizeof(file_path), "%s/%s/%s", INFO_PATH, resource, + filename); + + fp = fopen(file_path, "r"); + if (!fp) { + snprintf(reason, sizeof(reason), "Error in opening %s file\n", filename); + ksft_perror(reason); + return -1; + } + + if (fscanf(fp, "%u", val) <= 0) { + snprintf(reason, sizeof(reason), "Could not get %s's contents\n", filename); + ksft_perror(reason); + fclose(fp); + return -1; + } + + fclose(fp); + return 0; +} + /* * create_bit_mask- Create bit mask from start, len pair * @start: LSB of the mask
On Thu, 25 Jan 2024, Maciej Wieczor-Retman wrote:
The CAT non-contiguous selftests have to read the file responsible for reporting support of non-contiguous CBMs in kernel (resctrl). Then the test compares if that information matches what is reported by CPUID output.
Add a generic helper function to read an unsigned number from a file in /sys/fs/resctrl/info/<RESOURCE>/<FILE>.
Signed-off-by: Maciej Wieczor-Retman maciej.wieczor-retman@intel.com
Changelog v3:
- Rewrite patch message.
- Add documentation and rewrote the function. (Reinette)
Changelog v2:
- Add this patch.
tools/testing/selftests/resctrl/resctrl.h | 1 + tools/testing/selftests/resctrl/resctrlfs.c | 39 +++++++++++++++++++++ 2 files changed, 40 insertions(+)
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index a1462029998e..5116ea082d03 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -162,6 +162,7 @@ unsigned int count_contiguous_bits(unsigned long val, unsigned int *start); int get_full_cbm(const char *cache_type, unsigned long *mask); int get_mask_no_shareable(const char *cache_type, unsigned long *mask); int get_cache_size(int cpu_no, const char *cache_type, unsigned long *cache_size); +int resource_info_unsigned_get(const char *resource, const char *filename, unsigned int *val); void ctrlc_handler(int signum, siginfo_t *info, void *ptr); int signal_handler_register(void); void signal_handler_unregister(void); diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index 5750662cce57..cb5147c5f9a9 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -249,6 +249,45 @@ static int get_bit_mask(const char *filename, unsigned long *mask) return 0; } +/*
- resource_info_unsigned_get - Read an unsigned value from a file in
- /sys/fs/resctrl/info/RESOURCE/FILENAME
- @resource: Resource name that matches directory names in
/sys/fs/resctrl/info
- @filename: Filename of a file located in a directory specified with the
'resource' variable.
- @val: Variable where the read value is saved on success.
- Return: = 0 on success, < 0 on failure. On success the read value is saved into the 'val'
- variable.
- */
+int resource_info_unsigned_get(const char *resource, const char *filename,
unsigned int *val)+{
- char reason[128], file_path[PATH_MAX];
- FILE *fp;
- snprintf(file_path, sizeof(file_path), "%s/%s/%s", INFO_PATH, resource,
filename);- fp = fopen(file_path, "r");
- if (!fp) {
snprintf(reason, sizeof(reason), "Error in opening %s file\n", filename);ksft_perror(reason);
Was this the conclusion of the kstf_perror() discussion with Reinette? I expected a bit different outcome when I stopped following it...
In any case, it would be nice though if ksft_perror() (or some kselftest.h function yet to be added with a different name) would accept full printf interface and just add the errno string into the end of the string so one would not need to build constructs like this at all.
It will require a bit of macro trickery into kselftest.h. I don't know how it should handle the case where somebody just passes a char pointer to it, not a string literal, but I guess it would just throw an error while compiling if somebody tries to do that as the macro string literal concatenation could not build useful/compilable token.
It would make these prints informative enough to become actually useful without needed to resort to preparing the string in advance which seems to be required almost every single case with the current interface.
return -1;- }
- if (fscanf(fp, "%u", val) <= 0) {
snprintf(reason, sizeof(reason), "Could not get %s's contents\n", filename);ksft_perror(reason);fclose(fp);return -1;- }
- fclose(fp);
- return 0;
+}
/*
- create_bit_mask- Create bit mask from start, len pair
- @start: LSB of the mask
On 1/25/2024 4:14 AM, Ilpo Järvinen wrote:
On Thu, 25 Jan 2024, Maciej Wieczor-Retman wrote:
- fp = fopen(file_path, "r");
- if (!fp) {
snprintf(reason, sizeof(reason), "Error in opening %s file\n", filename);ksft_perror(reason);Was this the conclusion of the kstf_perror() discussion with Reinette? I expected a bit different outcome when I stopped following it...
In any case, it would be nice though if ksft_perror() (or some kselftest.h function yet to be added with a different name) would accept full printf interface and just add the errno string into the end of the string so one would not need to build constructs like this at all.
It will require a bit of macro trickery into kselftest.h. I don't know how it should handle the case where somebody just passes a char pointer to it, not a string literal, but I guess it would just throw an error while compiling if somebody tries to do that as the macro string literal concatenation could not build useful/compilable token.
It would make these prints informative enough to become actually useful without needed to resort to preparing the string in advance which seems to be required almost every single case with the current interface.
I think this can be accomplished with a new: void ksft_vprint_msg(const char *msg, va_list args)
... but ksft_perror() does conform to perror() and I expect that having one support variable number of arguments while the other does to cause confusion.
To support variable number of arguments with errno I'd propose just to use ksft_print_msg() with strerror(errno), errno as the arguments (or even %m that that errno handling within ksft_print_msg() aims to support). This does indeed seem to be the custom in other tests.
Reinette
Hi,
On 2024-01-26 at 10:58:04 -0800, Reinette Chatre wrote:
On 1/25/2024 4:14 AM, Ilpo Järvinen wrote:
On Thu, 25 Jan 2024, Maciej Wieczor-Retman wrote:
- fp = fopen(file_path, "r");
- if (!fp) {
snprintf(reason, sizeof(reason), "Error in opening %s file\n", filename);ksft_perror(reason);Was this the conclusion of the kstf_perror() discussion with Reinette? I expected a bit different outcome when I stopped following it...
In any case, it would be nice though if ksft_perror() (or some kselftest.h function yet to be added with a different name) would accept full printf interface and just add the errno string into the end of the string so one would not need to build constructs like this at all.
It will require a bit of macro trickery into kselftest.h. I don't know how it should handle the case where somebody just passes a char pointer to it, not a string literal, but I guess it would just throw an error while compiling if somebody tries to do that as the macro string literal concatenation could not build useful/compilable token.
It would make these prints informative enough to become actually useful without needed to resort to preparing the string in advance which seems to be required almost every single case with the current interface.
I think this can be accomplished with a new: void ksft_vprint_msg(const char *msg, va_list args)
... but ksft_perror() does conform to perror() and I expect that having one support variable number of arguments while the other does to cause confusion.
To support variable number of arguments with errno I'd propose just to use ksft_print_msg() with strerror(errno), errno as the arguments (or even %m that that errno handling within ksft_print_msg() aims to support). This does indeed seem to be the custom in other tests.
Does something like this look okay?
fp = fopen(file_path, "r"); if (!fp) { ksft_print_msg("Error in opening %s\n: %m\n", file_path); return -1; }
The '%m' seems to work fine but doesn't print errno's number code. Do you want me to add errno after '%m' so it is the same as ksft_perror()? I looked through some other tests where '%m' is used, and only few ones add errno with '%d'.
Reinette
On Wed, 31 Jan 2024, Maciej Wieczor-Retman wrote:
On 2024-01-26 at 10:58:04 -0800, Reinette Chatre wrote:
On 1/25/2024 4:14 AM, Ilpo Järvinen wrote:
On Thu, 25 Jan 2024, Maciej Wieczor-Retman wrote:
- fp = fopen(file_path, "r");
- if (!fp) {
snprintf(reason, sizeof(reason), "Error in opening %s file\n", filename);ksft_perror(reason);Was this the conclusion of the kstf_perror() discussion with Reinette? I expected a bit different outcome when I stopped following it...
In any case, it would be nice though if ksft_perror() (or some kselftest.h function yet to be added with a different name) would accept full printf interface and just add the errno string into the end of the string so one would not need to build constructs like this at all.
It will require a bit of macro trickery into kselftest.h. I don't know how it should handle the case where somebody just passes a char pointer to it, not a string literal, but I guess it would just throw an error while compiling if somebody tries to do that as the macro string literal concatenation could not build useful/compilable token.
It would make these prints informative enough to become actually useful without needed to resort to preparing the string in advance which seems to be required almost every single case with the current interface.
I think this can be accomplished with a new: void ksft_vprint_msg(const char *msg, va_list args)
... but ksft_perror() does conform to perror() and I expect that having one support variable number of arguments while the other does to cause confusion.
To support variable number of arguments with errno I'd propose just to use ksft_print_msg() with strerror(errno), errno as the arguments (or even %m that that errno handling within ksft_print_msg() aims to support). This does indeed seem to be the custom in other tests.
Does something like this look okay?
fp = fopen(file_path, "r"); if (!fp) { ksft_print_msg("Error in opening %s\n: %m\n", file_path); return -1; }
The '%m' seems to work fine but doesn't print errno's number code. Do you want me to add errno after '%m' so it is the same as ksft_perror()? I looked through some other tests where '%m' is used, and only few ones add errno with '%d'.
I think %m is enough.
Hi Maciej,
On 1/25/2024 3:10 AM, Maciej Wieczor-Retman wrote:
The CAT non-contiguous selftests have to read the file responsible for reporting support of non-contiguous CBMs in kernel (resctrl). Then the test compares if that information matches what is reported by CPUID output.
Add a generic helper function to read an unsigned number from a file in /sys/fs/resctrl/info/<RESOURCE>/<FILE>.
Signed-off-by: Maciej Wieczor-Retman maciej.wieczor-retman@intel.com
Changelog v3:
- Rewrite patch message.
- Add documentation and rewrote the function. (Reinette)
Changelog v2:
- Add this patch.
tools/testing/selftests/resctrl/resctrl.h | 1 + tools/testing/selftests/resctrl/resctrlfs.c | 39 +++++++++++++++++++++ 2 files changed, 40 insertions(+)
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index a1462029998e..5116ea082d03 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -162,6 +162,7 @@ unsigned int count_contiguous_bits(unsigned long val, unsigned int *start); int get_full_cbm(const char *cache_type, unsigned long *mask); int get_mask_no_shareable(const char *cache_type, unsigned long *mask); int get_cache_size(int cpu_no, const char *cache_type, unsigned long *cache_size); +int resource_info_unsigned_get(const char *resource, const char *filename, unsigned int *val); void ctrlc_handler(int signum, siginfo_t *info, void *ptr); int signal_handler_register(void); void signal_handler_unregister(void); diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index 5750662cce57..cb5147c5f9a9 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -249,6 +249,45 @@ static int get_bit_mask(const char *filename, unsigned long *mask) return 0; } +/*
By not starting with /** the following will not be interpreted as kernel-doc but the formatting does appear to follow the syntax, but not consistently so. I think it would be more readable if the kernel-doc syntax is followed consistently.
- resource_info_unsigned_get - Read an unsigned value from a file in
- /sys/fs/resctrl/info/RESOURCE/FILENAME
"Read an unsigned value from /sys/fs/resctrl/info/RESOURCE/FILENAME"?
- @resource: Resource name that matches directory names in
names? (plural)
/sys/fs/resctrl/info
- @filename: Filename of a file located in a directory specified with the
'resource' variable.
I think this can be shortened to "File in /sys/fs/resctrl/info/@resource"
- @val: Variable where the read value is saved on success.
"Contains read value on success."
(no need to refer to it as a variable/parameter, it is implied by syntax).
- Return: = 0 on success, < 0 on failure. On success the read value is saved into the 'val'
- variable.
"saved into the 'val' variable" -> "saved into @val" (since syntax indicates it is the parameter there is no need to elaborate). Also please let lines in comments be of consistent length.
- */
+int resource_info_unsigned_get(const char *resource, const char *filename,
unsigned int *val)+{
- char reason[128], file_path[PATH_MAX];
- FILE *fp;
- snprintf(file_path, sizeof(file_path), "%s/%s/%s", INFO_PATH, resource,
filename);- fp = fopen(file_path, "r");
- if (!fp) {
snprintf(reason, sizeof(reason), "Error in opening %s file\n", filename);
(apart from other discussions). "file" in message seems redundant. It can just be "Error opening %s". It may also be useful to print file_path instead of filename to be specific of what the code tried to open.
ksft_perror(reason);return -1;- }
- if (fscanf(fp, "%u", val) <= 0) {
snprintf(reason, sizeof(reason), "Could not get %s's contents\n", filename);ksft_perror(reason);
filename -> file_path ?
fclose(fp);return -1;- }
- fclose(fp);
- return 0;
+}
Reinette
Hello!
On 2024-01-26 at 13:08:19 -0800, Reinette Chatre wrote:
Hi Maciej,
On 1/25/2024 3:10 AM, Maciej Wieczor-Retman wrote:
The CAT non-contiguous selftests have to read the file responsible for reporting support of non-contiguous CBMs in kernel (resctrl). Then the test compares if that information matches what is reported by CPUID output.
Add a generic helper function to read an unsigned number from a file in /sys/fs/resctrl/info/<RESOURCE>/<FILE>.
Signed-off-by: Maciej Wieczor-Retman maciej.wieczor-retman@intel.com
Changelog v3:
- Rewrite patch message.
- Add documentation and rewrote the function. (Reinette)
Changelog v2:
- Add this patch.
tools/testing/selftests/resctrl/resctrl.h | 1 + tools/testing/selftests/resctrl/resctrlfs.c | 39 +++++++++++++++++++++ 2 files changed, 40 insertions(+)
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index a1462029998e..5116ea082d03 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -162,6 +162,7 @@ unsigned int count_contiguous_bits(unsigned long val, unsigned int *start); int get_full_cbm(const char *cache_type, unsigned long *mask); int get_mask_no_shareable(const char *cache_type, unsigned long *mask); int get_cache_size(int cpu_no, const char *cache_type, unsigned long *cache_size); +int resource_info_unsigned_get(const char *resource, const char *filename, unsigned int *val); void ctrlc_handler(int signum, siginfo_t *info, void *ptr); int signal_handler_register(void); void signal_handler_unregister(void); diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index 5750662cce57..cb5147c5f9a9 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -249,6 +249,45 @@ static int get_bit_mask(const char *filename, unsigned long *mask) return 0; } +/*
By not starting with /** the following will not be interpreted as kernel-doc but the formatting does appear to follow the syntax, but not consistently so. I think it would be more readable if the kernel-doc syntax is followed consistently.
Sure, I'll change it.
- resource_info_unsigned_get - Read an unsigned value from a file in
- /sys/fs/resctrl/info/RESOURCE/FILENAME
"Read an unsigned value from /sys/fs/resctrl/info/RESOURCE/FILENAME"?
Okay
- @resource: Resource name that matches directory names in
names? (plural)
Right, it doesn't make much sense, I'll move it to singular.
/sys/fs/resctrl/info
- @filename: Filename of a file located in a directory specified with the
'resource' variable.I think this can be shortened to "File in /sys/fs/resctrl/info/@resource"
Sure, thanks
- @val: Variable where the read value is saved on success.
"Contains read value on success."
(no need to refer to it as a variable/parameter, it is implied by syntax).
Right, I'll change it.
- Return: = 0 on success, < 0 on failure. On success the read value is saved into the 'val'
- variable.
"saved into the 'val' variable" -> "saved into @val" (since syntax indicates it is the parameter there is no need to elaborate).
Sure, thanks
Also please let lines in comments be of consistent length.
Okay, I'll keep it to 80 characters.
- */
+int resource_info_unsigned_get(const char *resource, const char *filename,
unsigned int *val)+{
- char reason[128], file_path[PATH_MAX];
- FILE *fp;
- snprintf(file_path, sizeof(file_path), "%s/%s/%s", INFO_PATH, resource,
filename);- fp = fopen(file_path, "r");
- if (!fp) {
snprintf(reason, sizeof(reason), "Error in opening %s file\n", filename);(apart from other discussions). "file" in message seems redundant. It can just be "Error opening %s". It may also be useful to print file_path instead of filename to be specific of what the code tried to open.
Okay, I'll change it to file_path.
ksft_perror(reason);return -1;- }
- if (fscanf(fp, "%u", val) <= 0) {
snprintf(reason, sizeof(reason), "Could not get %s's contents\n", filename);ksft_perror(reason);filename -> file_path ?
Same as above.
fclose(fp);return -1;- }
- fclose(fp);
- return 0;
+}
Reinette
validate_resctrl_feature_request() is used to test both if a resource is present in the info directory, and if a passed monitoring feature is present in the mon_features file.
Refactor validate_resctrl_feature_request() into two smaller functions that each accomplish one check to give feature checking more granularity: - Resource directory presence in the /sys/fs/resctrl/info directory. - Feature name presence in the /sys/fs/resctrl/info/L3_MON/mon_features file.
Signed-off-by: Maciej Wieczor-Retman maciej.wieczor-retman@intel.com --- Changelog v3: - Move new function to a separate patch. (Reinette) - Rewrite resctrl_mon_feature_exists() only for L3_MON.
Changelog v2: - Add this patch.
tools/testing/selftests/resctrl/cmt_test.c | 4 +-- tools/testing/selftests/resctrl/mba_test.c | 4 +-- tools/testing/selftests/resctrl/mbm_test.c | 6 ++-- tools/testing/selftests/resctrl/resctrl.h | 3 +- tools/testing/selftests/resctrl/resctrlfs.c | 33 +++++++++++++-------- 5 files changed, 30 insertions(+), 20 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c index dd5ca343c469..428de9df81c8 100644 --- a/tools/testing/selftests/resctrl/cmt_test.c +++ b/tools/testing/selftests/resctrl/cmt_test.c @@ -169,8 +169,8 @@ static int cmt_run_test(const struct resctrl_test *test, const struct user_param
static bool cmt_feature_check(const struct resctrl_test *test) { - return test_resource_feature_check(test) && - validate_resctrl_feature_request("L3_MON", "llc_occupancy"); + return resctrl_mon_feature_exists("llc_occupancy") && + resctrl_resource_exists("L3"); }
struct resctrl_test cmt_test = { diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c index da256d2dbe5c..e22285b80e37 100644 --- a/tools/testing/selftests/resctrl/mba_test.c +++ b/tools/testing/selftests/resctrl/mba_test.c @@ -170,8 +170,8 @@ static int mba_run_test(const struct resctrl_test *test, const struct user_param
static bool mba_feature_check(const struct resctrl_test *test) { - return test_resource_feature_check(test) && - validate_resctrl_feature_request("L3_MON", "mbm_local_bytes"); + return resctrl_resource_exists(test->resource) && + resctrl_mon_feature_exists("mbm_local_bytes"); }
struct resctrl_test mba_test = { diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c index 34879e7b71a0..9c885bc427ca 100644 --- a/tools/testing/selftests/resctrl/mbm_test.c +++ b/tools/testing/selftests/resctrl/mbm_test.c @@ -97,7 +97,7 @@ static int mbm_setup(const struct resctrl_test *test, return END_OF_TESTS;
/* Set up shemata with 100% allocation on the first run. */ - if (p->num_of_runs == 0 && validate_resctrl_feature_request("MB", NULL)) + if (p->num_of_runs == 0 && resctrl_resource_exists("MB")) ret = write_schemata(p->ctrlgrp, "100", uparams->cpu, test->resource);
p->num_of_runs++; @@ -140,8 +140,8 @@ static int mbm_run_test(const struct resctrl_test *test, const struct user_param
static bool mbm_feature_check(const struct resctrl_test *test) { - return validate_resctrl_feature_request("L3_MON", "mbm_total_bytes") && - validate_resctrl_feature_request("L3_MON", "mbm_local_bytes"); + return resctrl_mon_feature_exists("mbm_total_bytes") && + resctrl_mon_feature_exists("mbm_local_bytes"); }
struct resctrl_test mbm_test = { diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index 5116ea082d03..4603b215b97e 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -136,7 +136,8 @@ int get_domain_id(const char *resource, int cpu_no, int *domain_id); int mount_resctrlfs(void); int umount_resctrlfs(void); int validate_bw_report_request(char *bw_report); -bool validate_resctrl_feature_request(const char *resource, const char *feature); +bool resctrl_resource_exists(const char *resource); +bool resctrl_mon_feature_exists(const char *feature); bool test_resource_feature_check(const struct resctrl_test *test); char *fgrep(FILE *inf, const char *str); int taskset_benchmark(pid_t bm_pid, int cpu_no, cpu_set_t *old_affinity); diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index cb5147c5f9a9..e4ba8614fb7b 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -711,20 +711,16 @@ char *fgrep(FILE *inf, const char *str) }
/* - * validate_resctrl_feature_request - Check if requested feature is valid. - * @resource: Required resource (e.g., MB, L3, L2, L3_MON, etc.) - * @feature: Required monitor feature (in mon_features file). Can only be - * set for L3_MON. Must be NULL for all other resources. + * resctrl_resource_exists - Check if a resource is supported. + * @resource: Resctrl resource (e.g., MB, L3, L2, L3_MON, etc.) * - * Return: True if the resource/feature is supported, else false. False is + * Return: True if the resource is supported, else false. False is * also returned if resctrl FS is not mounted. */ -bool validate_resctrl_feature_request(const char *resource, const char *feature) +bool resctrl_resource_exists(const char *resource) { char res_path[PATH_MAX]; struct stat statbuf; - char *res; - FILE *inf; int ret;
if (!resource) @@ -739,11 +735,24 @@ bool validate_resctrl_feature_request(const char *resource, const char *feature) if (stat(res_path, &statbuf)) return false;
+ return true; +} + +/* + * resctrl_mon_feature_exists - Check if requested monitoring L3_MON feature is valid. + * @feature: Required monitor feature (in mon_features file). + * + * Return: True if the feature is supported, else false. + */ +bool resctrl_mon_feature_exists(const char *feature) +{ + char *res; + FILE *inf; + if (!feature) - return true; + return false;
- snprintf(res_path, sizeof(res_path), "%s/%s/mon_features", INFO_PATH, resource); - inf = fopen(res_path, "r"); + inf = fopen("/sys/fs/resctrl/info/L3_MON/mon_features", "r"); if (!inf) return false;
@@ -756,7 +765,7 @@ bool validate_resctrl_feature_request(const char *resource, const char *feature)
bool test_resource_feature_check(const struct resctrl_test *test) { - return validate_resctrl_feature_request(test->resource, NULL); + return resctrl_resource_exists(test->resource); }
int filter_dmesg(void)
On Thu, 25 Jan 2024, Maciej Wieczor-Retman wrote:
validate_resctrl_feature_request() is used to test both if a resource is present in the info directory, and if a passed monitoring feature is present in the mon_features file.
Refactor validate_resctrl_feature_request() into two smaller functions that each accomplish one check to give feature checking more granularity:
- Resource directory presence in the /sys/fs/resctrl/info directory.
- Feature name presence in the /sys/fs/resctrl/info/L3_MON/mon_features file.
Signed-off-by: Maciej Wieczor-Retman maciej.wieczor-retman@intel.com
Changelog v3:
- Move new function to a separate patch. (Reinette)
- Rewrite resctrl_mon_feature_exists() only for L3_MON.
Changelog v2:
- Add this patch.
tools/testing/selftests/resctrl/cmt_test.c | 4 +-- tools/testing/selftests/resctrl/mba_test.c | 4 +-- tools/testing/selftests/resctrl/mbm_test.c | 6 ++-- tools/testing/selftests/resctrl/resctrl.h | 3 +- tools/testing/selftests/resctrl/resctrlfs.c | 33 +++++++++++++-------- 5 files changed, 30 insertions(+), 20 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c index dd5ca343c469..428de9df81c8 100644 --- a/tools/testing/selftests/resctrl/cmt_test.c +++ b/tools/testing/selftests/resctrl/cmt_test.c @@ -169,8 +169,8 @@ static int cmt_run_test(const struct resctrl_test *test, const struct user_param static bool cmt_feature_check(const struct resctrl_test *test) {
- return test_resource_feature_check(test) &&
validate_resctrl_feature_request("L3_MON", "llc_occupancy");
- return resctrl_mon_feature_exists("llc_occupancy") &&
resctrl_resource_exists("L3");} struct resctrl_test cmt_test = { diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c index da256d2dbe5c..e22285b80e37 100644 --- a/tools/testing/selftests/resctrl/mba_test.c +++ b/tools/testing/selftests/resctrl/mba_test.c @@ -170,8 +170,8 @@ static int mba_run_test(const struct resctrl_test *test, const struct user_param static bool mba_feature_check(const struct resctrl_test *test) {
- return test_resource_feature_check(test) &&
validate_resctrl_feature_request("L3_MON", "mbm_local_bytes");
- return resctrl_resource_exists(test->resource) &&
I don't understand what's the advantage of converting away from test_resource_feature_check() in CMT and MBA case?
resctrl_mon_feature_exists("mbm_local_bytes");}
@@ -756,7 +765,7 @@ bool validate_resctrl_feature_request(const char *resource, const char *feature) bool test_resource_feature_check(const struct resctrl_test *test) {
- return validate_resctrl_feature_request(test->resource, NULL);
- return resctrl_resource_exists(test->resource);
...The replacement in MBA open coded test_resource_feature_check() 100% and CMT even replaces the test->resource with the string matching to what's in test->resource?
Hi Ilpo,
On 2024-01-25 at 13:46:51 +0200, Ilpo Järvinen wrote:
On Thu, 25 Jan 2024, Maciej Wieczor-Retman wrote:
validate_resctrl_feature_request() is used to test both if a resource is present in the info directory, and if a passed monitoring feature is present in the mon_features file.
Refactor validate_resctrl_feature_request() into two smaller functions that each accomplish one check to give feature checking more granularity:
- Resource directory presence in the /sys/fs/resctrl/info directory.
- Feature name presence in the /sys/fs/resctrl/info/L3_MON/mon_features file.
Signed-off-by: Maciej Wieczor-Retman maciej.wieczor-retman@intel.com
Changelog v3:
- Move new function to a separate patch. (Reinette)
- Rewrite resctrl_mon_feature_exists() only for L3_MON.
Changelog v2:
- Add this patch.
tools/testing/selftests/resctrl/cmt_test.c | 4 +-- tools/testing/selftests/resctrl/mba_test.c | 4 +-- tools/testing/selftests/resctrl/mbm_test.c | 6 ++-- tools/testing/selftests/resctrl/resctrl.h | 3 +- tools/testing/selftests/resctrl/resctrlfs.c | 33 +++++++++++++-------- 5 files changed, 30 insertions(+), 20 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c index dd5ca343c469..428de9df81c8 100644 --- a/tools/testing/selftests/resctrl/cmt_test.c +++ b/tools/testing/selftests/resctrl/cmt_test.c @@ -169,8 +169,8 @@ static int cmt_run_test(const struct resctrl_test *test, const struct user_param static bool cmt_feature_check(const struct resctrl_test *test) {
- return test_resource_feature_check(test) &&
validate_resctrl_feature_request("L3_MON", "llc_occupancy");
- return resctrl_mon_feature_exists("llc_occupancy") &&
resctrl_resource_exists("L3");} struct resctrl_test cmt_test = { diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c index da256d2dbe5c..e22285b80e37 100644 --- a/tools/testing/selftests/resctrl/mba_test.c +++ b/tools/testing/selftests/resctrl/mba_test.c @@ -170,8 +170,8 @@ static int mba_run_test(const struct resctrl_test *test, const struct user_param static bool mba_feature_check(const struct resctrl_test *test) {
- return test_resource_feature_check(test) &&
validate_resctrl_feature_request("L3_MON", "mbm_local_bytes");
- return resctrl_resource_exists(test->resource) &&
I don't understand what's the advantage of converting away from test_resource_feature_check() in CMT and MBA case?
resctrl_mon_feature_exists("mbm_local_bytes");}
@@ -756,7 +765,7 @@ bool validate_resctrl_feature_request(const char *resource, const char *feature) bool test_resource_feature_check(const struct resctrl_test *test) {
- return validate_resctrl_feature_request(test->resource, NULL);
- return resctrl_resource_exists(test->resource);
...The replacement in MBA open coded test_resource_feature_check() 100% and CMT even replaces the test->resource with the string matching to what's in test->resource?
You're right, I got carried away with refactoring a bit. I'll keep test_resource_feature_check() for CMT and MBM. Thanks!
-- i.
Feature checking done by resctrl_mon_feature_exists() covers features represented by the feature name presence inside the 'mon_features' file in /sys/fs/resctrl/info/L3_MON directory. There exists a different way to represent feature support and that is by the presence of 0 or 1 in a single file in the info/resource directory. In this case the filename represents what feature support is being indicated.
Add a generic function to check file presence in the /sys/fs/resctrl/info/<RESOURCE> directory.
Signed-off-by: Maciej Wieczor-Retman maciej.wieczor-retman@intel.com --- Changelog v3: - Split off the new function into this patch. (Reinette)
Changelog v2: - Add this patch.
tools/testing/selftests/resctrl/resctrl.h | 2 ++ tools/testing/selftests/resctrl/resctrlfs.c | 26 +++++++++++++++++++++ 2 files changed, 28 insertions(+)
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index 4603b215b97e..c39105f46da9 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -138,6 +138,8 @@ int umount_resctrlfs(void); int validate_bw_report_request(char *bw_report); bool resctrl_resource_exists(const char *resource); bool resctrl_mon_feature_exists(const char *feature); +bool resource_info_file_exists(const char *resource, + const char *feature); bool test_resource_feature_check(const struct resctrl_test *test); char *fgrep(FILE *inf, const char *str); int taskset_benchmark(pid_t bm_pid, int cpu_no, cpu_set_t *old_affinity); diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index e4ba8614fb7b..a6427732e0ad 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -763,6 +763,32 @@ bool resctrl_mon_feature_exists(const char *feature) return !!res; }
+/* + * resource_info_file_exists - Check if a file is present inside + * /sys/fs/resctrl/info/RESOURCE. + * @resource: Required resource (Eg: MB, L3, L2, etc.) + * @feature: Required feature. + * + * Return: True if the file exists, else false. + */ +bool resource_info_file_exists(const char *resource, + const char *feature) +{ + char res_path[PATH_MAX]; + struct stat statbuf; + + if (!feature || !resource) + return false; + + snprintf(res_path, sizeof(res_path), "%s/%s/%s", INFO_PATH, resource, + feature); + + if (stat(res_path, &statbuf)) + return false; + + return true; +} + bool test_resource_feature_check(const struct resctrl_test *test) { return resctrl_resource_exists(test->resource);
On Thu, 25 Jan 2024, Maciej Wieczor-Retman wrote:
Feature checking done by resctrl_mon_feature_exists() covers features represented by the feature name presence inside the 'mon_features' file in /sys/fs/resctrl/info/L3_MON directory. There exists a different way to represent feature support and that is by the presence of 0 or 1 in a single file in the info/resource directory. In this case the filename represents what feature support is being indicated.
Add a generic function to check file presence in the /sys/fs/resctrl/info/<RESOURCE> directory.
Signed-off-by: Maciej Wieczor-Retman maciej.wieczor-retman@intel.com
Changelog v3:
- Split off the new function into this patch. (Reinette)
Changelog v2:
- Add this patch.
tools/testing/selftests/resctrl/resctrl.h | 2 ++ tools/testing/selftests/resctrl/resctrlfs.c | 26 +++++++++++++++++++++ 2 files changed, 28 insertions(+)
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index 4603b215b97e..c39105f46da9 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -138,6 +138,8 @@ int umount_resctrlfs(void); int validate_bw_report_request(char *bw_report); bool resctrl_resource_exists(const char *resource); bool resctrl_mon_feature_exists(const char *feature); +bool resource_info_file_exists(const char *resource,
const char *feature);bool test_resource_feature_check(const struct resctrl_test *test); char *fgrep(FILE *inf, const char *str); int taskset_benchmark(pid_t bm_pid, int cpu_no, cpu_set_t *old_affinity); diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index e4ba8614fb7b..a6427732e0ad 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -763,6 +763,32 @@ bool resctrl_mon_feature_exists(const char *feature) return !!res; } +/*
- resource_info_file_exists - Check if a file is present inside
- /sys/fs/resctrl/info/RESOURCE.
- @resource: Required resource (Eg: MB, L3, L2, etc.)
- @feature: Required feature.
- Return: True if the file exists, else false.
- */
+bool resource_info_file_exists(const char *resource,
const char *feature)
Fits to one line.
+{
- char res_path[PATH_MAX];
- struct stat statbuf;
- if (!feature || !resource)
return false;- snprintf(res_path, sizeof(res_path), "%s/%s/%s", INFO_PATH, resource,
feature);- if (stat(res_path, &statbuf))
return false;- return true;
+}
bool test_resource_feature_check(const struct resctrl_test *test) { return resctrl_resource_exists(test->resource);
Reviewed-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com
Hi Maciej,
On 1/25/2024 3:12 AM, Maciej Wieczor-Retman wrote:
Feature checking done by resctrl_mon_feature_exists() covers features represented by the feature name presence inside the 'mon_features' file in /sys/fs/resctrl/info/L3_MON directory. There exists a different way to represent feature support and that is by the presence of 0 or 1 in a single file in the info/resource directory. In this case the filename represents what feature support is being indicated.
Add a generic function to check file presence in the /sys/fs/resctrl/info/<RESOURCE> directory.
Signed-off-by: Maciej Wieczor-Retman maciej.wieczor-retman@intel.com
Changelog v3:
- Split off the new function into this patch. (Reinette)
Changelog v2:
- Add this patch.
tools/testing/selftests/resctrl/resctrl.h | 2 ++ tools/testing/selftests/resctrl/resctrlfs.c | 26 +++++++++++++++++++++ 2 files changed, 28 insertions(+)
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index 4603b215b97e..c39105f46da9 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -138,6 +138,8 @@ int umount_resctrlfs(void); int validate_bw_report_request(char *bw_report); bool resctrl_resource_exists(const char *resource); bool resctrl_mon_feature_exists(const char *feature); +bool resource_info_file_exists(const char *resource,
const char *feature);
In addition what Ilpo commented about other line, this too can be on one line.
bool test_resource_feature_check(const struct resctrl_test *test); char *fgrep(FILE *inf, const char *str); int taskset_benchmark(pid_t bm_pid, int cpu_no, cpu_set_t *old_affinity); diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index e4ba8614fb7b..a6427732e0ad 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -763,6 +763,32 @@ bool resctrl_mon_feature_exists(const char *feature) return !!res; } +/*
- resource_info_file_exists - Check if a file is present inside
- /sys/fs/resctrl/info/RESOURCE.
As confirmed in the changelog this is intended to be a generic function that tests if a file exists ...
- @resource: Required resource (Eg: MB, L3, L2, etc.)
- @feature: Required feature.
... so assuming a use of this by referring to the file as "feature" is unexpected.
This function jumps between "file" and "feature" in comments and code, please just stick to goal of making it a generic utility that checks for a file and refer to it consistently so.
Reinette
Hello Reinette,
On 2024-01-26 at 13:08:52 -0800, Reinette Chatre wrote:
Hi Maciej,
On 1/25/2024 3:12 AM, Maciej Wieczor-Retman wrote:
Feature checking done by resctrl_mon_feature_exists() covers features represented by the feature name presence inside the 'mon_features' file in /sys/fs/resctrl/info/L3_MON directory. There exists a different way to represent feature support and that is by the presence of 0 or 1 in a single file in the info/resource directory. In this case the filename represents what feature support is being indicated.
Add a generic function to check file presence in the /sys/fs/resctrl/info/<RESOURCE> directory.
Signed-off-by: Maciej Wieczor-Retman maciej.wieczor-retman@intel.com
Changelog v3:
- Split off the new function into this patch. (Reinette)
Changelog v2:
- Add this patch.
tools/testing/selftests/resctrl/resctrl.h | 2 ++ tools/testing/selftests/resctrl/resctrlfs.c | 26 +++++++++++++++++++++ 2 files changed, 28 insertions(+)
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index 4603b215b97e..c39105f46da9 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -138,6 +138,8 @@ int umount_resctrlfs(void); int validate_bw_report_request(char *bw_report); bool resctrl_resource_exists(const char *resource); bool resctrl_mon_feature_exists(const char *feature); +bool resource_info_file_exists(const char *resource,
const char *feature);In addition what Ilpo commented about other line, this too can be on one line.
Thanks!
bool test_resource_feature_check(const struct resctrl_test *test); char *fgrep(FILE *inf, const char *str); int taskset_benchmark(pid_t bm_pid, int cpu_no, cpu_set_t *old_affinity); diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index e4ba8614fb7b..a6427732e0ad 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -763,6 +763,32 @@ bool resctrl_mon_feature_exists(const char *feature) return !!res; } +/*
- resource_info_file_exists - Check if a file is present inside
- /sys/fs/resctrl/info/RESOURCE.
As confirmed in the changelog this is intended to be a generic function that tests if a file exists ...
- @resource: Required resource (Eg: MB, L3, L2, etc.)
- @feature: Required feature.
... so assuming a use of this by referring to the file as "feature" is unexpected.
This function jumps between "file" and "feature" in comments and code, please just stick to goal of making it a generic utility that checks for a file and refer to it consistently so.
Okay, I'll stick to 'file'.
Reinette
Add tests for both L2 and L3 CAT to verify the return values generated by writing non-contiguous CBMs don't contradict the reported non-contiguous support information.
Use a logical XOR to confirm return value of write_schemata() and non-contiguous CBMs support information match.
Signed-off-by: Maciej Wieczor-Retman maciej.wieczor-retman@intel.com --- Changelog v3: - Roll back __cpuid_count part. (Reinette) - Update function name to read sparse_masks file. - Roll back get_cache_level() changes. - Add ksft_print_msg() to contiguous schemata write error handling (Reinette).
Changelog v2: - Redo the patch message. (Ilpo) - Tidy up __cpuid_count calls. (Ilpo) - Remove redundant AND in noncont_mask calculations (Ilpo) - Fix bit_center offset. - Add newline before function return. (Ilpo) - Group non-contiguous tests with CAT tests. (Ilpo) - Use a helper for reading sparse_masks file. (Ilpo) - Make get_cache_level() available in other source files. (Ilpo)
tools/testing/selftests/resctrl/cat_test.c | 81 +++++++++++++++++++ tools/testing/selftests/resctrl/resctrl.h | 2 + .../testing/selftests/resctrl/resctrl_tests.c | 2 + 3 files changed, 85 insertions(+)
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index 39fc9303b8e8..9086bf359072 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -294,6 +294,71 @@ static int cat_run_test(const struct resctrl_test *test, const struct user_param return ret; }
+static int noncont_cat_run_test(const struct resctrl_test *test, + const struct user_params *uparams) +{ + unsigned long full_cache_mask, cont_mask, noncont_mask; + unsigned int eax, ebx, ecx, edx, ret, sparse_masks; + char schemata[64]; + int bit_center; + + /* Check to compare sparse_masks content to CPUID output. */ + ret = resource_info_unsigned_get(test->resource, "sparse_masks", &sparse_masks); + if (ret) + return ret; + + 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 -EINVAL; + + if (sparse_masks != ((ecx >> 3) & 1)) { + ksft_print_msg("CPUID output doesn't match 'sparse_masks' file content!\n"); + return -1; + } + + /* Write checks initialization. */ + ret = get_full_cbm(test->resource, &full_cache_mask); + if (ret < 0) + return ret; + bit_center = count_bits(full_cache_mask) / 2; + cont_mask = full_cache_mask >> bit_center; + + /* Contiguous mask write check. */ + snprintf(schemata, sizeof(schemata), "%lx", cont_mask); + ret = write_schemata("", schemata, uparams->cpu, test->resource); + if (ret) { + ksft_print_msg("Write of contiguous CBM failed\n"); + return ret; + } + + /* + * Non-contiguous mask write check. CBM has a 0xf hole approximately in the middle. + * Output is compared with support information to catch any edge case errors. + */ + noncont_mask = ~(0xf << (bit_center - 2)) & full_cache_mask; + snprintf(schemata, sizeof(schemata), "%lx", noncont_mask); + ret = write_schemata("", schemata, uparams->cpu, test->resource); + if (ret && sparse_masks) + ksft_print_msg("Non-contiguous CBMs supported but write of non-contiguous CBM failed\n"); + else if (ret && !sparse_masks) + ksft_print_msg("Non-contiguous CBMs not supported and write of non-contiguous CBM failed as expected\n"); + else if (!ret && !sparse_masks) + ksft_print_msg("Non-contiguous CBMs not supported but write of non-contiguous CBM succeeded\n"); + + return !ret == !sparse_masks; +} + +static bool noncont_cat_feature_check(const struct resctrl_test *test) +{ + if (!resctrl_resource_exists(test->resource)) + return false; + + return resource_info_file_exists(test->resource, "sparse_masks"); +} + struct resctrl_test l3_cat_test = { .name = "L3_CAT", .group = "CAT", @@ -301,3 +366,19 @@ struct resctrl_test l3_cat_test = { .feature_check = test_resource_feature_check, .run_test = cat_run_test, }; + +struct resctrl_test l3_noncont_cat_test = { + .name = "L3_NONCONT_CAT", + .group = "CAT", + .resource = "L3", + .feature_check = noncont_cat_feature_check, + .run_test = noncont_cat_run_test, +}; + +struct resctrl_test l2_noncont_cat_test = { + .name = "L2_NONCONT_CAT", + .group = "CAT", + .resource = "L2", + .feature_check = noncont_cat_feature_check, + .run_test = noncont_cat_run_test, +}; diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index c39105f46da9..8cb97f278459 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -210,5 +210,7 @@ extern struct resctrl_test mbm_test; extern struct resctrl_test mba_test; extern struct resctrl_test cmt_test; extern struct resctrl_test l3_cat_test; +extern struct resctrl_test l3_noncont_cat_test; +extern struct resctrl_test l2_noncont_cat_test;
#endif /* RESCTRL_H */ diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index 3044179ee6e9..f3dc1b9696e7 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -19,6 +19,8 @@ static struct resctrl_test *resctrl_tests[] = { &mba_test, &cmt_test, &l3_cat_test, + &l3_noncont_cat_test, + &l2_noncont_cat_test, };
static int detect_vendor(void)
Hi Maciej,
On 1/25/2024 3:13 AM, Maciej Wieczor-Retman wrote:
Add tests for both L2 and L3 CAT to verify the return values generated by writing non-contiguous CBMs don't contradict the reported non-contiguous support information.
Use a logical XOR to confirm return value of write_schemata() and non-contiguous CBMs support information match.
Signed-off-by: Maciej Wieczor-Retman maciej.wieczor-retman@intel.com
Changelog v3:
- Roll back __cpuid_count part. (Reinette)
- Update function name to read sparse_masks file.
- Roll back get_cache_level() changes.
- Add ksft_print_msg() to contiguous schemata write error handling (Reinette).
Changelog v2:
- Redo the patch message. (Ilpo)
- Tidy up __cpuid_count calls. (Ilpo)
- Remove redundant AND in noncont_mask calculations (Ilpo)
- Fix bit_center offset.
- Add newline before function return. (Ilpo)
- Group non-contiguous tests with CAT tests. (Ilpo)
- Use a helper for reading sparse_masks file. (Ilpo)
- Make get_cache_level() available in other source files. (Ilpo)
tools/testing/selftests/resctrl/cat_test.c | 81 +++++++++++++++++++ tools/testing/selftests/resctrl/resctrl.h | 2 + .../testing/selftests/resctrl/resctrl_tests.c | 2 + 3 files changed, 85 insertions(+)
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index 39fc9303b8e8..9086bf359072 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -294,6 +294,71 @@ static int cat_run_test(const struct resctrl_test *test, const struct user_param return ret; } +static int noncont_cat_run_test(const struct resctrl_test *test,
const struct user_params *uparams)+{
- unsigned long full_cache_mask, cont_mask, noncont_mask;
- unsigned int eax, ebx, ecx, edx, ret, sparse_masks;
- char schemata[64];
- int bit_center;
- /* Check to compare sparse_masks content to CPUID output. */
- ret = resource_info_unsigned_get(test->resource, "sparse_masks", &sparse_masks);
- if (ret)
return ret;- 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 -EINVAL;- if (sparse_masks != ((ecx >> 3) & 1)) {
ksft_print_msg("CPUID output doesn't match 'sparse_masks' file content!\n");return -1;
If I understand correctly this falls into the "test failure" [1] category and should return 1? ...
- }
- /* Write checks initialization. */
- ret = get_full_cbm(test->resource, &full_cache_mask);
- if (ret < 0)
return ret;- bit_center = count_bits(full_cache_mask) / 2;
- cont_mask = full_cache_mask >> bit_center;
- /* Contiguous mask write check. */
- snprintf(schemata, sizeof(schemata), "%lx", cont_mask);
- ret = write_schemata("", schemata, uparams->cpu, test->resource);
- if (ret) {
ksft_print_msg("Write of contiguous CBM failed\n");return ret;
... although here I think the goal to distinguish between test error and test failure falls apart since it is not possible to tell within the test if the failure is because of error in the test or if test failed.
Reinette
[1] https://lore.kernel.org/all/33787043-5823-6de4-4e5c-a24a136ba541@linux.intel...
Hi!
On 2024-01-26 at 13:10:18 -0800, Reinette Chatre wrote:
Hi Maciej,
On 1/25/2024 3:13 AM, Maciej Wieczor-Retman wrote:
Add tests for both L2 and L3 CAT to verify the return values generated by writing non-contiguous CBMs don't contradict the reported non-contiguous support information.
Use a logical XOR to confirm return value of write_schemata() and non-contiguous CBMs support information match.
Signed-off-by: Maciej Wieczor-Retman maciej.wieczor-retman@intel.com
Changelog v3:
- Roll back __cpuid_count part. (Reinette)
- Update function name to read sparse_masks file.
- Roll back get_cache_level() changes.
- Add ksft_print_msg() to contiguous schemata write error handling (Reinette).
Changelog v2:
- Redo the patch message. (Ilpo)
- Tidy up __cpuid_count calls. (Ilpo)
- Remove redundant AND in noncont_mask calculations (Ilpo)
- Fix bit_center offset.
- Add newline before function return. (Ilpo)
- Group non-contiguous tests with CAT tests. (Ilpo)
- Use a helper for reading sparse_masks file. (Ilpo)
- Make get_cache_level() available in other source files. (Ilpo)
tools/testing/selftests/resctrl/cat_test.c | 81 +++++++++++++++++++ tools/testing/selftests/resctrl/resctrl.h | 2 + .../testing/selftests/resctrl/resctrl_tests.c | 2 + 3 files changed, 85 insertions(+)
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index 39fc9303b8e8..9086bf359072 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -294,6 +294,71 @@ static int cat_run_test(const struct resctrl_test *test, const struct user_param return ret; } +static int noncont_cat_run_test(const struct resctrl_test *test,
const struct user_params *uparams)+{
- unsigned long full_cache_mask, cont_mask, noncont_mask;
- unsigned int eax, ebx, ecx, edx, ret, sparse_masks;
- char schemata[64];
- int bit_center;
- /* Check to compare sparse_masks content to CPUID output. */
- ret = resource_info_unsigned_get(test->resource, "sparse_masks", &sparse_masks);
- if (ret)
return ret;- 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 -EINVAL;- if (sparse_masks != ((ecx >> 3) & 1)) {
ksft_print_msg("CPUID output doesn't match 'sparse_masks' file content!\n");return -1;If I understand correctly this falls into the "test failure" [1] category and should return 1? ...
- }
- /* Write checks initialization. */
- ret = get_full_cbm(test->resource, &full_cache_mask);
- if (ret < 0)
return ret;- bit_center = count_bits(full_cache_mask) / 2;
- cont_mask = full_cache_mask >> bit_center;
- /* Contiguous mask write check. */
- snprintf(schemata, sizeof(schemata), "%lx", cont_mask);
- ret = write_schemata("", schemata, uparams->cpu, test->resource);
- if (ret) {
ksft_print_msg("Write of contiguous CBM failed\n");return ret;... although here I think the goal to distinguish between test error and test failure falls apart since it is not possible to tell within the test if the failure is because of error in the test or if test failed.
Is there even a distinction between test error and failure in resctrl selftest? I've been looking at it for a while and can't find any instances where ksft_test_result_error() would be used. Everywhere I look it's either pass or fail. By grep-ing over all selftests I found only five tests that use ksft_test_result_error().
Furthermore there is this one "TODO" in kselftests.h:
/* TODO: how does "error" differ from "fail" or "skip"? */
If you meant the distintion less literally then I'd say the sparse_masks comparison to CPUID would be a failure. What I had in mind is that it tries to validate a resctrl interface relevant to non-contiguous CBMs. If it fails there is probably something wrong with the code concerning non-contiguous CBMs. On the other hand writing contiguous CBMs shouldn't fail as far as the non-contiguous CBMs in CAT test is concerned. So if that fails there might be something wrong on a higher level and I'd say that can be more of an error than a failure.
But I'm just saying how I undestood it so far. If there is some clear distinction between error and failure definitions I could try to separate it more explicitly.
Reinette
[1] https://lore.kernel.org/all/33787043-5823-6de4-4e5c-a24a136ba541@linux.intel...
Hi Maciej,
On 1/31/2024 4:55 AM, Maciej Wieczor-Retman wrote:
On 2024-01-26 at 13:10:18 -0800, Reinette Chatre wrote:
On 1/25/2024 3:13 AM, Maciej Wieczor-Retman wrote:
Add tests for both L2 and L3 CAT to verify the return values generated by writing non-contiguous CBMs don't contradict the reported non-contiguous support information.
Use a logical XOR to confirm return value of write_schemata() and non-contiguous CBMs support information match.
Signed-off-by: Maciej Wieczor-Retman maciej.wieczor-retman@intel.com
Changelog v3:
- Roll back __cpuid_count part. (Reinette)
- Update function name to read sparse_masks file.
- Roll back get_cache_level() changes.
- Add ksft_print_msg() to contiguous schemata write error handling (Reinette).
Changelog v2:
- Redo the patch message. (Ilpo)
- Tidy up __cpuid_count calls. (Ilpo)
- Remove redundant AND in noncont_mask calculations (Ilpo)
- Fix bit_center offset.
- Add newline before function return. (Ilpo)
- Group non-contiguous tests with CAT tests. (Ilpo)
- Use a helper for reading sparse_masks file. (Ilpo)
- Make get_cache_level() available in other source files. (Ilpo)
tools/testing/selftests/resctrl/cat_test.c | 81 +++++++++++++++++++ tools/testing/selftests/resctrl/resctrl.h | 2 + .../testing/selftests/resctrl/resctrl_tests.c | 2 + 3 files changed, 85 insertions(+)
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index 39fc9303b8e8..9086bf359072 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -294,6 +294,71 @@ static int cat_run_test(const struct resctrl_test *test, const struct user_param return ret; } +static int noncont_cat_run_test(const struct resctrl_test *test,
const struct user_params *uparams)+{
- unsigned long full_cache_mask, cont_mask, noncont_mask;
- unsigned int eax, ebx, ecx, edx, ret, sparse_masks;
- char schemata[64];
- int bit_center;
- /* Check to compare sparse_masks content to CPUID output. */
- ret = resource_info_unsigned_get(test->resource, "sparse_masks", &sparse_masks);
- if (ret)
return ret;- 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 -EINVAL;- if (sparse_masks != ((ecx >> 3) & 1)) {
ksft_print_msg("CPUID output doesn't match 'sparse_masks' file content!\n");return -1;If I understand correctly this falls into the "test failure" [1] category and should return 1? ...
- }
- /* Write checks initialization. */
- ret = get_full_cbm(test->resource, &full_cache_mask);
- if (ret < 0)
return ret;- bit_center = count_bits(full_cache_mask) / 2;
- cont_mask = full_cache_mask >> bit_center;
- /* Contiguous mask write check. */
- snprintf(schemata, sizeof(schemata), "%lx", cont_mask);
- ret = write_schemata("", schemata, uparams->cpu, test->resource);
- if (ret) {
ksft_print_msg("Write of contiguous CBM failed\n");return ret;... although here I think the goal to distinguish between test error and test failure falls apart since it is not possible to tell within the test if the failure is because of error in the test or if test failed.
Is there even a distinction between test error and failure in resctrl selftest?
There is such a distinction in the current tests (and from what I understand the reason behind the logical XOR used in this test) . In existing tests the running of the test precedes and is clearly separate from determining of the test pass/fail. All the current tests have a clear "run the test" phase where data is collected to a file, followed by an analysis (aka "check results") phase that looks at collected data to determine if the test passes or fails. Note how all the "check results" return either 0 or 1 to indicate test pass or fail respectively. Specifically, you can refer to: mbm_test.c->check_results() mba_test.c->check_results() cmt_test.c->check_results() cat_test.c->check_results()
I've been looking at it for a while and can't find any instances where ksft_test_result_error() would be used. Everywhere I look it's either pass or fail. By grep-ing over all selftests I found only five tests that use ksft_test_result_error().
Yes, from the user perspective there is no such distinction. This seems to be entirely internal to the resctrl selftests (but I do not think that this should or can be a hard requirement).
Furthermore there is this one "TODO" in kselftests.h:
/* TODO: how does "error" differ from "fail" or "skip"? */
If you meant the distintion less literally then I'd say the sparse_masks comparison to CPUID would be a failure. What I had in mind is that it tries to validate a resctrl interface relevant to non-contiguous CBMs. If it fails there is probably something wrong with the code concerning non-contiguous CBMs.
Wrong with which code? As I understand this particular check compares the resctrl view of the world to the hardware realities. If this check fails then I do not think this is an issue with the test code (which would make it a test error) but instead a resctrl bug and thus a test failure.
On the other hand writing contiguous CBMs shouldn't fail as far as the non-contiguous CBMs in CAT test is concerned. So if that fails there might be something wrong on a higher level and I'd say that can be more of an error than a failure.
I think that the write_schemata() can fail for a variety of reasons, some may indicate an issue with the test while some may indicate an issue with resctrl. It is not possible for the caller of write_schemata() to distinguish.
But I'm just saying how I undestood it so far. If there is some clear distinction between error and failure definitions I could try to separate it more explicitly.
I do not think it is possible to clearly distinguish between error and failure. These are already lumped together as a ksft_test_result_fail() anyway so no risk of confusion to folks just running the tests. I think the final test result may be confusing to folks parsing the resctrl selftest internals:
run_single_test() { ... ret = test->run_test(test, uparams); ksft_test_result(!ret, "%s: test\n", test->name); ... }
above means that a test returning negative or greater than zero value is considered a test failure and resctrl tests may return either in the case of an actual test failure ... but from user perspective there is no difference so I do not think it is an issue, just lack of consistency in the resctrl test internals in cases like write_schemata() failure where a possible test fail is captured as a test error.
I do not think it is required to be strict here. Keeping "test returns negative or greater than zero on test failure" seems reasonable to me.
Reinette
Hello!
On 2024-02-01 at 11:47:44 -0800, Reinette Chatre wrote:
Hi Maciej,
On 1/31/2024 4:55 AM, Maciej Wieczor-Retman wrote:
On 2024-01-26 at 13:10:18 -0800, Reinette Chatre wrote:
On 1/25/2024 3:13 AM, Maciej Wieczor-Retman wrote:
- if (sparse_masks != ((ecx >> 3) & 1)) {
ksft_print_msg("CPUID output doesn't match 'sparse_masks' file content!\n");return -1;If I understand correctly this falls into the "test failure" [1] category and should return 1? ...
- }
- /* Write checks initialization. */
- ret = get_full_cbm(test->resource, &full_cache_mask);
- if (ret < 0)
return ret;- bit_center = count_bits(full_cache_mask) / 2;
- cont_mask = full_cache_mask >> bit_center;
- /* Contiguous mask write check. */
- snprintf(schemata, sizeof(schemata), "%lx", cont_mask);
- ret = write_schemata("", schemata, uparams->cpu, test->resource);
- if (ret) {
ksft_print_msg("Write of contiguous CBM failed\n");return ret;... although here I think the goal to distinguish between test error and test failure falls apart since it is not possible to tell within the test if the failure is because of error in the test or if test failed.
Is there even a distinction between test error and failure in resctrl selftest?
There is such a distinction in the current tests (and from what I understand the reason behind the logical XOR used in this test) . In existing tests the running of the test precedes and is clearly separate from determining of the test pass/fail. All the current tests have a clear "run the test" phase where data is collected to a file, followed by an analysis (aka "check results") phase that looks at collected data to determine if the test passes or fails. Note how all the "check results" return either 0 or 1 to indicate test pass or fail respectively. Specifically, you can refer to: mbm_test.c->check_results() mba_test.c->check_results() cmt_test.c->check_results() cat_test.c->check_results()
I've been looking at it for a while and can't find any instances where ksft_test_result_error() would be used. Everywhere I look it's either pass or fail. By grep-ing over all selftests I found only five tests that use ksft_test_result_error().
Yes, from the user perspective there is no such distinction. This seems to be entirely internal to the resctrl selftests (but I do not think that this should or can be a hard requirement).
Okay, thank you, that's what I wanted to know.
Furthermore there is this one "TODO" in kselftests.h:
/* TODO: how does "error" differ from "fail" or "skip"? */
If you meant the distintion less literally then I'd say the sparse_masks comparison to CPUID would be a failure. What I had in mind is that it tries to validate a resctrl interface relevant to non-contiguous CBMs. If it fails there is probably something wrong with the code concerning non-contiguous CBMs.
Wrong with which code? As I understand this particular check compares the resctrl view of the world to the hardware realities. If this check fails then I do not think this is an issue with the test code (which would make it a test error) but instead a resctrl bug and thus a test failure.
I also meant a resctrl bug. I was thinking about the kernel resctrl code that handles taking the CPUID information about non-contiguous CBMs and putting it in the sparse_masks file.
If there was a hardware problem and CPUID returned wrong information, then the check wouldn't fail as sparse_masks relies on CPUID too and both values would match. So in view of this I thought that this check could make sure that the resctrl kernel code handles CPUID returned information properly.
So should this check be moved from the "run the test" phase to the end of the function ("check results" phase) to signify that it's not an error but a failure?
On the other hand writing contiguous CBMs shouldn't fail as far as the non-contiguous CBMs in CAT test is concerned. So if that fails there might be something wrong on a higher level and I'd say that can be more of an error than a failure.
I think that the write_schemata() can fail for a variety of reasons, some may indicate an issue with the test while some may indicate an issue with resctrl. It is not possible for the caller of write_schemata() to distinguish.
But I'm just saying how I undestood it so far. If there is some clear distinction between error and failure definitions I could try to separate it more explicitly.
I do not think it is possible to clearly distinguish between error and failure. These are already lumped together as a ksft_test_result_fail() anyway so no risk of confusion to folks just running the tests. I think the final test result may be confusing to folks parsing the resctrl selftest internals:
run_single_test() { ... ret = test->run_test(test, uparams); ksft_test_result(!ret, "%s: test\n", test->name); ... }
above means that a test returning negative or greater than zero value is considered a test failure and resctrl tests may return either in the case of an actual test failure ... but from user perspective there is no difference so I do not think it is an issue, just lack of consistency in the resctrl test internals in cases like write_schemata() failure where a possible test fail is captured as a test error.
I do not think it is required to be strict here. Keeping "test returns negative or greater than zero on test failure" seems reasonable to me.
Okay, so the approach I applied in noncont_cat_run_test() with write_schemata() is acceptable?
Reinette
Hi Maciej,
On 2/2/2024 2:17 AM, Maciej Wieczor-Retman wrote:
On 2024-02-01 at 11:47:44 -0800, Reinette Chatre wrote:
Hi Maciej,
On 1/31/2024 4:55 AM, Maciej Wieczor-Retman wrote:
On 2024-01-26 at 13:10:18 -0800, Reinette Chatre wrote:
On 1/25/2024 3:13 AM, Maciej Wieczor-Retman wrote:
- if (sparse_masks != ((ecx >> 3) & 1)) {
ksft_print_msg("CPUID output doesn't match 'sparse_masks' file content!\n");return -1;If I understand correctly this falls into the "test failure" [1] category and should return 1? ...
- }
- /* Write checks initialization. */
- ret = get_full_cbm(test->resource, &full_cache_mask);
- if (ret < 0)
return ret;- bit_center = count_bits(full_cache_mask) / 2;
- cont_mask = full_cache_mask >> bit_center;
- /* Contiguous mask write check. */
- snprintf(schemata, sizeof(schemata), "%lx", cont_mask);
- ret = write_schemata("", schemata, uparams->cpu, test->resource);
- if (ret) {
ksft_print_msg("Write of contiguous CBM failed\n");return ret;... although here I think the goal to distinguish between test error and test failure falls apart since it is not possible to tell within the test if the failure is because of error in the test or if test failed.
Is there even a distinction between test error and failure in resctrl selftest?
There is such a distinction in the current tests (and from what I understand the reason behind the logical XOR used in this test) . In existing tests the running of the test precedes and is clearly separate from determining of the test pass/fail. All the current tests have a clear "run the test" phase where data is collected to a file, followed by an analysis (aka "check results") phase that looks at collected data to determine if the test passes or fails. Note how all the "check results" return either 0 or 1 to indicate test pass or fail respectively. Specifically, you can refer to: mbm_test.c->check_results() mba_test.c->check_results() cmt_test.c->check_results() cat_test.c->check_results()
I've been looking at it for a while and can't find any instances where ksft_test_result_error() would be used. Everywhere I look it's either pass or fail. By grep-ing over all selftests I found only five tests that use ksft_test_result_error().
Yes, from the user perspective there is no such distinction. This seems to be entirely internal to the resctrl selftests (but I do not think that this should or can be a hard requirement).
Okay, thank you, that's what I wanted to know.
Furthermore there is this one "TODO" in kselftests.h:
/* TODO: how does "error" differ from "fail" or "skip"? */
If you meant the distintion less literally then I'd say the sparse_masks comparison to CPUID would be a failure. What I had in mind is that it tries to validate a resctrl interface relevant to non-contiguous CBMs. If it fails there is probably something wrong with the code concerning non-contiguous CBMs.
Wrong with which code? As I understand this particular check compares the resctrl view of the world to the hardware realities. If this check fails then I do not think this is an issue with the test code (which would make it a test error) but instead a resctrl bug and thus a test failure.
I also meant a resctrl bug. I was thinking about the kernel resctrl code that handles taking the CPUID information about non-contiguous CBMs and putting it in the sparse_masks file.
If there was a hardware problem and CPUID returned wrong information, then the check wouldn't fail as sparse_masks relies on CPUID too and both values would match. So in view of this I thought that this check could make sure that the resctrl kernel code handles CPUID returned information properly.
So should this check be moved from the "run the test" phase to the end of the function ("check results" phase) to signify that it's not an error but a failure?
I do not think this test matches the "run" and "check" phases of previous tests, unless you create a new test for every scenario checked within this test.
Just returning 1 when the check (if (sparse_masks != ((ecx >> 3) & 1))) fails should be ok, no?
On the other hand writing contiguous CBMs shouldn't fail as far as the non-contiguous CBMs in CAT test is concerned. So if that fails there might be something wrong on a higher level and I'd say that can be more of an error than a failure.
I think that the write_schemata() can fail for a variety of reasons, some may indicate an issue with the test while some may indicate an issue with resctrl. It is not possible for the caller of write_schemata() to distinguish.
But I'm just saying how I undestood it so far. If there is some clear distinction between error and failure definitions I could try to separate it more explicitly.
I do not think it is possible to clearly distinguish between error and failure. These are already lumped together as a ksft_test_result_fail() anyway so no risk of confusion to folks just running the tests. I think the final test result may be confusing to folks parsing the resctrl selftest internals:
run_single_test() { ... ret = test->run_test(test, uparams); ksft_test_result(!ret, "%s: test\n", test->name); ... }
above means that a test returning negative or greater than zero value is considered a test failure and resctrl tests may return either in the case of an actual test failure ... but from user perspective there is no difference so I do not think it is an issue, just lack of consistency in the resctrl test internals in cases like write_schemata() failure where a possible test fail is captured as a test error.
I do not think it is required to be strict here. Keeping "test returns negative or greater than zero on test failure" seems reasonable to me.
Okay, so the approach I applied in noncont_cat_run_test() with write_schemata() is acceptable?
In general I'd say a write_schemata() failure's return code will be acceptable, but you should be consistent in this test. There are two write_schemata() calls in this test, one treats an error return as a failure and the other treats an error return as an error. Considering this inconsistency I would thus rather suggest that you always treat write_schemata() error return as a test failure.
Reinette
linux-kselftest-mirror@lists.linaro.org
-
Ilpo Järvinen -
Maciej Wieczor-Retman -
Reinette Chatre