Message ID | 20201020235126.1871815-1-fenghua.yu@intel.com |
---|---|
Headers | show |
Series | Miscellaneous fixes for resctrl selftests | expand |
On 10/20/20 5:51 PM, Fenghua Yu wrote: > CMT (Cache Monitoring Technology) [1] is a H/W feature that reports cache > occupancy of a process. resctrl selftest suite has a unit test to test CMT > for LLC but the test is named as CQM (Cache Quality Monitoring). > Furthermore, the unit test source file is named as cqm_test.c and several > functions, variables, comments, preprocessors and statements widely use > "cqm" as either suffix or prefix. This rampant misusage of CQM for CMT > might confuse someone who is newly looking at resctrl selftests because > this feature is named CMT in the Intel Software Developer's Manual. > > Hence, rename all the occurrences (unit test source file name, functions, > variables, comments and preprocessors) of cqm with cmt. > > [1] Please see Intel SDM, Volume 3, chapter 17 and section 18 for more > information on CMT. Link please? > > Fixes: 78941183d1b1 ("selftests/resctrl: Add Cache QoS Monitoring (CQM) selftest") Why is this a fix though? Quote the commit in the commit log and please remove Fixes tag. No need to confuse tools that look for Fixes tag to select patches for stables. > Suggested-by: Reinette Chatre <reinette.chatre@intel.com> > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com> > --- > tools/testing/selftests/resctrl/README | 4 +-- > tools/testing/selftests/resctrl/cache.c | 4 +-- > .../resctrl/{cqm_test.c => cmt_test.c} | 20 +++++++------- > tools/testing/selftests/resctrl/resctrl.h | 4 +-- > .../testing/selftests/resctrl/resctrl_tests.c | 26 +++++++++---------- > tools/testing/selftests/resctrl/resctrl_val.c | 12 ++++----- > tools/testing/selftests/resctrl/resctrlfs.c | 10 +++---- > 7 files changed, 40 insertions(+), 40 deletions(-) > rename tools/testing/selftests/resctrl/{cqm_test.c => cmt_test.c} (89%) > > diff --git a/tools/testing/selftests/resctrl/README b/tools/testing/selftests/resctrl/README > index 6e5a0ffa18e8..4b36b25b6ac0 100644 > --- a/tools/testing/selftests/resctrl/README > +++ b/tools/testing/selftests/resctrl/README > @@ -46,8 +46,8 @@ ARGUMENTS > Parameter '-h' shows usage information. > > usage: resctrl_tests [-h] [-b "benchmark_cmd [options]"] [-t test list] [-n no_of_bits] > - -b benchmark_cmd [options]: run specified benchmark for MBM, MBA and CQM default benchmark is builtin fill_buf > - -t test list: run tests specified in the test list, e.g. -t mbm, mba, cqm, cat > + -b benchmark_cmd [options]: run specified benchmark for MBM, MBA and CMT default benchmark is builtin fill_buf > + -t test list: run tests specified in the test list, e.g. -t mbm, mba, cmt, cat > -n no_of_bits: run cache tests using specified no of bits in cache bit mask > -p cpu_no: specify CPU number to run the test. 1 is default > -h: help > diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c > index 38dbf4962e33..4d73edd81293 100644 > --- a/tools/testing/selftests/resctrl/cache.c > +++ b/tools/testing/selftests/resctrl/cache.c > @@ -111,7 +111,7 @@ static int get_llc_perf(unsigned long *llc_perf_miss) > > /* > * Get LLC Occupancy as reported by RESCTRL FS > - * For CQM, > + * For CMT, > * 1. If con_mon grp and mon grp given, then read from mon grp in > * con_mon grp > * 2. If only con_mon grp given, then read from con_mon grp > @@ -192,7 +192,7 @@ int measure_cache_vals(struct resctrl_val_param *param, int bm_pid) > /* > * Measure llc occupancy from resctrl. > */ > - if (!strcmp(param->resctrl_val, "cqm")) { > + if (!strcmp(param->resctrl_val, "cmt")) { > ret = get_llc_occu_resctrl(&llc_occu_resc); > if (ret < 0) > return ret; > diff --git a/tools/testing/selftests/resctrl/cqm_test.c b/tools/testing/selftests/resctrl/cmt_test.c > similarity index 89% > rename from tools/testing/selftests/resctrl/cqm_test.c > rename to tools/testing/selftests/resctrl/cmt_test.c > index c8756152bd61..13b01e010238 100644 > --- a/tools/testing/selftests/resctrl/cqm_test.c > +++ b/tools/testing/selftests/resctrl/cmt_test.c > @@ -1,6 +1,6 @@ > // SPDX-License-Identifier: GPL-2.0 > /* > - * Cache Monitoring Technology (CQM) test > + * Cache Monitoring Technology (CMT) test > * > * Copyright (C) 2018 Intel Corporation > * > @@ -11,7 +11,7 @@ > #include "resctrl.h" > #include <unistd.h> > > -#define RESULT_FILE_NAME "result_cqm" > +#define RESULT_FILE_NAME "result_cmt" > #define NUM_OF_RUNS 5 > #define MAX_DIFF 2000000 > #define MAX_DIFF_PERCENT 15 > @@ -21,7 +21,7 @@ char cbm_mask[256]; > unsigned long long_mask; > unsigned long cache_size; > > -static int cqm_setup(int num, ...) > +static int cmt_setup(int num, ...) > { > struct resctrl_val_param *p; > va_list param; > @@ -58,7 +58,7 @@ static void show_cache_info(unsigned long sum_llc_occu_resc, int no_of_bits, > else > res = false; > > - printf("%sok CQM: diff within %d, %d\%%\n", res ? "" : "not", > + printf("%sok CMT: diff within %d, %d\%%\n", res ? "" : "not", > MAX_DIFF, (int)MAX_DIFF_PERCENT); > > printf("# diff: %ld\n", avg_diff); > @@ -106,12 +106,12 @@ static int check_results(struct resctrl_val_param *param, int no_of_bits) > return 0; > } > > -void cqm_test_cleanup(void) > +void cmt_test_cleanup(void) > { > remove(RESULT_FILE_NAME); > } > > -int cqm_resctrl_val(int cpu_no, int n, char **benchmark_cmd) > +int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd) > { > int ret, mum_resctrlfs; > > @@ -122,7 +122,7 @@ int cqm_resctrl_val(int cpu_no, int n, char **benchmark_cmd) > if (ret) > return ret; > > - if (!validate_resctrl_feature_request("cqm")) > + if (!validate_resctrl_feature_request("cmt")) > return -1; > > ret = get_cbm_mask("L3"); > @@ -145,7 +145,7 @@ int cqm_resctrl_val(int cpu_no, int n, char **benchmark_cmd) > } > > struct resctrl_val_param param = { > - .resctrl_val = "cqm", > + .resctrl_val = "cmt", > .ctrlgrp = "c1", > .mongrp = "m1", > .cpu_no = cpu_no, > @@ -154,7 +154,7 @@ int cqm_resctrl_val(int cpu_no, int n, char **benchmark_cmd) > .mask = ~(long_mask << n) & long_mask, > .span = cache_size * n / count_of_bits, > .num_of_runs = 0, > - .setup = cqm_setup, > + .setup = cmt_setup, > }; > > if (strcmp(benchmark_cmd[0], "fill_buf") == 0) > @@ -170,7 +170,7 @@ int cqm_resctrl_val(int cpu_no, int n, char **benchmark_cmd) > if (ret) > return ret; > > - cqm_test_cleanup(); > + cmt_test_cleanup(); > > return 0; > } > diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h > index 39bf59c6b9c5..68522b19b235 100644 > --- a/tools/testing/selftests/resctrl/resctrl.h > +++ b/tools/testing/selftests/resctrl/resctrl.h > @@ -98,9 +98,9 @@ void ctrlc_handler(int signum, siginfo_t *info, void *ptr); > int cat_val(struct resctrl_val_param *param); > void cat_test_cleanup(void); > int cat_perf_miss_val(int cpu_no, int no_of_bits, char *cache_type); > -int cqm_resctrl_val(int cpu_no, int n, char **benchmark_cmd); > +int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd); > unsigned int count_bits(unsigned long n); > -void cqm_test_cleanup(void); > +void cmt_test_cleanup(void); > int get_core_sibling(int cpu_no); > int measure_cache_vals(struct resctrl_val_param *param, int bm_pid); > > diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c > index 425cc85ac883..35a91cab1b88 100644 > --- a/tools/testing/selftests/resctrl/resctrl_tests.c > +++ b/tools/testing/selftests/resctrl/resctrl_tests.c > @@ -37,10 +37,10 @@ void detect_amd(void) > static void cmd_help(void) > { > printf("usage: resctrl_tests [-h] [-b \"benchmark_cmd [options]\"] [-t test list] [-n no_of_bits]\n"); > - printf("\t-b benchmark_cmd [options]: run specified benchmark for MBM, MBA and CQM"); > + printf("\t-b benchmark_cmd [options]: run specified benchmark for MBM, MBA and CMT"); > printf("\t default benchmark is builtin fill_buf\n"); > printf("\t-t test list: run tests specified in the test list, "); > - printf("e.g. -t mbm, mba, cqm, cat\n"); > + printf("e.g. -t mbm, mba, cmt, cat\n"); > printf("\t-n no_of_bits: run cache tests using specified no of bits in cache bit mask\n"); > printf("\t-p cpu_no: specify CPU number to run the test. 1 is default\n"); > printf("\t-h: help\n"); > @@ -50,13 +50,13 @@ void tests_cleanup(void) > { > mbm_test_cleanup(); > mba_test_cleanup(); > - cqm_test_cleanup(); > + cmt_test_cleanup(); > cat_test_cleanup(); > } > > int main(int argc, char **argv) > { > - bool has_ben = false, mbm_test = true, mba_test = true, cqm_test = true; > + bool has_ben = false, mbm_test = true, mba_test = true, cmt_test = true; > int res, c, cpu_no = 1, span = 250, argc_new = argc, i, no_of_bits = 5; > char *benchmark_cmd[BENCHMARK_ARGS], bw_report[64], bm_type[64]; > char benchmark_cmd_area[BENCHMARK_ARGS][BENCHMARK_ARG_SIZE]; > @@ -82,15 +82,15 @@ int main(int argc, char **argv) > > mbm_test = false; > mba_test = false; > - cqm_test = false; > + cmt_test = false; > cat_test = false; > while (token) { > if (!strcmp(token, "mbm")) { > mbm_test = true; > } else if (!strcmp(token, "mba")) { > mba_test = true; > - } else if (!strcmp(token, "cqm")) { > - cqm_test = true; > + } else if (!strcmp(token, "cmt")) { > + cmt_test = true; > } else if (!strcmp(token, "cat")) { > cat_test = true; Why not use strncmp() here. Also, this code can be cleaned up. This is error prone. It appears there is a link to these tokens and offset in the benchmark_cmd[]. > } else { > @@ -178,13 +178,13 @@ int main(int argc, char **argv) > tests_run++; > } > > - if (cqm_test) { > - printf("# Starting CQM test ...\n"); > + if (cmt_test) { > + printf("# Starting CMT test ...\n"); > if (!has_ben) > - sprintf(benchmark_cmd[5], "%s", "cqm"); > - res = cqm_resctrl_val(cpu_no, no_of_bits, benchmark_cmd); > - printf("%sok CQM: test\n", res ? "not " : ""); > - cqm_test_cleanup(); > + sprintf(benchmark_cmd[5], "%s", "cmt"); Like here. Why not add defines that make it easier to read. > + res = cmt_resctrl_val(cpu_no, no_of_bits, benchmark_cmd); > + printf("%sok CMT: test\n", res ? "not " : ""); > + cmt_test_cleanup(); > tests_run++; > } > > diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c > index 520fea3606d1..270cd95e0026 100644 > --- a/tools/testing/selftests/resctrl/resctrl_val.c > +++ b/tools/testing/selftests/resctrl/resctrl_val.c > @@ -492,7 +492,7 @@ static int print_results_bw(char *filename, int bm_pid, float bw_imc, > return 0; > } > > -static void set_cqm_path(const char *ctrlgrp, const char *mongrp, char sock_num) > +static void set_cmt_path(const char *ctrlgrp, const char *mongrp, char sock_num) > { > if (strlen(ctrlgrp) && strlen(mongrp)) > sprintf(llc_occup_path, CON_MON_LCC_OCCUP_PATH, RESCTRL_PATH, > @@ -512,7 +512,7 @@ static void set_cqm_path(const char *ctrlgrp, const char *mongrp, char sock_num) > * @ctrlgrp: Name of the control monitor group (con_mon grp) > * @mongrp: Name of the monitor group (mon grp) > * @cpu_no: CPU number that the benchmark PID is binded to > - * @resctrl_val: Resctrl feature (Eg: cat, cqm.. etc) > + * @resctrl_val: Resctrl feature (Eg: cat, cmt.. etc) > */ > static void initialize_llc_occu_resctrl(const char *ctrlgrp, const char *mongrp, > int cpu_no, char *resctrl_val) > @@ -524,8 +524,8 @@ static void initialize_llc_occu_resctrl(const char *ctrlgrp, const char *mongrp, > return; > } > > - if (strcmp(resctrl_val, "cqm") == 0) > - set_cqm_path(ctrlgrp, mongrp, resource_id); > + if (strcmp(resctrl_val, "cmt") == 0) > + set_cmt_path(ctrlgrp, mongrp, resource_id); Why aren't these strncmp()s. Also these command strings are hard coded over a over again. Why not add defines for these. > } > > static int > @@ -682,7 +682,7 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param) > > initialize_mem_bw_resctrl(param->ctrlgrp, param->mongrp, > param->cpu_no, resctrl_val); > - } else if (strcmp(resctrl_val, "cqm") == 0) > + } else if (strcmp(resctrl_val, "cmt") == 0) > initialize_llc_occu_resctrl(param->ctrlgrp, param->mongrp, > param->cpu_no, resctrl_val); > > @@ -721,7 +721,7 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param) > ret = measure_vals(param, &bw_resc_start); > if (ret) > break; > - } else if (strcmp(resctrl_val, "cqm") == 0) { > + } else if (strcmp(resctrl_val, "cmt") == 0) { > ret = param->setup(1, param); > if (ret) { > ret = 0; > diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c > index 19c0ec4045a4..727e667e2cc9 100644 > --- a/tools/testing/selftests/resctrl/resctrlfs.c > +++ b/tools/testing/selftests/resctrl/resctrlfs.c > @@ -334,7 +334,7 @@ void run_benchmark(int signum, siginfo_t *info, void *ucontext) > operation = atoi(benchmark_cmd[4]); > sprintf(resctrl_val, "%s", benchmark_cmd[5]); > > - if (strcmp(resctrl_val, "cqm") != 0) > + if (strcmp(resctrl_val, "cmt") != 0) > buffer_span = span * MB; > else > buffer_span = span; > @@ -458,8 +458,8 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp, > if (ret) > goto out; > > - /* Create mon grp and write pid into it for "mbm" and "cqm" test */ > - if ((strcmp(resctrl_val, "cqm") == 0) || > + /* Create mon grp and write pid into it for "mbm" and "cmt" test */ > + if ((strcmp(resctrl_val, "cmt") == 0) || > (strcmp(resctrl_val, "mbm") == 0)) { > if (strlen(mongrp)) { > sprintf(monitorgroup_p, "%s/mon_groups", controlgroup); > @@ -507,7 +507,7 @@ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val) > > if ((strcmp(resctrl_val, "mba") != 0) && > (strcmp(resctrl_val, "cat") != 0) && > - (strcmp(resctrl_val, "cqm") != 0)) > + (strcmp(resctrl_val, "cmt") != 0)) > return -ENOENT; > > if (!schemata) { > @@ -528,7 +528,7 @@ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val) > else > sprintf(controlgroup, "%s/schemata", RESCTRL_PATH); > > - if (!strcmp(resctrl_val, "cat") || !strcmp(resctrl_val, "cqm")) > + if (!strcmp(resctrl_val, "cat") || !strcmp(resctrl_val, "cmt")) > sprintf(schema, "%s%d%c%s", "L3:", resource_id, '=', schemata); > if (strcmp(resctrl_val, "mba") == 0) > sprintf(schema, "%s%d%c%s", "MB:", resource_id, '=', schemata); > Let's clean this test up. thanks, -- Shuah
On 10/20/20 5:51 PM, Fenghua Yu wrote: > From: Reinette Chatre <reinette.chatre@intel.com> > > The format "%sok" is used to print results of a test. If the test passes, > the empty string is printed and if the test fails "not " is printed. This > results in output of "ok" when test passes and "not ok" > when test fails. > > Fix one instance where "not" (without a space) is printed on test > failure resulting in output of "notok" on test failure. > The commit summary is misleading. It isn't typo. You are adding a space to make the message correct? > Fixes: 78941183d1b1 ("selftests/resctrl: Add Cache QoS Monitoring (CQM) selftest") > Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com> > --- > tools/testing/selftests/resctrl/cmt_test.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c > index 13b01e010238..6ffb56c6a1e2 100644 > --- a/tools/testing/selftests/resctrl/cmt_test.c > +++ b/tools/testing/selftests/resctrl/cmt_test.c > @@ -58,7 +58,7 @@ static void show_cache_info(unsigned long sum_llc_occu_resc, int no_of_bits, > else > res = false; > > - printf("%sok CMT: diff within %d, %d\%%\n", res ? "" : "not", > + printf("%sok CMT: diff within %d, %d\%%\n", res ? "" : "not ", > MAX_DIFF, (int)MAX_DIFF_PERCENT); > > printf("# diff: %ld\n", avg_diff); > thanks, -- Shuah
On 10/20/20 5:51 PM, Fenghua Yu wrote: > From: Reinette Chatre <reinette.chatre@intel.com> > > Add a missing newline to the help text printed and fixup the next line > to line it up to previous line for improved readability. > > Fixes: 78941183d1b1 ("selftests/resctrl: Add Cache QoS Monitoring (CQM) selftest") > Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com> > --- > tools/testing/selftests/resctrl/resctrl_tests.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c > index 35a91cab1b88..b2a560c0c5dc 100644 > --- a/tools/testing/selftests/resctrl/resctrl_tests.c > +++ b/tools/testing/selftests/resctrl/resctrl_tests.c > @@ -37,8 +37,8 @@ void detect_amd(void) > static void cmd_help(void) > { > printf("usage: resctrl_tests [-h] [-b \"benchmark_cmd [options]\"] [-t test list] [-n no_of_bits]\n"); > - printf("\t-b benchmark_cmd [options]: run specified benchmark for MBM, MBA and CMT"); > - printf("\t default benchmark is builtin fill_buf\n"); > + printf("\t-b benchmark_cmd [options]: run specified benchmark for MBM, MBA and CMT\n"); > + printf("\t default benchmark is builtin fill_buf\n"); > printf("\t-t test list: run tests specified in the test list, "); > printf("e.g. -t mbm, mba, cmt, cat\n"); > printf("\t-n no_of_bits: run cache tests using specified no of bits in cache bit mask\n"); > The commit summary is misleading. It isn't typo. You are adding a space to make the message correct? thanks, -- Shuah
On 10/20/20 5:51 PM, Fenghua Yu wrote: > This patch set has several miscellaneous fixes to resctrl selftest tool > that are easily visible to user. V1 had fixes to CAT test and CMT test > but they were dropped in V2 because having them here made the patchset > humongous. So, changes to CAT test and CMT test will be posted in another > patchset. > This is still a very long patch series. Several of the patches can be combined and can be rearranged. 21 patches don't seem to any specific order. > Change Log: > v3: > Address various comments (commit messages, return value on test failure, > print failure info on test failure etc) from Reinette and Tony. > [v2: https://lore.kernel.org/linux-kselftest/cover.1589835155.git.sai.praneeth.prakhya@intel.com/] > > v2: > 1. Dropped changes to CAT test and CMT test as they will be posted in a later > series. > 2. Added several other fixes > [v1: https://lore.kernel.org/linux-kselftest/cover.1583657204.git.sai.praneeth.prakhya@intel.com/] > > Fenghua Yu (18): > selftests/resctrl: Rename CQM test as CMT test > selftests/resctrl: Declare global variables as extern > selftests/resctrl: Return if resctrl file system is not supported > selftests/resctrl: Check for resctrl mount point only if resctrl FS is > supported > selftests/resctrl: Use resctrl/info for feature detection > selftests/resctrl: Fix missing options "-n" and "-p" > selftests/resctrl: Fix MBA/MBM results reporting format > selftests/resctrl: Abort running tests if not root user > selftests/resctrl: Enable gcc checks to detect buffer overflows > selftests/resctrl: Don't hard code value of "no_of_bits" variable > selftests/resctrl: Modularize resctrl test suite main() function Yes. This is a needed change. I didn't make it to this patch yet. > selftests/resctrl: Skip the test if requested resctrl feature is not > supported Commented on this patch already. Look into using config file like other tests. > selftests/resctrl: Umount resctrl FS only if mounted > selftests/resctrl: Unmount resctrl FS after running all tests > selftests/resctrl: Fix incorrect parsing of iMC counters > selftests/resctrl: Fix checking for < 0 for unsigned values > selftests/resctrl: Fix unnecessary usage of global variables > selftests/resctrl: Don't use global variable for capacity bitmask > (CBM) > > Reinette Chatre (3): > selftests/resctrl: Fix typo > selftests/resctrl: Fix typo in help text Why not combine the above two patches. The commit summary doesn't make sense. > selftests/resctrl: Ensure sibling CPU is not same as original CPU > > tools/testing/selftests/resctrl/Makefile | 2 +- > tools/testing/selftests/resctrl/README | 4 +- > tools/testing/selftests/resctrl/cache.c | 4 +- > tools/testing/selftests/resctrl/cat_test.c | 20 +-- > .../resctrl/{cqm_test.c => cmt_test.c} | 34 ++-- > tools/testing/selftests/resctrl/mba_test.c | 23 ++- > tools/testing/selftests/resctrl/mbm_test.c | 16 +- > tools/testing/selftests/resctrl/resctrl.h | 20 ++- > .../testing/selftests/resctrl/resctrl_tests.c | 156 ++++++++++++------ > tools/testing/selftests/resctrl/resctrl_val.c | 75 ++++++--- > tools/testing/selftests/resctrl/resctrlfs.c | 79 ++++++--- > 11 files changed, 272 insertions(+), 161 deletions(-) > rename tools/testing/selftests/resctrl/{cqm_test.c => cmt_test.c} (85%) > I will review rest of the patches. Try to combine a few patches and collapse fixes. I would like to see all the fixes first and then renaming from CQM to CMT. thanks, -- Shuah