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

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