Re: [PATCH 3/5] selftests/resctrl: Refactor feature check to use resource and feature name

From: Reinette Chatre <reinette.chatre@intel.com>
To: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>,
	"Shuah Khan" <shuah@kernel.org>,
	"Shuah Khan" <skhan@linuxfoundation.org>,
	linux-kselftest@vger.kernel.org,
	"Maciej Wieczór-Retman" <maciej.wieczor-retman@intel.com>
Cc: <linux-kernel@vger.kernel.org>,
	Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>,
	<stable@vger.kernel.org>
Subject: Re: [PATCH 3/5] selftests/resctrl: Refactor feature check to use resource and feature name
Date: Tue, 12 Sep 2023 15:09:33 -0700	[thread overview]
Message-ID: <b5a36b8a-c5c7-84a9-270e-bef4478d4bff@intel.com> (raw)
In-Reply-To: <20230911111930.16088-4-ilpo.jarvinen@linux.intel.com>

Hi Ilpo,

On 9/11/2023 4:19 AM, Ilpo Järvinen wrote:
> Feature check in validate_resctrl_feature_request() takes in the test
> name string and maps that to what to check per test.
> 
> Pass resource and feature names to validate_resctrl_feature_request()
> directly rather than deriving them from the test name inside the
> function which makes the feature check easier to extend for new test
> cases.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Cc: <stable@vger.kernel.org>

This does not seem to be stable material.

> ---
>  tools/testing/selftests/resctrl/resctrl.h     |  6 +-
>  .../testing/selftests/resctrl/resctrl_tests.c | 10 +--
>  tools/testing/selftests/resctrl/resctrlfs.c   | 69 ++++++++-----------
>  3 files changed, 34 insertions(+), 51 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index dd07463cdf48..89ced4152933 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -28,10 +28,6 @@
>  #define RESCTRL_PATH		"/sys/fs/resctrl"
>  #define PHYS_ID_PATH		"/sys/devices/system/cpu/cpu"
>  #define INFO_PATH		"/sys/fs/resctrl/info"
> -#define L3_PATH			"/sys/fs/resctrl/info/L3"
> -#define MB_PATH			"/sys/fs/resctrl/info/MB"
> -#define L3_MON_PATH		"/sys/fs/resctrl/info/L3_MON"
> -#define L3_MON_FEATURES_PATH	"/sys/fs/resctrl/info/L3_MON/mon_features"
>  
>  #define ARCH_INTEL     1
>  #define ARCH_AMD       2
> @@ -88,7 +84,7 @@ int get_resource_id(int cpu_no, int *resource_id);
>  int mount_resctrlfs(void);
>  int umount_resctrlfs(void);
>  int validate_bw_report_request(char *bw_report);
> -bool validate_resctrl_feature_request(const char *resctrl_val);
> +bool validate_resctrl_feature_request(const char *resource, const char *feature);
>  char *fgrep(FILE *inf, const char *str);
>  int taskset_benchmark(pid_t bm_pid, int cpu_no);
>  void run_benchmark(int signum, siginfo_t *info, void *ucontext);
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index 3d66fbdc2df3..3052394ca884 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -84,7 +84,9 @@ static void run_mbm_test(const char * const *benchmark_cmd, int cpu_no)
>  		return;
>  	}
>  
> -	if (!validate_resctrl_feature_request(MBM_STR) || (get_vendor() != ARCH_INTEL)) {
> +	if (!validate_resctrl_feature_request("L3_MON", "mbm_total_bytes") ||
> +	    !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes") ||
> +	    (get_vendor() != ARCH_INTEL)) {
>  		ksft_test_result_skip("Hardware does not support MBM or MBM is disabled\n");
>  		goto umount;
>  	}
> @@ -116,7 +118,7 @@ static void run_mba_test(const char * const *benchmark_cmd, int cpu_no)
>  		return;
>  	}
>  
> -	if (!validate_resctrl_feature_request(MBA_STR) || (get_vendor() != ARCH_INTEL)) {
> +	if (!validate_resctrl_feature_request("MB", NULL) || (get_vendor() != ARCH_INTEL)) {
>  		ksft_test_result_skip("Hardware does not support MBA or MBA is disabled\n");
>  		goto umount;
>  	}
> @@ -146,7 +148,7 @@ static void run_cmt_test(const char * const *benchmark_cmd, int cpu_no)
>  		return;
>  	}
>  
> -	if (!validate_resctrl_feature_request(CMT_STR)) {
> +	if (!validate_resctrl_feature_request("L3_MON", "llc_occupancy")) {
>  		ksft_test_result_skip("Hardware does not support CMT or CMT is disabled\n");
>  		goto umount;
>  	}
> @@ -178,7 +180,7 @@ static void run_cat_test(int cpu_no, int no_of_bits)
>  		return;
>  	}
>  
> -	if (!validate_resctrl_feature_request(CAT_STR)) {
> +	if (!validate_resctrl_feature_request("L3", NULL)) {
>  		ksft_test_result_skip("Hardware does not support CAT or CAT is disabled\n");
>  		goto umount;
>  	}
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index bd36ee206602..bd547a10791c 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -10,6 +10,8 @@
>   */
>  #include "resctrl.h"
>  
> +#include <limits.h>
> +

Could you please include <limits.h> before the local resctrl.h?

>  static int find_resctrl_mount(char *buffer)
>  {
>  	FILE *mounts;
> @@ -604,63 +606,46 @@ char *fgrep(FILE *inf, const char *str)
>  
>  /*
>   * validate_resctrl_feature_request - Check if requested feature is valid.
> - * @resctrl_val:	Requested feature
> + * @resource:	Required resource (e.g., MB, L3, L2, L3_MON, etc.)
> + * @feature:	Feature to be checked under resource (can be NULL). This path
> + *		is relative to the resource path.

I do not think "this path" is accurate. @feature is not a path but an entry
within the mon_features file.

Also please note that mon_features only exists for L3_MON, none of the other
listed resources have an associated mon_features file in resctrl. This
function is created to be generic has specific requirements on what
valid (never checked) parameters should be. This may be ok with the usage
but it should not pretend to be generic.

>   *
> - * Return: True if the feature is supported, else false. False is also
> - *         returned if resctrl FS is not mounted.
> + * Return: True if the resource/feature is supported, else false. False is
> + *         also returned if resctrl FS is not mounted.
>   */
> -bool validate_resctrl_feature_request(const char *resctrl_val)
> +bool validate_resctrl_feature_request(const char *resource, const char *feature)
>  {
>  	struct stat statbuf;
> -	bool found = false;
> +	char res_path[PATH_MAX];

Please maintain reverse fir order.

>  	char *res;
>  	FILE *inf;
>  	int ret;
>  
> -	if (!resctrl_val)
> +	if (!resource)
>  		return false;
>  
>  	ret = find_resctrl_mount(NULL);
>  	if (ret)
>  		return false;
>  
> -	if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR))) {
> -		if (!stat(L3_PATH, &statbuf))
> -			return true;
> -	} else if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
> -		if (!stat(MB_PATH, &statbuf))
> -			return true;
> -	} else if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
> -		   !strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) {
> -		if (!stat(L3_MON_PATH, &statbuf)) {
> -			inf = fopen(L3_MON_FEATURES_PATH, "r");
> -			if (!inf)
> -				return false;
> -
> -			if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) {
> -				res = fgrep(inf, "llc_occupancy");
> -				if (res) {
> -					found = true;
> -					free(res);
> -				}
> -			}
> -
> -			if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR))) {
> -				res = fgrep(inf, "mbm_total_bytes");
> -				if (res) {
> -					free(res);
> -					res = fgrep(inf, "mbm_local_bytes");
> -					if (res) {
> -						found = true;
> -						free(res);
> -					}
> -				}
> -			}
> -			fclose(inf);
> -		}
> -	}
> +	snprintf(res_path, sizeof(res_path), "%s/%s", INFO_PATH, resource);
> +
> +	if (stat(res_path, &statbuf))
> +		return false;
> +
> +	if (!feature)
> +		return true;
> +
> +	snprintf(res_path, sizeof(res_path), "%s/%s/mon_features", INFO_PATH, resource);
> +	inf = fopen(res_path, "r");
> +	if (!inf)
> +		return false;
> +
> +	res = fgrep(inf, feature);
> +	free(res);
> +	fclose(inf);
>  
> -	return found;
> +	return res;

This is unexpected. Function should return bool but instead returns a char * that
has been freed?

>  }
>  
>  int filter_dmesg(void)

Reinette