[PATCH v6 11/16] selftests/resctrl: Simplify bandwidth report type handling
From: Ilpo Järvinen
Date: Fri Jun 07 2024 - 08:56:16 EST
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().
- Pass bw_report to measure_mem_bw() from ->measure() hook because
resctrl_val() no longer needs bw_report for anything.
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
Tested-by: Babu Moger <babu.moger@xxxxxxx>
Reviewed-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
---
v5:
- Use imperative tone in commit message
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 e38bcdd8b46f..2e85badd6d6b 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;
@@ -556,6 +556,7 @@ 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)
*
* Measure memory bandwidth from resctrl and from another source which is
* perf imc value or could be something else if perf imc event is not
@@ -563,13 +564,18 @@ static int print_results_bw(char *filename, pid_t bm_pid, float bw_imc,
* 1 sec to measure the data.
*/
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;
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;
@@ -590,7 +596,7 @@ int measure_mem_bw(const struct user_params *uparams,
if (ret < 0)
goto close_imc;
- ret = get_mem_bw_imc(param->bw_report, &bw_imc);
+ ret = get_mem_bw_imc(bw_report, &bw_imc);
if (ret < 0)
goto close_imc;
@@ -694,13 +700,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 917d677adbba..9e4cda154d66 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