All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
To: linux-kselftest@vger.kernel.org,
	"Reinette Chatre" <reinette.chatre@intel.com>,
	"Shuah Khan" <shuah@kernel.org>,
	"Babu Moger" <babu.moger@amd.com>,
	"Maciej Wieczór-Retman" <maciej.wieczor-retman@intel.com>,
	"Fenghua Yu" <fenghua.yu@intel.com>,
	linux-kernel@vger.kernel.org
Cc: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
Subject: [PATCH 09/13] selftests/resctrl: Simplify bandwidth report type handling
Date: Fri,  8 Mar 2024 15:22:51 +0200	[thread overview]
Message-ID: <20240308132255.14442-10-ilpo.jarvinen@linux.intel.com> (raw)
In-Reply-To: <20240308132255.14442-1-ilpo.jarvinen@linux.intel.com>

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>
---
 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 eb02d3322e0e..b5f76aa192eb 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -75,7 +75,7 @@ static int mba_setup(const struct resctrl_test *test,
 static int mba_measure(const struct user_params *uparams,
 		       struct resctrl_val_param *param, pid_t bm_pid)
 {
-	return measure_mem_bw(uparams, param, bm_pid, mbm_total_path);
+	return measure_mem_bw(uparams, param, bm_pid, mbm_total_path, "reads");
 }
 
 static bool show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc)
@@ -176,7 +176,6 @@ static int mba_run_test(const struct resctrl_test *test, const struct user_param
 		.ctrlgrp	= "c1",
 		.mongrp		= "m1",
 		.filename	= RESULT_FILE_NAME,
-		.bw_report	= "reads",
 		.init		= set_mba_path,
 		.setup		= mba_setup,
 		.measure	= mba_measure,
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index cb9f9d5101d9..85740177da41 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -129,7 +129,7 @@ static int mbm_setup(const struct resctrl_test *test,
 static int mbm_measure(const struct user_params *uparams,
 		       struct resctrl_val_param *param, pid_t bm_pid)
 {
-	return measure_mem_bw(uparams, param, bm_pid, mbm_total_path);
+	return measure_mem_bw(uparams, param, bm_pid, mbm_total_path, "reads");
 }
 
 void mbm_test_cleanup(void)
@@ -144,7 +144,6 @@ static int mbm_run_test(const struct resctrl_test *test, const struct user_param
 		.ctrlgrp	= "c1",
 		.mongrp		= "m1",
 		.filename	= RESULT_FILE_NAME,
-		.bw_report	= "reads",
 		.init		= set_mbm_path,
 		.setup		= mbm_setup,
 		.measure	= mbm_measure,
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index 511c87634d4f..9a1a3d0399ce 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -80,7 +80,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)
@@ -90,7 +89,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,
@@ -130,7 +128,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 validate_resctrl_feature_request(const char *resource, const char *feature);
 bool test_resource_feature_check(const struct resctrl_test *test);
 char *fgrep(FILE *inf, const char *str);
@@ -148,7 +146,8 @@ int run_fill_buf(size_t buf_size, int memflush, int op, bool once);
 int initialize_mem_bw_imc(void);
 int measure_mem_bw(const struct user_params *uparams,
 		   struct resctrl_val_param *param,
-		   pid_t bm_pid, const char *mbm_total_path);
+		   pid_t bm_pid, const char *mbm_total_path,
+		   const char *bw_report);
 int resctrl_val(const struct resctrl_test *test,
 		const struct user_params *uparams,
 		const char * const *benchmark_cmd,
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 1a96298592ed..2f166a5c0c9b 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -320,7 +320,7 @@ static void do_imc_mem_bw_test(void)
  *
  * Return: = 0 on success. < 0 on failure.
  */
-static int get_mem_bw_imc(char *bw_report, float *bw_imc)
+static int get_mem_bw_imc(const char *bw_report, float *bw_imc)
 {
 	float reads, writes, of_mul_read, of_mul_write;
 	int imc, j;
@@ -517,15 +517,21 @@ static int print_results_bw(char *filename, pid_t bm_pid, float bw_imc,
  * @param:		parameters passed to resctrl_val()
  * @bm_pid:		PID that runs the benchmark
  * @mbm_total_path:	Resctrl FS monitoring file to read mem BW from
+ * @bw_report:		Bandwidth report type (reads, writes)
  */
 int measure_mem_bw(const struct user_params *uparams,
 		   struct resctrl_val_param *param,
-		   pid_t bm_pid, const char *mbm_total_path)
+		   pid_t bm_pid, const char *mbm_total_path,
+		   const char *bw_report)
 {
 	unsigned long bw_resc, bw_resc_start, bw_resc_end;
 	float bw_imc;
 	int ret;
 
+	bw_report = get_bw_report_type(bw_report);
+	if (!bw_report)
+		return -1;
+
 	/*
 	 * Measure memory bandwidth from resctrl and from
 	 * another source which is perf imc value or could
@@ -547,7 +553,7 @@ int measure_mem_bw(const struct user_params *uparams,
 	if (ret < 0)
 		return ret;
 
-	ret = get_mem_bw_imc(param->bw_report, &bw_imc);
+	ret = get_mem_bw_imc(bw_report, &bw_imc);
 	if (ret < 0)
 		return ret;
 
@@ -642,13 +648,6 @@ int resctrl_val(const struct resctrl_test *test,
 		return ret;
 	}
 
-	if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) ||
-	    !strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR))) {
-		ret = validate_bw_report_request(param->bw_report);
-		if (ret)
-			return ret;
-	}
-
 	/*
 	 * If benchmark wasn't successfully started by child, then child should
 	 * kill parent, so save parent's pid
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index 5750662cce57..95a6be52d866 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -763,22 +763,21 @@ int filter_dmesg(void)
 	return 0;
 }
 
-int validate_bw_report_request(char *bw_report)
+const char *get_bw_report_type(const char *bw_report)
 {
 	if (strcmp(bw_report, "reads") == 0)
-		return 0;
+		return bw_report;
 	if (strcmp(bw_report, "writes") == 0)
-		return 0;
+		return bw_report;
 	if (strcmp(bw_report, "nt-writes") == 0) {
-		strcpy(bw_report, "writes");
-		return 0;
+		return "writes";
 	}
 	if (strcmp(bw_report, "total") == 0)
-		return 0;
+		return bw_report;
 
 	fprintf(stderr, "Requested iMC B/W report type unavailable\n");
 
-	return -1;
+	return NULL;
 }
 
 int perf_event_open(struct perf_event_attr *hw_event, pid_t pid, int cpu,
-- 
2.39.2


  parent reply	other threads:[~2024-03-08 13:24 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-08 13:22 [PATCH 00/13] selftests/resctrl: resctrl_val() related cleanups & improvements Ilpo Järvinen
2024-03-08 13:22 ` [PATCH 01/13] selftests/resctrl: Convert get_mem_bw_imc() fd close to for loop Ilpo Järvinen
2024-03-08 13:22 ` [PATCH 02/13] selftests/resctrl: Calculate resctrl FS derived mem bw over sleep(1) only Ilpo Järvinen
2024-03-08 13:22 ` [PATCH 03/13] selftests/resctrl: Consolidate get_domain_id() into resctrl_val() Ilpo Järvinen
2024-03-08 13:22 ` [PATCH 04/13] selftests/resctrl: Use correct type for pids Ilpo Järvinen
2024-03-08 13:22 ` [PATCH 05/13] selftests/resctrl: Cleanup bm_pid and ppid usage & limit scope Ilpo Järvinen
2024-03-08 13:22 ` [PATCH 06/13] selftests/resctrl: Rename measure_vals() to measure_mem_bw_vals() & document Ilpo Järvinen
2024-03-08 13:22 ` [PATCH 07/13] selftests/resctrl: Add ->measure() callback to resctrl_val_param Ilpo Järvinen
2024-03-08 13:22 ` [PATCH 08/13] selftests/resctrl: Add ->init() callback into resctrl_val_param Ilpo Järvinen
2024-03-08 13:22 ` Ilpo Järvinen [this message]
2024-03-08 13:22 ` [PATCH 10/13] selftests/resctrl: Make some strings passed to resctrlfs functions const Ilpo Järvinen
2024-03-08 13:22 ` [PATCH 11/13] selftests/resctrl: Convert ctrlgrp & mongrp to pointers Ilpo Järvinen
2024-03-08 13:22 ` [PATCH 12/13] selftests/resctrl: Remove mongrp from MBA test Ilpo Järvinen
2024-03-08 13:22 ` [PATCH 13/13] selftests/resctrl: Remove test name comparing from write_bm_pid_to_resctrl() Ilpo Järvinen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240308132255.14442-10-ilpo.jarvinen@linux.intel.com \
    --to=ilpo.jarvinen@linux.intel.com \
    --cc=babu.moger@amd.com \
    --cc=fenghua.yu@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=maciej.wieczor-retman@intel.com \
    --cc=reinette.chatre@intel.com \
    --cc=shuah@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.