[PATCH v4 0/5] selftests/resctrl: Add non-contiguous CBMs in Intel CAT selftest

[PATCH v4 0/5] selftests/resctrl: Add non-contiguous CBMs in Intel CAT selftest

[Maciej Wieczor-Retman]

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 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/20231215150515.36983-1-ilpo.jarvinen@linux.intel.com/
[2] https://lore.kernel.org/all/20231211121826.14392-1-ilpo.jarvinen@linux.intel.com/
[3] https://lore.kernel.org/all/cover.1696934091.git.maciej.wieczor-retman@intel.com/

Older versions of this series:
[v1] https://lore.kernel.org/all/20231109112847.432687-1-maciej.wieczor-retman@intel.com/
[v2] https://lore.kernel.org/all/cover.1702392177.git.maciej.wieczor-retman@intel.com/

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    |  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, 192 insertions(+), 24 deletions(-)

-- 
2.43.0

[PATCH v4 1/5] selftests/resctrl: Add test groups and name L3 CAT test L3_CAT

[Maciej Wieczor-Retman]

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;
-- 
2.43.0

[PATCH v4 2/5] selftests/resctrl: Add helpers for the non-contiguous test

[Maciej Wieczor-Retman]

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 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..e0fbc46a917a 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 the @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 in opening %s\n: %m\n", file_path);
+		return -1;
+	}
+
+	if (fscanf(fp, "%u", val) <= 0) {
+		ksft_print_msg("Could not get contents of %s\n: %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

[PATCH v4 3/5] selftests/resctrl: Split validate_resctrl_feature_request()

[Maciej Wieczor-Retman]

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 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   |  3 +-
 tools/testing/selftests/resctrl/resctrlfs.c | 33 +++++++++++++--------
 5 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index dd5ca343c469..c1157917a814 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_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..fa99a91c8ab7 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("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 e0fbc46a917a..0cfec8bb23fd 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,11 +732,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;
 
@@ -753,7 +762,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

[PATCH v4 4/5] selftests/resctrl: Add resource_info_file_exists()

[Maciej Wieczor-Retman]

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>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
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/resctrl.h   |  1 +
 tools/testing/selftests/resctrl/resctrlfs.c | 25 +++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index 4603b215b97e..2b9a3d0570c7 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -138,6 +138,7 @@ 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 0cfec8bb23fd..6a3082ca58b5 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -760,6 +760,31 @@ 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.)
+ * @file:	Required file.
+ *
+ * Return: True if the 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

[PATCH v4 5/5] selftests/resctrl: Add non-contiguous CBMs CAT test

[Maciej Wieczor-Retman]

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 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..20eb978e624b 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 = ~(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 2b9a3d0570c7..9e834496401c 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

Re: [PATCH v4 3/5] selftests/resctrl: Split validate_resctrl_feature_request()

[Ilpo Järvinen]

On Mon, 5 Feb 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 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   |  3 +-
>  tools/testing/selftests/resctrl/resctrlfs.c | 33 +++++++++++++--------
>  5 files changed, 28 insertions(+), 18 deletions(-)
> 

> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
> index dd5ca343c469..c1157917a814 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_resource_exists("L3");

This not correctly transformed.

> +/*
> + * 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");

This became less generic? Could there be other MON resource besides L3 
one? Perhaps there aren't today but why remove the ability give it as a 
parameter?


-- 
 i.

Re: [PATCH v4 5/5] selftests/resctrl: Add non-contiguous CBMs CAT test

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3817 bytes --]

On Mon, 5 Feb 2024, 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 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..20eb978e624b 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 = ~(0xf << (bit_center - 2)) & full_cache_mask;

To be on the safe side, I think the types could be made to match here 
with 0xfUL to avoid sizeof(int) vs sizeof(unsigned long) related bit 
drops in the & (although it feel somewhat theoretical given the bitmask 
sizes we are currently seeing).

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>


-- 
 i.

Re: [PATCH v4 2/5] selftests/resctrl: Add helpers for the non-contiguous test

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3366 bytes --]

On Mon, 5 Feb 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 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..e0fbc46a917a 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 the @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 in opening %s\n: %m\n", file_path);

Adding the extra \n in between there will likely mess up the test output 
formatting (the expected prefixes will not appear). Therefore, manually 
adding newlines should be avoided.

> +		return -1;
> +	}
> +
> +	if (fscanf(fp, "%u", val) <= 0) {
> +		ksft_print_msg("Could not get contents of %s\n: %m\n", file_path);

Ditto.

With those two fixed,

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>


-- 
 i.

Re: [PATCH v4 3/5] selftests/resctrl: Split validate_resctrl_feature_request()

[Maciej Wieczor-Retman]

On 2024-02-05 at 14:41:30 +0200, Ilpo Järvinen wrote:
>On Mon, 5 Feb 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 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   |  3 +-
>>  tools/testing/selftests/resctrl/resctrlfs.c | 33 +++++++++++++--------
>>  5 files changed, 28 insertions(+), 18 deletions(-)
>> 
>
>> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
>> index dd5ca343c469..c1157917a814 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_resource_exists("L3");
>
>This not correctly transformed.

Oops, sorry, I'll fix it for the next version.

>
>> +/*
>> + * 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");
>
>This became less generic? Could there be other MON resource besides L3 
>one? Perhaps there aren't today but why remove the ability give it as a 
>parameter?

During v2 discussion [1] Reinette made me realize this functionality only
interfaces with L3_MON/mon_features file and the 'resource' parameter isn't
needed. The 'mon_features' file is only mentioned for L3_MON and I don't know of
any plans for other MON resources so I assumed it doesn't need to be generic.

But sure, I can make it use a parameter if Reinette doesn't mind.

[1] https://lore.kernel.org/all/2o7adr2cos6qcikcu7oop4ss7vib2n6ue33djgfeds3v6gj53f@uu45lomrp5qv/

>
>
>-- 
> i.
>

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH v4 5/5] selftests/resctrl: Add non-contiguous CBMs CAT test

[Maciej Wieczor-Retman]

On 2024-02-05 at 15:11:22 +0200, Ilpo Järvinen wrote:
>On Mon, 5 Feb 2024, 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 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..20eb978e624b 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 = ~(0xf << (bit_center - 2)) & full_cache_mask;
>
>To be on the safe side, I think the types could be made to match here 
>with 0xfUL to avoid sizeof(int) vs sizeof(unsigned long) related bit 
>drops in the & (although it feel somewhat theoretical given the bitmask 
>sizes we are currently seeing).

Sure, I'll add that for the next version. Thanks!

>
>Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>
>
>-- 
> i.


-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH v4 2/5] selftests/resctrl: Add helpers for the non-contiguous test

[Maciej Wieczor-Retman]

On 2024-02-05 at 15:16:27 +0200, Ilpo Järvinen wrote:
>On Mon, 5 Feb 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 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..e0fbc46a917a 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 the @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 in opening %s\n: %m\n", file_path);
>
>Adding the extra \n in between there will likely mess up the test output 
>formatting (the expected prefixes will not appear). Therefore, manually 
>adding newlines should be avoided.
>
>> +		return -1;
>> +	}
>> +
>> +	if (fscanf(fp, "%u", val) <= 0) {
>> +		ksft_print_msg("Could not get contents of %s\n: %m\n", file_path);
>
>Ditto.

I thought I've seen some other error checking paths where '%m' was on the next
line but I couldn't find it again right now so I'll remove the newline.

>
>With those two fixed,
>
>Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>
>
>-- 
> i.


-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH v4 1/5] selftests/resctrl: Add test groups and name L3 CAT test L3_CAT

[Reinette Chatre]

Hi Maciej,

On 2/5/2024 4:07 AM, Maciej Wieczor-Retman wrote:
> 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>
> ---

Thank you.

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

Re: [PATCH v4 2/5] selftests/resctrl: Add helpers for the non-contiguous test

[Reinette Chatre]

Hi Maciej,

The subject mentions "helpers" (plural) that may not be accurate
anymore.

On 2/5/2024 4:08 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>.

The "a file in" above can be dropped or it should read "from a file in
/sys/fs/resctrl/info/<RESOURCE>".

> 
> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> ---
> 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..e0fbc46a917a 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;
>  }
>  
> +/**

Apologies for not being clear in my previous comment. I do not
think we want to start exposing resctrl selftest comments to the
kernel-doc tool using this change. The tests are using kernel-doc
style but this is done outside from the kernel-doc tool's view.

> + * 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 the @val.

It can just be "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 in opening %s\n: %m\n", file_path);
> +		return -1;
> +	}

Apart from Ilpo's comment this can also just be "Error opening %s: ..."

> +
> +	if (fscanf(fp, "%u", val) <= 0) {
> +		ksft_print_msg("Could not get contents of %s\n: %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


Reinette

Re: [PATCH v4 3/5] selftests/resctrl: Split validate_resctrl_feature_request()

[Reinette Chatre]

On 2/5/2024 5:24 AM, Maciej Wieczor-Retman wrote:
> On 2024-02-05 at 14:41:30 +0200, Ilpo Järvinen wrote:
>> On Mon, 5 Feb 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 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   |  3 +-
>>>  tools/testing/selftests/resctrl/resctrlfs.c | 33 +++++++++++++--------
>>>  5 files changed, 28 insertions(+), 18 deletions(-)
>>>
>>
>>> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
>>> index dd5ca343c469..c1157917a814 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_resource_exists("L3");
>>
>> This not correctly transformed.
> 
> Oops, sorry, I'll fix it for the next version.
> 
>>
>>> +/*
>>> + * 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");
>>
>> This became less generic? Could there be other MON resource besides L3 
>> one? Perhaps there aren't today but why remove the ability give it as a 
>> parameter?

This does make the function less generic but the benefit is that four copies of
the same hardcoded parameter is no longer needed. In my opinion this patch thus
makes the code cleaner but it is not a requirement that I will use to hold this
series back.

> During v2 discussion [1] Reinette made me realize this functionality only
> interfaces with L3_MON/mon_features file and the 'resource' parameter isn't
> needed. The 'mon_features' file is only mentioned for L3_MON and I don't know of
> any plans for other MON resources so I assumed it doesn't need to be generic.
> 
> But sure, I can make it use a parameter if Reinette doesn't mind.

I prefer what is in this patch, but I will not object if the function
is changed to take the resource as parameter.

> 
> [1] https://lore.kernel.org/all/2o7adr2cos6qcikcu7oop4ss7vib2n6ue33djgfeds3v6gj53f@uu45lomrp5qv/
> 

Reinette

Re: [PATCH v4 4/5] selftests/resctrl: Add resource_info_file_exists()

[Reinette Chatre]

Hi Maciej,

On 2/5/2024 4:08 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>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> 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/resctrl.h   |  1 +
>  tools/testing/selftests/resctrl/resctrlfs.c | 25 +++++++++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index 4603b215b97e..2b9a3d0570c7 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -138,6 +138,7 @@ 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);

One stray "feature" usage.

>  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 0cfec8bb23fd..6a3082ca58b5 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -760,6 +760,31 @@ 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.)
> + * @file:	Required file.
> + *
> + * Return: True if the file exists, else false.

How about "True if /sys/fs/resctrl/info/@resource/@file exists, else false"?

Reinette

Re: [PATCH v4 5/5] selftests/resctrl: Add non-contiguous CBMs CAT test

[Reinette Chatre]

Hi Maciej,

On 2/5/2024 4:08 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>
> ---

Thank you very much.

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

Re: [PATCH v4 4/5] selftests/resctrl: Add resource_info_file_exists()

[Maciej Wieczor-Retman]

Hi Reinette!

On 2024-02-05 at 20:17:48 -0800, Reinette Chatre wrote:
>Hi Maciej,
>
>On 2/5/2024 4:08 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>
>> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>> ---
>> 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/resctrl.h   |  1 +
>>  tools/testing/selftests/resctrl/resctrlfs.c | 25 +++++++++++++++++++++
>>  2 files changed, 26 insertions(+)
>> 
>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
>> index 4603b215b97e..2b9a3d0570c7 100644
>> --- a/tools/testing/selftests/resctrl/resctrl.h
>> +++ b/tools/testing/selftests/resctrl/resctrl.h
>> @@ -138,6 +138,7 @@ 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);
>
>One stray "feature" usage.

Thank you for catching that!

>
>>  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 0cfec8bb23fd..6a3082ca58b5 100644
>> --- a/tools/testing/selftests/resctrl/resctrlfs.c
>> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
>> @@ -760,6 +760,31 @@ 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.)
>> + * @file:	Required file.
>> + *
>> + * Return: True if the file exists, else false.
>
>How about "True if /sys/fs/resctrl/info/@resource/@file exists, else false"?

Sure. I was wondering what the best format of paths in the function comments
could be and that does look very sensible. I'll redo the other paths in the
comments of this series for consistency.

>
>Reinette

-- 
Kind regards
Maciej Wieczór-Retman