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 (Maciej has
privately agreed to base his series on top of this series) and also
with the MBA/MBM series from Babu.
--
i.
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: Open get_mem_bw_imc() fd for loops
selftests/resctrl: Calculate resctrl FS derived mem bw over sleep(1)
only
selftests/resctrl: Fix closing IMC fds on error
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 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 | 26 +-
tools/testing/selftests/resctrl/mbm_test.c | 25 +-
tools/testing/selftests/resctrl/resctrl.h | 49 ++-
tools/testing/selftests/resctrl/resctrl_val.c | 295 +++++++-----------
tools/testing/selftests/resctrl/resctrlfs.c | 64 ++--
8 files changed, 238 insertions(+), 253 deletions(-)
--
2.39.2
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 <[email protected]>
---
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 3059ccc51a5a..b88762c4228a 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);
@@ -118,7 +124,8 @@ static int mbm_run_test(const struct resctrl_test *test, const struct user_param
.mongrp = "m1",
.filename = RESULT_FILE_NAME,
.bw_report = "reads",
- .setup = mbm_setup
+ .setup = mbm_setup,
+ .measure = mbm_measure,
};
int ret;
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index 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 e8e5c0f7f20b..990648bcc366 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -598,8 +598,8 @@ static void initialize_llc_occu_resctrl(const char *ctrlgrp, const char *mongrp,
* @param: parameters passed to resctrl_val()
* @bm_pid: PID that runs the benchmark
*/
-static int measure_mem_bw(const struct user_params *uparams,
- struct resctrl_val_param *param, pid_t bm_pid)
+int measure_mem_bw(const struct user_params *uparams,
+ struct resctrl_val_param *param, pid_t bm_pid)
{
unsigned long bw_resc, bw_resc_start, bw_resc_end;
float bw_imc;
@@ -851,17 +851,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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On 4/8/24 10:32, Ilpo Järvinen wrote:
> 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 (Maciej has
> privately agreed to base his series on top of this series) and also
> with the MBA/MBM series from Babu.
>
> --
> i.
>
> 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)
>
I can apply these for Linux 6.10-rc1 once I get an Ack from Reinette.
thanks,
-- Shuah
Hi Shuah and Ilpo,
On 4/24/2024 6:49 AM, Shuah Khan wrote:
> On 4/8/24 10:32, Ilpo Järvinen wrote:
>> 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 (Maciej has
>> privately agreed to base his series on top of this series) and also
>> with the MBA/MBM series from Babu.
>>
>> --
>> i.
>>
>> 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)
>>
>
> I can apply these for Linux 6.10-rc1 once I get an Ack from Reinette.
>
Apologies for the delay in reviewing this. I've now provided feedback for
consideration.
Reinette