[PATCH v2 00/13] selftests/resctrl: resctrl_val() related cleanups & improvements

[PATCH v2 00/13] selftests/resctrl: resctrl_val() related cleanups & improvements

[Ilpo Järvinen]

Hi all,

This series does a number of cleanups into resctrl_val() and
generalizes it by removing test name specific handling from the
function.

One of the changes improves MBA/MBM measurement by narrowing down the
period the resctrl FS derived memory bandwidth numbers are measured
over. My feel is it didn't cause noticeable difference into the numbers
because they're generally good anyway except for the small number of
outliers. To see the impact on outliers, I'd need to setup a test to
run large number of replications and do a statistical analysis, which
I've not spent my time on. Even without the statistical analysis, the
new way to measure seems obviously better and makes sense even if I
cannot see a major improvement with the setup I'm using.

This series has some conflicts with SNC series from Maciej and also
with the MBA/MBM series from Babu.

--
 i.

v2:
- Resolved conflicts with kselftest/next
- Spaces -> tabs correction

Ilpo Järvinen (13):
  selftests/resctrl: Convert get_mem_bw_imc() fd close to for loop
  selftests/resctrl: Calculate resctrl FS derived mem bw over sleep(1)
    only
  selftests/resctrl: Consolidate get_domain_id() into resctrl_val()
  selftests/resctrl: Use correct type for pids
  selftests/resctrl: Cleanup bm_pid and ppid usage & limit scope
  selftests/resctrl: Rename measure_vals() to measure_mem_bw_vals() &
    document
  selftests/resctrl: Add ->measure() callback to resctrl_val_param
  selftests/resctrl: Add ->init() callback into resctrl_val_param
  selftests/resctrl: Simplify bandwidth report type handling
  selftests/resctrl: Make some strings passed to resctrlfs functions
    const
  selftests/resctrl: Convert ctrlgrp & mongrp to pointers
  selftests/resctrl: Remove mongrp from MBA test
  selftests/resctrl: Remove test name comparing from
    write_bm_pid_to_resctrl()

 tools/testing/selftests/resctrl/cache.c       |   6 +-
 tools/testing/selftests/resctrl/cat_test.c    |   5 +-
 tools/testing/selftests/resctrl/cmt_test.c    |  21 +-
 tools/testing/selftests/resctrl/mba_test.c    |  34 ++-
 tools/testing/selftests/resctrl/mbm_test.c    |  33 ++-
 tools/testing/selftests/resctrl/resctrl.h     |  48 ++--
 tools/testing/selftests/resctrl/resctrl_val.c | 269 ++++++------------
 tools/testing/selftests/resctrl/resctrlfs.c   |  55 ++--
 8 files changed, 224 insertions(+), 247 deletions(-)

-- 
2.39.2

[PATCH v2 01/13] selftests/resctrl: Convert get_mem_bw_imc() fd close to for loop

[Ilpo Järvinen]

The open() side handles fds in a for loop but close() is based on two
fixed indexes READ and WRITE.

Match the close() side with the open() side by using for loop for
consistency.

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

diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 5a49f07a6c85..36139cba7be8 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -368,10 +368,9 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
 		writes += w->return_value.value * of_mul_write * SCALE;
 	}
 
-	for (imc = 0; imc < imcs; imc++) {
-		close(imc_counters_config[imc][READ].fd);
-		close(imc_counters_config[imc][WRITE].fd);
-	}
+	for (imc = 0; imc < imcs; imc++)
+		for (j = 0; j < 2; j++)
+			close(imc_counters_config[imc][j].fd);
 
 	if (strcmp(bw_report, "reads") == 0) {
 		*bw_imc = reads;
-- 
2.39.2

[PATCH v2 02/13] selftests/resctrl: Calculate resctrl FS derived mem bw over sleep(1) only

[Ilpo Järvinen]

For MBM/MBA tests, measure_vals() calls get_mem_bw_imc() that performs
the measurement over a duration of sleep(1) call. The memory bandwidth
numbers from IMC are derived over this duration. The resctrl FS derived
memory bandwidth, however, is calculated inside measure_vals() and only
takes delta between the previous value and the current one which
besides the actual test, also samples inter-test noise.

Rework the logic in measure_vals() and get_mem_bw_imc() such that the
resctrl FS memory bandwidth section covers much shorter duration
closely matching that of the IMC perf counters to improve measurement
accuracy.

Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/resctrl_val.c | 72 +++++++++++++------
 1 file changed, 50 insertions(+), 22 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 36139cba7be8..4df2cd738f88 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -293,28 +293,35 @@ static int initialize_mem_bw_imc(void)
 }
 
 /*
- * get_mem_bw_imc:	Memory band width as reported by iMC counters
+ * perf_open_imc_mem_bw - Open perf fds for IMCs
  * @cpu_no:		CPU number that the benchmark PID is binded to
- * @bw_report:		Bandwidth report type (reads, writes)
- *
- * Memory B/W utilized by a process on a socket can be calculated using
- * iMC counters. Perf events are used to read these counters.
- *
- * Return: = 0 on success. < 0 on failure.
  */
-static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
+static int perf_open_imc_mem_bw(int cpu_no)
 {
-	float reads, writes, of_mul_read, of_mul_write;
 	int imc, j, ret;
 
-	/* Start all iMC counters to log values (both read and write) */
-	reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
 	for (imc = 0; imc < imcs; imc++) {
 		for (j = 0; j < 2; j++) {
 			ret = open_perf_event(imc, cpu_no, j);
 			if (ret)
 				return -1;
 		}
+	}
+
+	return 0;
+}
+
+/*
+ * do_mem_bw_test - Perform memory bandwidth test
+ *
+ * Runs memory bandwidth test over one second period. Also, handles starting
+ * and stopping of the IMC perf counters around the test.
+ */
+static void do_imc_mem_bw_test(void)
+{
+	int imc, j;
+
+	for (imc = 0; imc < imcs; imc++) {
 		for (j = 0; j < 2; j++)
 			membw_ioctl_perf_event_ioc_reset_enable(imc, j);
 	}
@@ -326,6 +333,24 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
 		for (j = 0; j < 2; j++)
 			membw_ioctl_perf_event_ioc_disable(imc, j);
 	}
+}
+
+/*
+ * get_mem_bw_imc - Memory band width as reported by iMC counters
+ * @bw_report:		Bandwidth report type (reads, writes)
+ *
+ * Memory B/W utilized by a process on a socket can be calculated using
+ * iMC counters. Perf events are used to read these counters.
+ *
+ * Return: = 0 on success. < 0 on failure.
+ */
+static int get_mem_bw_imc(char *bw_report, float *bw_imc)
+{
+	float reads, writes, of_mul_read, of_mul_write;
+	int imc, j;
+
+	/* Start all iMC counters to log values (both read and write) */
+	reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
 
 	/*
 	 * Get results which are stored in struct type imc_counter_config
@@ -593,10 +618,9 @@ static void initialize_llc_occu_resctrl(const char *ctrlgrp, const char *mongrp,
 }
 
 static int measure_vals(const struct user_params *uparams,
-			struct resctrl_val_param *param,
-			unsigned long *bw_resc_start)
+			struct resctrl_val_param *param)
 {
-	unsigned long bw_resc, bw_resc_end;
+	unsigned long bw_resc, bw_resc_start, bw_resc_end;
 	float bw_imc;
 	int ret;
 
@@ -607,22 +631,27 @@ static int measure_vals(const struct user_params *uparams,
 	 * Compare the two values to validate resctrl value.
 	 * It takes 1sec to measure the data.
 	 */
-	ret = get_mem_bw_imc(uparams->cpu, param->bw_report, &bw_imc);
+	ret = perf_open_imc_mem_bw(uparams->cpu);
 	if (ret < 0)
 		return ret;
 
+	ret = get_mem_bw_resctrl(&bw_resc_start);
+	if (ret < 0)
+		return ret;
+
+	do_imc_mem_bw_test();
+
 	ret = get_mem_bw_resctrl(&bw_resc_end);
 	if (ret < 0)
 		return ret;
 
-	bw_resc = (bw_resc_end - *bw_resc_start) / MB;
-	ret = print_results_bw(param->filename, bm_pid, bw_imc, bw_resc);
-	if (ret)
+	ret = get_mem_bw_imc(param->bw_report, &bw_imc);
+	if (ret < 0)
 		return ret;
 
-	*bw_resc_start = bw_resc_end;
+	bw_resc = (bw_resc_end - bw_resc_start) / MB;
 
-	return 0;
+	return print_results_bw(param->filename, bm_pid, bw_imc, bw_resc);
 }
 
 /*
@@ -696,7 +725,6 @@ int resctrl_val(const struct resctrl_test *test,
 		struct resctrl_val_param *param)
 {
 	char *resctrl_val = param->resctrl_val;
-	unsigned long bw_resc_start = 0;
 	struct sigaction sigact;
 	int ret = 0, pipefd[2];
 	char pipe_message = 0;
@@ -838,7 +866,7 @@ int resctrl_val(const struct resctrl_test *test,
 
 		if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
 		    !strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
-			ret = measure_vals(uparams, param, &bw_resc_start);
+			ret = measure_vals(uparams, param);
 			if (ret)
 				break;
 		} else if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) {
-- 
2.39.2

[PATCH v2 03/13] selftests/resctrl: Consolidate get_domain_id() into resctrl_val()

[Ilpo Järvinen]

Both initialize_mem_bw_resctrl() and initialize_llc_occu_resctrl() that
are called from resctrl_val() need to determine domain ID to construct
resctrl fs related paths. Both functions do it by taking CPU ID which
neither needs for any other purpose than determining the domain ID.

Consolidate determining the domain ID into resctrl_val() and pass the
domain ID instead of CPU ID to initialize_mem_bw_resctrl() and
initialize_llc_occu_resctrl().

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

diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 4df2cd738f88..7981589f4db0 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -431,19 +431,12 @@ void set_mbm_path(const char *ctrlgrp, const char *mongrp, int domain_id)
  * initialize_mem_bw_resctrl:	Appropriately populate "mbm_total_path"
  * @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
+ * @domain_id:			Domain ID (cache ID; for MB, L3 cache ID)
  * @resctrl_val:		Resctrl feature (Eg: mbm, mba.. etc)
  */
 static void initialize_mem_bw_resctrl(const char *ctrlgrp, const char *mongrp,
-				      int cpu_no, char *resctrl_val)
+				      int domain_id, char *resctrl_val)
 {
-	int domain_id;
-
-	if (get_domain_id("MB", cpu_no, &domain_id) < 0) {
-		ksft_print_msg("Could not get domain ID\n");
-		return;
-	}
-
 	if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)))
 		set_mbm_path(ctrlgrp, mongrp, domain_id);
 
@@ -600,19 +593,12 @@ static void set_cmt_path(const char *ctrlgrp, const char *mongrp, char sock_num)
  * initialize_llc_occu_resctrl:	Appropriately populate "llc_occup_path"
  * @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
+ * @domain_id:			Domain ID (cache ID; for MB, L3 cache ID)
  * @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)
+					int domain_id, char *resctrl_val)
 {
-	int domain_id;
-
-	if (get_domain_id("L3", cpu_no, &domain_id) < 0) {
-		ksft_print_msg("Could not get domain ID\n");
-		return;
-	}
-
 	if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
 		set_cmt_path(ctrlgrp, mongrp, domain_id);
 }
@@ -729,10 +715,17 @@ int resctrl_val(const struct resctrl_test *test,
 	int ret = 0, pipefd[2];
 	char pipe_message = 0;
 	union sigval value;
+	int domain_id;
 
 	if (strcmp(param->filename, "") == 0)
 		sprintf(param->filename, "stdio");
 
+	ret = get_domain_id(test->resource, uparams->cpu, &domain_id);
+	if (ret < 0) {
+		ksft_print_msg("Could not get domain ID\n");
+		return ret;
+	}
+
 	if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) ||
 	    !strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR))) {
 		ret = validate_bw_report_request(param->bw_report);
@@ -827,10 +820,10 @@ int resctrl_val(const struct resctrl_test *test,
 			goto out;
 
 		initialize_mem_bw_resctrl(param->ctrlgrp, param->mongrp,
-					  uparams->cpu, resctrl_val);
+					  domain_id, resctrl_val);
 	} else if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
 		initialize_llc_occu_resctrl(param->ctrlgrp, param->mongrp,
-					    uparams->cpu, resctrl_val);
+					    domain_id, resctrl_val);
 
 	/* Parent waits for child to be ready. */
 	close(pipefd[1]);
-- 
2.39.2

[PATCH v2 04/13] selftests/resctrl: Use correct type for pids

[Ilpo Järvinen]

A few functions receive PIDs through int arguments. PIDs variables
should be of type pid_t, not int.

Convert pid arguments from int to pid_t.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/cache.c       | 6 +++---
 tools/testing/selftests/resctrl/resctrl.h     | 4 ++--
 tools/testing/selftests/resctrl/resctrl_val.c | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
index 1b339d6bbff1..9b74fce80037 100644
--- a/tools/testing/selftests/resctrl/cache.c
+++ b/tools/testing/selftests/resctrl/cache.c
@@ -101,7 +101,7 @@ static int get_llc_occu_resctrl(unsigned long *llc_occupancy)
  *
  * Return:		0 on success, < 0 on error.
  */
-static int print_results_cache(const char *filename, int bm_pid, __u64 llc_value)
+static int print_results_cache(const char *filename, pid_t bm_pid, __u64 llc_value)
 {
 	FILE *fp;
 
@@ -133,7 +133,7 @@ static int print_results_cache(const char *filename, int bm_pid, __u64 llc_value
  * Return: =0 on success. <0 on failure.
  */
 int perf_event_measure(int pe_fd, struct perf_event_read *pe_read,
-		       const char *filename, int bm_pid)
+		       const char *filename, pid_t bm_pid)
 {
 	int ret;
 
@@ -161,7 +161,7 @@ int perf_event_measure(int pe_fd, struct perf_event_read *pe_read,
  *
  * Return: =0 on success. <0 on failure.
  */
-int measure_llc_resctrl(const char *filename, int bm_pid)
+int measure_llc_resctrl(const char *filename, pid_t bm_pid)
 {
 	unsigned long llc_occu_resc = 0;
 	int ret;
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index 2051bd135e0d..f810a3c5692c 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -177,8 +177,8 @@ void perf_event_initialize_read_format(struct perf_event_read *pe_read);
 int perf_open(struct perf_event_attr *pea, pid_t pid, int cpu_no);
 int perf_event_reset_enable(int pe_fd);
 int perf_event_measure(int pe_fd, struct perf_event_read *pe_read,
-		       const char *filename, int bm_pid);
-int measure_llc_resctrl(const char *filename, int bm_pid);
+		       const char *filename, pid_t bm_pid);
+int measure_llc_resctrl(const char *filename, pid_t bm_pid);
 void show_cache_info(int no_of_bits, __u64 avg_llc_val, size_t cache_span, bool lines);
 
 /*
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 7981589f4db0..07fd57d8d125 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -545,7 +545,7 @@ void signal_handler_unregister(void)
  *
  * Return:		0 on success, < 0 on error.
  */
-static int print_results_bw(char *filename,  int bm_pid, float bw_imc,
+static int print_results_bw(char *filename, pid_t bm_pid, float bw_imc,
 			    unsigned long bw_resc)
 {
 	unsigned long diff = fabs(bw_imc - bw_resc);
-- 
2.39.2

[PATCH v2 05/13] selftests/resctrl: Cleanup bm_pid and ppid usage & limit scope

[Ilpo Järvinen]

'bm_pid' and 'ppid' are global variables. As they are used by different
processes and in signal handler, they cannot be entirely converted into
local variables.

The scope of those variables can still be reduced into resctrl_val.c
only. As PARENT_EXIT() macro is using 'ppid', make it a function in
resctrl_val.c and pass ppid to it as an argument because it is easier
to understand than using the global variable directly.

Pass 'bm_pid' into measure_val() instead of relying on the global
variable which helps to make the call signatures of measure_val() and
measure_llc_resctrl() more similar to each other.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/resctrl.h     |  9 --------
 tools/testing/selftests/resctrl/resctrl_val.c | 23 ++++++++++++-------
 2 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index f810a3c5692c..90fc00a61d72 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -43,13 +43,6 @@
 
 #define DEFAULT_SPAN		(250 * MB)
 
-#define PARENT_EXIT()				\
-	do {					\
-		kill(ppid, SIGKILL);		\
-		umount_resctrlfs();		\
-		exit(EXIT_FAILURE);		\
-	} while (0)
-
 /*
  * user_params:		User supplied parameters
  * @cpu:		CPU number to which the benchmark will be bound to
@@ -125,8 +118,6 @@ struct perf_event_read {
  */
 extern volatile int *value_sink;
 
-extern pid_t bm_pid, ppid;
-
 extern char llc_occup_path[1024];
 
 int get_vendor(void);
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 07fd57d8d125..04a8577b5e0a 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -481,7 +481,7 @@ static int get_mem_bw_resctrl(unsigned long *mbm_total)
 	return 0;
 }
 
-pid_t bm_pid, ppid;
+static pid_t bm_pid, ppid;
 
 void ctrlc_handler(int signum, siginfo_t *info, void *ptr)
 {
@@ -536,6 +536,13 @@ void signal_handler_unregister(void)
 	}
 }
 
+static void parent_exit(pid_t ppid)
+{
+	kill(ppid, SIGKILL);
+	umount_resctrlfs();
+	exit(EXIT_FAILURE);
+}
+
 /*
  * print_results_bw:	the memory bandwidth results are stored in a file
  * @filename:		file that stores the results
@@ -604,7 +611,7 @@ static void initialize_llc_occu_resctrl(const char *ctrlgrp, const char *mongrp,
 }
 
 static int measure_vals(const struct user_params *uparams,
-			struct resctrl_val_param *param)
+			struct resctrl_val_param *param, pid_t bm_pid)
 {
 	unsigned long bw_resc, bw_resc_start, bw_resc_end;
 	float bw_imc;
@@ -664,7 +671,7 @@ static void run_benchmark(int signum, siginfo_t *info, void *ucontext)
 	fp = freopen("/dev/null", "w", stdout);
 	if (!fp) {
 		ksft_perror("Unable to direct benchmark status to /dev/null");
-		PARENT_EXIT();
+		parent_exit(ppid);
 	}
 
 	if (strcmp(benchmark_cmd[0], "fill_buf") == 0) {
@@ -678,7 +685,7 @@ static void run_benchmark(int signum, siginfo_t *info, void *ucontext)
 			once = false;
 		} else {
 			ksft_print_msg("Invalid once parameter\n");
-			PARENT_EXIT();
+			parent_exit(ppid);
 		}
 
 		if (run_fill_buf(span, memflush, operation, once))
@@ -692,7 +699,7 @@ static void run_benchmark(int signum, siginfo_t *info, void *ucontext)
 
 	fclose(stdout);
 	ksft_print_msg("Unable to run specified benchmark\n");
-	PARENT_EXIT();
+	parent_exit(ppid);
 }
 
 /*
@@ -771,7 +778,7 @@ int resctrl_val(const struct resctrl_test *test,
 		/* Register for "SIGUSR1" signal from parent */
 		if (sigaction(SIGUSR1, &sigact, NULL)) {
 			ksft_perror("Can't register child for signal");
-			PARENT_EXIT();
+			parent_exit(ppid);
 		}
 
 		/* Tell parent that child is ready */
@@ -789,7 +796,7 @@ int resctrl_val(const struct resctrl_test *test,
 		sigsuspend(&sigact.sa_mask);
 
 		ksft_perror("Child is done");
-		PARENT_EXIT();
+		parent_exit(ppid);
 	}
 
 	ksft_print_msg("Benchmark PID: %d\n", bm_pid);
@@ -859,7 +866,7 @@ int resctrl_val(const struct resctrl_test *test,
 
 		if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
 		    !strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
-			ret = measure_vals(uparams, param);
+			ret = measure_vals(uparams, param, bm_pid);
 			if (ret)
 				break;
 		} else if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) {
-- 
2.39.2

[PATCH v2 06/13] selftests/resctrl: Rename measure_vals() to measure_mem_bw_vals() & document

[Ilpo Järvinen]

measure_val() is awfully generic name so rename it to measure_mem_bw()
to describe better what it does and document the function parameters.

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

diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 04a8577b5e0a..80e5174df828 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -610,8 +610,14 @@ static void initialize_llc_occu_resctrl(const char *ctrlgrp, const char *mongrp,
 		set_cmt_path(ctrlgrp, mongrp, domain_id);
 }
 
-static int measure_vals(const struct user_params *uparams,
-			struct resctrl_val_param *param, pid_t bm_pid)
+/*
+ * measure_mem_bw - Measures memory bandwidth numbers while benchmark runs
+ * @uparams:		User supplied parameters
+ * @param:		parameters passed to resctrl_val()
+ * @bm_pid:		PID that runs the benchmark
+ */
+static int measure_mem_bw(const struct user_params *uparams,
+			  struct resctrl_val_param *param, pid_t bm_pid)
 {
 	unsigned long bw_resc, bw_resc_start, bw_resc_end;
 	float bw_imc;
@@ -866,7 +872,7 @@ int resctrl_val(const struct resctrl_test *test,
 
 		if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
 		    !strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
-			ret = measure_vals(uparams, param, bm_pid);
+			ret = measure_mem_bw(uparams, param, bm_pid);
 			if (ret)
 				break;
 		} else if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) {
-- 
2.39.2

[PATCH v2 07/13] selftests/resctrl: Add ->measure() callback to resctrl_val_param

[Ilpo Järvinen]

The measurement done in resctrl_val() varies depending on test type.
The decision for how to measure is decided based on the string compare
to test name which is quite inflexible.

Add ->measure() callback into the struct resctrl_val_param to allow
each test to provide necessary code as a function which simplifies what
resctrl_val() has to do.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---

v2:
- spaces -> tabs
---
 tools/testing/selftests/resctrl/cmt_test.c    |  8 ++++++++
 tools/testing/selftests/resctrl/mba_test.c    |  9 ++++++++-
 tools/testing/selftests/resctrl/mbm_test.c    |  9 ++++++++-
 tools/testing/selftests/resctrl/resctrl.h     |  6 ++++++
 tools/testing/selftests/resctrl/resctrl_val.c | 18 +++++-------------
 5 files changed, 35 insertions(+), 15 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index a81f91222a89..241c0b129b58 100644
--- a/tools/testing/selftests/resctrl/cmt_test.c
+++ b/tools/testing/selftests/resctrl/cmt_test.c
@@ -29,6 +29,13 @@ static int cmt_setup(const struct resctrl_test *test,
 	return 0;
 }
 
+static int cmt_measure(const struct user_params *uparams,
+		       struct resctrl_val_param *param, pid_t bm_pid)
+{
+	sleep(1);
+	return measure_llc_resctrl(param->filename, bm_pid);
+}
+
 static int show_results_info(unsigned long sum_llc_val, int no_of_bits,
 			     unsigned long cache_span, unsigned long max_diff,
 			     unsigned long max_diff_percent, unsigned long num_of_runs,
@@ -133,6 +140,7 @@ static int cmt_run_test(const struct resctrl_test *test, const struct user_param
 		.mask		= ~(long_mask << n) & long_mask,
 		.num_of_runs	= 0,
 		.setup		= cmt_setup,
+		.measure	= cmt_measure,
 	};
 
 	span = cache_portion_size(cache_total_size, param.mask, long_mask);
diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
index 7946e32e85c8..0939f86514f7 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -51,6 +51,12 @@ static int mba_setup(const struct resctrl_test *test,
 	return 0;
 }
 
+static int mba_measure(const struct user_params *uparams,
+		       struct resctrl_val_param *param, pid_t bm_pid)
+{
+	return measure_mem_bw(uparams, param, bm_pid);
+}
+
 static bool show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc)
 {
 	int allocation, runs;
@@ -150,7 +156,8 @@ static int mba_run_test(const struct resctrl_test *test, const struct user_param
 		.mongrp		= "m1",
 		.filename	= RESULT_FILE_NAME,
 		.bw_report	= "reads",
-		.setup		= mba_setup
+		.setup		= mba_setup,
+		.measure	= mba_measure,
 	};
 	int ret;
 
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index d67ffa3ec63a..17398cd3aace 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -105,6 +105,12 @@ static int mbm_setup(const struct resctrl_test *test,
 	return ret;
 }
 
+static int mbm_measure(const struct user_params *uparams,
+		       struct resctrl_val_param *param, pid_t bm_pid)
+{
+	return measure_mem_bw(uparams, param, bm_pid);
+}
+
 void mbm_test_cleanup(void)
 {
 	remove(RESULT_FILE_NAME);
@@ -118,7 +124,8 @@ static int mbm_run_test(const struct resctrl_test *test, const struct user_param
 		.mongrp		= "m1",
 		.filename	= RESULT_FILE_NAME,
 		.bw_report	= "reads",
-		.setup		= mbm_setup
+		.setup		= mbm_setup,
+		.measure	= mbm_measure,
 	};
 	int ret;
 
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index 90fc00a61d72..2da642e11b61 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -85,6 +85,7 @@ struct resctrl_test {
  * @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
+ * @measure:		Callback that performs the measurement (a single test)
  */
 struct resctrl_val_param {
 	char		*resctrl_val;
@@ -97,6 +98,9 @@ struct resctrl_val_param {
 	int		(*setup)(const struct resctrl_test *test,
 				 const struct user_params *uparams,
 				 struct resctrl_val_param *param);
+	int		(*measure)(const struct user_params *uparams,
+				   struct resctrl_val_param *param,
+				   pid_t bm_pid);
 };
 
 struct perf_event_read {
@@ -143,6 +147,8 @@ unsigned char *alloc_buffer(size_t buf_size, int memflush);
 void mem_flush(unsigned char *buf, size_t buf_size);
 void fill_cache_read(unsigned char *buf, size_t buf_size, bool once);
 int run_fill_buf(size_t buf_size, int memflush, int op, bool once);
+int measure_mem_bw(const struct user_params *uparams,
+		   struct resctrl_val_param *param, pid_t bm_pid);
 int resctrl_val(const struct resctrl_test *test,
 		const struct user_params *uparams,
 		const char * const *benchmark_cmd,
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 80e5174df828..13d89d24474e 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -616,8 +616,8 @@ static void initialize_llc_occu_resctrl(const char *ctrlgrp, const char *mongrp,
  * @param:		parameters passed to resctrl_val()
  * @bm_pid:		PID that runs the benchmark
  */
-static int measure_mem_bw(const struct user_params *uparams,
-			  struct resctrl_val_param *param, pid_t bm_pid)
+int measure_mem_bw(const struct user_params *uparams,
+		   struct resctrl_val_param *param, pid_t bm_pid)
 {
 	unsigned long bw_resc, bw_resc_start, bw_resc_end;
 	float bw_imc;
@@ -870,17 +870,9 @@ int resctrl_val(const struct resctrl_test *test,
 		if (ret < 0)
 			break;
 
-		if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
-		    !strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
-			ret = measure_mem_bw(uparams, param, bm_pid);
-			if (ret)
-				break;
-		} else if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) {
-			sleep(1);
-			ret = measure_llc_resctrl(param->filename, bm_pid);
-			if (ret)
-				break;
-		}
+		ret = param->measure(uparams, param, bm_pid);
+		if (ret)
+			break;
 	}
 
 out:
-- 
2.39.2

[PATCH v2 08/13] selftests/resctrl: Add ->init() callback into resctrl_val_param

[Ilpo Järvinen]

The struct resctrl_val_param is there to customize behavior inside
resctrl_val() which is currently not used to full extent and there are
number of strcmp()s for test name in resctrl_val done by resctrl_val().

Create ->init() hook into the struct resctrl_val_param to cleanly
do per test initialization.

Remove also unused branches to setup paths and the related #defines.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/cmt_test.c    |  12 ++
 tools/testing/selftests/resctrl/mba_test.c    |  24 +++-
 tools/testing/selftests/resctrl/mbm_test.c    |  24 +++-
 tools/testing/selftests/resctrl/resctrl.h     |   9 +-
 tools/testing/selftests/resctrl/resctrl_val.c | 123 ++----------------
 5 files changed, 75 insertions(+), 117 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index 241c0b129b58..e79eca9346f3 100644
--- a/tools/testing/selftests/resctrl/cmt_test.c
+++ b/tools/testing/selftests/resctrl/cmt_test.c
@@ -16,6 +16,17 @@
 #define MAX_DIFF		2000000
 #define MAX_DIFF_PERCENT	15
 
+#define CON_MON_LCC_OCCUP_PATH		\
+	"%s/%s/mon_groups/%s/mon_data/mon_L3_%02d/llc_occupancy"
+
+static int set_cmt_path(const struct resctrl_val_param *param, int domain_id)
+{
+	sprintf(llc_occup_path,	CON_MON_LCC_OCCUP_PATH,	RESCTRL_PATH,
+		param->ctrlgrp, param->mongrp, domain_id);
+
+	return 0;
+}
+
 static int cmt_setup(const struct resctrl_test *test,
 		     const struct user_params *uparams,
 		     struct resctrl_val_param *p)
@@ -139,6 +150,7 @@ static int cmt_run_test(const struct resctrl_test *test, const struct user_param
 		.filename	= RESULT_FILE_NAME,
 		.mask		= ~(long_mask << n) & long_mask,
 		.num_of_runs	= 0,
+		.init		= set_cmt_path,
 		.setup		= cmt_setup,
 		.measure	= cmt_measure,
 	};
diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
index 0939f86514f7..22c1f5e43352 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -8,6 +8,8 @@
  *    Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>,
  *    Fenghua Yu <fenghua.yu@intel.com>
  */
+#include <limits.h>
+
 #include "resctrl.h"
 
 #define RESULT_FILE_NAME	"result_mba"
@@ -17,6 +19,25 @@
 #define ALLOCATION_MIN		10
 #define ALLOCATION_STEP		10
 
+#define CON_MBM_LOCAL_BYTES_PATH \
+	"%s/%s/mon_data/mon_L3_%02d/mbm_local_bytes"
+
+static char mbm_total_path[PATH_MAX];
+
+static int set_mba_path(const struct resctrl_val_param *param, int domain_id)
+{
+	int ret;
+
+	ret = initialize_mem_bw_imc();
+	if (ret)
+		return ret;
+
+	sprintf(mbm_total_path, CON_MBM_LOCAL_BYTES_PATH,
+		RESCTRL_PATH, param->ctrlgrp, domain_id);
+
+	return 0;
+}
+
 /*
  * Change schemata percentage from 100 to 10%. Write schemata to specified
  * con_mon grp, mon_grp in resctrl FS.
@@ -54,7 +75,7 @@ static int mba_setup(const struct resctrl_test *test,
 static int mba_measure(const struct user_params *uparams,
 		       struct resctrl_val_param *param, pid_t bm_pid)
 {
-	return measure_mem_bw(uparams, param, bm_pid);
+	return measure_mem_bw(uparams, param, bm_pid, mbm_total_path);
 }
 
 static bool show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc)
@@ -156,6 +177,7 @@ static int mba_run_test(const struct resctrl_test *test, const struct user_param
 		.mongrp		= "m1",
 		.filename	= RESULT_FILE_NAME,
 		.bw_report	= "reads",
+		.init		= set_mba_path,
 		.setup		= mba_setup,
 		.measure	= mba_measure,
 	};
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index 17398cd3aace..ffbfcecf9bd6 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -8,12 +8,19 @@
  *    Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>,
  *    Fenghua Yu <fenghua.yu@intel.com>
  */
+#include <limits.h>
+
 #include "resctrl.h"
 
 #define RESULT_FILE_NAME	"result_mbm"
 #define MAX_DIFF_PERCENT	8
 #define NUM_OF_RUNS		5
 
+#define CON_MON_MBM_LOCAL_BYTES_PATH \
+	"%s/%s/mon_groups/%s/mon_data/mon_L3_%02d/mbm_local_bytes"
+
+static char mbm_total_path[PATH_MAX];
+
 static int
 show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc, size_t span)
 {
@@ -86,6 +93,20 @@ static int check_results(size_t span)
 	return ret;
 }
 
+static int set_mbm_path(const struct resctrl_val_param *param, int domain_id)
+{
+	int ret;
+
+	ret = initialize_mem_bw_imc();
+	if (ret)
+		return ret;
+
+	sprintf(mbm_total_path, CON_MON_MBM_LOCAL_BYTES_PATH,
+		RESCTRL_PATH, param->ctrlgrp, param->mongrp, domain_id);
+
+	return 0;
+}
+
 static int mbm_setup(const struct resctrl_test *test,
 		     const struct user_params *uparams,
 		     struct resctrl_val_param *p)
@@ -108,7 +129,7 @@ static int mbm_setup(const struct resctrl_test *test,
 static int mbm_measure(const struct user_params *uparams,
 		       struct resctrl_val_param *param, pid_t bm_pid)
 {
-	return measure_mem_bw(uparams, param, bm_pid);
+	return measure_mem_bw(uparams, param, bm_pid, mbm_total_path);
 }
 
 void mbm_test_cleanup(void)
@@ -124,6 +145,7 @@ static int mbm_run_test(const struct resctrl_test *test, const struct user_param
 		.mongrp		= "m1",
 		.filename	= RESULT_FILE_NAME,
 		.bw_report	= "reads",
+		.init		= set_mbm_path,
 		.setup		= mbm_setup,
 		.measure	= mbm_measure,
 	};
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index 2da642e11b61..d9c6443a8524 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -84,7 +84,8 @@ struct resctrl_test {
  * @mongrp:		Name of the monitor group (mon grp)
  * @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
+ * @init:		Callback function to initialize test environment
+ * @setup:		Callback function to setup per test run environment
  * @measure:		Callback that performs the measurement (a single test)
  */
 struct resctrl_val_param {
@@ -95,6 +96,8 @@ struct resctrl_val_param {
 	char		*bw_report;
 	unsigned long	mask;
 	int		num_of_runs;
+	int		(*init)(const struct resctrl_val_param *param,
+				int domain_id);
 	int		(*setup)(const struct resctrl_test *test,
 				 const struct user_params *uparams,
 				 struct resctrl_val_param *param);
@@ -147,8 +150,10 @@ unsigned char *alloc_buffer(size_t buf_size, int memflush);
 void mem_flush(unsigned char *buf, size_t buf_size);
 void fill_cache_read(unsigned char *buf, size_t buf_size, bool once);
 int run_fill_buf(size_t buf_size, int memflush, int op, bool once);
+int initialize_mem_bw_imc(void);
 int measure_mem_bw(const struct user_params *uparams,
-		   struct resctrl_val_param *param, pid_t bm_pid);
+		   struct resctrl_val_param *param,
+		   pid_t bm_pid, const char *mbm_total_path);
 int resctrl_val(const struct resctrl_test *test,
 		const struct user_params *uparams,
 		const char * const *benchmark_cmd,
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 13d89d24474e..1a96298592ed 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -19,29 +19,6 @@
 #define MAX_TOKENS		5
 #define READ			0
 #define WRITE			1
-#define CON_MON_MBM_LOCAL_BYTES_PATH				\
-	"%s/%s/mon_groups/%s/mon_data/mon_L3_%02d/mbm_local_bytes"
-
-#define CON_MBM_LOCAL_BYTES_PATH		\
-	"%s/%s/mon_data/mon_L3_%02d/mbm_local_bytes"
-
-#define MON_MBM_LOCAL_BYTES_PATH		\
-	"%s/mon_groups/%s/mon_data/mon_L3_%02d/mbm_local_bytes"
-
-#define MBM_LOCAL_BYTES_PATH			\
-	"%s/mon_data/mon_L3_%02d/mbm_local_bytes"
-
-#define CON_MON_LCC_OCCUP_PATH		\
-	"%s/%s/mon_groups/%s/mon_data/mon_L3_%02d/llc_occupancy"
-
-#define CON_LCC_OCCUP_PATH		\
-	"%s/%s/mon_data/mon_L3_%02d/llc_occupancy"
-
-#define MON_LCC_OCCUP_PATH		\
-	"%s/mon_groups/%s/mon_data/mon_L3_%02d/llc_occupancy"
-
-#define LCC_OCCUP_PATH			\
-	"%s/mon_data/mon_L3_%02d/llc_occupancy"
 
 struct membw_read_format {
 	__u64 value;         /* The value of the event */
@@ -59,7 +36,6 @@ struct imc_counter_config {
 	int fd;
 };
 
-static char mbm_total_path[1024];
 static int imcs;
 static struct imc_counter_config imc_counters_config[MAX_IMCS][2];
 
@@ -275,7 +251,7 @@ static int num_of_imcs(void)
 	return count;
 }
 
-static int initialize_mem_bw_imc(void)
+int initialize_mem_bw_imc(void)
 {
 	int imc, j;
 
@@ -411,56 +387,10 @@ static int get_mem_bw_imc(char *bw_report, float *bw_imc)
 	return 0;
 }
 
-void set_mbm_path(const char *ctrlgrp, const char *mongrp, int domain_id)
-{
-	if (ctrlgrp && mongrp)
-		sprintf(mbm_total_path, CON_MON_MBM_LOCAL_BYTES_PATH,
-			RESCTRL_PATH, ctrlgrp, mongrp, domain_id);
-	else if (!ctrlgrp && mongrp)
-		sprintf(mbm_total_path, MON_MBM_LOCAL_BYTES_PATH, RESCTRL_PATH,
-			mongrp, domain_id);
-	else if (ctrlgrp && !mongrp)
-		sprintf(mbm_total_path, CON_MBM_LOCAL_BYTES_PATH, RESCTRL_PATH,
-			ctrlgrp, domain_id);
-	else if (!ctrlgrp && !mongrp)
-		sprintf(mbm_total_path, MBM_LOCAL_BYTES_PATH, RESCTRL_PATH,
-			domain_id);
-}
-
-/*
- * initialize_mem_bw_resctrl:	Appropriately populate "mbm_total_path"
- * @ctrlgrp:			Name of the control monitor group (con_mon grp)
- * @mongrp:			Name of the monitor group (mon grp)
- * @domain_id:			Domain ID (cache ID; for MB, L3 cache ID)
- * @resctrl_val:		Resctrl feature (Eg: mbm, mba.. etc)
- */
-static void initialize_mem_bw_resctrl(const char *ctrlgrp, const char *mongrp,
-				      int domain_id, char *resctrl_val)
-{
-	if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)))
-		set_mbm_path(ctrlgrp, mongrp, domain_id);
-
-	if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
-		if (ctrlgrp)
-			sprintf(mbm_total_path, CON_MBM_LOCAL_BYTES_PATH,
-				RESCTRL_PATH, ctrlgrp, domain_id);
-		else
-			sprintf(mbm_total_path, MBM_LOCAL_BYTES_PATH,
-				RESCTRL_PATH, domain_id);
-	}
-}
-
 /*
  * Get MBM Local bytes as reported by resctrl FS
- * For MBM,
- * 1. If con_mon grp and mon grp are given, then read from con_mon grp's mon grp
- * 2. If only con_mon grp is given, then read from con_mon grp
- * 3. If both are not given, then read from root con_mon grp
- * For MBA,
- * 1. If con_mon grp is given, then read from it
- * 2. If con_mon grp is not given, then read from root con_mon grp
  */
-static int get_mem_bw_resctrl(unsigned long *mbm_total)
+static int get_mem_bw_resctrl(const char *mbm_total_path, unsigned long *mbm_total)
 {
 	FILE *fp;
 
@@ -581,43 +511,16 @@ static int print_results_bw(char *filename, pid_t bm_pid, float bw_imc,
 	return 0;
 }
 
-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,
-			ctrlgrp, mongrp, sock_num);
-	else if (!strlen(ctrlgrp) && strlen(mongrp))
-		sprintf(llc_occup_path,	MON_LCC_OCCUP_PATH, RESCTRL_PATH,
-			mongrp, sock_num);
-	else if (strlen(ctrlgrp) && !strlen(mongrp))
-		sprintf(llc_occup_path,	CON_LCC_OCCUP_PATH, RESCTRL_PATH,
-			ctrlgrp, sock_num);
-	else if (!strlen(ctrlgrp) && !strlen(mongrp))
-		sprintf(llc_occup_path, LCC_OCCUP_PATH,	RESCTRL_PATH, sock_num);
-}
-
-/*
- * initialize_llc_occu_resctrl:	Appropriately populate "llc_occup_path"
- * @ctrlgrp:			Name of the control monitor group (con_mon grp)
- * @mongrp:			Name of the monitor group (mon grp)
- * @domain_id:			Domain ID (cache ID; for MB, L3 cache ID)
- * @resctrl_val:		Resctrl feature (Eg: cat, cmt.. etc)
- */
-static void initialize_llc_occu_resctrl(const char *ctrlgrp, const char *mongrp,
-					int domain_id, char *resctrl_val)
-{
-	if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
-		set_cmt_path(ctrlgrp, mongrp, domain_id);
-}
-
 /*
  * measure_mem_bw - Measures memory bandwidth numbers while benchmark runs
  * @uparams:		User supplied parameters
  * @param:		parameters passed to resctrl_val()
  * @bm_pid:		PID that runs the benchmark
+ * @mbm_total_path:	Resctrl FS monitoring file to read mem BW from
  */
 int measure_mem_bw(const struct user_params *uparams,
-		   struct resctrl_val_param *param, pid_t bm_pid)
+		   struct resctrl_val_param *param,
+		   pid_t bm_pid, const char *mbm_total_path)
 {
 	unsigned long bw_resc, bw_resc_start, bw_resc_end;
 	float bw_imc;
@@ -634,13 +537,13 @@ int measure_mem_bw(const struct user_params *uparams,
 	if (ret < 0)
 		return ret;
 
-	ret = get_mem_bw_resctrl(&bw_resc_start);
+	ret = get_mem_bw_resctrl(mbm_total_path, &bw_resc_start);
 	if (ret < 0)
 		return ret;
 
 	do_imc_mem_bw_test();
 
-	ret = get_mem_bw_resctrl(&bw_resc_end);
+	ret = get_mem_bw_resctrl(mbm_total_path, &bw_resc_end);
 	if (ret < 0)
 		return ret;
 
@@ -826,17 +729,11 @@ int resctrl_val(const struct resctrl_test *test,
 	if (ret)
 		goto out;
 
-	if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
-	    !strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
-		ret = initialize_mem_bw_imc();
+	if (param->init) {
+		ret = param->init(param, domain_id);
 		if (ret)
 			goto out;
-
-		initialize_mem_bw_resctrl(param->ctrlgrp, param->mongrp,
-					  domain_id, resctrl_val);
-	} else if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
-		initialize_llc_occu_resctrl(param->ctrlgrp, param->mongrp,
-					    domain_id, resctrl_val);
+	}
 
 	/* Parent waits for child to be ready. */
 	close(pipefd[1]);
-- 
2.39.2

[PATCH v2 09/13] selftests/resctrl: Simplify bandwidth report type handling

[Ilpo Järvinen]

bw_report is only needed for selecting the correct value from the
values IMC measured. It is a member in the resctrl_val_param struct and
is always set to "reads". The value is then checked in resctrl_val()
using validate_bw_report_request() that besides validating the input,
assumes it can mutate the string which is questionable programming
practice.

Simplify handling bw_report:

- Convert validate_bw_report_request() into get_bw_report_type() that
  inputs and returns const char *. Use NULL to indicate error.

- Validate the report types inside measure_mem_bw(), not in
  resctrl_val().

- As resctrl_val() no longer needs bw_report for anything, it can just
  be passed to measure_mem_bw() by the ->measure() hooks.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---

v2:
- Rebased on top of next to resolve conflict in resctrl.h
---
 tools/testing/selftests/resctrl/mba_test.c    |  3 +--
 tools/testing/selftests/resctrl/mbm_test.c    |  3 +--
 tools/testing/selftests/resctrl/resctrl.h     |  7 +++----
 tools/testing/selftests/resctrl/resctrl_val.c | 19 +++++++++----------
 tools/testing/selftests/resctrl/resctrlfs.c   | 13 ++++++-------
 5 files changed, 20 insertions(+), 25 deletions(-)

diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
index 22c1f5e43352..89ad7f2cdd65 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -75,7 +75,7 @@ static int mba_setup(const struct resctrl_test *test,
 static int mba_measure(const struct user_params *uparams,
 		       struct resctrl_val_param *param, pid_t bm_pid)
 {
-	return measure_mem_bw(uparams, param, bm_pid, mbm_total_path);
+	return measure_mem_bw(uparams, param, bm_pid, mbm_total_path, "reads");
 }
 
 static bool show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc)
@@ -176,7 +176,6 @@ static int mba_run_test(const struct resctrl_test *test, const struct user_param
 		.ctrlgrp	= "c1",
 		.mongrp		= "m1",
 		.filename	= RESULT_FILE_NAME,
-		.bw_report	= "reads",
 		.init		= set_mba_path,
 		.setup		= mba_setup,
 		.measure	= mba_measure,
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index ffbfcecf9bd6..c8c9aba81db8 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -129,7 +129,7 @@ static int mbm_setup(const struct resctrl_test *test,
 static int mbm_measure(const struct user_params *uparams,
 		       struct resctrl_val_param *param, pid_t bm_pid)
 {
-	return measure_mem_bw(uparams, param, bm_pid, mbm_total_path);
+	return measure_mem_bw(uparams, param, bm_pid, mbm_total_path, "reads");
 }
 
 void mbm_test_cleanup(void)
@@ -144,7 +144,6 @@ static int mbm_run_test(const struct resctrl_test *test, const struct user_param
 		.ctrlgrp	= "c1",
 		.mongrp		= "m1",
 		.filename	= RESULT_FILE_NAME,
-		.bw_report	= "reads",
 		.init		= set_mbm_path,
 		.setup		= mbm_setup,
 		.measure	= mbm_measure,
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index d9c6443a8524..0931c5c09c4f 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -83,7 +83,6 @@ struct resctrl_test {
  * @ctrlgrp:		Name of the control monitor group (con_mon grp)
  * @mongrp:		Name of the monitor group (mon grp)
  * @filename:		Name of file to which the o/p should be written
- * @bw_report:		Bandwidth report type (reads vs writes)
  * @init:		Callback function to initialize test environment
  * @setup:		Callback function to setup per test run environment
  * @measure:		Callback that performs the measurement (a single test)
@@ -93,7 +92,6 @@ struct resctrl_val_param {
 	char		ctrlgrp[64];
 	char		mongrp[64];
 	char		filename[64];
-	char		*bw_report;
 	unsigned long	mask;
 	int		num_of_runs;
 	int		(*init)(const struct resctrl_val_param *param,
@@ -133,7 +131,7 @@ int filter_dmesg(void);
 int get_domain_id(const char *resource, int cpu_no, int *domain_id);
 int mount_resctrlfs(void);
 int umount_resctrlfs(void);
-int validate_bw_report_request(char *bw_report);
+const char *get_bw_report_type(const char *bw_report);
 bool resctrl_resource_exists(const char *resource);
 bool resctrl_mon_feature_exists(const char *resource, const char *feature);
 bool resource_info_file_exists(const char *resource, const char *file);
@@ -153,7 +151,8 @@ int run_fill_buf(size_t buf_size, int memflush, int op, bool once);
 int initialize_mem_bw_imc(void);
 int measure_mem_bw(const struct user_params *uparams,
 		   struct resctrl_val_param *param,
-		   pid_t bm_pid, const char *mbm_total_path);
+		   pid_t bm_pid, const char *mbm_total_path,
+		   const char *bw_report);
 int resctrl_val(const struct resctrl_test *test,
 		const struct user_params *uparams,
 		const char * const *benchmark_cmd,
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 1a96298592ed..2f166a5c0c9b 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -320,7 +320,7 @@ static void do_imc_mem_bw_test(void)
  *
  * Return: = 0 on success. < 0 on failure.
  */
-static int get_mem_bw_imc(char *bw_report, float *bw_imc)
+static int get_mem_bw_imc(const char *bw_report, float *bw_imc)
 {
 	float reads, writes, of_mul_read, of_mul_write;
 	int imc, j;
@@ -517,15 +517,21 @@ static int print_results_bw(char *filename, pid_t bm_pid, float bw_imc,
  * @param:		parameters passed to resctrl_val()
  * @bm_pid:		PID that runs the benchmark
  * @mbm_total_path:	Resctrl FS monitoring file to read mem BW from
+ * @bw_report:		Bandwidth report type (reads, writes)
  */
 int measure_mem_bw(const struct user_params *uparams,
 		   struct resctrl_val_param *param,
-		   pid_t bm_pid, const char *mbm_total_path)
+		   pid_t bm_pid, const char *mbm_total_path,
+		   const char *bw_report)
 {
 	unsigned long bw_resc, bw_resc_start, bw_resc_end;
 	float bw_imc;
 	int ret;
 
+	bw_report = get_bw_report_type(bw_report);
+	if (!bw_report)
+		return -1;
+
 	/*
 	 * Measure memory bandwidth from resctrl and from
 	 * another source which is perf imc value or could
@@ -547,7 +553,7 @@ int measure_mem_bw(const struct user_params *uparams,
 	if (ret < 0)
 		return ret;
 
-	ret = get_mem_bw_imc(param->bw_report, &bw_imc);
+	ret = get_mem_bw_imc(bw_report, &bw_imc);
 	if (ret < 0)
 		return ret;
 
@@ -642,13 +648,6 @@ int resctrl_val(const struct resctrl_test *test,
 		return ret;
 	}
 
-	if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) ||
-	    !strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR))) {
-		ret = validate_bw_report_request(param->bw_report);
-		if (ret)
-			return ret;
-	}
-
 	/*
 	 * If benchmark wasn't successfully started by child, then child should
 	 * kill parent, so save parent's pid
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index 1cade75176eb..5f113b813253 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -837,22 +837,21 @@ int filter_dmesg(void)
 	return 0;
 }
 
-int validate_bw_report_request(char *bw_report)
+const char *get_bw_report_type(const char *bw_report)
 {
 	if (strcmp(bw_report, "reads") == 0)
-		return 0;
+		return bw_report;
 	if (strcmp(bw_report, "writes") == 0)
-		return 0;
+		return bw_report;
 	if (strcmp(bw_report, "nt-writes") == 0) {
-		strcpy(bw_report, "writes");
-		return 0;
+		return "writes";
 	}
 	if (strcmp(bw_report, "total") == 0)
-		return 0;
+		return bw_report;
 
 	fprintf(stderr, "Requested iMC B/W report type unavailable\n");
 
-	return -1;
+	return NULL;
 }
 
 int perf_event_open(struct perf_event_attr *hw_event, pid_t pid, int cpu,
-- 
2.39.2

[PATCH v2 10/13] selftests/resctrl: Make some strings passed to resctrlfs functions const

[Ilpo Järvinen]

Control group, monitor group and resctrl_val are not mutated and
should not be mutated within resctrlfs.c functions.

Mark this by using const char * for the arguments.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/resctrl.h   | 7 ++++---
 tools/testing/selftests/resctrl/resctrlfs.c | 7 ++++---
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index 0931c5c09c4f..52769b075233 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -139,9 +139,10 @@ bool test_resource_feature_check(const struct resctrl_test *test);
 char *fgrep(FILE *inf, const char *str);
 int taskset_benchmark(pid_t bm_pid, int cpu_no, cpu_set_t *old_affinity);
 int taskset_restore(pid_t bm_pid, cpu_set_t *old_affinity);
-int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, const char *resource);
-int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp,
-			    char *resctrl_val);
+int write_schemata(const char *ctrlgrp, char *schemata, int cpu_no,
+		   const char *resource);
+int write_bm_pid_to_resctrl(pid_t bm_pid, const char *ctrlgrp,
+			    const char *mongrp, const char *resctrl_val);
 int perf_event_open(struct perf_event_attr *hw_event, pid_t pid, int cpu,
 		    int group_fd, unsigned long flags);
 unsigned char *alloc_buffer(size_t buf_size, int memflush);
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index 5f113b813253..79cf1c593106 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -534,8 +534,8 @@ static int write_pid_to_tasks(char *tasks, pid_t pid)
  *
  * Return: 0 on success, < 0 on error.
  */
-int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp,
-			    char *resctrl_val)
+int write_bm_pid_to_resctrl(pid_t bm_pid, const char *ctrlgrp,
+			    const char *mongrp, const char *resctrl_val)
 {
 	char controlgroup[128], monitorgroup[512], monitorgroup_p[256];
 	char tasks[1024];
@@ -593,7 +593,8 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp,
  *
  * Return: 0 on success, < 0 on error.
  */
-int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, const char *resource)
+int write_schemata(const char *ctrlgrp, char *schemata, int cpu_no,
+		   const char *resource)
 {
 	char controlgroup[1024], reason[128], schema[1024] = {};
 	int domain_id, fd, schema_len, ret = 0;
-- 
2.39.2

[PATCH v2 11/13] selftests/resctrl: Convert ctrlgrp & mongrp to pointers

[Ilpo Järvinen]

The struct resctrl_val_param has control and monitor groups as char
arrays but they are not supposed to be mutated within resctrl_val().

Convert the ctrlgrp and mongrp char array within resctrl_val_param to
plain const char pointers and adjust the strlen() based checks to
check NULL instead.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/resctrl.h   | 4 ++--
 tools/testing/selftests/resctrl/resctrlfs.c | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index 52769b075233..54e5bce4c698 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -89,8 +89,8 @@ struct resctrl_test {
  */
 struct resctrl_val_param {
 	char		*resctrl_val;
-	char		ctrlgrp[64];
-	char		mongrp[64];
+	const char	*ctrlgrp;
+	const char	*mongrp;
 	char		filename[64];
 	unsigned long	mask;
 	int		num_of_runs;
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index 79cf1c593106..dbe0cc6d74fa 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -469,7 +469,7 @@ static int create_grp(const char *grp_name, char *grp, const char *parent_grp)
 	 * length of grp_name == 0, it means, user wants to use root con_mon
 	 * grp, so do nothing
 	 */
-	if (strlen(grp_name) == 0)
+	if (!grp_name)
 		return 0;
 
 	/* Check if requested grp exists or not */
@@ -541,7 +541,7 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, const char *ctrlgrp,
 	char tasks[1024];
 	int ret = 0;
 
-	if (strlen(ctrlgrp))
+	if (ctrlgrp)
 		sprintf(controlgroup, "%s/%s", RESCTRL_PATH, ctrlgrp);
 	else
 		sprintf(controlgroup, "%s", RESCTRL_PATH);
@@ -558,7 +558,7 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, const char *ctrlgrp,
 	/* Create mon grp and write pid into it for "mbm" and "cmt" test */
 	if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)) ||
 	    !strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR))) {
-		if (strlen(mongrp)) {
+		if (mongrp) {
 			sprintf(monitorgroup_p, "%s/mon_groups", controlgroup);
 			sprintf(monitorgroup, "%s/%s", monitorgroup_p, mongrp);
 			ret = create_grp(mongrp, monitorgroup, monitorgroup_p);
@@ -612,7 +612,7 @@ int write_schemata(const char *ctrlgrp, char *schemata, int cpu_no,
 		goto out;
 	}
 
-	if (strlen(ctrlgrp) != 0)
+	if (ctrlgrp)
 		sprintf(controlgroup, "%s/%s/schemata", RESCTRL_PATH, ctrlgrp);
 	else
 		sprintf(controlgroup, "%s/schemata", RESCTRL_PATH);
-- 
2.39.2

[PATCH v2 12/13] selftests/resctrl: Remove mongrp from MBA test

[Ilpo Järvinen]

Nothing during MBA test uses mongrp even if it has been defined ever
since the introduction of the MBA test in the commit 01fee6b4d1f9
("selftests/resctrl: Add MBA test").

Remove the mongrp from MBA test.

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

diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
index 89ad7f2cdd65..5bb73e6cabc3 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -174,7 +174,6 @@ static int mba_run_test(const struct resctrl_test *test, const struct user_param
 	struct resctrl_val_param param = {
 		.resctrl_val	= MBA_STR,
 		.ctrlgrp	= "c1",
-		.mongrp		= "m1",
 		.filename	= RESULT_FILE_NAME,
 		.init		= set_mba_path,
 		.setup		= mba_setup,
-- 
2.39.2

[PATCH v2 13/13] selftests/resctrl: Remove test name comparing from write_bm_pid_to_resctrl()

[Ilpo Järvinen]

write_bm_pid_to_resctrl() uses resctrl_val to check test name which is
not a good interface generic resctrl FS functions should provide.

Only MBM and CMT tests define mongrp so the test name check in
write_bm_pid_to_resctrl() can be changed to depend simply on mongrp
being non-NULL.

With last user of resctrl_val gone, the parameter and member from the
struct resctrl_val_param can removed. Test name constants can also be
removed because they are not used anymore.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/cat_test.c    |  5 +--
 tools/testing/selftests/resctrl/cmt_test.c    |  1 -
 tools/testing/selftests/resctrl/mba_test.c    |  1 -
 tools/testing/selftests/resctrl/mbm_test.c    |  1 -
 tools/testing/selftests/resctrl/resctrl.h     | 10 +-----
 tools/testing/selftests/resctrl/resctrl_val.c |  4 +--
 tools/testing/selftests/resctrl/resctrlfs.c   | 33 ++++++++-----------
 7 files changed, 17 insertions(+), 38 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
index 4cb991be8e31..c0291ecf1d1c 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -158,7 +158,6 @@ static int cat_test(const struct resctrl_test *test,
 		    struct resctrl_val_param *param,
 		    size_t span, unsigned long current_mask)
 {
-	char *resctrl_val = param->resctrl_val;
 	struct perf_event_read pe_read;
 	struct perf_event_attr pea;
 	cpu_set_t old_affinity;
@@ -178,8 +177,7 @@ static int cat_test(const struct resctrl_test *test,
 		return ret;
 
 	/* Write benchmark to specified con_mon grp, mon_grp in resctrl FS*/
-	ret = write_bm_pid_to_resctrl(bm_pid, param->ctrlgrp, param->mongrp,
-				      resctrl_val);
+	ret = write_bm_pid_to_resctrl(bm_pid, param->ctrlgrp, param->mongrp);
 	if (ret)
 		goto reset_affinity;
 
@@ -272,7 +270,6 @@ static int cat_run_test(const struct resctrl_test *test, const struct user_param
 	start_mask = create_bit_mask(start, n);
 
 	struct resctrl_val_param param = {
-		.resctrl_val	= CAT_STR,
 		.ctrlgrp	= "c1",
 		.filename	= RESULT_FILE_NAME,
 		.num_of_runs	= 0,
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index e79eca9346f3..ab96411d1015 100644
--- a/tools/testing/selftests/resctrl/cmt_test.c
+++ b/tools/testing/selftests/resctrl/cmt_test.c
@@ -144,7 +144,6 @@ static int cmt_run_test(const struct resctrl_test *test, const struct user_param
 	}
 
 	struct resctrl_val_param param = {
-		.resctrl_val	= CMT_STR,
 		.ctrlgrp	= "c1",
 		.mongrp		= "m1",
 		.filename	= RESULT_FILE_NAME,
diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
index 5bb73e6cabc3..c44af1f62f55 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -172,7 +172,6 @@ void mba_test_cleanup(void)
 static int mba_run_test(const struct resctrl_test *test, const struct user_params *uparams)
 {
 	struct resctrl_val_param param = {
-		.resctrl_val	= MBA_STR,
 		.ctrlgrp	= "c1",
 		.filename	= RESULT_FILE_NAME,
 		.init		= set_mba_path,
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index c8c9aba81db8..120c356a069e 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -140,7 +140,6 @@ void mbm_test_cleanup(void)
 static int mbm_run_test(const struct resctrl_test *test, const struct user_params *uparams)
 {
 	struct resctrl_val_param param = {
-		.resctrl_val	= MBM_STR,
 		.ctrlgrp	= "c1",
 		.mongrp		= "m1",
 		.filename	= RESULT_FILE_NAME,
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index 54e5bce4c698..4cd7997b39e6 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -79,7 +79,6 @@ struct resctrl_test {
 
 /*
  * resctrl_val_param:	resctrl test parameters
- * @resctrl_val:	Resctrl feature (Eg: mbm, mba.. etc)
  * @ctrlgrp:		Name of the control monitor group (con_mon grp)
  * @mongrp:		Name of the monitor group (mon grp)
  * @filename:		Name of file to which the o/p should be written
@@ -88,7 +87,6 @@ struct resctrl_test {
  * @measure:		Callback that performs the measurement (a single test)
  */
 struct resctrl_val_param {
-	char		*resctrl_val;
 	const char	*ctrlgrp;
 	const char	*mongrp;
 	char		filename[64];
@@ -111,11 +109,6 @@ struct perf_event_read {
 	} values[2];
 };
 
-#define MBM_STR			"mbm"
-#define MBA_STR			"mba"
-#define CMT_STR			"cmt"
-#define CAT_STR			"cat"
-
 /*
  * Memory location that consumes values compiler must not optimize away.
  * Volatile ensures writes to this location cannot be optimized away by
@@ -141,8 +134,7 @@ int taskset_benchmark(pid_t bm_pid, int cpu_no, cpu_set_t *old_affinity);
 int taskset_restore(pid_t bm_pid, cpu_set_t *old_affinity);
 int write_schemata(const char *ctrlgrp, char *schemata, int cpu_no,
 		   const char *resource);
-int write_bm_pid_to_resctrl(pid_t bm_pid, const char *ctrlgrp,
-			    const char *mongrp, const char *resctrl_val);
+int write_bm_pid_to_resctrl(pid_t bm_pid, const char *ctrlgrp, const char *mongrp);
 int perf_event_open(struct perf_event_attr *hw_event, pid_t pid, int cpu,
 		    int group_fd, unsigned long flags);
 unsigned char *alloc_buffer(size_t buf_size, int memflush);
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 2f166a5c0c9b..f2101ee665ba 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -632,7 +632,6 @@ int resctrl_val(const struct resctrl_test *test,
 		const char * const *benchmark_cmd,
 		struct resctrl_val_param *param)
 {
-	char *resctrl_val = param->resctrl_val;
 	struct sigaction sigact;
 	int ret = 0, pipefd[2];
 	char pipe_message = 0;
@@ -723,8 +722,7 @@ int resctrl_val(const struct resctrl_test *test,
 		goto out;
 
 	/* Write benchmark to specified control&monitoring grp in resctrl FS */
-	ret = write_bm_pid_to_resctrl(bm_pid, param->ctrlgrp, param->mongrp,
-				      resctrl_val);
+	ret = write_bm_pid_to_resctrl(bm_pid, param->ctrlgrp, param->mongrp);
 	if (ret)
 		goto out;
 
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index dbe0cc6d74fa..1bd70ac73ae2 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -524,7 +524,6 @@ static int write_pid_to_tasks(char *tasks, pid_t pid)
  * @bm_pid:		PID that should be written
  * @ctrlgrp:		Name of the control monitor group (con_mon grp)
  * @mongrp:		Name of the monitor group (mon grp)
- * @resctrl_val:	Resctrl feature (Eg: mbm, mba.. etc)
  *
  * If a con_mon grp is requested, create it and write pid to it, otherwise
  * write pid to root con_mon grp.
@@ -534,8 +533,7 @@ static int write_pid_to_tasks(char *tasks, pid_t pid)
  *
  * Return: 0 on success, < 0 on error.
  */
-int write_bm_pid_to_resctrl(pid_t bm_pid, const char *ctrlgrp,
-			    const char *mongrp, const char *resctrl_val)
+int write_bm_pid_to_resctrl(pid_t bm_pid, const char *ctrlgrp, const char *mongrp)
 {
 	char controlgroup[128], monitorgroup[512], monitorgroup_p[256];
 	char tasks[1024];
@@ -555,22 +553,19 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, const char *ctrlgrp,
 	if (ret)
 		goto out;
 
-	/* Create mon grp and write pid into it for "mbm" and "cmt" test */
-	if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)) ||
-	    !strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR))) {
-		if (mongrp) {
-			sprintf(monitorgroup_p, "%s/mon_groups", controlgroup);
-			sprintf(monitorgroup, "%s/%s", monitorgroup_p, mongrp);
-			ret = create_grp(mongrp, monitorgroup, monitorgroup_p);
-			if (ret)
-				goto out;
-
-			sprintf(tasks, "%s/mon_groups/%s/tasks",
-				controlgroup, mongrp);
-			ret = write_pid_to_tasks(tasks, bm_pid);
-			if (ret)
-				goto out;
-		}
+	/* Create monitor group and write pid into if it is used */
+	if (mongrp) {
+		sprintf(monitorgroup_p, "%s/mon_groups", controlgroup);
+		sprintf(monitorgroup, "%s/%s", monitorgroup_p, mongrp);
+		ret = create_grp(mongrp, monitorgroup, monitorgroup_p);
+		if (ret)
+			goto out;
+
+		sprintf(tasks, "%s/mon_groups/%s/tasks",
+			controlgroup, mongrp);
+		ret = write_pid_to_tasks(tasks, bm_pid);
+		if (ret)
+			goto out;
 	}
 
 out:
-- 
2.39.2

Re: [PATCH v2 08/13] selftests/resctrl: Add ->init() callback into resctrl_val_param

[Maciej Wieczor-Retman]

Hi,

On 2024-03-11 at 15:52:25 +0200, Ilpo Järvinen wrote:
>diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
>index 241c0b129b58..e79eca9346f3 100644
>--- a/tools/testing/selftests/resctrl/cmt_test.c
>+++ b/tools/testing/selftests/resctrl/cmt_test.c
>@@ -16,6 +16,17 @@
> #define MAX_DIFF		2000000
> #define MAX_DIFF_PERCENT	15
> 
>+#define CON_MON_LCC_OCCUP_PATH		\
>+	"%s/%s/mon_groups/%s/mon_data/mon_L3_%02d/llc_occupancy"
>+
>+static int set_cmt_path(const struct resctrl_val_param *param, int domain_id)
>+{
>+	sprintf(llc_occup_path,	CON_MON_LCC_OCCUP_PATH,	RESCTRL_PATH,
>+		param->ctrlgrp, param->mongrp, domain_id);
>+
>+	return 0;
>+}
>+

Is there an option to make this function (and the set_mbm_path()) global through
the resctrl.h?

I'd like to use it in my SNC series [1] for looping over different nodes and
that requires changing the paths during the measure phase of the tests and that
part is currently in cache.c:measure_llc_resctrl().

Or would you suggest some other way of changing these paths in cache?

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH v2 08/13] selftests/resctrl: Add ->init() callback into resctrl_val_param

[Maciej Wieczor-Retman]

On 2024-03-13 at 11:15:30 +0100, Maciej Wieczor-Retman wrote:
>Hi,
>
>On 2024-03-11 at 15:52:25 +0200, Ilpo Järvinen wrote:
>>diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
>>index 241c0b129b58..e79eca9346f3 100644
>>--- a/tools/testing/selftests/resctrl/cmt_test.c
>>+++ b/tools/testing/selftests/resctrl/cmt_test.c
>>@@ -16,6 +16,17 @@
>> #define MAX_DIFF		2000000
>> #define MAX_DIFF_PERCENT	15
>> 
>>+#define CON_MON_LCC_OCCUP_PATH		\
>>+	"%s/%s/mon_groups/%s/mon_data/mon_L3_%02d/llc_occupancy"
>>+
>>+static int set_cmt_path(const struct resctrl_val_param *param, int domain_id)
>>+{
>>+	sprintf(llc_occup_path,	CON_MON_LCC_OCCUP_PATH,	RESCTRL_PATH,
>>+		param->ctrlgrp, param->mongrp, domain_id);
>>+
>>+	return 0;
>>+}
>>+
>
>Is there an option to make this function (and the set_mbm_path()) global through
>the resctrl.h?
>
>I'd like to use it in my SNC series [1] for looping over different nodes and
>that requires changing the paths during the measure phase of the tests and that
>part is currently in cache.c:measure_llc_resctrl().
>
>Or would you suggest some other way of changing these paths in cache?
>

+forgot to add the link :b

[1] https://lore.kernel.org/all/cover.1709721159.git.maciej.wieczor-retman@intel.com/

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH v2 08/13] selftests/resctrl: Add ->init() callback into resctrl_val_param

[Ilpo Järvinen]

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

On Wed, 13 Mar 2024, Maciej Wieczor-Retman wrote:
> On 2024-03-13 at 11:15:30 +0100, Maciej Wieczor-Retman wrote:
> >On 2024-03-11 at 15:52:25 +0200, Ilpo Järvinen wrote:
> >>diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
> >>index 241c0b129b58..e79eca9346f3 100644
> >>--- a/tools/testing/selftests/resctrl/cmt_test.c
> >>+++ b/tools/testing/selftests/resctrl/cmt_test.c
> >>@@ -16,6 +16,17 @@
> >> #define MAX_DIFF		2000000
> >> #define MAX_DIFF_PERCENT	15
> >> 
> >>+#define CON_MON_LCC_OCCUP_PATH		\
> >>+	"%s/%s/mon_groups/%s/mon_data/mon_L3_%02d/llc_occupancy"
> >>+
> >>+static int set_cmt_path(const struct resctrl_val_param *param, int domain_id)
> >>+{
> >>+	sprintf(llc_occup_path,	CON_MON_LCC_OCCUP_PATH,	RESCTRL_PATH,
> >>+		param->ctrlgrp, param->mongrp, domain_id);
> >>+
> >>+	return 0;
> >>+}
> >>+
> >
> >Is there an option to make this function (and the set_mbm_path()) global through
> >the resctrl.h?
> >
> >I'd like to use it in my SNC series [1] for looping over different nodes and
> >that requires changing the paths during the measure phase of the tests and that
> >part is currently in cache.c:measure_llc_resctrl().
> >
> >Or would you suggest some other way of changing these paths in cache?
> >
> 
> +forgot to add the link :b
> 
> [1] https://lore.kernel.org/all/cover.1709721159.git.maciej.wieczor-retman@intel.com/

Perhaps ->init() should just prepare an array of filenames to read from 
to support SNC. That would keep the filename preparations out of the 
measurement period.

It feels slightly hacky to have an array of files but I cannot think of 
anything else that would be cleaner and would not require creating the 
filenames during the actual test.


-- 
 i.

Re: [PATCH v2 08/13] selftests/resctrl: Add ->init() callback into resctrl_val_param

[Maciej Wieczor-Retman]

On 2024-03-13 at 13:37:51 +0200, Ilpo Järvinen wrote:
>On Wed, 13 Mar 2024, Maciej Wieczor-Retman wrote:
>> On 2024-03-13 at 11:15:30 +0100, Maciej Wieczor-Retman wrote:
>> >On 2024-03-11 at 15:52:25 +0200, Ilpo Järvinen wrote:
>> >>diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
>> >>index 241c0b129b58..e79eca9346f3 100644
>> >>--- a/tools/testing/selftests/resctrl/cmt_test.c
>> >>+++ b/tools/testing/selftests/resctrl/cmt_test.c
>> >>@@ -16,6 +16,17 @@
>> >> #define MAX_DIFF		2000000
>> >> #define MAX_DIFF_PERCENT	15
>> >> 
>> >>+#define CON_MON_LCC_OCCUP_PATH		\
>> >>+	"%s/%s/mon_groups/%s/mon_data/mon_L3_%02d/llc_occupancy"
>> >>+
>> >>+static int set_cmt_path(const struct resctrl_val_param *param, int domain_id)
>> >>+{
>> >>+	sprintf(llc_occup_path,	CON_MON_LCC_OCCUP_PATH,	RESCTRL_PATH,
>> >>+		param->ctrlgrp, param->mongrp, domain_id);
>> >>+
>> >>+	return 0;
>> >>+}
>> >>+
>> >
>> >Is there an option to make this function (and the set_mbm_path()) global through
>> >the resctrl.h?
>> >
>> >I'd like to use it in my SNC series [1] for looping over different nodes and
>> >that requires changing the paths during the measure phase of the tests and that
>> >part is currently in cache.c:measure_llc_resctrl().
>> >
>> >Or would you suggest some other way of changing these paths in cache?
>> >
>> 
>> +forgot to add the link :b
>> 
>> [1] https://lore.kernel.org/all/cover.1709721159.git.maciej.wieczor-retman@intel.com/
>
>Perhaps ->init() should just prepare an array of filenames to read from 
>to support SNC. That would keep the filename preparations out of the 
>measurement period.
>
>It feels slightly hacky to have an array of files but I cannot think of 
>anything else that would be cleaner and would not require creating the 
>filenames during the actual test.

So the array of names would be a part of "struct resctrl_val_param"?

It would have to be dynamically allocated too before running the tests.

I kinda agree, creating filenames during measurements messes the whole
separation idea between setup and measuring. And as you said there are like 4
nodes max so not much memory would be wasted there.

I can experiment with it and try to add it in my series - since it's much more
tied to SNC. Unless you see it'd better fit here then that's fine with me too.

>
>
>-- 
> i.


-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH v2 08/13] selftests/resctrl: Add ->init() callback into resctrl_val_param

[Ilpo Järvinen]

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

On Wed, 13 Mar 2024, Maciej Wieczor-Retman wrote:

> On 2024-03-13 at 13:37:51 +0200, Ilpo Järvinen wrote:
> >On Wed, 13 Mar 2024, Maciej Wieczor-Retman wrote:
> >> On 2024-03-13 at 11:15:30 +0100, Maciej Wieczor-Retman wrote:
> >> >On 2024-03-11 at 15:52:25 +0200, Ilpo Järvinen wrote:
> >> >>diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
> >> >>index 241c0b129b58..e79eca9346f3 100644
> >> >>--- a/tools/testing/selftests/resctrl/cmt_test.c
> >> >>+++ b/tools/testing/selftests/resctrl/cmt_test.c
> >> >>@@ -16,6 +16,17 @@
> >> >> #define MAX_DIFF		2000000
> >> >> #define MAX_DIFF_PERCENT	15
> >> >> 
> >> >>+#define CON_MON_LCC_OCCUP_PATH		\
> >> >>+	"%s/%s/mon_groups/%s/mon_data/mon_L3_%02d/llc_occupancy"
> >> >>+
> >> >>+static int set_cmt_path(const struct resctrl_val_param *param, int domain_id)
> >> >>+{
> >> >>+	sprintf(llc_occup_path,	CON_MON_LCC_OCCUP_PATH,	RESCTRL_PATH,
> >> >>+		param->ctrlgrp, param->mongrp, domain_id);
> >> >>+
> >> >>+	return 0;
> >> >>+}
> >> >>+
> >> >
> >> >Is there an option to make this function (and the set_mbm_path()) global through
> >> >the resctrl.h?
> >> >
> >> >I'd like to use it in my SNC series [1] for looping over different nodes and
> >> >that requires changing the paths during the measure phase of the tests and that
> >> >part is currently in cache.c:measure_llc_resctrl().
> >> >
> >> >Or would you suggest some other way of changing these paths in cache?
> >> >
> >> 
> >> +forgot to add the link :b
> >> 
> >> [1] https://lore.kernel.org/all/cover.1709721159.git.maciej.wieczor-retman@intel.com/
> >
> >Perhaps ->init() should just prepare an array of filenames to read from 
> >to support SNC. That would keep the filename preparations out of the 
> >measurement period.
> >
> >It feels slightly hacky to have an array of files but I cannot think of 
> >anything else that would be cleaner and would not require creating the 
> >filenames during the actual test.
> 
> So the array of names would be a part of "struct resctrl_val_param"?

This series has in independent of resctrl_val_param because resctrl_val() 
itself doesn't care after the separation of generic and per test logic. 

But that meant making it globals into the per test files which I didn't 
like that much either. So perhaps having it in resctrl_val_param is 
still better.

-- 
 i.

> It would have to be dynamically allocated too before running the tests.
> 
> I kinda agree, creating filenames during measurements messes the whole
> separation idea between setup and measuring. And as you said there are like 4
> nodes max so not much memory would be wasted there.
> 
> I can experiment with it and try to add it in my series - since it's much more
> tied to SNC. Unless you see it'd better fit here then that's fine with me too.

Re: [PATCH v2 08/13] selftests/resctrl: Add ->init() callback into resctrl_val_param

[Maciej Wieczor-Retman]

Hi again :)

On 2024-03-11 at 15:52:25 +0200, Ilpo Järvinen wrote:
>diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
>index 17398cd3aace..ffbfcecf9bd6 100644
>--- a/tools/testing/selftests/resctrl/mbm_test.c
>+++ b/tools/testing/selftests/resctrl/mbm_test.c
>@@ -8,12 +8,19 @@
>  *    Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>,
>  *    Fenghua Yu <fenghua.yu@intel.com>
>  */
>+#include <limits.h>
>+
> #include "resctrl.h"
> 
> #define RESULT_FILE_NAME	"result_mbm"
> #define MAX_DIFF_PERCENT	8
> #define NUM_OF_RUNS		5
> 
>+#define CON_MON_MBM_LOCAL_BYTES_PATH \
>+	"%s/%s/mon_groups/%s/mon_data/mon_L3_%02d/mbm_local_bytes"
>+
>+static char mbm_total_path[PATH_MAX];
>+
> static int
> show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc, size_t span)
> {
>@@ -86,6 +93,20 @@ static int check_results(size_t span)
> 	return ret;
> }
> 
>+static int set_mbm_path(const struct resctrl_val_param *param, int domain_id)
>+{
>+	int ret;
>+
>+	ret = initialize_mem_bw_imc();

I just noticed this. Since there is not only path stuff here but also some imc
logic maybe the function names could be changed? Something like

	set_mbm_path -> init_mbm

The same could apply for all these init functions or at least the mba one.

>+	if (ret)
>+		return ret;
>+
>+	sprintf(mbm_total_path, CON_MON_MBM_LOCAL_BYTES_PATH,
>+		RESCTRL_PATH, param->ctrlgrp, param->mongrp, domain_id);
>+
>+	return 0;
>+}
>+
>

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH v2 08/13] selftests/resctrl: Add ->init() callback into resctrl_val_param

[Ilpo Järvinen]

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

On Thu, 14 Mar 2024, Maciej Wieczor-Retman wrote:
> On 2024-03-11 at 15:52:25 +0200, Ilpo Järvinen wrote:
> >diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
> >index 17398cd3aace..ffbfcecf9bd6 100644
> >--- a/tools/testing/selftests/resctrl/mbm_test.c
> >+++ b/tools/testing/selftests/resctrl/mbm_test.c
> >@@ -8,12 +8,19 @@
> >  *    Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>,
> >  *    Fenghua Yu <fenghua.yu@intel.com>
> >  */
> >+#include <limits.h>
> >+
> > #include "resctrl.h"
> > 
> > #define RESULT_FILE_NAME	"result_mbm"
> > #define MAX_DIFF_PERCENT	8
> > #define NUM_OF_RUNS		5
> > 
> >+#define CON_MON_MBM_LOCAL_BYTES_PATH \
> >+	"%s/%s/mon_groups/%s/mon_data/mon_L3_%02d/mbm_local_bytes"
> >+
> >+static char mbm_total_path[PATH_MAX];
> >+
> > static int
> > show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc, size_t span)
> > {
> >@@ -86,6 +93,20 @@ static int check_results(size_t span)
> > 	return ret;
> > }
> > 
> >+static int set_mbm_path(const struct resctrl_val_param *param, int domain_id)
> >+{
> >+	int ret;
> >+
> >+	ret = initialize_mem_bw_imc();
> 
> I just noticed this. Since there is not only path stuff here but also some imc
> logic maybe the function names could be changed? Something like
> 
> 	set_mbm_path -> init_mbm
> 
> The same could apply for all these init functions or at least the mba one.

Ah yes, I'll rename them.

-- 
 i.

Re: [PATCH v2 01/13] selftests/resctrl: Convert get_mem_bw_imc() fd close to for loop

[Reinette Chatre]

Hi Ilpo,

On 3/11/2024 6:52 AM, Ilpo Järvinen wrote:
> The open() side handles fds in a for loop but close() is based on two
> fixed indexes READ and WRITE.
> 
> Match the close() side with the open() side by using for loop for
> consistency.

I find the close() side to be more appropriate. I say this for two
reasons: (a) looking at the close() calls as they are now it is
obvious what the close() applies to and transitioning to a loop
adds a layer of unnecessary indirection, (b) I do not think a loop
is appropriate for the READ/WRITE define that just happen to be 0
and 1 ... there should not be an assumption about their underlying
value.

Reinette

Re: [PATCH v2 02/13] selftests/resctrl: Calculate resctrl FS derived mem bw over sleep(1) only

[Reinette Chatre]

Hi Ilpo,

On 3/11/2024 6:52 AM, Ilpo Järvinen wrote:
> For MBM/MBA tests, measure_vals() calls get_mem_bw_imc() that performs
> the measurement over a duration of sleep(1) call. The memory bandwidth
> numbers from IMC are derived over this duration. The resctrl FS derived
> memory bandwidth, however, is calculated inside measure_vals() and only
> takes delta between the previous value and the current one which
> besides the actual test, also samples inter-test noise.
> 
> Rework the logic in measure_vals() and get_mem_bw_imc() such that the
> resctrl FS memory bandwidth section covers much shorter duration
> closely matching that of the IMC perf counters to improve measurement
> accuracy.
> 

Thank you very much for doing this.

> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  tools/testing/selftests/resctrl/resctrl_val.c | 72 +++++++++++++------
>  1 file changed, 50 insertions(+), 22 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index 36139cba7be8..4df2cd738f88 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -293,28 +293,35 @@ static int initialize_mem_bw_imc(void)
>  }
>  
>  /*
> - * get_mem_bw_imc:	Memory band width as reported by iMC counters
> + * perf_open_imc_mem_bw - Open perf fds for IMCs
>   * @cpu_no:		CPU number that the benchmark PID is binded to
> - * @bw_report:		Bandwidth report type (reads, writes)
> - *
> - * Memory B/W utilized by a process on a socket can be calculated using
> - * iMC counters. Perf events are used to read these counters.
> - *
> - * Return: = 0 on success. < 0 on failure.

This "Return" still seems relevant.

>   */
> -static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
> +static int perf_open_imc_mem_bw(int cpu_no)
>  {
> -	float reads, writes, of_mul_read, of_mul_write;
>  	int imc, j, ret;
>  
> -	/* Start all iMC counters to log values (both read and write) */
> -	reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
>  	for (imc = 0; imc < imcs; imc++) {
>  		for (j = 0; j < 2; j++) {
>  			ret = open_perf_event(imc, cpu_no, j);
>  			if (ret)
>  				return -1;
>  		}

I'm feeling more strongly that this inner loop makes the code harder to
understand and unwinding it would make it easier to understand.

> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * do_mem_bw_test - Perform memory bandwidth test
> + *
> + * Runs memory bandwidth test over one second period. Also, handles starting
> + * and stopping of the IMC perf counters around the test.
> + */
> +static void do_imc_mem_bw_test(void)
> +{
> +	int imc, j;
> +
> +	for (imc = 0; imc < imcs; imc++) {
>  		for (j = 0; j < 2; j++)
>  			membw_ioctl_perf_event_ioc_reset_enable(imc, j);

Here also. I find these loops unnecessary. I do not think it optimizes anything
and it makes the code harder to understand.

>  	}
> @@ -326,6 +333,24 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
>  		for (j = 0; j < 2; j++)
>  			membw_ioctl_perf_event_ioc_disable(imc, j);

Same here.

>  	}
> +}
> +
> +/*
> + * get_mem_bw_imc - Memory band width as reported by iMC counters
> + * @bw_report:		Bandwidth report type (reads, writes)
> + *
> + * Memory B/W utilized by a process on a socket can be calculated using
> + * iMC counters. Perf events are used to read these counters.

In the above there are three variations of the same: "band width", "Bandwidth",
and "B/W". Please just use one and use it consistently.

> + *
> + * Return: = 0 on success. < 0 on failure.
> + */
> +static int get_mem_bw_imc(char *bw_report, float *bw_imc)
> +{
> +	float reads, writes, of_mul_read, of_mul_write;
> +	int imc, j;
> +
> +	/* Start all iMC counters to log values (both read and write) */
> +	reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
>  
>  	/*
>  	 * Get results which are stored in struct type imc_counter_config
> @@ -593,10 +618,9 @@ static void initialize_llc_occu_resctrl(const char *ctrlgrp, const char *mongrp,
>  }
>  
>  static int measure_vals(const struct user_params *uparams,
> -			struct resctrl_val_param *param,
> -			unsigned long *bw_resc_start)
> +			struct resctrl_val_param *param)
>  {
> -	unsigned long bw_resc, bw_resc_end;
> +	unsigned long bw_resc, bw_resc_start, bw_resc_end;
>  	float bw_imc;
>  	int ret;
>  
> @@ -607,22 +631,27 @@ static int measure_vals(const struct user_params *uparams,
>  	 * Compare the two values to validate resctrl value.
>  	 * It takes 1sec to measure the data.
>  	 */
> -	ret = get_mem_bw_imc(uparams->cpu, param->bw_report, &bw_imc);
> +	ret = perf_open_imc_mem_bw(uparams->cpu);
>  	if (ret < 0)
>  		return ret;
>  
> +	ret = get_mem_bw_resctrl(&bw_resc_start);
> +	if (ret < 0)
> +		return ret;
> +
> +	do_imc_mem_bw_test();
> +
>  	ret = get_mem_bw_resctrl(&bw_resc_end);
>  	if (ret < 0)
>  		return ret;
>  
> -	bw_resc = (bw_resc_end - *bw_resc_start) / MB;
> -	ret = print_results_bw(param->filename, bm_pid, bw_imc, bw_resc);
> -	if (ret)
> +	ret = get_mem_bw_imc(param->bw_report, &bw_imc);
> +	if (ret < 0)
>  		return ret;
>  
> -	*bw_resc_start = bw_resc_end;
> +	bw_resc = (bw_resc_end - bw_resc_start) / MB;
>  
> -	return 0;
> +	return print_results_bw(param->filename, bm_pid, bw_imc, bw_resc);
>  }
>  
>  /*
> @@ -696,7 +725,6 @@ int resctrl_val(const struct resctrl_test *test,
>  		struct resctrl_val_param *param)
>  {
>  	char *resctrl_val = param->resctrl_val;
> -	unsigned long bw_resc_start = 0;

In the current implementation the first iteration's starting measurement
is, as seen above, 0 ... which makes the first measurement unreliable
and dropped for both the MBA and MBM tests. In this enhancement, the
first measurement is no longer skewed so much so I wonder if this enhancement
can be expanded to the analysis phase where first measurement no longer
needs to be dropped?

>  	struct sigaction sigact;
>  	int ret = 0, pipefd[2];
>  	char pipe_message = 0;
> @@ -838,7 +866,7 @@ int resctrl_val(const struct resctrl_test *test,
>  
>  		if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
>  		    !strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
> -			ret = measure_vals(uparams, param, &bw_resc_start);
> +			ret = measure_vals(uparams, param);
>  			if (ret)
>  				break;
>  		} else if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) {

Reinette

Re: [PATCH v2 08/13] selftests/resctrl: Add ->init() callback into resctrl_val_param

[Reinette Chatre]

Hi Ilpo,

On 3/11/2024 6:52 AM, Ilpo Järvinen wrote:
> The struct resctrl_val_param is there to customize behavior inside
> resctrl_val() which is currently not used to full extent and there are
> number of strcmp()s for test name in resctrl_val done by resctrl_val().
> 
> Create ->init() hook into the struct resctrl_val_param to cleanly
> do per test initialization.
> 
> Remove also unused branches to setup paths and the related #defines.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  tools/testing/selftests/resctrl/cmt_test.c    |  12 ++
>  tools/testing/selftests/resctrl/mba_test.c    |  24 +++-
>  tools/testing/selftests/resctrl/mbm_test.c    |  24 +++-
>  tools/testing/selftests/resctrl/resctrl.h     |   9 +-
>  tools/testing/selftests/resctrl/resctrl_val.c | 123 ++----------------
>  5 files changed, 75 insertions(+), 117 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
> index 241c0b129b58..e79eca9346f3 100644
> --- a/tools/testing/selftests/resctrl/cmt_test.c
> +++ b/tools/testing/selftests/resctrl/cmt_test.c
> @@ -16,6 +16,17 @@
>  #define MAX_DIFF		2000000
>  #define MAX_DIFF_PERCENT	15
>  
> +#define CON_MON_LCC_OCCUP_PATH		\
> +	"%s/%s/mon_groups/%s/mon_data/mon_L3_%02d/llc_occupancy"
> +
> +static int set_cmt_path(const struct resctrl_val_param *param, int domain_id)
> +{
> +	sprintf(llc_occup_path,	CON_MON_LCC_OCCUP_PATH,	RESCTRL_PATH,

Strangely some tab characters sneaked in above.

> +		param->ctrlgrp, param->mongrp, domain_id);
> +
> +	return 0;
> +}
> +
>  static int cmt_setup(const struct resctrl_test *test,
>  		     const struct user_params *uparams,
>  		     struct resctrl_val_param *p)

...

> @@ -826,17 +729,11 @@ int resctrl_val(const struct resctrl_test *test,
>  	if (ret)
>  		goto out;
>  
> -	if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
> -	    !strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
> -		ret = initialize_mem_bw_imc();
> +	if (param->init) {
> +		ret = param->init(param, domain_id);
>  		if (ret)
>  			goto out;
> -
> -		initialize_mem_bw_resctrl(param->ctrlgrp, param->mongrp,
> -					  domain_id, resctrl_val);
> -	} else if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
> -		initialize_llc_occu_resctrl(param->ctrlgrp, param->mongrp,
> -					    domain_id, resctrl_val);
> +	}
>  
>  	/* Parent waits for child to be ready. */
>  	close(pipefd[1]);

I am trying to make sense of what these tests are trying to do and
it is starting to look like the monitor groups (as they are used here)
are unnecessary.

Looking at resctrl_val()->write_bm_pid_to_resctrl() I see that the
control group is created with "bm_pid" written to its "tasks" file
and then a monitor group is created as child of the control group
with the same "bm_pid" written to its "tasks" file.
This looks unnecessary to me because when the original control
group is created on a system that supports monitoring then that
control group would automatically be a monitoring group also. In
the resctrl kernel document these different groups are termed
"CTRL_MON" and "MON" groups respectively.

For example, if user creates control group "c1":
# mkdir /sys/fs/resctrl/c1
Then, automatically it would also be a monitoring group with
its monitoring data available from
/sys/fs/resctrl/c1/mon_data/mon_L3_XX/*

PIDs written to /sys/fs/resctrl/c1/tasks will have allocations
assigned in /sys/fs/resctrl/c1/schemata and monitoring data
/sys/fd/resctrl/c1/mon_data/mon_L3_XX/* ... it is not necessary
to create a separate monitoring group to collect that 
monitoring data.

This seems to be the discrepancy between the MBA and MBM test ...
if I understand correctly they end up needing the same data but
the one uses the data from the CTRL_MON group while the other
creates a redundant child MON group to read the same data
that is available from the CTRL_MON group.

Reinette 

Re: [PATCH v2 11/13] selftests/resctrl: Convert ctrlgrp & mongrp to pointers

[Reinette Chatre]

Hi Ilpo,

On 3/11/2024 6:52 AM, Ilpo Järvinen wrote:
> The struct resctrl_val_param has control and monitor groups as char
> arrays but they are not supposed to be mutated within resctrl_val().
> 
> Convert the ctrlgrp and mongrp char array within resctrl_val_param to
> plain const char pointers and adjust the strlen() based checks to
> check NULL instead.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  tools/testing/selftests/resctrl/resctrl.h   | 4 ++--
>  tools/testing/selftests/resctrl/resctrlfs.c | 8 ++++----
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index 52769b075233..54e5bce4c698 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -89,8 +89,8 @@ struct resctrl_test {
>   */
>  struct resctrl_val_param {
>  	char		*resctrl_val;
> -	char		ctrlgrp[64];
> -	char		mongrp[64];
> +	const char	*ctrlgrp;
> +	const char	*mongrp;
>  	char		filename[64];
>  	unsigned long	mask;
>  	int		num_of_runs;
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index 79cf1c593106..dbe0cc6d74fa 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -469,7 +469,7 @@ static int create_grp(const char *grp_name, char *grp, const char *parent_grp)
>  	 * length of grp_name == 0, it means, user wants to use root con_mon
>  	 * grp, so do nothing
>  	 */

Could you please confirm that the comments are still accurate?

> -	if (strlen(grp_name) == 0)
> +	if (!grp_name)
>  		return 0;
>  


Reinette

Re: [PATCH v2 01/13] selftests/resctrl: Convert get_mem_bw_imc() fd close to for loop

[Ilpo Järvinen]

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

On Tue, 19 Mar 2024, Reinette Chatre wrote:
> On 3/11/2024 6:52 AM, Ilpo Järvinen wrote:
> > The open() side handles fds in a for loop but close() is based on two
> > fixed indexes READ and WRITE.
> > 
> > Match the close() side with the open() side by using for loop for
> > consistency.
> 
> I find the close() side to be more appropriate. I say this for two
> reasons: (a) looking at the close() calls as they are now it is
> obvious what the close() applies to and transitioning to a loop
> adds a layer of unnecessary indirection, (b) I do not think a loop
> is appropriate for the READ/WRITE define that just happen to be 0
> and 1 ... there should not be an assumption about their underlying
> value.

Hi,

So to confirm are you suggesting I should remove all the other loops 
instead?

-- 
 i.

Re: [PATCH v2 01/13] selftests/resctrl: Convert get_mem_bw_imc() fd close to for loop

[Ilpo Järvinen]

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

On Fri, 22 Mar 2024, Ilpo Järvinen wrote:

> On Tue, 19 Mar 2024, Reinette Chatre wrote:
> > On 3/11/2024 6:52 AM, Ilpo Järvinen wrote:
> > > The open() side handles fds in a for loop but close() is based on two
> > > fixed indexes READ and WRITE.
> > > 
> > > Match the close() side with the open() side by using for loop for
> > > consistency.
> > 
> > I find the close() side to be more appropriate. I say this for two
> > reasons: (a) looking at the close() calls as they are now it is
> > obvious what the close() applies to and transitioning to a loop
> > adds a layer of unnecessary indirection, (b) I do not think a loop
> > is appropriate for the READ/WRITE define that just happen to be 0
> > and 1 ... there should not be an assumption about their underlying
> > value.
> 
> Hi,
> 
> So to confirm are you suggesting I should remove all the other loops 
> instead?

Nevermind, I read the comment to second patch, so the answer is yes. :-)

-- 
 i.

Re: [PATCH v2 02/13] selftests/resctrl: Calculate resctrl FS derived mem bw over sleep(1) only

[Ilpo Järvinen]

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

On Tue, 19 Mar 2024, Reinette Chatre wrote:
> On 3/11/2024 6:52 AM, Ilpo Järvinen wrote:
> > For MBM/MBA tests, measure_vals() calls get_mem_bw_imc() that performs
> > the measurement over a duration of sleep(1) call. The memory bandwidth
> > numbers from IMC are derived over this duration. The resctrl FS derived
> > memory bandwidth, however, is calculated inside measure_vals() and only
> > takes delta between the previous value and the current one which
> > besides the actual test, also samples inter-test noise.
> > 
> > Rework the logic in measure_vals() and get_mem_bw_imc() such that the
> > resctrl FS memory bandwidth section covers much shorter duration
> > closely matching that of the IMC perf counters to improve measurement
> > accuracy.
> > 
> 
> Thank you very much for doing this.
> 
> > Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> >  tools/testing/selftests/resctrl/resctrl_val.c | 72 +++++++++++++------
> >  1 file changed, 50 insertions(+), 22 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> > index 36139cba7be8..4df2cd738f88 100644
> > --- a/tools/testing/selftests/resctrl/resctrl_val.c
> > +++ b/tools/testing/selftests/resctrl/resctrl_val.c

> > -static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
> > +static int perf_open_imc_mem_bw(int cpu_no)
> >  {
> > -	float reads, writes, of_mul_read, of_mul_write;
> >  	int imc, j, ret;
> >  
> > -	/* Start all iMC counters to log values (both read and write) */
> > -	reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
> >  	for (imc = 0; imc < imcs; imc++) {
> >  		for (j = 0; j < 2; j++) {
> >  			ret = open_perf_event(imc, cpu_no, j);
> >  			if (ret)
> >  				return -1;
> >  		}
> 
> I'm feeling more strongly that this inner loop makes the code harder to
> understand and unwinding it would make it easier to understand.

Okay, I'll unwind them in the first patch.

> >  	}
> > +}
> > +
> > +/*
> > + * get_mem_bw_imc - Memory band width as reported by iMC counters
> > + * @bw_report:		Bandwidth report type (reads, writes)
> > + *
> > + * Memory B/W utilized by a process on a socket can be calculated using
> > + * iMC counters. Perf events are used to read these counters.
> 
> In the above there are three variations of the same: "band width", "Bandwidth",
> and "B/W". Please just use one and use it consistently.

Okay but I'll do that in a separate patch because these are just the 
"removed" lines above, the diff is more messy than the actual change 
here as is often the case with this kind of split function refactoring 
because the diff algorithm fails to pair the lines optimally from 
human-readed PoV.

> > + * Return: = 0 on success. < 0 on failure.
> > + */
> > +static int get_mem_bw_imc(char *bw_report, float *bw_imc)
> > +{
> > +	float reads, writes, of_mul_read, of_mul_write;
> > +	int imc, j;
> > +
> > +	/* Start all iMC counters to log values (both read and write) */
> > +	reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
> >  
> >  	/*
> >  	 * Get results which are stored in struct type imc_counter_config

> > @@ -696,7 +725,6 @@ int resctrl_val(const struct resctrl_test *test,
> >  		struct resctrl_val_param *param)
> >  {
> >  	char *resctrl_val = param->resctrl_val;
> > -	unsigned long bw_resc_start = 0;
> 
> In the current implementation the first iteration's starting measurement
> is, as seen above, 0 ... which makes the first measurement unreliable
> and dropped for both the MBA and MBM tests. In this enhancement, the
> first measurement is no longer skewed so much so I wonder if this enhancement
> can be expanded to the analysis phase where first measurement no longer
> needs to be dropped?

In ideal world, yes, but I'll have to check the raw numbers. My general 
feel is that the numbers tend to converge slowly with more iterations 
being run so the first iteration might still be "off" by quite much (this 
is definitely the case with CAT tests iterations but I'm not entirely sure 
any more how it is with other selftests).

Thanks for the review.

-- 
 i.

Re: [PATCH v2 08/13] selftests/resctrl: Add ->init() callback into resctrl_val_param

[Ilpo Järvinen]

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

On Tue, 19 Mar 2024, Reinette Chatre wrote:
> On 3/11/2024 6:52 AM, Ilpo Järvinen wrote:
> > The struct resctrl_val_param is there to customize behavior inside
> > resctrl_val() which is currently not used to full extent and there are
> > number of strcmp()s for test name in resctrl_val done by resctrl_val().
> > 
> > Create ->init() hook into the struct resctrl_val_param to cleanly
> > do per test initialization.
> > 
> > Remove also unused branches to setup paths and the related #defines.
> > 
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> >  tools/testing/selftests/resctrl/cmt_test.c    |  12 ++
> >  tools/testing/selftests/resctrl/mba_test.c    |  24 +++-
> >  tools/testing/selftests/resctrl/mbm_test.c    |  24 +++-
> >  tools/testing/selftests/resctrl/resctrl.h     |   9 +-
> >  tools/testing/selftests/resctrl/resctrl_val.c | 123 ++----------------
> >  5 files changed, 75 insertions(+), 117 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
> > index 241c0b129b58..e79eca9346f3 100644
> > --- a/tools/testing/selftests/resctrl/cmt_test.c
> > +++ b/tools/testing/selftests/resctrl/cmt_test.c
> > @@ -16,6 +16,17 @@
> >  #define MAX_DIFF		2000000
> >  #define MAX_DIFF_PERCENT	15
> >  
> > +#define CON_MON_LCC_OCCUP_PATH		\
> > +	"%s/%s/mon_groups/%s/mon_data/mon_L3_%02d/llc_occupancy"
> > +
> > +static int set_cmt_path(const struct resctrl_val_param *param, int domain_id)
> > +{
> > +	sprintf(llc_occup_path,	CON_MON_LCC_OCCUP_PATH,	RESCTRL_PATH,
> 
> Strangely some tab characters sneaked in above.

Ah, fixed it now. They seemed to came directly from the old code.

> > @@ -826,17 +729,11 @@ int resctrl_val(const struct resctrl_test *test,
> >  	if (ret)
> >  		goto out;
> >  
> > -	if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
> > -	    !strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
> > -		ret = initialize_mem_bw_imc();
> > +	if (param->init) {
> > +		ret = param->init(param, domain_id);
> >  		if (ret)
> >  			goto out;
> > -
> > -		initialize_mem_bw_resctrl(param->ctrlgrp, param->mongrp,
> > -					  domain_id, resctrl_val);
> > -	} else if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
> > -		initialize_llc_occu_resctrl(param->ctrlgrp, param->mongrp,
> > -					    domain_id, resctrl_val);
> > +	}
> >  
> >  	/* Parent waits for child to be ready. */
> >  	close(pipefd[1]);
> 
> I am trying to make sense of what these tests are trying to do and
> it is starting to look like the monitor groups (as they are used here)
> are unnecessary.
> 
> Looking at resctrl_val()->write_bm_pid_to_resctrl() I see that the
> control group is created with "bm_pid" written to its "tasks" file
> and then a monitor group is created as child of the control group
> with the same "bm_pid" written to its "tasks" file.
> This looks unnecessary to me because when the original control
> group is created on a system that supports monitoring then that
> control group would automatically be a monitoring group also. In
> the resctrl kernel document these different groups are termed
> "CTRL_MON" and "MON" groups respectively.
> 
> For example, if user creates control group "c1":
> # mkdir /sys/fs/resctrl/c1
> Then, automatically it would also be a monitoring group with
> its monitoring data available from
> /sys/fs/resctrl/c1/mon_data/mon_L3_XX/*
> 
> PIDs written to /sys/fs/resctrl/c1/tasks will have allocations
> assigned in /sys/fs/resctrl/c1/schemata and monitoring data
> /sys/fd/resctrl/c1/mon_data/mon_L3_XX/* ... it is not necessary
> to create a separate monitoring group to collect that 
> monitoring data.
> 
> This seems to be the discrepancy between the MBA and MBM test ...
> if I understand correctly they end up needing the same data but
> the one uses the data from the CTRL_MON group while the other
> creates a redundant child MON group to read the same data
> that is available from the CTRL_MON group.

Okay, I can look into this too. I've not looked too deeply why the 
difference between MBA and MBM test is there.

-- 
 i.

Re: [PATCH v2 11/13] selftests/resctrl: Convert ctrlgrp & mongrp to pointers

[Ilpo Järvinen]

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

On Wed, 20 Mar 2024, Reinette Chatre wrote:
> On 3/11/2024 6:52 AM, Ilpo Järvinen wrote:
> > The struct resctrl_val_param has control and monitor groups as char
> > arrays but they are not supposed to be mutated within resctrl_val().
> > 
> > Convert the ctrlgrp and mongrp char array within resctrl_val_param to
> > plain const char pointers and adjust the strlen() based checks to
> > check NULL instead.
> > 
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> >  tools/testing/selftests/resctrl/resctrl.h   | 4 ++--
> >  tools/testing/selftests/resctrl/resctrlfs.c | 8 ++++----
> >  2 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> > index 52769b075233..54e5bce4c698 100644
> > --- a/tools/testing/selftests/resctrl/resctrl.h
> > +++ b/tools/testing/selftests/resctrl/resctrl.h
> > @@ -89,8 +89,8 @@ struct resctrl_test {
> >   */
> >  struct resctrl_val_param {
> >  	char		*resctrl_val;
> > -	char		ctrlgrp[64];
> > -	char		mongrp[64];
> > +	const char	*ctrlgrp;
> > +	const char	*mongrp;
> >  	char		filename[64];
> >  	unsigned long	mask;
> >  	int		num_of_runs;
> > diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> > index 79cf1c593106..dbe0cc6d74fa 100644
> > --- a/tools/testing/selftests/resctrl/resctrlfs.c
> > +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> > @@ -469,7 +469,7 @@ static int create_grp(const char *grp_name, char *grp, const char *parent_grp)
> >  	 * length of grp_name == 0, it means, user wants to use root con_mon
> >  	 * grp, so do nothing
> >  	 */
> 
> Could you please confirm that the comments are still accurate?

It's not, I missed it.

> > -	if (strlen(grp_name) == 0)
> > +	if (!grp_name)
> >  		return 0;

But now when looking into the surrounding code, to me it looks the correct 
action here is to remove the comment and return -1 instead of 0. It makes
this just an internal sanity check that grp_name is provided by the 
caller.

-- 
 i.

Re: [PATCH v2 02/13] selftests/resctrl: Calculate resctrl FS derived mem bw over sleep(1) only

[Reinette Chatre]

Hi Ilpo,

On 3/22/2024 5:11 AM, Ilpo Järvinen wrote:
> On Tue, 19 Mar 2024, Reinette Chatre wrote:
>> On 3/11/2024 6:52 AM, Ilpo Järvinen wrote:


>>> -static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
>>> +static int perf_open_imc_mem_bw(int cpu_no)
>>>  {
>>> -	float reads, writes, of_mul_read, of_mul_write;
>>>  	int imc, j, ret;
>>>  
>>> -	/* Start all iMC counters to log values (both read and write) */
>>> -	reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
>>>  	for (imc = 0; imc < imcs; imc++) {
>>>  		for (j = 0; j < 2; j++) {
>>>  			ret = open_perf_event(imc, cpu_no, j);
>>>  			if (ret)
>>>  				return -1;
>>>  		}
>>
>> I'm feeling more strongly that this inner loop makes the code harder to
>> understand and unwinding it would make it easier to understand.
> 
> Okay, I'll unwind them in the first patch.

Thank you very much.

> 
>>>  	}
>>> +}
>>> +
>>> +/*
>>> + * get_mem_bw_imc - Memory band width as reported by iMC counters
>>> + * @bw_report:		Bandwidth report type (reads, writes)
>>> + *
>>> + * Memory B/W utilized by a process on a socket can be calculated using
>>> + * iMC counters. Perf events are used to read these counters.
>>
>> In the above there are three variations of the same: "band width", "Bandwidth",
>> and "B/W". Please just use one and use it consistently.
> 
> Okay but I'll do that in a separate patch because these are just the 
> "removed" lines above, the diff is more messy than the actual change 
> here as is often the case with this kind of split function refactoring 
> because the diff algorithm fails to pair the lines optimally from 
> human-readed PoV.

I have not used these much myself bit I've heard folks having success
getting more readable diffs when using the --patience or --histogram
algorithms.

> 
>>> + * Return: = 0 on success. < 0 on failure.
>>> + */
>>> +static int get_mem_bw_imc(char *bw_report, float *bw_imc)
>>> +{
>>> +	float reads, writes, of_mul_read, of_mul_write;
>>> +	int imc, j;
>>> +
>>> +	/* Start all iMC counters to log values (both read and write) */
>>> +	reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
>>>  
>>>  	/*
>>>  	 * Get results which are stored in struct type imc_counter_config
> 
>>> @@ -696,7 +725,6 @@ int resctrl_val(const struct resctrl_test *test,
>>>  		struct resctrl_val_param *param)
>>>  {
>>>  	char *resctrl_val = param->resctrl_val;
>>> -	unsigned long bw_resc_start = 0;
>>
>> In the current implementation the first iteration's starting measurement
>> is, as seen above, 0 ... which makes the first measurement unreliable
>> and dropped for both the MBA and MBM tests. In this enhancement, the
>> first measurement is no longer skewed so much so I wonder if this enhancement
>> can be expanded to the analysis phase where first measurement no longer
>> needs to be dropped?
> 
> In ideal world, yes, but I'll have to check the raw numbers. My general 
> feel is that the numbers tend to converge slowly with more iterations 
> being run so the first iteration might still be "off" by quite much (this 
> is definitely the case with CAT tests iterations but I'm not entirely sure 
> any more how it is with other selftests).
> 

From what I can tell the CAT test is not dropping any results. It looks
to me that any "settling" is and should be handled in the test before
the data collection starts.

Reinette

Re: [PATCH v2 11/13] selftests/resctrl: Convert ctrlgrp & mongrp to pointers

[Reinette Chatre]

Hi Ilpo,

On 3/22/2024 5:30 AM, Ilpo Järvinen wrote:
> On Wed, 20 Mar 2024, Reinette Chatre wrote:
>> On 3/11/2024 6:52 AM, Ilpo Järvinen wrote:
>>> The struct resctrl_val_param has control and monitor groups as char
>>> arrays but they are not supposed to be mutated within resctrl_val().
>>>
>>> Convert the ctrlgrp and mongrp char array within resctrl_val_param to
>>> plain const char pointers and adjust the strlen() based checks to
>>> check NULL instead.
>>>
>>> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>>> ---
>>>  tools/testing/selftests/resctrl/resctrl.h   | 4 ++--
>>>  tools/testing/selftests/resctrl/resctrlfs.c | 8 ++++----
>>>  2 files changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
>>> index 52769b075233..54e5bce4c698 100644
>>> --- a/tools/testing/selftests/resctrl/resctrl.h
>>> +++ b/tools/testing/selftests/resctrl/resctrl.h
>>> @@ -89,8 +89,8 @@ struct resctrl_test {
>>>   */
>>>  struct resctrl_val_param {
>>>  	char		*resctrl_val;
>>> -	char		ctrlgrp[64];
>>> -	char		mongrp[64];
>>> +	const char	*ctrlgrp;
>>> +	const char	*mongrp;
>>>  	char		filename[64];
>>>  	unsigned long	mask;
>>>  	int		num_of_runs;
>>> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
>>> index 79cf1c593106..dbe0cc6d74fa 100644
>>> --- a/tools/testing/selftests/resctrl/resctrlfs.c
>>> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
>>> @@ -469,7 +469,7 @@ static int create_grp(const char *grp_name, char *grp, const char *parent_grp)
>>>  	 * length of grp_name == 0, it means, user wants to use root con_mon
>>>  	 * grp, so do nothing
>>>  	 */
>>
>> Could you please confirm that the comments are still accurate?
> 
> It's not, I missed it.
> 
>>> -	if (strlen(grp_name) == 0)
>>> +	if (!grp_name)
>>>  		return 0;
> 
> But now when looking into the surrounding code, to me it looks the correct 
> action here is to remove the comment and return -1 instead of 0. It makes
> this just an internal sanity check that grp_name is provided by the 
> caller.
> 

hmmm ... this should not be an error because the caller is not required
to provide grp_name. Not providing grp_name has a specific meaning
of this operating on the CON_MON group and a failure would break flows
operating on the CON_MON group.

Reinette

Re: [PATCH v2 02/13] selftests/resctrl: Calculate resctrl FS derived mem bw over sleep(1) only

[Ilpo Järvinen]

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

On Fri, 22 Mar 2024, Reinette Chatre wrote:
> On 3/22/2024 5:11 AM, Ilpo Järvinen wrote:
> > On Tue, 19 Mar 2024, Reinette Chatre wrote:
> >> On 3/11/2024 6:52 AM, Ilpo Järvinen wrote:

> >>> + * Return: = 0 on success. < 0 on failure.
> >>> + */
> >>> +static int get_mem_bw_imc(char *bw_report, float *bw_imc)
> >>> +{
> >>> +	float reads, writes, of_mul_read, of_mul_write;
> >>> +	int imc, j;
> >>> +
> >>> +	/* Start all iMC counters to log values (both read and write) */
> >>> +	reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
> >>>  
> >>>  	/*
> >>>  	 * Get results which are stored in struct type imc_counter_config
> > 
> >>> @@ -696,7 +725,6 @@ int resctrl_val(const struct resctrl_test *test,
> >>>  		struct resctrl_val_param *param)
> >>>  {
> >>>  	char *resctrl_val = param->resctrl_val;
> >>> -	unsigned long bw_resc_start = 0;
> >>
> >> In the current implementation the first iteration's starting measurement
> >> is, as seen above, 0 ... which makes the first measurement unreliable
> >> and dropped for both the MBA and MBM tests. In this enhancement, the
> >> first measurement is no longer skewed so much so I wonder if this enhancement
> >> can be expanded to the analysis phase where first measurement no longer
> >> needs to be dropped?
> > 
> > In ideal world, yes, but I'll have to check the raw numbers. My general 
> > feel is that the numbers tend to converge slowly with more iterations 
> > being run so the first iteration might still be "off" by quite much (this 
> > is definitely the case with CAT tests iterations but I'm not entirely sure 
> > any more how it is with other selftests).
> 
> >From what I can tell the CAT test is not dropping any results. It looks
> to me that any "settling" is and should be handled in the test before
> the data collection starts.

It doesn't, but the "settling" is there in the raw numbers. I've 
considered adding warm-up test(s) before the actual runs to improve the 
situation but there's just so many thing still to do...

-- 
 i.

Re: [PATCH v2 11/13] selftests/resctrl: Convert ctrlgrp & mongrp to pointers

[Ilpo Järvinen]

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

On Fri, 22 Mar 2024, Reinette Chatre wrote:
> On 3/22/2024 5:30 AM, Ilpo Järvinen wrote:
> > On Wed, 20 Mar 2024, Reinette Chatre wrote:
> >> On 3/11/2024 6:52 AM, Ilpo Järvinen wrote:
> >>> The struct resctrl_val_param has control and monitor groups as char
> >>> arrays but they are not supposed to be mutated within resctrl_val().
> >>>
> >>> Convert the ctrlgrp and mongrp char array within resctrl_val_param to
> >>> plain const char pointers and adjust the strlen() based checks to
> >>> check NULL instead.
> >>>
> >>> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> >>> ---
> >>>  tools/testing/selftests/resctrl/resctrl.h   | 4 ++--
> >>>  tools/testing/selftests/resctrl/resctrlfs.c | 8 ++++----
> >>>  2 files changed, 6 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> >>> index 52769b075233..54e5bce4c698 100644
> >>> --- a/tools/testing/selftests/resctrl/resctrl.h
> >>> +++ b/tools/testing/selftests/resctrl/resctrl.h
> >>> @@ -89,8 +89,8 @@ struct resctrl_test {
> >>>   */
> >>>  struct resctrl_val_param {
> >>>  	char		*resctrl_val;
> >>> -	char		ctrlgrp[64];
> >>> -	char		mongrp[64];
> >>> +	const char	*ctrlgrp;
> >>> +	const char	*mongrp;
> >>>  	char		filename[64];
> >>>  	unsigned long	mask;
> >>>  	int		num_of_runs;
> >>> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> >>> index 79cf1c593106..dbe0cc6d74fa 100644
> >>> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> >>> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> >>> @@ -469,7 +469,7 @@ static int create_grp(const char *grp_name, char *grp, const char *parent_grp)
> >>>  	 * length of grp_name == 0, it means, user wants to use root con_mon
> >>>  	 * grp, so do nothing
> >>>  	 */
> >>
> >> Could you please confirm that the comments are still accurate?
> > 
> > It's not, I missed it.
> > 
> >>> -	if (strlen(grp_name) == 0)
> >>> +	if (!grp_name)
> >>>  		return 0;
> > 
> > But now when looking into the surrounding code, to me it looks the correct 
> > action here is to remove the comment and return -1 instead of 0. It makes
> > this just an internal sanity check that grp_name is provided by the 
> > caller.
> > 
> 
> hmmm ... this should not be an error because the caller is not required
> to provide grp_name. Not providing grp_name has a specific meaning
> of this operating on the CON_MON group and a failure would break flows
> operating on the CON_MON group.

write_bm_pid_to_resctrl() checks for non-NULL mongrp before it calls into 
create_grp() so with current code, I don't think it changes anything. And 
param->ctrlgrp is always non-NULL too so I don't think the return ever 
triggers with the current codebase.

However, I was more talking from API point of view. It feels more natural 
for "create group" function to return error if the caller is inconsistent
with itself by asking to create a group but doesn't want to create a 
group.

-- 
 i.