[PATCH 0/7] selftests/resctrl: Rework benchmark command handling

[PATCH 0/7] selftests/resctrl: Rework benchmark command handling

[Ilpo Järvinen]

The benchmark command handling (-b) in resctrl selftests is overly
complicated code. This series turns the benchmark command immutable to
preserve it for all selftests and improves benchmark command related
error handling.

This series also ends up removing the strcpy() calls which were pointed
out earlier.

Ilpo Järvinen (7):
  selftests/resctrl: Ensure the benchmark commands fits to its array
  selftests/resctrl: Correct benchmark command help
  selftests/resctrl: Remove bw_report and bm_type from main()
  selftests/resctrl: Simplify span lifetime
  selftests/resctrl: Use pointers to build benchmark cmd and make it
    const
  selftests/resctrl: remove ben_count variable
  selftests/resctrl: Cleanup benchmark argument parsing

 tools/testing/selftests/resctrl/cache.c       |  5 +-
 tools/testing/selftests/resctrl/cat_test.c    | 13 +--
 tools/testing/selftests/resctrl/cmt_test.c    | 32 +++++--
 tools/testing/selftests/resctrl/mba_test.c    |  4 +-
 tools/testing/selftests/resctrl/mbm_test.c    |  7 +-
 tools/testing/selftests/resctrl/resctrl.h     | 22 +++--
 .../testing/selftests/resctrl/resctrl_tests.c | 88 ++++++++-----------
 tools/testing/selftests/resctrl/resctrl_val.c | 10 ++-
 8 files changed, 98 insertions(+), 83 deletions(-)

-- 
2.30.2

[PATCH 1/7] selftests/resctrl: Ensure the benchmark commands fits to its array

[Ilpo Järvinen]

Benchmark command is copied into an array in the stack. The array is
BENCHMARK_ARGS items long but the command line could try to provide a
longer command.

Return error in case the benchmark command does not fit to its array.

Fixes: ecdbb911f22d ("selftests/resctrl: Add MBM test")
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/resctrl_tests.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index d511daeb6851..eef9e02516ad 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -255,6 +255,9 @@ int main(int argc, char **argv)
 		return ksft_exit_skip("Not running as root. Skipping...\n");
 
 	if (has_ben) {
+		if (argc - ben_ind >= BENCHMARK_ARGS - 1)
+			ksft_exit_fail_msg("Too long benchmark command");
+
 		/* Extract benchmark command from command line. */
 		for (i = ben_ind; i < argc; i++) {
 			benchmark_cmd[i - ben_ind] = benchmark_cmd_area[i];
-- 
2.30.2

[PATCH 2/7] selftests/resctrl: Correct benchmark command help

[Ilpo Järvinen]

Benchmark command must be the last argument because it consumes all the
remaining arguments but help misleadingly shows it as the first
argument. The benchmark command is also shown in quotes but it does not
match with the code.

Correct -b argument place in the help message and remove the quotes.
Tweak also how the options are presented by using ... notation.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/resctrl_tests.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index eef9e02516ad..559868b16079 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -52,7 +52,7 @@ int get_vendor(void)
 
 static void cmd_help(void)
 {
-	printf("usage: resctrl_tests [-h] [-b \"benchmark_cmd [options]\"] [-t test list] [-n no_of_bits]\n");
+	printf("usage: resctrl_tests [-h] [-t test list] [-n no_of_bits] [-b benchmark_cmd [option]...]\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, ");
-- 
2.30.2

[PATCH 3/7] selftests/resctrl: Remove bw_report and bm_type from main()

[Ilpo Järvinen]

bw_report is always set to "reads" and bm_type is set to "fill_buf" but
is never used.

Set bw_report directly to "reads" in MBA/MBM test and remove bm_type.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/mba_test.c     |  4 ++--
 tools/testing/selftests/resctrl/mbm_test.c     |  4 ++--
 tools/testing/selftests/resctrl/resctrl.h      |  4 ++--
 .../testing/selftests/resctrl/resctrl_tests.c  | 18 +++++++-----------
 4 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
index 4d2f145804b8..094424d835d0 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)
 	remove(RESULT_FILE_NAME);
 }
 
-int mba_schemata_change(int cpu_no, char *bw_report, char **benchmark_cmd)
+int mba_schemata_change(int cpu_no, char **benchmark_cmd)
 {
 	struct resctrl_val_param param = {
 		.resctrl_val	= MBA_STR,
@@ -149,7 +149,7 @@ int mba_schemata_change(int cpu_no, char *bw_report, char **benchmark_cmd)
 		.mongrp		= "m1",
 		.cpu_no		= cpu_no,
 		.filename	= RESULT_FILE_NAME,
-		.bw_report	= bw_report,
+		.bw_report	= "reads",
 		.setup		= mba_setup
 	};
 	int ret;
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index c7de6f5977f6..3e4a800e0e40 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -109,7 +109,7 @@ void mbm_test_cleanup(void)
 	remove(RESULT_FILE_NAME);
 }
 
-int mbm_bw_change(size_t span, int cpu_no, char *bw_report, char **benchmark_cmd)
+int mbm_bw_change(size_t span, int cpu_no, char **benchmark_cmd)
 {
 	struct resctrl_val_param param = {
 		.resctrl_val	= MBM_STR,
@@ -118,7 +118,7 @@ int mbm_bw_change(size_t span, int cpu_no, char *bw_report, char **benchmark_cmd
 		.span		= span,
 		.cpu_no		= cpu_no,
 		.filename	= RESULT_FILE_NAME,
-		.bw_report	=  bw_report,
+		.bw_report	= "reads",
 		.setup		= mbm_setup
 	};
 	int ret;
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index 838d1a438f33..f3446ac664c2 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -98,10 +98,10 @@ int perf_event_open(struct perf_event_attr *hw_event, pid_t pid, int cpu,
 		    int group_fd, unsigned long flags);
 int run_fill_buf(size_t span, int memflush, int op, bool once);
 int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param);
-int mbm_bw_change(size_t span, int cpu_no, char *bw_report, char **benchmark_cmd);
+int mbm_bw_change(size_t span, int cpu_no, char **benchmark_cmd);
 void tests_cleanup(void);
 void mbm_test_cleanup(void);
-int mba_schemata_change(int cpu_no, char *bw_report, char **benchmark_cmd);
+int mba_schemata_change(int cpu_no, char **benchmark_cmd);
 void mba_test_cleanup(void);
 int get_cbm_mask(char *cache_type, char *cbm_mask);
 int get_cache_size(int cpu_no, char *cache_type, unsigned long *cache_size);
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index 559868b16079..3dcd15a4b813 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -70,8 +70,7 @@ void tests_cleanup(void)
 	cat_test_cleanup();
 }
 
-static void run_mbm_test(char **benchmark_cmd, size_t span,
-			 int cpu_no, char *bw_report)
+static void run_mbm_test(char **benchmark_cmd, size_t span, int cpu_no)
 {
 	int res;
 
@@ -88,7 +87,7 @@ static void run_mbm_test(char **benchmark_cmd, size_t span,
 		goto umount;
 	}
 
-	res = mbm_bw_change(span, cpu_no, bw_report, benchmark_cmd);
+	res = mbm_bw_change(span, cpu_no, 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");
@@ -97,7 +96,7 @@ static void run_mbm_test(char **benchmark_cmd, size_t span,
 	umount_resctrlfs();
 }
 
-static void run_mba_test(char **benchmark_cmd, int cpu_no, char *bw_report)
+static void run_mba_test(char **benchmark_cmd, int cpu_no)
 {
 	int res;
 
@@ -114,7 +113,7 @@ static void run_mba_test(char **benchmark_cmd, int cpu_no, char *bw_report)
 		goto umount;
 	}
 
-	res = mba_schemata_change(cpu_no, bw_report, benchmark_cmd);
+	res = mba_schemata_change(cpu_no, benchmark_cmd);
 	ksft_test_result(!res, "MBA: schemata change\n");
 
 umount:
@@ -174,9 +173,9 @@ static void run_cat_test(int cpu_no, int no_of_bits)
 int main(int argc, char **argv)
 {
 	bool has_ben = false, mbm_test = true, mba_test = true, cmt_test = true;
-	char *benchmark_cmd[BENCHMARK_ARGS], bw_report[64], bm_type[64];
 	char benchmark_cmd_area[BENCHMARK_ARGS][BENCHMARK_ARG_SIZE];
 	int c, cpu_no = 1, argc_new = argc, i, no_of_bits = 0;
+	char *benchmark_cmd[BENCHMARK_ARGS];
 	int ben_ind, ben_count, tests = 0;
 	size_t span = 250 * MB;
 	bool cat_test = true;
@@ -277,9 +276,6 @@ int main(int argc, char **argv)
 		benchmark_cmd[5] = NULL;
 	}
 
-	sprintf(bw_report, "reads");
-	sprintf(bm_type, "fill_buf");
-
 	if (!check_resctrlfs_support())
 		return ksft_exit_skip("resctrl FS does not exist. Enable X86_CPU_RESCTRL config option.\n");
 
@@ -291,10 +287,10 @@ int main(int argc, char **argv)
 	ksft_set_plan(tests ? : 4);
 
 	if (mbm_test)
-		run_mbm_test(benchmark_cmd, span, cpu_no, bw_report);
+		run_mbm_test(benchmark_cmd, span, cpu_no);
 
 	if (mba_test)
-		run_mba_test(benchmark_cmd, cpu_no, bw_report);
+		run_mba_test(benchmark_cmd, cpu_no);
 
 	if (cmt_test)
 		run_cmt_test(benchmark_cmd, cpu_no);
-- 
2.30.2

[PATCH 4/7] selftests/resctrl: Simplify span lifetime

[Ilpo Järvinen]

struct resctrl_val_param contains span member. resctrl_val(), however,
never uses it because the value of span is embedded into the default
benchmark command and parsed from it by run_benchmark().

Remove span from resctrl_val_param. Provide DEFAULT_SPAN for the code
that needs it. CMT and CAT tests communicate span that is different
from the DEFAULT_SPAN between their internal functions which is
converted into passing it directly as a parameter.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/cache.c         |  5 +++--
 tools/testing/selftests/resctrl/cat_test.c      | 13 +++++++------
 tools/testing/selftests/resctrl/cmt_test.c      | 11 ++++++-----
 tools/testing/selftests/resctrl/mbm_test.c      |  5 ++---
 tools/testing/selftests/resctrl/resctrl.h       |  8 ++++----
 tools/testing/selftests/resctrl/resctrl_tests.c |  9 ++++-----
 6 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
index d3cbb829ff6a..a0318bd3a63d 100644
--- a/tools/testing/selftests/resctrl/cache.c
+++ b/tools/testing/selftests/resctrl/cache.c
@@ -205,10 +205,11 @@ int measure_cache_vals(struct resctrl_val_param *param, int bm_pid)
  * cache_val:		execute benchmark and measure LLC occupancy resctrl
  * and perf cache miss for the benchmark
  * @param:		parameters passed to cache_val()
+ * @span:		buffer size for the benchmark
  *
  * Return:		0 on success. non-zero on failure.
  */
-int cat_val(struct resctrl_val_param *param)
+int cat_val(struct resctrl_val_param *param, size_t span)
 {
 	int memflush = 1, operation = 0, ret = 0;
 	char *resctrl_val = param->resctrl_val;
@@ -245,7 +246,7 @@ int cat_val(struct resctrl_val_param *param)
 		if (ret)
 			break;
 
-		if (run_fill_buf(param->span, memflush, operation, true)) {
+		if (run_fill_buf(span, memflush, operation, true)) {
 			fprintf(stderr, "Error-running fill buffer\n");
 			ret = -1;
 			goto pe_close;
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
index 3848dfb46aba..97b87285ab2a 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -41,7 +41,7 @@ static int cat_setup(struct resctrl_val_param *p)
 	return ret;
 }
 
-static int check_results(struct resctrl_val_param *param)
+static int check_results(struct resctrl_val_param *param, size_t span)
 {
 	char *token_array[8], temp[512];
 	unsigned long sum_llc_perf_miss = 0;
@@ -76,7 +76,7 @@ static int check_results(struct resctrl_val_param *param)
 	fclose(fp);
 	no_of_bits = count_bits(param->mask);
 
-	return show_cache_info(sum_llc_perf_miss, no_of_bits, param->span / 64,
+	return show_cache_info(sum_llc_perf_miss, no_of_bits, span / 64,
 			       MAX_DIFF, MAX_DIFF_PERCENT, runs - 1,
 			       get_vendor() == ARCH_INTEL, false);
 }
@@ -96,6 +96,7 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
 	char cbm_mask[256];
 	int count_of_bits;
 	char pipe_message;
+	size_t span;
 
 	/* Get default cbm mask for L3/L2 cache */
 	ret = get_cbm_mask(cache_type, cbm_mask);
@@ -140,7 +141,7 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
 	/* Set param values for parent thread which will be allocated bitmask
 	 * with (max_bits - n) bits
 	 */
-	param.span = cache_size * (count_of_bits - n) / count_of_bits;
+	span = cache_size * (count_of_bits - n) / count_of_bits;
 	strcpy(param.ctrlgrp, "c2");
 	strcpy(param.mongrp, "m2");
 	strcpy(param.filename, RESULT_FILE_NAME2);
@@ -162,7 +163,7 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
 		param.mask = l_mask_1;
 		strcpy(param.ctrlgrp, "c1");
 		strcpy(param.mongrp, "m1");
-		param.span = cache_size * n / count_of_bits;
+		span = cache_size * n / count_of_bits;
 		strcpy(param.filename, RESULT_FILE_NAME1);
 		param.num_of_runs = 0;
 		param.cpu_no = sibling_cpu_no;
@@ -176,9 +177,9 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
 
 	remove(param.filename);
 
-	ret = cat_val(&param);
+	ret = cat_val(&param, span);
 	if (ret == 0)
-		ret = check_results(&param);
+		ret = check_results(&param, span);
 
 	if (bm_pid == 0) {
 		/* Tell parent that child is ready */
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index cb2197647c6c..9d8e38e995ef 100644
--- a/tools/testing/selftests/resctrl/cmt_test.c
+++ b/tools/testing/selftests/resctrl/cmt_test.c
@@ -27,7 +27,7 @@ static int cmt_setup(struct resctrl_val_param *p)
 	return 0;
 }
 
-static int check_results(struct resctrl_val_param *param, int no_of_bits)
+static int check_results(struct resctrl_val_param *param, size_t span, int no_of_bits)
 {
 	char *token_array[8], temp[512];
 	unsigned long sum_llc_occu_resc = 0;
@@ -58,7 +58,7 @@ static int check_results(struct resctrl_val_param *param, int no_of_bits)
 	}
 	fclose(fp);
 
-	return show_cache_info(sum_llc_occu_resc, no_of_bits, param->span,
+	return show_cache_info(sum_llc_occu_resc, no_of_bits, span,
 			       MAX_DIFF, MAX_DIFF_PERCENT, runs - 1,
 			       true, true);
 }
@@ -74,6 +74,7 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
 	unsigned long long_mask;
 	char cbm_mask[256];
 	int count_of_bits;
+	size_t span;
 	int ret;
 
 	if (!validate_resctrl_feature_request(CMT_STR))
@@ -105,13 +106,13 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
 		.cpu_no		= cpu_no,
 		.filename	= RESULT_FILE_NAME,
 		.mask		= ~(long_mask << n) & long_mask,
-		.span		= cache_size * n / count_of_bits,
 		.num_of_runs	= 0,
 		.setup		= cmt_setup,
 	};
 
+	span = cache_size * n / count_of_bits;
 	if (strcmp(benchmark_cmd[0], "fill_buf") == 0)
-		sprintf(benchmark_cmd[1], "%zu", param.span);
+		sprintf(benchmark_cmd[1], "%zu", span);
 
 	remove(RESULT_FILE_NAME);
 
@@ -119,7 +120,7 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
 	if (ret)
 		goto out;
 
-	ret = check_results(&param, n);
+	ret = check_results(&param, span, n);
 
 out:
 	cmt_test_cleanup();
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index 3e4a800e0e40..b830fc84338b 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -109,13 +109,12 @@ void mbm_test_cleanup(void)
 	remove(RESULT_FILE_NAME);
 }
 
-int mbm_bw_change(size_t span, int cpu_no, char **benchmark_cmd)
+int mbm_bw_change(int cpu_no, char **benchmark_cmd)
 {
 	struct resctrl_val_param param = {
 		.resctrl_val	= MBM_STR,
 		.ctrlgrp	= "c1",
 		.mongrp		= "m1",
-		.span		= span,
 		.cpu_no		= cpu_no,
 		.filename	= RESULT_FILE_NAME,
 		.bw_report	= "reads",
@@ -129,7 +128,7 @@ int mbm_bw_change(size_t span, int cpu_no, char **benchmark_cmd)
 	if (ret)
 		goto out;
 
-	ret = check_results(span);
+	ret = check_results(DEFAULT_SPAN);
 
 out:
 	mbm_test_cleanup();
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index f3446ac664c2..bcd0d2060f81 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -38,6 +38,8 @@
 
 #define END_OF_TESTS	1
 
+#define DEFAULT_SPAN		(250 * MB)
+
 #define PARENT_EXIT(err_msg)			\
 	do {					\
 		perror(err_msg);		\
@@ -52,7 +54,6 @@
  * @ctrlgrp:		Name of the control monitor group (con_mon grp)
  * @mongrp:		Name of the monitor group (mon grp)
  * @cpu_no:		CPU number to which the benchmark would be binded
- * @span:		Memory bytes accessed in each benchmark iteration
  * @filename:		Name of file to which the o/p should be written
  * @bw_report:		Bandwidth report type (reads vs writes)
  * @setup:		Call back function to setup test environment
@@ -62,7 +63,6 @@ struct resctrl_val_param {
 	char		ctrlgrp[64];
 	char		mongrp[64];
 	int		cpu_no;
-	size_t		span;
 	char		filename[64];
 	char		*bw_report;
 	unsigned long	mask;
@@ -98,7 +98,7 @@ int perf_event_open(struct perf_event_attr *hw_event, pid_t pid, int cpu,
 		    int group_fd, unsigned long flags);
 int run_fill_buf(size_t span, int memflush, int op, bool once);
 int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param);
-int mbm_bw_change(size_t span, int cpu_no, char **benchmark_cmd);
+int mbm_bw_change(int cpu_no, char **benchmark_cmd);
 void tests_cleanup(void);
 void mbm_test_cleanup(void);
 int mba_schemata_change(int cpu_no, char **benchmark_cmd);
@@ -108,7 +108,7 @@ int get_cache_size(int cpu_no, char *cache_type, unsigned long *cache_size);
 void ctrlc_handler(int signum, siginfo_t *info, void *ptr);
 int signal_handler_register(void);
 void signal_handler_unregister(void);
-int cat_val(struct resctrl_val_param *param);
+int cat_val(struct resctrl_val_param *param, size_t span);
 void cat_test_cleanup(void);
 int cat_perf_miss_val(int cpu_no, int no_of_bits, char *cache_type);
 int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd);
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index 3dcd15a4b813..903167a192d7 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -70,7 +70,7 @@ void tests_cleanup(void)
 	cat_test_cleanup();
 }
 
-static void run_mbm_test(char **benchmark_cmd, size_t span, int cpu_no)
+static void run_mbm_test(char **benchmark_cmd, int cpu_no)
 {
 	int res;
 
@@ -87,7 +87,7 @@ static void run_mbm_test(char **benchmark_cmd, size_t span, int cpu_no)
 		goto umount;
 	}
 
-	res = mbm_bw_change(span, cpu_no, benchmark_cmd);
+	res = mbm_bw_change(cpu_no, 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");
@@ -177,7 +177,6 @@ int main(int argc, char **argv)
 	int c, cpu_no = 1, argc_new = argc, i, no_of_bits = 0;
 	char *benchmark_cmd[BENCHMARK_ARGS];
 	int ben_ind, ben_count, tests = 0;
-	size_t span = 250 * MB;
 	bool cat_test = true;
 
 	for (i = 0; i < argc; i++) {
@@ -269,7 +268,7 @@ int main(int argc, char **argv)
 			benchmark_cmd[i] = benchmark_cmd_area[i];
 
 		strcpy(benchmark_cmd[0], "fill_buf");
-		sprintf(benchmark_cmd[1], "%zu", span);
+		sprintf(benchmark_cmd[1], "%zu", (size_t)DEFAULT_SPAN);
 		strcpy(benchmark_cmd[2], "1");
 		strcpy(benchmark_cmd[3], "0");
 		strcpy(benchmark_cmd[4], "false");
@@ -287,7 +286,7 @@ int main(int argc, char **argv)
 	ksft_set_plan(tests ? : 4);
 
 	if (mbm_test)
-		run_mbm_test(benchmark_cmd, span, cpu_no);
+		run_mbm_test(benchmark_cmd, cpu_no);
 
 	if (mba_test)
 		run_mba_test(benchmark_cmd, cpu_no);
-- 
2.30.2

[PATCH 5/7] selftests/resctrl: Use pointers to build benchmark cmd and make it const

[Ilpo Järvinen]

Benchmark parameter uses fixed-size buffers in stack which is slightly
dangerous. As benchmark command is used in multiple tests, it should
not be mutated by the tests. Due to the order of tests, mutating the
span argument in CMT test does not trigger any real problems currently.

Mark benchmark_cmd strings as const and setup the benchmark command
using pointers. As span is constant in main(), just provide the default
span also as string to be used in setting up the default fill_buf
argument so no malloc() is required for it.

CMT test has to create a copy of the benchmark command before altering
the benchmark command.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/cmt_test.c    | 23 ++++++++++---
 tools/testing/selftests/resctrl/mba_test.c    |  2 +-
 tools/testing/selftests/resctrl/mbm_test.c    |  2 +-
 tools/testing/selftests/resctrl/resctrl.h     | 16 ++++++---
 .../testing/selftests/resctrl/resctrl_tests.c | 33 ++++++++-----------
 tools/testing/selftests/resctrl/resctrl_val.c | 10 ++++--
 6 files changed, 54 insertions(+), 32 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index 9d8e38e995ef..a40e12c3b1a7 100644
--- a/tools/testing/selftests/resctrl/cmt_test.c
+++ b/tools/testing/selftests/resctrl/cmt_test.c
@@ -68,14 +68,16 @@ void cmt_test_cleanup(void)
 	remove(RESULT_FILE_NAME);
 }
 
-int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
+int cmt_resctrl_val(int cpu_no, int n, const char * const *benchmark_cmd)
 {
+	const char *cmd[BENCHMARK_ARGS];
 	unsigned long cache_size = 0;
 	unsigned long long_mask;
+	char *span_str = NULL;
 	char cbm_mask[256];
 	int count_of_bits;
 	size_t span;
-	int ret;
+	int ret, i;
 
 	if (!validate_resctrl_feature_request(CMT_STR))
 		return -1;
@@ -111,12 +113,22 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
 	};
 
 	span = cache_size * n / count_of_bits;
-	if (strcmp(benchmark_cmd[0], "fill_buf") == 0)
-		sprintf(benchmark_cmd[1], "%zu", span);
+	/* Duplicate the command to be able to replace span in it */
+	for (i = 0; benchmark_cmd[i]; i++)
+		cmd[i] = benchmark_cmd[i];
+	cmd[i] = NULL;
+
+	if (strcmp(cmd[0], "fill_buf") == 0) {
+		span_str = malloc(SIZE_MAX_DECIMAL_SIZE);
+		if (!span_str)
+			return -1;
+		snprintf(span_str, SIZE_MAX_DECIMAL_SIZE, "%zu", span);
+		cmd[1] = span_str;
+	}
 
 	remove(RESULT_FILE_NAME);
 
-	ret = resctrl_val(benchmark_cmd, &param);
+	ret = resctrl_val(cmd, &param);
 	if (ret)
 		goto out;
 
@@ -124,6 +136,7 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
 
 out:
 	cmt_test_cleanup();
+	free(span_str);
 
 	return ret;
 }
diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
index 094424d835d0..cf8284dadcb2 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)
 	remove(RESULT_FILE_NAME);
 }
 
-int mba_schemata_change(int cpu_no, char **benchmark_cmd)
+int mba_schemata_change(int cpu_no, const char * const *benchmark_cmd)
 {
 	struct resctrl_val_param param = {
 		.resctrl_val	= MBA_STR,
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index b830fc84338b..1ae131a2e246 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -109,7 +109,7 @@ void mbm_test_cleanup(void)
 	remove(RESULT_FILE_NAME);
 }
 
-int mbm_bw_change(int cpu_no, char **benchmark_cmd)
+int mbm_bw_change(int cpu_no, const char * const *benchmark_cmd)
 {
 	struct resctrl_val_param param = {
 		.resctrl_val	= MBM_STR,
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index bcd0d2060f81..ddb1e83a3a64 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -6,6 +6,7 @@
 #include <math.h>
 #include <errno.h>
 #include <sched.h>
+#include <stdint.h>
 #include <stdlib.h>
 #include <unistd.h>
 #include <string.h>
@@ -38,7 +39,14 @@
 
 #define END_OF_TESTS	1
 
+#define BENCHMARK_ARGS		64
+
+/* Approximate %zu max length */
+#define SIZE_MAX_DECIMAL_SIZE	(sizeof(SIZE_MAX) * 8 / 3 + 2)
+
+/* Define default span both as integer and string, these should match */
 #define DEFAULT_SPAN		(250 * MB)
+#define DEFAULT_SPAN_STR	"262144000"
 
 #define PARENT_EXIT(err_msg)			\
 	do {					\
@@ -97,11 +105,11 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp,
 int perf_event_open(struct perf_event_attr *hw_event, pid_t pid, int cpu,
 		    int group_fd, unsigned long flags);
 int run_fill_buf(size_t span, int memflush, int op, bool once);
-int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param);
-int mbm_bw_change(int cpu_no, char **benchmark_cmd);
+int resctrl_val(const char * const *benchmark_cmd, struct resctrl_val_param *param);
+int mbm_bw_change(int cpu_no, const char * const *benchmark_cmd);
 void tests_cleanup(void);
 void mbm_test_cleanup(void);
-int mba_schemata_change(int cpu_no, char **benchmark_cmd);
+int mba_schemata_change(int cpu_no, const char *const *benchmark_cmd);
 void mba_test_cleanup(void);
 int get_cbm_mask(char *cache_type, char *cbm_mask);
 int get_cache_size(int cpu_no, char *cache_type, unsigned long *cache_size);
@@ -111,7 +119,7 @@ void signal_handler_unregister(void);
 int cat_val(struct resctrl_val_param *param, size_t span);
 void cat_test_cleanup(void);
 int cat_perf_miss_val(int cpu_no, int no_of_bits, char *cache_type);
-int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd);
+int cmt_resctrl_val(int cpu_no, int n, const char * const *benchmark_cmd);
 unsigned int count_bits(unsigned long n);
 void cmt_test_cleanup(void);
 int get_core_sibling(int cpu_no);
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index 903167a192d7..74a10abeb01d 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -10,8 +10,9 @@
  */
 #include "resctrl.h"
 
-#define BENCHMARK_ARGS		64
-#define BENCHMARK_ARG_SIZE	64
+/* Define default span both as integer and string, these should match */
+#define DEFAULT_SPAN		(250 * MB)
+#define DEFAULT_SPAN_STR	"262144000"
 
 static int detect_vendor(void)
 {
@@ -70,7 +71,7 @@ void tests_cleanup(void)
 	cat_test_cleanup();
 }
 
-static void run_mbm_test(char **benchmark_cmd, int cpu_no)
+static void run_mbm_test(const char **benchmark_cmd, int cpu_no)
 {
 	int res;
 
@@ -96,7 +97,7 @@ static void run_mbm_test(char **benchmark_cmd, int cpu_no)
 	umount_resctrlfs();
 }
 
-static void run_mba_test(char **benchmark_cmd, int cpu_no)
+static void run_mba_test(const char **benchmark_cmd, int cpu_no)
 {
 	int res;
 
@@ -120,7 +121,7 @@ static void run_mba_test(char **benchmark_cmd, int cpu_no)
 	umount_resctrlfs();
 }
 
-static void run_cmt_test(char **benchmark_cmd, int cpu_no)
+static void run_cmt_test(const char **benchmark_cmd, int cpu_no)
 {
 	int res;
 
@@ -173,9 +174,8 @@ static void run_cat_test(int cpu_no, int no_of_bits)
 int main(int argc, char **argv)
 {
 	bool has_ben = false, mbm_test = true, mba_test = true, cmt_test = true;
-	char benchmark_cmd_area[BENCHMARK_ARGS][BENCHMARK_ARG_SIZE];
 	int c, cpu_no = 1, argc_new = argc, i, no_of_bits = 0;
-	char *benchmark_cmd[BENCHMARK_ARGS];
+	const char *benchmark_cmd[BENCHMARK_ARGS];
 	int ben_ind, ben_count, tests = 0;
 	bool cat_test = true;
 
@@ -257,21 +257,16 @@ int main(int argc, char **argv)
 			ksft_exit_fail_msg("Too long benchmark command");
 
 		/* Extract benchmark command from command line. */
-		for (i = ben_ind; i < argc; i++) {
-			benchmark_cmd[i - ben_ind] = benchmark_cmd_area[i];
-			sprintf(benchmark_cmd[i - ben_ind], "%s", argv[i]);
-		}
+		for (i = 0; i < argc - ben_ind; i++)
+			benchmark_cmd[i] = argv[i + ben_ind];
 		benchmark_cmd[ben_count] = NULL;
 	} else {
 		/* If no benchmark is given by "-b" argument, use fill_buf. */
-		for (i = 0; i < 5; i++)
-			benchmark_cmd[i] = benchmark_cmd_area[i];
-
-		strcpy(benchmark_cmd[0], "fill_buf");
-		sprintf(benchmark_cmd[1], "%zu", (size_t)DEFAULT_SPAN);
-		strcpy(benchmark_cmd[2], "1");
-		strcpy(benchmark_cmd[3], "0");
-		strcpy(benchmark_cmd[4], "false");
+		benchmark_cmd[0] = "fill_buf";
+		benchmark_cmd[1] = DEFAULT_SPAN_STR;
+		benchmark_cmd[2] = "1";
+		benchmark_cmd[3] = "0";
+		benchmark_cmd[4] = "false";
 		benchmark_cmd[5] = NULL;
 	}
 
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index f0f6c5f6e98b..51963a6f2186 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -629,7 +629,7 @@ measure_vals(struct resctrl_val_param *param, unsigned long *bw_resc_start)
  *
  * Return:		0 on success. non-zero on failure.
  */
-int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param)
+int resctrl_val(const char * const *benchmark_cmd, struct resctrl_val_param *param)
 {
 	char *resctrl_val = param->resctrl_val;
 	unsigned long bw_resc_start = 0;
@@ -710,7 +710,13 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param)
 	if (ret)
 		goto out;
 
-	value.sival_ptr = benchmark_cmd;
+	/*
+	 * The cast removes constness but nothing mutates benchmark_cmd within
+	 * the context of this process. At the receiving process, it becomes
+	 * argv, which is mutable, on exec() but that's after fork() so it
+	 * doesn't matter for the process running the tests.
+	 */
+	value.sival_ptr = (void *)benchmark_cmd;
 
 	/* Taskset benchmark to specified cpu */
 	ret = taskset_benchmark(bm_pid, param->cpu_no);
-- 
2.30.2

[PATCH 6/7] selftests/resctrl: remove ben_count variable

[Ilpo Järvinen]

ben_count is only used to write the terminator for the list. It is
enough to use i from the loop so no need for another variable.

Remove ben_count variable as it is not needed.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/resctrl_tests.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index 74a10abeb01d..81c2ed299e6f 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -176,13 +176,12 @@ int main(int argc, char **argv)
 	bool has_ben = false, mbm_test = true, mba_test = true, cmt_test = true;
 	int c, cpu_no = 1, argc_new = argc, i, no_of_bits = 0;
 	const char *benchmark_cmd[BENCHMARK_ARGS];
-	int ben_ind, ben_count, tests = 0;
+	int ben_ind, tests = 0;
 	bool cat_test = true;
 
 	for (i = 0; i < argc; i++) {
 		if (strcmp(argv[i], "-b") == 0) {
 			ben_ind = i + 1;
-			ben_count = argc - ben_ind;
 			argc_new = ben_ind - 1;
 			has_ben = true;
 			break;
@@ -259,7 +258,7 @@ int main(int argc, char **argv)
 		/* Extract benchmark command from command line. */
 		for (i = 0; i < argc - ben_ind; i++)
 			benchmark_cmd[i] = argv[i + ben_ind];
-		benchmark_cmd[ben_count] = NULL;
+		benchmark_cmd[i] = NULL;
 	} else {
 		/* If no benchmark is given by "-b" argument, use fill_buf. */
 		benchmark_cmd[0] = "fill_buf";
-- 
2.30.2

[PATCH 7/7] selftests/resctrl: Cleanup benchmark argument parsing

[Ilpo Järvinen]

Benchmark argument is handled by custom argument parsing code which is
more complicated than it needs to be.

Process benchmark argument within the normal getopt() handling and drop
entirely unnecessary ben_ind and has_ben variables. If -b is not given,
setup the default benchmark command right after the switch statement
and make -b to goto over it while it terminates the getopt() loop.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 .../testing/selftests/resctrl/resctrl_tests.c | 56 +++++++++----------
 1 file changed, 25 insertions(+), 31 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index 81c2ed299e6f..a437aaa69cc5 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -173,25 +173,27 @@ static void run_cat_test(int cpu_no, int no_of_bits)
 
 int main(int argc, char **argv)
 {
-	bool has_ben = false, mbm_test = true, mba_test = true, cmt_test = true;
-	int c, cpu_no = 1, argc_new = argc, i, no_of_bits = 0;
+	bool mbm_test = true, mba_test = true, cmt_test = true;
+	int c, cpu_no = 1, i, no_of_bits = 0;
 	const char *benchmark_cmd[BENCHMARK_ARGS];
-	int ben_ind, tests = 0;
 	bool cat_test = true;
+	int tests = 0;
 
-	for (i = 0; i < argc; i++) {
-		if (strcmp(argv[i], "-b") == 0) {
-			ben_ind = i + 1;
-			argc_new = ben_ind - 1;
-			has_ben = true;
-			break;
-		}
-	}
-
-	while ((c = getopt(argc_new, argv, "ht:b:n:p:")) != -1) {
+	while ((c = getopt(argc, argv, "ht:b:n:p:")) != -1) {
 		char *token;
 
 		switch (c) {
+		case 'b':
+			optind--;			/* Back to optarg */
+			if (argc - optind >= BENCHMARK_ARGS - 1)
+				ksft_exit_fail_msg("Too long benchmark command");
+
+			/* Extract benchmark command from command line. */
+			for (i = 0; i < argc - optind; i++)
+				benchmark_cmd[i] = argv[i + optind];
+			benchmark_cmd[i] = NULL;
+
+			goto last_arg;
 		case 't':
 			token = strtok(optarg, ",");
 
@@ -241,6 +243,16 @@ int main(int argc, char **argv)
 		}
 	}
 
+	/* If no benchmark is given by "-b" argument, use fill_buf. */
+	benchmark_cmd[0] = "fill_buf";
+	benchmark_cmd[1] = DEFAULT_SPAN_STR;
+	benchmark_cmd[2] = "1";
+	benchmark_cmd[3] = "0";
+	benchmark_cmd[4] = "false";
+	benchmark_cmd[5] = NULL;
+
+last_arg:
+
 	ksft_print_header();
 
 	/*
@@ -251,24 +263,6 @@ int main(int argc, char **argv)
 	if (geteuid() != 0)
 		return ksft_exit_skip("Not running as root. Skipping...\n");
 
-	if (has_ben) {
-		if (argc - ben_ind >= BENCHMARK_ARGS - 1)
-			ksft_exit_fail_msg("Too long benchmark command");
-
-		/* Extract benchmark command from command line. */
-		for (i = 0; i < argc - ben_ind; i++)
-			benchmark_cmd[i] = argv[i + ben_ind];
-		benchmark_cmd[i] = NULL;
-	} else {
-		/* If no benchmark is given by "-b" argument, use fill_buf. */
-		benchmark_cmd[0] = "fill_buf";
-		benchmark_cmd[1] = DEFAULT_SPAN_STR;
-		benchmark_cmd[2] = "1";
-		benchmark_cmd[3] = "0";
-		benchmark_cmd[4] = "false";
-		benchmark_cmd[5] = NULL;
-	}
-
 	if (!check_resctrlfs_support())
 		return ksft_exit_skip("resctrl FS does not exist. Enable X86_CPU_RESCTRL config option.\n");
 
-- 
2.30.2

Re: [PATCH 1/7] selftests/resctrl: Ensure the benchmark commands fits to its array

[Reinette Chatre]

Hi Ilpo,

On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
> Benchmark command is copied into an array in the stack. The array is
> BENCHMARK_ARGS items long but the command line could try to provide a
> longer command.
> 
> Return error in case the benchmark command does not fit to its array.
> 
> Fixes: ecdbb911f22d ("selftests/resctrl: Add MBM test")
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  tools/testing/selftests/resctrl/resctrl_tests.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index d511daeb6851..eef9e02516ad 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -255,6 +255,9 @@ int main(int argc, char **argv)
>  		return ksft_exit_skip("Not running as root. Skipping...\n");
>  
>  	if (has_ben) {
> +		if (argc - ben_ind >= BENCHMARK_ARGS - 1)
> +			ksft_exit_fail_msg("Too long benchmark command");
> +

I think there are two potential issues here: too many arguments and too
long arguments. Current code can handle 64 (63 with last required to be
NULL) arguments each expected to be 64 bytes (63 to end with \0).
The above fix looks to be handling the first issue, with this the
error message could be more accurate if it refers to the
number of arguments instead. It looks to me as though the latter
issue still needs to be handled. I understand that this becomes 
unnecessary via the refactor in following patches but I expect that
this fix needs to be backported (cc. stable also) and thus
it may benefit from a precision added to the sprintf() that follows
the snippet below?

>  		/* Extract benchmark command from command line. */
>  		for (i = ben_ind; i < argc; i++) {
>  			benchmark_cmd[i - ben_ind] = benchmark_cmd_area[i];

Reinette

ps. Unless you have an updated email address that works, could you please
remove Sai's email from future submissions?

Re: [PATCH 2/7] selftests/resctrl: Correct benchmark command help

[Reinette Chatre]

Hi Ilpo,

On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
> Benchmark command must be the last argument because it consumes all the
> remaining arguments but help misleadingly shows it as the first
> argument. The benchmark command is also shown in quotes but it does not
> match with the code.
> 
> Correct -b argument place in the help message and remove the quotes.
> Tweak also how the options are presented by using ... notation.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  tools/testing/selftests/resctrl/resctrl_tests.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index eef9e02516ad..559868b16079 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -52,7 +52,7 @@ int get_vendor(void)
>  
>  static void cmd_help(void)
>  {
> -	printf("usage: resctrl_tests [-h] [-b \"benchmark_cmd [options]\"] [-t test list] [-n no_of_bits]\n");
> +	printf("usage: resctrl_tests [-h] [-t test list] [-n no_of_bits] [-b benchmark_cmd [option]...]\n");
>  	printf("\t-b benchmark_cmd [options]: run specified benchmark for MBM, MBA and CMT\n");

Considering it was documented goal of patch to use "..." notation, should
it be done consistently by changing above line also?

>  	printf("\t   default benchmark is builtin fill_buf\n");
>  	printf("\t-t test list: run tests specified in the test list, ");

Reinette

Re: [PATCH 3/7] selftests/resctrl: Remove bw_report and bm_type from main()

[Reinette Chatre]

Hi Ilpo,

On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
> bw_report is always set to "reads" and bm_type is set to "fill_buf" but
> is never used.
> 
> Set bw_report directly to "reads" in MBA/MBM test and remove bm_type.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

Re: [PATCH 4/7] selftests/resctrl: Simplify span lifetime

[Reinette Chatre]

Hi Ilpo,

On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
> struct resctrl_val_param contains span member. resctrl_val(), however,
> never uses it because the value of span is embedded into the default
> benchmark command and parsed from it by run_benchmark().
> 
> Remove span from resctrl_val_param. Provide DEFAULT_SPAN for the code
> that needs it. CMT and CAT tests communicate span that is different
> from the DEFAULT_SPAN between their internal functions which is
> converted into passing it directly as a parameter.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

If I understand correctly the custom benchmark has always been
required to operate on the same span as the default benchmark in
order for the results to be measured accurately.

Thank you for eliminating unnecessary code.

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

Re: [PATCH 5/7] selftests/resctrl: Use pointers to build benchmark cmd and make it const

[Reinette Chatre]

Hi Ilpo,

On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
> Benchmark parameter uses fixed-size buffers in stack which is slightly
> dangerous. As benchmark command is used in multiple tests, it should

Could you please be specific with issues with current implementation?
The term "slightly dangerous" is vague.

> not be mutated by the tests. Due to the order of tests, mutating the
> span argument in CMT test does not trigger any real problems currently.
> 
> Mark benchmark_cmd strings as const and setup the benchmark command
> using pointers. As span is constant in main(), just provide the default
> span also as string to be used in setting up the default fill_buf
> argument so no malloc() is required for it.

What is wrong with using malloc()?

> 
> CMT test has to create a copy of the benchmark command before altering
> the benchmark command.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  tools/testing/selftests/resctrl/cmt_test.c    | 23 ++++++++++---
>  tools/testing/selftests/resctrl/mba_test.c    |  2 +-
>  tools/testing/selftests/resctrl/mbm_test.c    |  2 +-
>  tools/testing/selftests/resctrl/resctrl.h     | 16 ++++++---
>  .../testing/selftests/resctrl/resctrl_tests.c | 33 ++++++++-----------
>  tools/testing/selftests/resctrl/resctrl_val.c | 10 ++++--
>  6 files changed, 54 insertions(+), 32 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
> index 9d8e38e995ef..a40e12c3b1a7 100644
> --- a/tools/testing/selftests/resctrl/cmt_test.c
> +++ b/tools/testing/selftests/resctrl/cmt_test.c
> @@ -68,14 +68,16 @@ void cmt_test_cleanup(void)
>  	remove(RESULT_FILE_NAME);
>  }
>  
> -int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
> +int cmt_resctrl_val(int cpu_no, int n, const char * const *benchmark_cmd)
>  {
> +	const char *cmd[BENCHMARK_ARGS];
>  	unsigned long cache_size = 0;
>  	unsigned long long_mask;
> +	char *span_str = NULL;
>  	char cbm_mask[256];
>  	int count_of_bits;
>  	size_t span;
> -	int ret;
> +	int ret, i;
>  
>  	if (!validate_resctrl_feature_request(CMT_STR))
>  		return -1;
> @@ -111,12 +113,22 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
>  	};
>  
>  	span = cache_size * n / count_of_bits;
> -	if (strcmp(benchmark_cmd[0], "fill_buf") == 0)
> -		sprintf(benchmark_cmd[1], "%zu", span);
> +	/* Duplicate the command to be able to replace span in it */
> +	for (i = 0; benchmark_cmd[i]; i++)
> +		cmd[i] = benchmark_cmd[i];
> +	cmd[i] = NULL;
> +
> +	if (strcmp(cmd[0], "fill_buf") == 0) {
> +		span_str = malloc(SIZE_MAX_DECIMAL_SIZE);
> +		if (!span_str)
> +			return -1;
> +		snprintf(span_str, SIZE_MAX_DECIMAL_SIZE, "%zu", span);

Have you considered asprintf()?

> +		cmd[1] = span_str;
> +	}

It looks to me that array only needs to be duplicated if the
default benchmark is used?

>  
>  	remove(RESULT_FILE_NAME);
>  
> -	ret = resctrl_val(benchmark_cmd, &param);
> +	ret = resctrl_val(cmd, &param);
>  	if (ret)
>  		goto out;
>  

...

> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index bcd0d2060f81..ddb1e83a3a64 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -6,6 +6,7 @@
>  #include <math.h>
>  #include <errno.h>
>  #include <sched.h>
> +#include <stdint.h>
>  #include <stdlib.h>
>  #include <unistd.h>
>  #include <string.h>
> @@ -38,7 +39,14 @@
>  
>  #define END_OF_TESTS	1
>  
> +#define BENCHMARK_ARGS		64
> +
> +/* Approximate %zu max length */
> +#define SIZE_MAX_DECIMAL_SIZE	(sizeof(SIZE_MAX) * 8 / 3 + 2)
> +
> +/* Define default span both as integer and string, these should match */
>  #define DEFAULT_SPAN		(250 * MB)
> +#define DEFAULT_SPAN_STR	"262144000"

I think above hardcoding can be eliminated by using asprintf()? This
does allocate memory though so I would like to understand why one
goal is to not dynamically allocate memory.

>  
>  #define PARENT_EXIT(err_msg)			\
>  	do {					\

Reinette

Re: [PATCH 6/7] selftests/resctrl: remove ben_count variable

[Reinette Chatre]

Hi Ilpo,

Just a nitpick ... this patch is standing out among the rest by not
starting the subject with a capital letter.

On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
> ben_count is only used to write the terminator for the list. It is
> enough to use i from the loop so no need for another variable.
> 
> Remove ben_count variable as it is not needed.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  tools/testing/selftests/resctrl/resctrl_tests.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

Re: [PATCH 7/7] selftests/resctrl: Cleanup benchmark argument parsing

[Reinette Chatre]

Hi Ilpo,

On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
> Benchmark argument is handled by custom argument parsing code which is
> more complicated than it needs to be.
> 
> Process benchmark argument within the normal getopt() handling and drop
> entirely unnecessary ben_ind and has_ben variables. If -b is not given,
> setup the default benchmark command right after the switch statement
> and make -b to goto over it while it terminates the getopt() loop.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  .../testing/selftests/resctrl/resctrl_tests.c | 56 +++++++++----------
>  1 file changed, 25 insertions(+), 31 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index 81c2ed299e6f..a437aaa69cc5 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -173,25 +173,27 @@ static void run_cat_test(int cpu_no, int no_of_bits)
>  
>  int main(int argc, char **argv)
>  {
> -	bool has_ben = false, mbm_test = true, mba_test = true, cmt_test = true;
> -	int c, cpu_no = 1, argc_new = argc, i, no_of_bits = 0;
> +	bool mbm_test = true, mba_test = true, cmt_test = true;
> +	int c, cpu_no = 1, i, no_of_bits = 0;
>  	const char *benchmark_cmd[BENCHMARK_ARGS];
> -	int ben_ind, tests = 0;
>  	bool cat_test = true;
> +	int tests = 0;
>  
> -	for (i = 0; i < argc; i++) {
> -		if (strcmp(argv[i], "-b") == 0) {
> -			ben_ind = i + 1;
> -			argc_new = ben_ind - 1;
> -			has_ben = true;
> -			break;
> -		}
> -	}
> -
> -	while ((c = getopt(argc_new, argv, "ht:b:n:p:")) != -1) {
> +	while ((c = getopt(argc, argv, "ht:b:n:p:")) != -1) {
>  		char *token;
>  
>  		switch (c) {
> +		case 'b':
> +			optind--;			/* Back to optarg */

The above tab usage is not clear. Also, tail comments in code has been
found to disturb reading to the point that x86 contributors are explicitly
asked to refrain from using it.
Perhaps rather a short summary of algorithm before the code starts?

Apart from this style nitpick it looks good to me. After that is
handled you can add:

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

Re: [PATCH 1/7] selftests/resctrl: Ensure the benchmark commands fits to its array

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2881 bytes --]

On Mon, 14 Aug 2023, Reinette Chatre wrote:
> On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
> > Benchmark command is copied into an array in the stack. The array is
> > BENCHMARK_ARGS items long but the command line could try to provide a
> > longer command.
> > 
> > Return error in case the benchmark command does not fit to its array.
> > 
> > Fixes: ecdbb911f22d ("selftests/resctrl: Add MBM test")
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> >  tools/testing/selftests/resctrl/resctrl_tests.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> > index d511daeb6851..eef9e02516ad 100644
> > --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> > +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> > @@ -255,6 +255,9 @@ int main(int argc, char **argv)
> >  		return ksft_exit_skip("Not running as root. Skipping...\n");
> >  
> >  	if (has_ben) {
> > +		if (argc - ben_ind >= BENCHMARK_ARGS - 1)
> > +			ksft_exit_fail_msg("Too long benchmark command");
> > +
> 
> I think there are two potential issues here: too many arguments and too
> long arguments. Current code can handle 64 (63 with last required to be
> NULL) arguments each expected to be 64 bytes (63 to end with \0).
> The above fix looks to be handling the first issue, with this the
> error message could be more accurate if it refers to the
> number of arguments instead. It looks to me as though the latter
> issue still needs to be handled. I understand that this becomes 
> unnecessary via the refactor in following patches but I expect that
> this fix needs to be backported (cc. stable also) and thus
> it may benefit from a precision added to the sprintf() that follows
> the snippet below?

Thanks for taking a look, yes, both are problems. Third problem which 
still remains is if "fill_buf" is the first argument of -b, the arguments 
are not validated to match the formatting used internally (I guess it 
might be intentional to allow overriding the internal fill_buf arguments 
but just the validation is lacking).

I'll add strlen() check instead of using sprintf() in order to properly 
fail rather than silently truncating the arguments.

> >  		/* Extract benchmark command from command line. */
> >  		for (i = ben_ind; i < argc; i++) {
> >  			benchmark_cmd[i - ben_ind] = benchmark_cmd_area[i];
> 
> Reinette
> 
> ps. Unless you have an updated email address that works, could you please
> remove Sai's email from future submissions?

It's auto-added by git send-email machinery. I guess I can try to make 
an exception to my usual workflow by sending only to manually specified To 
addresses (if I remember). Perhaps one day I'll write a tool to filter out
the addresses from git send-email generated ones but as is I don't have 
one.

-- 
 i.

Re: [PATCH 2/7] selftests/resctrl: Correct benchmark command help

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1526 bytes --]

On Mon, 14 Aug 2023, Reinette Chatre wrote:

> Hi Ilpo,
> 
> On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
> > Benchmark command must be the last argument because it consumes all the
> > remaining arguments but help misleadingly shows it as the first
> > argument. The benchmark command is also shown in quotes but it does not
> > match with the code.
> > 
> > Correct -b argument place in the help message and remove the quotes.
> > Tweak also how the options are presented by using ... notation.
> > 
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> >  tools/testing/selftests/resctrl/resctrl_tests.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> > index eef9e02516ad..559868b16079 100644
> > --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> > +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> > @@ -52,7 +52,7 @@ int get_vendor(void)
> >  
> >  static void cmd_help(void)
> >  {
> > -	printf("usage: resctrl_tests [-h] [-b \"benchmark_cmd [options]\"] [-t test list] [-n no_of_bits]\n");
> > +	printf("usage: resctrl_tests [-h] [-t test list] [-n no_of_bits] [-b benchmark_cmd [option]...]\n");
> >  	printf("\t-b benchmark_cmd [options]: run specified benchmark for MBM, MBA and CMT\n");
> 
> Considering it was documented goal of patch to use "..." notation, should
> it be done consistently by changing above line also?

Yes, I'll do that.

-- 
 i.

Re: [PATCH 5/7] selftests/resctrl: Use pointers to build benchmark cmd and make it const

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 5627 bytes --]

On Mon, 14 Aug 2023, Reinette Chatre wrote:

> Hi Ilpo,
> 
> On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
> > Benchmark parameter uses fixed-size buffers in stack which is slightly
> > dangerous. As benchmark command is used in multiple tests, it should
> 
> Could you please be specific with issues with current implementation?
> The term "slightly dangerous" is vague.

I've reworded this so this fragment no longer remains here because the 
earlier patch got changes so the dangerous part is no longer there.

> > not be mutated by the tests. Due to the order of tests, mutating the
> > span argument in CMT test does not trigger any real problems currently.
> > 
> > Mark benchmark_cmd strings as const and setup the benchmark command
> > using pointers. As span is constant in main(), just provide the default
> > span also as string to be used in setting up the default fill_buf
> > argument so no malloc() is required for it.
> 
> What is wrong with using malloc()?

Nothing. I think you slightly misunderstood what I meant here.

The main challenge is not malloc() itself but keeping track of what memory 
has been dynamically allocated, which is simple if nothing has been 
malloc()ed. With the const benchmark command and default span, there's no 
need to malloc(), thus I avoid it to keep things simpler on the free() 
side.

I've tried to reword the entire changelog, please check the v2 changelog 
once I post it.

> > CMT test has to create a copy of the benchmark command before altering
> > the benchmark command.
> > 
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> >  tools/testing/selftests/resctrl/cmt_test.c    | 23 ++++++++++---
> >  tools/testing/selftests/resctrl/mba_test.c    |  2 +-
> >  tools/testing/selftests/resctrl/mbm_test.c    |  2 +-
> >  tools/testing/selftests/resctrl/resctrl.h     | 16 ++++++---
> >  .../testing/selftests/resctrl/resctrl_tests.c | 33 ++++++++-----------
> >  tools/testing/selftests/resctrl/resctrl_val.c | 10 ++++--
> >  6 files changed, 54 insertions(+), 32 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
> > index 9d8e38e995ef..a40e12c3b1a7 100644
> > --- a/tools/testing/selftests/resctrl/cmt_test.c
> > +++ b/tools/testing/selftests/resctrl/cmt_test.c
> > @@ -68,14 +68,16 @@ void cmt_test_cleanup(void)
> >  	remove(RESULT_FILE_NAME);
> >  }
> >  
> > -int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
> > +int cmt_resctrl_val(int cpu_no, int n, const char * const *benchmark_cmd)
> >  {
> > +	const char *cmd[BENCHMARK_ARGS];
> >  	unsigned long cache_size = 0;
> >  	unsigned long long_mask;
> > +	char *span_str = NULL;
> >  	char cbm_mask[256];
> >  	int count_of_bits;
> >  	size_t span;
> > -	int ret;
> > +	int ret, i;
> >  
> >  	if (!validate_resctrl_feature_request(CMT_STR))
> >  		return -1;
> > @@ -111,12 +113,22 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
> >  	};
> >  
> >  	span = cache_size * n / count_of_bits;
> > -	if (strcmp(benchmark_cmd[0], "fill_buf") == 0)
> > -		sprintf(benchmark_cmd[1], "%zu", span);
> > +	/* Duplicate the command to be able to replace span in it */
> > +	for (i = 0; benchmark_cmd[i]; i++)
> > +		cmd[i] = benchmark_cmd[i];
> > +	cmd[i] = NULL;
> > +
> > +	if (strcmp(cmd[0], "fill_buf") == 0) {
> > +		span_str = malloc(SIZE_MAX_DECIMAL_SIZE);
> > +		if (!span_str)
> > +			return -1;
> > +		snprintf(span_str, SIZE_MAX_DECIMAL_SIZE, "%zu", span);
> 
> Have you considered asprintf()?

Changed to asprintf() now.
 
> > +		cmd[1] = span_str;
> > +	}
> 
> It looks to me that array only needs to be duplicated if the
> default benchmark is used?

While it's true, another aspect is how that affects the code flow. If I 
make that change, the benchmark command could come from two different 
places which is now avoided. IMHO, the current approach is simpler to 
understand even if it does the unnecessary copy of a few pointers.

But please let me know if you still prefer the other way around so I can 
change to that.

> >  	remove(RESULT_FILE_NAME);
> >  
> > -	ret = resctrl_val(benchmark_cmd, &param);
> > +	ret = resctrl_val(cmd, &param);
> >  	if (ret)
> >  		goto out;
> >  
> 
> ...
> 
> > diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> > index bcd0d2060f81..ddb1e83a3a64 100644
> > --- a/tools/testing/selftests/resctrl/resctrl.h
> > +++ b/tools/testing/selftests/resctrl/resctrl.h
> > @@ -6,6 +6,7 @@
> >  #include <math.h>
> >  #include <errno.h>
> >  #include <sched.h>
> > +#include <stdint.h>
> >  #include <stdlib.h>
> >  #include <unistd.h>
> >  #include <string.h>
> > @@ -38,7 +39,14 @@
> >  
> >  #define END_OF_TESTS	1
> >  
> > +#define BENCHMARK_ARGS		64
> > +
> > +/* Approximate %zu max length */
> > +#define SIZE_MAX_DECIMAL_SIZE	(sizeof(SIZE_MAX) * 8 / 3 + 2)
> > +
> > +/* Define default span both as integer and string, these should match */
> >  #define DEFAULT_SPAN		(250 * MB)
> > +#define DEFAULT_SPAN_STR	"262144000"
> 
> I think above hardcoding can be eliminated by using asprintf()? This
> does allocate memory though so I would like to understand why one
> goal is to not dynamically allocate memory.

Because it's simpler on the _free() side_. If there's no allocation, no 
free() is needed.

Only challenge that remains is the int -> string conversion for the 
default span which can be either done like in the patch or using some 
preprocessor trickery to convert the number to string. If you prefer the 
latter, I can change to that so it's not hardcoded both as int and string.

-- 
 i.

Re: [PATCH 1/7] selftests/resctrl: Ensure the benchmark commands fits to its array

[Reinette Chatre]

Hi Ilpo,

On 8/15/2023 2:10 AM, Ilpo Järvinen wrote:
>> ps. Unless you have an updated email address that works, could you please
>> remove Sai's email from future submissions?
> 
> It's auto-added by git send-email machinery. I guess I can try to make 
> an exception to my usual workflow by sending only to manually specified To 
> addresses (if I remember). Perhaps one day I'll write a tool to filter out
> the addresses from git send-email generated ones but as is I don't have 
> one.
> 

Which git send-email machinery are you referring to? If I understand correctly
it does not automatically pick addresses but you can provide custom commands to
it that can do it.

Reinette

Re: [PATCH 5/7] selftests/resctrl: Use pointers to build benchmark cmd and make it const

[Reinette Chatre]

Hi Ilpo,

On 8/15/2023 2:42 AM, Ilpo Järvinen wrote:
> On Mon, 14 Aug 2023, Reinette Chatre wrote:
> 
>> Hi Ilpo,
>>
>> On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
>>> Benchmark parameter uses fixed-size buffers in stack which is slightly
>>> dangerous. As benchmark command is used in multiple tests, it should
>>
>> Could you please be specific with issues with current implementation?
>> The term "slightly dangerous" is vague.
> 
> I've reworded this so this fragment no longer remains here because the 
> earlier patch got changes so the dangerous part is no longer there.
> 
>>> not be mutated by the tests. Due to the order of tests, mutating the
>>> span argument in CMT test does not trigger any real problems currently.
>>>
>>> Mark benchmark_cmd strings as const and setup the benchmark command
>>> using pointers. As span is constant in main(), just provide the default
>>> span also as string to be used in setting up the default fill_buf
>>> argument so no malloc() is required for it.
>>
>> What is wrong with using malloc()?
> 
> Nothing. I think you slightly misunderstood what I meant here.
> 
> The main challenge is not malloc() itself but keeping track of what memory 
> has been dynamically allocated, which is simple if nothing has been 
> malloc()ed. With the const benchmark command and default span, there's no 
> need to malloc(), thus I avoid it to keep things simpler on the free() 
> side.

Keeping things symmetrical helps.

> 
> I've tried to reword the entire changelog, please check the v2 changelog 
> once I post it.
> 
>>> CMT test has to create a copy of the benchmark command before altering
>>> the benchmark command.
>>>
>>> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>>> ---
>>>  tools/testing/selftests/resctrl/cmt_test.c    | 23 ++++++++++---
>>>  tools/testing/selftests/resctrl/mba_test.c    |  2 +-
>>>  tools/testing/selftests/resctrl/mbm_test.c    |  2 +-
>>>  tools/testing/selftests/resctrl/resctrl.h     | 16 ++++++---
>>>  .../testing/selftests/resctrl/resctrl_tests.c | 33 ++++++++-----------
>>>  tools/testing/selftests/resctrl/resctrl_val.c | 10 ++++--
>>>  6 files changed, 54 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
>>> index 9d8e38e995ef..a40e12c3b1a7 100644
>>> --- a/tools/testing/selftests/resctrl/cmt_test.c
>>> +++ b/tools/testing/selftests/resctrl/cmt_test.c
>>> @@ -68,14 +68,16 @@ void cmt_test_cleanup(void)
>>>  	remove(RESULT_FILE_NAME);
>>>  }
>>>  
>>> -int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
>>> +int cmt_resctrl_val(int cpu_no, int n, const char * const *benchmark_cmd)
>>>  {
>>> +	const char *cmd[BENCHMARK_ARGS];
>>>  	unsigned long cache_size = 0;
>>>  	unsigned long long_mask;
>>> +	char *span_str = NULL;
>>>  	char cbm_mask[256];
>>>  	int count_of_bits;
>>>  	size_t span;
>>> -	int ret;
>>> +	int ret, i;
>>>  
>>>  	if (!validate_resctrl_feature_request(CMT_STR))
>>>  		return -1;
>>> @@ -111,12 +113,22 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
>>>  	};
>>>  
>>>  	span = cache_size * n / count_of_bits;
>>> -	if (strcmp(benchmark_cmd[0], "fill_buf") == 0)
>>> -		sprintf(benchmark_cmd[1], "%zu", span);
>>> +	/* Duplicate the command to be able to replace span in it */
>>> +	for (i = 0; benchmark_cmd[i]; i++)
>>> +		cmd[i] = benchmark_cmd[i];
>>> +	cmd[i] = NULL;
>>> +
>>> +	if (strcmp(cmd[0], "fill_buf") == 0) {
>>> +		span_str = malloc(SIZE_MAX_DECIMAL_SIZE);
>>> +		if (!span_str)
>>> +			return -1;
>>> +		snprintf(span_str, SIZE_MAX_DECIMAL_SIZE, "%zu", span);
>>
>> Have you considered asprintf()?
> 
> Changed to asprintf() now.
>  
>>> +		cmd[1] = span_str;
>>> +	}
>>
>> It looks to me that array only needs to be duplicated if the
>> default benchmark is used?
> 
> While it's true, another aspect is how that affects the code flow. If I 
> make that change, the benchmark command could come from two different 
> places which is now avoided. IMHO, the current approach is simpler to 
> understand even if it does the unnecessary copy of a few pointers.

cmd provided to resctrl_val() can point to original buffer or modified
buffer. What is wrong with a pointer possibly pointing to two different
locations? 

> 
> But please let me know if you still prefer the other way around so I can 
> change to that.

Your motivation for this approach is not clear to me.

> 
>>>  	remove(RESULT_FILE_NAME);
>>>  
>>> -	ret = resctrl_val(benchmark_cmd, &param);
>>> +	ret = resctrl_val(cmd, &param);
>>>  	if (ret)
>>>  		goto out;
>>>  
>>
>> ...
>>
>>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
>>> index bcd0d2060f81..ddb1e83a3a64 100644
>>> --- a/tools/testing/selftests/resctrl/resctrl.h
>>> +++ b/tools/testing/selftests/resctrl/resctrl.h
>>> @@ -6,6 +6,7 @@
>>>  #include <math.h>
>>>  #include <errno.h>
>>>  #include <sched.h>
>>> +#include <stdint.h>
>>>  #include <stdlib.h>
>>>  #include <unistd.h>
>>>  #include <string.h>
>>> @@ -38,7 +39,14 @@
>>>  
>>>  #define END_OF_TESTS	1
>>>  
>>> +#define BENCHMARK_ARGS		64
>>> +
>>> +/* Approximate %zu max length */
>>> +#define SIZE_MAX_DECIMAL_SIZE	(sizeof(SIZE_MAX) * 8 / 3 + 2)
>>> +
>>> +/* Define default span both as integer and string, these should match */
>>>  #define DEFAULT_SPAN		(250 * MB)
>>> +#define DEFAULT_SPAN_STR	"262144000"
>>
>> I think above hardcoding can be eliminated by using asprintf()? This
>> does allocate memory though so I would like to understand why one
>> goal is to not dynamically allocate memory.
> 
> Because it's simpler on the _free() side_. If there's no allocation, no 
> free() is needed.
> 
> Only challenge that remains is the int -> string conversion for the 
> default span which can be either done like in the patch or using some 
> preprocessor trickery to convert the number to string. If you prefer the 
> latter, I can change to that so it's not hardcoded both as int and string.
> 

This manual int->string sounds like the trickery to me and can be avoided
by just using asprintf(). I understand that no free() is needed when no
memory is allocated but it looks to me as though these allocations can
be symmetrical - allocate the memory before the tests are run and free it
after?

Reinette

Re: [PATCH 1/7] selftests/resctrl: Ensure the benchmark commands fits to its array

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1899 bytes --]

On Tue, 15 Aug 2023, Reinette Chatre wrote:
> On 8/15/2023 2:10 AM, Ilpo Järvinen wrote:
> >> ps. Unless you have an updated email address that works, could you please
> >> remove Sai's email from future submissions?
> > 
> > It's auto-added by git send-email machinery. I guess I can try to make 
> > an exception to my usual workflow by sending only to manually specified To 
> > addresses (if I remember). Perhaps one day I'll write a tool to filter out
> > the addresses from git send-email generated ones but as is I don't have 
> > one.
> > 
> 
> Which git send-email machinery are you referring to? If I understand correctly
> it does not automatically pick addresses but you can provide custom commands to
> it that can do it.

Ah sorry, it is actually scripts/get_maintainer.pl automation I use with 
git send-email to figure out where to send the patches besides the --to & 
--cc entries I provided. For this patch, get_maintainer.pl returns this 
list:

Fenghua Yu <fenghua.yu@intel.com> (supporter:RDT - RESOURCE ALLOCATION,blamed_fixes:1/1=100%)
Reinette Chatre <reinette.chatre@intel.com> (supporter:RDT - RESOURCE ALLOCATION)
Shuah Khan <shuah@kernel.org> (maintainer:KERNEL SELFTEST FRAMEWORK,blamed_fixes:1/1=100%)
Babu Moger <babu.moger@amd.com> (blamed_fixes:1/1=100%)
Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com> (blamed_fixes:1/1=100%)
linux-kernel@vger.kernel.org (open list:RDT - RESOURCE ALLOCATION)
linux-kselftest@vger.kernel.org (open list:KERNEL SELFTEST FRAMEWORK)

...which includes Sai's address (not much I can do about that, it's 
immutably crafted into git history that those lines were once touched by 
Sai). I've thought of writing yet another wrapper to filter out known 
failing addresses but until that's done, either I need to (remember to) 
manually send the series w/o get_maintainer.pl automation or accept a few 
failures here and there.

-- 
 i.

Re: [PATCH 5/7] selftests/resctrl: Use pointers to build benchmark cmd and make it const

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 7064 bytes --]

On Tue, 15 Aug 2023, Reinette Chatre wrote:
> On 8/15/2023 2:42 AM, Ilpo Järvinen wrote:
> > On Mon, 14 Aug 2023, Reinette Chatre wrote:
> >>
> >> On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
> >>> Benchmark parameter uses fixed-size buffers in stack which is slightly
> >>> dangerous. As benchmark command is used in multiple tests, it should
> >>
> >> Could you please be specific with issues with current implementation?
> >> The term "slightly dangerous" is vague.
> > 
> > I've reworded this so this fragment no longer remains here because the 
> > earlier patch got changes so the dangerous part is no longer there.
> > 
> >>> not be mutated by the tests. Due to the order of tests, mutating the
> >>> span argument in CMT test does not trigger any real problems currently.
> >>>
> >>> Mark benchmark_cmd strings as const and setup the benchmark command
> >>> using pointers. As span is constant in main(), just provide the default
> >>> span also as string to be used in setting up the default fill_buf
> >>> argument so no malloc() is required for it.
> >>
> >> What is wrong with using malloc()?
> > 
> > Nothing. I think you slightly misunderstood what I meant here.
> > 
> > The main challenge is not malloc() itself but keeping track of what memory 
> > has been dynamically allocated, which is simple if nothing has been 
> > malloc()ed. With the const benchmark command and default span, there's no 
> > need to malloc(), thus I avoid it to keep things simpler on the free() 
> > side.
> 
> Keeping things symmetrical helps.
>
> > I've tried to reword the entire changelog, please check the v2 changelog 
> > once I post it.
> > 
> >>> CMT test has to create a copy of the benchmark command before altering
> >>> the benchmark command.
> >>>
> >>> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> >>> ---
> >>>  tools/testing/selftests/resctrl/cmt_test.c    | 23 ++++++++++---
> >>>  tools/testing/selftests/resctrl/mba_test.c    |  2 +-
> >>>  tools/testing/selftests/resctrl/mbm_test.c    |  2 +-
> >>>  tools/testing/selftests/resctrl/resctrl.h     | 16 ++++++---
> >>>  .../testing/selftests/resctrl/resctrl_tests.c | 33 ++++++++-----------
> >>>  tools/testing/selftests/resctrl/resctrl_val.c | 10 ++++--
> >>>  6 files changed, 54 insertions(+), 32 deletions(-)
> >>>
> >>> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
> >>> index 9d8e38e995ef..a40e12c3b1a7 100644
> >>> --- a/tools/testing/selftests/resctrl/cmt_test.c
> >>> +++ b/tools/testing/selftests/resctrl/cmt_test.c
> >>> @@ -68,14 +68,16 @@ void cmt_test_cleanup(void)
> >>>  	remove(RESULT_FILE_NAME);
> >>>  }
> >>>  
> >>> -int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
> >>> +int cmt_resctrl_val(int cpu_no, int n, const char * const *benchmark_cmd)
> >>>  {
> >>> +	const char *cmd[BENCHMARK_ARGS];
> >>>  	unsigned long cache_size = 0;
> >>>  	unsigned long long_mask;
> >>> +	char *span_str = NULL;
> >>>  	char cbm_mask[256];
> >>>  	int count_of_bits;
> >>>  	size_t span;
> >>> -	int ret;
> >>> +	int ret, i;
> >>>  
> >>>  	if (!validate_resctrl_feature_request(CMT_STR))
> >>>  		return -1;
> >>> @@ -111,12 +113,22 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
> >>>  	};
> >>>  
> >>>  	span = cache_size * n / count_of_bits;
> >>> -	if (strcmp(benchmark_cmd[0], "fill_buf") == 0)
> >>> -		sprintf(benchmark_cmd[1], "%zu", span);
> >>> +	/* Duplicate the command to be able to replace span in it */
> >>> +	for (i = 0; benchmark_cmd[i]; i++)
> >>> +		cmd[i] = benchmark_cmd[i];
> >>> +	cmd[i] = NULL;
> >>> +
> >>> +	if (strcmp(cmd[0], "fill_buf") == 0) {
> >>> +		span_str = malloc(SIZE_MAX_DECIMAL_SIZE);
> >>> +		if (!span_str)
> >>> +			return -1;
> >>> +		snprintf(span_str, SIZE_MAX_DECIMAL_SIZE, "%zu", span);
> >>
> >> Have you considered asprintf()?
> > 
> > Changed to asprintf() now.
> >  
> >>> +		cmd[1] = span_str;
> >>> +	}
> >>
> >> It looks to me that array only needs to be duplicated if the
> >> default benchmark is used?
> > 
> > While it's true, another aspect is how that affects the code flow. If I 
> > make that change, the benchmark command could come from two different 
> > places which is now avoided. IMHO, the current approach is simpler to 
> > understand even if it does the unnecessary copy of a few pointers.
> 
> cmd provided to resctrl_val() can point to original buffer or modified
> buffer. What is wrong with a pointer possibly pointing to two different
> locations? 

I'll change to that.

> > But please let me know if you still prefer the other way around so I can 
> > change to that.
> 
> Your motivation for this approach is not clear to me.
> 
> > 
> >>>  	remove(RESULT_FILE_NAME);
> >>>  
> >>> -	ret = resctrl_val(benchmark_cmd, &param);
> >>> +	ret = resctrl_val(cmd, &param);
> >>>  	if (ret)
> >>>  		goto out;
> >>>  
> >>
> >> ...
> >>
> >>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> >>> index bcd0d2060f81..ddb1e83a3a64 100644
> >>> --- a/tools/testing/selftests/resctrl/resctrl.h
> >>> +++ b/tools/testing/selftests/resctrl/resctrl.h
> >>> @@ -6,6 +6,7 @@
> >>>  #include <math.h>
> >>>  #include <errno.h>
> >>>  #include <sched.h>
> >>> +#include <stdint.h>
> >>>  #include <stdlib.h>
> >>>  #include <unistd.h>
> >>>  #include <string.h>
> >>> @@ -38,7 +39,14 @@
> >>>  
> >>>  #define END_OF_TESTS	1
> >>>  
> >>> +#define BENCHMARK_ARGS		64
> >>> +
> >>> +/* Approximate %zu max length */
> >>> +#define SIZE_MAX_DECIMAL_SIZE	(sizeof(SIZE_MAX) * 8 / 3 + 2)
> >>> +
> >>> +/* Define default span both as integer and string, these should match */
> >>>  #define DEFAULT_SPAN		(250 * MB)
> >>> +#define DEFAULT_SPAN_STR	"262144000"
> >>
> >> I think above hardcoding can be eliminated by using asprintf()? This
> >> does allocate memory though so I would like to understand why one
> >> goal is to not dynamically allocate memory.
> > 
> > Because it's simpler on the _free() side_. If there's no allocation, no 
> > free() is needed.
> > 
> > Only challenge that remains is the int -> string conversion for the 
> > default span which can be either done like in the patch or using some 
> > preprocessor trickery to convert the number to string. If you prefer the 
> > latter, I can change to that so it's not hardcoded both as int and string.
> > 
> 
> This manual int->string sounds like the trickery to me and can be avoided
> by just using asprintf(). I understand that no free() is needed when no
> memory is allocated but it looks to me as though these allocations can
> be symmetrical - allocate the memory before the tests are run and free it
> after?

It could be symmetrical but that means I'll be doing unnecessary alloc if 
-b is provided which I assume you're against given your comment on always 
creating copy of cmd in CMT test's case.

I think I'll use similar resolution to this as CMT test does, it has an 
extra variable which is NULL in when -b is provided so free() is no-op
on that path. Then I can use asprintf().

-- 
 i.

Re: [PATCH 1/7] selftests/resctrl: Ensure the benchmark commands fits to its array

[Reinette Chatre]

Hi Ilpo,

On 8/15/2023 11:32 PM, Ilpo Järvinen wrote:
> Ah sorry, it is actually scripts/get_maintainer.pl automation I use with 
> git send-email to figure out where to send the patches besides the --to & 
> --cc entries I provided. For this patch, get_maintainer.pl returns this 
> list:
> 
> Fenghua Yu <fenghua.yu@intel.com> (supporter:RDT - RESOURCE ALLOCATION,blamed_fixes:1/1=100%)
> Reinette Chatre <reinette.chatre@intel.com> (supporter:RDT - RESOURCE ALLOCATION)
> Shuah Khan <shuah@kernel.org> (maintainer:KERNEL SELFTEST FRAMEWORK,blamed_fixes:1/1=100%)
> Babu Moger <babu.moger@amd.com> (blamed_fixes:1/1=100%)
> Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com> (blamed_fixes:1/1=100%)
> linux-kernel@vger.kernel.org (open list:RDT - RESOURCE ALLOCATION)
> linux-kselftest@vger.kernel.org (open list:KERNEL SELFTEST FRAMEWORK)
> 
> ...which includes Sai's address (not much I can do about that, it's 
> immutably crafted into git history that those lines were once touched by 
> Sai). I've thought of writing yet another wrapper to filter out known 
> failing addresses but until that's done, either I need to (remember to) 
> manually send the series w/o get_maintainer.pl automation or accept a few 
> failures here and there.
> 

You can append Sai's address to .get_maintainer.ignore.

Reinette

Re: [PATCH 5/7] selftests/resctrl: Use pointers to build benchmark cmd and make it const

[Reinette Chatre]

Hi Ilpo,

On 8/16/2023 12:13 AM, Ilpo Järvinen wrote:
> On Tue, 15 Aug 2023, Reinette Chatre wrote:
>> On 8/15/2023 2:42 AM, Ilpo Järvinen wrote:
>>> On Mon, 14 Aug 2023, Reinette Chatre wrote:
>>>>
>>>> On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:

...
>>>>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
>>>>> index bcd0d2060f81..ddb1e83a3a64 100644
>>>>> --- a/tools/testing/selftests/resctrl/resctrl.h
>>>>> +++ b/tools/testing/selftests/resctrl/resctrl.h
>>>>> @@ -6,6 +6,7 @@
>>>>>  #include <math.h>
>>>>>  #include <errno.h>
>>>>>  #include <sched.h>
>>>>> +#include <stdint.h>
>>>>>  #include <stdlib.h>
>>>>>  #include <unistd.h>
>>>>>  #include <string.h>
>>>>> @@ -38,7 +39,14 @@
>>>>>  
>>>>>  #define END_OF_TESTS	1
>>>>>  
>>>>> +#define BENCHMARK_ARGS		64
>>>>> +
>>>>> +/* Approximate %zu max length */
>>>>> +#define SIZE_MAX_DECIMAL_SIZE	(sizeof(SIZE_MAX) * 8 / 3 + 2)
>>>>> +
>>>>> +/* Define default span both as integer and string, these should match */
>>>>>  #define DEFAULT_SPAN		(250 * MB)
>>>>> +#define DEFAULT_SPAN_STR	"262144000"
>>>>
>>>> I think above hardcoding can be eliminated by using asprintf()? This
>>>> does allocate memory though so I would like to understand why one
>>>> goal is to not dynamically allocate memory.
>>>
>>> Because it's simpler on the _free() side_. If there's no allocation, no 
>>> free() is needed.
>>>
>>> Only challenge that remains is the int -> string conversion for the 
>>> default span which can be either done like in the patch or using some 
>>> preprocessor trickery to convert the number to string. If you prefer the 
>>> latter, I can change to that so it's not hardcoded both as int and string.
>>>
>>
>> This manual int->string sounds like the trickery to me and can be avoided
>> by just using asprintf(). I understand that no free() is needed when no
>> memory is allocated but it looks to me as though these allocations can
>> be symmetrical - allocate the memory before the tests are run and free it
>> after?
> 
> It could be symmetrical but that means I'll be doing unnecessary alloc if 
> -b is provided which I assume you're against given your comment on always 
> creating copy of cmd in CMT test's case.

I seemed to have lost track here ... could you please elaborate where the
unnecessary alloc will be?

> 
> I think I'll use similar resolution to this as CMT test does, it has an 
> extra variable which is NULL in when -b is provided so free() is no-op
> on that path. Then I can use asprintf().
> 

Reinette

Re: [PATCH 5/7] selftests/resctrl: Use pointers to build benchmark cmd and make it const

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3100 bytes --]

On Wed, 16 Aug 2023, Reinette Chatre wrote:
> On 8/16/2023 12:13 AM, Ilpo Järvinen wrote:
> > On Tue, 15 Aug 2023, Reinette Chatre wrote:
> >> On 8/15/2023 2:42 AM, Ilpo Järvinen wrote:
> >>> On Mon, 14 Aug 2023, Reinette Chatre wrote:
> >>>>
> >>>> On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
> 
> ...
> >>>>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> >>>>> index bcd0d2060f81..ddb1e83a3a64 100644
> >>>>> --- a/tools/testing/selftests/resctrl/resctrl.h
> >>>>> +++ b/tools/testing/selftests/resctrl/resctrl.h
> >>>>> @@ -6,6 +6,7 @@
> >>>>>  #include <math.h>
> >>>>>  #include <errno.h>
> >>>>>  #include <sched.h>
> >>>>> +#include <stdint.h>
> >>>>>  #include <stdlib.h>
> >>>>>  #include <unistd.h>
> >>>>>  #include <string.h>
> >>>>> @@ -38,7 +39,14 @@
> >>>>>  
> >>>>>  #define END_OF_TESTS	1
> >>>>>  
> >>>>> +#define BENCHMARK_ARGS		64
> >>>>> +
> >>>>> +/* Approximate %zu max length */
> >>>>> +#define SIZE_MAX_DECIMAL_SIZE	(sizeof(SIZE_MAX) * 8 / 3 + 2)
> >>>>> +
> >>>>> +/* Define default span both as integer and string, these should match */
> >>>>>  #define DEFAULT_SPAN		(250 * MB)
> >>>>> +#define DEFAULT_SPAN_STR	"262144000"
> >>>>
> >>>> I think above hardcoding can be eliminated by using asprintf()? This
> >>>> does allocate memory though so I would like to understand why one
> >>>> goal is to not dynamically allocate memory.
> >>>
> >>> Because it's simpler on the _free() side_. If there's no allocation, no 
> >>> free() is needed.
> >>>
> >>> Only challenge that remains is the int -> string conversion for the 
> >>> default span which can be either done like in the patch or using some 
> >>> preprocessor trickery to convert the number to string. If you prefer the 
> >>> latter, I can change to that so it's not hardcoded both as int and string.
> >>>
> >>
> >> This manual int->string sounds like the trickery to me and can be avoided
> >> by just using asprintf(). I understand that no free() is needed when no
> >> memory is allocated but it looks to me as though these allocations can
> >> be symmetrical - allocate the memory before the tests are run and free it
> >> after?
> > 
> > It could be symmetrical but that means I'll be doing unnecessary alloc if 
> > -b is provided which I assume you're against given your comment on always 
> > creating copy of cmd in CMT test's case.
> 
> I seemed to have lost track here ... could you please elaborate where the
> unnecessary alloc will be?

If there's what you call "symmetry", it implies the code always does 
alloc. However, the logic in main() is such that when -b is provided, no 
default benchmark command needs to be assigned, so no alloc for span is 
necessary. Thus, there either is unnecessary alloc with -b or _no 
symmetry_.

But I've already converted to asprintf() so no need to continue this 
discussion.

> > I think I'll use similar resolution to this as CMT test does, it has an 
> > extra variable which is NULL in when -b is provided so free() is no-op
> > on that path. Then I can use asprintf().
> > 
> 
> Reinette
> 

-- 
 i.

Re: [PATCH 5/7] selftests/resctrl: Use pointers to build benchmark cmd and make it const

[Reinette Chatre]

Hi Ilpo,

On 8/17/2023 1:32 AM, Ilpo Järvinen wrote:
> On Wed, 16 Aug 2023, Reinette Chatre wrote:
>> On 8/16/2023 12:13 AM, Ilpo Järvinen wrote:
>>> On Tue, 15 Aug 2023, Reinette Chatre wrote:
>>>> On 8/15/2023 2:42 AM, Ilpo Järvinen wrote:
>>>>> On Mon, 14 Aug 2023, Reinette Chatre wrote:
>>>>>>
>>>>>> On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
>>
>> ...
>>>>>>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
>>>>>>> index bcd0d2060f81..ddb1e83a3a64 100644
>>>>>>> --- a/tools/testing/selftests/resctrl/resctrl.h
>>>>>>> +++ b/tools/testing/selftests/resctrl/resctrl.h
>>>>>>> @@ -6,6 +6,7 @@
>>>>>>>  #include <math.h>
>>>>>>>  #include <errno.h>
>>>>>>>  #include <sched.h>
>>>>>>> +#include <stdint.h>
>>>>>>>  #include <stdlib.h>
>>>>>>>  #include <unistd.h>
>>>>>>>  #include <string.h>
>>>>>>> @@ -38,7 +39,14 @@
>>>>>>>  
>>>>>>>  #define END_OF_TESTS	1
>>>>>>>  
>>>>>>> +#define BENCHMARK_ARGS		64
>>>>>>> +
>>>>>>> +/* Approximate %zu max length */
>>>>>>> +#define SIZE_MAX_DECIMAL_SIZE	(sizeof(SIZE_MAX) * 8 / 3 + 2)
>>>>>>> +
>>>>>>> +/* Define default span both as integer and string, these should match */
>>>>>>>  #define DEFAULT_SPAN		(250 * MB)
>>>>>>> +#define DEFAULT_SPAN_STR	"262144000"
>>>>>>
>>>>>> I think above hardcoding can be eliminated by using asprintf()? This
>>>>>> does allocate memory though so I would like to understand why one
>>>>>> goal is to not dynamically allocate memory.
>>>>>
>>>>> Because it's simpler on the _free() side_. If there's no allocation, no 
>>>>> free() is needed.
>>>>>
>>>>> Only challenge that remains is the int -> string conversion for the 
>>>>> default span which can be either done like in the patch or using some 
>>>>> preprocessor trickery to convert the number to string. If you prefer the 
>>>>> latter, I can change to that so it's not hardcoded both as int and string.
>>>>>
>>>>
>>>> This manual int->string sounds like the trickery to me and can be avoided
>>>> by just using asprintf(). I understand that no free() is needed when no
>>>> memory is allocated but it looks to me as though these allocations can
>>>> be symmetrical - allocate the memory before the tests are run and free it
>>>> after?
>>>
>>> It could be symmetrical but that means I'll be doing unnecessary alloc if 
>>> -b is provided which I assume you're against given your comment on always 
>>> creating copy of cmd in CMT test's case.
>>
>> I seemed to have lost track here ... could you please elaborate where the
>> unnecessary alloc will be?
> 
> If there's what you call "symmetry", it implies the code always does 
> alloc. However, the logic in main() is such that when -b is provided, no 

No. Symmetry does not mean "always alloc" - what I attempted to covey was
that tracking allocations become easier if the memory is freed in code
that is symmetrical to where the memory is allocated. For example, if memory
is allocated at the beginning of main(), then it is freed on exit of main(),
or if there is a "test_resources_alloc()" that is called before a test is
run then there could be a "test_resources_free()" that is called
after a test is run.

> default benchmark command needs to be assigned, so no alloc for span is 
> necessary. Thus, there either is unnecessary alloc with -b or _no 
> symmetry_.
> 
> But I've already converted to asprintf() so no need to continue this 
> discussion.

Please note that asprintf() allocates memory that needs to be freed.

Reinette

Re: [PATCH 5/7] selftests/resctrl: Use pointers to build benchmark cmd and make it const

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 4621 bytes --]

On Thu, 17 Aug 2023, Reinette Chatre wrote:
> On 8/17/2023 1:32 AM, Ilpo Järvinen wrote:
> > On Wed, 16 Aug 2023, Reinette Chatre wrote:
> >> On 8/16/2023 12:13 AM, Ilpo Järvinen wrote:
> >>> On Tue, 15 Aug 2023, Reinette Chatre wrote:
> >>>> On 8/15/2023 2:42 AM, Ilpo Järvinen wrote:
> >>>>> On Mon, 14 Aug 2023, Reinette Chatre wrote:
> >>>>>> On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
> >>>>>>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> >>>>>>> index bcd0d2060f81..ddb1e83a3a64 100644
> >>>>>>> --- a/tools/testing/selftests/resctrl/resctrl.h
> >>>>>>> +++ b/tools/testing/selftests/resctrl/resctrl.h
> >>>>>>> @@ -6,6 +6,7 @@
> >>>>>>>  #include <math.h>
> >>>>>>>  #include <errno.h>
> >>>>>>>  #include <sched.h>
> >>>>>>> +#include <stdint.h>
> >>>>>>>  #include <stdlib.h>
> >>>>>>>  #include <unistd.h>
> >>>>>>>  #include <string.h>
> >>>>>>> @@ -38,7 +39,14 @@
> >>>>>>>  
> >>>>>>>  #define END_OF_TESTS	1
> >>>>>>>  
> >>>>>>> +#define BENCHMARK_ARGS		64
> >>>>>>> +
> >>>>>>> +/* Approximate %zu max length */
> >>>>>>> +#define SIZE_MAX_DECIMAL_SIZE	(sizeof(SIZE_MAX) * 8 / 3 + 2)
> >>>>>>> +
> >>>>>>> +/* Define default span both as integer and string, these should match */
> >>>>>>>  #define DEFAULT_SPAN		(250 * MB)
> >>>>>>> +#define DEFAULT_SPAN_STR	"262144000"
> >>>>>>
> >>>>>> I think above hardcoding can be eliminated by using asprintf()? This
> >>>>>> does allocate memory though so I would like to understand why one
> >>>>>> goal is to not dynamically allocate memory.
> >>>>>
> >>>>> Because it's simpler on the _free() side_. If there's no allocation, no 
> >>>>> free() is needed.
> >>>>>
> >>>>> Only challenge that remains is the int -> string conversion for the 
> >>>>> default span which can be either done like in the patch or using some 
> >>>>> preprocessor trickery to convert the number to string. If you prefer the 
> >>>>> latter, I can change to that so it's not hardcoded both as int and string.
> >>>>>
> >>>>
> >>>> This manual int->string sounds like the trickery to me and can be avoided
> >>>> by just using asprintf(). I understand that no free() is needed when no
> >>>> memory is allocated but it looks to me as though these allocations can
> >>>> be symmetrical - allocate the memory before the tests are run and free it
> >>>> after?
> >>>
> >>> It could be symmetrical but that means I'll be doing unnecessary alloc if 
> >>> -b is provided which I assume you're against given your comment on always 
> >>> creating copy of cmd in CMT test's case.
> >>
> >> I seemed to have lost track here ... could you please elaborate where the
> >> unnecessary alloc will be?
> > 
> > If there's what you call "symmetry", it implies the code always does 
> > alloc. However, the logic in main() is such that when -b is provided, no 
> 
> No. Symmetry does not mean "always alloc"

Oh, so it simply meant code without memory leaks :-).

> - what I attempted to covey was
> that tracking allocations become easier if the memory is freed in code
> that is symmetrical to where the memory is allocated.

That's, unfortunately, what I needed to do even if it resulted in less 
clean code when I, in a later patch that is not part of this series, 
added a function the setup the default parameters into user parameters 
struct. main() will now pass that span_str for it to do "symmetrical" 
free inside main().

> For example, if memory
> is allocated at the beginning of main(), then it is freed on exit of main(),

you make it sound easier than the reality is. There's no singular point 
that is "exit of main()". It has way too many exit paths because of how 
selftests framework works. It doesn't give you back control when you ask 
it to exit the tests.

You'll see how complicated this gets once we get to the user parameters 
structure patch but I'll use asprintf()+free() for now ;-). We can revisit 
this discussion if you feel like it when we get to that patch.

...And to think this all is because C cannot easily make known constant 
int -> string conversion w/o some runtime code.

> or if there is a "test_resources_alloc()" that is called before a test is
> run then there could be a "test_resources_free()" that is called
> after a test is run.
> 
> > default benchmark command needs to be assigned, so no alloc for span is 
> > necessary. Thus, there either is unnecessary alloc with -b or _no 
> > symmetry_.
> > 
> > But I've already converted to asprintf() so no need to continue this 
> > discussion.
> 
> Please note that asprintf() allocates memory that needs to be freed.

Of course.

-- 
 i.