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.
Signed-off-by: Ilpo Järvinen <[email protected]>
---
tools/testing/selftests/resctrl/cmt_test.c | 12 ++
tools/testing/selftests/resctrl/mba_test.c | 24 +++-
tools/testing/selftests/resctrl/mbm_test.c | 24 +++-
tools/testing/selftests/resctrl/resctrl.h | 9 +-
tools/testing/selftests/resctrl/resctrl_val.c | 123 ++----------------
5 files changed, 75 insertions(+), 117 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index 241c0b129b58..e79eca9346f3 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 set_cmt_path(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 = set_cmt_path,
.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 0939f86514f7..22c1f5e43352 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -8,6 +8,8 @@
* Sai Praneeth Prakhya <[email protected]>,
* Fenghua Yu <[email protected]>
*/
+#include <limits.h>
+
#include "resctrl.h"
#define RESULT_FILE_NAME "result_mba"
@@ -17,6 +19,25 @@
#define ALLOCATION_MIN 10
#define ALLOCATION_STEP 10
+#define CON_MBM_LOCAL_BYTES_PATH \
+ "%s/%s/mon_data/mon_L3_%02d/mbm_local_bytes"
+
+static char mbm_total_path[PATH_MAX];
+
+static int set_mba_path(const struct resctrl_val_param *param, int domain_id)
+{
+ int ret;
+
+ ret = initialize_mem_bw_imc();
+ if (ret)
+ return ret;
+
+ sprintf(mbm_total_path, CON_MBM_LOCAL_BYTES_PATH,
+ RESCTRL_PATH, param->ctrlgrp, domain_id);
+
+ return 0;
+}
+
/*
* Change schemata percentage from 100 to 10%. Write schemata to specified
* con_mon grp, mon_grp in resctrl FS.
@@ -54,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);
+ return measure_mem_bw(uparams, param, bm_pid, mbm_total_path);
}
static bool show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc)
@@ -156,6 +177,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 = 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 17398cd3aace..ffbfcecf9bd6 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -8,12 +8,19 @@
* Sai Praneeth Prakhya <[email protected]>,
* Fenghua Yu <[email protected]>
*/
+#include <limits.h>
+
#include "resctrl.h"
#define RESULT_FILE_NAME "result_mbm"
#define MAX_DIFF_PERCENT 8
#define NUM_OF_RUNS 5
+#define CON_MON_MBM_LOCAL_BYTES_PATH \
+ "%s/%s/mon_groups/%s/mon_data/mon_L3_%02d/mbm_local_bytes"
+
+static char mbm_total_path[PATH_MAX];
+
static int
show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc, size_t span)
{
@@ -86,6 +93,20 @@ static int check_results(size_t span)
return ret;
}
+static int set_mbm_path(const struct resctrl_val_param *param, int domain_id)
+{
+ int ret;
+
+ ret = initialize_mem_bw_imc();
+ if (ret)
+ return ret;
+
+ sprintf(mbm_total_path, CON_MON_MBM_LOCAL_BYTES_PATH,
+ RESCTRL_PATH, param->ctrlgrp, param->mongrp, domain_id);
+
+ return 0;
+}
+
static int mbm_setup(const struct resctrl_test *test,
const struct user_params *uparams,
struct resctrl_val_param *p)
@@ -108,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);
+ return measure_mem_bw(uparams, param, bm_pid, mbm_total_path);
}
void mbm_test_cleanup(void)
@@ -124,6 +145,7 @@ static int mbm_run_test(const struct resctrl_test *test, const struct user_param
.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 2da642e11b61..d9c6443a8524 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -84,7 +84,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 {
@@ -95,6 +96,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);
@@ -147,8 +150,10 @@ 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);
+ struct resctrl_val_param *param,
+ pid_t bm_pid, const char *mbm_total_path);
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 13d89d24474e..1a96298592ed 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -19,29 +19,6 @@
#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"
-
-#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 */
@@ -59,7 +36,6 @@ struct imc_counter_config {
int fd;
};
-static char mbm_total_path[1024];
static int imcs;
static struct imc_counter_config imc_counters_config[MAX_IMCS][2];
@@ -275,7 +251,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;
@@ -411,56 +387,10 @@ static int get_mem_bw_imc(char *bw_report, float *bw_imc)
return 0;
}
-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)
-{
- 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);
- }
-}
-
/*
* 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
*/
-static int get_mem_bw_resctrl(unsigned long *mbm_total)
+static int get_mem_bw_resctrl(const char *mbm_total_path, unsigned long *mbm_total)
{
FILE *fp;
@@ -581,43 +511,16 @@ 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
* @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
*/
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 *mbm_total_path)
{
unsigned long bw_resc, bw_resc_start, bw_resc_end;
float bw_imc;
@@ -634,13 +537,13 @@ int measure_mem_bw(const struct user_params *uparams,
if (ret < 0)
return ret;
- ret = get_mem_bw_resctrl(&bw_resc_start);
+ ret = get_mem_bw_resctrl(mbm_total_path, &bw_resc_start);
if (ret < 0)
return ret;
do_imc_mem_bw_test();
- ret = get_mem_bw_resctrl(&bw_resc_end);
+ ret = get_mem_bw_resctrl(mbm_total_path, &bw_resc_end);
if (ret < 0)
return ret;
@@ -826,17 +729,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, param->mongrp,
- domain_id, resctrl_val);
- } 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
Hi,
On 2024-03-11 at 15:52:25 +0200, Ilpo J?rvinen wrote:
>diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
>index 241c0b129b58..e79eca9346f3 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 set_cmt_path(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;
>+}
>+
Is there an option to make this function (and the set_mbm_path()) global through
the resctrl.h?
I'd like to use it in my SNC series [1] for looping over different nodes and
that requires changing the paths during the measure phase of the tests and that
part is currently in cache.c:measure_llc_resctrl().
Or would you suggest some other way of changing these paths in cache?
--
Kind regards
Maciej Wiecz?r-Retman
On 2024-03-13 at 11:15:30 +0100, Maciej Wieczor-Retman wrote:
>Hi,
>
>On 2024-03-11 at 15:52:25 +0200, Ilpo J?rvinen wrote:
>>diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
>>index 241c0b129b58..e79eca9346f3 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 set_cmt_path(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;
>>+}
>>+
>
>Is there an option to make this function (and the set_mbm_path()) global through
>the resctrl.h?
>
>I'd like to use it in my SNC series [1] for looping over different nodes and
>that requires changing the paths during the measure phase of the tests and that
>part is currently in cache.c:measure_llc_resctrl().
>
>Or would you suggest some other way of changing these paths in cache?
>
+forgot to add the link :b
[1] https://lore.kernel.org/all/[email protected]/
--
Kind regards
Maciej Wiecz?r-Retman
On Wed, 13 Mar 2024, Maciej Wieczor-Retman wrote:
> On 2024-03-13 at 11:15:30 +0100, Maciej Wieczor-Retman wrote:
> >On 2024-03-11 at 15:52:25 +0200, Ilpo J?rvinen wrote:
> >>diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
> >>index 241c0b129b58..e79eca9346f3 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 set_cmt_path(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;
> >>+}
> >>+
> >
> >Is there an option to make this function (and the set_mbm_path()) global through
> >the resctrl.h?
> >
> >I'd like to use it in my SNC series [1] for looping over different nodes and
> >that requires changing the paths during the measure phase of the tests and that
> >part is currently in cache.c:measure_llc_resctrl().
> >
> >Or would you suggest some other way of changing these paths in cache?
> >
>
> +forgot to add the link :b
>
> [1] https://lore.kernel.org/all/[email protected]/
Perhaps ->init() should just prepare an array of filenames to read from
to support SNC. That would keep the filename preparations out of the
measurement period.
It feels slightly hacky to have an array of files but I cannot think of
anything else that would be cleaner and would not require creating the
filenames during the actual test.
--
i.
On 2024-03-13 at 13:37:51 +0200, Ilpo J?rvinen wrote:
>On Wed, 13 Mar 2024, Maciej Wieczor-Retman wrote:
>> On 2024-03-13 at 11:15:30 +0100, Maciej Wieczor-Retman wrote:
>> >On 2024-03-11 at 15:52:25 +0200, Ilpo J?rvinen wrote:
>> >>diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
>> >>index 241c0b129b58..e79eca9346f3 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 set_cmt_path(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;
>> >>+}
>> >>+
>> >
>> >Is there an option to make this function (and the set_mbm_path()) global through
>> >the resctrl.h?
>> >
>> >I'd like to use it in my SNC series [1] for looping over different nodes and
>> >that requires changing the paths during the measure phase of the tests and that
>> >part is currently in cache.c:measure_llc_resctrl().
>> >
>> >Or would you suggest some other way of changing these paths in cache?
>> >
>>
>> +forgot to add the link :b
>>
>> [1] https://lore.kernel.org/all/[email protected]/
>
>Perhaps ->init() should just prepare an array of filenames to read from
>to support SNC. That would keep the filename preparations out of the
>measurement period.
>
>It feels slightly hacky to have an array of files but I cannot think of
>anything else that would be cleaner and would not require creating the
>filenames during the actual test.
So the array of names would be a part of "struct resctrl_val_param"?
It would have to be dynamically allocated too before running the tests.
I kinda agree, creating filenames during measurements messes the whole
separation idea between setup and measuring. And as you said there are like 4
nodes max so not much memory would be wasted there.
I can experiment with it and try to add it in my series - since it's much more
tied to SNC. Unless you see it'd better fit here then that's fine with me too.
>
>
>--
> i.
--
Kind regards
Maciej Wiecz?r-Retman
On Wed, 13 Mar 2024, Maciej Wieczor-Retman wrote:
> On 2024-03-13 at 13:37:51 +0200, Ilpo J?rvinen wrote:
> >On Wed, 13 Mar 2024, Maciej Wieczor-Retman wrote:
> >> On 2024-03-13 at 11:15:30 +0100, Maciej Wieczor-Retman wrote:
> >> >On 2024-03-11 at 15:52:25 +0200, Ilpo J?rvinen wrote:
> >> >>diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
> >> >>index 241c0b129b58..e79eca9346f3 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 set_cmt_path(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;
> >> >>+}
> >> >>+
> >> >
> >> >Is there an option to make this function (and the set_mbm_path()) global through
> >> >the resctrl.h?
> >> >
> >> >I'd like to use it in my SNC series [1] for looping over different nodes and
> >> >that requires changing the paths during the measure phase of the tests and that
> >> >part is currently in cache.c:measure_llc_resctrl().
> >> >
> >> >Or would you suggest some other way of changing these paths in cache?
> >> >
> >>
> >> +forgot to add the link :b
> >>
> >> [1] https://lore.kernel.org/all/[email protected]/
> >
> >Perhaps ->init() should just prepare an array of filenames to read from
> >to support SNC. That would keep the filename preparations out of the
> >measurement period.
> >
> >It feels slightly hacky to have an array of files but I cannot think of
> >anything else that would be cleaner and would not require creating the
> >filenames during the actual test.
>
> So the array of names would be a part of "struct resctrl_val_param"?
This series has in independent of resctrl_val_param because resctrl_val()
itself doesn't care after the separation of generic and per test logic.
But that meant making it globals into the per test files which I didn't
like that much either. So perhaps having it in resctrl_val_param is
still better.
--
i.
> It would have to be dynamically allocated too before running the tests.
>
> I kinda agree, creating filenames during measurements messes the whole
> separation idea between setup and measuring. And as you said there are like 4
> nodes max so not much memory would be wasted there.
>
> I can experiment with it and try to add it in my series - since it's much more
> tied to SNC. Unless you see it'd better fit here then that's fine with me too.
Hi again :)
On 2024-03-11 at 15:52:25 +0200, Ilpo J?rvinen wrote:
>diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
>index 17398cd3aace..ffbfcecf9bd6 100644
>--- a/tools/testing/selftests/resctrl/mbm_test.c
>+++ b/tools/testing/selftests/resctrl/mbm_test.c
>@@ -8,12 +8,19 @@
> * Sai Praneeth Prakhya <[email protected]>,
> * Fenghua Yu <[email protected]>
> */
>+#include <limits.h>
>+
> #include "resctrl.h"
>
> #define RESULT_FILE_NAME "result_mbm"
> #define MAX_DIFF_PERCENT 8
> #define NUM_OF_RUNS 5
>
>+#define CON_MON_MBM_LOCAL_BYTES_PATH \
>+ "%s/%s/mon_groups/%s/mon_data/mon_L3_%02d/mbm_local_bytes"
>+
>+static char mbm_total_path[PATH_MAX];
>+
> static int
> show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc, size_t span)
> {
>@@ -86,6 +93,20 @@ static int check_results(size_t span)
> return ret;
> }
>
>+static int set_mbm_path(const struct resctrl_val_param *param, int domain_id)
>+{
>+ int ret;
>+
>+ ret = initialize_mem_bw_imc();
I just noticed this. Since there is not only path stuff here but also some imc
logic maybe the function names could be changed? Something like
set_mbm_path -> init_mbm
The same could apply for all these init functions or at least the mba one.
>+ if (ret)
>+ return ret;
>+
>+ sprintf(mbm_total_path, CON_MON_MBM_LOCAL_BYTES_PATH,
>+ RESCTRL_PATH, param->ctrlgrp, param->mongrp, domain_id);
>+
>+ return 0;
>+}
>+
>
--
Kind regards
Maciej Wiecz?r-Retman
On Thu, 14 Mar 2024, Maciej Wieczor-Retman wrote:
> On 2024-03-11 at 15:52:25 +0200, Ilpo J?rvinen wrote:
> >diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
> >index 17398cd3aace..ffbfcecf9bd6 100644
> >--- a/tools/testing/selftests/resctrl/mbm_test.c
> >+++ b/tools/testing/selftests/resctrl/mbm_test.c
> >@@ -8,12 +8,19 @@
> > * Sai Praneeth Prakhya <[email protected]>,
> > * Fenghua Yu <[email protected]>
> > */
> >+#include <limits.h>
> >+
> > #include "resctrl.h"
> >
> > #define RESULT_FILE_NAME "result_mbm"
> > #define MAX_DIFF_PERCENT 8
> > #define NUM_OF_RUNS 5
> >
> >+#define CON_MON_MBM_LOCAL_BYTES_PATH \
> >+ "%s/%s/mon_groups/%s/mon_data/mon_L3_%02d/mbm_local_bytes"
> >+
> >+static char mbm_total_path[PATH_MAX];
> >+
> > static int
> > show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc, size_t span)
> > {
> >@@ -86,6 +93,20 @@ static int check_results(size_t span)
> > return ret;
> > }
> >
> >+static int set_mbm_path(const struct resctrl_val_param *param, int domain_id)
> >+{
> >+ int ret;
> >+
> >+ ret = initialize_mem_bw_imc();
>
> I just noticed this. Since there is not only path stuff here but also some imc
> logic maybe the function names could be changed? Something like
>
> set_mbm_path -> init_mbm
>
> The same could apply for all these init functions or at least the mba one.
Ah yes, I'll rename them.
--
i.
Hi Ilpo,
On 3/11/2024 6:52 AM, Ilpo Järvinen wrote:
> 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.
>
> Signed-off-by: Ilpo Järvinen <[email protected]>
> ---
> tools/testing/selftests/resctrl/cmt_test.c | 12 ++
> tools/testing/selftests/resctrl/mba_test.c | 24 +++-
> tools/testing/selftests/resctrl/mbm_test.c | 24 +++-
> tools/testing/selftests/resctrl/resctrl.h | 9 +-
> tools/testing/selftests/resctrl/resctrl_val.c | 123 ++----------------
> 5 files changed, 75 insertions(+), 117 deletions(-)
>
> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
> index 241c0b129b58..e79eca9346f3 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 set_cmt_path(const struct resctrl_val_param *param, int domain_id)
> +{
> + sprintf(llc_occup_path, CON_MON_LCC_OCCUP_PATH, RESCTRL_PATH,
Strangely some tab characters sneaked in above.
> + 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)
..
> @@ -826,17 +729,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, param->mongrp,
> - domain_id, resctrl_val);
> - } 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]);
I am trying to make sense of what these tests are trying to do and
it is starting to look like the monitor groups (as they are used here)
are unnecessary.
Looking at resctrl_val()->write_bm_pid_to_resctrl() I see that the
control group is created with "bm_pid" written to its "tasks" file
and then a monitor group is created as child of the control group
with the same "bm_pid" written to its "tasks" file.
This looks unnecessary to me because when the original control
group is created on a system that supports monitoring then that
control group would automatically be a monitoring group also. In
the resctrl kernel document these different groups are termed
"CTRL_MON" and "MON" groups respectively.
For example, if user creates control group "c1":
# mkdir /sys/fs/resctrl/c1
Then, automatically it would also be a monitoring group with
its monitoring data available from
/sys/fs/resctrl/c1/mon_data/mon_L3_XX/*
PIDs written to /sys/fs/resctrl/c1/tasks will have allocations
assigned in /sys/fs/resctrl/c1/schemata and monitoring data
/sys/fd/resctrl/c1/mon_data/mon_L3_XX/* ... it is not necessary
to create a separate monitoring group to collect that
monitoring data.
This seems to be the discrepancy between the MBA and MBM test ...
if I understand correctly they end up needing the same data but
the one uses the data from the CTRL_MON group while the other
creates a redundant child MON group to read the same data
that is available from the CTRL_MON group.
Reinette
On Tue, 19 Mar 2024, Reinette Chatre wrote:
> On 3/11/2024 6:52 AM, Ilpo Järvinen wrote:
> > 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.
> >
> > Signed-off-by: Ilpo Järvinen <[email protected]>
> > ---
> > tools/testing/selftests/resctrl/cmt_test.c | 12 ++
> > tools/testing/selftests/resctrl/mba_test.c | 24 +++-
> > tools/testing/selftests/resctrl/mbm_test.c | 24 +++-
> > tools/testing/selftests/resctrl/resctrl.h | 9 +-
> > tools/testing/selftests/resctrl/resctrl_val.c | 123 ++----------------
> > 5 files changed, 75 insertions(+), 117 deletions(-)
> >
> > diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
> > index 241c0b129b58..e79eca9346f3 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 set_cmt_path(const struct resctrl_val_param *param, int domain_id)
> > +{
> > + sprintf(llc_occup_path, CON_MON_LCC_OCCUP_PATH, RESCTRL_PATH,
>
> Strangely some tab characters sneaked in above.
Ah, fixed it now. They seemed to came directly from the old code.
> > @@ -826,17 +729,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, param->mongrp,
> > - domain_id, resctrl_val);
> > - } 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]);
>
> I am trying to make sense of what these tests are trying to do and
> it is starting to look like the monitor groups (as they are used here)
> are unnecessary.
>
> Looking at resctrl_val()->write_bm_pid_to_resctrl() I see that the
> control group is created with "bm_pid" written to its "tasks" file
> and then a monitor group is created as child of the control group
> with the same "bm_pid" written to its "tasks" file.
> This looks unnecessary to me because when the original control
> group is created on a system that supports monitoring then that
> control group would automatically be a monitoring group also. In
> the resctrl kernel document these different groups are termed
> "CTRL_MON" and "MON" groups respectively.
>
> For example, if user creates control group "c1":
> # mkdir /sys/fs/resctrl/c1
> Then, automatically it would also be a monitoring group with
> its monitoring data available from
> /sys/fs/resctrl/c1/mon_data/mon_L3_XX/*
>
> PIDs written to /sys/fs/resctrl/c1/tasks will have allocations
> assigned in /sys/fs/resctrl/c1/schemata and monitoring data
> /sys/fd/resctrl/c1/mon_data/mon_L3_XX/* ... it is not necessary
> to create a separate monitoring group to collect that
> monitoring data.
>
> This seems to be the discrepancy between the MBA and MBM test ...
> if I understand correctly they end up needing the same data but
> the one uses the data from the CTRL_MON group while the other
> creates a redundant child MON group to read the same data
> that is available from the CTRL_MON group.
Okay, I can look into this too. I've not looked too deeply why the
difference between MBA and MBM test is there.
--
i.