[PATCH v4 00/16] selftests/resctrl: resctrl_val() related cleanups & improvements

[PATCH v4 00/16] 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.

v4:
- Merged close fix into IMC READ+WRITE rework patch
- Add loop to reset imc_counters_config fds to -1 to be able know which
  need closing
- Introduce perf_close_imc_mem_bw() to close fds
- Open resctrl mem bw file (twice) beforehand to avoid opening it during
  the test
- Remove MBM .mongrp setup
- Remove mongrp from CMT test

v3:
- Rename init functions to <testname>_init()
- Replace for loops with READ+WRITE statements for clarity
- Don't drop Return: entry from perf_open_imc_mem_bw() func comment
- New patch: Fix closing of IMC fds in case of error
- New patch: Make "bandwidth" consistent in comments & prints
- New patch: Simplify mem bandwidth file code
- Remove wrong comment
- Changed grp_name check to return -1 on fail (internal sanity check)

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

Ilpo Järvinen (16):
  selftests/resctrl: Fix closing IMC fds on error and open-code R+W
    instead of loops
  selftests/resctrl: Calculate resctrl FS derived mem bw over sleep(1)
    only
  selftests/resctrl: Make "bandwidth" consistent in comments & prints
  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: Simplify mem bandwidth file code for MBA & MBM
    tests
  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 mongrp from CMT 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    |  22 +-
 tools/testing/selftests/resctrl/mba_test.c    |  26 +-
 tools/testing/selftests/resctrl/mbm_test.c    |  26 +-
 tools/testing/selftests/resctrl/resctrl.h     |  49 ++-
 tools/testing/selftests/resctrl/resctrl_val.c | 362 ++++++++----------
 tools/testing/selftests/resctrl/resctrlfs.c   |  64 ++--
 8 files changed, 287 insertions(+), 273 deletions(-)

-- 
2.39.2

[PATCH v4 01/16] selftests/resctrl: Fix closing IMC fds on error and open-code R+W instead of loops

[Ilpo Järvinen]

The imc perf fd close() calls are missing from all error paths. In
addition, get_mem_bw_imc() handles fds in a for loop but close() is
based on two fixed indexes READ and WRITE.

Open code inner for loops to READ+WRITE entries for clarity and add a
function to close() IMC fds properly in all cases.

Fixes: 7f4d257e3a2a ("selftests/resctrl: Add callback to start a benchmark")
Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---

v4:
- Merge with the fix close patch
- Add loop to reset imc_counters_config fds to -1 to be able know which
  need closing
- Introduce perf_close_imc_mem_bw() to close fds
v3:
- Rework entirely, use open coding instead of for loops for clarity
---
 tools/testing/selftests/resctrl/resctrl_val.c | 54 ++++++++++++-------
 1 file changed, 36 insertions(+), 18 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 445f306d4c2f..f55f5989de72 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -293,6 +293,18 @@ static int initialize_mem_bw_imc(void)
 	return 0;
 }
 
+static void perf_close_imc_mem_bw(void)
+{
+	int mc;
+
+	for (mc = 0; mc < imcs; mc++) {
+		if (imc_counters_config[mc][READ].fd != -1)
+			close(imc_counters_config[mc][READ].fd);
+		if (imc_counters_config[mc][WRITE].fd != -1)
+			close(imc_counters_config[mc][WRITE].fd);
+	}
+}
+
 /*
  * get_mem_bw_imc:	Memory band width as reported by iMC counters
  * @cpu_no:		CPU number that the benchmark PID is binded to
@@ -306,26 +318,33 @@ static int initialize_mem_bw_imc(void)
 static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
 {
 	float reads, writes, of_mul_read, of_mul_write;
-	int imc, j, ret;
+	int imc, ret;
+
+	for (imc = 0; imc < imcs; imc++) {
+		imc_counters_config[imc][READ].fd = -1;
+		imc_counters_config[imc][WRITE].fd = -1;
+	}
 
 	/* 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;
-		}
-		for (j = 0; j < 2; j++)
-			membw_ioctl_perf_event_ioc_reset_enable(imc, j);
+		ret = open_perf_event(imc, cpu_no, READ);
+		if (ret)
+			goto close_fds;
+		ret = open_perf_event(imc, cpu_no, WRITE);
+		if (ret)
+			goto close_fds;
+
+		membw_ioctl_perf_event_ioc_reset_enable(imc, READ);
+		membw_ioctl_perf_event_ioc_reset_enable(imc, WRITE);
 	}
 
 	sleep(1);
 
 	/* Stop counters after a second to get results (both read and write) */
 	for (imc = 0; imc < imcs; imc++) {
-		for (j = 0; j < 2; j++)
-			membw_ioctl_perf_event_ioc_disable(imc, j);
+		membw_ioctl_perf_event_ioc_disable(imc, READ);
+		membw_ioctl_perf_event_ioc_disable(imc, WRITE);
 	}
 
 	/*
@@ -341,15 +360,13 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
 		if (read(r->fd, &r->return_value,
 			 sizeof(struct membw_read_format)) == -1) {
 			ksft_perror("Couldn't get read b/w through iMC");
-
-			return -1;
+			goto close_fds;
 		}
 
 		if (read(w->fd, &w->return_value,
 			 sizeof(struct membw_read_format)) == -1) {
 			ksft_perror("Couldn't get write bw through iMC");
-
-			return -1;
+			goto close_fds;
 		}
 
 		__u64 r_time_enabled = r->return_value.time_enabled;
@@ -369,10 +386,7 @@ 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);
-	}
+	perf_close_imc_mem_bw();
 
 	if (strcmp(bw_report, "reads") == 0) {
 		*bw_imc = reads;
@@ -386,6 +400,10 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
 
 	*bw_imc = reads + writes;
 	return 0;
+
+close_fds:
+	perf_close_imc_mem_bw();
+	return -1;
 }
 
 void set_mbm_path(const char *ctrlgrp, const char *mongrp, int domain_id)
-- 
2.39.2

Re: [PATCH v4 01/16] selftests/resctrl: Fix closing IMC fds on error and open-code R+W instead of loops

[Reinette Chatre]

Hi Ilpo,

On 5/20/24 5:30 AM, Ilpo Järvinen wrote:
> The imc perf fd close() calls are missing from all error paths. In
> addition, get_mem_bw_imc() handles fds in a for loop but close() is
> based on two fixed indexes READ and WRITE.
> 
> Open code inner for loops to READ+WRITE entries for clarity and add a
> function to close() IMC fds properly in all cases.
> 
> Fixes: 7f4d257e3a2a ("selftests/resctrl: Add callback to start a benchmark")
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---

Thank you.

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

Reinette

[PATCH v4 02/16] 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. Open two the resctrl mem bw files twice to avoid opening
after the test during measurement period (reading the same file twice
returns the same value so two files are needed).

Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---

v4:
- Open resctrl mem bw file (twice) beforehand to avoid opening it during
  the test
v3:
- Don't drop Return: entry from perf_open_imc_mem_bw() func comment
---
 tools/testing/selftests/resctrl/resctrl_val.c | 119 +++++++++++++-----
 1 file changed, 85 insertions(+), 34 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index f55f5989de72..bd5a1cbb8a21 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -306,18 +306,13 @@ static void perf_close_imc_mem_bw(void)
 }
 
 /*
- * get_mem_bw_imc:	Memory band width as reported by iMC counters
- * @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.
+ * perf_open_imc_mem_bw - Open perf fds for IMCs
+ * @cpu_no: CPU number that the benchmark PID is binded to
  *
  * 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, ret;
 
 	for (imc = 0; imc < imcs; imc++) {
@@ -325,8 +320,6 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
 		imc_counters_config[imc][WRITE].fd = -1;
 	}
 
-	/* 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++) {
 		ret = open_perf_event(imc, cpu_no, READ);
 		if (ret)
@@ -334,7 +327,26 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
 		ret = open_perf_event(imc, cpu_no, WRITE);
 		if (ret)
 			goto close_fds;
+	}
 
+	return 0;
+
+close_fds:
+	perf_close_imc_mem_bw();
+	return -1;
+}
+
+/*
+ * 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;
+
+	for (imc = 0; imc < imcs; imc++) {
 		membw_ioctl_perf_event_ioc_reset_enable(imc, READ);
 		membw_ioctl_perf_event_ioc_reset_enable(imc, WRITE);
 	}
@@ -346,6 +358,24 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
 		membw_ioctl_perf_event_ioc_disable(imc, READ);
 		membw_ioctl_perf_event_ioc_disable(imc, WRITE);
 	}
+}
+
+/*
+ * 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;
+
+	/* 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
@@ -462,24 +492,23 @@ static void initialize_mem_bw_resctrl(const char *ctrlgrp, const char *mongrp,
  * 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 FILE *open_mem_bw_resctrl(const char *mbm_bw_file)
 {
 	FILE *fp;
 
-	fp = fopen(mbm_total_path, "r");
-	if (!fp) {
+	fp = fopen(mbm_bw_file, "r");
+	if (!fp)
 		ksft_perror("Failed to open total bw file");
 
-		return -1;
-	}
+	return fp;
+}
+
+static int get_mem_bw_resctrl(FILE *fp, unsigned long *mbm_total)
+{
 	if (fscanf(fp, "%lu", mbm_total) <= 0) {
 		ksft_perror("Could not get mbm local bytes");
-		fclose(fp);
-
 		return -1;
 	}
-	fclose(fp);
-
 	return 0;
 }
 
@@ -616,13 +645,22 @@ 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;
+	FILE *mem_bw_fp, *mem_bw_fp2;
 	float bw_imc;
 	int ret;
 
+	mem_bw_fp = open_mem_bw_resctrl(mbm_total_path);
+	if (!mem_bw_fp)
+		return -1;
+	mem_bw_fp2 = open_mem_bw_resctrl(mbm_total_path);
+	if (!mem_bw_fp2) {
+		ret = -1;
+		goto close_fp;
+	}
+
 	/*
 	 * Measure memory bandwidth from resctrl and from
 	 * another source which is perf imc value or could
@@ -630,22 +668,36 @@ 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;
+		goto close_fp2;
 
-	ret = get_mem_bw_resctrl(&bw_resc_end);
+	ret = get_mem_bw_resctrl(mem_bw_fp, &bw_resc_start);
 	if (ret < 0)
-		return ret;
+		goto close_fp2;
 
-	bw_resc = (bw_resc_end - *bw_resc_start) / MB;
-	ret = print_results_bw(param->filename, bm_pid, bw_imc, bw_resc);
-	if (ret)
-		return ret;
+	do_imc_mem_bw_test();
+
+	ret = get_mem_bw_resctrl(mem_bw_fp2, &bw_resc_end);
+	if (ret < 0)
+		goto close_fp2;
+
+	ret = get_mem_bw_imc(param->bw_report, &bw_imc);
+	if (ret < 0)
+		goto close_fp2;
 
-	*bw_resc_start = bw_resc_end;
+	fclose(mem_bw_fp2);
+	fclose(mem_bw_fp);
 
-	return 0;
+	bw_resc = (bw_resc_end - bw_resc_start) / MB;
+
+	return print_results_bw(param->filename, bm_pid, bw_imc, bw_resc);
+
+close_fp2:
+	fclose(mem_bw_fp2);
+close_fp:
+	fclose(mem_bw_fp);
+	return ret;
 }
 
 /*
@@ -719,7 +771,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;
@@ -861,7 +912,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

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

[Reinette Chatre]

Hi Ilpo,

On 5/20/24 5:30 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. Open two the resctrl mem bw files twice to avoid opening
> after the test during measurement period (reading the same file twice
> returns the same value so two files are needed).

I think this is only because of how the current reading is done, resctrl
surely supports keeping a file open and reading from it multiple times.

There seems to be two things that prevent current code from doing this
correctly:
(a) the fscanf() code does not take into account that resctrl also
     prints a "\n" ... (this seems to be the part that may cause the same
     value to be returned).
     So:
	if (fscanf(fp, "%lu", mbm_total) <= 0) {
     should be:
	if (fscanf(fp, "%lu\n", mbm_total) <= 0) {
(b) the current reading does not reset the file position so a second
     read will attempt to read past the beginning. A "rewind(fp)"
     should help here.

A small program like below worked for me by showing different values
on every read:

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>

const char *mbm_total_path = "/sys/fs/resctrl/mon_data/mon_L3_00/mbm_total_bytes";

int main(void)
{
	unsigned long mbm_total;
	FILE *fp;
	int count;

	fp = fopen(mbm_total_path, "r");
	if (!fp) {
		perror("Opening data file\n");
		exit(1);
	}
	for (count = 0; count < 100; count++) {
		if (fscanf(fp, "%lu\n", &mbm_total) <= 0) {
			perror("Unable to read from data file\n");
			exit(1);
		}
		printf("Read %d: %lu\n",count ,mbm_total );
		sleep(1);
		rewind(fp);
	}
	fclose(fp);
	return 0;
}

Reinette

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

[Ilpo Järvinen]

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

On Thu, 23 May 2024, Reinette Chatre wrote:
> On 5/20/24 5:30 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. Open two the resctrl mem bw files twice to avoid opening
> > after the test during measurement period (reading the same file twice
> > returns the same value so two files are needed).
> 
> I think this is only because of how the current reading is done, resctrl
> surely supports keeping a file open and reading from it multiple times.
> 
> There seems to be two things that prevent current code from doing this
> correctly:
> (a) the fscanf() code does not take into account that resctrl also
>     prints a "\n" ... (this seems to be the part that may cause the same
>     value to be returned).
>     So:
> 	if (fscanf(fp, "%lu", mbm_total) <= 0) {
>     should be:
> 	if (fscanf(fp, "%lu\n", mbm_total) <= 0) {
> (b) the current reading does not reset the file position so a second
>     read will attempt to read past the beginning. A "rewind(fp)"
>     should help here.

(b) cannot be the cause for returning the same value again. It would 
not be able to reread the number at all if file position is not moved.

I certainly tried with fseek() and it is when I got same value on the 
second read which is when I just went to two files solution.

> A small program like below worked for me by showing different values
> on every read:
> 
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>
> 
> const char *mbm_total_path =
> "/sys/fs/resctrl/mon_data/mon_L3_00/mbm_total_bytes";
> 
> int main(void)
> {
> 	unsigned long mbm_total;
> 	FILE *fp;
> 	int count;
> 
> 	fp = fopen(mbm_total_path, "r");
> 	if (!fp) {
> 		perror("Opening data file\n");
> 		exit(1);
> 	}
> 	for (count = 0; count < 100; count++) {
> 		if (fscanf(fp, "%lu\n", &mbm_total) <= 0) {
> 			perror("Unable to read from data file\n");
> 			exit(1);
> 		}
> 		printf("Read %d: %lu\n",count ,mbm_total );
> 		sleep(1);
> 		rewind(fp);
> 	}
> 	fclose(fp);
> 	return 0;
> }

Okay, so perhaps it's your explanation (a) but can libc be trusted to not 
do buffering/caching for FILE *? So to be on the safe side, it would 
need to use syscalls directly to guarantee it's read the file twice.

If I convert it into fds, fscanf() cannot be used which would complicate 
the string processing by adding extra steps.

-- 
 i.

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

[Reinette Chatre]

Hi Ilpo,

On 5/24/24 12:57 AM, Ilpo Järvinen wrote:
> On Thu, 23 May 2024, Reinette Chatre wrote:
>> On 5/20/24 5:30 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. Open two the resctrl mem bw files twice to avoid opening
>>> after the test during measurement period (reading the same file twice
>>> returns the same value so two files are needed).
>>
>> I think this is only because of how the current reading is done, resctrl
>> surely supports keeping a file open and reading from it multiple times.
>>
>> There seems to be two things that prevent current code from doing this
>> correctly:
>> (a) the fscanf() code does not take into account that resctrl also
>>      prints a "\n" ... (this seems to be the part that may cause the same
>>      value to be returned).
>>      So:
>> 	if (fscanf(fp, "%lu", mbm_total) <= 0) {
>>      should be:
>> 	if (fscanf(fp, "%lu\n", mbm_total) <= 0) {
>> (b) the current reading does not reset the file position so a second
>>      read will attempt to read past the beginning. A "rewind(fp)"
>>      should help here.
> 
> (b) cannot be the cause for returning the same value again. It would
> not be able to reread the number at all if file position is not moved.

I know. This was not intended to explain the duplicate answer but instead
describe another change required to use current code in a loop. I
specifically said in (a) that "(this seems to be the part that may cause
the same value to be returned)".

> I certainly tried with fseek() and it is when I got same value on the
> second read which is when I just went to two files solution.
> 
>> A small program like below worked for me by showing different values
>> on every read:
>>
>> #include <stdio.h>
>> #include <stdlib.h>
>> #include <unistd.h>
>>
>> const char *mbm_total_path =
>> "/sys/fs/resctrl/mon_data/mon_L3_00/mbm_total_bytes";
>>
>> int main(void)
>> {
>> 	unsigned long mbm_total;
>> 	FILE *fp;
>> 	int count;
>>
>> 	fp = fopen(mbm_total_path, "r");
>> 	if (!fp) {
>> 		perror("Opening data file\n");
>> 		exit(1);
>> 	}
>> 	for (count = 0; count < 100; count++) {
>> 		if (fscanf(fp, "%lu\n", &mbm_total) <= 0) {
>> 			perror("Unable to read from data file\n");
>> 			exit(1);
>> 		}
>> 		printf("Read %d: %lu\n",count ,mbm_total );
>> 		sleep(1);
>> 		rewind(fp);
>> 	}
>> 	fclose(fp);
>> 	return 0;
>> }
> 
> Okay, so perhaps it's your explanation (a) but can libc be trusted to not
> do buffering/caching for FILE *? So to be on the safe side, it would

Coding with expectation that libc cannot be trusted sounds strange to me.

> need to use syscalls directly to guarantee it's read the file twice.
> 
> If I convert it into fds, fscanf() cannot be used which would complicate
> the string processing by adding extra steps.
> 

It is not clear to me why you think that fscanf() cannot be used. Could
you please elaborate what the buffering issues are?

It is not necessary to open and close the file every time a value needs
to be read from it.

Reinette

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

[Ilpo Järvinen]

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

On Fri, 24 May 2024, Reinette Chatre wrote:
> On 5/24/24 12:57 AM, Ilpo Järvinen wrote:
> > On Thu, 23 May 2024, Reinette Chatre wrote:
> > > On 5/20/24 5:30 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. Open two the resctrl mem bw files twice to avoid opening
> > > > after the test during measurement period (reading the same file twice
> > > > returns the same value so two files are needed).
> > > 
> > > I think this is only because of how the current reading is done, resctrl
> > > surely supports keeping a file open and reading from it multiple times.
> > > 
> > > There seems to be two things that prevent current code from doing this
> > > correctly:
> > > (a) the fscanf() code does not take into account that resctrl also
> > >      prints a "\n" ... (this seems to be the part that may cause the same
> > >      value to be returned).
> > >      So:
> > > 	if (fscanf(fp, "%lu", mbm_total) <= 0) {
> > >      should be:
> > > 	if (fscanf(fp, "%lu\n", mbm_total) <= 0) {
> > > (b) the current reading does not reset the file position so a second
> > >      read will attempt to read past the beginning. A "rewind(fp)"
> > >      should help here.
> > 
> > (b) cannot be the cause for returning the same value again. It would
> > not be able to reread the number at all if file position is not moved.
> 
> I know. This was not intended to explain the duplicate answer but instead
> describe another change required to use current code in a loop. I
> specifically said in (a) that "(this seems to be the part that may cause
> the same value to be returned)".
> 
> > I certainly tried with fseek() and it is when I got same value on the
> > second read which is when I just went to two files solution.
> > 
> > > A small program like below worked for me by showing different values
> > > on every read:
> > > 
> > > #include <stdio.h>
> > > #include <stdlib.h>
> > > #include <unistd.h>
> > > 
> > > const char *mbm_total_path =
> > > "/sys/fs/resctrl/mon_data/mon_L3_00/mbm_total_bytes";
> > > 
> > > int main(void)
> > > {
> > > 	unsigned long mbm_total;
> > > 	FILE *fp;
> > > 	int count;
> > > 
> > > 	fp = fopen(mbm_total_path, "r");
> > > 	if (!fp) {
> > > 		perror("Opening data file\n");
> > > 		exit(1);
> > > 	}
> > > 	for (count = 0; count < 100; count++) {
> > > 		if (fscanf(fp, "%lu\n", &mbm_total) <= 0) {
> > > 			perror("Unable to read from data file\n");
> > > 			exit(1);
> > > 		}
> > > 		printf("Read %d: %lu\n",count ,mbm_total );
> > > 		sleep(1);
> > > 		rewind(fp);
> > > 	}
> > > 	fclose(fp);
> > > 	return 0;
> > > }
> > 
> > Okay, so perhaps it's your explanation (a) but can libc be trusted to not
> > do buffering/caching for FILE *? So to be on the safe side, it would
> 
> Coding with expectation that libc cannot be trusted sounds strange to me.
> 
> > need to use syscalls directly to guarantee it's read the file twice.
> > 
> > If I convert it into fds, fscanf() cannot be used which would complicate
> > the string processing by adding extra steps.
> > 
> 
> It is not clear to me why you think that fscanf() cannot be used.

This was related to fscanf() not being able to read from an fd which is 
different interface than what libc's FILE * is.

> Could you please elaborate what the buffering issues are?

I'm pretty sure that by default libc does some buffering (even std* 
streams are line buffered and others streams even more). I'm not entirely 
sure about the extent of that buffering but here we need to always read 
the up to date value from the file itself, not from some buffer.

Maybe there never is any problem that the earlier read values are returned 
from some libc buffer when lseek/rewind is used, I just don't know that 
for sure. You seem to be more certain but I've not seen on what basis 
(other than the anecdotial test you provided).

> It is not necessary to open and close the file every time a value needs
> to be read from it.

-- 
 i.

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

[Ilpo Järvinen]

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

On Fri, 24 May 2024, Ilpo Järvinen wrote:

> On Fri, 24 May 2024, Reinette Chatre wrote:
> > On 5/24/24 12:57 AM, Ilpo Järvinen wrote:
> > > On Thu, 23 May 2024, Reinette Chatre wrote:
> > > > On 5/20/24 5:30 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. Open two the resctrl mem bw files twice to avoid opening
> > > > > after the test during measurement period (reading the same file twice
> > > > > returns the same value so two files are needed).
> > > > 
> > > > I think this is only because of how the current reading is done, resctrl
> > > > surely supports keeping a file open and reading from it multiple times.
> > > > 
> > > > There seems to be two things that prevent current code from doing this
> > > > correctly:
> > > > (a) the fscanf() code does not take into account that resctrl also
> > > >      prints a "\n" ... (this seems to be the part that may cause the same
> > > >      value to be returned).
> > > >      So:
> > > > 	if (fscanf(fp, "%lu", mbm_total) <= 0) {
> > > >      should be:
> > > > 	if (fscanf(fp, "%lu\n", mbm_total) <= 0) {
> > > > (b) the current reading does not reset the file position so a second
> > > >      read will attempt to read past the beginning. A "rewind(fp)"
> > > >      should help here.
> > > 
> > > (b) cannot be the cause for returning the same value again. It would
> > > not be able to reread the number at all if file position is not moved.
> > 
> > I know. This was not intended to explain the duplicate answer but instead
> > describe another change required to use current code in a loop. I
> > specifically said in (a) that "(this seems to be the part that may cause
> > the same value to be returned)".
> > 
> > > I certainly tried with fseek() and it is when I got same value on the
> > > second read which is when I just went to two files solution.
> > > 
> > > > A small program like below worked for me by showing different values
> > > > on every read:
> > > > 
> > > > #include <stdio.h>
> > > > #include <stdlib.h>
> > > > #include <unistd.h>
> > > > 
> > > > const char *mbm_total_path =
> > > > "/sys/fs/resctrl/mon_data/mon_L3_00/mbm_total_bytes";
> > > > 
> > > > int main(void)
> > > > {
> > > > 	unsigned long mbm_total;
> > > > 	FILE *fp;
> > > > 	int count;
> > > > 
> > > > 	fp = fopen(mbm_total_path, "r");
> > > > 	if (!fp) {
> > > > 		perror("Opening data file\n");
> > > > 		exit(1);
> > > > 	}
> > > > 	for (count = 0; count < 100; count++) {
> > > > 		if (fscanf(fp, "%lu\n", &mbm_total) <= 0) {
> > > > 			perror("Unable to read from data file\n");
> > > > 			exit(1);
> > > > 		}
> > > > 		printf("Read %d: %lu\n",count ,mbm_total );
> > > > 		sleep(1);
> > > > 		rewind(fp);
> > > > 	}
> > > > 	fclose(fp);
> > > > 	return 0;
> > > > }
> > > 
> > > Okay, so perhaps it's your explanation (a) but can libc be trusted to not
> > > do buffering/caching for FILE *? So to be on the safe side, it would
> > 
> > Coding with expectation that libc cannot be trusted sounds strange to me.
> > 
> > > need to use syscalls directly to guarantee it's read the file twice.
> > > 
> > > If I convert it into fds, fscanf() cannot be used which would complicate
> > > the string processing by adding extra steps.
> > > 
> > 
> > It is not clear to me why you think that fscanf() cannot be used.
> 
> This was related to fscanf() not being able to read from an fd which is 
> different interface than what libc's FILE * is.
> 
> > Could you please elaborate what the buffering issues are?
> 
> I'm pretty sure that by default libc does some buffering (even std* 
> streams are line buffered and others streams even more). I'm not entirely 
> sure about the extent of that buffering but here we need to always read 
> the up to date value from the file itself, not from some buffer.
> 
> Maybe there never is any problem that the earlier read values are returned 
> from some libc buffer when lseek/rewind is used, I just don't know that 
> for sure. You seem to be more certain but I've not seen on what basis 
> (other than the anecdotial test you provided).
> 
> > It is not necessary to open and close the file every time a value needs
> > to be read from it.

I'm bit unsure where to go with this. While I could change the code to 
match what you described, I realized with the two files approach there's 
no need to do even review/lseek() call during the measurement. It might 
not be very significant compared with the open that was there initially 
but it's still extra.

-- 
 i.

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

[Reinette Chatre]

Hi Ilpo,

On 5/28/24 3:19 AM, Ilpo Järvinen wrote:
> On Fri, 24 May 2024, Ilpo Järvinen wrote:
> 
>> On Fri, 24 May 2024, Reinette Chatre wrote:
>>> On 5/24/24 12:57 AM, Ilpo Järvinen wrote:
>>>> On Thu, 23 May 2024, Reinette Chatre wrote:
>>>>> On 5/20/24 5:30 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. Open two the resctrl mem bw files twice to avoid opening
>>>>>> after the test during measurement period (reading the same file twice
>>>>>> returns the same value so two files are needed).
>>>>>
>>>>> I think this is only because of how the current reading is done, resctrl
>>>>> surely supports keeping a file open and reading from it multiple times.
>>>>>
>>>>> There seems to be two things that prevent current code from doing this
>>>>> correctly:
>>>>> (a) the fscanf() code does not take into account that resctrl also
>>>>>       prints a "\n" ... (this seems to be the part that may cause the same
>>>>>       value to be returned).
>>>>>       So:
>>>>> 	if (fscanf(fp, "%lu", mbm_total) <= 0) {
>>>>>       should be:
>>>>> 	if (fscanf(fp, "%lu\n", mbm_total) <= 0) {
>>>>> (b) the current reading does not reset the file position so a second
>>>>>       read will attempt to read past the beginning. A "rewind(fp)"
>>>>>       should help here.
>>>>
>>>> (b) cannot be the cause for returning the same value again. It would
>>>> not be able to reread the number at all if file position is not moved.
>>>
>>> I know. This was not intended to explain the duplicate answer but instead
>>> describe another change required to use current code in a loop. I
>>> specifically said in (a) that "(this seems to be the part that may cause
>>> the same value to be returned)".
>>>
>>>> I certainly tried with fseek() and it is when I got same value on the
>>>> second read which is when I just went to two files solution.
>>>>
>>>>> A small program like below worked for me by showing different values
>>>>> on every read:
>>>>>
>>>>> #include <stdio.h>
>>>>> #include <stdlib.h>
>>>>> #include <unistd.h>
>>>>>
>>>>> const char *mbm_total_path =
>>>>> "/sys/fs/resctrl/mon_data/mon_L3_00/mbm_total_bytes";
>>>>>
>>>>> int main(void)
>>>>> {
>>>>> 	unsigned long mbm_total;
>>>>> 	FILE *fp;
>>>>> 	int count;
>>>>>
>>>>> 	fp = fopen(mbm_total_path, "r");
>>>>> 	if (!fp) {
>>>>> 		perror("Opening data file\n");
>>>>> 		exit(1);
>>>>> 	}
>>>>> 	for (count = 0; count < 100; count++) {
>>>>> 		if (fscanf(fp, "%lu\n", &mbm_total) <= 0) {
>>>>> 			perror("Unable to read from data file\n");
>>>>> 			exit(1);
>>>>> 		}
>>>>> 		printf("Read %d: %lu\n",count ,mbm_total );
>>>>> 		sleep(1);
>>>>> 		rewind(fp);
>>>>> 	}
>>>>> 	fclose(fp);
>>>>> 	return 0;
>>>>> }
>>>>
>>>> Okay, so perhaps it's your explanation (a) but can libc be trusted to not
>>>> do buffering/caching for FILE *? So to be on the safe side, it would
>>>
>>> Coding with expectation that libc cannot be trusted sounds strange to me.
>>>
>>>> need to use syscalls directly to guarantee it's read the file twice.
>>>>
>>>> If I convert it into fds, fscanf() cannot be used which would complicate
>>>> the string processing by adding extra steps.
>>>>
>>>
>>> It is not clear to me why you think that fscanf() cannot be used.
>>
>> This was related to fscanf() not being able to read from an fd which is
>> different interface than what libc's FILE * is.

The part I am missing is why you believe syscalls are required. Could
you please elaborate why FILE * cannot be used?

>>
>>> Could you please elaborate what the buffering issues are?
>>
>> I'm pretty sure that by default libc does some buffering (even std*
>> streams are line buffered and others streams even more). I'm not entirely
>> sure about the extent of that buffering but here we need to always read
>> the up to date value from the file itself, not from some buffer.
>>
>> Maybe there never is any problem that the earlier read values are returned
>> from some libc buffer when lseek/rewind is used, I just don't know that
>> for sure. You seem to be more certain but I've not seen on what basis
>> (other than the anecdotial test you provided).

I demonstrated that it works. I have not heard a clear reason why this conclusion
is incorrect. The above remains vague to me and I cannot find a description of
a clear problem that can be studied.

>>
>>> It is not necessary to open and close the file every time a value needs
>>> to be read from it.
> 
> I'm bit unsure where to go with this. While I could change the code to
> match what you described, I realized with the two files approach there's
> no need to do even review/lseek() call during the measurement. It might
> not be very significant compared with the open that was there initially
> but it's still extra.
> 

We are discussing the resctrl selftests that will accompany the resctrl
filesystem in the kernel. When in doubt on how to interact with resctrl users
use the selftests as reference. Needing to open and close a resctrl file
every time a value needs to be read from it is not the correct guidance.

Reinette

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

[Ilpo Järvinen]

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

On Tue, 28 May 2024, Reinette Chatre wrote:
> On 5/28/24 3:19 AM, Ilpo Järvinen wrote:
> > On Fri, 24 May 2024, Ilpo Järvinen wrote:
> > > On Fri, 24 May 2024, Reinette Chatre wrote:
> > > > On 5/24/24 12:57 AM, Ilpo Järvinen wrote:
> > > > > On Thu, 23 May 2024, Reinette Chatre wrote:
> > > > > > On 5/20/24 5:30 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. Open two the resctrl mem bw files twice to avoid opening
> > > > > > > after the test during measurement period (reading the same file
> > > > > > > twice
> > > > > > > returns the same value so two files are needed).
> > > > > > 
> > > > > > I think this is only because of how the current reading is done,
> > > > > > resctrl
> > > > > > surely supports keeping a file open and reading from it multiple
> > > > > > times.
> > > > > > 
> > > > > > There seems to be two things that prevent current code from doing
> > > > > > this
> > > > > > correctly:
> > > > > > (a) the fscanf() code does not take into account that resctrl also
> > > > > >       prints a "\n" ... (this seems to be the part that may cause
> > > > > > the same
> > > > > >       value to be returned).
> > > > > >       So:
> > > > > > 	if (fscanf(fp, "%lu", mbm_total) <= 0) {
> > > > > >       should be:
> > > > > > 	if (fscanf(fp, "%lu\n", mbm_total) <= 0) {
> > > > > > (b) the current reading does not reset the file position so a second
> > > > > >       read will attempt to read past the beginning. A "rewind(fp)"
> > > > > >       should help here.
> > > > > 
> > > > > (b) cannot be the cause for returning the same value again. It would
> > > > > not be able to reread the number at all if file position is not moved.
> > > > 
> > > > I know. This was not intended to explain the duplicate answer but
> > > > instead
> > > > describe another change required to use current code in a loop. I
> > > > specifically said in (a) that "(this seems to be the part that may cause
> > > > the same value to be returned)".
> > > > 
> > > > > I certainly tried with fseek() and it is when I got same value on the
> > > > > second read which is when I just went to two files solution.
> > > > > 
> > > > > > A small program like below worked for me by showing different values
> > > > > > on every read:
> > > > > > 
> > > > > > #include <stdio.h>
> > > > > > #include <stdlib.h>
> > > > > > #include <unistd.h>
> > > > > > 
> > > > > > const char *mbm_total_path =
> > > > > > "/sys/fs/resctrl/mon_data/mon_L3_00/mbm_total_bytes";
> > > > > > 
> > > > > > int main(void)
> > > > > > {
> > > > > > 	unsigned long mbm_total;
> > > > > > 	FILE *fp;
> > > > > > 	int count;
> > > > > > 
> > > > > > 	fp = fopen(mbm_total_path, "r");
> > > > > > 	if (!fp) {
> > > > > > 		perror("Opening data file\n");
> > > > > > 		exit(1);
> > > > > > 	}
> > > > > > 	for (count = 0; count < 100; count++) {
> > > > > > 		if (fscanf(fp, "%lu\n", &mbm_total) <= 0) {
> > > > > > 			perror("Unable to read from data file\n");
> > > > > > 			exit(1);
> > > > > > 		}
> > > > > > 		printf("Read %d: %lu\n",count ,mbm_total );
> > > > > > 		sleep(1);
> > > > > > 		rewind(fp);
> > > > > > 	}
> > > > > > 	fclose(fp);
> > > > > > 	return 0;
> > > > > > }
> > > > > 
> > > > > Okay, so perhaps it's your explanation (a) but can libc be trusted to
> > > > > not
> > > > > do buffering/caching for FILE *? So to be on the safe side, it would
> > > > 
> > > > Coding with expectation that libc cannot be trusted sounds strange to
> > > > me.
> > > > 
> > > > > need to use syscalls directly to guarantee it's read the file twice.
> > > > > 
> > > > > If I convert it into fds, fscanf() cannot be used which would
> > > > > complicate
> > > > > the string processing by adding extra steps.
> > > > > 
> > > > 
> > > > It is not clear to me why you think that fscanf() cannot be used.
> > > 
> > > This was related to fscanf() not being able to read from an fd which is
> > > different interface than what libc's FILE * is.
> 
> The part I am missing is why you believe syscalls are required. Could
> you please elaborate why FILE * cannot be used?
> 
> > > 
> > > > Could you please elaborate what the buffering issues are?
> > > 
> > > I'm pretty sure that by default libc does some buffering (even std*
> > > streams are line buffered and others streams even more). I'm not entirely
> > > sure about the extent of that buffering but here we need to always read
> > > the up to date value from the file itself, not from some buffer.
> > > 
> > > Maybe there never is any problem that the earlier read values are returned
> > > from some libc buffer when lseek/rewind is used, I just don't know that
> > > for sure. You seem to be more certain but I've not seen on what basis
> > > (other than the anecdotial test you provided).
> 
> I demonstrated that it works. I have not heard a clear reason why this
> conclusion
> is incorrect. The above remains vague to me and I cannot find a description of
> a clear problem that can be studied.

It's pointless to continue this discussion as I don't have anything 
concrete to prove you wrong.

So count it as my paranoia when it comes to putting buffering in between 
something that is read more than once and expecting to get different 
bits out of that buffered interface :-).

I'll just use a single FILE *.

> > > > It is not necessary to open and close the file every time a value needs
> > > > to be read from it.
> > 
> > I'm bit unsure where to go with this. While I could change the code to
> > match what you described, I realized with the two files approach there's
> > no need to do even review/lseek() call during the measurement. It might
> > not be very significant compared with the open that was there initially
> > but it's still extra.
> 
> We are discussing the resctrl selftests that will accompany the resctrl
> filesystem in the kernel. When in doubt on how to interact with resctrl users
> use the selftests as reference. Needing to open and close a resctrl file
> every time a value needs to be read from it is not the correct guidance.

That's actually a different goal from the earlier, but I've no problem 
adjusting to it.

Initially, this open/close() refactoring was made because of another goal 
which was to avoid doing extra syscalls during the test.

-- 
 i.

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

[Reinette Chatre]

Hi Ilpo,

On 5/30/24 4:11 AM, Ilpo Järvinen wrote:
> On Tue, 28 May 2024, Reinette Chatre wrote:
>> On 5/28/24 3:19 AM, Ilpo Järvinen wrote:
>>> On Fri, 24 May 2024, Ilpo Järvinen wrote:
>>>> On Fri, 24 May 2024, Reinette Chatre wrote:
>>>>> On 5/24/24 12:57 AM, Ilpo Järvinen wrote:
>>>>>> On Thu, 23 May 2024, Reinette Chatre wrote:

...

>>>>> It is not necessary to open and close the file every time a value needs
>>>>> to be read from it.
>>>
>>> I'm bit unsure where to go with this. While I could change the code to
>>> match what you described, I realized with the two files approach there's
>>> no need to do even review/lseek() call during the measurement. It might
>>> not be very significant compared with the open that was there initially
>>> but it's still extra.
>>
>> We are discussing the resctrl selftests that will accompany the resctrl
>> filesystem in the kernel. When in doubt on how to interact with resctrl users
>> use the selftests as reference. Needing to open and close a resctrl file
>> every time a value needs to be read from it is not the correct guidance.
> 
> That's actually a different goal from the earlier, but I've no problem
> adjusting to it.
> 
> Initially, this open/close() refactoring was made because of another goal
> which was to avoid doing extra syscalls during the test.
> 

It is not clear what you hint at here. Reading twice from an open file
should not be a huge adjustment so it is not clear to me how this results
in a big change to this work. As I understand this does match with original goal
of reducing syscalls since the file need not be opened and closed twice.

Reinette

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

[Ilpo Järvinen]

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

On Thu, 30 May 2024, Reinette Chatre wrote:

> Hi Ilpo,
> 
> On 5/30/24 4:11 AM, Ilpo Järvinen wrote:
> > On Tue, 28 May 2024, Reinette Chatre wrote:
> > > On 5/28/24 3:19 AM, Ilpo Järvinen wrote:
> > > > On Fri, 24 May 2024, Ilpo Järvinen wrote:
> > > > > On Fri, 24 May 2024, Reinette Chatre wrote:
> > > > > > On 5/24/24 12:57 AM, Ilpo Järvinen wrote:
> > > > > > > On Thu, 23 May 2024, Reinette Chatre wrote:
> 
> ...
> 
> > > > > > It is not necessary to open and close the file every time a value
> > > > > > needs
> > > > > > to be read from it.
> > > > 
> > > > I'm bit unsure where to go with this. While I could change the code to
> > > > match what you described, I realized with the two files approach there's
> > > > no need to do even review/lseek() call during the measurement. It might
> > > > not be very significant compared with the open that was there initially
> > > > but it's still extra.
> > > 
> > > We are discussing the resctrl selftests that will accompany the resctrl
> > > filesystem in the kernel. When in doubt on how to interact with resctrl
> > > users
> > > use the selftests as reference. Needing to open and close a resctrl file
> > > every time a value needs to be read from it is not the correct guidance.
> > 
> > That's actually a different goal from the earlier, but I've no problem
> > adjusting to it.
> > 
> > Initially, this open/close() refactoring was made because of another goal
> > which was to avoid doing extra syscalls during the test.
> > 
> 
> It is not clear what you hint at here. Reading twice from an open file
> should not be a huge adjustment so it is not clear to me how this results
> in a big change to this work. As I understand this does match with original
> goal
> of reducing syscalls since the file need not be opened and closed twice.

What I tried to say is that with a single file, the test uses rewind() 
that also needs to do a syscall within the test period, whereas if the 
file is opened twice in advance rewind() won't be needed.

But I've converted it into single file for the sake of serving as an 
example for other resctrl users.

-- 
 i.

[PATCH v4 03/16] selftests/resctrl: Make "bandwidth" consistent in comments & prints

[Ilpo Järvinen]

Resctrl selftests refer to "bandwidth" currently in two other forms in
the code ("B/W" and "band width").

Use "bandwidth" consistently everywhere. While at it, fix also one
"over flow" -> "overflow" on a line that is touched by the change.

Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---

v3:
- New patch
---
 tools/testing/selftests/resctrl/resctrl_val.c | 14 +++++++-------
 tools/testing/selftests/resctrl/resctrlfs.c   |  2 +-
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index bd5a1cbb8a21..126a9cad588b 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -361,11 +361,11 @@ static void do_imc_mem_bw_test(void)
 }
 
 /*
- * get_mem_bw_imc - Memory band width as reported by iMC counters
+ * get_mem_bw_imc - Memory bandwidth 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.
+ * Memory bandwidth 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.
  */
@@ -379,7 +379,7 @@ static int get_mem_bw_imc(char *bw_report, float *bw_imc)
 
 	/*
 	 * Get results which are stored in struct type imc_counter_config
-	 * Take over flow into consideration before calculating total b/w
+	 * Take overflow into consideration before calculating total bandwidth.
 	 */
 	for (imc = 0; imc < imcs; imc++) {
 		struct imc_counter_config *r =
@@ -389,13 +389,13 @@ static int get_mem_bw_imc(char *bw_report, float *bw_imc)
 
 		if (read(r->fd, &r->return_value,
 			 sizeof(struct membw_read_format)) == -1) {
-			ksft_perror("Couldn't get read b/w through iMC");
+			ksft_perror("Couldn't get read bandwidth through iMC");
 			goto close_fds;
 		}
 
 		if (read(w->fd, &w->return_value,
 			 sizeof(struct membw_read_format)) == -1) {
-			ksft_perror("Couldn't get write bw through iMC");
+			ksft_perror("Couldn't get write bandwidth through iMC");
 			goto close_fds;
 		}
 
@@ -498,7 +498,7 @@ static FILE *open_mem_bw_resctrl(const char *mbm_bw_file)
 
 	fp = fopen(mbm_bw_file, "r");
 	if (!fp)
-		ksft_perror("Failed to open total bw file");
+		ksft_perror("Failed to open total memory bandwidth file");
 
 	return fp;
 }
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index 1cade75176eb..9b86f826a40c 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -850,7 +850,7 @@ int validate_bw_report_request(char *bw_report)
 	if (strcmp(bw_report, "total") == 0)
 		return 0;
 
-	fprintf(stderr, "Requested iMC B/W report type unavailable\n");
+	fprintf(stderr, "Requested iMC bandwidth report type unavailable\n");
 
 	return -1;
 }
-- 
2.39.2

Re: [PATCH v4 03/16] selftests/resctrl: Make "bandwidth" consistent in comments & prints

[Reinette Chatre]

Hi Ilpo,

On 5/20/24 5:30 AM, Ilpo Järvinen wrote:
> Resctrl selftests refer to "bandwidth" currently in two other forms in
> the code ("B/W" and "band width").
> 
> Use "bandwidth" consistently everywhere. While at it, fix also one
> "over flow" -> "overflow" on a line that is touched by the change.
> 
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---

Thank you.

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

Reinette

[PATCH v4 04/16] 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 126a9cad588b..6ea8fd2c65cc 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -456,19 +456,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);
 
@@ -627,19 +620,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);
 }
@@ -775,10 +761,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);
@@ -873,10 +866,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

Re: [PATCH v4 04/16] selftests/resctrl: Consolidate get_domain_id() into resctrl_val()

[Reinette Chatre]

Hi Ilpo,

On 5/20/24 5:30 AM, Ilpo Järvinen wrote:
> 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>
> ---

Thank you.

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

Reinette

[PATCH v4 05/16] 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 00d51fa7531c..e6f221236c79 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -174,8 +174,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 6ea8fd2c65cc..18bfdff78089 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -572,7 +572,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

Re: [PATCH v4 05/16] selftests/resctrl: Use correct type for pids

[Reinette Chatre]

Hi Ilpo,

On 5/20/24 5:30 AM, Ilpo Järvinen wrote:
> 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.
> 

This looks good to me. A couple of places remain that treat
pid_t as int ... that is via the %d format specifier
used to do the printing in print_results_cache() as well as
print_results_bw(). I understand we do not see any warnings
since these are indeed int. So, I wonder if an explicit (int) cast
would not make things obvious and match the stated goal of
not treating pid_t as int?

Reinette

[PATCH v4 06/16] 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 e6f221236c79..e4b6dc672ecc 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
@@ -127,8 +120,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 18bfdff78089..198b5133432a 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -505,7 +505,7 @@ static int get_mem_bw_resctrl(FILE *fp, 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)
 {
@@ -563,6 +563,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
@@ -631,7 +638,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;
 	FILE *mem_bw_fp, *mem_bw_fp2;
@@ -710,7 +717,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) {
@@ -724,7 +731,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))
@@ -738,7 +745,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);
 }
 
 /*
@@ -817,7 +824,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 */
@@ -835,7 +842,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);
@@ -905,7 +912,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

Re: [PATCH v4 06/16] selftests/resctrl: Cleanup bm_pid and ppid usage & limit scope

[Reinette Chatre]

Hi Ilpo,

On 5/20/24 5:30 AM, Ilpo Järvinen wrote:
> '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.

measure_val() -> measure_vals() (in two places above)

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

With typos fixed:

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

Reinette

[PATCH v4 07/16] 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 198b5133432a..6c53eb9171ea 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -637,8 +637,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;
 	FILE *mem_bw_fp, *mem_bw_fp2;
@@ -912,7 +918,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

Re: [PATCH v4 07/16] selftests/resctrl: Rename measure_vals() to measure_mem_bw_vals() & document

[Reinette Chatre]

Hi Ilpo,

Function name in shortlog does not match the patch.

On 5/20/24 5:30 AM, Ilpo Järvinen wrote:
> measure_val() is awfully generic name so rename it to measure_mem_bw()

measure_val() -> measure_vals()

> 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 198b5133432a..6c53eb9171ea 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -637,8 +637,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()

Please consistently start descriptions with upper case.

> + * @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;
>   	FILE *mem_bw_fp, *mem_bw_fp2;
> @@ -912,7 +918,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))) {

With typos fixed:

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

Reinette

[PATCH v4 08/16] selftests/resctrl: Simplify mem bandwidth file code for MBA & MBM tests

[Ilpo Järvinen]

initialize_mem_bw_resctrl() and set_mbm_path() contain complicated set
of conditions, each yielding different file to be opened to measure
memory bandwidth through resctrl FS. In practice, only two of them are
used. For MBA test, ctrlgrp is always provided, and for MBM test both
ctrlgrp and mongrp are set.

The file used differ between MBA/MBM test, however, MBM test
unnecessarily create monitor group because resctrl FS already provides
monitoring interface underneath any ctrlgrp too, which is what the MBA
selftest uses.

Consolidate memory bandwidth file used to the one used by the MBA
selftest. Remove all unused branches opening other files to simplify
the code.

Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---

v4:
- Remove also MBM .mongrp setup
v3:
- New patch
---
 tools/testing/selftests/resctrl/mbm_test.c    |  1 -
 tools/testing/selftests/resctrl/resctrl_val.c | 45 ++-----------------
 2 files changed, 4 insertions(+), 42 deletions(-)

diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index 3059ccc51a5a..96d279b06377 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -115,7 +115,6 @@ static int mbm_run_test(const struct resctrl_test *test, const struct user_param
 	struct resctrl_val_param param = {
 		.resctrl_val	= MBM_STR,
 		.ctrlgrp	= "c1",
-		.mongrp		= "m1",
 		.filename	= RESULT_FILE_NAME,
 		.bw_report	= "reads",
 		.setup		= mbm_setup
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 6c53eb9171ea..a3cf3c5ed17f 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -19,18 +19,10 @@
 #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"
 
@@ -436,43 +428,15 @@ static int get_mem_bw_imc(char *bw_report, float *bw_imc)
 	return -1;
 }
 
-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)
+static void initialize_mem_bw_resctrl(const char *ctrlgrp, int domain_id)
 {
-	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);
-	}
+	sprintf(mbm_total_path, CON_MBM_LOCAL_BYTES_PATH, RESCTRL_PATH,
+		ctrlgrp, domain_id);
 }
 
 /*
@@ -878,8 +842,7 @@ int resctrl_val(const struct resctrl_test *test,
 		if (ret)
 			goto out;
 
-		initialize_mem_bw_resctrl(param->ctrlgrp, param->mongrp,
-					  domain_id, resctrl_val);
+		initialize_mem_bw_resctrl(param->ctrlgrp, domain_id);
 	} else if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
 		initialize_llc_occu_resctrl(param->ctrlgrp, param->mongrp,
 					    domain_id, resctrl_val);
-- 
2.39.2

Re: [PATCH v4 08/16] selftests/resctrl: Simplify mem bandwidth file code for MBA & MBM tests

[Reinette Chatre]

Hi Ilpo,

On 5/20/24 5:30 AM, Ilpo Järvinen wrote:
> initialize_mem_bw_resctrl() and set_mbm_path() contain complicated set
> of conditions, each yielding different file to be opened to measure
> memory bandwidth through resctrl FS. In practice, only two of them are
> used. For MBA test, ctrlgrp is always provided, and for MBM test both
> ctrlgrp and mongrp are set.
> 
> The file used differ between MBA/MBM test, however, MBM test
> unnecessarily create monitor group because resctrl FS already provides
> monitoring interface underneath any ctrlgrp too, which is what the MBA
> selftest uses.
> 
> Consolidate memory bandwidth file used to the one used by the MBA
> selftest. Remove all unused branches opening other files to simplify
> the code.
> 
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---

fyi ... if the write_bm_pid_to_resctrl() fix in patch #16
was closer to this change it would have made this series easier to
review. That fix is clearly needed here and its omission makes this
patch as well as later patches #14 and #15 harder to review. No
need to rework the series at this point. It is just some comment on
how a simple patch ordering change can make a series easier to
understand.

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

Reinette

[PATCH v4 09/16] 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 a44e6fcd37b7..d8521386cd18 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 5d6af9e8afed..de6e29faf214 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 96d279b06377..59e26adf60bb 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);
+}
+
 static void mbm_test_cleanup(void)
 {
 	remove(RESULT_FILE_NAME);
@@ -117,7 +123,8 @@ static int mbm_run_test(const struct resctrl_test *test, const struct user_param
 		.ctrlgrp	= "c1",
 		.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 e4b6dc672ecc..5dc3def70669 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -87,6 +87,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;
@@ -99,6 +100,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 {
@@ -145,6 +149,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 a3cf3c5ed17f..76ae89e1aea3 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -607,8 +607,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;
 	FILE *mem_bw_fp, *mem_bw_fp2;
@@ -879,17 +879,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

Re: [PATCH v4 09/16] selftests/resctrl: Add ->measure() callback to resctrl_val_param

[Reinette Chatre]

Hi Ilpo,

On 5/20/24 5:30 AM, Ilpo Järvinen wrote:
> 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>
> ---

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

Reinette

[PATCH v4 10/16] 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
for CMT test.

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

v3:
- Rename init functions to <testname>_init()
- Removed tabs intermixed with code
- Leave now common mbm bw filename setup into resctrl_val.c
---
 tools/testing/selftests/resctrl/cmt_test.c    | 12 +++
 tools/testing/selftests/resctrl/mba_test.c    | 14 ++++
 tools/testing/selftests/resctrl/mbm_test.c    | 14 ++++
 tools/testing/selftests/resctrl/resctrl.h     |  8 +-
 tools/testing/selftests/resctrl/resctrl_val.c | 75 ++++---------------
 5 files changed, 60 insertions(+), 63 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index d8521386cd18..238f514ba7e6 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 cmt_init(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		= cmt_init,
 		.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 de6e29faf214..0a95c42f1616 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -17,6 +17,19 @@
 #define ALLOCATION_MIN		10
 #define ALLOCATION_STEP		10
 
+static int mba_init(const struct resctrl_val_param *param, int domain_id)
+{
+	int ret;
+
+	ret = initialize_mem_bw_imc();
+	if (ret)
+		return ret;
+
+	initialize_mem_bw_resctrl(param, domain_id);
+
+	return 0;
+}
+
 /*
  * Change schemata percentage from 100 to 10%. Write schemata to specified
  * con_mon grp, mon_grp in resctrl FS.
@@ -156,6 +169,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		= mba_init,
 		.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 59e26adf60bb..00e60b01188a 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -86,6 +86,19 @@ static int check_results(size_t span)
 	return ret;
 }
 
+static int mbm_init(const struct resctrl_val_param *param, int domain_id)
+{
+	int ret;
+
+	ret = initialize_mem_bw_imc();
+	if (ret)
+		return ret;
+
+	initialize_mem_bw_resctrl(param, domain_id);
+
+	return 0;
+}
+
 static int mbm_setup(const struct resctrl_test *test,
 		     const struct user_params *uparams,
 		     struct resctrl_val_param *p)
@@ -123,6 +136,7 @@ static int mbm_run_test(const struct resctrl_test *test, const struct user_param
 		.ctrlgrp	= "c1",
 		.filename	= RESULT_FILE_NAME,
 		.bw_report	= "reads",
+		.init		= mbm_init,
 		.setup		= mbm_setup,
 		.measure	= mbm_measure,
 	};
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index 5dc3def70669..d3fbb957309d 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -86,7 +86,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 {
@@ -97,6 +98,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);
@@ -149,8 +152,11 @@ 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);
+void initialize_mem_bw_resctrl(const struct resctrl_val_param *param,
+			       int domain_id);
 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 76ae89e1aea3..23a82bd547a3 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -23,18 +23,6 @@
 #define CON_MBM_LOCAL_BYTES_PATH		\
 	"%s/%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 */
 	__u64 time_enabled;  /* if PERF_FORMAT_TOTAL_TIME_ENABLED */
@@ -268,7 +256,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;
 
@@ -430,24 +418,18 @@ static int get_mem_bw_imc(char *bw_report, float *bw_imc)
 
 /*
  * initialize_mem_bw_resctrl:	Appropriately populate "mbm_total_path"
- * @ctrlgrp:			Name of the control monitor group (con_mon grp)
- * @domain_id:			Domain ID (cache ID; for MB, L3 cache ID)
+ * @param:	parameters passed to resctrl_val()
+ * @domain_id:	Domain ID (cache ID; for MB, L3 cache ID)
  */
-static void initialize_mem_bw_resctrl(const char *ctrlgrp, int domain_id)
+void initialize_mem_bw_resctrl(const struct resctrl_val_param *param,
+			       int domain_id)
 {
 	sprintf(mbm_total_path, CON_MBM_LOCAL_BYTES_PATH, RESCTRL_PATH,
-		ctrlgrp, domain_id);
+		param->ctrlgrp, 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
+ * Open file to read MBM local bytes from resctrl FS
  */
 static FILE *open_mem_bw_resctrl(const char *mbm_bw_file)
 {
@@ -460,6 +442,9 @@ static FILE *open_mem_bw_resctrl(const char *mbm_bw_file)
 	return fp;
 }
 
+/*
+ * Get MBM Local bytes as reported by resctrl FS
+ */
 static int get_mem_bw_resctrl(FILE *fp, unsigned long *mbm_total)
 {
 	if (fscanf(fp, "%lu", mbm_total) <= 0) {
@@ -572,35 +557,6 @@ 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
@@ -836,16 +792,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, domain_id);
-	} 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

Re: [PATCH v4 10/16] selftests/resctrl: Add ->init() callback into resctrl_val_param

[Reinette Chatre]

Hi Ilpo,

On 5/20/24 5:30 AM, Ilpo Järvinen wrote:

...

> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index 5dc3def70669..d3fbb957309d 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -86,7 +86,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

hmmm ... ok ... this unrelated change could sneak in with the stretch argument of
making the text consistent.

>    * @measure:		Callback that performs the measurement (a single test)
>    */
>   struct resctrl_val_param {
> @@ -97,6 +98,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);
> @@ -149,8 +152,11 @@ 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);
> +void initialize_mem_bw_resctrl(const struct resctrl_val_param *param,
> +			       int domain_id);
>   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 76ae89e1aea3..23a82bd547a3 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -23,18 +23,6 @@
>   #define CON_MBM_LOCAL_BYTES_PATH		\
>   	"%s/%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 */
>   	__u64 time_enabled;  /* if PERF_FORMAT_TOTAL_TIME_ENABLED */
> @@ -268,7 +256,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;
>   
> @@ -430,24 +418,18 @@ static int get_mem_bw_imc(char *bw_report, float *bw_imc)
>   
>   /*
>    * initialize_mem_bw_resctrl:	Appropriately populate "mbm_total_path"
> - * @ctrlgrp:			Name of the control monitor group (con_mon grp)
> - * @domain_id:			Domain ID (cache ID; for MB, L3 cache ID)
> + * @param:	parameters passed to resctrl_val()

Please let description consistently start with upper case.

> + * @domain_id:	Domain ID (cache ID; for MB, L3 cache ID)
>    */
> -static void initialize_mem_bw_resctrl(const char *ctrlgrp, int domain_id)
> +void initialize_mem_bw_resctrl(const struct resctrl_val_param *param,
> +			       int domain_id)
>   {
>   	sprintf(mbm_total_path, CON_MBM_LOCAL_BYTES_PATH, RESCTRL_PATH,
> -		ctrlgrp, domain_id);
> +		param->ctrlgrp, domain_id);
>   }

With typo fixed:

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

Reinette

[PATCH v4 11/16] 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 0a95c42f1616..9c9a4f22e529 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -67,7 +67,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, "reads");
 }
 
 static bool show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc)
@@ -168,7 +168,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		= mba_init,
 		.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 00e60b01188a..27b936fe60bc 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -121,7 +121,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, "reads");
 }
 
 static void mbm_test_cleanup(void)
@@ -135,7 +135,6 @@ static int mbm_run_test(const struct resctrl_test *test, const struct user_param
 		.resctrl_val	= MBM_STR,
 		.ctrlgrp	= "c1",
 		.filename	= RESULT_FILE_NAME,
-		.bw_report	= "reads",
 		.init		= mbm_init,
 		.setup		= mbm_setup,
 		.measure	= mbm_measure,
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index d3fbb957309d..4446a0e493ef 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -85,7 +85,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)
@@ -95,7 +94,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,
@@ -135,7 +133,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);
@@ -154,7 +152,8 @@ 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 *bw_report);
 void initialize_mem_bw_resctrl(const struct resctrl_val_param *param,
 			       int domain_id);
 int resctrl_val(const struct resctrl_test *test,
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 23a82bd547a3..b2b944a70fb9 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -349,7 +349,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;
@@ -562,15 +562,21 @@ static int print_results_bw(char *filename, pid_t bm_pid, float bw_imc,
  * @uparams:		User supplied parameters
  * @param:		parameters passed to resctrl_val()
  * @bm_pid:		PID that runs the benchmark
+ * @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)
+		   struct resctrl_val_param *param, pid_t bm_pid,
+		   const char *bw_report)
 {
 	unsigned long bw_resc, bw_resc_start, bw_resc_end;
 	FILE *mem_bw_fp, *mem_bw_fp2;
 	float bw_imc;
 	int ret;
 
+	bw_report = get_bw_report_type(bw_report);
+	if (!bw_report)
+		return -1;
+
 	mem_bw_fp = open_mem_bw_resctrl(mbm_total_path);
 	if (!mem_bw_fp)
 		return -1;
@@ -601,7 +607,7 @@ int measure_mem_bw(const struct user_params *uparams,
 	if (ret < 0)
 		goto close_fp2;
 
-	ret = get_mem_bw_imc(param->bw_report, &bw_imc);
+	ret = get_mem_bw_imc(bw_report, &bw_imc);
 	if (ret < 0)
 		goto close_fp2;
 
@@ -705,13 +711,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 9b86f826a40c..aac382eaa032 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 bandwidth 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

Re: [PATCH v4 11/16] selftests/resctrl: Simplify bandwidth report type handling

[Reinette Chatre]

Hi Ilpo,

On 5/20/24 5:30 AM, Ilpo Järvinen wrote:
> 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.

Needs imperative tone

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

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

Reinette

[PATCH v4 12/16] 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 4446a0e493ef..5967389038d4 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -141,9 +141,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 aac382eaa032..a0e84157eb63 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

Re: [PATCH v4 12/16] selftests/resctrl: Make some strings passed to resctrlfs functions const

[Reinette Chatre]

Hi Ilpo,

On 5/20/24 5:30 AM, Ilpo Järvinen wrote:
> 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>
> ---

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

Reinette

[PATCH v4 13/16] 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.

Convert !grp_name check in create_grp() into internal sanity check by
returning error if the caller asked to create a group but doesn't
provide a name for the group. The existing code already abides this by
only calling create_grp() if mongrp is non-NULL so the error should
never be returned with the current selftests (ctrlgrp is never NULL).

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

v3:
- Removed wrong comment
- Changed grp_name check to return -1 on fail (internal sanity check)
---
 tools/testing/selftests/resctrl/resctrl.h   |  4 ++--
 tools/testing/selftests/resctrl/resctrlfs.c | 15 +++++----------
 2 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index 5967389038d4..a999fbc13fd3 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -91,8 +91,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 a0e84157eb63..6b4448588666 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -464,13 +464,8 @@ static int create_grp(const char *grp_name, char *grp, const char *parent_grp)
 	struct dirent *ep;
 	DIR *dp;
 
-	/*
-	 * At this point, we are guaranteed to have resctrl FS mounted and if
-	 * length of grp_name == 0, it means, user wants to use root con_mon
-	 * grp, so do nothing
-	 */
-	if (strlen(grp_name) == 0)
-		return 0;
+	if (!grp_name)
+		return -1;
 
 	/* Check if requested grp exists or not */
 	dp = opendir(parent_grp);
@@ -541,7 +536,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 +553,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 +607,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

Re: [PATCH v4 13/16] selftests/resctrl: Convert ctrlgrp & mongrp to pointers

[Reinette Chatre]

Hi Ilpo,

On 5/20/24 5:30 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.
> 
> Convert !grp_name check in create_grp() into internal sanity check by
> returning error if the caller asked to create a group but doesn't
> provide a name for the group. The existing code already abides this by
> only calling create_grp() if mongrp is non-NULL so the error should
> never be returned with the current selftests (ctrlgrp is never NULL).
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> 
> v3:
> - Removed wrong comment
> - Changed grp_name check to return -1 on fail (internal sanity check)
> ---
>   tools/testing/selftests/resctrl/resctrl.h   |  4 ++--
>   tools/testing/selftests/resctrl/resctrlfs.c | 15 +++++----------
>   2 files changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index 5967389038d4..a999fbc13fd3 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -91,8 +91,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 a0e84157eb63..6b4448588666 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -464,13 +464,8 @@ static int create_grp(const char *grp_name, char *grp, const char *parent_grp)
>   	struct dirent *ep;
>   	DIR *dp;
>   
> -	/*
> -	 * At this point, we are guaranteed to have resctrl FS mounted and if
> -	 * length of grp_name == 0, it means, user wants to use root con_mon
> -	 * grp, so do nothing
> -	 */
> -	if (strlen(grp_name) == 0)
> -		return 0;
> +	if (!grp_name)
> +		return -1;

As I said during review of v2, this should not be an error. I went back to read
your comments and the argument that this is done for benefit of API is unclear since
the default control group does not have a name, exactly what create_grp() supports
when not returning an error in this case.

Reinette

Re: [PATCH v4 13/16] selftests/resctrl: Convert ctrlgrp & mongrp to pointers

[Ilpo Järvinen]

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

On Wed, 29 May 2024, Reinette Chatre wrote:

> Hi Ilpo,
> 
> On 5/20/24 5:30 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.
> > 
> > Convert !grp_name check in create_grp() into internal sanity check by
> > returning error if the caller asked to create a group but doesn't
> > provide a name for the group. The existing code already abides this by
> > only calling create_grp() if mongrp is non-NULL so the error should
> > never be returned with the current selftests (ctrlgrp is never NULL).
> > 
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> > 
> > v3:
> > - Removed wrong comment
> > - Changed grp_name check to return -1 on fail (internal sanity check)
> > ---
> >   tools/testing/selftests/resctrl/resctrl.h   |  4 ++--
> >   tools/testing/selftests/resctrl/resctrlfs.c | 15 +++++----------
> >   2 files changed, 7 insertions(+), 12 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/resctrl/resctrl.h
> > b/tools/testing/selftests/resctrl/resctrl.h
> > index 5967389038d4..a999fbc13fd3 100644
> > --- a/tools/testing/selftests/resctrl/resctrl.h
> > +++ b/tools/testing/selftests/resctrl/resctrl.h
> > @@ -91,8 +91,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 a0e84157eb63..6b4448588666 100644
> > --- a/tools/testing/selftests/resctrl/resctrlfs.c
> > +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> > @@ -464,13 +464,8 @@ static int create_grp(const char *grp_name, char *grp,
> > const char *parent_grp)
> >   	struct dirent *ep;
> >   	DIR *dp;
> >   -	/*
> > -	 * At this point, we are guaranteed to have resctrl FS mounted and if
> > -	 * length of grp_name == 0, it means, user wants to use root con_mon
> > -	 * grp, so do nothing
> > -	 */
> > -	if (strlen(grp_name) == 0)
> > -		return 0;
> > +	if (!grp_name)
> > +		return -1;
> 
> As I said during review of v2, this should not be an error. I went back to
> read
> your comments and the argument that this is done for benefit of API is unclear
> since
> the default control group does not have a name, exactly what create_grp()
> supports
> when not returning an error in this case.

Okay, I did not know about this expectation related to the default control 
group because it's not used anywhere in the selftest code that always 
provides a ctrlgrp name.

I'll change it to return 0.

-- 
 i.

[PATCH v4 14/16] 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 9c9a4f22e529..5e0b1e794295 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -166,7 +166,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		= mba_init,
 		.setup		= mba_setup,
-- 
2.39.2

Re: [PATCH v4 14/16] selftests/resctrl: Remove mongrp from MBA test

[Reinette Chatre]

Hi Ilpo,

On 5/20/24 5:30 AM, Ilpo Järvinen wrote:
> 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 9c9a4f22e529..5e0b1e794295 100644
> --- a/tools/testing/selftests/resctrl/mba_test.c
> +++ b/tools/testing/selftests/resctrl/mba_test.c
> @@ -166,7 +166,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		= mba_init,
>   		.setup		= mba_setup,

This may explain the unexpected checks that are removed in final patch?

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

Reinette

Re: [PATCH v4 14/16] selftests/resctrl: Remove mongrp from MBA test

[Ilpo Järvinen]

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

On Wed, 29 May 2024, Reinette Chatre wrote:
> On 5/20/24 5:30 AM, Ilpo Järvinen wrote:
> > 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 9c9a4f22e529..5e0b1e794295 100644
> > --- a/tools/testing/selftests/resctrl/mba_test.c
> > +++ b/tools/testing/selftests/resctrl/mba_test.c
> > @@ -166,7 +166,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		= mba_init,
> >   		.setup		= mba_setup,
> 
> This may explain the unexpected checks that are removed in final patch?

While possible, I just have gotten a feeling that not much thought has 
been put on generality until now. Because of that, the solution had always 
been adding new ifs, no matter the place, instead of thinking how to 
parametrize things properly instead. It has lead to fully overlapping 
checks, dead code, and incomplete error handling which is hopefully now 
slowly getting less and less.

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

-- 
 i.

Re: [PATCH v4 14/16] selftests/resctrl: Remove mongrp from MBA test

[Reinette Chatre]

Hi Ilpo,

On 5/30/24 4:56 AM, Ilpo Järvinen wrote:
> On Wed, 29 May 2024, Reinette Chatre wrote:
>> On 5/20/24 5:30 AM, Ilpo Järvinen wrote:
>>> 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 9c9a4f22e529..5e0b1e794295 100644
>>> --- a/tools/testing/selftests/resctrl/mba_test.c
>>> +++ b/tools/testing/selftests/resctrl/mba_test.c
>>> @@ -166,7 +166,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		= mba_init,
>>>    		.setup		= mba_setup,
>>
>> This may explain the unexpected checks that are removed in final patch?
> 
> While possible, I just have gotten a feeling that not much thought has
> been put on generality until now. Because of that, the solution had always
> been adding new ifs, no matter the place, instead of thinking how to
> parametrize things properly instead. It has lead to fully overlapping
> checks, dead code, and incomplete error handling which is hopefully now
> slowly getting less and less.
  

Ack. Your work is greatly appreciated.

Reinette

[PATCH v4 15/16] selftests/resctrl: Remove mongrp from CMT test

[Ilpo Järvinen]

The CMT selftest instanciates a monitor group to read LLC occupancy.
Since the test also creates a control group, it is unnecessary to
create another one for monitoring because control groups already
provide monitoring too.

Remove the unnecessary monitor group from the CMT selftest.

Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
--

v4:
- New patch
---
 tools/testing/selftests/resctrl/cmt_test.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index 238f514ba7e6..b63fa1e93307 100644
--- a/tools/testing/selftests/resctrl/cmt_test.c
+++ b/tools/testing/selftests/resctrl/cmt_test.c
@@ -17,12 +17,12 @@
 #define MAX_DIFF_PERCENT	15
 
 #define CON_MON_LCC_OCCUP_PATH		\
-	"%s/%s/mon_groups/%s/mon_data/mon_L3_%02d/llc_occupancy"
+	"%s/%s/mon_data/mon_L3_%02d/llc_occupancy"
 
 static int cmt_init(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);
+		param->ctrlgrp, domain_id);
 
 	return 0;
 }
@@ -146,7 +146,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,
 		.mask		= ~(long_mask << n) & long_mask,
 		.num_of_runs	= 0,
-- 
2.39.2

Re: [PATCH v4 15/16] selftests/resctrl: Remove mongrp from CMT test

[Reinette Chatre]

Hi Ilpo,

On 5/20/24 5:30 AM, Ilpo Järvinen wrote:
> The CMT selftest instanciates a monitor group to read LLC occupancy.

instanciates -> instantiates

> Since the test also creates a control group, it is unnecessary to
> create another one for monitoring because control groups already
> provide monitoring too.
> 
> Remove the unnecessary monitor group from the CMT selftest.
> 
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> --

With typo fixed:

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

Reinette

[PATCH v4 16/16] 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 c7686fb6641a..d4dffc934bc3 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 b63fa1e93307..d1c272743eb2 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",
 		.filename	= RESULT_FILE_NAME,
 		.mask		= ~(long_mask << n) & long_mask,
diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
index 5e0b1e794295..1f2a7dc73b62 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -164,7 +164,6 @@ static 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		= mba_init,
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index 27b936fe60bc..39aa70374154 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -132,7 +132,6 @@ static 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",
 		.filename	= RESULT_FILE_NAME,
 		.init		= mbm_init,
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index a999fbc13fd3..2dda56084588 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -81,7 +81,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
@@ -90,7 +89,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];
@@ -113,11 +111,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
@@ -143,8 +136,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 b2b944a70fb9..719a08ece9d3 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -695,7 +695,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;
@@ -786,8 +785,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 6b4448588666..4bf1fe6dc308 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -519,7 +519,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.
@@ -529,8 +528,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];
@@ -550,22 +548,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 v4 16/16] selftests/resctrl: Remove test name comparing from write_bm_pid_to_resctrl()

[Reinette Chatre]

Hi Ilpo,

On 5/20/24 5:30 AM, Ilpo Järvinen wrote:
> 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.

Second paragraph needs to be reworked because at this point
"Only MBM and CMT tests define mongrp" is not accurate.
Perhaps just (feel free to improve):
	Tests define mongrp when needed. Remove the test name check in
	write_bm_pid_to_resctrl() to only rely 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.

Imperative tone needed.


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

Patch looks good. Thank you.

Reinette