Re: [PATCH v5 1/2] selftests/resctrl: Extend CPU vendor detection

[Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx>]

On 3/23/22 2:09 AM, Shaopeng Tan wrote:
> Currently, the resctrl_tests only has a function to detect AMD vendor.
> Since when the Intel Sub-NUMA Clustering feature is enabled,
> Intel CMT and MBM counters may not be accurate,
> the resctrl_tests also need a function to detect Intel vendor.
> And in the future, resctrl_tests will need a function to detect different
> vendors, such as Arm.
>
> Extend the function to detect Intel vendor as well. Also,
> this function can be easily extended to detect other vendors.
>
> Signed-off-by: Shaopeng Tan <tan.shaopeng@xxxxxxxxxxxxxx>
> ---
>   tools/testing/selftests/resctrl/cat_test.c    |  2 +-
>   tools/testing/selftests/resctrl/resctrl.h     |  5 ++-
>   .../testing/selftests/resctrl/resctrl_tests.c | 41 ++++++++++++-------
>   tools/testing/selftests/resctrl/resctrlfs.c   |  2 +-
>   4 files changed, 33 insertions(+), 17 deletions(-)
>
> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
> index cd4f68388e0f..1c5e90c63254 100644
> --- a/tools/testing/selftests/resctrl/cat_test.c
> +++ b/tools/testing/selftests/resctrl/cat_test.c
> @@ -89,7 +89,7 @@ static int check_results(struct resctrl_val_param *param)
>   
>   	return show_cache_info(sum_llc_perf_miss, no_of_bits, param->span / 64,
>   			       MAX_DIFF, MAX_DIFF_PERCENT, NUM_OF_RUNS,
> -			       !is_amd, false);
> +			       get_vendor() == ARCH_INTEL, false);
>   }
>   
>   void cat_test_cleanup(void)
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index 1ad10c47e31d..f0ded31fb3c7 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -34,6 +34,9 @@
>   #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
> +
>   #define PARENT_EXIT(err_msg)			\
>   	do {					\
>   		perror(err_msg);		\
> @@ -75,8 +78,8 @@ struct resctrl_val_param {
>   extern pid_t bm_pid, ppid;
>   
>   extern char llc_occup_path[1024];
> -extern bool is_amd;
>   
> +int get_vendor(void);
>   bool check_resctrlfs_support(void);
>   int filter_dmesg(void);
>   int remount_resctrlfs(bool mum_resctrlfs);
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index 973f09a66e1e..3e7cdf1125df 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -13,25 +13,41 @@
>   #define BENCHMARK_ARGS		64
>   #define BENCHMARK_ARG_SIZE	64
>   
> -bool is_amd;
> -
> -void detect_amd(void)
> +static int detect_vendor(void)
>   {
>   	FILE *inf = fopen("/proc/cpuinfo", "r");
> +	int vendor_id = 0;
> +	char *s = NULL;
>   	char *res;
>   
>   	if (!inf)
> -		return;
> +		return vendor_id;
>   
>   	res = fgrep(inf, "vendor_id");
>   
> -	if (res) {
> -		char *s = strchr(res, ':');
> +	if (res)
> +		s = strchr(res, ':');
> +
> +	if (s && !strcmp(s, ": GenuineIntel\n"))
> +		vendor_id = ARCH_INTEL;
> +	else if (s && !strcmp(s, ": AuthenticAMD\n"))
> +		vendor_id = ARCH_AMD;
>   
> -		is_amd = s && !strcmp(s, ": AuthenticAMD\n");
> -		free(res);
> -	}
>   	fclose(inf);
> +	free(res);
> +	return vendor_id;
> +}
> +
> +int get_vendor(void)
> +{
> +	static int vendor = -1;
> +
> +	if (vendor == -1)
> +		vendor = detect_vendor();
> +	if (vendor == 0)
> +		ksft_print_msg("Can not get vendor info...\n");
> +
> +	return vendor;
>   }
>   
>   static void cmd_help(void)
> @@ -207,9 +223,6 @@ int main(int argc, char **argv)
>   	if (geteuid() != 0)
>   		return ksft_exit_fail_msg("Not running as root, abort testing.\n");
>   
> -	/* Detect AMD vendor */
> -	detect_amd();
> -
>   	if (has_ben) {
>   		/* Extract benchmark command from command line. */
>   		for (i = ben_ind; i < argc; i++) {
> @@ -241,10 +254,10 @@ int main(int argc, char **argv)
>   
>   	ksft_set_plan(tests ? : 4);
>   
> -	if (!is_amd && mbm_test)
> +	if ((get_vendor() == ARCH_INTEL) && mbm_test)
>   		run_mbm_test(has_ben, benchmark_cmd, span, cpu_no, bw_report);
>   
> -	if (!is_amd && mba_test)
> +	if ((get_vendor() == ARCH_INTEL) && mba_test)
>   		run_mba_test(has_ben, benchmark_cmd, span, cpu_no, bw_report);
>   
>   	if (cmt_test)
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index 5f5a166ade60..6f543e470ad4 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -106,7 +106,7 @@ int get_resource_id(int cpu_no, int *resource_id)
>   	char phys_pkg_path[1024];
>   	FILE *fp;
>   
> -	if (is_amd)
> +	if (get_vendor() == ARCH_AMD)
>   		sprintf(phys_pkg_path, "%s%d/cache/index3/id",
>   			PHYS_ID_PATH, cpu_no);
>   	else

Looks good to me.

Reviewed-by: Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx>

thanks,
-- Shuah