Fenghua Yu (1):
selftests/resctrl: Don't hard code divisor in MBM results
Ilpo Järvinen (2):
selftests/resctrl: Change initialize_llc_perf() return type to void
selftests/resctrl: Use remount_resctrlfs() consistently with boolean
Signed-off-by: Fenghua Yu (1):
selftests/resctrl: Change name from CBM_MASK_PATH to INFO_PATH
tools/testing/selftests/resctrl/cache.c | 11 +++--------
tools/testing/selftests/resctrl/cat_test.c | 2 +-
tools/testing/selftests/resctrl/cmt_test.c | 7 +++----
tools/testing/selftests/resctrl/mba_test.c | 2 +-
tools/testing/selftests/resctrl/mbm_test.c | 6 +++---
tools/testing/selftests/resctrl/resctrl.h | 4 ++--
tools/testing/selftests/resctrl/resctrlfs.c | 2 +-
7 files changed, 14 insertions(+), 20 deletions(-)
--
2.30.2
initialize_llc_perf() unconditionally does return 0 so no point in
having it's return type as int. Hence, change it's return type from int
to void.
Co-developed-by: Fenghua Yu <[email protected]>
Signed-off-by: Fenghua Yu <[email protected]>
Signed-off-by: Ilpo Järvinen <[email protected]>
---
tools/testing/selftests/resctrl/cache.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
index 68ff856d36f0..4b8ee81aedae 100644
--- a/tools/testing/selftests/resctrl/cache.c
+++ b/tools/testing/selftests/resctrl/cache.c
@@ -48,7 +48,7 @@ static int perf_event_open_llc_miss(pid_t pid, int cpu_no)
return 0;
}
-static int initialize_llc_perf(void)
+static void initialize_llc_perf(void)
{
memset(&pea_llc_miss, 0, sizeof(struct perf_event_attr));
memset(&rf_cqm, 0, sizeof(struct read_format));
@@ -59,8 +59,6 @@ static int initialize_llc_perf(void)
pea_llc_miss.config = PERF_COUNT_HW_CACHE_MISSES;
rf_cqm.nr = 1;
-
- return 0;
}
static int reset_enable_llc_perf(pid_t pid, int cpu_no)
@@ -234,11 +232,8 @@ int cat_val(struct resctrl_val_param *param)
if (ret)
return ret;
- if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR))) {
- ret = initialize_llc_perf();
- if (ret)
- return ret;
- }
+ if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)))
+ initialize_llc_perf();
/* Test runs until the callback setup() tells the test to stop. */
while (1) {
--
2.30.2
From: Fenghua Yu <[email protected]>
Presently, while calculating MBM results, the divisor is hard coded as 4.
It's hard coded to 4 because "NUM_OF_RUNS" is defined as 5 and the test
does not count first result and hence 4. So, instead of hard coding the
value to 4, change it to NUM_OF_RUNS - 1.
Signed-off-by: Fenghua Yu <[email protected]>
---
tools/testing/selftests/resctrl/mbm_test.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index 8392e5c55ed0..4f85cbbfd037 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -31,8 +31,8 @@ show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc, int span)
sum_bw_resc += bw_resc[runs];
}
- avg_bw_imc = sum_bw_imc / 4;
- avg_bw_resc = sum_bw_resc / 4;
+ avg_bw_imc = sum_bw_imc / (NUM_OF_RUNS - 1);
+ avg_bw_resc = sum_bw_resc / (NUM_OF_RUNS - 1);
avg_diff = (float)labs(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
avg_diff_per = (int)(avg_diff * 100);
--
2.30.2
From: "Signed-off-by: Fenghua Yu" <[email protected]>
CBM_MASK_PATH is actually the path to resctrl/info, so change the macro
name to correctly indicate what it represents.
Signed-off-by: Fenghua Yu <[email protected]>
---
tools/testing/selftests/resctrl/resctrl.h | 2 +-
tools/testing/selftests/resctrl/resctrlfs.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index f0ded31fb3c7..4f0976f12634 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -28,7 +28,7 @@
#define MB (1024 * 1024)
#define RESCTRL_PATH "/sys/fs/resctrl"
#define PHYS_ID_PATH "/sys/devices/system/cpu/cpu"
-#define CBM_MASK_PATH "/sys/fs/resctrl/info"
+#define INFO_PATH "/sys/fs/resctrl/info"
#define L3_PATH "/sys/fs/resctrl/info/L3"
#define MB_PATH "/sys/fs/resctrl/info/MB"
#define L3_MON_PATH "/sys/fs/resctrl/info/L3_MON"
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index 6f543e470ad4..cc6cf49e3129 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -210,7 +210,7 @@ int get_cbm_mask(char *cache_type, char *cbm_mask)
if (!cbm_mask)
return -1;
- sprintf(cbm_mask_path, "%s/%s/cbm_mask", CBM_MASK_PATH, cache_type);
+ sprintf(cbm_mask_path, "%s/%s/cbm_mask", INFO_PATH, cache_type);
fp = fopen(cbm_mask_path, "r");
if (!fp) {
--
2.30.2
remount_resctrlfs() accepts a boolean value as an argument and presently,
some tests pass 0/1 and some tests pass true/false. Make all the callers of
remount_resctrlfs() use true/false so that it's usage is consistent across
tests.
Co-developed-by: Fenghua Yu <[email protected]>
Signed-off-by: Fenghua Yu <[email protected]>
Signed-off-by: Ilpo Järvinen <[email protected]>
---
tools/testing/selftests/resctrl/cat_test.c | 2 +-
tools/testing/selftests/resctrl/cmt_test.c | 7 +++----
tools/testing/selftests/resctrl/mba_test.c | 2 +-
tools/testing/selftests/resctrl/mbm_test.c | 2 +-
tools/testing/selftests/resctrl/resctrl.h | 2 +-
5 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
index 1c5e90c63254..265302094095 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -145,7 +145,7 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
struct resctrl_val_param param = {
.resctrl_val = CAT_STR,
.cpu_no = cpu_no,
- .mum_resctrlfs = 0,
+ .mum_resctrlfs = false,
.setup = cat_setup,
};
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index 8968e36db99d..4903e5eb04a1 100644
--- a/tools/testing/selftests/resctrl/cmt_test.c
+++ b/tools/testing/selftests/resctrl/cmt_test.c
@@ -82,12 +82,11 @@ void cmt_test_cleanup(void)
int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
{
- int ret, mum_resctrlfs;
+ int ret;
cache_size = 0;
- mum_resctrlfs = 1;
- ret = remount_resctrlfs(mum_resctrlfs);
+ ret = remount_resctrlfs(true);
if (ret)
return ret;
@@ -118,7 +117,7 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
.ctrlgrp = "c1",
.mongrp = "m1",
.cpu_no = cpu_no,
- .mum_resctrlfs = 0,
+ .mum_resctrlfs = false,
.filename = RESULT_FILE_NAME,
.mask = ~(long_mask << n) & long_mask,
.span = cache_size * n / count_of_bits,
diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
index 1a1bdb6180cf..7c2a5668e215 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -149,7 +149,7 @@ int mba_schemata_change(int cpu_no, char *bw_report, char **benchmark_cmd)
.ctrlgrp = "c1",
.mongrp = "m1",
.cpu_no = cpu_no,
- .mum_resctrlfs = 1,
+ .mum_resctrlfs = true,
.filename = RESULT_FILE_NAME,
.bw_report = bw_report,
.setup = mba_setup
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index 4f85cbbfd037..b7f53c000252 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -122,7 +122,7 @@ int mbm_bw_change(int span, int cpu_no, char *bw_report, char **benchmark_cmd)
.mongrp = "m1",
.span = span,
.cpu_no = cpu_no,
- .mum_resctrlfs = 1,
+ .mum_resctrlfs = true,
.filename = RESULT_FILE_NAME,
.bw_report = bw_report,
.setup = mbm_setup
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index 4f0976f12634..2bd593d7661f 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -62,7 +62,7 @@ struct resctrl_val_param {
char mongrp[64];
int cpu_no;
unsigned long span;
- int mum_resctrlfs;
+ bool mum_resctrlfs;
char filename[64];
char *bw_report;
unsigned long mask;
--
2.30.2
On Wed, 8 Feb 2023, Ilpo J?rvinen wrote:
> From: "Signed-off-by: Fenghua Yu" <[email protected]>
I seem to have managed to do a little copy paste error there. I can
resubmit the series if needed.
--
i.
> CBM_MASK_PATH is actually the path to resctrl/info, so change the macro
> name to correctly indicate what it represents.
>
> Signed-off-by: Fenghua Yu <[email protected]>
> ---
> tools/testing/selftests/resctrl/resctrl.h | 2 +-
> tools/testing/selftests/resctrl/resctrlfs.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index f0ded31fb3c7..4f0976f12634 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -28,7 +28,7 @@
> #define MB (1024 * 1024)
> #define RESCTRL_PATH "/sys/fs/resctrl"
> #define PHYS_ID_PATH "/sys/devices/system/cpu/cpu"
> -#define CBM_MASK_PATH "/sys/fs/resctrl/info"
> +#define INFO_PATH "/sys/fs/resctrl/info"
> #define L3_PATH "/sys/fs/resctrl/info/L3"
> #define MB_PATH "/sys/fs/resctrl/info/MB"
> #define L3_MON_PATH "/sys/fs/resctrl/info/L3_MON"
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index 6f543e470ad4..cc6cf49e3129 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -210,7 +210,7 @@ int get_cbm_mask(char *cache_type, char *cbm_mask)
> if (!cbm_mask)
> return -1;
>
> - sprintf(cbm_mask_path, "%s/%s/cbm_mask", CBM_MASK_PATH, cache_type);
> + sprintf(cbm_mask_path, "%s/%s/cbm_mask", INFO_PATH, cache_type);
>
> fp = fopen(cbm_mask_path, "r");
> if (!fp) {
>
Hi Ilpo,
On 2/8/2023 1:40 AM, Ilpo Järvinen wrote:
> initialize_llc_perf() unconditionally does return 0 so no point in
> having it's return type as int. Hence, change it's return type from int
> to void.
>
Thank you very much for contributing to resctrl. As a new resctrl
contributor I would like to share that resctrl follows the x86
style guidance and to be consistent this is for the most part
true for the resctrl selftest area.
To that point, changelogs are easier to read if the context, problem,
and solution are clearly separated by placing them in separate
paragraphs. See "Changelog" in
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/maintainer-tip.rst
Please compare to a changelog as follows:
"
initialize_llc_perf() unconditionally returns 0.
initialize_llc_perf() performs memory initialization, none of
which can fail.
Change the return type from int to void to accurately
reflect that there is no checking of return value needed.
"
For such a small change as this, the changelog could possibly be
simplified but the context, problem, and solution should always
be clear to the reader. This may be significant changelog feedback
for such a small change. This is because it is your first patch to
this area and my goal is to point out the style that will help
your future resctrl contributions to have the pattern that
x86 maintainers expect.
Reinette
Hi Ilpo,
The subject is "Don't hard code divisor ..." yet it seems to me
that the hard coding persists. It is just changed from a magic
constant to a macro.
On 2/8/2023 1:40 AM, Ilpo Järvinen wrote:
> From: Fenghua Yu <[email protected]>
>
> Presently, while calculating MBM results, the divisor is hard coded as 4.
"Presently" can be removed. Here and in the rest of the series the usage of
"presently" and "currently" can usually be removed to improve clarity.
> It's hard coded to 4 because "NUM_OF_RUNS" is defined as 5 and the test
> does not count first result and hence 4. So, instead of hard coding the
> value to 4, change it to NUM_OF_RUNS - 1.
Are there any plans surrounding using struct resctrl_val_param::num_of_runs
instead?
>
> Signed-off-by: Fenghua Yu <[email protected]>
Missing your Signed-off-by?
> ---
> tools/testing/selftests/resctrl/mbm_test.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
> index 8392e5c55ed0..4f85cbbfd037 100644
> --- a/tools/testing/selftests/resctrl/mbm_test.c
> +++ b/tools/testing/selftests/resctrl/mbm_test.c
> @@ -31,8 +31,8 @@ show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc, int span)
> sum_bw_resc += bw_resc[runs];
> }
>
> - avg_bw_imc = sum_bw_imc / 4;
> - avg_bw_resc = sum_bw_resc / 4;
> + avg_bw_imc = sum_bw_imc / (NUM_OF_RUNS - 1);
> + avg_bw_resc = sum_bw_resc / (NUM_OF_RUNS - 1);
> avg_diff = (float)labs(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
> avg_diff_per = (int)(avg_diff * 100);
>
Reinette
Hi Ilpo,
On 2/8/2023 6:04 AM, Ilpo Järvinen wrote:
> On Wed, 8 Feb 2023, Ilpo Järvinen wrote:
>
>> From: "Signed-off-by: Fenghua Yu" <[email protected]>
> I seem to have managed to do a little copy paste error there. I can
> resubmit the series if needed.
Yes please.
>
> -- i.
>> CBM_MASK_PATH is actually the path to resctrl/info, so change the macro
>> name to correctly indicate what it represents.
>>
>> Signed-off-by: Fenghua Yu <[email protected]>
Missing your Signed-off-by?
Reinette
On Mon, 13 Feb 2023, Reinette Chatre wrote:
> Hi Ilpo,
>
> The subject is "Don't hard code divisor ..." yet it seems to me
> that the hard coding persists. It is just changed from a magic
> constant to a macro.
Yeah, it was a bad wording.
> On 2/8/2023 1:40 AM, Ilpo Järvinen wrote:
> > From: Fenghua Yu <[email protected]>
> >
> > Presently, while calculating MBM results, the divisor is hard coded as 4.
>
> "Presently" can be removed. Here and in the rest of the series the usage of
> "presently" and "currently" can usually be removed to improve clarity.
>
> > It's hard coded to 4 because "NUM_OF_RUNS" is defined as 5 and the test
> > does not count first result and hence 4. So, instead of hard coding the
> > value to 4, change it to NUM_OF_RUNS - 1.
>
> Are there any plans surrounding using struct resctrl_val_param::num_of_runs
> instead?
Actually no.
What I'd want to do is that the functions which call these result
calculator functions would specify the number of tests they passed
into the result calculator. It seems safer because the results are read
back from a file which could have changed (or got deleted due to an
ipc bug prematurely cleaning up the file) and would better take account
those cases where the first value is skipped when reading the results.
I think I'll drop this patch for now.
--
i.
Hi Ilpo,
On 2/14/2023 2:00 AM, Ilpo Järvinen wrote:
> On Mon, 13 Feb 2023, Reinette Chatre wrote:
>> On 2/8/2023 1:40 AM, Ilpo Järvinen wrote:
>>> From: Fenghua Yu <[email protected]>
>>>
>>> Presently, while calculating MBM results, the divisor is hard coded as 4.
>>
>> "Presently" can be removed. Here and in the rest of the series the usage of
>> "presently" and "currently" can usually be removed to improve clarity.
>>
>>> It's hard coded to 4 because "NUM_OF_RUNS" is defined as 5 and the test
>>> does not count first result and hence 4. So, instead of hard coding the
>>> value to 4, change it to NUM_OF_RUNS - 1.
>>
>> Are there any plans surrounding using struct resctrl_val_param::num_of_runs
>> instead?
>
> Actually no.
>
> What I'd want to do is that the functions which call these result
> calculator functions would specify the number of tests they passed
> into the result calculator. It seems safer because the results are read
Would it not simplify things to pass the test parameters (struct resctrl_val_param)
to the result calculator? That contains the number of tests run and would
reign in the hard coding.
> back from a file which could have changed (or got deleted due to an
> ipc bug prematurely cleaning up the file) and would better take account
> those cases where the first value is skipped when reading the results.
>
This sounds good. Thank you.
Reinette