Re: [PATCH v4 1/2] selftest/resctrl: Extend cpu vendor detection

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Shaopeng Tan,

Thank you for adding this change.

Please note the typo (selftest vs. selftests) and capitalization
in the subject. It could read:
selftests/resctrl: Extend CPU vendor detection

On 3/15/2022 10:59 PM, 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.

Please take care of capitalization for CPU and the vendors (AMD, Arm, Intel).

> 
> Extend the function to detect intel vendor as well. Also,
> this function can be easily extended to detect other venders.

intel -> Intel
venders -> vendors

> 
> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> ---
>  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..2daa63991628 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_AMD), false);

These parenthesis should not be needed.

Now that you add better vendor detection it would be clearer to test
for Intel and not "is not AMD".

>  }
>  
>  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..dc548d3e2454 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");
>  	char *res;
> +	char *s = NULL;
> +	int vendor_id = 0;

Please order the declarations based on line length from longest
to shortest (also known as "reverse christmas tree order").

>  
>  	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;

Extra white space here.

>  
> -		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_AMD) && mbm_test)
>  		run_mbm_test(has_ben, benchmark_cmd, span, cpu_no, bw_report);
>  
> -	if (!is_amd && mba_test)
> +	if ((get_vendor() != ARCH_AMD) && mba_test)
>  		run_mba_test(has_ben, benchmark_cmd, span, cpu_no, bw_report);
>  

This would end up running the test even if vendor detection failed. Instead of
the negative test with AMD this can be changed to only run the test on
Intel platforms.

>  	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


Reinette