[v2,3/4] selftests/resctrl: Split validate_resctrl_feature_request()
Commit Message
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. There exists a different way to
represent feature support and that is by the presence of 0 or 1 in
single file in the info/resource directory. In this case the filename
represents what feature support is being indicated.
Split validate_resctrl_feature_request() into three smaller functions
that each accomplish one check:
- Resource directory presence in the /sys/fs/resctrl/info directory.
- Feature name presence in the /sys/fs/resctrl/info/RESOURCE/mon_features file.
- Feature file presence in a given /sys/fs/resctrl/info/RESOURCE directory.
Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
---
Changelog v2:
- Added this patch.
tools/testing/selftests/resctrl/cat_test.c | 2 -
tools/testing/selftests/resctrl/cmt_test.c | 4 +-
tools/testing/selftests/resctrl/mba_test.c | 5 +-
tools/testing/selftests/resctrl/mbm_test.c | 6 +--
tools/testing/selftests/resctrl/resctrl.h | 6 ++-
tools/testing/selftests/resctrl/resctrlfs.c | 59 +++++++++++++++++----
6 files changed, 63 insertions(+), 19 deletions(-)
Comments
Hi Maciej,
On 12/12/2023 6:52 AM, 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. There exists a different way to
> represent feature support and that is by the presence of 0 or 1 in
> single file in the info/resource directory. In this case the filename
> represents what feature support is being indicated.
>
> Split validate_resctrl_feature_request() into three smaller functions
> that each accomplish one check:
> - Resource directory presence in the /sys/fs/resctrl/info directory.
> - Feature name presence in the /sys/fs/resctrl/info/RESOURCE/mon_features file.
> - Feature file presence in a given /sys/fs/resctrl/info/RESOURCE directory.
Please present refactoring of existing code and introduction of new
feature as separate patches.
>
> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> ---
> Changelog v2:
> - Added this patch.
>
> tools/testing/selftests/resctrl/cat_test.c | 2 -
> tools/testing/selftests/resctrl/cmt_test.c | 4 +-
> tools/testing/selftests/resctrl/mba_test.c | 5 +-
> tools/testing/selftests/resctrl/mbm_test.c | 6 +--
> tools/testing/selftests/resctrl/resctrl.h | 6 ++-
> tools/testing/selftests/resctrl/resctrlfs.c | 59 +++++++++++++++++----
> 6 files changed, 63 insertions(+), 19 deletions(-)
>
> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
> index 39fc9303b8e8..7dc7206b3b99 100644
> --- a/tools/testing/selftests/resctrl/cat_test.c
> +++ b/tools/testing/selftests/resctrl/cat_test.c
> @@ -1,9 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0
> /*
> * Cache Allocation Technology (CAT) test
> - *
> * Copyright (C) 2018 Intel Corporation
> - *
> * Authors:
> * Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>,
> * Fenghua Yu <fenghua.yu@intel.com>
Some unrelated changes here.
> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
> index dd5ca343c469..7b63aec8e2c4 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("L3_MON", "llc_occupancy") &&
> + resctrl_resource_exists("L3");
> }
>
> struct resctrl_test cmt_test = {
> diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
> index da256d2dbe5c..ecf1c186448d 100644
> --- a/tools/testing/selftests/resctrl/mba_test.c
> +++ b/tools/testing/selftests/resctrl/mba_test.c
> @@ -170,8 +170,9 @@ 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("L3_MON",
> + "mbm_local_bytes");
> }
>
> struct resctrl_test mba_test = {
> diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
> index 34879e7b71a0..d67ffa3ec63a 100644
> --- a/tools/testing/selftests/resctrl/mbm_test.c
> +++ b/tools/testing/selftests/resctrl/mbm_test.c
> @@ -97,7 +97,7 @@ static int mbm_setup(const struct resctrl_test *test,
> return END_OF_TESTS;
>
> /* Set up shemata with 100% allocation on the first run. */
> - if (p->num_of_runs == 0 && validate_resctrl_feature_request("MB", NULL))
> + if (p->num_of_runs == 0 && resctrl_resource_exists("MB"))
> ret = write_schemata(p->ctrlgrp, "100", uparams->cpu, test->resource);
>
> p->num_of_runs++;
> @@ -140,8 +140,8 @@ static int mbm_run_test(const struct resctrl_test *test, const struct user_param
>
> static bool mbm_feature_check(const struct resctrl_test *test)
> {
> - return validate_resctrl_feature_request("L3_MON", "mbm_total_bytes") &&
> - validate_resctrl_feature_request("L3_MON", "mbm_local_bytes");
> + return resctrl_mon_feature_exists("L3_MON", "mbm_total_bytes") &&
> + resctrl_mon_feature_exists("L3_MON", "mbm_local_bytes");
> }
>
> struct resctrl_test mbm_test = {
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index 8f72d94b9cbe..74041a35d4ba 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -135,7 +135,11 @@ int get_domain_id(const char *resource, int cpu_no, int *domain_id);
> int mount_resctrlfs(void);
> int umount_resctrlfs(void);
> int validate_bw_report_request(char *bw_report);
> -bool validate_resctrl_feature_request(const char *resource, const char *feature);
> +bool resctrl_resource_exists(const char *resource);
> +bool resctrl_mon_feature_exists(const char *resource,
> + const char *feature);
> +bool resctrl_cache_feature_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 70333440ff2f..8546421f0940 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -697,20 +697,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)
> @@ -725,6 +721,25 @@ bool validate_resctrl_feature_request(const char *resource, const char *feature)
> if (stat(res_path, &statbuf))
> return false;
>
> + return true;
> +}
> +
> +/*
> + * resctrl_mon_feature_exists - Check if requested feature is valid.
> + * @resource: Required resource (e.g., MB, L3, L2, L3_MON, etc.)
If this is intended for the monitoring resource and L3_MON (per below) is the
only valid resource then @resource cannot be all of the examples shown. Why is
the @resource argument needed?
> + * @feature: Required monitor feature (in mon_features file). Can only be
> + * set for L3_MON. Must be NULL for all other resources.
Which other resources?
> + *
> + * Return: True if the resource/feature is supported, else false. False is
> + * also returned if resctrl FS is not mounted.
> + */
> +bool resctrl_mon_feature_exists(const char *resource,
> + const char *feature)
> +{
> + char res_path[PATH_MAX];
> + char *res;
> + FILE *inf;
> +
> if (!feature)
> return true;
Doesn't this mean that resctrl_mon_feature_exists(NULL, NULL) will return true?
>
> @@ -740,9 +755,35 @@ bool validate_resctrl_feature_request(const char *resource, const char *feature)
> return !!res;
> }
>
> +/*
> + * resctrl_cache_feature_exists - Check if a file that indicates a
> + * cache related feature support is present.
Seems like this is not really specific to a cache ... it can
check for any info file related to any resource.
> + * @resource: Required cache resource (L3 or L2)
> + * @feature: Required cache feature.
This seems to assume some usage of this utility. What if it
is, for example, resource_info_file_exists() or resource_info_file_readable()?
> + *
> + * Return: True if the feature is supported, else false.
> + */
> +bool resctrl_cache_feature_exists(const char *resource,
> + const char *feature)
> +{
> + char res_path[PATH_MAX];
> + struct stat statbuf;
> +
> + if (!feature)
> + return true;
resctrl_cache_feature_exists(NULL, NULL) will return true, no?
> +
> + snprintf(res_path, sizeof(res_path), "%s/%s/%s", INFO_PATH, resource,
> + feature);
> +
> + if (stat(res_path, &statbuf))
> + return false;
I think it will be more robust to look at statbuf to learn if the file type
is correct and the file is actually readable.
> +
> + return true;
> +}
> +
> 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)
Reinette
Hi!
On 2024-01-08 at 14:38:45 -0800, Reinette Chatre wrote:
>Hi Maciej,
>
>On 12/12/2023 6:52 AM, 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. There exists a different way to
>> represent feature support and that is by the presence of 0 or 1 in
>> single file in the info/resource directory. In this case the filename
>> represents what feature support is being indicated.
>>
>> Split validate_resctrl_feature_request() into three smaller functions
>> that each accomplish one check:
>> - Resource directory presence in the /sys/fs/resctrl/info directory.
>> - Feature name presence in the /sys/fs/resctrl/info/RESOURCE/mon_features file.
>> - Feature file presence in a given /sys/fs/resctrl/info/RESOURCE directory.
>
>Please present refactoring of existing code and introduction of new
>feature as separate patches.
Sure, will do.
>>
>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>> ---
>> Changelog v2:
>> - Added this patch.
>>
>> tools/testing/selftests/resctrl/cat_test.c | 2 -
>> tools/testing/selftests/resctrl/cmt_test.c | 4 +-
>> tools/testing/selftests/resctrl/mba_test.c | 5 +-
>> tools/testing/selftests/resctrl/mbm_test.c | 6 +--
>> tools/testing/selftests/resctrl/resctrl.h | 6 ++-
>> tools/testing/selftests/resctrl/resctrlfs.c | 59 +++++++++++++++++----
>> 6 files changed, 63 insertions(+), 19 deletions(-)
>>
>> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
>> index 39fc9303b8e8..7dc7206b3b99 100644
>> --- a/tools/testing/selftests/resctrl/cat_test.c
>> +++ b/tools/testing/selftests/resctrl/cat_test.c
>> @@ -1,9 +1,7 @@
>> // SPDX-License-Identifier: GPL-2.0
>> /*
>> * Cache Allocation Technology (CAT) test
>> - *
>> * Copyright (C) 2018 Intel Corporation
>> - *
>> * Authors:
>> * Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>,
>> * Fenghua Yu <fenghua.yu@intel.com>
>
>Some unrelated changes here.
Oops, sorry, I'll remove these.
>> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
>> index 70333440ff2f..8546421f0940 100644
>> --- a/tools/testing/selftests/resctrl/resctrlfs.c
>> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
>> @@ -725,6 +721,25 @@ bool validate_resctrl_feature_request(const char *resource, const char *feature)
>> if (stat(res_path, &statbuf))
>> return false;
>>
>> + return true;
>> +}
>> +
>> +/*
>> + * resctrl_mon_feature_exists - Check if requested feature is valid.
>> + * @resource: Required resource (e.g., MB, L3, L2, L3_MON, etc.)
>
>If this is intended for the monitoring resource and L3_MON (per below) is the
>only valid resource then @resource cannot be all of the examples shown. Why is
>the @resource argument needed?
I see now that these functions turned out rather silly. I'll redo them according
to all your other comments. Thanks for pointing these out!
>> + * @feature: Required monitor feature (in mon_features file). Can only be
>> + * set for L3_MON. Must be NULL for all other resources.
>
>Which other resources?
I'll remove it and just put L3_MON in the path.
>> + *
>> + * Return: True if the resource/feature is supported, else false. False is
>> + * also returned if resctrl FS is not mounted.
>> + */
>> +bool resctrl_mon_feature_exists(const char *resource,
>> + const char *feature)
>> +{
>> + char res_path[PATH_MAX];
>> + char *res;
>> + FILE *inf;
>> +
>> if (!feature)
>> return true;
>
>Doesn't this mean that resctrl_mon_feature_exists(NULL, NULL) will return true?
I'll get rid of this check.
>>
>> @@ -740,9 +755,35 @@ bool validate_resctrl_feature_request(const char *resource, const char *feature)
>> return !!res;
>> }
>>
>> +/*
>> + * resctrl_cache_feature_exists - Check if a file that indicates a
>> + * cache related feature support is present.
>
>Seems like this is not really specific to a cache ... it can
>check for any info file related to any resource.
Right, I'll rewrite this.
>
>> + * @resource: Required cache resource (L3 or L2)
>> + * @feature: Required cache feature.
>
>This seems to assume some usage of this utility. What if it
>is, for example, resource_info_file_exists() or resource_info_file_readable()?
Yes, I think resource_info_file_exists() fits better.
>> + *
>> + * Return: True if the feature is supported, else false.
>> + */
>> +bool resctrl_cache_feature_exists(const char *resource,
>> + const char *feature)
>> +{
>> + char res_path[PATH_MAX];
>> + struct stat statbuf;
>> +
>> + if (!feature)
>> + return true;
>
>resctrl_cache_feature_exists(NULL, NULL) will return true, no?
I'll remove this check.
>> +
>> + snprintf(res_path, sizeof(res_path), "%s/%s/%s", INFO_PATH, resource,
>> + feature);
>> +
>> + if (stat(res_path, &statbuf))
>> + return false;
>
>I think it will be more robust to look at statbuf to learn if the file type
>is correct and the file is actually readable.
Could that file be unreadable or of wrong type?
Also I thought that since read_info_res_file() opens and reads that file any
errors should be handled there. Shouldn't this part of the test only return
whether the file is there or not since that indicates if something is supported
in the kernel?
>> +
>> + return true;
>> +}
>> +
>> 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)
>
>Reinette
Hi Maciej,
On 1/17/2024 1:49 AM, Maciej Wieczór-Retman wrote:
> On 2024-01-08 at 14:38:45 -0800, Reinette Chatre wrote:
>> On 12/12/2023 6:52 AM, Maciej Wieczor-Retman wrote:
>>> +
>>> + snprintf(res_path, sizeof(res_path), "%s/%s/%s", INFO_PATH, resource,
>>> + feature);
>>> +
>>> + if (stat(res_path, &statbuf))
>>> + return false;
>>
>> I think it will be more robust to look at statbuf to learn if the file type
>> is correct and the file is actually readable.
>
> Could that file be unreadable or of wrong type?
It should be readable and the correct type when all goes well. Hence the term
"more robust".
>
> Also I thought that since read_info_res_file() opens and reads that file any
> errors should be handled there. Shouldn't this part of the test only return
> whether the file is there or not since that indicates if something is supported
> in the kernel?
ok.
Reinette
@@ -1,9 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
/*
* Cache Allocation Technology (CAT) test
- *
* Copyright (C) 2018 Intel Corporation
- *
* Authors:
* Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>,
* Fenghua Yu <fenghua.yu@intel.com>
@@ -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("L3_MON", "llc_occupancy") &&
+ resctrl_resource_exists("L3");
}
struct resctrl_test cmt_test = {
@@ -170,8 +170,9 @@ 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("L3_MON",
+ "mbm_local_bytes");
}
struct resctrl_test mba_test = {
@@ -97,7 +97,7 @@ static int mbm_setup(const struct resctrl_test *test,
return END_OF_TESTS;
/* Set up shemata with 100% allocation on the first run. */
- if (p->num_of_runs == 0 && validate_resctrl_feature_request("MB", NULL))
+ if (p->num_of_runs == 0 && resctrl_resource_exists("MB"))
ret = write_schemata(p->ctrlgrp, "100", uparams->cpu, test->resource);
p->num_of_runs++;
@@ -140,8 +140,8 @@ static int mbm_run_test(const struct resctrl_test *test, const struct user_param
static bool mbm_feature_check(const struct resctrl_test *test)
{
- return validate_resctrl_feature_request("L3_MON", "mbm_total_bytes") &&
- validate_resctrl_feature_request("L3_MON", "mbm_local_bytes");
+ return resctrl_mon_feature_exists("L3_MON", "mbm_total_bytes") &&
+ resctrl_mon_feature_exists("L3_MON", "mbm_local_bytes");
}
struct resctrl_test mbm_test = {
@@ -135,7 +135,11 @@ int get_domain_id(const char *resource, int cpu_no, int *domain_id);
int mount_resctrlfs(void);
int umount_resctrlfs(void);
int validate_bw_report_request(char *bw_report);
-bool validate_resctrl_feature_request(const char *resource, const char *feature);
+bool resctrl_resource_exists(const char *resource);
+bool resctrl_mon_feature_exists(const char *resource,
+ const char *feature);
+bool resctrl_cache_feature_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);
@@ -697,20 +697,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)
@@ -725,6 +721,25 @@ bool validate_resctrl_feature_request(const char *resource, const char *feature)
if (stat(res_path, &statbuf))
return false;
+ return true;
+}
+
+/*
+ * resctrl_mon_feature_exists - Check if requested 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.
+ *
+ * Return: True if the resource/feature is supported, else false. False is
+ * also returned if resctrl FS is not mounted.
+ */
+bool resctrl_mon_feature_exists(const char *resource,
+ const char *feature)
+{
+ char res_path[PATH_MAX];
+ char *res;
+ FILE *inf;
+
if (!feature)
return true;
@@ -740,9 +755,35 @@ bool validate_resctrl_feature_request(const char *resource, const char *feature)
return !!res;
}
+/*
+ * resctrl_cache_feature_exists - Check if a file that indicates a
+ * cache related feature support is present.
+ * @resource: Required cache resource (L3 or L2)
+ * @feature: Required cache feature.
+ *
+ * Return: True if the feature is supported, else false.
+ */
+bool resctrl_cache_feature_exists(const char *resource,
+ const char *feature)
+{
+ char res_path[PATH_MAX];
+ struct stat statbuf;
+
+ if (!feature)
+ return true;
+
+ 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 validate_resctrl_feature_request(test->resource, NULL);
+ return resctrl_resource_exists(test->resource);
}
int filter_dmesg(void)