[PATCH 0/4] selftests/resctrl: Small cleanups

[PATCH 0/4] selftests/resctrl: Small cleanups

[Ilpo Järvinen]

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

[PATCH 1/4] selftests/resctrl: Change initialize_llc_perf() return type to void

[Ilpo Järvinen]

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 <fenghua.yu@intel.com>
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 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

Re: [PATCH 1/4] selftests/resctrl: Change initialize_llc_perf() return type to void

[Reinette Chatre]

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

[PATCH 2/4] selftests/resctrl: Don't hard code divisor in MBM results

[Ilpo Järvinen]

From: Fenghua Yu <fenghua.yu@intel.com>

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 <fenghua.yu@intel.com>
---
 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

Re: [PATCH 2/4] selftests/resctrl: Don't hard code divisor in MBM results

[Reinette Chatre]

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 <fenghua.yu@intel.com>
> 
> 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 <fenghua.yu@intel.com>

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

Re: [PATCH 2/4] selftests/resctrl: Don't hard code divisor in MBM results

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1345 bytes --]

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 <fenghua.yu@intel.com>
> > 
> > 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.

Re: [PATCH 2/4] selftests/resctrl: Don't hard code divisor in MBM results

[Reinette Chatre]

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 <fenghua.yu@intel.com>
>>>
>>> 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

[PATCH 3/4] selftests/resctrl: Change name from CBM_MASK_PATH to INFO_PATH

[Ilpo Järvinen]

From: "Signed-off-by: Fenghua Yu" <fenghua.yu@intel.com>

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 <fenghua.yu@intel.com>
---
 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

Re: [PATCH 3/4] selftests/resctrl: Change name from CBM_MASK_PATH to INFO_PATH

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1753 bytes --]

On Wed, 8 Feb 2023, Ilpo Järvinen wrote:

> From: "Signed-off-by: Fenghua Yu" <fenghua.yu@intel.com>

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 <fenghua.yu@intel.com>
> ---
>  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) {
> 

Re: [PATCH 3/4] selftests/resctrl: Change name from CBM_MASK_PATH to INFO_PATH

[Reinette Chatre]

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" <fenghua.yu@intel.com>
> 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 <fenghua.yu@intel.com>

Missing your Signed-off-by?


Reinette

[PATCH 4/4] selftests/resctrl: Use remount_resctrlfs() consistently with boolean

[Ilpo Järvinen]

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 <fenghua.yu@intel.com>
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 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