Messages in this thread | | | Subject | Re: [PATCH v4 02/17] selftests/resctrl: Clean up resctrl features check | From | Shuah Khan <> | Date | Mon, 25 Jan 2021 19:08:55 -0700 |
| |
On 11/30/20 1:19 PM, Fenghua Yu wrote: > Checking resctrl features call strcmp() to compare feature strings > (e.g. "mba", "cat" etc). The checkings are error prone and don't have > good coding style. Define the constant strings in macros and call > strncmp() to solve the potential issues. > > Suggested-by: Shuah Khan <shuah@kernel.org> > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com> > --- > tools/testing/selftests/resctrl/cache.c | 8 +++--- > tools/testing/selftests/resctrl/cat_test.c | 2 +- > tools/testing/selftests/resctrl/cqm_test.c | 2 +- > tools/testing/selftests/resctrl/fill_buf.c | 4 +-- > tools/testing/selftests/resctrl/mba_test.c | 2 +- > tools/testing/selftests/resctrl/mbm_test.c | 2 +- > tools/testing/selftests/resctrl/resctrl.h | 25 +++++++++++++++++++ > .../testing/selftests/resctrl/resctrl_tests.c | 12 ++++----- > tools/testing/selftests/resctrl/resctrl_val.c | 19 ++++++-------- > tools/testing/selftests/resctrl/resctrlfs.c | 14 +++++------ > 10 files changed, 55 insertions(+), 35 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c > index 38dbf4962e33..248bf000c978 100644 > --- a/tools/testing/selftests/resctrl/cache.c > +++ b/tools/testing/selftests/resctrl/cache.c > @@ -182,7 +182,7 @@ int measure_cache_vals(struct resctrl_val_param *param, int bm_pid) > /* > * Measure cache miss from perf. > */ > - if (!strcmp(param->resctrl_val, "cat")) { > + if (is_cat(param->resctrl_val)) {
Is there reason to add this function for one line of code? Same comment for all these is_* routines in this patch.
> ret = get_llc_perf(&llc_perf_miss); > if (ret < 0) > return ret; > @@ -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 (is_cqm(param->resctrl_val)) { > ret = get_llc_occu_resctrl(&llc_occu_resc); > if (ret < 0) > return ret; > @@ -234,7 +234,7 @@ int cat_val(struct resctrl_val_param *param) > if (ret) > return ret; > > - if ((strcmp(resctrl_val, "cat") == 0)) { > + if (is_cat(resctrl_val)) { > ret = initialize_llc_perf(); > if (ret) > return ret; > @@ -242,7 +242,7 @@ int cat_val(struct resctrl_val_param *param) > > /* Test runs until the callback setup() tells the test to stop. */ > while (1) { > - if (strcmp(resctrl_val, "cat") == 0) { > + if (is_cat(resctrl_val)) { > ret = param->setup(1, param); > if (ret) { > ret = 0; > diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c > index 7f723bd8f328..6d9a41f3939a 100644 > --- a/tools/testing/selftests/resctrl/cat_test.c > +++ b/tools/testing/selftests/resctrl/cat_test.c > @@ -160,7 +160,7 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) > return -1; > > struct resctrl_val_param param = { > - .resctrl_val = "cat", > + .resctrl_val = CAT_STR, > .cpu_no = cpu_no, > .mum_resctrlfs = 0, > .setup = cat_setup, > diff --git a/tools/testing/selftests/resctrl/cqm_test.c b/tools/testing/selftests/resctrl/cqm_test.c > index b6af940ccfc2..6635b24a74cc 100644 > --- a/tools/testing/selftests/resctrl/cqm_test.c > +++ b/tools/testing/selftests/resctrl/cqm_test.c > @@ -142,7 +142,7 @@ int cqm_resctrl_val(int cpu_no, int n, char **benchmark_cmd) > } > > struct resctrl_val_param param = { > - .resctrl_val = "cqm", > + .resctrl_val = CQM_STR, > .ctrlgrp = "c1", > .mongrp = "m1", > .cpu_no = cpu_no, > diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c > index 79c611c99a3d..bece8bb4b575 100644 > --- a/tools/testing/selftests/resctrl/fill_buf.c > +++ b/tools/testing/selftests/resctrl/fill_buf.c > @@ -115,7 +115,7 @@ static int fill_cache_read(unsigned char *start_ptr, unsigned char *end_ptr, > > while (1) { > ret = fill_one_span_read(start_ptr, end_ptr); > - if (!strcmp(resctrl_val, "cat")) > + if (is_cat(resctrl_val)) > break; > } > > @@ -134,7 +134,7 @@ static int fill_cache_write(unsigned char *start_ptr, unsigned char *end_ptr, > { > while (1) { > fill_one_span_write(start_ptr, end_ptr); > - if (!strcmp(resctrl_val, "cat")) > + if (is_cat(resctrl_val)) > break; > } > > diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c > index 7bf8eaa6204b..6449fbd96096 100644 > --- a/tools/testing/selftests/resctrl/mba_test.c > +++ b/tools/testing/selftests/resctrl/mba_test.c > @@ -141,7 +141,7 @@ void mba_test_cleanup(void) > int mba_schemata_change(int cpu_no, char *bw_report, char **benchmark_cmd) > { > struct resctrl_val_param param = { > - .resctrl_val = "mba", > + .resctrl_val = MBA_STR, > .ctrlgrp = "c1", > .mongrp = "m1", > .cpu_no = cpu_no, > diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c > index 4700f7453f81..ec6cfe01c9c2 100644 > --- a/tools/testing/selftests/resctrl/mbm_test.c > +++ b/tools/testing/selftests/resctrl/mbm_test.c > @@ -114,7 +114,7 @@ void mbm_test_cleanup(void) > int mbm_bw_change(int span, int cpu_no, char *bw_report, char **benchmark_cmd) > { > struct resctrl_val_param param = { > - .resctrl_val = "mbm", > + .resctrl_val = MBM_STR, > .ctrlgrp = "c1", > .mongrp = "m1", > .span = span, > diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h > index 12b77182cb44..bfbc16b39a9e 100644 > --- a/tools/testing/selftests/resctrl/resctrl.h > +++ b/tools/testing/selftests/resctrl/resctrl.h > @@ -62,6 +62,31 @@ struct resctrl_val_param { > int (*setup)(int num, ...); > }; > > +#define MBM_STR "mbm" > +#define MBA_STR "mba" > +#define CQM_STR "cqm" > +#define CAT_STR "cat" > +
This is fine.
> +static inline bool is_mbm(const char *str) > +{ > + return !strncmp(str, MBM_STR, 3);
Why not use sizeof(MBM_STR) instead of hardcoding? Same comment on all the other such usahes below.
> +} > + There is no need to add an entire routine for this.
> +static inline bool is_mba(const char *str) > +{ > + return !strncmp(str, MBA_STR, 3); > +} > +
There is no need to add an entire routine for this.
> +static inline bool is_cqm(const char *str) > +{ > + return !strncmp(str, CQM_STR, 3); > +} > +
There is no need to add an entire routine for this.
> +static inline bool is_cat(const char *str) > +{ > + return !strncmp(str, CAT_STR, 3); > +} > +
There is no need to add an entire routine for this.
> extern pid_t bm_pid, ppid; > extern int tests_run; > > diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c > index 425cc85ac883..f425eaf8c331 100644 > --- a/tools/testing/selftests/resctrl/resctrl_tests.c > +++ b/tools/testing/selftests/resctrl/resctrl_tests.c > @@ -85,13 +85,13 @@ int main(int argc, char **argv) > cqm_test = false; > cat_test = false; > while (token) { > - if (!strcmp(token, "mbm")) { > + if (is_mbm(token)) { > mbm_test = true; > - } else if (!strcmp(token, "mba")) { > + } else if (is_mba(token)) { > mba_test = true; > - } else if (!strcmp(token, "cqm")) { > + } else if (is_cqm(token)) { > cqm_test = true; > - } else if (!strcmp(token, "cat")) { > + } else if (is_cat(token)) { > cat_test = true; > } else { > printf("invalid argument\n"); > @@ -161,7 +161,7 @@ int main(int argc, char **argv) > if (!is_amd && mbm_test) { > printf("# Starting MBM BW change ...\n"); > if (!has_ben) > - sprintf(benchmark_cmd[5], "%s", "mba"); > + sprintf(benchmark_cmd[5], "%s", MBA_STR); > res = mbm_bw_change(span, cpu_no, bw_report, benchmark_cmd); > printf("%sok MBM: bw change\n", res ? "not " : ""); > mbm_test_cleanup(); > @@ -181,7 +181,7 @@ int main(int argc, char **argv) > if (cqm_test) { > printf("# Starting CQM test ...\n"); > if (!has_ben) > - sprintf(benchmark_cmd[5], "%s", "cqm"); > + sprintf(benchmark_cmd[5], "%s", CQM_STR); > res = cqm_resctrl_val(cpu_no, no_of_bits, benchmark_cmd); > printf("%sok CQM: test\n", res ? "not " : ""); > cqm_test_cleanup(); > diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c > index 520fea3606d1..f55e04a30a77 100644 > --- a/tools/testing/selftests/resctrl/resctrl_val.c > +++ b/tools/testing/selftests/resctrl/resctrl_val.c > @@ -397,10 +397,10 @@ static void initialize_mem_bw_resctrl(const char *ctrlgrp, const char *mongrp, > return; > } > > - if (strcmp(resctrl_val, "mbm") == 0) > + if (is_mbm(resctrl_val)) > set_mbm_path(ctrlgrp, mongrp, resource_id); > > - if ((strcmp(resctrl_val, "mba") == 0)) { > + if (is_mba(resctrl_val)) { > if (ctrlgrp) > sprintf(mbm_total_path, CON_MBM_LOCAL_BYTES_PATH, > RESCTRL_PATH, ctrlgrp, resource_id); > @@ -524,7 +524,7 @@ static void initialize_llc_occu_resctrl(const char *ctrlgrp, const char *mongrp, > return; > } > > - if (strcmp(resctrl_val, "cqm") == 0) > + if (is_cqm(resctrl_val)) > set_cqm_path(ctrlgrp, mongrp, resource_id); > } > > @@ -579,8 +579,7 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param) > if (strcmp(param->filename, "") == 0) > sprintf(param->filename, "stdio"); > > - if ((strcmp(resctrl_val, "mba")) == 0 || > - (strcmp(resctrl_val, "mbm")) == 0) { > + if (is_mba(resctrl_val) || is_mbm(resctrl_val)) { > ret = validate_bw_report_request(param->bw_report); > if (ret) > return ret; > @@ -674,15 +673,14 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param) > if (ret) > goto out; > > - if ((strcmp(resctrl_val, "mbm") == 0) || > - (strcmp(resctrl_val, "mba") == 0)) { > + if (is_mbm(resctrl_val) || is_mba(resctrl_val)) { > ret = initialize_mem_bw_imc(); > if (ret) > goto out; > > initialize_mem_bw_resctrl(param->ctrlgrp, param->mongrp, > param->cpu_no, resctrl_val); > - } else if (strcmp(resctrl_val, "cqm") == 0) > + } else if (is_cqm(resctrl_val)) > initialize_llc_occu_resctrl(param->ctrlgrp, param->mongrp, > param->cpu_no, resctrl_val); > > @@ -710,8 +708,7 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param) > > /* Test runs until the callback setup() tells the test to stop. */ > while (1) { > - if ((strcmp(resctrl_val, "mbm") == 0) || > - (strcmp(resctrl_val, "mba") == 0)) { > + if (is_mbm(resctrl_val) || is_mba(resctrl_val)) { > ret = param->setup(1, param); > if (ret) { > ret = 0; > @@ -721,7 +718,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 (is_cqm(resctrl_val)) { > 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 2a16100c9c3f..dc4f1286aefa 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 (!is_cqm(resctrl_val)) > buffer_span = span * MB; > else > buffer_span = span; > @@ -459,8 +459,7 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp, > goto out; > > /* Create mon grp and write pid into it for "mbm" and "cqm" test */ > - if ((strcmp(resctrl_val, "cqm") == 0) || > - (strcmp(resctrl_val, "mbm") == 0)) { > + if (is_cqm(resctrl_val) || is_mbm(resctrl_val)) { > if (strlen(mongrp)) { > sprintf(monitorgroup_p, "%s/mon_groups", controlgroup); > sprintf(monitorgroup, "%s/%s", monitorgroup_p, mongrp); > @@ -505,9 +504,8 @@ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val) > int resource_id, ret = 0; > FILE *fp; > > - if ((strcmp(resctrl_val, "mba") != 0) && > - (strcmp(resctrl_val, "cat") != 0) && > - (strcmp(resctrl_val, "cqm") != 0)) > + if (!is_mba(resctrl_val) && !is_cat(resctrl_val) && > + !is_cqm(resctrl_val)) > return -ENOENT; > > if (!schemata) { > @@ -528,9 +526,9 @@ 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 (is_cat(resctrl_val) || is_cqm(resctrl_val)) > sprintf(schema, "%s%d%c%s", "L3:", resource_id, '=', schemata); > - if (strcmp(resctrl_val, "mba") == 0) > + if (is_mba(resctrl_val)) > sprintf(schema, "%s%d%c%s", "MB:", resource_id, '=', schemata); > > fp = fopen(controlgroup, "w"); >
thanks, -- Shuah
| |