mbox series

[v3,00/21] Miscellaneous fixes for resctrl selftests

Message ID 20201020235126.1871815-1-fenghua.yu@intel.com
Headers show
Series Miscellaneous fixes for resctrl selftests | expand

Message

Fenghua Yu Oct. 20, 2020, 11:51 p.m. UTC
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.

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
  selftests/resctrl: Skip the test if requested resctrl feature is not
    supported
  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
  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%)

Comments

Shuah Khan Oct. 27, 2020, 8:55 p.m. UTC | #1
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
Shuah Khan Oct. 27, 2020, 8:56 p.m. UTC | #2
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
Shuah Khan Oct. 27, 2020, 8:58 p.m. UTC | #3
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
Shuah Khan Oct. 28, 2020, 12:46 a.m. UTC | #4
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