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 v5:
- Add a few reviewed-by tags.
- Make some minor text changes in patch messages and comments.
- Redo resctrl_mon_feature_exists() to be more generic and fix some of
my errors in refactoring feature checking.
Changelog v4:
- Changes to error failure return values in non-contiguous test.
- Some minor text refactoring without functional changes.
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/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/
[3] https://lore.kernel.org/all/[email protected]/
Older versions of this series:
[v1] https://lore.kernel.org/all/[email protected]/
[v2] https://lore.kernel.org/all/[email protected]/
[v3] https://lore.kernel.org/all/[email protected]/
[v4] https://lore.kernel.org/all/[email protected]/
Ilpo Järvinen (1):
selftests/resctrl: Add test groups and name L3 CAT test L3_CAT
Maciej Wieczor-Retman (4):
selftests/resctrl: Add a helper 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 | 2 +-
tools/testing/selftests/resctrl/mba_test.c | 2 +-
tools/testing/selftests/resctrl/mbm_test.c | 6 +-
tools/testing/selftests/resctrl/resctrl.h | 10 +-
.../testing/selftests/resctrl/resctrl_tests.c | 18 +++-
tools/testing/selftests/resctrl/resctrlfs.c | 94 +++++++++++++++++--
7 files changed, 194 insertions(+), 22 deletions(-)
--
2.43.0
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
/sys/fs/resctrl/info/<RESOURCE>/<FILE>.
Signed-off-by: Maciej Wieczor-Retman <[email protected]>
Reviewed-by: Ilpo Järvinen <[email protected]>
---
Changelog v5:
- Remove '\n' from ksft_print_msg() calls. (Ilpo)
- Add Ilpo's reviewed-by tag.
- Change 'helpers' -> 'a helper' in the subject.
- Change 'a file in' -> '' in patch message.
- Remove '*' from resource_info_file_exists()'s comment.
- 'saved into the @val' -> 'saved into @val'
- 'Error in opening' -> 'Error opening'
- Redo path in function comment.
Changelog v4:
- Rewrite function comment.
- Redo ksft_perror() as ksft_print_msg(). (Reinette)
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 | 36 +++++++++++++++++++++
2 files changed, 37 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..8a183c73bc23 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -249,6 +249,42 @@ static int get_bit_mask(const char *filename, unsigned long *mask)
return 0;
}
+/*
+ * resource_info_unsigned_get - Read an unsigned value from
+ * /sys/fs/resctrl/info/@resource/@filename
+ * @resource: Resource name that matches directory name in
+ * /sys/fs/resctrl/info
+ * @filename: File in /sys/fs/resctrl/info/@resource
+ * @val: Contains read value on success.
+ *
+ * Return: = 0 on success, < 0 on failure. On success the read
+ * value is saved into @val.
+ */
+int resource_info_unsigned_get(const char *resource, const char *filename,
+ unsigned int *val)
+{
+ char 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) {
+ ksft_print_msg("Error opening %s: %m\n", file_path);
+ return -1;
+ }
+
+ if (fscanf(fp, "%u", val) <= 0) {
+ ksft_print_msg("Could not get contents of %s: %m\n", file_path);
+ fclose(fp);
+ return -1;
+ }
+
+ fclose(fp);
+ return 0;
+}
+
/*
* create_bit_mask- Create bit mask from start, len pair
* @start: LSB of the mask
--
2.43.0
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/<RESOURCE>/mon_features
file.
Signed-off-by: Maciej Wieczor-Retman <[email protected]>
---
Changelog v5:
- Move back to using resource parameter in mon_feature handling
function. (Ilpo)
Changelog v4:
- Roll back to using test_resource_feature_check() for CMT and MBA.
(Ilpo).
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 | 2 +-
tools/testing/selftests/resctrl/mba_test.c | 2 +-
tools/testing/selftests/resctrl/mbm_test.c | 6 ++--
tools/testing/selftests/resctrl/resctrl.h | 4 ++-
tools/testing/selftests/resctrl/resctrlfs.c | 33 ++++++++++++++-------
5 files changed, 31 insertions(+), 16 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index dd5ca343c469..a81f91222a89 100644
--- a/tools/testing/selftests/resctrl/cmt_test.c
+++ b/tools/testing/selftests/resctrl/cmt_test.c
@@ -170,7 +170,7 @@ 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");
+ resctrl_mon_feature_exists("L3_MON", "llc_occupancy");
}
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..7946e32e85c8 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -171,7 +171,7 @@ 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");
+ resctrl_mon_feature_exists("L3_MON", "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..d67ffa3ec63a 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("L3_MON", "mbm_total_bytes") &&
+ resctrl_mon_feature_exists("L3_MON", "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..f434a6543b4f 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -136,7 +136,9 @@ 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 *resource, const char *feature);
+bool resource_info_file_exists(const char *resource, const char *file);
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 8a183c73bc23..af4f800ab23d 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -708,20 +708,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)
@@ -736,8 +732,25 @@ 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 feature is valid.
+ * @resource: Resource that uses the mon_features file. Currently only L3_MON
+ * 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 *resource, const char *feature)
+{
+ char res_path[PATH_MAX];
+ 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");
@@ -753,7 +766,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)
--
2.43.0
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 <[email protected]>
Reviewed-by: Ilpo Järvinen <[email protected]>
---
Changelog v5:
- feature -> file. (Reinette)
- Redo return description and path in the comment. (Reinette)
Changelog v4:
- Remove unnecessary new lines.
- Change 'feature' -> 'file' to keep things generic. (Reinette)
- Add Ilpo's reviewed-by tag.
Changelog v3:
- Split off the new function into this patch. (Reinette)
Changelog v2:
- Add this patch.
tools/testing/selftests/resctrl/resctrlfs.c | 25 +++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index af4f800ab23d..0e930fc06ab5 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -764,6 +764,31 @@ bool resctrl_mon_feature_exists(const char *resource, 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.)
+ * @file: Required file.
+ *
+ * Return: True if the /sys/fs/resctrl/info/@resource/@file exists, else false.
+ */
+bool resource_info_file_exists(const char *resource, const char *file)
+{
+ char res_path[PATH_MAX];
+ struct stat statbuf;
+
+ if (!file || !resource)
+ return false;
+
+ snprintf(res_path, sizeof(res_path), "%s/%s/%s", INFO_PATH, resource,
+ file);
+
+ 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);
--
2.43.0
From: Ilpo Järvinen <[email protected]>
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 <[email protected]>
Signed-off-by: Maciej Wieczor-Retman <[email protected]>
Reviewed-by: Reinette Chatre <[email protected]>
---
Changelog v5:
- Add Reinette's reviewed-by tag.
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;
--
2.43.0
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 <[email protected]>
Reviewed-by: Reinette Chatre <[email protected]>
---
Changelog v5:
- Add Reinette's reviewed-by tag.
- Make 0xf UL in case the CBMs get bigger in the future. (Ilpo)
Changelog v4:
- Return failure instead of error on check of cpuid against sparse_masks
and on contiguous write_schemata fail. (Reinette)
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..d4b7bf8a6780 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 1;
+ }
+
+ /*
+ * 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 = ~(0xfUL << (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 f434a6543b4f..2051bd135e0d 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -209,5 +209,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)
--
2.43.0
Hi Maciej,
On 2/9/2024 6:02 AM, Maciej Wieczor-Retman wrote:
..
> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
> index 39fc9303b8e8..d4b7bf8a6780 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;
I missed that "ret" is "unsigned int" while the test expects it to
be signed ("if (ret < 0)") and it is used to have return value of functions
that return < 0 on error.
> + 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;
It would be nice if no new static check issues are introduced into the
resctrl selftests. I did a quick check and this is a problematic portion.
We know that the full_cache_mask cannot have zero bits but it is not
obvious to the checkers, thus thinking that bit_center may be zero
resulting in a bit shift of "-2" bits attempt later on. Could you please
add some error checking to ensure expected values to avoid extra noise from
checkers when this code lands upstream?
Thank you
Reinette
Hi Maciej,
On 2/9/2024 6:01 AM, Maciej Wieczor-Retman wrote:
..
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index 5116ea082d03..f434a6543b4f 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -136,7 +136,9 @@ 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 *resource, const char *feature);
> +bool resource_info_file_exists(const char *resource, const char *file);
Portion of next patch sneaked in here.
> 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 8a183c73bc23..af4f800ab23d 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -708,20 +708,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)
> @@ -736,8 +732,25 @@ 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 feature is valid.
> + * @resource: Resource that uses the mon_features file. Currently only L3_MON
> + * 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 *resource, const char *feature)
> +{
> + char res_path[PATH_MAX];
> + char *res;
> + FILE *inf;
> +
> if (!feature)
Now that resource is a new parameter should it be checked also? It is not obvious
why only one parameter is checked.
Reinette
Hi Maciej,
On 2/9/2024 6:01 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
> /sys/fs/resctrl/info/<RESOURCE>/<FILE>.
>
> Signed-off-by: Maciej Wieczor-Retman <[email protected]>
> Reviewed-by: Ilpo Järvinen <[email protected]>
> ---
Thank you.
Reviewed-by: Reinette Chatre <[email protected]>
Reinette
Hello!
On 2024-02-09 at 09:21:16 -0800, Reinette Chatre wrote:
>Hi Maciej,
>
>On 2/9/2024 6:02 AM, Maciej Wieczor-Retman wrote:
>
>...
>
>> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
>> index 39fc9303b8e8..d4b7bf8a6780 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;
>
>I missed that "ret" is "unsigned int" while the test expects it to
>be signed ("if (ret < 0)") and it is used to have return value of functions
>that return < 0 on error.
>
Oh, sorry, I'll fix that.
>
>> + 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;
>
>It would be nice if no new static check issues are introduced into the
>resctrl selftests. I did a quick check and this is a problematic portion.
>We know that the full_cache_mask cannot have zero bits but it is not
>obvious to the checkers, thus thinking that bit_center may be zero
>resulting in a bit shift of "-2" bits attempt later on. Could you please
>add some error checking to ensure expected values to avoid extra noise from
>checkers when this code lands upstream?
>
>Thank you
Sure, I guess I could make the check 'if (bit_center < 3)' to also check if the
full_cache_mask isn't too short for some reason (since later 2 is substracted
from bit_center for the 'hole' bit shift).
Or would this need some comment as well (why exactly the '3' is there, maybe
write something about about the minimal full_cache_mask length for this test)?
>
>Reinette
>
--
Kind regards
Maciej Wiecz?r-Retman
Hi,
On 2024-02-09 at 09:20:27 -0800, Reinette Chatre wrote:
>Hi Maciej,
>
>On 2/9/2024 6:01 AM, Maciej Wieczor-Retman wrote:
>
>...
>
>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
>> index 5116ea082d03..f434a6543b4f 100644
>> --- a/tools/testing/selftests/resctrl/resctrl.h
>> +++ b/tools/testing/selftests/resctrl/resctrl.h
>> @@ -136,7 +136,9 @@ 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 *resource, const char *feature);
>> +bool resource_info_file_exists(const char *resource, const char *file);
>
>Portion of next patch sneaked in here.
Missed it, thanks for pointing that out!
>
>> 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 8a183c73bc23..af4f800ab23d 100644
>> --- a/tools/testing/selftests/resctrl/resctrlfs.c
>> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
>> @@ -708,20 +708,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)
>> @@ -736,8 +732,25 @@ 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 feature is valid.
>> + * @resource: Resource that uses the mon_features file. Currently only L3_MON
>> + * 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 *resource, const char *feature)
>> +{
>> + char res_path[PATH_MAX];
>> + char *res;
>> + FILE *inf;
>> +
>> if (!feature)
>
>Now that resource is a new parameter should it be checked also? It is not obvious
>why only one parameter is checked.
Right, I'll add the check there.
>
>Reinette
--
Kind regards
Maciej Wiecz?r-Retman
Hi Maciej,
On 2/11/2024 11:38 PM, Maciej Wieczor-Retman wrote:
> Sure, I guess I could make the check 'if (bit_center < 3)' to also check if the
> full_cache_mask isn't too short for some reason (since later 2 is substracted
> from bit_center for the 'hole' bit shift).
Thank you.
> Or would this need some comment as well (why exactly the '3' is there, maybe
> write something about about the minimal full_cache_mask length for this test)?
The use of "Or" and "as well" makes it unclear what you propose. I do
think the check in addition to a comment will be helpful.
Thank you.
Reinette