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

[PATCH v3 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 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    |  4 +-
 tools/testing/selftests/resctrl/mba_test.c    |  4 +-
 tools/testing/selftests/resctrl/mbm_test.c    |  6 +-
 tools/testing/selftests/resctrl/resctrl.h     | 11 ++-
 .../testing/selftests/resctrl/resctrl_tests.c | 18 +++-
 tools/testing/selftests/resctrl/resctrlfs.c   | 98 ++++++++++++++++---
 7 files changed, 199 insertions(+), 26 deletions(-)

-- 
2.43.0

[PATCH v3 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 v3 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 v3:
- Rewrite patch message.
- Add documentation and rewrote the function. (Reinette)

Changelog v2:
- Add this patch.

 tools/testing/selftests/resctrl/resctrl.h   |  1 +
 tools/testing/selftests/resctrl/resctrlfs.c | 39 +++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index a1462029998e..5116ea082d03 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -162,6 +162,7 @@ unsigned int count_contiguous_bits(unsigned long val, unsigned int *start);
 int get_full_cbm(const char *cache_type, unsigned long *mask);
 int get_mask_no_shareable(const char *cache_type, unsigned long *mask);
 int get_cache_size(int cpu_no, const char *cache_type, unsigned long *cache_size);
+int resource_info_unsigned_get(const char *resource, const char *filename, unsigned int *val);
 void ctrlc_handler(int signum, siginfo_t *info, void *ptr);
 int signal_handler_register(void);
 void signal_handler_unregister(void);
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index 5750662cce57..cb5147c5f9a9 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -249,6 +249,45 @@ static int get_bit_mask(const char *filename, unsigned long *mask)
 	return 0;
 }
 
+/*
+ * resource_info_unsigned_get - Read an unsigned value from a file in
+ * /sys/fs/resctrl/info/RESOURCE/FILENAME
+ * @resource:	Resource name that matches directory names in
+ *		/sys/fs/resctrl/info
+ * @filename:	Filename of a file located in a directory specified with the
+ *		'resource' variable.
+ * @val:	Variable where the read value is saved on success.
+ *
+ * Return: = 0 on success, < 0 on failure. On success the read value is saved into the 'val'
+ * variable.
+ */
+int resource_info_unsigned_get(const char *resource, const char *filename,
+			       unsigned int *val)
+{
+	char reason[128], file_path[PATH_MAX];
+	FILE *fp;
+
+	snprintf(file_path, sizeof(file_path), "%s/%s/%s", INFO_PATH, resource,
+		 filename);
+
+	fp = fopen(file_path, "r");
+	if (!fp) {
+		snprintf(reason, sizeof(reason), "Error in opening %s file\n", filename);
+		ksft_perror(reason);
+		return -1;
+	}
+
+	if (fscanf(fp, "%u", val) <= 0) {
+		snprintf(reason, sizeof(reason), "Could not get %s's contents\n", filename);
+		ksft_perror(reason);
+		fclose(fp);
+		return -1;
+	}
+
+	fclose(fp);
+	return 0;
+}
+
 /*
  * create_bit_mask- Create bit mask from start, len pair
  * @start:	LSB of the mask
-- 
2.43.0

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

[Ilpo Järvinen]

On Thu, 25 Jan 2024, Maciej Wieczor-Retman wrote:

> The CAT non-contiguous selftests have to read the file responsible for
> reporting support of non-contiguous CBMs in kernel (resctrl). Then the
> test compares if that information matches what is reported by CPUID
> output.
> 
> Add a generic helper function to read an unsigned number from a file in
> /sys/fs/resctrl/info/<RESOURCE>/<FILE>.
> 
> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> ---
> Changelog v3:
> - Rewrite patch message.
> - Add documentation and rewrote the function. (Reinette)
> 
> Changelog v2:
> - Add this patch.
> 
>  tools/testing/selftests/resctrl/resctrl.h   |  1 +
>  tools/testing/selftests/resctrl/resctrlfs.c | 39 +++++++++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index a1462029998e..5116ea082d03 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -162,6 +162,7 @@ unsigned int count_contiguous_bits(unsigned long val, unsigned int *start);
>  int get_full_cbm(const char *cache_type, unsigned long *mask);
>  int get_mask_no_shareable(const char *cache_type, unsigned long *mask);
>  int get_cache_size(int cpu_no, const char *cache_type, unsigned long *cache_size);
> +int resource_info_unsigned_get(const char *resource, const char *filename, unsigned int *val);
>  void ctrlc_handler(int signum, siginfo_t *info, void *ptr);
>  int signal_handler_register(void);
>  void signal_handler_unregister(void);
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index 5750662cce57..cb5147c5f9a9 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -249,6 +249,45 @@ static int get_bit_mask(const char *filename, unsigned long *mask)
>  	return 0;
>  }
>  
> +/*
> + * resource_info_unsigned_get - Read an unsigned value from a file in
> + * /sys/fs/resctrl/info/RESOURCE/FILENAME
> + * @resource:	Resource name that matches directory names in
> + *		/sys/fs/resctrl/info
> + * @filename:	Filename of a file located in a directory specified with the
> + *		'resource' variable.
> + * @val:	Variable where the read value is saved on success.
> + *
> + * Return: = 0 on success, < 0 on failure. On success the read value is saved into the 'val'
> + * variable.
> + */
> +int resource_info_unsigned_get(const char *resource, const char *filename,
> +			       unsigned int *val)
> +{
> +	char reason[128], file_path[PATH_MAX];
> +	FILE *fp;
> +
> +	snprintf(file_path, sizeof(file_path), "%s/%s/%s", INFO_PATH, resource,
> +		 filename);
> +
> +	fp = fopen(file_path, "r");
> +	if (!fp) {
> +		snprintf(reason, sizeof(reason), "Error in opening %s file\n", filename);
> +		ksft_perror(reason);

Was this the conclusion of the kstf_perror() discussion with Reinette? I 
expected a bit different outcome when I stopped following it...

In any case, it would be nice though if ksft_perror() (or some kselftest.h 
function yet to be added with a different name) would accept full printf 
interface and just add the errno string into the end of the string so one 
would not need to build constructs like this at all.

It will require a bit of macro trickery into kselftest.h. I don't know how 
it should handle the case where somebody just passes a char pointer to it, 
not a string literal, but I guess it would just throw an error while 
compiling if somebody tries to do that as the macro string literal 
concatenation could not build useful/compilable token.

It would make these prints informative enough to become actually useful 
without needed to resort to preparing the string in advance which seems
to be required almost every single case with the current interface.

> +		return -1;
> +	}
> +
> +	if (fscanf(fp, "%u", val) <= 0) {
> +		snprintf(reason, sizeof(reason), "Could not get %s's contents\n", filename);
> +		ksft_perror(reason);
> +		fclose(fp);
> +		return -1;
> +	}
> +
> +	fclose(fp);
> +	return 0;
> +}
> +
>  /*
>   * create_bit_mask- Create bit mask from start, len pair
>   * @start:	LSB of the mask
> 

-- 
 i.

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

[Reinette Chatre]

On 1/25/2024 4:14 AM, Ilpo Järvinen wrote:
> On Thu, 25 Jan 2024, Maciej Wieczor-Retman wrote:


>> +	fp = fopen(file_path, "r");
>> +	if (!fp) {
>> +		snprintf(reason, sizeof(reason), "Error in opening %s file\n", filename);
>> +		ksft_perror(reason);
> 
> Was this the conclusion of the kstf_perror() discussion with Reinette? I 
> expected a bit different outcome when I stopped following it...
> 
> In any case, it would be nice though if ksft_perror() (or some kselftest.h 
> function yet to be added with a different name) would accept full printf 
> interface and just add the errno string into the end of the string so one 
> would not need to build constructs like this at all.
> 
> It will require a bit of macro trickery into kselftest.h. I don't know how 
> it should handle the case where somebody just passes a char pointer to it, 
> not a string literal, but I guess it would just throw an error while 
> compiling if somebody tries to do that as the macro string literal 
> concatenation could not build useful/compilable token.
> 
> It would make these prints informative enough to become actually useful 
> without needed to resort to preparing the string in advance which seems
> to be required almost every single case with the current interface.

I think this can be accomplished with a new:
	void  ksft_vprint_msg(const char *msg, va_list args)

... but ksft_perror() does conform to perror() and I expect that having one
support variable number of arguments while the other does to cause confusion.

To support variable number of arguments with errno I'd propose just to use
ksft_print_msg() with strerror(errno), errno as the arguments (or even %m
that that errno handling within ksft_print_msg() aims to support). This does
indeed seem to be the custom in other tests.

Reinette

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

[Maciej Wieczor-Retman]

Hi,

On 2024-01-26 at 10:58:04 -0800, Reinette Chatre wrote:
>
>
>On 1/25/2024 4:14 AM, Ilpo Järvinen wrote:
>> On Thu, 25 Jan 2024, Maciej Wieczor-Retman wrote:
>
>
>>> +	fp = fopen(file_path, "r");
>>> +	if (!fp) {
>>> +		snprintf(reason, sizeof(reason), "Error in opening %s file\n", filename);
>>> +		ksft_perror(reason);
>> 
>> Was this the conclusion of the kstf_perror() discussion with Reinette? I 
>> expected a bit different outcome when I stopped following it...
>> 
>> In any case, it would be nice though if ksft_perror() (or some kselftest.h 
>> function yet to be added with a different name) would accept full printf 
>> interface and just add the errno string into the end of the string so one 
>> would not need to build constructs like this at all.
>> 
>> It will require a bit of macro trickery into kselftest.h. I don't know how 
>> it should handle the case where somebody just passes a char pointer to it, 
>> not a string literal, but I guess it would just throw an error while 
>> compiling if somebody tries to do that as the macro string literal 
>> concatenation could not build useful/compilable token.
>> 
>> It would make these prints informative enough to become actually useful 
>> without needed to resort to preparing the string in advance which seems
>> to be required almost every single case with the current interface.
>
>I think this can be accomplished with a new:
>	void  ksft_vprint_msg(const char *msg, va_list args)
>
>... but ksft_perror() does conform to perror() and I expect that having one
>support variable number of arguments while the other does to cause confusion.
>
>To support variable number of arguments with errno I'd propose just to use
>ksft_print_msg() with strerror(errno), errno as the arguments (or even %m
>that that errno handling within ksft_print_msg() aims to support). This does
>indeed seem to be the custom in other tests.

Does something like this look okay?

	fp = fopen(file_path, "r");
	if (!fp) {
		ksft_print_msg("Error in opening %s\n: %m\n", file_path);
		return -1;
	}

The '%m' seems to work fine but doesn't print errno's number code. Do you want
me to add errno after '%m' so it is the same as ksft_perror()? I looked through
some other tests where '%m' is used, and only few ones add errno with '%d'.

>Reinette

-- 
Kind regards
Maciej Wieczór-Retman

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

[Ilpo Järvinen]

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

On Wed, 31 Jan 2024, Maciej Wieczor-Retman wrote:
> On 2024-01-26 at 10:58:04 -0800, Reinette Chatre wrote:
> >On 1/25/2024 4:14 AM, Ilpo Järvinen wrote:
> >> On Thu, 25 Jan 2024, Maciej Wieczor-Retman wrote:
> >
> >>> +	fp = fopen(file_path, "r");
> >>> +	if (!fp) {
> >>> +		snprintf(reason, sizeof(reason), "Error in opening %s file\n", filename);
> >>> +		ksft_perror(reason);
> >> 
> >> Was this the conclusion of the kstf_perror() discussion with Reinette? I 
> >> expected a bit different outcome when I stopped following it...
> >> 
> >> In any case, it would be nice though if ksft_perror() (or some kselftest.h 
> >> function yet to be added with a different name) would accept full printf 
> >> interface and just add the errno string into the end of the string so one 
> >> would not need to build constructs like this at all.
> >> 
> >> It will require a bit of macro trickery into kselftest.h. I don't know how 
> >> it should handle the case where somebody just passes a char pointer to it, 
> >> not a string literal, but I guess it would just throw an error while 
> >> compiling if somebody tries to do that as the macro string literal 
> >> concatenation could not build useful/compilable token.
> >> 
> >> It would make these prints informative enough to become actually useful 
> >> without needed to resort to preparing the string in advance which seems
> >> to be required almost every single case with the current interface.
> >
> >I think this can be accomplished with a new:
> >	void  ksft_vprint_msg(const char *msg, va_list args)
> >
> >... but ksft_perror() does conform to perror() and I expect that having one
> >support variable number of arguments while the other does to cause confusion.
> >
> >To support variable number of arguments with errno I'd propose just to use
> >ksft_print_msg() with strerror(errno), errno as the arguments (or even %m
> >that that errno handling within ksft_print_msg() aims to support). This does
> >indeed seem to be the custom in other tests.
> 
> Does something like this look okay?
> 
> 	fp = fopen(file_path, "r");
> 	if (!fp) {
> 		ksft_print_msg("Error in opening %s\n: %m\n", file_path);
> 		return -1;
> 	}
> 
> The '%m' seems to work fine but doesn't print errno's number code. Do you want
> me to add errno after '%m' so it is the same as ksft_perror()? I looked through
> some other tests where '%m' is used, and only few ones add errno with '%d'.

I think %m is enough.

-- 
 i.

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

[Reinette Chatre]

Hi Maciej,

On 1/25/2024 3:10 AM, Maciej Wieczor-Retman wrote:
> The CAT non-contiguous selftests have to read the file responsible for
> reporting support of non-contiguous CBMs in kernel (resctrl). Then the
> test compares if that information matches what is reported by CPUID
> output.
> 
> Add a generic helper function to read an unsigned number from a file in
> /sys/fs/resctrl/info/<RESOURCE>/<FILE>.
> 
> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> ---
> Changelog v3:
> - Rewrite patch message.
> - Add documentation and rewrote the function. (Reinette)
> 
> Changelog v2:
> - Add this patch.
> 
>  tools/testing/selftests/resctrl/resctrl.h   |  1 +
>  tools/testing/selftests/resctrl/resctrlfs.c | 39 +++++++++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index a1462029998e..5116ea082d03 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -162,6 +162,7 @@ unsigned int count_contiguous_bits(unsigned long val, unsigned int *start);
>  int get_full_cbm(const char *cache_type, unsigned long *mask);
>  int get_mask_no_shareable(const char *cache_type, unsigned long *mask);
>  int get_cache_size(int cpu_no, const char *cache_type, unsigned long *cache_size);
> +int resource_info_unsigned_get(const char *resource, const char *filename, unsigned int *val);
>  void ctrlc_handler(int signum, siginfo_t *info, void *ptr);
>  int signal_handler_register(void);
>  void signal_handler_unregister(void);
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index 5750662cce57..cb5147c5f9a9 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -249,6 +249,45 @@ static int get_bit_mask(const char *filename, unsigned long *mask)
>  	return 0;
>  }
>  
> +/*

By not starting with /** the following will not be interpreted as kernel-doc
but the formatting does appear to follow the syntax, but not consistently so.
I think it would be more readable if the kernel-doc syntax is followed consistently.

> + * resource_info_unsigned_get - Read an unsigned value from a file in
> + * /sys/fs/resctrl/info/RESOURCE/FILENAME

"Read an unsigned value from /sys/fs/resctrl/info/RESOURCE/FILENAME"?

> + * @resource:	Resource name that matches directory names in

names? (plural)

> + *		/sys/fs/resctrl/info
> + * @filename:	Filename of a file located in a directory specified with the
> + *		'resource' variable.

I think this can be shortened to "File in /sys/fs/resctrl/info/@resource"

> + * @val:	Variable where the read value is saved on success.

"Contains read value on success."

(no need to refer to it as a variable/parameter, it is implied by syntax).

> + *
> + * Return: = 0 on success, < 0 on failure. On success the read value is saved into the 'val'
> + * variable.

"saved into the 'val' variable" -> "saved into @val" (since syntax indicates it is the parameter
there is no need to elaborate). 
Also please let lines in comments be of consistent length.

> + */


> +int resource_info_unsigned_get(const char *resource, const char *filename,
> +			       unsigned int *val)
> +{
> +	char reason[128], file_path[PATH_MAX];
> +	FILE *fp;
> +
> +	snprintf(file_path, sizeof(file_path), "%s/%s/%s", INFO_PATH, resource,
> +		 filename);
> +
> +	fp = fopen(file_path, "r");
> +	if (!fp) {
> +		snprintf(reason, sizeof(reason), "Error in opening %s file\n", filename);

(apart from other discussions). "file" in message seems redundant. It can just be "Error
opening %s". It may also be useful to print file_path instead of filename to be specific
of what the code tried to open.

> +		ksft_perror(reason);
> +		return -1;
> +	}
> +
> +	if (fscanf(fp, "%u", val) <= 0) {
> +		snprintf(reason, sizeof(reason), "Could not get %s's contents\n", filename);
> +		ksft_perror(reason);

filename -> file_path ?

> +		fclose(fp);
> +		return -1;
> +	}
> +
> +	fclose(fp);
> +	return 0;
> +}
> +


Reinette

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

[Maciej Wieczor-Retman]

Hello!

On 2024-01-26 at 13:08:19 -0800, Reinette Chatre wrote:
>Hi Maciej,
>
>On 1/25/2024 3:10 AM, Maciej Wieczor-Retman wrote:
>> The CAT non-contiguous selftests have to read the file responsible for
>> reporting support of non-contiguous CBMs in kernel (resctrl). Then the
>> test compares if that information matches what is reported by CPUID
>> output.
>> 
>> Add a generic helper function to read an unsigned number from a file in
>> /sys/fs/resctrl/info/<RESOURCE>/<FILE>.
>> 
>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>> ---
>> Changelog v3:
>> - Rewrite patch message.
>> - Add documentation and rewrote the function. (Reinette)
>> 
>> Changelog v2:
>> - Add this patch.
>> 
>>  tools/testing/selftests/resctrl/resctrl.h   |  1 +
>>  tools/testing/selftests/resctrl/resctrlfs.c | 39 +++++++++++++++++++++
>>  2 files changed, 40 insertions(+)
>> 
>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
>> index a1462029998e..5116ea082d03 100644
>> --- a/tools/testing/selftests/resctrl/resctrl.h
>> +++ b/tools/testing/selftests/resctrl/resctrl.h
>> @@ -162,6 +162,7 @@ unsigned int count_contiguous_bits(unsigned long val, unsigned int *start);
>>  int get_full_cbm(const char *cache_type, unsigned long *mask);
>>  int get_mask_no_shareable(const char *cache_type, unsigned long *mask);
>>  int get_cache_size(int cpu_no, const char *cache_type, unsigned long *cache_size);
>> +int resource_info_unsigned_get(const char *resource, const char *filename, unsigned int *val);
>>  void ctrlc_handler(int signum, siginfo_t *info, void *ptr);
>>  int signal_handler_register(void);
>>  void signal_handler_unregister(void);
>> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
>> index 5750662cce57..cb5147c5f9a9 100644
>> --- a/tools/testing/selftests/resctrl/resctrlfs.c
>> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
>> @@ -249,6 +249,45 @@ static int get_bit_mask(const char *filename, unsigned long *mask)
>>  	return 0;
>>  }
>>  
>> +/*
>
>By not starting with /** the following will not be interpreted as kernel-doc
>but the formatting does appear to follow the syntax, but not consistently so.
>I think it would be more readable if the kernel-doc syntax is followed consistently.

Sure, I'll change it.

>
>> + * resource_info_unsigned_get - Read an unsigned value from a file in
>> + * /sys/fs/resctrl/info/RESOURCE/FILENAME
>
>"Read an unsigned value from /sys/fs/resctrl/info/RESOURCE/FILENAME"?

Okay

>
>> + * @resource:	Resource name that matches directory names in
>
>names? (plural)

Right, it doesn't make much sense, I'll move it to singular.

>
>> + *		/sys/fs/resctrl/info
>> + * @filename:	Filename of a file located in a directory specified with the
>> + *		'resource' variable.
>
>I think this can be shortened to "File in /sys/fs/resctrl/info/@resource"

Sure, thanks

>
>> + * @val:	Variable where the read value is saved on success.
>
>"Contains read value on success."
>
>(no need to refer to it as a variable/parameter, it is implied by syntax).

Right, I'll change it.

>
>> + *
>> + * Return: = 0 on success, < 0 on failure. On success the read value is saved into the 'val'
>> + * variable.
>
>"saved into the 'val' variable" -> "saved into @val" (since syntax indicates it is the parameter
>there is no need to elaborate). 

Sure, thanks

>Also please let lines in comments be of consistent length.

Okay, I'll keep it to 80 characters.

>
>> + */
>
>
>> +int resource_info_unsigned_get(const char *resource, const char *filename,
>> +			       unsigned int *val)
>> +{
>> +	char reason[128], file_path[PATH_MAX];
>> +	FILE *fp;
>> +
>> +	snprintf(file_path, sizeof(file_path), "%s/%s/%s", INFO_PATH, resource,
>> +		 filename);
>> +
>> +	fp = fopen(file_path, "r");
>> +	if (!fp) {
>> +		snprintf(reason, sizeof(reason), "Error in opening %s file\n", filename);
>
>(apart from other discussions). "file" in message seems redundant. It can just be "Error
>opening %s". It may also be useful to print file_path instead of filename to be specific
>of what the code tried to open.

Okay, I'll change it to file_path.

>
>> +		ksft_perror(reason);
>> +		return -1;
>> +	}
>> +
>> +	if (fscanf(fp, "%u", val) <= 0) {
>> +		snprintf(reason, sizeof(reason), "Could not get %s's contents\n", filename);
>> +		ksft_perror(reason);
>
>filename -> file_path ?

Same as above.

>
>> +		fclose(fp);
>> +		return -1;
>> +	}
>> +
>> +	fclose(fp);
>> +	return 0;
>> +}
>> +
>
>
>Reinette

-- 
Kind regards
Maciej Wieczór-Retman

[PATCH v3 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 v3:
- Move new function to a separate patch. (Reinette)
- Rewrite resctrl_mon_feature_exists() only for L3_MON.

Changelog v2:
- Add this patch.

 tools/testing/selftests/resctrl/cmt_test.c  |  4 +--
 tools/testing/selftests/resctrl/mba_test.c  |  4 +--
 tools/testing/selftests/resctrl/mbm_test.c  |  6 ++--
 tools/testing/selftests/resctrl/resctrl.h   |  3 +-
 tools/testing/selftests/resctrl/resctrlfs.c | 33 +++++++++++++--------
 5 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index dd5ca343c469..428de9df81c8 100644
--- a/tools/testing/selftests/resctrl/cmt_test.c
+++ b/tools/testing/selftests/resctrl/cmt_test.c
@@ -169,8 +169,8 @@ static int cmt_run_test(const struct resctrl_test *test, const struct user_param
 
 static bool cmt_feature_check(const struct resctrl_test *test)
 {
-	return test_resource_feature_check(test) &&
-	       validate_resctrl_feature_request("L3_MON", "llc_occupancy");
+	return resctrl_mon_feature_exists("llc_occupancy") &&
+	       resctrl_resource_exists("L3");
 }
 
 struct resctrl_test cmt_test = {
diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
index da256d2dbe5c..e22285b80e37 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -170,8 +170,8 @@ static int mba_run_test(const struct resctrl_test *test, const struct user_param
 
 static bool mba_feature_check(const struct resctrl_test *test)
 {
-	return test_resource_feature_check(test) &&
-	       validate_resctrl_feature_request("L3_MON", "mbm_local_bytes");
+	return resctrl_resource_exists(test->resource) &&
+	       resctrl_mon_feature_exists("mbm_local_bytes");
 }
 
 struct resctrl_test mba_test = {
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index 34879e7b71a0..9c885bc427ca 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -97,7 +97,7 @@ static int mbm_setup(const struct resctrl_test *test,
 		return END_OF_TESTS;
 
 	/* Set up shemata with 100% allocation on the first run. */
-	if (p->num_of_runs == 0 && validate_resctrl_feature_request("MB", NULL))
+	if (p->num_of_runs == 0 && resctrl_resource_exists("MB"))
 		ret = write_schemata(p->ctrlgrp, "100", uparams->cpu, test->resource);
 
 	p->num_of_runs++;
@@ -140,8 +140,8 @@ static int mbm_run_test(const struct resctrl_test *test, const struct user_param
 
 static bool mbm_feature_check(const struct resctrl_test *test)
 {
-	return validate_resctrl_feature_request("L3_MON", "mbm_total_bytes") &&
-	       validate_resctrl_feature_request("L3_MON", "mbm_local_bytes");
+	return resctrl_mon_feature_exists("mbm_total_bytes") &&
+	       resctrl_mon_feature_exists("mbm_local_bytes");
 }
 
 struct resctrl_test mbm_test = {
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index 5116ea082d03..4603b215b97e 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -136,7 +136,8 @@ int get_domain_id(const char *resource, int cpu_no, int *domain_id);
 int mount_resctrlfs(void);
 int umount_resctrlfs(void);
 int validate_bw_report_request(char *bw_report);
-bool validate_resctrl_feature_request(const char *resource, const char *feature);
+bool resctrl_resource_exists(const char *resource);
+bool resctrl_mon_feature_exists(const char *feature);
 bool test_resource_feature_check(const struct resctrl_test *test);
 char *fgrep(FILE *inf, const char *str);
 int taskset_benchmark(pid_t bm_pid, int cpu_no, cpu_set_t *old_affinity);
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index cb5147c5f9a9..e4ba8614fb7b 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -711,20 +711,16 @@ char *fgrep(FILE *inf, const char *str)
 }
 
 /*
- * validate_resctrl_feature_request - Check if requested feature is valid.
- * @resource:	Required resource (e.g., MB, L3, L2, L3_MON, etc.)
- * @feature:	Required monitor feature (in mon_features file). Can only be
- *		set for L3_MON. Must be NULL for all other resources.
+ * resctrl_resource_exists - Check if a resource is supported.
+ * @resource:	Resctrl resource (e.g., MB, L3, L2, L3_MON, etc.)
  *
- * Return: True if the resource/feature is supported, else false. False is
+ * Return: True if the resource is supported, else false. False is
  *         also returned if resctrl FS is not mounted.
  */
-bool validate_resctrl_feature_request(const char *resource, const char *feature)
+bool resctrl_resource_exists(const char *resource)
 {
 	char res_path[PATH_MAX];
 	struct stat statbuf;
-	char *res;
-	FILE *inf;
 	int ret;
 
 	if (!resource)
@@ -739,11 +735,24 @@ bool validate_resctrl_feature_request(const char *resource, const char *feature)
 	if (stat(res_path, &statbuf))
 		return false;
 
+	return true;
+}
+
+/*
+ * resctrl_mon_feature_exists - Check if requested monitoring L3_MON feature is valid.
+ * @feature:	Required monitor feature (in mon_features file).
+ *
+ * Return: True if the feature is supported, else false.
+ */
+bool resctrl_mon_feature_exists(const char *feature)
+{
+	char *res;
+	FILE *inf;
+
 	if (!feature)
-		return true;
+		return false;
 
-	snprintf(res_path, sizeof(res_path), "%s/%s/mon_features", INFO_PATH, resource);
-	inf = fopen(res_path, "r");
+	inf = fopen("/sys/fs/resctrl/info/L3_MON/mon_features", "r");
 	if (!inf)
 		return false;
 
@@ -756,7 +765,7 @@ bool validate_resctrl_feature_request(const char *resource, const char *feature)
 
 bool test_resource_feature_check(const struct resctrl_test *test)
 {
-	return validate_resctrl_feature_request(test->resource, NULL);
+	return resctrl_resource_exists(test->resource);
 }
 
 int filter_dmesg(void)
-- 
2.43.0

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

[Ilpo Järvinen]

On Thu, 25 Jan 2024, Maciej Wieczor-Retman wrote:

> validate_resctrl_feature_request() is used to test both if a resource is
> present in the info directory, and if a passed monitoring feature is
> present in the mon_features file.
> 
> Refactor validate_resctrl_feature_request() into two smaller functions
> that each accomplish one check to give feature checking more
> granularity:
> - Resource directory presence in the /sys/fs/resctrl/info directory.
> - Feature name presence in the /sys/fs/resctrl/info/L3_MON/mon_features
>   file.
> 
> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> ---
> Changelog v3:
> - Move new function to a separate patch. (Reinette)
> - Rewrite resctrl_mon_feature_exists() only for L3_MON.
> 
> Changelog v2:
> - Add this patch.
> 
>  tools/testing/selftests/resctrl/cmt_test.c  |  4 +--
>  tools/testing/selftests/resctrl/mba_test.c  |  4 +--
>  tools/testing/selftests/resctrl/mbm_test.c  |  6 ++--
>  tools/testing/selftests/resctrl/resctrl.h   |  3 +-
>  tools/testing/selftests/resctrl/resctrlfs.c | 33 +++++++++++++--------
>  5 files changed, 30 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
> index dd5ca343c469..428de9df81c8 100644
> --- a/tools/testing/selftests/resctrl/cmt_test.c
> +++ b/tools/testing/selftests/resctrl/cmt_test.c
> @@ -169,8 +169,8 @@ static int cmt_run_test(const struct resctrl_test *test, const struct user_param
>  
>  static bool cmt_feature_check(const struct resctrl_test *test)
>  {
> -	return test_resource_feature_check(test) &&
> -	       validate_resctrl_feature_request("L3_MON", "llc_occupancy");
> +	return resctrl_mon_feature_exists("llc_occupancy") &&
> +	       resctrl_resource_exists("L3");
>  }
>  
>  struct resctrl_test cmt_test = {
> diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
> index da256d2dbe5c..e22285b80e37 100644
> --- a/tools/testing/selftests/resctrl/mba_test.c
> +++ b/tools/testing/selftests/resctrl/mba_test.c
> @@ -170,8 +170,8 @@ static int mba_run_test(const struct resctrl_test *test, const struct user_param
>  
>  static bool mba_feature_check(const struct resctrl_test *test)
>  {
> -	return test_resource_feature_check(test) &&
> -	       validate_resctrl_feature_request("L3_MON", "mbm_local_bytes");
> +	return resctrl_resource_exists(test->resource) &&

I don't understand what's the advantage of converting away from 
test_resource_feature_check() in CMT and MBA case?

> +	       resctrl_mon_feature_exists("mbm_local_bytes");
>  }

> @@ -756,7 +765,7 @@ bool validate_resctrl_feature_request(const char *resource, const char *feature)
>  
>  bool test_resource_feature_check(const struct resctrl_test *test)
>  {
> -	return validate_resctrl_feature_request(test->resource, NULL);
> +	return resctrl_resource_exists(test->resource);

...The replacement in MBA open coded test_resource_feature_check() 100% 
and CMT even replaces the test->resource with the string matching to 
what's in test->resource?


-- 
 i.

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

[Maciej Wieczor-Retman]

Hi Ilpo,

On 2024-01-25 at 13:46:51 +0200, Ilpo Järvinen wrote:
>On Thu, 25 Jan 2024, Maciej Wieczor-Retman wrote:
>
>> validate_resctrl_feature_request() is used to test both if a resource is
>> present in the info directory, and if a passed monitoring feature is
>> present in the mon_features file.
>> 
>> Refactor validate_resctrl_feature_request() into two smaller functions
>> that each accomplish one check to give feature checking more
>> granularity:
>> - Resource directory presence in the /sys/fs/resctrl/info directory.
>> - Feature name presence in the /sys/fs/resctrl/info/L3_MON/mon_features
>>   file.
>> 
>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>> ---
>> Changelog v3:
>> - Move new function to a separate patch. (Reinette)
>> - Rewrite resctrl_mon_feature_exists() only for L3_MON.
>> 
>> Changelog v2:
>> - Add this patch.
>> 
>>  tools/testing/selftests/resctrl/cmt_test.c  |  4 +--
>>  tools/testing/selftests/resctrl/mba_test.c  |  4 +--
>>  tools/testing/selftests/resctrl/mbm_test.c  |  6 ++--
>>  tools/testing/selftests/resctrl/resctrl.h   |  3 +-
>>  tools/testing/selftests/resctrl/resctrlfs.c | 33 +++++++++++++--------
>>  5 files changed, 30 insertions(+), 20 deletions(-)
>> 
>> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
>> index dd5ca343c469..428de9df81c8 100644
>> --- a/tools/testing/selftests/resctrl/cmt_test.c
>> +++ b/tools/testing/selftests/resctrl/cmt_test.c
>> @@ -169,8 +169,8 @@ static int cmt_run_test(const struct resctrl_test *test, const struct user_param
>>  
>>  static bool cmt_feature_check(const struct resctrl_test *test)
>>  {
>> -	return test_resource_feature_check(test) &&
>> -	       validate_resctrl_feature_request("L3_MON", "llc_occupancy");
>> +	return resctrl_mon_feature_exists("llc_occupancy") &&
>> +	       resctrl_resource_exists("L3");
>>  }
>>  
>>  struct resctrl_test cmt_test = {
>> diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
>> index da256d2dbe5c..e22285b80e37 100644
>> --- a/tools/testing/selftests/resctrl/mba_test.c
>> +++ b/tools/testing/selftests/resctrl/mba_test.c
>> @@ -170,8 +170,8 @@ static int mba_run_test(const struct resctrl_test *test, const struct user_param
>>  
>>  static bool mba_feature_check(const struct resctrl_test *test)
>>  {
>> -	return test_resource_feature_check(test) &&
>> -	       validate_resctrl_feature_request("L3_MON", "mbm_local_bytes");
>> +	return resctrl_resource_exists(test->resource) &&
>
>I don't understand what's the advantage of converting away from 
>test_resource_feature_check() in CMT and MBA case?
>
>> +	       resctrl_mon_feature_exists("mbm_local_bytes");
>>  }
>
>> @@ -756,7 +765,7 @@ bool validate_resctrl_feature_request(const char *resource, const char *feature)
>>  
>>  bool test_resource_feature_check(const struct resctrl_test *test)
>>  {
>> -	return validate_resctrl_feature_request(test->resource, NULL);
>> +	return resctrl_resource_exists(test->resource);
>
>...The replacement in MBA open coded test_resource_feature_check() 100% 
>and CMT even replaces the test->resource with the string matching to 
>what's in test->resource?
>

You're right, I got carried away with refactoring a bit. I'll keep
test_resource_feature_check() for CMT and MBM. Thanks!

>
>-- 
> i.
>

-- 
Kind regards
Maciej Wieczór-Retman

[PATCH v3 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>
---
Changelog v3:
- Split off the new function into this patch. (Reinette)

Changelog v2:
- Add this patch.

 tools/testing/selftests/resctrl/resctrl.h   |  2 ++
 tools/testing/selftests/resctrl/resctrlfs.c | 26 +++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index 4603b215b97e..c39105f46da9 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -138,6 +138,8 @@ int umount_resctrlfs(void);
 int validate_bw_report_request(char *bw_report);
 bool resctrl_resource_exists(const char *resource);
 bool resctrl_mon_feature_exists(const char *feature);
+bool resource_info_file_exists(const char *resource,
+			       const char *feature);
 bool test_resource_feature_check(const struct resctrl_test *test);
 char *fgrep(FILE *inf, const char *str);
 int taskset_benchmark(pid_t bm_pid, int cpu_no, cpu_set_t *old_affinity);
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index e4ba8614fb7b..a6427732e0ad 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -763,6 +763,32 @@ bool resctrl_mon_feature_exists(const char *feature)
 	return !!res;
 }
 
+/*
+ * resource_info_file_exists - Check if a file is present inside
+ * /sys/fs/resctrl/info/RESOURCE.
+ * @resource:	Required resource (Eg: MB, L3, L2, etc.)
+ * @feature:	Required feature.
+ *
+ * Return: True if the file exists, else false.
+ */
+bool resource_info_file_exists(const char *resource,
+			       const char *feature)
+{
+	char res_path[PATH_MAX];
+	struct stat statbuf;
+
+	if (!feature || !resource)
+		return false;
+
+	snprintf(res_path, sizeof(res_path), "%s/%s/%s", INFO_PATH, resource,
+		 feature);
+
+	if (stat(res_path, &statbuf))
+		return false;
+
+	return true;
+}
+
 bool test_resource_feature_check(const struct resctrl_test *test)
 {
 	return resctrl_resource_exists(test->resource);
-- 
2.43.0

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

[Ilpo Järvinen]

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

On Thu, 25 Jan 2024, Maciej Wieczor-Retman wrote:

> Feature checking done by resctrl_mon_feature_exists() covers features
> represented by the feature name presence inside the 'mon_features' file
> in /sys/fs/resctrl/info/L3_MON directory. There exists a different way
> to represent feature support and that is by the presence of 0 or 1 in a
> single file in the info/resource directory. In this case the filename
> represents what feature support is being indicated.
> 
> Add a generic function to check file presence in the
> /sys/fs/resctrl/info/<RESOURCE> directory.
> 
> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> ---
> Changelog v3:
> - Split off the new function into this patch. (Reinette)
> 
> Changelog v2:
> - Add this patch.
> 
>  tools/testing/selftests/resctrl/resctrl.h   |  2 ++
>  tools/testing/selftests/resctrl/resctrlfs.c | 26 +++++++++++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index 4603b215b97e..c39105f46da9 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -138,6 +138,8 @@ int umount_resctrlfs(void);
>  int validate_bw_report_request(char *bw_report);
>  bool resctrl_resource_exists(const char *resource);
>  bool resctrl_mon_feature_exists(const char *feature);
> +bool resource_info_file_exists(const char *resource,
> +			       const char *feature);
>  bool test_resource_feature_check(const struct resctrl_test *test);
>  char *fgrep(FILE *inf, const char *str);
>  int taskset_benchmark(pid_t bm_pid, int cpu_no, cpu_set_t *old_affinity);
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index e4ba8614fb7b..a6427732e0ad 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -763,6 +763,32 @@ bool resctrl_mon_feature_exists(const char *feature)
>  	return !!res;
>  }
>  
> +/*
> + * resource_info_file_exists - Check if a file is present inside
> + * /sys/fs/resctrl/info/RESOURCE.
> + * @resource:	Required resource (Eg: MB, L3, L2, etc.)
> + * @feature:	Required feature.
> + *
> + * Return: True if the file exists, else false.
> + */
> +bool resource_info_file_exists(const char *resource,
> +			       const char *feature)

Fits to one line.

> +{
> +	char res_path[PATH_MAX];
> +	struct stat statbuf;
> +
> +	if (!feature || !resource)
> +		return false;
> +
> +	snprintf(res_path, sizeof(res_path), "%s/%s/%s", INFO_PATH, resource,
> +		 feature);
> +
> +	if (stat(res_path, &statbuf))
> +		return false;
> +
> +	return true;
> +}
> +
>  bool test_resource_feature_check(const struct resctrl_test *test)
>  {
>  	return resctrl_resource_exists(test->resource);
> 

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

-- 
 i.

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

[Reinette Chatre]

Hi Maciej,

On 1/25/2024 3:12 AM, Maciej Wieczor-Retman wrote:
> Feature checking done by resctrl_mon_feature_exists() covers features
> represented by the feature name presence inside the 'mon_features' file
> in /sys/fs/resctrl/info/L3_MON directory. There exists a different way
> to represent feature support and that is by the presence of 0 or 1 in a
> single file in the info/resource directory. In this case the filename
> represents what feature support is being indicated.
> 
> Add a generic function to check file presence in the
> /sys/fs/resctrl/info/<RESOURCE> directory.
> 
> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> ---
> Changelog v3:
> - Split off the new function into this patch. (Reinette)
> 
> Changelog v2:
> - Add this patch.
> 
>  tools/testing/selftests/resctrl/resctrl.h   |  2 ++
>  tools/testing/selftests/resctrl/resctrlfs.c | 26 +++++++++++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index 4603b215b97e..c39105f46da9 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -138,6 +138,8 @@ int umount_resctrlfs(void);
>  int validate_bw_report_request(char *bw_report);
>  bool resctrl_resource_exists(const char *resource);
>  bool resctrl_mon_feature_exists(const char *feature);
> +bool resource_info_file_exists(const char *resource,
> +			       const char *feature);

In addition what Ilpo commented about other line, this too can be
on one line.

>  bool test_resource_feature_check(const struct resctrl_test *test);
>  char *fgrep(FILE *inf, const char *str);
>  int taskset_benchmark(pid_t bm_pid, int cpu_no, cpu_set_t *old_affinity);
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index e4ba8614fb7b..a6427732e0ad 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -763,6 +763,32 @@ bool resctrl_mon_feature_exists(const char *feature)
>  	return !!res;
>  }
>  
> +/*
> + * resource_info_file_exists - Check if a file is present inside
> + * /sys/fs/resctrl/info/RESOURCE.

As confirmed in the changelog this is intended to be a generic function that
tests if a file exists ...

> + * @resource:	Required resource (Eg: MB, L3, L2, etc.)
> + * @feature:	Required feature.

... so assuming a use of this by referring to the file as "feature" is unexpected.

This function jumps between "file" and "feature" in comments and code, please
just stick to goal of making it a generic utility that checks for a file and
refer to it consistently so.

Reinette

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

[Maciej Wieczor-Retman]

Hello Reinette,

On 2024-01-26 at 13:08:52 -0800, Reinette Chatre wrote:
>Hi Maciej,
>
>On 1/25/2024 3:12 AM, Maciej Wieczor-Retman wrote:
>> Feature checking done by resctrl_mon_feature_exists() covers features
>> represented by the feature name presence inside the 'mon_features' file
>> in /sys/fs/resctrl/info/L3_MON directory. There exists a different way
>> to represent feature support and that is by the presence of 0 or 1 in a
>> single file in the info/resource directory. In this case the filename
>> represents what feature support is being indicated.
>> 
>> Add a generic function to check file presence in the
>> /sys/fs/resctrl/info/<RESOURCE> directory.
>> 
>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>> ---
>> Changelog v3:
>> - Split off the new function into this patch. (Reinette)
>> 
>> Changelog v2:
>> - Add this patch.
>> 
>>  tools/testing/selftests/resctrl/resctrl.h   |  2 ++
>>  tools/testing/selftests/resctrl/resctrlfs.c | 26 +++++++++++++++++++++
>>  2 files changed, 28 insertions(+)
>> 
>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
>> index 4603b215b97e..c39105f46da9 100644
>> --- a/tools/testing/selftests/resctrl/resctrl.h
>> +++ b/tools/testing/selftests/resctrl/resctrl.h
>> @@ -138,6 +138,8 @@ int umount_resctrlfs(void);
>>  int validate_bw_report_request(char *bw_report);
>>  bool resctrl_resource_exists(const char *resource);
>>  bool resctrl_mon_feature_exists(const char *feature);
>> +bool resource_info_file_exists(const char *resource,
>> +			       const char *feature);
>
>In addition what Ilpo commented about other line, this too can be
>on one line.

Thanks!

>
>>  bool test_resource_feature_check(const struct resctrl_test *test);
>>  char *fgrep(FILE *inf, const char *str);
>>  int taskset_benchmark(pid_t bm_pid, int cpu_no, cpu_set_t *old_affinity);
>> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
>> index e4ba8614fb7b..a6427732e0ad 100644
>> --- a/tools/testing/selftests/resctrl/resctrlfs.c
>> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
>> @@ -763,6 +763,32 @@ bool resctrl_mon_feature_exists(const char *feature)
>>  	return !!res;
>>  }
>>  
>> +/*
>> + * resource_info_file_exists - Check if a file is present inside
>> + * /sys/fs/resctrl/info/RESOURCE.
>
>As confirmed in the changelog this is intended to be a generic function that
>tests if a file exists ...
>
>> + * @resource:	Required resource (Eg: MB, L3, L2, etc.)
>> + * @feature:	Required feature.
>
>... so assuming a use of this by referring to the file as "feature" is unexpected.
>
>This function jumps between "file" and "feature" in comments and code, please
>just stick to goal of making it a generic utility that checks for a file and
>refer to it consistently so.

Okay, I'll stick to 'file'.

>
>Reinette

-- 
Kind regards
Maciej Wieczór-Retman

[PATCH v3 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 v3:
- Roll back __cpuid_count part. (Reinette)
- Update function name to read sparse_masks file.
- Roll back get_cache_level() changes.
- Add ksft_print_msg() to contiguous schemata write error handling
  (Reinette).

Changelog v2:
- Redo the patch message. (Ilpo)
- Tidy up __cpuid_count calls. (Ilpo)
- Remove redundant AND in noncont_mask calculations (Ilpo)
- Fix bit_center offset.
- Add newline before function return. (Ilpo)
- Group non-contiguous tests with CAT tests. (Ilpo)
- Use a helper for reading sparse_masks file. (Ilpo)
- Make get_cache_level() available in other source files. (Ilpo)

 tools/testing/selftests/resctrl/cat_test.c    | 81 +++++++++++++++++++
 tools/testing/selftests/resctrl/resctrl.h     |  2 +
 .../testing/selftests/resctrl/resctrl_tests.c |  2 +
 3 files changed, 85 insertions(+)

diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
index 39fc9303b8e8..9086bf359072 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -294,6 +294,71 @@ static int cat_run_test(const struct resctrl_test *test, const struct user_param
 	return ret;
 }
 
+static int noncont_cat_run_test(const struct resctrl_test *test,
+				const struct user_params *uparams)
+{
+	unsigned long full_cache_mask, cont_mask, noncont_mask;
+	unsigned int eax, ebx, ecx, edx, ret, sparse_masks;
+	char schemata[64];
+	int bit_center;
+
+	/* Check to compare sparse_masks content to CPUID output. */
+	ret = resource_info_unsigned_get(test->resource, "sparse_masks", &sparse_masks);
+	if (ret)
+		return ret;
+
+	if (!strcmp(test->resource, "L3"))
+		__cpuid_count(0x10, 1, eax, ebx, ecx, edx);
+	else if (!strcmp(test->resource, "L2"))
+		__cpuid_count(0x10, 2, eax, ebx, ecx, edx);
+	else
+		return -EINVAL;
+
+	if (sparse_masks != ((ecx >> 3) & 1)) {
+		ksft_print_msg("CPUID output doesn't match 'sparse_masks' file content!\n");
+		return -1;
+	}
+
+	/* Write checks initialization. */
+	ret = get_full_cbm(test->resource, &full_cache_mask);
+	if (ret < 0)
+		return ret;
+	bit_center = count_bits(full_cache_mask) / 2;
+	cont_mask = full_cache_mask >> bit_center;
+
+	/* Contiguous mask write check. */
+	snprintf(schemata, sizeof(schemata), "%lx", cont_mask);
+	ret = write_schemata("", schemata, uparams->cpu, test->resource);
+	if (ret) {
+		ksft_print_msg("Write of contiguous CBM failed\n");
+		return ret;
+	}
+
+	/*
+	 * Non-contiguous mask write check. CBM has a 0xf hole approximately in the middle.
+	 * Output is compared with support information to catch any edge case errors.
+	 */
+	noncont_mask = ~(0xf << (bit_center - 2)) & full_cache_mask;
+	snprintf(schemata, sizeof(schemata), "%lx", noncont_mask);
+	ret = write_schemata("", schemata, uparams->cpu, test->resource);
+	if (ret && sparse_masks)
+		ksft_print_msg("Non-contiguous CBMs supported but write of non-contiguous CBM failed\n");
+	else if (ret && !sparse_masks)
+		ksft_print_msg("Non-contiguous CBMs not supported and write of non-contiguous CBM failed as expected\n");
+	else if (!ret && !sparse_masks)
+		ksft_print_msg("Non-contiguous CBMs not supported but write of non-contiguous CBM succeeded\n");
+
+	return !ret == !sparse_masks;
+}
+
+static bool noncont_cat_feature_check(const struct resctrl_test *test)
+{
+	if (!resctrl_resource_exists(test->resource))
+		return false;
+
+	return resource_info_file_exists(test->resource, "sparse_masks");
+}
+
 struct resctrl_test l3_cat_test = {
 	.name = "L3_CAT",
 	.group = "CAT",
@@ -301,3 +366,19 @@ struct resctrl_test l3_cat_test = {
 	.feature_check = test_resource_feature_check,
 	.run_test = cat_run_test,
 };
+
+struct resctrl_test l3_noncont_cat_test = {
+	.name = "L3_NONCONT_CAT",
+	.group = "CAT",
+	.resource = "L3",
+	.feature_check = noncont_cat_feature_check,
+	.run_test = noncont_cat_run_test,
+};
+
+struct resctrl_test l2_noncont_cat_test = {
+	.name = "L2_NONCONT_CAT",
+	.group = "CAT",
+	.resource = "L2",
+	.feature_check = noncont_cat_feature_check,
+	.run_test = noncont_cat_run_test,
+};
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index c39105f46da9..8cb97f278459 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -210,5 +210,7 @@ extern struct resctrl_test mbm_test;
 extern struct resctrl_test mba_test;
 extern struct resctrl_test cmt_test;
 extern struct resctrl_test l3_cat_test;
+extern struct resctrl_test l3_noncont_cat_test;
+extern struct resctrl_test l2_noncont_cat_test;
 
 #endif /* RESCTRL_H */
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index 3044179ee6e9..f3dc1b9696e7 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -19,6 +19,8 @@ static struct resctrl_test *resctrl_tests[] = {
 	&mba_test,
 	&cmt_test,
 	&l3_cat_test,
+	&l3_noncont_cat_test,
+	&l2_noncont_cat_test,
 };
 
 static int detect_vendor(void)
-- 
2.43.0

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

[Reinette Chatre]

Hi Maciej,

On 1/25/2024 3:13 AM, Maciej Wieczor-Retman wrote:
> Add tests for both L2 and L3 CAT to verify the return values
> generated by writing non-contiguous CBMs don't contradict the
> reported non-contiguous support information.
> 
> Use a logical XOR to confirm return value of write_schemata() and
> non-contiguous CBMs support information match.
> 
> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> ---
> Changelog v3:
> - Roll back __cpuid_count part. (Reinette)
> - Update function name to read sparse_masks file.
> - Roll back get_cache_level() changes.
> - Add ksft_print_msg() to contiguous schemata write error handling
>   (Reinette).
> 
> Changelog v2:
> - Redo the patch message. (Ilpo)
> - Tidy up __cpuid_count calls. (Ilpo)
> - Remove redundant AND in noncont_mask calculations (Ilpo)
> - Fix bit_center offset.
> - Add newline before function return. (Ilpo)
> - Group non-contiguous tests with CAT tests. (Ilpo)
> - Use a helper for reading sparse_masks file. (Ilpo)
> - Make get_cache_level() available in other source files. (Ilpo)
> 
>  tools/testing/selftests/resctrl/cat_test.c    | 81 +++++++++++++++++++
>  tools/testing/selftests/resctrl/resctrl.h     |  2 +
>  .../testing/selftests/resctrl/resctrl_tests.c |  2 +
>  3 files changed, 85 insertions(+)
> 
> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
> index 39fc9303b8e8..9086bf359072 100644
> --- a/tools/testing/selftests/resctrl/cat_test.c
> +++ b/tools/testing/selftests/resctrl/cat_test.c
> @@ -294,6 +294,71 @@ static int cat_run_test(const struct resctrl_test *test, const struct user_param
>  	return ret;
>  }
>  
> +static int noncont_cat_run_test(const struct resctrl_test *test,
> +				const struct user_params *uparams)
> +{
> +	unsigned long full_cache_mask, cont_mask, noncont_mask;
> +	unsigned int eax, ebx, ecx, edx, ret, sparse_masks;
> +	char schemata[64];
> +	int bit_center;
> +
> +	/* Check to compare sparse_masks content to CPUID output. */
> +	ret = resource_info_unsigned_get(test->resource, "sparse_masks", &sparse_masks);
> +	if (ret)
> +		return ret;
> +
> +	if (!strcmp(test->resource, "L3"))
> +		__cpuid_count(0x10, 1, eax, ebx, ecx, edx);
> +	else if (!strcmp(test->resource, "L2"))
> +		__cpuid_count(0x10, 2, eax, ebx, ecx, edx);
> +	else
> +		return -EINVAL;
> +
> +	if (sparse_masks != ((ecx >> 3) & 1)) {
> +		ksft_print_msg("CPUID output doesn't match 'sparse_masks' file content!\n");
> +		return -1;

If I understand correctly this falls into the "test failure" [1] category
and should return 1? ...

> +	}
> +
> +	/* Write checks initialization. */
> +	ret = get_full_cbm(test->resource, &full_cache_mask);
> +	if (ret < 0)
> +		return ret;
> +	bit_center = count_bits(full_cache_mask) / 2;
> +	cont_mask = full_cache_mask >> bit_center;
> +
> +	/* Contiguous mask write check. */
> +	snprintf(schemata, sizeof(schemata), "%lx", cont_mask);
> +	ret = write_schemata("", schemata, uparams->cpu, test->resource);
> +	if (ret) {
> +		ksft_print_msg("Write of contiguous CBM failed\n");
> +		return ret;

... although here I think the goal to distinguish between test error and test failure
falls apart since it is not possible to tell within the test if the failure is
because of error in the test or if test failed.

Reinette

[1] https://lore.kernel.org/all/33787043-5823-6de4-4e5c-a24a136ba541@linux.intel.com/

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

[Maciej Wieczor-Retman]

Hi!

On 2024-01-26 at 13:10:18 -0800, Reinette Chatre wrote:
>Hi Maciej,
>
>On 1/25/2024 3:13 AM, Maciej Wieczor-Retman wrote:
>> Add tests for both L2 and L3 CAT to verify the return values
>> generated by writing non-contiguous CBMs don't contradict the
>> reported non-contiguous support information.
>> 
>> Use a logical XOR to confirm return value of write_schemata() and
>> non-contiguous CBMs support information match.
>> 
>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>> ---
>> Changelog v3:
>> - Roll back __cpuid_count part. (Reinette)
>> - Update function name to read sparse_masks file.
>> - Roll back get_cache_level() changes.
>> - Add ksft_print_msg() to contiguous schemata write error handling
>>   (Reinette).
>> 
>> Changelog v2:
>> - Redo the patch message. (Ilpo)
>> - Tidy up __cpuid_count calls. (Ilpo)
>> - Remove redundant AND in noncont_mask calculations (Ilpo)
>> - Fix bit_center offset.
>> - Add newline before function return. (Ilpo)
>> - Group non-contiguous tests with CAT tests. (Ilpo)
>> - Use a helper for reading sparse_masks file. (Ilpo)
>> - Make get_cache_level() available in other source files. (Ilpo)
>> 
>>  tools/testing/selftests/resctrl/cat_test.c    | 81 +++++++++++++++++++
>>  tools/testing/selftests/resctrl/resctrl.h     |  2 +
>>  .../testing/selftests/resctrl/resctrl_tests.c |  2 +
>>  3 files changed, 85 insertions(+)
>> 
>> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
>> index 39fc9303b8e8..9086bf359072 100644
>> --- a/tools/testing/selftests/resctrl/cat_test.c
>> +++ b/tools/testing/selftests/resctrl/cat_test.c
>> @@ -294,6 +294,71 @@ static int cat_run_test(const struct resctrl_test *test, const struct user_param
>>  	return ret;
>>  }
>>  
>> +static int noncont_cat_run_test(const struct resctrl_test *test,
>> +				const struct user_params *uparams)
>> +{
>> +	unsigned long full_cache_mask, cont_mask, noncont_mask;
>> +	unsigned int eax, ebx, ecx, edx, ret, sparse_masks;
>> +	char schemata[64];
>> +	int bit_center;
>> +
>> +	/* Check to compare sparse_masks content to CPUID output. */
>> +	ret = resource_info_unsigned_get(test->resource, "sparse_masks", &sparse_masks);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (!strcmp(test->resource, "L3"))
>> +		__cpuid_count(0x10, 1, eax, ebx, ecx, edx);
>> +	else if (!strcmp(test->resource, "L2"))
>> +		__cpuid_count(0x10, 2, eax, ebx, ecx, edx);
>> +	else
>> +		return -EINVAL;
>> +
>> +	if (sparse_masks != ((ecx >> 3) & 1)) {
>> +		ksft_print_msg("CPUID output doesn't match 'sparse_masks' file content!\n");
>> +		return -1;
>
>If I understand correctly this falls into the "test failure" [1] category
>and should return 1? ...
>
>> +	}
>> +
>> +	/* Write checks initialization. */
>> +	ret = get_full_cbm(test->resource, &full_cache_mask);
>> +	if (ret < 0)
>> +		return ret;
>> +	bit_center = count_bits(full_cache_mask) / 2;
>> +	cont_mask = full_cache_mask >> bit_center;
>> +
>> +	/* Contiguous mask write check. */
>> +	snprintf(schemata, sizeof(schemata), "%lx", cont_mask);
>> +	ret = write_schemata("", schemata, uparams->cpu, test->resource);
>> +	if (ret) {
>> +		ksft_print_msg("Write of contiguous CBM failed\n");
>> +		return ret;
>
>... although here I think the goal to distinguish between test error and test failure
>falls apart since it is not possible to tell within the test if the failure is
>because of error in the test or if test failed.

Is there even a distinction between test error and failure in resctrl selftest?
I've been looking at it for a while and can't find any instances where
ksft_test_result_error() would be used. Everywhere I look it's either pass or
fail. By grep-ing over all selftests I found only five tests that use
ksft_test_result_error().

Furthermore there is this one "TODO" in kselftests.h:

	/* TODO: how does "error" differ from "fail" or "skip"? */

If you meant the distintion less literally then I'd say the sparse_masks
comparison to CPUID would be a failure. What I had in mind is that it tries to
validate a resctrl interface relevant to non-contiguous CBMs. If it fails
there is probably something wrong with the code concerning non-contiguous CBMs.
On the other hand writing contiguous CBMs shouldn't fail as far as the
non-contiguous CBMs in CAT test is concerned. So if that fails there might be
something wrong on a higher level and I'd say that can be more of an error than
a failure.

But I'm just saying how I undestood it so far. If there is some clear
distinction between error and failure definitions I could try to separate it
more explicitly.

>
>Reinette
>
>[1] https://lore.kernel.org/all/33787043-5823-6de4-4e5c-a24a136ba541@linux.intel.com/
>

-- 
Kind regards
Maciej Wieczór-Retman

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

[Reinette Chatre]

Hi Maciej,

On 1/31/2024 4:55 AM, Maciej Wieczor-Retman wrote:
> On 2024-01-26 at 13:10:18 -0800, Reinette Chatre wrote:
>> On 1/25/2024 3:13 AM, Maciej Wieczor-Retman wrote:
>>> Add tests for both L2 and L3 CAT to verify the return values
>>> generated by writing non-contiguous CBMs don't contradict the
>>> reported non-contiguous support information.
>>>
>>> Use a logical XOR to confirm return value of write_schemata() and
>>> non-contiguous CBMs support information match.
>>>
>>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>>> ---
>>> Changelog v3:
>>> - Roll back __cpuid_count part. (Reinette)
>>> - Update function name to read sparse_masks file.
>>> - Roll back get_cache_level() changes.
>>> - Add ksft_print_msg() to contiguous schemata write error handling
>>>   (Reinette).
>>>
>>> Changelog v2:
>>> - Redo the patch message. (Ilpo)
>>> - Tidy up __cpuid_count calls. (Ilpo)
>>> - Remove redundant AND in noncont_mask calculations (Ilpo)
>>> - Fix bit_center offset.
>>> - Add newline before function return. (Ilpo)
>>> - Group non-contiguous tests with CAT tests. (Ilpo)
>>> - Use a helper for reading sparse_masks file. (Ilpo)
>>> - Make get_cache_level() available in other source files. (Ilpo)
>>>
>>>  tools/testing/selftests/resctrl/cat_test.c    | 81 +++++++++++++++++++
>>>  tools/testing/selftests/resctrl/resctrl.h     |  2 +
>>>  .../testing/selftests/resctrl/resctrl_tests.c |  2 +
>>>  3 files changed, 85 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
>>> index 39fc9303b8e8..9086bf359072 100644
>>> --- a/tools/testing/selftests/resctrl/cat_test.c
>>> +++ b/tools/testing/selftests/resctrl/cat_test.c
>>> @@ -294,6 +294,71 @@ static int cat_run_test(const struct resctrl_test *test, const struct user_param
>>>  	return ret;
>>>  }
>>>  
>>> +static int noncont_cat_run_test(const struct resctrl_test *test,
>>> +				const struct user_params *uparams)
>>> +{
>>> +	unsigned long full_cache_mask, cont_mask, noncont_mask;
>>> +	unsigned int eax, ebx, ecx, edx, ret, sparse_masks;
>>> +	char schemata[64];
>>> +	int bit_center;
>>> +
>>> +	/* Check to compare sparse_masks content to CPUID output. */
>>> +	ret = resource_info_unsigned_get(test->resource, "sparse_masks", &sparse_masks);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (!strcmp(test->resource, "L3"))
>>> +		__cpuid_count(0x10, 1, eax, ebx, ecx, edx);
>>> +	else if (!strcmp(test->resource, "L2"))
>>> +		__cpuid_count(0x10, 2, eax, ebx, ecx, edx);
>>> +	else
>>> +		return -EINVAL;
>>> +
>>> +	if (sparse_masks != ((ecx >> 3) & 1)) {
>>> +		ksft_print_msg("CPUID output doesn't match 'sparse_masks' file content!\n");
>>> +		return -1;
>>
>> If I understand correctly this falls into the "test failure" [1] category
>> and should return 1? ...
>>
>>> +	}
>>> +
>>> +	/* Write checks initialization. */
>>> +	ret = get_full_cbm(test->resource, &full_cache_mask);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +	bit_center = count_bits(full_cache_mask) / 2;
>>> +	cont_mask = full_cache_mask >> bit_center;
>>> +
>>> +	/* Contiguous mask write check. */
>>> +	snprintf(schemata, sizeof(schemata), "%lx", cont_mask);
>>> +	ret = write_schemata("", schemata, uparams->cpu, test->resource);
>>> +	if (ret) {
>>> +		ksft_print_msg("Write of contiguous CBM failed\n");
>>> +		return ret;
>>
>> ... although here I think the goal to distinguish between test error and test failure
>> falls apart since it is not possible to tell within the test if the failure is
>> because of error in the test or if test failed.
> 
> Is there even a distinction between test error and failure in resctrl selftest?

There is such a distinction in the current tests (and from what I understand the reason
behind the logical XOR used in this test) . In existing tests the running of
the test precedes and is clearly separate from determining of the test pass/fail.
All the current tests have a clear "run the test" phase where data is collected to
a file, followed by an analysis (aka "check results") phase that looks at collected
data to determine if the test passes or fails.
Note how all the "check results" return either 0 or 1 to indicate test pass
or fail respectively. Specifically, you can refer to:
mbm_test.c->check_results()
mba_test.c->check_results()
cmt_test.c->check_results()
cat_test.c->check_results()

> I've been looking at it for a while and can't find any instances where
> ksft_test_result_error() would be used. Everywhere I look it's either pass or
> fail. By grep-ing over all selftests I found only five tests that use
> ksft_test_result_error().

Yes, from the user perspective there is no such distinction. This seems to
be entirely internal to the resctrl selftests (but I do not think that this
should or can be a hard requirement).

> 
> Furthermore there is this one "TODO" in kselftests.h:
> 
> 	/* TODO: how does "error" differ from "fail" or "skip"? */
> 
> If you meant the distintion less literally then I'd say the sparse_masks
> comparison to CPUID would be a failure. What I had in mind is that it tries to
> validate a resctrl interface relevant to non-contiguous CBMs. If it fails
> there is probably something wrong with the code concerning non-contiguous CBMs.

Wrong with which code? As I understand this particular check compares the
resctrl view of the world to the hardware realities. If this check fails
then I do not think this is an issue with the test code (which would make it a test
error) but instead a resctrl bug and thus a test failure.

> On the other hand writing contiguous CBMs shouldn't fail as far as the
> non-contiguous CBMs in CAT test is concerned. So if that fails there might be
> something wrong on a higher level and I'd say that can be more of an error than
> a failure.

I think that the write_schemata() can fail for a variety of reasons, some may
indicate an issue with the test while some may indicate an issue with resctrl.
It is not possible for the caller of write_schemata() to distinguish.

> But I'm just saying how I undestood it so far. If there is some clear
> distinction between error and failure definitions I could try to separate it
> more explicitly.

I do not think it is possible to clearly distinguish between error and failure.
These are already lumped together as a ksft_test_result_fail() anyway so no
risk of confusion to folks just running the tests.
I think the final test result may be confusing to folks parsing the
resctrl selftest internals:

	run_single_test()
	{
		...
		ret = test->run_test(test, uparams);
		ksft_test_result(!ret, "%s: test\n", test->name);
		...
	}

above means that a test returning negative or greater than zero value is
considered a test failure and resctrl tests may return either in the case of
an actual test failure ... but from user perspective there is no difference
so I do not think it is an issue, just lack of consistency in the resctrl
test internals in cases like write_schemata() failure where a possible
test fail is captured as a test error. 

I do not think it is required to be strict here. Keeping "test returns
negative or greater than zero on test failure" seems reasonable to me.

Reinette

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

[Maciej Wieczor-Retman]

Hello!

On 2024-02-01 at 11:47:44 -0800, Reinette Chatre wrote:
>Hi Maciej,
>
>On 1/31/2024 4:55 AM, Maciej Wieczor-Retman wrote:
>> On 2024-01-26 at 13:10:18 -0800, Reinette Chatre wrote:
>>> On 1/25/2024 3:13 AM, Maciej Wieczor-Retman wrote:
>>>> +	if (sparse_masks != ((ecx >> 3) & 1)) {
>>>> +		ksft_print_msg("CPUID output doesn't match 'sparse_masks' file content!\n");
>>>> +		return -1;
>>>
>>> If I understand correctly this falls into the "test failure" [1] category
>>> and should return 1? ...
>>>
>>>> +	}
>>>> +
>>>> +	/* Write checks initialization. */
>>>> +	ret = get_full_cbm(test->resource, &full_cache_mask);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +	bit_center = count_bits(full_cache_mask) / 2;
>>>> +	cont_mask = full_cache_mask >> bit_center;
>>>> +
>>>> +	/* Contiguous mask write check. */
>>>> +	snprintf(schemata, sizeof(schemata), "%lx", cont_mask);
>>>> +	ret = write_schemata("", schemata, uparams->cpu, test->resource);
>>>> +	if (ret) {
>>>> +		ksft_print_msg("Write of contiguous CBM failed\n");
>>>> +		return ret;
>>>
>>> ... although here I think the goal to distinguish between test error and test failure
>>> falls apart since it is not possible to tell within the test if the failure is
>>> because of error in the test or if test failed.
>> 
>> Is there even a distinction between test error and failure in resctrl selftest?
>
>There is such a distinction in the current tests (and from what I understand the reason
>behind the logical XOR used in this test) . In existing tests the running of
>the test precedes and is clearly separate from determining of the test pass/fail.
>All the current tests have a clear "run the test" phase where data is collected to
>a file, followed by an analysis (aka "check results") phase that looks at collected
>data to determine if the test passes or fails.
>Note how all the "check results" return either 0 or 1 to indicate test pass
>or fail respectively. Specifically, you can refer to:
>mbm_test.c->check_results()
>mba_test.c->check_results()
>cmt_test.c->check_results()
>cat_test.c->check_results()
>
>> I've been looking at it for a while and can't find any instances where
>> ksft_test_result_error() would be used. Everywhere I look it's either pass or
>> fail. By grep-ing over all selftests I found only five tests that use
>> ksft_test_result_error().
>
>Yes, from the user perspective there is no such distinction. This seems to
>be entirely internal to the resctrl selftests (but I do not think that this
>should or can be a hard requirement).

Okay, thank you, that's what I wanted to know.

>
>> 
>> Furthermore there is this one "TODO" in kselftests.h:
>> 
>> 	/* TODO: how does "error" differ from "fail" or "skip"? */
>> 
>> If you meant the distintion less literally then I'd say the sparse_masks
>> comparison to CPUID would be a failure. What I had in mind is that it tries to
>> validate a resctrl interface relevant to non-contiguous CBMs. If it fails
>> there is probably something wrong with the code concerning non-contiguous CBMs.
>
>Wrong with which code? As I understand this particular check compares the
>resctrl view of the world to the hardware realities. If this check fails
>then I do not think this is an issue with the test code (which would make it a test
>error) but instead a resctrl bug and thus a test failure.

I also meant a resctrl bug. I was thinking about the kernel resctrl code that
handles taking the CPUID information about non-contiguous CBMs and putting it in
the sparse_masks file.

If there was a hardware problem and CPUID returned wrong information, then the
check wouldn't fail as sparse_masks relies on CPUID too and both values would
match. So in view of this I thought that this check could make sure that the
resctrl kernel code handles CPUID returned information properly.

So should this check be moved from the "run the test" phase to the end of the
function ("check results" phase) to signify that it's not an error but a
failure?

>
>> On the other hand writing contiguous CBMs shouldn't fail as far as the
>> non-contiguous CBMs in CAT test is concerned. So if that fails there might be
>> something wrong on a higher level and I'd say that can be more of an error than
>> a failure.
>
>I think that the write_schemata() can fail for a variety of reasons, some may
>indicate an issue with the test while some may indicate an issue with resctrl.
>It is not possible for the caller of write_schemata() to distinguish.
>
>> But I'm just saying how I undestood it so far. If there is some clear
>> distinction between error and failure definitions I could try to separate it
>> more explicitly.
>
>I do not think it is possible to clearly distinguish between error and failure.
>These are already lumped together as a ksft_test_result_fail() anyway so no
>risk of confusion to folks just running the tests.
>I think the final test result may be confusing to folks parsing the
>resctrl selftest internals:
>
>	run_single_test()
>	{
>		...
>		ret = test->run_test(test, uparams);
>		ksft_test_result(!ret, "%s: test\n", test->name);
>		...
>	}
>
>above means that a test returning negative or greater than zero value is
>considered a test failure and resctrl tests may return either in the case of
>an actual test failure ... but from user perspective there is no difference
>so I do not think it is an issue, just lack of consistency in the resctrl
>test internals in cases like write_schemata() failure where a possible
>test fail is captured as a test error. 
>
>I do not think it is required to be strict here. Keeping "test returns
>negative or greater than zero on test failure" seems reasonable to me.

Okay, so the approach I applied in noncont_cat_run_test() with write_schemata()
is acceptable?

>
>Reinette

-- 
Kind regards
Maciej Wieczór-Retman

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

[Reinette Chatre]

Hi Maciej,

On 2/2/2024 2:17 AM, Maciej Wieczor-Retman wrote:
> On 2024-02-01 at 11:47:44 -0800, Reinette Chatre wrote:
>> Hi Maciej,
>>
>> On 1/31/2024 4:55 AM, Maciej Wieczor-Retman wrote:
>>> On 2024-01-26 at 13:10:18 -0800, Reinette Chatre wrote:
>>>> On 1/25/2024 3:13 AM, Maciej Wieczor-Retman wrote:
>>>>> +	if (sparse_masks != ((ecx >> 3) & 1)) {
>>>>> +		ksft_print_msg("CPUID output doesn't match 'sparse_masks' file content!\n");
>>>>> +		return -1;
>>>>
>>>> If I understand correctly this falls into the "test failure" [1] category
>>>> and should return 1? ...
>>>>
>>>>> +	}
>>>>> +
>>>>> +	/* Write checks initialization. */
>>>>> +	ret = get_full_cbm(test->resource, &full_cache_mask);
>>>>> +	if (ret < 0)
>>>>> +		return ret;
>>>>> +	bit_center = count_bits(full_cache_mask) / 2;
>>>>> +	cont_mask = full_cache_mask >> bit_center;
>>>>> +
>>>>> +	/* Contiguous mask write check. */
>>>>> +	snprintf(schemata, sizeof(schemata), "%lx", cont_mask);
>>>>> +	ret = write_schemata("", schemata, uparams->cpu, test->resource);
>>>>> +	if (ret) {
>>>>> +		ksft_print_msg("Write of contiguous CBM failed\n");
>>>>> +		return ret;
>>>>
>>>> ... although here I think the goal to distinguish between test error and test failure
>>>> falls apart since it is not possible to tell within the test if the failure is
>>>> because of error in the test or if test failed.
>>>
>>> Is there even a distinction between test error and failure in resctrl selftest?
>>
>> There is such a distinction in the current tests (and from what I understand the reason
>> behind the logical XOR used in this test) . In existing tests the running of
>> the test precedes and is clearly separate from determining of the test pass/fail.
>> All the current tests have a clear "run the test" phase where data is collected to
>> a file, followed by an analysis (aka "check results") phase that looks at collected
>> data to determine if the test passes or fails.
>> Note how all the "check results" return either 0 or 1 to indicate test pass
>> or fail respectively. Specifically, you can refer to:
>> mbm_test.c->check_results()
>> mba_test.c->check_results()
>> cmt_test.c->check_results()
>> cat_test.c->check_results()
>>
>>> I've been looking at it for a while and can't find any instances where
>>> ksft_test_result_error() would be used. Everywhere I look it's either pass or
>>> fail. By grep-ing over all selftests I found only five tests that use
>>> ksft_test_result_error().
>>
>> Yes, from the user perspective there is no such distinction. This seems to
>> be entirely internal to the resctrl selftests (but I do not think that this
>> should or can be a hard requirement).
> 
> Okay, thank you, that's what I wanted to know.
> 
>>
>>>
>>> Furthermore there is this one "TODO" in kselftests.h:
>>>
>>> 	/* TODO: how does "error" differ from "fail" or "skip"? */
>>>
>>> If you meant the distintion less literally then I'd say the sparse_masks
>>> comparison to CPUID would be a failure. What I had in mind is that it tries to
>>> validate a resctrl interface relevant to non-contiguous CBMs. If it fails
>>> there is probably something wrong with the code concerning non-contiguous CBMs.
>>
>> Wrong with which code? As I understand this particular check compares the
>> resctrl view of the world to the hardware realities. If this check fails
>> then I do not think this is an issue with the test code (which would make it a test
>> error) but instead a resctrl bug and thus a test failure.
> 
> I also meant a resctrl bug. I was thinking about the kernel resctrl code that
> handles taking the CPUID information about non-contiguous CBMs and putting it in
> the sparse_masks file.
> 
> If there was a hardware problem and CPUID returned wrong information, then the
> check wouldn't fail as sparse_masks relies on CPUID too and both values would
> match. So in view of this I thought that this check could make sure that the
> resctrl kernel code handles CPUID returned information properly.
> 
> So should this check be moved from the "run the test" phase to the end of the
> function ("check results" phase) to signify that it's not an error but a
> failure?

I do not think this test matches the "run" and "check" phases of previous tests,
unless you create a new test for every scenario checked within this test.

Just returning 1 when the check (if (sparse_masks != ((ecx >> 3) & 1))) fails
should be ok, no?

>>> On the other hand writing contiguous CBMs shouldn't fail as far as the
>>> non-contiguous CBMs in CAT test is concerned. So if that fails there might be
>>> something wrong on a higher level and I'd say that can be more of an error than
>>> a failure.
>>
>> I think that the write_schemata() can fail for a variety of reasons, some may
>> indicate an issue with the test while some may indicate an issue with resctrl.
>> It is not possible for the caller of write_schemata() to distinguish.
>>
>>> But I'm just saying how I undestood it so far. If there is some clear
>>> distinction between error and failure definitions I could try to separate it
>>> more explicitly.
>>
>> I do not think it is possible to clearly distinguish between error and failure.
>> These are already lumped together as a ksft_test_result_fail() anyway so no
>> risk of confusion to folks just running the tests.
>> I think the final test result may be confusing to folks parsing the
>> resctrl selftest internals:
>>
>> 	run_single_test()
>> 	{
>> 		...
>> 		ret = test->run_test(test, uparams);
>> 		ksft_test_result(!ret, "%s: test\n", test->name);
>> 		...
>> 	}
>>
>> above means that a test returning negative or greater than zero value is
>> considered a test failure and resctrl tests may return either in the case of
>> an actual test failure ... but from user perspective there is no difference
>> so I do not think it is an issue, just lack of consistency in the resctrl
>> test internals in cases like write_schemata() failure where a possible
>> test fail is captured as a test error. 
>>
>> I do not think it is required to be strict here. Keeping "test returns
>> negative or greater than zero on test failure" seems reasonable to me.
> 
> Okay, so the approach I applied in noncont_cat_run_test() with write_schemata()
> is acceptable?

In general I'd say a write_schemata() failure's return code will be acceptable,
but you should be consistent in this test. There are two write_schemata()
calls in this test, one treats an error return as a failure and the other treats
an error return as an error. Considering this inconsistency I would thus rather
suggest that you always treat write_schemata() error return as a test failure.

Reinette