Hi all,
This series does a number of cleanups into resctrl_val() and
generalizes it by removing test name specific handling from the
function.
One of the changes improves MBA/MBM measurement by narrowing down the
period the resctrl FS derived memory bandwidth numbers are measured
over. My feel is it didn't cause noticeable difference into the numbers
because they're generally good anyway except for the small number of
outliers. To see the impact on outliers, I'd need to setup a test to
run large number of replications and do a statistical analysis, which
I've not spent my time on. Even without the statistical analysis, the
new way to measure seems obviously better and makes sense even if I
cannot see a major improvement with the setup I'm using.
This series has some conflicts with SNC series from Maciej and also
with the MBA/MBM series from Babu.
--
i.
v2:
- Resolved conflicts with kselftest/next
- Spaces -> tabs correction
Ilpo Järvinen (13):
selftests/resctrl: Convert get_mem_bw_imc() fd close to for loop
selftests/resctrl: Calculate resctrl FS derived mem bw over sleep(1)
only
selftests/resctrl: Consolidate get_domain_id() into resctrl_val()
selftests/resctrl: Use correct type for pids
selftests/resctrl: Cleanup bm_pid and ppid usage & limit scope
selftests/resctrl: Rename measure_vals() to measure_mem_bw_vals() &
document
selftests/resctrl: Add ->measure() callback to resctrl_val_param
selftests/resctrl: Add ->init() callback into resctrl_val_param
selftests/resctrl: Simplify bandwidth report type handling
selftests/resctrl: Make some strings passed to resctrlfs functions
const
selftests/resctrl: Convert ctrlgrp & mongrp to pointers
selftests/resctrl: Remove mongrp from MBA test
selftests/resctrl: Remove test name comparing from
write_bm_pid_to_resctrl()
tools/testing/selftests/resctrl/cache.c | 6 +-
tools/testing/selftests/resctrl/cat_test.c | 5 +-
tools/testing/selftests/resctrl/cmt_test.c | 21 +-
tools/testing/selftests/resctrl/mba_test.c | 34 ++-
tools/testing/selftests/resctrl/mbm_test.c | 33 ++-
tools/testing/selftests/resctrl/resctrl.h | 48 ++--
tools/testing/selftests/resctrl/resctrl_val.c | 269 ++++++------------
tools/testing/selftests/resctrl/resctrlfs.c | 55 ++--
8 files changed, 224 insertions(+), 247 deletions(-)
--
2.39.2
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 <[email protected]>
---
tools/testing/selftests/resctrl/cache.c | 6 +++---
tools/testing/selftests/resctrl/resctrl.h | 4 ++--
tools/testing/selftests/resctrl/resctrl_val.c | 2 +-
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
index 1b339d6bbff1..9b74fce80037 100644
--- a/tools/testing/selftests/resctrl/cache.c
+++ b/tools/testing/selftests/resctrl/cache.c
@@ -101,7 +101,7 @@ static int get_llc_occu_resctrl(unsigned long *llc_occupancy)
*
* Return: 0 on success, < 0 on error.
*/
-static int print_results_cache(const char *filename, int bm_pid, __u64 llc_value)
+static int print_results_cache(const char *filename, pid_t bm_pid, __u64 llc_value)
{
FILE *fp;
@@ -133,7 +133,7 @@ static int print_results_cache(const char *filename, int bm_pid, __u64 llc_value
* Return: =0 on success. <0 on failure.
*/
int perf_event_measure(int pe_fd, struct perf_event_read *pe_read,
- const char *filename, int bm_pid)
+ const char *filename, pid_t bm_pid)
{
int ret;
@@ -161,7 +161,7 @@ int perf_event_measure(int pe_fd, struct perf_event_read *pe_read,
*
* Return: =0 on success. <0 on failure.
*/
-int measure_llc_resctrl(const char *filename, int bm_pid)
+int measure_llc_resctrl(const char *filename, pid_t bm_pid)
{
unsigned long llc_occu_resc = 0;
int ret;
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index 2051bd135e0d..f810a3c5692c 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -177,8 +177,8 @@ void perf_event_initialize_read_format(struct perf_event_read *pe_read);
int perf_open(struct perf_event_attr *pea, pid_t pid, int cpu_no);
int perf_event_reset_enable(int pe_fd);
int perf_event_measure(int pe_fd, struct perf_event_read *pe_read,
- const char *filename, int bm_pid);
-int measure_llc_resctrl(const char *filename, int bm_pid);
+ const char *filename, pid_t bm_pid);
+int measure_llc_resctrl(const char *filename, pid_t bm_pid);
void show_cache_info(int no_of_bits, __u64 avg_llc_val, size_t cache_span, bool lines);
/*
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 7981589f4db0..07fd57d8d125 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -545,7 +545,7 @@ void signal_handler_unregister(void)
*
* Return: 0 on success, < 0 on error.
*/
-static int print_results_bw(char *filename, int bm_pid, float bw_imc,
+static int print_results_bw(char *filename, pid_t bm_pid, float bw_imc,
unsigned long bw_resc)
{
unsigned long diff = fabs(bw_imc - bw_resc);
--
2.39.2
For MBM/MBA tests, measure_vals() calls get_mem_bw_imc() that performs
the measurement over a duration of sleep(1) call. The memory bandwidth
numbers from IMC are derived over this duration. The resctrl FS derived
memory bandwidth, however, is calculated inside measure_vals() and only
takes delta between the previous value and the current one which
besides the actual test, also samples inter-test noise.
Rework the logic in measure_vals() and get_mem_bw_imc() such that the
resctrl FS memory bandwidth section covers much shorter duration
closely matching that of the IMC perf counters to improve measurement
accuracy.
Suggested-by: Reinette Chatre <[email protected]>
Signed-off-by: Ilpo Järvinen <[email protected]>
---
tools/testing/selftests/resctrl/resctrl_val.c | 72 +++++++++++++------
1 file changed, 50 insertions(+), 22 deletions(-)
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 36139cba7be8..4df2cd738f88 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -293,28 +293,35 @@ static int initialize_mem_bw_imc(void)
}
/*
- * get_mem_bw_imc: Memory band width as reported by iMC counters
+ * perf_open_imc_mem_bw - Open perf fds for IMCs
* @cpu_no: CPU number that the benchmark PID is binded to
- * @bw_report: Bandwidth report type (reads, writes)
- *
- * Memory B/W utilized by a process on a socket can be calculated using
- * iMC counters. Perf events are used to read these counters.
- *
- * Return: = 0 on success. < 0 on failure.
*/
-static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
+static int perf_open_imc_mem_bw(int cpu_no)
{
- float reads, writes, of_mul_read, of_mul_write;
int imc, j, ret;
- /* Start all iMC counters to log values (both read and write) */
- reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
for (imc = 0; imc < imcs; imc++) {
for (j = 0; j < 2; j++) {
ret = open_perf_event(imc, cpu_no, j);
if (ret)
return -1;
}
+ }
+
+ return 0;
+}
+
+/*
+ * do_mem_bw_test - Perform memory bandwidth test
+ *
+ * Runs memory bandwidth test over one second period. Also, handles starting
+ * and stopping of the IMC perf counters around the test.
+ */
+static void do_imc_mem_bw_test(void)
+{
+ int imc, j;
+
+ for (imc = 0; imc < imcs; imc++) {
for (j = 0; j < 2; j++)
membw_ioctl_perf_event_ioc_reset_enable(imc, j);
}
@@ -326,6 +333,24 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
for (j = 0; j < 2; j++)
membw_ioctl_perf_event_ioc_disable(imc, j);
}
+}
+
+/*
+ * get_mem_bw_imc - Memory band width as reported by iMC counters
+ * @bw_report: Bandwidth report type (reads, writes)
+ *
+ * Memory B/W utilized by a process on a socket can be calculated using
+ * iMC counters. Perf events are used to read these counters.
+ *
+ * Return: = 0 on success. < 0 on failure.
+ */
+static int get_mem_bw_imc(char *bw_report, float *bw_imc)
+{
+ float reads, writes, of_mul_read, of_mul_write;
+ int imc, j;
+
+ /* Start all iMC counters to log values (both read and write) */
+ reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
/*
* Get results which are stored in struct type imc_counter_config
@@ -593,10 +618,9 @@ static void initialize_llc_occu_resctrl(const char *ctrlgrp, const char *mongrp,
}
static int measure_vals(const struct user_params *uparams,
- struct resctrl_val_param *param,
- unsigned long *bw_resc_start)
+ struct resctrl_val_param *param)
{
- unsigned long bw_resc, bw_resc_end;
+ unsigned long bw_resc, bw_resc_start, bw_resc_end;
float bw_imc;
int ret;
@@ -607,22 +631,27 @@ static int measure_vals(const struct user_params *uparams,
* Compare the two values to validate resctrl value.
* It takes 1sec to measure the data.
*/
- ret = get_mem_bw_imc(uparams->cpu, param->bw_report, &bw_imc);
+ ret = perf_open_imc_mem_bw(uparams->cpu);
if (ret < 0)
return ret;
+ ret = get_mem_bw_resctrl(&bw_resc_start);
+ if (ret < 0)
+ return ret;
+
+ do_imc_mem_bw_test();
+
ret = get_mem_bw_resctrl(&bw_resc_end);
if (ret < 0)
return ret;
- bw_resc = (bw_resc_end - *bw_resc_start) / MB;
- ret = print_results_bw(param->filename, bm_pid, bw_imc, bw_resc);
- if (ret)
+ ret = get_mem_bw_imc(param->bw_report, &bw_imc);
+ if (ret < 0)
return ret;
- *bw_resc_start = bw_resc_end;
+ bw_resc = (bw_resc_end - bw_resc_start) / MB;
- return 0;
+ return print_results_bw(param->filename, bm_pid, bw_imc, bw_resc);
}
/*
@@ -696,7 +725,6 @@ int resctrl_val(const struct resctrl_test *test,
struct resctrl_val_param *param)
{
char *resctrl_val = param->resctrl_val;
- unsigned long bw_resc_start = 0;
struct sigaction sigact;
int ret = 0, pipefd[2];
char pipe_message = 0;
@@ -838,7 +866,7 @@ int resctrl_val(const struct resctrl_test *test,
if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
- ret = measure_vals(uparams, param, &bw_resc_start);
+ ret = measure_vals(uparams, param);
if (ret)
break;
} else if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) {
--
2.39.2
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 <[email protected]>
---
tools/testing/selftests/resctrl/resctrl_val.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 04a8577b5e0a..80e5174df828 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -610,8 +610,14 @@ static void initialize_llc_occu_resctrl(const char *ctrlgrp, const char *mongrp,
set_cmt_path(ctrlgrp, mongrp, domain_id);
}
-static int measure_vals(const struct user_params *uparams,
- struct resctrl_val_param *param, pid_t bm_pid)
+/*
+ * measure_mem_bw - Measures memory bandwidth numbers while benchmark runs
+ * @uparams: User supplied parameters
+ * @param: parameters passed to resctrl_val()
+ * @bm_pid: PID that runs the benchmark
+ */
+static int measure_mem_bw(const struct user_params *uparams,
+ struct resctrl_val_param *param, pid_t bm_pid)
{
unsigned long bw_resc, bw_resc_start, bw_resc_end;
float bw_imc;
@@ -866,7 +872,7 @@ int resctrl_val(const struct resctrl_test *test,
if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
- ret = measure_vals(uparams, param, bm_pid);
+ ret = measure_mem_bw(uparams, param, bm_pid);
if (ret)
break;
} else if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) {
--
2.39.2
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 <[email protected]>
---
tools/testing/selftests/resctrl/mba_test.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
index 89ad7f2cdd65..5bb73e6cabc3 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -174,7 +174,6 @@ static int mba_run_test(const struct resctrl_test *test, const struct user_param
struct resctrl_val_param param = {
.resctrl_val = MBA_STR,
.ctrlgrp = "c1",
- .mongrp = "m1",
.filename = RESULT_FILE_NAME,
.init = set_mba_path,
.setup = mba_setup,
--
2.39.2
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 <[email protected]>
---
tools/testing/selftests/resctrl/cat_test.c | 5 +--
tools/testing/selftests/resctrl/cmt_test.c | 1 -
tools/testing/selftests/resctrl/mba_test.c | 1 -
tools/testing/selftests/resctrl/mbm_test.c | 1 -
tools/testing/selftests/resctrl/resctrl.h | 10 +-----
tools/testing/selftests/resctrl/resctrl_val.c | 4 +--
tools/testing/selftests/resctrl/resctrlfs.c | 33 ++++++++-----------
7 files changed, 17 insertions(+), 38 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
index 4cb991be8e31..c0291ecf1d1c 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -158,7 +158,6 @@ static int cat_test(const struct resctrl_test *test,
struct resctrl_val_param *param,
size_t span, unsigned long current_mask)
{
- char *resctrl_val = param->resctrl_val;
struct perf_event_read pe_read;
struct perf_event_attr pea;
cpu_set_t old_affinity;
@@ -178,8 +177,7 @@ static int cat_test(const struct resctrl_test *test,
return ret;
/* Write benchmark to specified con_mon grp, mon_grp in resctrl FS*/
- ret = write_bm_pid_to_resctrl(bm_pid, param->ctrlgrp, param->mongrp,
- resctrl_val);
+ ret = write_bm_pid_to_resctrl(bm_pid, param->ctrlgrp, param->mongrp);
if (ret)
goto reset_affinity;
@@ -272,7 +270,6 @@ static int cat_run_test(const struct resctrl_test *test, const struct user_param
start_mask = create_bit_mask(start, n);
struct resctrl_val_param param = {
- .resctrl_val = CAT_STR,
.ctrlgrp = "c1",
.filename = RESULT_FILE_NAME,
.num_of_runs = 0,
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index e79eca9346f3..ab96411d1015 100644
--- a/tools/testing/selftests/resctrl/cmt_test.c
+++ b/tools/testing/selftests/resctrl/cmt_test.c
@@ -144,7 +144,6 @@ static int cmt_run_test(const struct resctrl_test *test, const struct user_param
}
struct resctrl_val_param param = {
- .resctrl_val = CMT_STR,
.ctrlgrp = "c1",
.mongrp = "m1",
.filename = RESULT_FILE_NAME,
diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
index 5bb73e6cabc3..c44af1f62f55 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -172,7 +172,6 @@ void mba_test_cleanup(void)
static int mba_run_test(const struct resctrl_test *test, const struct user_params *uparams)
{
struct resctrl_val_param param = {
- .resctrl_val = MBA_STR,
.ctrlgrp = "c1",
.filename = RESULT_FILE_NAME,
.init = set_mba_path,
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index c8c9aba81db8..120c356a069e 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -140,7 +140,6 @@ void mbm_test_cleanup(void)
static int mbm_run_test(const struct resctrl_test *test, const struct user_params *uparams)
{
struct resctrl_val_param param = {
- .resctrl_val = MBM_STR,
.ctrlgrp = "c1",
.mongrp = "m1",
.filename = RESULT_FILE_NAME,
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index 54e5bce4c698..4cd7997b39e6 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -79,7 +79,6 @@ struct resctrl_test {
/*
* resctrl_val_param: resctrl test parameters
- * @resctrl_val: Resctrl feature (Eg: mbm, mba.. etc)
* @ctrlgrp: Name of the control monitor group (con_mon grp)
* @mongrp: Name of the monitor group (mon grp)
* @filename: Name of file to which the o/p should be written
@@ -88,7 +87,6 @@ struct resctrl_test {
* @measure: Callback that performs the measurement (a single test)
*/
struct resctrl_val_param {
- char *resctrl_val;
const char *ctrlgrp;
const char *mongrp;
char filename[64];
@@ -111,11 +109,6 @@ struct perf_event_read {
} values[2];
};
-#define MBM_STR "mbm"
-#define MBA_STR "mba"
-#define CMT_STR "cmt"
-#define CAT_STR "cat"
-
/*
* Memory location that consumes values compiler must not optimize away.
* Volatile ensures writes to this location cannot be optimized away by
@@ -141,8 +134,7 @@ int taskset_benchmark(pid_t bm_pid, int cpu_no, cpu_set_t *old_affinity);
int taskset_restore(pid_t bm_pid, cpu_set_t *old_affinity);
int write_schemata(const char *ctrlgrp, char *schemata, int cpu_no,
const char *resource);
-int write_bm_pid_to_resctrl(pid_t bm_pid, const char *ctrlgrp,
- const char *mongrp, const char *resctrl_val);
+int write_bm_pid_to_resctrl(pid_t bm_pid, const char *ctrlgrp, const char *mongrp);
int perf_event_open(struct perf_event_attr *hw_event, pid_t pid, int cpu,
int group_fd, unsigned long flags);
unsigned char *alloc_buffer(size_t buf_size, int memflush);
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 2f166a5c0c9b..f2101ee665ba 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -632,7 +632,6 @@ int resctrl_val(const struct resctrl_test *test,
const char * const *benchmark_cmd,
struct resctrl_val_param *param)
{
- char *resctrl_val = param->resctrl_val;
struct sigaction sigact;
int ret = 0, pipefd[2];
char pipe_message = 0;
@@ -723,8 +722,7 @@ int resctrl_val(const struct resctrl_test *test,
goto out;
/* Write benchmark to specified control&monitoring grp in resctrl FS */
- ret = write_bm_pid_to_resctrl(bm_pid, param->ctrlgrp, param->mongrp,
- resctrl_val);
+ ret = write_bm_pid_to_resctrl(bm_pid, param->ctrlgrp, param->mongrp);
if (ret)
goto out;
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index dbe0cc6d74fa..1bd70ac73ae2 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -524,7 +524,6 @@ static int write_pid_to_tasks(char *tasks, pid_t pid)
* @bm_pid: PID that should be written
* @ctrlgrp: Name of the control monitor group (con_mon grp)
* @mongrp: Name of the monitor group (mon grp)
- * @resctrl_val: Resctrl feature (Eg: mbm, mba.. etc)
*
* If a con_mon grp is requested, create it and write pid to it, otherwise
* write pid to root con_mon grp.
@@ -534,8 +533,7 @@ static int write_pid_to_tasks(char *tasks, pid_t pid)
*
* Return: 0 on success, < 0 on error.
*/
-int write_bm_pid_to_resctrl(pid_t bm_pid, const char *ctrlgrp,
- const char *mongrp, const char *resctrl_val)
+int write_bm_pid_to_resctrl(pid_t bm_pid, const char *ctrlgrp, const char *mongrp)
{
char controlgroup[128], monitorgroup[512], monitorgroup_p[256];
char tasks[1024];
@@ -555,22 +553,19 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, const char *ctrlgrp,
if (ret)
goto out;
- /* Create mon grp and write pid into it for "mbm" and "cmt" test */
- if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)) ||
- !strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR))) {
- if (mongrp) {
- sprintf(monitorgroup_p, "%s/mon_groups", controlgroup);
- sprintf(monitorgroup, "%s/%s", monitorgroup_p, mongrp);
- ret = create_grp(mongrp, monitorgroup, monitorgroup_p);
- if (ret)
- goto out;
-
- sprintf(tasks, "%s/mon_groups/%s/tasks",
- controlgroup, mongrp);
- ret = write_pid_to_tasks(tasks, bm_pid);
- if (ret)
- goto out;
- }
+ /* Create monitor group and write pid into if it is used */
+ if (mongrp) {
+ sprintf(monitorgroup_p, "%s/mon_groups", controlgroup);
+ sprintf(monitorgroup, "%s/%s", monitorgroup_p, mongrp);
+ ret = create_grp(mongrp, monitorgroup, monitorgroup_p);
+ if (ret)
+ goto out;
+
+ sprintf(tasks, "%s/mon_groups/%s/tasks",
+ controlgroup, mongrp);
+ ret = write_pid_to_tasks(tasks, bm_pid);
+ if (ret)
+ goto out;
}
out:
--
2.39.2
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 <[email protected]>
---
tools/testing/selftests/resctrl/resctrl_val.c | 33 ++++++++-----------
1 file changed, 13 insertions(+), 20 deletions(-)
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 4df2cd738f88..7981589f4db0 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -431,19 +431,12 @@ void set_mbm_path(const char *ctrlgrp, const char *mongrp, int domain_id)
* initialize_mem_bw_resctrl: Appropriately populate "mbm_total_path"
* @ctrlgrp: Name of the control monitor group (con_mon grp)
* @mongrp: Name of the monitor group (mon grp)
- * @cpu_no: CPU number that the benchmark PID is binded to
+ * @domain_id: Domain ID (cache ID; for MB, L3 cache ID)
* @resctrl_val: Resctrl feature (Eg: mbm, mba.. etc)
*/
static void initialize_mem_bw_resctrl(const char *ctrlgrp, const char *mongrp,
- int cpu_no, char *resctrl_val)
+ int domain_id, char *resctrl_val)
{
- int domain_id;
-
- if (get_domain_id("MB", cpu_no, &domain_id) < 0) {
- ksft_print_msg("Could not get domain ID\n");
- return;
- }
-
if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)))
set_mbm_path(ctrlgrp, mongrp, domain_id);
@@ -600,19 +593,12 @@ static void set_cmt_path(const char *ctrlgrp, const char *mongrp, char sock_num)
* initialize_llc_occu_resctrl: Appropriately populate "llc_occup_path"
* @ctrlgrp: Name of the control monitor group (con_mon grp)
* @mongrp: Name of the monitor group (mon grp)
- * @cpu_no: CPU number that the benchmark PID is binded to
+ * @domain_id: Domain ID (cache ID; for MB, L3 cache ID)
* @resctrl_val: Resctrl feature (Eg: cat, cmt.. etc)
*/
static void initialize_llc_occu_resctrl(const char *ctrlgrp, const char *mongrp,
- int cpu_no, char *resctrl_val)
+ int domain_id, char *resctrl_val)
{
- int domain_id;
-
- if (get_domain_id("L3", cpu_no, &domain_id) < 0) {
- ksft_print_msg("Could not get domain ID\n");
- return;
- }
-
if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
set_cmt_path(ctrlgrp, mongrp, domain_id);
}
@@ -729,10 +715,17 @@ int resctrl_val(const struct resctrl_test *test,
int ret = 0, pipefd[2];
char pipe_message = 0;
union sigval value;
+ int domain_id;
if (strcmp(param->filename, "") == 0)
sprintf(param->filename, "stdio");
+ ret = get_domain_id(test->resource, uparams->cpu, &domain_id);
+ if (ret < 0) {
+ ksft_print_msg("Could not get domain ID\n");
+ return ret;
+ }
+
if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) ||
!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR))) {
ret = validate_bw_report_request(param->bw_report);
@@ -827,10 +820,10 @@ int resctrl_val(const struct resctrl_test *test,
goto out;
initialize_mem_bw_resctrl(param->ctrlgrp, param->mongrp,
- uparams->cpu, resctrl_val);
+ domain_id, resctrl_val);
} else if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
initialize_llc_occu_resctrl(param->ctrlgrp, param->mongrp,
- uparams->cpu, resctrl_val);
+ domain_id, resctrl_val);
/* Parent waits for child to be ready. */
close(pipefd[1]);
--
2.39.2
Hi Ilpo,
On 3/11/2024 6:52 AM, Ilpo Järvinen wrote:
> For MBM/MBA tests, measure_vals() calls get_mem_bw_imc() that performs
> the measurement over a duration of sleep(1) call. The memory bandwidth
> numbers from IMC are derived over this duration. The resctrl FS derived
> memory bandwidth, however, is calculated inside measure_vals() and only
> takes delta between the previous value and the current one which
> besides the actual test, also samples inter-test noise.
>
> Rework the logic in measure_vals() and get_mem_bw_imc() such that the
> resctrl FS memory bandwidth section covers much shorter duration
> closely matching that of the IMC perf counters to improve measurement
> accuracy.
>
Thank you very much for doing this.
> Suggested-by: Reinette Chatre <[email protected]>
> Signed-off-by: Ilpo Järvinen <[email protected]>
> ---
> tools/testing/selftests/resctrl/resctrl_val.c | 72 +++++++++++++------
> 1 file changed, 50 insertions(+), 22 deletions(-)
>
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index 36139cba7be8..4df2cd738f88 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -293,28 +293,35 @@ static int initialize_mem_bw_imc(void)
> }
>
> /*
> - * get_mem_bw_imc: Memory band width as reported by iMC counters
> + * perf_open_imc_mem_bw - Open perf fds for IMCs
> * @cpu_no: CPU number that the benchmark PID is binded to
> - * @bw_report: Bandwidth report type (reads, writes)
> - *
> - * Memory B/W utilized by a process on a socket can be calculated using
> - * iMC counters. Perf events are used to read these counters.
> - *
> - * Return: = 0 on success. < 0 on failure.
This "Return" still seems relevant.
> */
> -static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
> +static int perf_open_imc_mem_bw(int cpu_no)
> {
> - float reads, writes, of_mul_read, of_mul_write;
> int imc, j, ret;
>
> - /* Start all iMC counters to log values (both read and write) */
> - reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
> for (imc = 0; imc < imcs; imc++) {
> for (j = 0; j < 2; j++) {
> ret = open_perf_event(imc, cpu_no, j);
> if (ret)
> return -1;
> }
I'm feeling more strongly that this inner loop makes the code harder to
understand and unwinding it would make it easier to understand.
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * do_mem_bw_test - Perform memory bandwidth test
> + *
> + * Runs memory bandwidth test over one second period. Also, handles starting
> + * and stopping of the IMC perf counters around the test.
> + */
> +static void do_imc_mem_bw_test(void)
> +{
> + int imc, j;
> +
> + for (imc = 0; imc < imcs; imc++) {
> for (j = 0; j < 2; j++)
> membw_ioctl_perf_event_ioc_reset_enable(imc, j);
Here also. I find these loops unnecessary. I do not think it optimizes anything
and it makes the code harder to understand.
> }
> @@ -326,6 +333,24 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
> for (j = 0; j < 2; j++)
> membw_ioctl_perf_event_ioc_disable(imc, j);
Same here.
> }
> +}
> +
> +/*
> + * get_mem_bw_imc - Memory band width as reported by iMC counters
> + * @bw_report: Bandwidth report type (reads, writes)
> + *
> + * Memory B/W utilized by a process on a socket can be calculated using
> + * iMC counters. Perf events are used to read these counters.
In the above there are three variations of the same: "band width", "Bandwidth",
and "B/W". Please just use one and use it consistently.
> + *
> + * Return: = 0 on success. < 0 on failure.
> + */
> +static int get_mem_bw_imc(char *bw_report, float *bw_imc)
> +{
> + float reads, writes, of_mul_read, of_mul_write;
> + int imc, j;
> +
> + /* Start all iMC counters to log values (both read and write) */
> + reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
>
> /*
> * Get results which are stored in struct type imc_counter_config
> @@ -593,10 +618,9 @@ static void initialize_llc_occu_resctrl(const char *ctrlgrp, const char *mongrp,
> }
>
> static int measure_vals(const struct user_params *uparams,
> - struct resctrl_val_param *param,
> - unsigned long *bw_resc_start)
> + struct resctrl_val_param *param)
> {
> - unsigned long bw_resc, bw_resc_end;
> + unsigned long bw_resc, bw_resc_start, bw_resc_end;
> float bw_imc;
> int ret;
>
> @@ -607,22 +631,27 @@ static int measure_vals(const struct user_params *uparams,
> * Compare the two values to validate resctrl value.
> * It takes 1sec to measure the data.
> */
> - ret = get_mem_bw_imc(uparams->cpu, param->bw_report, &bw_imc);
> + ret = perf_open_imc_mem_bw(uparams->cpu);
> if (ret < 0)
> return ret;
>
> + ret = get_mem_bw_resctrl(&bw_resc_start);
> + if (ret < 0)
> + return ret;
> +
> + do_imc_mem_bw_test();
> +
> ret = get_mem_bw_resctrl(&bw_resc_end);
> if (ret < 0)
> return ret;
>
> - bw_resc = (bw_resc_end - *bw_resc_start) / MB;
> - ret = print_results_bw(param->filename, bm_pid, bw_imc, bw_resc);
> - if (ret)
> + ret = get_mem_bw_imc(param->bw_report, &bw_imc);
> + if (ret < 0)
> return ret;
>
> - *bw_resc_start = bw_resc_end;
> + bw_resc = (bw_resc_end - bw_resc_start) / MB;
>
> - return 0;
> + return print_results_bw(param->filename, bm_pid, bw_imc, bw_resc);
> }
>
> /*
> @@ -696,7 +725,6 @@ int resctrl_val(const struct resctrl_test *test,
> struct resctrl_val_param *param)
> {
> char *resctrl_val = param->resctrl_val;
> - unsigned long bw_resc_start = 0;
In the current implementation the first iteration's starting measurement
is, as seen above, 0 ... which makes the first measurement unreliable
and dropped for both the MBA and MBM tests. In this enhancement, the
first measurement is no longer skewed so much so I wonder if this enhancement
can be expanded to the analysis phase where first measurement no longer
needs to be dropped?
> struct sigaction sigact;
> int ret = 0, pipefd[2];
> char pipe_message = 0;
> @@ -838,7 +866,7 @@ int resctrl_val(const struct resctrl_test *test,
>
> if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
> !strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
> - ret = measure_vals(uparams, param, &bw_resc_start);
> + ret = measure_vals(uparams, param);
> if (ret)
> break;
> } else if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) {
Reinette
On Tue, 19 Mar 2024, Reinette Chatre wrote:
> On 3/11/2024 6:52 AM, Ilpo Järvinen wrote:
> > For MBM/MBA tests, measure_vals() calls get_mem_bw_imc() that performs
> > the measurement over a duration of sleep(1) call. The memory bandwidth
> > numbers from IMC are derived over this duration. The resctrl FS derived
> > memory bandwidth, however, is calculated inside measure_vals() and only
> > takes delta between the previous value and the current one which
> > besides the actual test, also samples inter-test noise.
> >
> > Rework the logic in measure_vals() and get_mem_bw_imc() such that the
> > resctrl FS memory bandwidth section covers much shorter duration
> > closely matching that of the IMC perf counters to improve measurement
> > accuracy.
> >
>
> Thank you very much for doing this.
>
> > Suggested-by: Reinette Chatre <[email protected]>
> > Signed-off-by: Ilpo Järvinen <[email protected]>
> > ---
> > tools/testing/selftests/resctrl/resctrl_val.c | 72 +++++++++++++------
> > 1 file changed, 50 insertions(+), 22 deletions(-)
> >
> > diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> > index 36139cba7be8..4df2cd738f88 100644
> > --- a/tools/testing/selftests/resctrl/resctrl_val.c
> > +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> > -static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
> > +static int perf_open_imc_mem_bw(int cpu_no)
> > {
> > - float reads, writes, of_mul_read, of_mul_write;
> > int imc, j, ret;
> >
> > - /* Start all iMC counters to log values (both read and write) */
> > - reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
> > for (imc = 0; imc < imcs; imc++) {
> > for (j = 0; j < 2; j++) {
> > ret = open_perf_event(imc, cpu_no, j);
> > if (ret)
> > return -1;
> > }
>
> I'm feeling more strongly that this inner loop makes the code harder to
> understand and unwinding it would make it easier to understand.
Okay, I'll unwind them in the first patch.
> > }
> > +}
> > +
> > +/*
> > + * get_mem_bw_imc - Memory band width as reported by iMC counters
> > + * @bw_report: Bandwidth report type (reads, writes)
> > + *
> > + * Memory B/W utilized by a process on a socket can be calculated using
> > + * iMC counters. Perf events are used to read these counters.
>
> In the above there are three variations of the same: "band width", "Bandwidth",
> and "B/W". Please just use one and use it consistently.
Okay but I'll do that in a separate patch because these are just the
"removed" lines above, the diff is more messy than the actual change
here as is often the case with this kind of split function refactoring
because the diff algorithm fails to pair the lines optimally from
human-readed PoV.
> > + * Return: = 0 on success. < 0 on failure.
> > + */
> > +static int get_mem_bw_imc(char *bw_report, float *bw_imc)
> > +{
> > + float reads, writes, of_mul_read, of_mul_write;
> > + int imc, j;
> > +
> > + /* Start all iMC counters to log values (both read and write) */
> > + reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
> >
> > /*
> > * Get results which are stored in struct type imc_counter_config
> > @@ -696,7 +725,6 @@ int resctrl_val(const struct resctrl_test *test,
> > struct resctrl_val_param *param)
> > {
> > char *resctrl_val = param->resctrl_val;
> > - unsigned long bw_resc_start = 0;
>
> In the current implementation the first iteration's starting measurement
> is, as seen above, 0 ... which makes the first measurement unreliable
> and dropped for both the MBA and MBM tests. In this enhancement, the
> first measurement is no longer skewed so much so I wonder if this enhancement
> can be expanded to the analysis phase where first measurement no longer
> needs to be dropped?
In ideal world, yes, but I'll have to check the raw numbers. My general
feel is that the numbers tend to converge slowly with more iterations
being run so the first iteration might still be "off" by quite much (this
is definitely the case with CAT tests iterations but I'm not entirely sure
any more how it is with other selftests).
Thanks for the review.
--
i.
Hi Ilpo,
On 3/22/2024 5:11 AM, Ilpo Järvinen wrote:
> On Tue, 19 Mar 2024, Reinette Chatre wrote:
>> On 3/11/2024 6:52 AM, Ilpo Järvinen wrote:
>>> -static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
>>> +static int perf_open_imc_mem_bw(int cpu_no)
>>> {
>>> - float reads, writes, of_mul_read, of_mul_write;
>>> int imc, j, ret;
>>>
>>> - /* Start all iMC counters to log values (both read and write) */
>>> - reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
>>> for (imc = 0; imc < imcs; imc++) {
>>> for (j = 0; j < 2; j++) {
>>> ret = open_perf_event(imc, cpu_no, j);
>>> if (ret)
>>> return -1;
>>> }
>>
>> I'm feeling more strongly that this inner loop makes the code harder to
>> understand and unwinding it would make it easier to understand.
>
> Okay, I'll unwind them in the first patch.
Thank you very much.
>
>>> }
>>> +}
>>> +
>>> +/*
>>> + * get_mem_bw_imc - Memory band width as reported by iMC counters
>>> + * @bw_report: Bandwidth report type (reads, writes)
>>> + *
>>> + * Memory B/W utilized by a process on a socket can be calculated using
>>> + * iMC counters. Perf events are used to read these counters.
>>
>> In the above there are three variations of the same: "band width", "Bandwidth",
>> and "B/W". Please just use one and use it consistently.
>
> Okay but I'll do that in a separate patch because these are just the
> "removed" lines above, the diff is more messy than the actual change
> here as is often the case with this kind of split function refactoring
> because the diff algorithm fails to pair the lines optimally from
> human-readed PoV.
I have not used these much myself bit I've heard folks having success
getting more readable diffs when using the --patience or --histogram
algorithms.
>
>>> + * Return: = 0 on success. < 0 on failure.
>>> + */
>>> +static int get_mem_bw_imc(char *bw_report, float *bw_imc)
>>> +{
>>> + float reads, writes, of_mul_read, of_mul_write;
>>> + int imc, j;
>>> +
>>> + /* Start all iMC counters to log values (both read and write) */
>>> + reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
>>>
>>> /*
>>> * Get results which are stored in struct type imc_counter_config
>
>>> @@ -696,7 +725,6 @@ int resctrl_val(const struct resctrl_test *test,
>>> struct resctrl_val_param *param)
>>> {
>>> char *resctrl_val = param->resctrl_val;
>>> - unsigned long bw_resc_start = 0;
>>
>> In the current implementation the first iteration's starting measurement
>> is, as seen above, 0 ... which makes the first measurement unreliable
>> and dropped for both the MBA and MBM tests. In this enhancement, the
>> first measurement is no longer skewed so much so I wonder if this enhancement
>> can be expanded to the analysis phase where first measurement no longer
>> needs to be dropped?
>
> In ideal world, yes, but I'll have to check the raw numbers. My general
> feel is that the numbers tend to converge slowly with more iterations
> being run so the first iteration might still be "off" by quite much (this
> is definitely the case with CAT tests iterations but I'm not entirely sure
> any more how it is with other selftests).
>
From what I can tell the CAT test is not dropping any results. It looks
to me that any "settling" is and should be handled in the test before
the data collection starts.
Reinette
On Fri, 22 Mar 2024, Reinette Chatre wrote:
> On 3/22/2024 5:11 AM, Ilpo Järvinen wrote:
> > On Tue, 19 Mar 2024, Reinette Chatre wrote:
> >> On 3/11/2024 6:52 AM, Ilpo Järvinen wrote:
> >>> + * Return: = 0 on success. < 0 on failure.
> >>> + */
> >>> +static int get_mem_bw_imc(char *bw_report, float *bw_imc)
> >>> +{
> >>> + float reads, writes, of_mul_read, of_mul_write;
> >>> + int imc, j;
> >>> +
> >>> + /* Start all iMC counters to log values (both read and write) */
> >>> + reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
> >>>
> >>> /*
> >>> * Get results which are stored in struct type imc_counter_config
> >
> >>> @@ -696,7 +725,6 @@ int resctrl_val(const struct resctrl_test *test,
> >>> struct resctrl_val_param *param)
> >>> {
> >>> char *resctrl_val = param->resctrl_val;
> >>> - unsigned long bw_resc_start = 0;
> >>
> >> In the current implementation the first iteration's starting measurement
> >> is, as seen above, 0 ... which makes the first measurement unreliable
> >> and dropped for both the MBA and MBM tests. In this enhancement, the
> >> first measurement is no longer skewed so much so I wonder if this enhancement
> >> can be expanded to the analysis phase where first measurement no longer
> >> needs to be dropped?
> >
> > In ideal world, yes, but I'll have to check the raw numbers. My general
> > feel is that the numbers tend to converge slowly with more iterations
> > being run so the first iteration might still be "off" by quite much (this
> > is definitely the case with CAT tests iterations but I'm not entirely sure
> > any more how it is with other selftests).
>
> >From what I can tell the CAT test is not dropping any results. It looks
> to me that any "settling" is and should be handled in the test before
> the data collection starts.
It doesn't, but the "settling" is there in the raw numbers. I've
considered adding warm-up test(s) before the actual runs to improve the
situation but there's just so many thing still to do...
--
i.