[PATCH v4 0/2] selftests/resctrl: Print a message if the result of MBM&CMT tests is failed on Intel cpu

[PATCH v4 0/2] selftests/resctrl: Print a message if the result of MBM&CMT tests is failed on Intel cpu

[Shaopeng Tan]

Hello,

The aim of this series is to print a message to let users know a possible
cause of failure, if the result of MBM&CMT tests is failed on Intel cpu.
In order to detect Intel vendor, I extended amd vendor detect function.

Difference from v3:
I separated the patch into two patches.
-In order to detect intel vendor, extended the amd vendor detect function. 
 [PATCH V4 1/2]
-The same as before that print a message if the result of MBM&CMT tests
 is failed on Intel cpu.[PATCH V4 2/2]

https://lore.kernel.org/lkml/20220304103957.487660-1-tan.shaopeng@jp.fujitsu.com/

This patch series is based on v5.17-rc8.

Shaopeng Tan (2):
  selftest/resctrl: Extend cpu vendor detection
  selftests/resctrl: Print a message if the result of MBM&CMT tests is
    failed on Intel cpu

 tools/testing/selftests/resctrl/cat_test.c    |  2 +-
 tools/testing/selftests/resctrl/resctrl.h     |  5 ++-
 .../testing/selftests/resctrl/resctrl_tests.c | 45 +++++++++++++------
 tools/testing/selftests/resctrl/resctrlfs.c   |  2 +-
 4 files changed, 37 insertions(+), 17 deletions(-)

-- 
2.27.0

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

[Shaopeng Tan]

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 venders.

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);
 }
 
 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;
 
 	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_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);
 
 	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
-- 
2.27.0

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

[Reinette Chatre]

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

[PATCH v4 2/2] selftests/resctrl: Print a message if the result of MBM&CMT tests is failed on Intel cpu

[Shaopeng Tan]

According to "Intel Resource Director Technology (Intel RDT) on
2nd Generation Intel Xeon Scalable Processors Reference Manual",
When the Intel Sub-NUMA Clustering(SNC) feature is enabled,
Intel CMT and MBM counters may not be accurate.

However, there does not seem to be an architectural way to detect
if SNC is enabled.

If the result of MBM&CMT test fails on Intel CPU,
print a message to let users know a possible cause of failure.

Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
---
 tools/testing/selftests/resctrl/resctrl_tests.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index dc548d3e2454..97ad6cfee6ee 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -86,6 +86,8 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, int span,
 		sprintf(benchmark_cmd[5], "%s", MBA_STR);
 	res = mbm_bw_change(span, cpu_no, bw_report, benchmark_cmd);
 	ksft_test_result(!res, "MBM: bw change\n");
+	if ((get_vendor() == ARCH_INTEL) && res)
+		ksft_print_msg("Intel MBM may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");
 	mbm_test_cleanup();
 }
 
@@ -122,6 +124,8 @@ static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no)
 		sprintf(benchmark_cmd[5], "%s", CMT_STR);
 	res = cmt_resctrl_val(cpu_no, 5, benchmark_cmd);
 	ksft_test_result(!res, "CMT: test\n");
+	if ((get_vendor() == ARCH_INTEL) && res)
+		ksft_print_msg("Intel CMT may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");
 	cmt_test_cleanup();
 }
 
-- 
2.27.0

Re: [PATCH v4 2/2] selftests/resctrl: Print a message if the result of MBM&CMT tests is failed on Intel cpu

[Reinette Chatre]

Hi Shaopeng Tan, 

Please use capitalization for CPU (in subject).

On 3/15/2022 10:59 PM, Shaopeng Tan wrote:
> According to "Intel Resource Director Technology (Intel RDT) on
> 2nd Generation Intel Xeon Scalable Processors Reference Manual",
> When the Intel Sub-NUMA Clustering(SNC) feature is enabled,
> Intel CMT and MBM counters may not be accurate.
> 
> However, there does not seem to be an architectural way to detect
> if SNC is enabled.
> 
> If the result of MBM&CMT test fails on Intel CPU,
> print a message to let users know a possible cause of failure.
> 
> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> ---
>  tools/testing/selftests/resctrl/resctrl_tests.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index dc548d3e2454..97ad6cfee6ee 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -86,6 +86,8 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, int span,
>  		sprintf(benchmark_cmd[5], "%s", MBA_STR);
>  	res = mbm_bw_change(span, cpu_no, bw_report, benchmark_cmd);
>  	ksft_test_result(!res, "MBM: bw change\n");
> +	if ((get_vendor() == ARCH_INTEL) && res)
> +		ksft_print_msg("Intel MBM may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");
>  	mbm_test_cleanup();
>  }
>  
> @@ -122,6 +124,8 @@ static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no)
>  		sprintf(benchmark_cmd[5], "%s", CMT_STR);
>  	res = cmt_resctrl_val(cpu_no, 5, benchmark_cmd);
>  	ksft_test_result(!res, "CMT: test\n");
> +	if ((get_vendor() == ARCH_INTEL) && res)
> +		ksft_print_msg("Intel CMT may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");
>  	cmt_test_cleanup();
>  }
>  

Thank you very much for adding this.

Acked-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette