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-ret...@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.prak...@intel.com>,
> * Fenghua Yu <fenghua...@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
