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