RE: [PATCH v5 04/21] selftests/resctrl: Clean up resctrl features check

[Babu Moger]

> -----Original Message-----
> From: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> Sent: Sunday, March 7, 2021 8:55 AM
> To: Shuah Khan <shuah@xxxxxxxxxx>; Tony Luck <tony.luck@xxxxxxxxx>;
> Reinette Chatre <reinette.chatre@xxxxxxxxx>; Moger, Babu
> <Babu.Moger@xxxxxxx>
> Cc: linux-kselftest <linux-kselftest@xxxxxxxxxxxxxxx>; linux-kernel <linux-
> kernel@xxxxxxxxxxxxxxx>; Fenghua Yu <fenghua.yu@xxxxxxxxx>
> Subject: [PATCH v5 04/21] selftests/resctrl: Clean up resctrl features check
> 
> 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@xxxxxxxxxx>
> Signed-off-by: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> ---
> Change Log:
> v5:
> - Remove is_cat() etc functions and directly call strncmp() to check
>   the features (Shuah).
> 
>  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     |  5 +++++
>  .../testing/selftests/resctrl/resctrl_tests.c | 12 +++++-----
> tools/testing/selftests/resctrl/resctrl_val.c | 22 +++++++++----------
>  tools/testing/selftests/resctrl/resctrlfs.c   | 17 +++++++-------
>  10 files changed, 41 insertions(+), 35 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cache.c
> b/tools/testing/selftests/resctrl/cache.c
> index 38dbf4962e33..5922cc1b0386 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 (!strncmp(param->resctrl_val, CAT_STR, sizeof(CAT_STR))) {
>  		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 (!strncmp(param->resctrl_val, CQM_STR, sizeof(CQM_STR))) {
>  		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 (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR))) {
>  		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 (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR))) {
>  			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 bdeeb5772592..20823725daca 100644
> --- a/tools/testing/selftests/resctrl/cat_test.c
> +++ b/tools/testing/selftests/resctrl/cat_test.c
> @@ -164,7 +164,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 de33d1c0466e..271752e9ef5b 100644
> --- a/tools/testing/selftests/resctrl/cqm_test.c
> +++ b/tools/testing/selftests/resctrl/cqm_test.c
> @@ -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	= 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..51e5cf22632f 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 (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)))
>  			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 (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)))
>  			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..36da6136af96 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -62,6 +62,11 @@ struct resctrl_val_param {
>  	int		(*setup)(int num, ...);
>  };
> 
> +#define MBM_STR			"mbm"
> +#define MBA_STR			"mba"
> +#define CQM_STR			"cqm"
> +#define CAT_STR			"cat"
> +
>  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..4b109a59f72d 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 (!strncmp(token, MBM_STR,
> sizeof(MBM_STR))) {
>  					mbm_test = true;
> -				} else if (!strcmp(token, "mba")) {
> +				} else if (!strncmp(token, MBA_STR,
> sizeof(MBA_STR))) {
>  					mba_test = true;
> -				} else if (!strcmp(token, "cqm")) {
> +				} else if (!strncmp(token, CQM_STR,
> sizeof(CQM_STR))) {
>  					cqm_test = true;
> -				} else if (!strcmp(token, "cat")) {
> +				} else if (!strncmp(token, CAT_STR,
> sizeof(CAT_STR))) {
>  					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..aed71fd0713b 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 (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)))
>  		set_mbm_path(ctrlgrp, mongrp, resource_id);
> 
> -	if ((strcmp(resctrl_val, "mba") == 0)) {
> +	if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
>  		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 (!strncmp(resctrl_val, CQM_STR, sizeof(CQM_STR)))
>  		set_cqm_path(ctrlgrp, mongrp, resource_id);  }
> 
> @@ -579,8 +579,8 @@ 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 (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) ||
> +	    !strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR))) {
>  		ret = validate_bw_report_request(param->bw_report);
>  		if (ret)
>  			return ret;
> @@ -674,15 +674,15 @@ 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 (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
> +	    !strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
>  		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 (!strncmp(resctrl_val, CQM_STR, sizeof(CQM_STR)))
>  		initialize_llc_occu_resctrl(param->ctrlgrp, param->mongrp,
>  					    param->cpu_no, resctrl_val);
> 
> @@ -710,8 +710,8 @@ 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 (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
> +		    !strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
>  			ret = param->setup(1, param);
>  			if (ret) {
>  				ret = 0;
> @@ -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 (!strncmp(resctrl_val, CQM_STR, sizeof(CQM_STR))) {
>  			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..4174e48e06d1 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 (strncmp(resctrl_val, CQM_STR, sizeof(CQM_STR)))
>  			buffer_span = span * MB;
>  		else
>  			buffer_span = span;
> @@ -459,8 +459,8 @@ 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 (!strncmp(resctrl_val, CQM_STR, sizeof(CQM_STR)) ||
> +	    !strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR))) {
>  		if (strlen(mongrp)) {
>  			sprintf(monitorgroup_p, "%s/mon_groups",
> controlgroup);
>  			sprintf(monitorgroup, "%s/%s", monitorgroup_p,
> mongrp); @@ -505,9 +505,9 @@ 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 (strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) &&
> +	    strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)) &&
> +	    strncmp(resctrl_val, CQM_STR, sizeof(CQM_STR)))
>  		return -ENOENT;
> 
>  	if (!schemata) {
> @@ -528,9 +528,10 @@ 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 (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)) ||
> +	    !strncmp(resctrl_val, CQM_STR, sizeof(CQM_STR)))
>  		sprintf(schema, "%s%d%c%s", "L3:", resource_id, '=',
> schemata);
> -	if (strcmp(resctrl_val, "mba") == 0)
> +	if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)))
>  		sprintf(schema, "%s%d%c%s", "MB:", resource_id, '=',
> schemata);
> 
>  	fp = fopen(controlgroup, "w");
> --
> 2.30.1

I see there are few other references as well.  Like this.

1 cat_test.c  cat_perf_miss_val  135 if
(!validate_resctrl_feature_request("cat"))

2 cqm_test.c  cqm_resctrl_val                  125 if
(!validate_resctrl_feature_request("cqm"))

3 mba_test.c  mba_schemata_change    157 if
(!validate_resctrl_feature_request("mba"))

4 mbm_test.c  mbm_bw_change             131 if
(!validate_resctrl_feature_request("mbm"))

Should you use CAT_STR and CQM_STR etc.. in here as well?