[PATCH v4 00/19] selftests/resctrl: Fixes and cleanups

[PATCH v4 00/19] selftests/resctrl: Fixes and cleanups

[Ilpo Järvinen]

Here is a series with some fixes and cleanups to resctrl selftests.
Only has a minor change in code ordering in main() compared with v3.

v4:
- Move resctrlfs (unconditional) umount after resctrl fs support check

v3:
- Don't include rewritten CAT test into this series!
- Tweak wildcard style in Makefile
- Fix many changelog typos, remove some wrong claims, and generally
  improve them.
- Add fix to PARENT_EXIT() to unmount resctrl FS
- Add unmounting resctrl FS before starting any tests
- Add fix for buf leak
- Add fix for perf fd closing
- Split mount/remount/umount patches differently
- Use size_t and %zu for span
- Keep MBM print as MB, only internally use span in bytes
- Drop start_buf global from fill_buf


v2 (was sent with CAT test rewrite which is no longer included in v3):
- Rebased on top of next to solve the conflicts
- Added 2 patches related to resctrl FS mount/umount (fix + cleanup)
- Consistently use "alloc" in cache_alloc_size()
- CAT test error handling tweaked
- Remove a spurious newline change from the CAT patch
- Small improvements to changelogs

Ilpo Järvinen (19):
  selftests/resctrl: Add resctrl.h into build deps
  selftests/resctrl: Don't leak buffer in fill_cache()
  selftests/resctrl: Unmount resctrl FS if child fails to run benchmark
  selftests/resctrl: Close perf value read fd on errors
  selftests/resctrl: Unmount resctrl FS before starting the first test
  selftests/resctrl: Move resctrl FS mount/umount to higher level
  selftests/resctrl: Refactor remount_resctrl(bool mum_resctrlfs) to
    mount_resctrl()
  selftests/resctrl: Remove mum_resctrlfs from struct resctrl_val_param
  selftests/resctrl: Convert span to size_t
  selftests/resctrl: Express span internally in bytes
  selftests/resctrl: Remove duplicated preparation for span arg
  selftests/resctrl: Remove "malloc_and_init_memory" param from
    run_fill_buf()
  selftests/resctrl: Remove unnecessary startptr global from fill_buf
  selftests/resctrl: Improve parameter consistency in fill_buf
  selftests/resctrl: Don't pass test name to fill_buf
  selftests/resctrl: Don't use variable argument list for ->setup()
  selftests/resctrl: Move CAT/CMT test global vars to function they are
    used in
  selftests/resctrl: Pass the real number of tests to show_cache_info()
  selftests/resctrl: Remove test type checks from cat_val()

 tools/testing/selftests/resctrl/Makefile      |  2 +-
 tools/testing/selftests/resctrl/cache.c       | 64 +++++++-------
 tools/testing/selftests/resctrl/cat_test.c    | 28 ++----
 tools/testing/selftests/resctrl/cmt_test.c    | 29 ++-----
 tools/testing/selftests/resctrl/fill_buf.c    | 87 +++++++------------
 tools/testing/selftests/resctrl/mba_test.c    |  9 +-
 tools/testing/selftests/resctrl/mbm_test.c    | 17 ++--
 tools/testing/selftests/resctrl/resctrl.h     | 17 ++--
 .../testing/selftests/resctrl/resctrl_tests.c | 82 +++++++++++------
 tools/testing/selftests/resctrl/resctrl_val.c |  7 +-
 tools/testing/selftests/resctrl/resctrlfs.c   | 57 ++++++------
 11 files changed, 169 insertions(+), 230 deletions(-)

-- 
2.30.2

[PATCH v4 01/19] selftests/resctrl: Add resctrl.h into build deps

[Ilpo Järvinen]

Makefile only lists *.c as build dependencies for the resctrl_tests
executable which excludes resctrl.h.

Add *.h to wildcard() to include resctrl.h.

Fixes: 591a6e8588fc ("selftests/resctrl: Add basic resctrl file system operations and data")
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/resctrl/Makefile b/tools/testing/selftests/resctrl/Makefile
index 73d53257df42..5073dbc96125 100644
--- a/tools/testing/selftests/resctrl/Makefile
+++ b/tools/testing/selftests/resctrl/Makefile
@@ -7,4 +7,4 @@ TEST_GEN_PROGS := resctrl_tests
 
 include ../lib.mk
 
-$(OUTPUT)/resctrl_tests: $(wildcard *.c)
+$(OUTPUT)/resctrl_tests: $(wildcard *.[ch])
-- 
2.30.2

Re: [PATCH v4 01/19] selftests/resctrl: Add resctrl.h into build deps

[Reinette Chatre]

Hi Ilpo,

On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> Makefile only lists *.c as build dependencies for the resctrl_tests
> executable which excludes resctrl.h.
> 
> Add *.h to wildcard() to include resctrl.h.
> 
> Fixes: 591a6e8588fc ("selftests/resctrl: Add basic resctrl file system operations and data")
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Thank you.

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

[PATCH v4 02/19] selftests/resctrl: Don't leak buffer in fill_cache()

[Ilpo Järvinen]

The error path in fill_cache() does return before the allocated buffer
is freed leaking the buffer.

The leak was introduced when fill_cache_read() started to return errors
in c7b607fa9325 ("selftests/resctrl: Fix null pointer dereference on
open failed"), before that both fill functions always returned 0.

Move free() earlier to prevent the mem leak.

Fixes: c7b607fa9325 ("selftests/resctrl: Fix null pointer dereference on open failed")
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/fill_buf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c
index 341cc93ca84c..3b328c844896 100644
--- a/tools/testing/selftests/resctrl/fill_buf.c
+++ b/tools/testing/selftests/resctrl/fill_buf.c
@@ -177,12 +177,13 @@ fill_cache(unsigned long long buf_size, int malloc_and_init, int memflush,
 	else
 		ret = fill_cache_write(start_ptr, end_ptr, resctrl_val);
 
+	free(startptr);
+
 	if (ret) {
 		printf("\n Error in fill cache read/write...\n");
 		return -1;
 	}
 
-	free(startptr);
 
 	return 0;
 }
-- 
2.30.2

Re: [PATCH v4 02/19] selftests/resctrl: Don't leak buffer in fill_cache()

[Reinette Chatre]

Hi Ilpo,

On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> The error path in fill_cache() does return before the allocated buffer
> is freed leaking the buffer.
> 
> The leak was introduced when fill_cache_read() started to return errors
> in c7b607fa9325 ("selftests/resctrl: Fix null pointer dereference on

Could you please prefix the sha with "commit" to get a clean
checkpatch.pl check?

> open failed"), before that both fill functions always returned 0.
> 
> Move free() earlier to prevent the mem leak.
> 
> Fixes: c7b607fa9325 ("selftests/resctrl: Fix null pointer dereference on open failed")
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Thank you for catching this.

With the changelog update:

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

[PATCH v4 03/19] selftests/resctrl: Unmount resctrl FS if child fails to run benchmark

[Ilpo Järvinen]

A child calls PARENT_EXIT() when it fails to run a benchmark to kill
the parent process. PARENT_EXIT() lacks unmount for the resctrl FS and
the parent won't be there to unmount it either after it gets killed.

Add the resctrl FS unmount also to PARENT_EXIT().

Fixes: 591a6e8588fc ("selftests/resctrl: Add basic resctrl file system operations and data")
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/resctrl.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index 87e39456dee0..f455f0b7e314 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -43,6 +43,7 @@
 	do {					\
 		perror(err_msg);		\
 		kill(ppid, SIGKILL);		\
+		umount_resctrlfs();		\
 		exit(EXIT_FAILURE);		\
 	} while (0)
 
-- 
2.30.2

Re: [PATCH v4 03/19] selftests/resctrl: Unmount resctrl FS if child fails to run benchmark

[Reinette Chatre]

Hi Ilpo,

On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> A child calls PARENT_EXIT() when it fails to run a benchmark to kill
> the parent process. PARENT_EXIT() lacks unmount for the resctrl FS and
> the parent won't be there to unmount it either after it gets killed.
> 
> Add the resctrl FS unmount also to PARENT_EXIT().
> 
> Fixes: 591a6e8588fc ("selftests/resctrl: Add basic resctrl file system operations and data")
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Thank you.

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

[PATCH v4 04/19] selftests/resctrl: Close perf value read fd on errors

[Ilpo Järvinen]

Perf event fd (fd_lm) is not closed on some error paths.

Always close fd_lm in get_llc_perf() and add close into an error
handling block in cat_val().

Fixes: 790bf585b0ee ("selftests/resctrl: Add Cache Allocation Technology (CAT) selftest")
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/cache.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
index 8a4fe8693be6..ced47b445d1e 100644
--- a/tools/testing/selftests/resctrl/cache.c
+++ b/tools/testing/selftests/resctrl/cache.c
@@ -87,21 +87,20 @@ static int reset_enable_llc_perf(pid_t pid, int cpu_no)
 static int get_llc_perf(unsigned long *llc_perf_miss)
 {
 	__u64 total_misses;
+	int ret;
 
 	/* Stop counters after one span to get miss rate */
 
 	ioctl(fd_lm, PERF_EVENT_IOC_DISABLE, 0);
 
-	if (read(fd_lm, &rf_cqm, sizeof(struct read_format)) == -1) {
+	ret = read(fd_lm, &rf_cqm, sizeof(struct read_format));
+	close(fd_lm);
+	if (ret == -1) {
 		perror("Could not get llc misses through perf");
-
 		return -1;
 	}
 
 	total_misses = rf_cqm.values[0].value;
-
-	close(fd_lm);
-
 	*llc_perf_miss = total_misses;
 
 	return 0;
@@ -253,6 +252,7 @@ int cat_val(struct resctrl_val_param *param)
 					 memflush, operation, resctrl_val)) {
 				fprintf(stderr, "Error-running fill buffer\n");
 				ret = -1;
+				close(fd_lm);
 				break;
 			}
 
-- 
2.30.2

Re: [PATCH v4 04/19] selftests/resctrl: Close perf value read fd on errors

[Reinette Chatre]

Hi Ilpo,

On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> Perf event fd (fd_lm) is not closed on some error paths.
> 
> Always close fd_lm in get_llc_perf() and add close into an error
> handling block in cat_val().
> 
> Fixes: 790bf585b0ee ("selftests/resctrl: Add Cache Allocation Technology (CAT) selftest")
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  tools/testing/selftests/resctrl/cache.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
> index 8a4fe8693be6..ced47b445d1e 100644
> --- a/tools/testing/selftests/resctrl/cache.c
> +++ b/tools/testing/selftests/resctrl/cache.c
> @@ -87,21 +87,20 @@ static int reset_enable_llc_perf(pid_t pid, int cpu_no)
>  static int get_llc_perf(unsigned long *llc_perf_miss)
>  {
>  	__u64 total_misses;
> +	int ret;
>  
>  	/* Stop counters after one span to get miss rate */
>  
>  	ioctl(fd_lm, PERF_EVENT_IOC_DISABLE, 0);
>  
> -	if (read(fd_lm, &rf_cqm, sizeof(struct read_format)) == -1) {
> +	ret = read(fd_lm, &rf_cqm, sizeof(struct read_format));
> +	close(fd_lm);
> +	if (ret == -1) {
>  		perror("Could not get llc misses through perf");
> -
>  		return -1;
>  	}
>  
>  	total_misses = rf_cqm.values[0].value;
> -
> -	close(fd_lm);
> -
>  	*llc_perf_miss = total_misses;
>  
>  	return 0;
> @@ -253,6 +252,7 @@ int cat_val(struct resctrl_val_param *param)
>  					 memflush, operation, resctrl_val)) {
>  				fprintf(stderr, "Error-running fill buffer\n");
>  				ret = -1;
> +				close(fd_lm);
>  				break;
>  			}
>  

Instead of fixing these existing patterns I think it would make the code
easier to understand and maintain if it is made symmetrical.
Having the perf event fd opened in one place but its close()
scattered elsewhere has the potential for confusion and making later
mistakes easy to miss.

What if perf event fd is closed in a new "disable_llc_perf()" that
is matched with "reset_enable_llc_perf()" and called
from cat_val()?

I think this raises another issue with the test trickery where
measure_cache_vals() has some assumptions about state based on the
test name.

Reinette

Re: [PATCH v4 04/19] selftests/resctrl: Close perf value read fd on errors

[Ilpo Järvinen]

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

On Thu, 13 Jul 2023, Reinette Chatre wrote:

> Hi Ilpo,
> 
> On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> > Perf event fd (fd_lm) is not closed on some error paths.
> > 
> > Always close fd_lm in get_llc_perf() and add close into an error
> > handling block in cat_val().
> > 
> > Fixes: 790bf585b0ee ("selftests/resctrl: Add Cache Allocation Technology (CAT) selftest")
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> >  tools/testing/selftests/resctrl/cache.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
> > index 8a4fe8693be6..ced47b445d1e 100644
> > --- a/tools/testing/selftests/resctrl/cache.c
> > +++ b/tools/testing/selftests/resctrl/cache.c
> > @@ -87,21 +87,20 @@ static int reset_enable_llc_perf(pid_t pid, int cpu_no)
> >  static int get_llc_perf(unsigned long *llc_perf_miss)
> >  {
> >  	__u64 total_misses;
> > +	int ret;
> >  
> >  	/* Stop counters after one span to get miss rate */
> >  
> >  	ioctl(fd_lm, PERF_EVENT_IOC_DISABLE, 0);
> >  
> > -	if (read(fd_lm, &rf_cqm, sizeof(struct read_format)) == -1) {
> > +	ret = read(fd_lm, &rf_cqm, sizeof(struct read_format));
> > +	close(fd_lm);
> > +	if (ret == -1) {
> >  		perror("Could not get llc misses through perf");
> > -
> >  		return -1;
> >  	}
> >  
> >  	total_misses = rf_cqm.values[0].value;
> > -
> > -	close(fd_lm);
> > -
> >  	*llc_perf_miss = total_misses;
> >  
> >  	return 0;
> > @@ -253,6 +252,7 @@ int cat_val(struct resctrl_val_param *param)
> >  					 memflush, operation, resctrl_val)) {
> >  				fprintf(stderr, "Error-running fill buffer\n");
> >  				ret = -1;
> > +				close(fd_lm);
> >  				break;
> >  			}
> >  
> 
> Instead of fixing these existing patterns I think it would make the code
> easier to understand and maintain if it is made symmetrical.
> Having the perf event fd opened in one place but its close()
> scattered elsewhere has the potential for confusion and making later
> mistakes easy to miss.
> 
> What if perf event fd is closed in a new "disable_llc_perf()" that
> is matched with "reset_enable_llc_perf()" and called
> from cat_val()?
> 
> I think this raises another issue with the test trickery where
> measure_cache_vals() has some assumptions about state based on the
> test name.

I very much agree on the principle here, and thus I already have created 
patches which will do a major cleanup on this area. The cleaned-up code 
has pe_fd local var to cat_val() and handles closing it in cat_val() with 
the usual patterns.

However, the patch is currently resides post L3 CAT test rewrite. 
Backporting the cleanups/refactors into this series would require 
considerable effort due to how convoluted all those n-step cleanup patches 
and L3 CAT test rewrite are in this area. There's just very much to 
cleanup here and L3 rewrite will touch the same areas so its a net 
full of conflicts.

Do you want me to spend the effort to backport them into this series 
(I expect will take some time)?

I currently have these items pending besides this series (in order):
- L3 CAT test rewrite and its preparatory patches
- More cleanups (including the pe_fd cleanup)
- New generalized test framework
- L2 CAT test

-- 
 i.

Re: [PATCH v4 04/19] selftests/resctrl: Close perf value read fd on errors

[Reinette Chatre]

Hi Ilpo,

On 7/14/2023 3:35 AM, Ilpo Järvinen wrote:
> On Thu, 13 Jul 2023, Reinette Chatre wrote:
>> On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
>>> Perf event fd (fd_lm) is not closed on some error paths.
>>>
>>> Always close fd_lm in get_llc_perf() and add close into an error
>>> handling block in cat_val().
>>>
>>> Fixes: 790bf585b0ee ("selftests/resctrl: Add Cache Allocation Technology (CAT) selftest")
>>> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>>> ---
>>>  tools/testing/selftests/resctrl/cache.c | 10 +++++-----
>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
>>> index 8a4fe8693be6..ced47b445d1e 100644
>>> --- a/tools/testing/selftests/resctrl/cache.c
>>> +++ b/tools/testing/selftests/resctrl/cache.c
>>> @@ -87,21 +87,20 @@ static int reset_enable_llc_perf(pid_t pid, int cpu_no)
>>>  static int get_llc_perf(unsigned long *llc_perf_miss)
>>>  {
>>>  	__u64 total_misses;
>>> +	int ret;
>>>  
>>>  	/* Stop counters after one span to get miss rate */
>>>  
>>>  	ioctl(fd_lm, PERF_EVENT_IOC_DISABLE, 0);
>>>  
>>> -	if (read(fd_lm, &rf_cqm, sizeof(struct read_format)) == -1) {
>>> +	ret = read(fd_lm, &rf_cqm, sizeof(struct read_format));
>>> +	close(fd_lm);
>>> +	if (ret == -1) {
>>>  		perror("Could not get llc misses through perf");
>>> -
>>>  		return -1;
>>>  	}
>>>  
>>>  	total_misses = rf_cqm.values[0].value;
>>> -
>>> -	close(fd_lm);
>>> -
>>>  	*llc_perf_miss = total_misses;
>>>  
>>>  	return 0;
>>> @@ -253,6 +252,7 @@ int cat_val(struct resctrl_val_param *param)
>>>  					 memflush, operation, resctrl_val)) {
>>>  				fprintf(stderr, "Error-running fill buffer\n");
>>>  				ret = -1;
>>> +				close(fd_lm);
>>>  				break;
>>>  			}
>>>  
>>
>> Instead of fixing these existing patterns I think it would make the code
>> easier to understand and maintain if it is made symmetrical.
>> Having the perf event fd opened in one place but its close()
>> scattered elsewhere has the potential for confusion and making later
>> mistakes easy to miss.
>>
>> What if perf event fd is closed in a new "disable_llc_perf()" that
>> is matched with "reset_enable_llc_perf()" and called
>> from cat_val()?
>>
>> I think this raises another issue with the test trickery where
>> measure_cache_vals() has some assumptions about state based on the
>> test name.
> 
> I very much agree on the principle here, and thus I already have created 
> patches which will do a major cleanup on this area. The cleaned-up code 
> has pe_fd local var to cat_val() and handles closing it in cat_val() with 
> the usual patterns.
> 
> However, the patch is currently resides post L3 CAT test rewrite. 
> Backporting the cleanups/refactors into this series would require 
> considerable effort due to how convoluted all those n-step cleanup patches 
> and L3 CAT test rewrite are in this area. There's just very much to 
> cleanup here and L3 rewrite will touch the same areas so its a net 
> full of conflicts.
> 
> Do you want me to spend the effort to backport them into this series 
> (I expect will take some time)?

Considering the "Fixes" tag, having a smaller fix that can easily
be backported would be ideal so I am ok with deferring a bigger
rework.

I do think this fix can be made more robust with a couple of small
changes that should not introduce significant conflicts:
* initialize fd_lm to -1 
* do not close() fd_lm in get_llc_perf() but instead move its
  close() to at exit of cat_val().
* add check in get_llc_perf() that it does not attempt ioctl()
  on "fd_lm == -1" (later addition would be error checking of
  the ioctl())

> I currently have these items pending besides this series (in order):
> - L3 CAT test rewrite and its preparatory patches
> - More cleanups (including the pe_fd cleanup)
> - New generalized test framework
> - L2 CAT test

Thank you very much for taking this on.

Reinette
 

Re: [PATCH v4 04/19] selftests/resctrl: Close perf value read fd on errors

[Ilpo Järvinen]

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

On Fri, 14 Jul 2023, Reinette Chatre wrote:
> On 7/14/2023 3:35 AM, Ilpo Järvinen wrote:
> > On Thu, 13 Jul 2023, Reinette Chatre wrote:
> >> On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> >>> Perf event fd (fd_lm) is not closed on some error paths.
> >>>
> >>> Always close fd_lm in get_llc_perf() and add close into an error
> >>> handling block in cat_val().
> >>>
> >>> Fixes: 790bf585b0ee ("selftests/resctrl: Add Cache Allocation Technology (CAT) selftest")
> >>> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> >>> ---
> >>>  tools/testing/selftests/resctrl/cache.c | 10 +++++-----
> >>>  1 file changed, 5 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
> >>> index 8a4fe8693be6..ced47b445d1e 100644
> >>> --- a/tools/testing/selftests/resctrl/cache.c
> >>> +++ b/tools/testing/selftests/resctrl/cache.c
> >>> @@ -87,21 +87,20 @@ static int reset_enable_llc_perf(pid_t pid, int cpu_no)
> >>>  static int get_llc_perf(unsigned long *llc_perf_miss)
> >>>  {
> >>>  	__u64 total_misses;
> >>> +	int ret;
> >>>  
> >>>  	/* Stop counters after one span to get miss rate */
> >>>  
> >>>  	ioctl(fd_lm, PERF_EVENT_IOC_DISABLE, 0);
> >>>  
> >>> -	if (read(fd_lm, &rf_cqm, sizeof(struct read_format)) == -1) {
> >>> +	ret = read(fd_lm, &rf_cqm, sizeof(struct read_format));
> >>> +	close(fd_lm);
> >>> +	if (ret == -1) {
> >>>  		perror("Could not get llc misses through perf");
> >>> -
> >>>  		return -1;
> >>>  	}
> >>>  
> >>>  	total_misses = rf_cqm.values[0].value;
> >>> -
> >>> -	close(fd_lm);
> >>> -
> >>>  	*llc_perf_miss = total_misses;
> >>>  
> >>>  	return 0;
> >>> @@ -253,6 +252,7 @@ int cat_val(struct resctrl_val_param *param)
> >>>  					 memflush, operation, resctrl_val)) {
> >>>  				fprintf(stderr, "Error-running fill buffer\n");
> >>>  				ret = -1;
> >>> +				close(fd_lm);
> >>>  				break;
> >>>  			}
> >>>  
> >>
> >> Instead of fixing these existing patterns I think it would make the code
> >> easier to understand and maintain if it is made symmetrical.
> >> Having the perf event fd opened in one place but its close()
> >> scattered elsewhere has the potential for confusion and making later
> >> mistakes easy to miss.
> >>
> >> What if perf event fd is closed in a new "disable_llc_perf()" that
> >> is matched with "reset_enable_llc_perf()" and called
> >> from cat_val()?
> >>
> >> I think this raises another issue with the test trickery where
> >> measure_cache_vals() has some assumptions about state based on the
> >> test name.
> > 
> > I very much agree on the principle here, and thus I already have created 
> > patches which will do a major cleanup on this area. The cleaned-up code 
> > has pe_fd local var to cat_val() and handles closing it in cat_val() with 
> > the usual patterns.
> > 
> > However, the patch is currently resides post L3 CAT test rewrite. 
> > Backporting the cleanups/refactors into this series would require 
> > considerable effort due to how convoluted all those n-step cleanup patches 
> > and L3 CAT test rewrite are in this area. There's just very much to 
> > cleanup here and L3 rewrite will touch the same areas so its a net 
> > full of conflicts.
> > 
> > Do you want me to spend the effort to backport them into this series 
> > (I expect will take some time)?
> 
> Considering the "Fixes" tag, having a smaller fix that can easily
> be backported would be ideal so I am ok with deferring a bigger
> rework.
> 
> I do think this fix can be made more robust with a couple of small
> changes that should not introduce significant conflicts:
> * initialize fd_lm to -1 

> * do not close() fd_lm in get_llc_perf() but instead move its
>   close() to at exit of cat_val().

I changed the test to only close the fd in cat_val() which is the 
direction the later refactor/cleanup changes (not in this series) was 
moving anyway.

> * add check in get_llc_perf() that it does not attempt ioctl()
>   on "fd_lm == -1" (later addition would be error checking of
>   the ioctl())

The other two things suggested seem unnecessary and I've not implemented 
them, I don't thinkg fd_lm can be -1 at ioctl(). Given this code is going 
to be replaced soonish, putting any extra "safety" effort into it now 
seems waste of time.

-- 
 i.

Re: [PATCH v4 04/19] selftests/resctrl: Close perf value read fd on errors

[Reinette Chatre]

Hi Ilpo,

On 7/17/2023 6:05 AM, Ilpo Järvinen wrote:
> On Fri, 14 Jul 2023, Reinette Chatre wrote:
>> * add check in get_llc_perf() that it does not attempt ioctl()
>>   on "fd_lm == -1" (later addition would be error checking of
>>   the ioctl())
> 
> The other two things suggested seem unnecessary and I've not implemented 
> them, I don't thinkg fd_lm can be -1 at ioctl(). Given this code is going 
> to be replaced soonish, putting any extra "safety" effort into it now 
> seems waste of time.

Yes, this suggestion was indeed to make the code more robust. I
certainly do not want to waste your time. Please keep in mind 
when you respond that I do not have insight into the reworks
you are still planning. 

Reinette
 

[PATCH v4 05/19] selftests/resctrl: Unmount resctrl FS before starting the first test

[Ilpo Järvinen]

Resctrl FS mount/remount/umount code is hard to track. Better approach
is to use mount/umount pair for each test but that assumes resctrl FS
is not mounted beforehand.

Change umount_resctrlfs() so that it can unmount resctrl FS from any
path, and enable further simplifications into mount/remount/umount
logic by unmounting resctrl FS at the start if a pre-existing
mountpoint is found.

Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/resctrl_tests.c | 2 ++
 tools/testing/selftests/resctrl/resctrlfs.c     | 6 ++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index 9b9751206e1c..b1b2d28b52f7 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -250,6 +250,8 @@ int main(int argc, char **argv)
 	if (!check_resctrlfs_support())
 		return ksft_exit_skip("resctrl FS does not exist. Enable X86_CPU_RESCTRL config option.\n");
 
+	umount_resctrlfs();
+
 	filter_dmesg();
 
 	ksft_set_plan(tests ? : 4);
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index fb00245dee92..23f75aeaa198 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -82,10 +82,12 @@ int remount_resctrlfs(bool mum_resctrlfs)
 
 int umount_resctrlfs(void)
 {
-	if (find_resctrl_mount(NULL))
+	char mountpoint[256];
+
+	if (find_resctrl_mount(mountpoint))
 		return 0;
 
-	if (umount(RESCTRL_PATH)) {
+	if (umount(mountpoint)) {
 		perror("# Unable to umount resctrl");
 
 		return errno;
-- 
2.30.2

Re: [PATCH v4 05/19] selftests/resctrl: Unmount resctrl FS before starting the first test

[Reinette Chatre]

Hi Ilpo,

On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> Resctrl FS mount/remount/umount code is hard to track. Better approach
> is to use mount/umount pair for each test but that assumes resctrl FS
> is not mounted beforehand.
> 
> Change umount_resctrlfs() so that it can unmount resctrl FS from any
> path, and enable further simplifications into mount/remount/umount
> logic by unmounting resctrl FS at the start if a pre-existing
> mountpoint is found.
> 
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  tools/testing/selftests/resctrl/resctrl_tests.c | 2 ++
>  tools/testing/selftests/resctrl/resctrlfs.c     | 6 ++++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index 9b9751206e1c..b1b2d28b52f7 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -250,6 +250,8 @@ int main(int argc, char **argv)
>  	if (!check_resctrlfs_support())
>  		return ksft_exit_skip("resctrl FS does not exist. Enable X86_CPU_RESCTRL config option.\n");
>  
> +	umount_resctrlfs();
> +

umount_resctrlfs() can fail. I think there should be error checking
here and no tests should be run if resctrl cannot be unmounted since
the hardware and resctrl state is not known (without more effort).

>  	filter_dmesg();
>  
>  	ksft_set_plan(tests ? : 4);
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index fb00245dee92..23f75aeaa198 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -82,10 +82,12 @@ int remount_resctrlfs(bool mum_resctrlfs)
>  
>  int umount_resctrlfs(void)
>  {
> -	if (find_resctrl_mount(NULL))
> +	char mountpoint[256];
> +
> +	if (find_resctrl_mount(mountpoint))
>  		return 0;

It looks like the intent is to return 0 if find_resctrl_mount()
returns -ENOENT. It is not clear to me that it should also
return 0 if find_resctrl_mount() returns -ENXIO.

>  
> -	if (umount(RESCTRL_PATH)) {
> +	if (umount(mountpoint)) {
>  		perror("# Unable to umount resctrl");
>  
>  		return errno;

Reinette

[PATCH v4 06/19] selftests/resctrl: Move resctrl FS mount/umount to higher level

[Ilpo Järvinen]

A few places currently lack umounting resctrl FS on error paths:
  - cmt_resctrl_val() has multiple error paths with direct return.
  - cat_perf_miss_val() has multiple error paths with direct return.
In addition, validate_resctrl_feature_request() is called by
run_mbm_test() and run_mba_test(). Neither MBA nor MBM test tries to
umount resctrl FS.

Each and every test does require resctrl FS to be present already for
feature check. Thus, it makes sense to just mount it on higher level in
resctrl_tests.c and properly pair it with umount.

Move resctrl FS (re)mount/unmount into each test function in
resctrl_tests.c. Make feature validation to simply check that resctrl
FS is mounted.

Fixes: 91db4fd9019a ("selftests/resctrl: Remove duplicate codes that clear each test result file")
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/cat_test.c    |  6 ---
 tools/testing/selftests/resctrl/cmt_test.c    |  4 --
 .../testing/selftests/resctrl/resctrl_tests.c | 47 ++++++++++++++++---
 tools/testing/selftests/resctrl/resctrl_val.c |  5 --
 tools/testing/selftests/resctrl/resctrlfs.c   |  7 ++-
 5 files changed, 46 insertions(+), 23 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
index fb1443f888c4..e1c071dec1b0 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -106,10 +106,6 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
 
 	cache_size = 0;
 
-	ret = remount_resctrlfs(true);
-	if (ret)
-		return ret;
-
 	/* Get default cbm mask for L3/L2 cache */
 	ret = get_cbm_mask(cache_type, cbm_mask);
 	if (ret)
@@ -227,8 +223,6 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
 
 out:
 	cat_test_cleanup();
-	if (bm_pid)
-		umount_resctrlfs();
 
 	return ret;
 }
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index af71b2141271..426d11189a99 100644
--- a/tools/testing/selftests/resctrl/cmt_test.c
+++ b/tools/testing/selftests/resctrl/cmt_test.c
@@ -86,10 +86,6 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
 
 	cache_size = 0;
 
-	ret = remount_resctrlfs(true);
-	if (ret)
-		return ret;
-
 	if (!validate_resctrl_feature_request(CMT_STR))
 		return -1;
 
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index b1b2d28b52f7..a421d045de08 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -77,9 +77,15 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, int span,
 
 	ksft_print_msg("Starting MBM BW change ...\n");
 
+	res = remount_resctrlfs(true);
+	if (res) {
+		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
+		return;
+	}
+
 	if (!validate_resctrl_feature_request(MBM_STR) || (get_vendor() != ARCH_INTEL)) {
 		ksft_test_result_skip("Hardware does not support MBM or MBM is disabled\n");
-		return;
+		goto umount;
 	}
 
 	if (!has_ben)
@@ -88,6 +94,9 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, int span,
 	ksft_test_result(!res, "MBM: bw change\n");
 	if ((get_vendor() == ARCH_INTEL) && res)
 		ksft_print_msg("Intel MBM may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");
+
+umount:
+	umount_resctrlfs();
 }
 
 static void run_mba_test(bool has_ben, char **benchmark_cmd, int span,
@@ -97,15 +106,24 @@ static void run_mba_test(bool has_ben, char **benchmark_cmd, int span,
 
 	ksft_print_msg("Starting MBA Schemata change ...\n");
 
+	res = remount_resctrlfs(true);
+	if (res) {
+		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
+		return;
+	}
+
 	if (!validate_resctrl_feature_request(MBA_STR) || (get_vendor() != ARCH_INTEL)) {
 		ksft_test_result_skip("Hardware does not support MBA or MBA is disabled\n");
-		return;
+		goto umount;
 	}
 
 	if (!has_ben)
 		sprintf(benchmark_cmd[1], "%d", span);
 	res = mba_schemata_change(cpu_no, bw_report, benchmark_cmd);
 	ksft_test_result(!res, "MBA: schemata change\n");
+
+umount:
+	umount_resctrlfs();
 }
 
 static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no)
@@ -113,9 +131,16 @@ static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no)
 	int res;
 
 	ksft_print_msg("Starting CMT test ...\n");
+
+	res = remount_resctrlfs(true);
+	if (res) {
+		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
+		return;
+	}
+
 	if (!validate_resctrl_feature_request(CMT_STR)) {
 		ksft_test_result_skip("Hardware does not support CMT or CMT is disabled\n");
-		return;
+		goto umount;
 	}
 
 	if (!has_ben)
@@ -124,6 +149,9 @@ static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no)
 	ksft_test_result(!res, "CMT: test\n");
 	if ((get_vendor() == ARCH_INTEL) && res)
 		ksft_print_msg("Intel CMT may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");
+
+umount:
+	umount_resctrlfs();
 }
 
 static void run_cat_test(int cpu_no, int no_of_bits)
@@ -132,13 +160,22 @@ static void run_cat_test(int cpu_no, int no_of_bits)
 
 	ksft_print_msg("Starting CAT test ...\n");
 
+	res = remount_resctrlfs(true);
+	if (res) {
+		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
+		return;
+	}
+
 	if (!validate_resctrl_feature_request(CAT_STR)) {
 		ksft_test_result_skip("Hardware does not support CAT or CAT is disabled\n");
-		return;
+		goto umount;
 	}
 
 	res = cat_perf_miss_val(cpu_no, no_of_bits, "L3");
 	ksft_test_result(!res, "CAT: test\n");
+
+umount:
+	umount_resctrlfs();
 }
 
 int main(int argc, char **argv)
@@ -268,7 +305,5 @@ int main(int argc, char **argv)
 	if (cat_test)
 		run_cat_test(cpu_no, no_of_bits);
 
-	umount_resctrlfs();
-
 	ksft_finished();
 }
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index ab1eab1e7ff6..e8f1e6a59f4a 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -648,10 +648,6 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param)
 			return ret;
 	}
 
-	ret = remount_resctrlfs(param->mum_resctrlfs);
-	if (ret)
-		return ret;
-
 	/*
 	 * If benchmark wasn't successfully started by child, then child should
 	 * kill parent, so save parent's pid
@@ -788,7 +784,6 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param)
 	signal_handler_unregister();
 out:
 	kill(bm_pid, SIGKILL);
-	umount_resctrlfs();
 
 	return ret;
 }
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index 23f75aeaa198..b3a05488d360 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -613,7 +613,8 @@ char *fgrep(FILE *inf, const char *str)
  * validate_resctrl_feature_request - Check if requested feature is valid.
  * @resctrl_val:	Requested feature
  *
- * Return: True if the feature is supported, else false
+ * Return: True if the feature is supported, else false. False is also
+ *         returned if resctrl FS is not mounted.
  */
 bool validate_resctrl_feature_request(const char *resctrl_val)
 {
@@ -621,11 +622,13 @@ bool validate_resctrl_feature_request(const char *resctrl_val)
 	bool found = false;
 	char *res;
 	FILE *inf;
+	int ret;
 
 	if (!resctrl_val)
 		return false;
 
-	if (remount_resctrlfs(false))
+	ret = find_resctrl_mount(NULL);
+	if (ret)
 		return false;
 
 	if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR))) {
-- 
2.30.2

Re: [PATCH v4 06/19] selftests/resctrl: Move resctrl FS mount/umount to higher level

[Reinette Chatre]

Hi Ilpo,

On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> A few places currently lack umounting resctrl FS on error paths:
>   - cmt_resctrl_val() has multiple error paths with direct return.
>   - cat_perf_miss_val() has multiple error paths with direct return.
> In addition, validate_resctrl_feature_request() is called by
> run_mbm_test() and run_mba_test(). Neither MBA nor MBM test tries to
> umount resctrl FS.
> 
> Each and every test does require resctrl FS to be present already for
> feature check. Thus, it makes sense to just mount it on higher level in
> resctrl_tests.c and properly pair it with umount.
> 
> Move resctrl FS (re)mount/unmount into each test function in
> resctrl_tests.c. Make feature validation to simply check that resctrl
> FS is mounted.
> 
> Fixes: 91db4fd9019a ("selftests/resctrl: Remove duplicate codes that clear each test result file")

Could you please elaborate how this commit is the culprit?


Reinette

Re: [PATCH v4 06/19] selftests/resctrl: Move resctrl FS mount/umount to higher level

[Ilpo Järvinen]

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

On Thu, 13 Jul 2023, Reinette Chatre wrote:

> Hi Ilpo,
> 
> On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> > A few places currently lack umounting resctrl FS on error paths:
> >   - cmt_resctrl_val() has multiple error paths with direct return.
> >   - cat_perf_miss_val() has multiple error paths with direct return.
> > In addition, validate_resctrl_feature_request() is called by
> > run_mbm_test() and run_mba_test(). Neither MBA nor MBM test tries to
> > umount resctrl FS.
> > 
> > Each and every test does require resctrl FS to be present already for
> > feature check. Thus, it makes sense to just mount it on higher level in
> > resctrl_tests.c and properly pair it with umount.
> > 
> > Move resctrl FS (re)mount/unmount into each test function in
> > resctrl_tests.c. Make feature validation to simply check that resctrl
> > FS is mounted.
> > 
> > Fixes: 91db4fd9019a ("selftests/resctrl: Remove duplicate codes that clear each test result file")
> 
> Could you please elaborate how this commit is the culprit?

Of course it isn't. I'm pretty sure I had some idea when that was added 
but it was before the patches were rearranged/modified, maybe I 
incorrectly thought that the cleanup functions do umount (but they don't).

I'll changed it to these:
Fixes: f1dd71982d19 ("selftests/resctrl: Skip the test if requested resctrl feature is not supported")
Fixes: 01fee6b4d1f9 ("selftests/resctrl: Add MBA test")
Fixes: ecdbb911f22d ("selftests/resctrl: Add MBM test")
Fixes: 790bf585b0ee ("selftests/resctrl: Add Cache Allocation Technology (CAT) selftest")
Fixes: 78941183d1b1 ("selftests/resctrl: Add Cache QoS Monitoring (CQM) selftest")

...however, I was also considering dropping Fixes completely because 
main() has the final umount() at the end so no lingering resctrl FS after 
tests, and inter-test issues are hard to track due to how complicated the 
code is so I'm not entirely sure if there are real issues under any 
combination of tests and all the mounting/unmounting going on).

-- 
 i.

Re: [PATCH v4 06/19] selftests/resctrl: Move resctrl FS mount/umount to higher level

[Reinette Chatre]

Hi Ilpo,

On 7/14/2023 4:31 AM, Ilpo Järvinen wrote:
> On Thu, 13 Jul 2023, Reinette Chatre wrote:
> 
>> Hi Ilpo,
>>
>> On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
>>> A few places currently lack umounting resctrl FS on error paths:
>>>   - cmt_resctrl_val() has multiple error paths with direct return.
>>>   - cat_perf_miss_val() has multiple error paths with direct return.
>>> In addition, validate_resctrl_feature_request() is called by
>>> run_mbm_test() and run_mba_test(). Neither MBA nor MBM test tries to
>>> umount resctrl FS.
>>>
>>> Each and every test does require resctrl FS to be present already for
>>> feature check. Thus, it makes sense to just mount it on higher level in
>>> resctrl_tests.c and properly pair it with umount.
>>>
>>> Move resctrl FS (re)mount/unmount into each test function in
>>> resctrl_tests.c. Make feature validation to simply check that resctrl
>>> FS is mounted.
>>>
>>> Fixes: 91db4fd9019a ("selftests/resctrl: Remove duplicate codes that clear each test result file")
>>
>> Could you please elaborate how this commit is the culprit?
> 
> Of course it isn't. I'm pretty sure I had some idea when that was added 
> but it was before the patches were rearranged/modified, maybe I 
> incorrectly thought that the cleanup functions do umount (but they don't).
> 
> I'll changed it to these:
> Fixes: f1dd71982d19 ("selftests/resctrl: Skip the test if requested resctrl feature is not supported")
> Fixes: 01fee6b4d1f9 ("selftests/resctrl: Add MBA test")
> Fixes: ecdbb911f22d ("selftests/resctrl: Add MBM test")
> Fixes: 790bf585b0ee ("selftests/resctrl: Add Cache Allocation Technology (CAT) selftest")
> Fixes: 78941183d1b1 ("selftests/resctrl: Add Cache QoS Monitoring (CQM) selftest")
> 
> ...however, I was also considering dropping Fixes completely because 
> main() has the final umount() at the end so no lingering resctrl FS after 
> tests, and inter-test issues are hard to track due to how complicated the 
> code is so I'm not entirely sure if there are real issues under any 
> combination of tests and all the mounting/unmounting going on).
> 

Indeed. The problem statement is that some places lack
unmounting resctrl FS on error paths. This may create impression
that there are scenarios where resctrl is left mounted on failure.
As you indicate this is not the case. I agree that the "Fixes" tag
can be dropped with this categorized as code refactor instead.

Reinette

[PATCH v4 07/19] selftests/resctrl: Refactor remount_resctrl(bool mum_resctrlfs) to mount_resctrl()

[Ilpo Järvinen]

Mount/umount of the resctrl FS is now paired nicely per test.

Rename remount_resctrl(bool mum_resctrlfs) to mount_resctrl(). Make
it unconditionally try to mount the resctrl FS and return error if
resctrl FS was mounted already.

While at it, group the mount/umount prototypes in the header.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/resctrl.h     |  2 +-
 .../testing/selftests/resctrl/resctrl_tests.c |  8 ++++----
 tools/testing/selftests/resctrl/resctrlfs.c   | 20 +++++--------------
 3 files changed, 10 insertions(+), 20 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index f455f0b7e314..23af3fb73cb4 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -85,8 +85,8 @@ extern char llc_occup_path[1024];
 int get_vendor(void);
 bool check_resctrlfs_support(void);
 int filter_dmesg(void);
-int remount_resctrlfs(bool mum_resctrlfs);
 int get_resource_id(int cpu_no, int *resource_id);
+int mount_resctrlfs(void);
 int umount_resctrlfs(void);
 int validate_bw_report_request(char *bw_report);
 bool validate_resctrl_feature_request(const char *resctrl_val);
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index a421d045de08..3f26d2279f75 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -77,7 +77,7 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, int span,
 
 	ksft_print_msg("Starting MBM BW change ...\n");
 
-	res = remount_resctrlfs(true);
+	res = mount_resctrlfs();
 	if (res) {
 		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
 		return;
@@ -106,7 +106,7 @@ static void run_mba_test(bool has_ben, char **benchmark_cmd, int span,
 
 	ksft_print_msg("Starting MBA Schemata change ...\n");
 
-	res = remount_resctrlfs(true);
+	res = mount_resctrlfs();
 	if (res) {
 		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
 		return;
@@ -132,7 +132,7 @@ static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no)
 
 	ksft_print_msg("Starting CMT test ...\n");
 
-	res = remount_resctrlfs(true);
+	res = mount_resctrlfs();
 	if (res) {
 		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
 		return;
@@ -160,7 +160,7 @@ static void run_cat_test(int cpu_no, int no_of_bits)
 
 	ksft_print_msg("Starting CAT test ...\n");
 
-	res = remount_resctrlfs(true);
+	res = mount_resctrlfs();
 	if (res) {
 		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
 		return;
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index b3a05488d360..f622245adafe 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -48,29 +48,19 @@ static int find_resctrl_mount(char *buffer)
 }
 
 /*
- * remount_resctrlfs - Remount resctrl FS at /sys/fs/resctrl
- * @mum_resctrlfs:	Should the resctrl FS be remounted?
+ * mount_resctrlfs - Mount resctrl FS at /sys/fs/resctrl
  *
  * If not mounted, mount it.
- * If mounted and mum_resctrlfs then remount resctrl FS.
- * If mounted and !mum_resctrlfs then noop
  *
  * Return: 0 on success, non-zero on failure
  */
-int remount_resctrlfs(bool mum_resctrlfs)
+int mount_resctrlfs(void)
 {
-	char mountpoint[256];
 	int ret;
 
-	ret = find_resctrl_mount(mountpoint);
-	if (ret)
-		strcpy(mountpoint, RESCTRL_PATH);
-
-	if (!ret && mum_resctrlfs && umount(mountpoint))
-		ksft_print_msg("Fail: unmounting \"%s\"\n", mountpoint);
-
-	if (!ret && !mum_resctrlfs)
-		return 0;
+	ret = find_resctrl_mount(NULL);
+	if (!ret)
+		return -1;
 
 	ksft_print_msg("Mounting resctrl to \"%s\"\n", RESCTRL_PATH);
 	ret = mount("resctrl", RESCTRL_PATH, "resctrl", 0, NULL);
-- 
2.30.2

Re: [PATCH v4 07/19] selftests/resctrl: Refactor remount_resctrl(bool mum_resctrlfs) to mount_resctrl()

[Reinette Chatre]

Hi Ilpo,

On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> Mount/umount of the resctrl FS is now paired nicely per test.
> 
> Rename remount_resctrl(bool mum_resctrlfs) to mount_resctrl(). Make
> it unconditionally try to mount the resctrl FS and return error if
> resctrl FS was mounted already.
> 
> While at it, group the mount/umount prototypes in the header.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  tools/testing/selftests/resctrl/resctrl.h     |  2 +-
>  .../testing/selftests/resctrl/resctrl_tests.c |  8 ++++----
>  tools/testing/selftests/resctrl/resctrlfs.c   | 20 +++++--------------
>  3 files changed, 10 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index f455f0b7e314..23af3fb73cb4 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -85,8 +85,8 @@ extern char llc_occup_path[1024];
>  int get_vendor(void);
>  bool check_resctrlfs_support(void);
>  int filter_dmesg(void);
> -int remount_resctrlfs(bool mum_resctrlfs);
>  int get_resource_id(int cpu_no, int *resource_id);
> +int mount_resctrlfs(void);
>  int umount_resctrlfs(void);
>  int validate_bw_report_request(char *bw_report);
>  bool validate_resctrl_feature_request(const char *resctrl_val);
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index a421d045de08..3f26d2279f75 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -77,7 +77,7 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, int span,
>  
>  	ksft_print_msg("Starting MBM BW change ...\n");
>  
> -	res = remount_resctrlfs(true);
> +	res = mount_resctrlfs();
>  	if (res) {
>  		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
>  		return;
> @@ -106,7 +106,7 @@ static void run_mba_test(bool has_ben, char **benchmark_cmd, int span,
>  
>  	ksft_print_msg("Starting MBA Schemata change ...\n");
>  
> -	res = remount_resctrlfs(true);
> +	res = mount_resctrlfs();
>  	if (res) {
>  		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
>  		return;
> @@ -132,7 +132,7 @@ static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no)
>  
>  	ksft_print_msg("Starting CMT test ...\n");
>  
> -	res = remount_resctrlfs(true);
> +	res = mount_resctrlfs();
>  	if (res) {
>  		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
>  		return;
> @@ -160,7 +160,7 @@ static void run_cat_test(int cpu_no, int no_of_bits)
>  
>  	ksft_print_msg("Starting CAT test ...\n");
>  
> -	res = remount_resctrlfs(true);
> +	res = mount_resctrlfs();
>  	if (res) {
>  		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
>  		return;
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index b3a05488d360..f622245adafe 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -48,29 +48,19 @@ static int find_resctrl_mount(char *buffer)
>  }
>  
>  /*
> - * remount_resctrlfs - Remount resctrl FS at /sys/fs/resctrl
> - * @mum_resctrlfs:	Should the resctrl FS be remounted?
> + * mount_resctrlfs - Mount resctrl FS at /sys/fs/resctrl
>   *
>   * If not mounted, mount it.
> - * If mounted and mum_resctrlfs then remount resctrl FS.
> - * If mounted and !mum_resctrlfs then noop
>   *
>   * Return: 0 on success, non-zero on failure
>   */

Since it is not obviously a "failure" I do think it will help to
add to the comments that resctrl already being mounted is treated as
a failure.

> -int remount_resctrlfs(bool mum_resctrlfs)
> +int mount_resctrlfs(void)
>  {
> -	char mountpoint[256];
>  	int ret;
>  
> -	ret = find_resctrl_mount(mountpoint);
> -	if (ret)
> -		strcpy(mountpoint, RESCTRL_PATH);
> -
> -	if (!ret && mum_resctrlfs && umount(mountpoint))
> -		ksft_print_msg("Fail: unmounting \"%s\"\n", mountpoint);
> -
> -	if (!ret && !mum_resctrlfs)
> -		return 0;
> +	ret = find_resctrl_mount(NULL);
> +	if (!ret)
> +		return -1;

This treats "ret == 0" as a failure. What about -ENXIO? It seems to
me that only "ret == -ENOENT" is "success".

>  
>  	ksft_print_msg("Mounting resctrl to \"%s\"\n", RESCTRL_PATH);
>  	ret = mount("resctrl", RESCTRL_PATH, "resctrl", 0, NULL);


Reinette

Re: [PATCH v4 07/19] selftests/resctrl: Refactor remount_resctrl(bool mum_resctrlfs) to mount_resctrl()

[Ilpo Järvinen]

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

On Thu, 13 Jul 2023, Reinette Chatre wrote:
> On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> > Mount/umount of the resctrl FS is now paired nicely per test.
> > 
> > Rename remount_resctrl(bool mum_resctrlfs) to mount_resctrl(). Make
> > it unconditionally try to mount the resctrl FS and return error if
> > resctrl FS was mounted already.
> > 
> > While at it, group the mount/umount prototypes in the header.
> > 
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> >  tools/testing/selftests/resctrl/resctrl.h     |  2 +-
> >  .../testing/selftests/resctrl/resctrl_tests.c |  8 ++++----
> >  tools/testing/selftests/resctrl/resctrlfs.c   | 20 +++++--------------
> >  3 files changed, 10 insertions(+), 20 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> > index f455f0b7e314..23af3fb73cb4 100644
> > --- a/tools/testing/selftests/resctrl/resctrl.h
> > +++ b/tools/testing/selftests/resctrl/resctrl.h
> > @@ -85,8 +85,8 @@ extern char llc_occup_path[1024];
> >  int get_vendor(void);
> >  bool check_resctrlfs_support(void);
> >  int filter_dmesg(void);
> > -int remount_resctrlfs(bool mum_resctrlfs);
> >  int get_resource_id(int cpu_no, int *resource_id);
> > +int mount_resctrlfs(void);
> >  int umount_resctrlfs(void);
> >  int validate_bw_report_request(char *bw_report);
> >  bool validate_resctrl_feature_request(const char *resctrl_val);
> > diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> > index a421d045de08..3f26d2279f75 100644
> > --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> > +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> > @@ -77,7 +77,7 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, int span,
> >  
> >  	ksft_print_msg("Starting MBM BW change ...\n");
> >  
> > -	res = remount_resctrlfs(true);
> > +	res = mount_resctrlfs();
> >  	if (res) {
> >  		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
> >  		return;
> > @@ -106,7 +106,7 @@ static void run_mba_test(bool has_ben, char **benchmark_cmd, int span,
> >  
> >  	ksft_print_msg("Starting MBA Schemata change ...\n");
> >  
> > -	res = remount_resctrlfs(true);
> > +	res = mount_resctrlfs();
> >  	if (res) {
> >  		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
> >  		return;
> > @@ -132,7 +132,7 @@ static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no)
> >  
> >  	ksft_print_msg("Starting CMT test ...\n");
> >  
> > -	res = remount_resctrlfs(true);
> > +	res = mount_resctrlfs();
> >  	if (res) {
> >  		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
> >  		return;
> > @@ -160,7 +160,7 @@ static void run_cat_test(int cpu_no, int no_of_bits)
> >  
> >  	ksft_print_msg("Starting CAT test ...\n");
> >  
> > -	res = remount_resctrlfs(true);
> > +	res = mount_resctrlfs();
> >  	if (res) {
> >  		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
> >  		return;
> > diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> > index b3a05488d360..f622245adafe 100644
> > --- a/tools/testing/selftests/resctrl/resctrlfs.c
> > +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> > @@ -48,29 +48,19 @@ static int find_resctrl_mount(char *buffer)
> >  }
> >  
> >  /*
> > - * remount_resctrlfs - Remount resctrl FS at /sys/fs/resctrl
> > - * @mum_resctrlfs:	Should the resctrl FS be remounted?
> > + * mount_resctrlfs - Mount resctrl FS at /sys/fs/resctrl
> >   *
> >   * If not mounted, mount it.
> > - * If mounted and mum_resctrlfs then remount resctrl FS.
> > - * If mounted and !mum_resctrlfs then noop
> >   *
> >   * Return: 0 on success, non-zero on failure
> >   */
> 
> Since it is not obviously a "failure" I do think it will help to
> add to the comments that resctrl already being mounted is treated as
> a failure.
> 
> > -int remount_resctrlfs(bool mum_resctrlfs)
> > +int mount_resctrlfs(void)
> >  {
> > -	char mountpoint[256];
> >  	int ret;
> >  
> > -	ret = find_resctrl_mount(mountpoint);
> > -	if (ret)
> > -		strcpy(mountpoint, RESCTRL_PATH);
> > -
> > -	if (!ret && mum_resctrlfs && umount(mountpoint))
> > -		ksft_print_msg("Fail: unmounting \"%s\"\n", mountpoint);
> > -
> > -	if (!ret && !mum_resctrlfs)
> > -		return 0;
> > +	ret = find_resctrl_mount(NULL);
> > +	if (!ret)
> > +		return -1;
> 
> This treats "ret == 0" as a failure. What about -ENXIO? It seems to
> me that only "ret == -ENOENT" is "success".

Yes, it's a good catch.

-- 
 i.

Re: [PATCH v4 07/19] selftests/resctrl: Refactor remount_resctrl(bool mum_resctrlfs) to mount_resctrl()

[Wieczor-Retman, Maciej]

Hi!

On 14.07.2023 13:03, Ilpo Järvinen wrote:
> On Thu, 13 Jul 2023, Reinette Chatre wrote:
>> On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
>>> -int remount_resctrlfs(bool mum_resctrlfs)
>>> +int mount_resctrlfs(void)
>>>  {
>>> -	char mountpoint[256];
>>>  	int ret;
>>>  
>>> -	ret = find_resctrl_mount(mountpoint);
>>> -	if (ret)
>>> -		strcpy(mountpoint, RESCTRL_PATH);
>>> -
>>> -	if (!ret && mum_resctrlfs && umount(mountpoint))
>>> -		ksft_print_msg("Fail: unmounting \"%s\"\n", mountpoint);
>>> -
>>> -	if (!ret && !mum_resctrlfs)
>>> -		return 0;
>>> +	ret = find_resctrl_mount(NULL);
>>> +	if (!ret)
>>> +		return -1;
>>
>> This treats "ret == 0" as a failure. What about -ENXIO? It seems to
>> me that only "ret == -ENOENT" is "success".
> 
> Yes, it's a good catch.
> 

I had an idea about a small redesign of find_resctrl_mount
return values so it is easier to see what the function tries
to accomplish.

When there is an error (-ENXIO for example) it could 
return the negative error value. When no mount is found
it could return a zero (instead of the -ENOENT error code).
Finally when a mount point was found it could return a positive
value (for example return 1). This way errors could be 
separate from regular return values and in my opinion the
function logic would be more transparent.

What do you think about it?

Kind regards
Maciej Wieczór-Retman

Re: [PATCH v4 07/19] selftests/resctrl: Refactor remount_resctrl(bool mum_resctrlfs) to mount_resctrl()

[Ilpo Järvinen]

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

On Mon, 24 Jul 2023, Wieczor-Retman, Maciej wrote:
> On 14.07.2023 13:03, Ilpo Järvinen wrote:
> > On Thu, 13 Jul 2023, Reinette Chatre wrote:
> >> On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> >>> -int remount_resctrlfs(bool mum_resctrlfs)
> >>> +int mount_resctrlfs(void)
> >>>  {
> >>> -	char mountpoint[256];
> >>>  	int ret;
> >>>  
> >>> -	ret = find_resctrl_mount(mountpoint);
> >>> -	if (ret)
> >>> -		strcpy(mountpoint, RESCTRL_PATH);
> >>> -
> >>> -	if (!ret && mum_resctrlfs && umount(mountpoint))
> >>> -		ksft_print_msg("Fail: unmounting \"%s\"\n", mountpoint);
> >>> -
> >>> -	if (!ret && !mum_resctrlfs)
> >>> -		return 0;
> >>> +	ret = find_resctrl_mount(NULL);
> >>> +	if (!ret)
> >>> +		return -1;
> >>
> >> This treats "ret == 0" as a failure. What about -ENXIO? It seems to
> >> me that only "ret == -ENOENT" is "success".
> > 
> > Yes, it's a good catch.
> > 
> 
> I had an idea about a small redesign of find_resctrl_mount
> return values so it is easier to see what the function tries
> to accomplish.
> 
> When there is an error (-ENXIO for example) it could 
> return the negative error value. When no mount is found
> it could return a zero (instead of the -ENOENT error code).
> Finally when a mount point was found it could return a positive
> value (for example return 1). This way errors could be 
> separate from regular return values and in my opinion the
> function logic would be more transparent.
> 
> What do you think about it?

Yes, it's a great idea. It would be more logical and thus easier to 
comprehend.

Since this already wast applied, it has to be done in another change.


-- 
 i.

[PATCH v4 08/19] selftests/resctrl: Remove mum_resctrlfs from struct resctrl_val_param

[Ilpo Järvinen]

Resctrl FS mount/umount are now cleanly paired leaving .mum_resctrlfs
in the struct resctrl_val_param unused.

Remove .mum_resctrlfs from the struct resctrl_val_param that is
leftover from the remount trickery and no longer needed.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/cat_test.c | 1 -
 tools/testing/selftests/resctrl/cmt_test.c | 1 -
 tools/testing/selftests/resctrl/mba_test.c | 1 -
 tools/testing/selftests/resctrl/mbm_test.c | 1 -
 tools/testing/selftests/resctrl/resctrl.h  | 2 --
 5 files changed, 6 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
index e1c071dec1b0..480db0dc4e0e 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -140,7 +140,6 @@ 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	= false,
 		.setup		= cat_setup,
 	};
 
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index 426d11189a99..d31e28416bb7 100644
--- a/tools/testing/selftests/resctrl/cmt_test.c
+++ b/tools/testing/selftests/resctrl/cmt_test.c
@@ -113,7 +113,6 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
 		.ctrlgrp	= "c1",
 		.mongrp		= "m1",
 		.cpu_no		= cpu_no,
-		.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 cde3781a9ab0..3d53c6c9b9ce 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -154,7 +154,6 @@ int mba_schemata_change(int cpu_no, char *bw_report, char **benchmark_cmd)
 		.ctrlgrp	= "c1",
 		.mongrp		= "m1",
 		.cpu_no		= cpu_no,
-		.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 538d35a6485a..24326cb7bc21 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -123,7 +123,6 @@ 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	= 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 23af3fb73cb4..99678d688a80 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -54,7 +54,6 @@
  * @mongrp:		Name of the monitor group (mon grp)
  * @cpu_no:		CPU number to which the benchmark would be binded
  * @span:		Memory bytes accessed in each benchmark iteration
- * @mum_resctrlfs:	Should the resctrl FS be remounted?
  * @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
@@ -65,7 +64,6 @@ struct resctrl_val_param {
 	char		mongrp[64];
 	int		cpu_no;
 	unsigned long	span;
-	bool		mum_resctrlfs;
 	char		filename[64];
 	char		*bw_report;
 	unsigned long	mask;
-- 
2.30.2

Re: [PATCH v4 08/19] selftests/resctrl: Remove mum_resctrlfs from struct resctrl_val_param

[Reinette Chatre]

Hi Ilpo,

On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> Resctrl FS mount/umount are now cleanly paired leaving .mum_resctrlfs
> in the struct resctrl_val_param unused.
> 
> Remove .mum_resctrlfs from the struct resctrl_val_param that is
> leftover from the remount trickery and no longer needed.

s/needed/used. Actually, considering the first paragraph the
second paragraph can just be:
"Remove .mum_resctrlfs from struct resctrl_val_param."

> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

With the changelog updated:

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

[PATCH v4 09/19] selftests/resctrl: Convert span to size_t

[Ilpo Järvinen]

Span is defined either as unsigned long or int.

Consistently use size_t everywhere for span as it refers to size of the
memory block.

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         |  4 ++--
 tools/testing/selftests/resctrl/cmt_test.c      |  2 +-
 tools/testing/selftests/resctrl/fill_buf.c      |  8 ++++----
 tools/testing/selftests/resctrl/mbm_test.c      |  8 ++++----
 tools/testing/selftests/resctrl/resctrl.h       | 10 +++++-----
 tools/testing/selftests/resctrl/resctrl_tests.c | 11 ++++++-----
 tools/testing/selftests/resctrl/resctrlfs.c     |  2 +-
 7 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
index ced47b445d1e..f2c7d322bd76 100644
--- a/tools/testing/selftests/resctrl/cache.c
+++ b/tools/testing/selftests/resctrl/cache.c
@@ -282,7 +282,7 @@ int cat_val(struct resctrl_val_param *param)
  * Return:		0 on success. non-zero on failure.
  */
 int show_cache_info(unsigned long sum_llc_val, int no_of_bits,
-		    unsigned long cache_span, unsigned long max_diff,
+		    size_t cache_span, unsigned long max_diff,
 		    unsigned long max_diff_percent, unsigned long num_of_runs,
 		    bool platform, bool cmt)
 {
@@ -304,7 +304,7 @@ int show_cache_info(unsigned long sum_llc_val, int no_of_bits,
 	ksft_print_msg("Percent diff=%d\n", abs((int)diff_percent));
 	ksft_print_msg("Number of bits: %d\n", no_of_bits);
 	ksft_print_msg("Average LLC val: %lu\n", avg_llc_val);
-	ksft_print_msg("Cache span (%s): %lu\n", cmt ? "bytes" : "lines",
+	ksft_print_msg("Cache span (%s): %zu\n", cmt ? "bytes" : "lines",
 		       cache_span);
 
 	return ret;
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index d31e28416bb7..beb0f0687c6d 100644
--- a/tools/testing/selftests/resctrl/cmt_test.c
+++ b/tools/testing/selftests/resctrl/cmt_test.c
@@ -121,7 +121,7 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
 	};
 
 	if (strcmp(benchmark_cmd[0], "fill_buf") == 0)
-		sprintf(benchmark_cmd[1], "%lu", param.span);
+		sprintf(benchmark_cmd[1], "%zu", param.span);
 
 	remove(RESULT_FILE_NAME);
 
diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c
index 3b328c844896..785cbd8d0148 100644
--- a/tools/testing/selftests/resctrl/fill_buf.c
+++ b/tools/testing/selftests/resctrl/fill_buf.c
@@ -139,7 +139,7 @@ static int fill_cache_write(unsigned char *start_ptr, unsigned char *end_ptr,
 }
 
 static int
-fill_cache(unsigned long long buf_size, int malloc_and_init, int memflush,
+fill_cache(size_t buf_size, int malloc_and_init, int memflush,
 	   int op, char *resctrl_val)
 {
 	unsigned char *start_ptr, *end_ptr;
@@ -188,10 +188,10 @@ fill_cache(unsigned long long buf_size, int malloc_and_init, int memflush,
 	return 0;
 }
 
-int run_fill_buf(unsigned long span, int malloc_and_init_memory,
-		 int memflush, int op, char *resctrl_val)
+int run_fill_buf(size_t span, int malloc_and_init_memory, int memflush, int op,
+		 char *resctrl_val)
 {
-	unsigned long long cache_size = span;
+	size_t cache_size = span;
 	int ret;
 
 	ret = fill_cache(cache_size, malloc_and_init_memory, memflush, op,
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index 24326cb7bc21..fd116158d008 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -15,7 +15,7 @@
 #define NUM_OF_RUNS		5
 
 static int
-show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc, int span)
+show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc, size_t span)
 {
 	unsigned long avg_bw_imc = 0, avg_bw_resc = 0;
 	unsigned long sum_bw_imc = 0, sum_bw_resc = 0;
@@ -40,14 +40,14 @@ show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc, int span)
 	ksft_print_msg("%s Check MBM diff within %d%%\n",
 		       ret ? "Fail:" : "Pass:", MAX_DIFF_PERCENT);
 	ksft_print_msg("avg_diff_per: %d%%\n", avg_diff_per);
-	ksft_print_msg("Span (MB): %d\n", span);
+	ksft_print_msg("Span (MB): %zu\n", span);
 	ksft_print_msg("avg_bw_imc: %lu\n", avg_bw_imc);
 	ksft_print_msg("avg_bw_resc: %lu\n", avg_bw_resc);
 
 	return ret;
 }
 
-static int check_results(int span)
+static int check_results(size_t span)
 {
 	unsigned long bw_imc[NUM_OF_RUNS], bw_resc[NUM_OF_RUNS];
 	char temp[1024], *token_array[8];
@@ -115,7 +115,7 @@ void mbm_test_cleanup(void)
 	remove(RESULT_FILE_NAME);
 }
 
-int mbm_bw_change(int span, int cpu_no, char *bw_report, char **benchmark_cmd)
+int mbm_bw_change(size_t span, int cpu_no, char *bw_report, char **benchmark_cmd)
 {
 	struct resctrl_val_param param = {
 		.resctrl_val	= MBM_STR,
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index 99678d688a80..52068ceea956 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -63,7 +63,7 @@ struct resctrl_val_param {
 	char		ctrlgrp[64];
 	char		mongrp[64];
 	int		cpu_no;
-	unsigned long	span;
+	size_t		span;
 	char		filename[64];
 	char		*bw_report;
 	unsigned long	mask;
@@ -97,10 +97,10 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp,
 			    char *resctrl_val);
 int perf_event_open(struct perf_event_attr *hw_event, pid_t pid, int cpu,
 		    int group_fd, unsigned long flags);
-int run_fill_buf(unsigned long span, int malloc_and_init_memory, int memflush,
-		 int op, char *resctrl_va);
+int run_fill_buf(size_t span, int malloc_and_init_memory, int memflush, int op,
+		 char *resctrl_va);
 int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param);
-int mbm_bw_change(int span, int cpu_no, char *bw_report, char **benchmark_cmd);
+int mbm_bw_change(size_t span, int cpu_no, char *bw_report, char **benchmark_cmd);
 void tests_cleanup(void);
 void mbm_test_cleanup(void);
 int mba_schemata_change(int cpu_no, char *bw_report, char **benchmark_cmd);
@@ -119,7 +119,7 @@ void cmt_test_cleanup(void);
 int get_core_sibling(int cpu_no);
 int measure_cache_vals(struct resctrl_val_param *param, int bm_pid);
 int show_cache_info(unsigned long sum_llc_val, int no_of_bits,
-		    unsigned long cache_span, unsigned long max_diff,
+		    size_t cache_span, unsigned long max_diff,
 		    unsigned long max_diff_percent, unsigned long num_of_runs,
 		    bool platform, bool cmt);
 
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index 3f26d2279f75..6b2ca1b4744f 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -70,7 +70,7 @@ void tests_cleanup(void)
 	cat_test_cleanup();
 }
 
-static void run_mbm_test(bool has_ben, char **benchmark_cmd, int span,
+static void run_mbm_test(bool has_ben, char **benchmark_cmd, size_t span,
 			 int cpu_no, char *bw_report)
 {
 	int res;
@@ -99,7 +99,7 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, int span,
 	umount_resctrlfs();
 }
 
-static void run_mba_test(bool has_ben, char **benchmark_cmd, int span,
+static void run_mba_test(bool has_ben, char **benchmark_cmd, size_t span,
 			 int cpu_no, char *bw_report)
 {
 	int res;
@@ -118,7 +118,7 @@ static void run_mba_test(bool has_ben, char **benchmark_cmd, int span,
 	}
 
 	if (!has_ben)
-		sprintf(benchmark_cmd[1], "%d", span);
+		sprintf(benchmark_cmd[1], "%zu", span);
 	res = mba_schemata_change(cpu_no, bw_report, benchmark_cmd);
 	ksft_test_result(!res, "MBA: schemata change\n");
 
@@ -181,11 +181,12 @@ static void run_cat_test(int cpu_no, int no_of_bits)
 int main(int argc, char **argv)
 {
 	bool has_ben = false, mbm_test = true, mba_test = true, cmt_test = true;
-	int c, cpu_no = 1, span = 250, argc_new = argc, i, no_of_bits = 0;
 	char *benchmark_cmd[BENCHMARK_ARGS], bw_report[64], bm_type[64];
 	char benchmark_cmd_area[BENCHMARK_ARGS][BENCHMARK_ARG_SIZE];
+	int c, cpu_no = 1, argc_new = argc, i, no_of_bits = 0;
 	int ben_ind, ben_count, tests = 0;
 	bool cat_test = true;
+	size_t span = 250;
 
 	for (i = 0; i < argc; i++) {
 		if (strcmp(argv[i], "-b") == 0) {
@@ -273,7 +274,7 @@ int main(int argc, char **argv)
 			benchmark_cmd[i] = benchmark_cmd_area[i];
 
 		strcpy(benchmark_cmd[0], "fill_buf");
-		sprintf(benchmark_cmd[1], "%d", span);
+		sprintf(benchmark_cmd[1], "%zu", span);
 		strcpy(benchmark_cmd[2], "1");
 		strcpy(benchmark_cmd[3], "1");
 		strcpy(benchmark_cmd[4], "0");
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index f622245adafe..8be5b745226d 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -298,7 +298,7 @@ int taskset_benchmark(pid_t bm_pid, int cpu_no)
 void run_benchmark(int signum, siginfo_t *info, void *ucontext)
 {
 	int operation, ret, malloc_and_init_memory, memflush;
-	unsigned long span, buffer_span;
+	size_t span, buffer_span;
 	char **benchmark_cmd;
 	char resctrl_val[64];
 	FILE *fp;
-- 
2.30.2

Re: [PATCH v4 09/19] selftests/resctrl: Convert span to size_t

[Reinette Chatre]

Hi Ilpo,

On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:

...

> @@ -188,10 +188,10 @@ fill_cache(unsigned long long buf_size, int malloc_and_init, int memflush,
>  	return 0;
>  }
>  
> -int run_fill_buf(unsigned long span, int malloc_and_init_memory,
> -		 int memflush, int op, char *resctrl_val)
> +int run_fill_buf(size_t span, int malloc_and_init_memory, int memflush, int op,
> +		 char *resctrl_val)
>  {
> -	unsigned long long cache_size = span;
> +	size_t cache_size = span;
>  	int ret;
>  
>  	ret = fill_cache(cache_size, malloc_and_init_memory, memflush, op,

Any idea what the purpose being run_fill_buf() is? From what I can tell it is
an unnecessary level of indirection.

...

> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index f622245adafe..8be5b745226d 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -298,7 +298,7 @@ int taskset_benchmark(pid_t bm_pid, int cpu_no)
>  void run_benchmark(int signum, siginfo_t *info, void *ucontext)
>  {
>  	int operation, ret, malloc_and_init_memory, memflush;
> -	unsigned long span, buffer_span;
> +	size_t span, buffer_span;
>  	char **benchmark_cmd;
>  	char resctrl_val[64];
>  	FILE *fp;

Do we now need a cast in the initialization of span?

Reinette

Re: [PATCH v4 09/19] selftests/resctrl: Convert span to size_t

[Ilpo Järvinen]

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

On Thu, 13 Jul 2023, Reinette Chatre wrote:

> Hi Ilpo,
> 
> On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> 
> ...
> 
> > @@ -188,10 +188,10 @@ fill_cache(unsigned long long buf_size, int malloc_and_init, int memflush,
> >  	return 0;
> >  }
> >  
> > -int run_fill_buf(unsigned long span, int malloc_and_init_memory,
> > -		 int memflush, int op, char *resctrl_val)
> > +int run_fill_buf(size_t span, int malloc_and_init_memory, int memflush, int op,
> > +		 char *resctrl_val)
> >  {
> > -	unsigned long long cache_size = span;
> > +	size_t cache_size = span;
> >  	int ret;
> >  
> >  	ret = fill_cache(cache_size, malloc_and_init_memory, memflush, op,
> 
> Any idea what the purpose being run_fill_buf() is? From what I can tell it is
> an unnecessary level of indirection.

You already mentioned it, fill_cache() could be included into 
run_fill_buf() but it's not part of this series.

> > diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> > index f622245adafe..8be5b745226d 100644
> > --- a/tools/testing/selftests/resctrl/resctrlfs.c
> > +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> > @@ -298,7 +298,7 @@ int taskset_benchmark(pid_t bm_pid, int cpu_no)
> >  void run_benchmark(int signum, siginfo_t *info, void *ucontext)
> >  {
> >  	int operation, ret, malloc_and_init_memory, memflush;
> > -	unsigned long span, buffer_span;
> > +	size_t span, buffer_span;
> >  	char **benchmark_cmd;
> >  	char resctrl_val[64];
> >  	FILE *fp;
> 
> Do we now need a cast in the initialization of span?

I don't see any warning w/o cast, unsigned long -> size_t seems pretty 
safe anyway. For internally provided values, overflow does not seem 
possible even if the type sizes would disagree.

There's no existing error checking for the command in the case where 
"fill_buf" is used with -b (an orthogonal issue).

-- 
 i.

[PATCH v4 10/19] selftests/resctrl: Express span internally in bytes

[Ilpo Järvinen]

MBA and MBM tests to use megabytes to represent span. CMT test uses
bytes. The difference requires run_benchmark() to size the buffer
differently based on the test name, which in turn requires passing the
test name into run_benchmark().

Convert MBA and MBM tests to use internally bytes like CMT test to
remove the internal inconsistency between the tests. Remove the test
dependent buffer sizing from run_benchmark().

This change eliminates one of the reasons why the test name has to be
passed around but there are still other users too so the test name
passing cannot yet be removed.

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/mbm_test.c      | 2 +-
 tools/testing/selftests/resctrl/resctrl_tests.c | 2 +-
 tools/testing/selftests/resctrl/resctrlfs.c     | 9 ++-------
 3 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index fd116158d008..2d58d4b8a918 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -40,7 +40,7 @@ show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc, size_t span)
 	ksft_print_msg("%s Check MBM diff within %d%%\n",
 		       ret ? "Fail:" : "Pass:", MAX_DIFF_PERCENT);
 	ksft_print_msg("avg_diff_per: %d%%\n", avg_diff_per);
-	ksft_print_msg("Span (MB): %zu\n", span);
+	ksft_print_msg("Span (MB): %zu\n", span / MB);
 	ksft_print_msg("avg_bw_imc: %lu\n", avg_bw_imc);
 	ksft_print_msg("avg_bw_resc: %lu\n", avg_bw_resc);
 
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index 6b2ca1b4744f..5205a13ed250 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -185,8 +185,8 @@ int main(int argc, char **argv)
 	char benchmark_cmd_area[BENCHMARK_ARGS][BENCHMARK_ARG_SIZE];
 	int c, cpu_no = 1, argc_new = argc, i, no_of_bits = 0;
 	int ben_ind, ben_count, tests = 0;
+	size_t span = 250 * MB;
 	bool cat_test = true;
-	size_t span = 250;
 
 	for (i = 0; i < argc; i++) {
 		if (strcmp(argv[i], "-b") == 0) {
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index 8be5b745226d..847e0a80c6cd 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -298,9 +298,9 @@ int taskset_benchmark(pid_t bm_pid, int cpu_no)
 void run_benchmark(int signum, siginfo_t *info, void *ucontext)
 {
 	int operation, ret, malloc_and_init_memory, memflush;
-	size_t span, buffer_span;
 	char **benchmark_cmd;
 	char resctrl_val[64];
+	size_t span;
 	FILE *fp;
 
 	benchmark_cmd = info->si_ptr;
@@ -321,12 +321,7 @@ void run_benchmark(int signum, siginfo_t *info, void *ucontext)
 		operation = atoi(benchmark_cmd[4]);
 		sprintf(resctrl_val, "%s", benchmark_cmd[5]);
 
-		if (strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
-			buffer_span = span * MB;
-		else
-			buffer_span = span;
-
-		if (run_fill_buf(buffer_span, malloc_and_init_memory, memflush,
+		if (run_fill_buf(span, malloc_and_init_memory, memflush,
 				 operation, resctrl_val))
 			fprintf(stderr, "Error in running fill buffer\n");
 	} else {
-- 
2.30.2

Re: [PATCH v4 10/19] selftests/resctrl: Express span internally in bytes

[Reinette Chatre]

Hi Ilpo,

On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> MBA and MBM tests to use megabytes to represent span. CMT test uses
> bytes. The difference requires run_benchmark() to size the buffer
> differently based on the test name, which in turn requires passing the
> test name into run_benchmark().
> 
> Convert MBA and MBM tests to use internally bytes like CMT test to
> remove the internal inconsistency between the tests. Remove the test
> dependent buffer sizing from run_benchmark().

If I understand correctly the intention is to always use bytes internally
and only convert to megabytes when displayed to user space. The above
implies that this takes care of the conversion but there still seems
to be places that that do not follow my understanding. For example,
resctrl_val.c:measure_vals() converts to megabytes before proceeding.

While MBA, MBM, and CMT tests use resctrl_val() for testing it seems
as though the function still exits with the MBA/MBM data recorded in
megabytes with the CMT data recorded in bytes. That seems to be why
show_mba_info() needs no conversion when displaying the data.

Reinette

Re: [PATCH v4 10/19] selftests/resctrl: Express span internally in bytes

[Wieczor-Retman, Maciej]

Hi Reinette!

On 14.07.2023 01:00, Reinette Chatre wrote:
> Hi Ilpo,
> 
> On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
>> MBA and MBM tests to use megabytes to represent span. CMT test uses
>> bytes. The difference requires run_benchmark() to size the buffer
>> differently based on the test name, which in turn requires passing the
>> test name into run_benchmark().
>>
>> Convert MBA and MBM tests to use internally bytes like CMT test to
>> remove the internal inconsistency between the tests. Remove the test
>> dependent buffer sizing from run_benchmark().
> 
> If I understand correctly the intention is to always use bytes internally
> and only convert to megabytes when displayed to user space. The above
> implies that this takes care of the conversion but there still seems
> to be places that that do not follow my understanding. For example,
> resctrl_val.c:measure_vals() converts to megabytes before proceeding.

Doesn't the use case inside resctrl_val.c:measure_vals() satisfy
the idea of only displaying data to the user space? From my
understanding it reads the number of bytes and only converts to
MB when printing the value. Or did I miss some detail there?

> 
> While MBA, MBM, and CMT tests use resctrl_val() for testing it seems
> as though the function still exits with the MBA/MBM data recorded in
> megabytes with the CMT data recorded in bytes. That seems to be why
> show_mba_info() needs no conversion when displaying the data.
> 
> Reinette

Kind regards
Maciej Wieczór-Retman

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w transakcjach handlowych.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.

Re: [PATCH v4 10/19] selftests/resctrl: Express span internally in bytes

[Ilpo Järvinen]

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

On Fri, 14 Jul 2023, Wieczor-Retman, Maciej wrote:

> Hi Reinette!
> 
> On 14.07.2023 01:00, Reinette Chatre wrote:
> > Hi Ilpo,
> > 
> > On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> >> MBA and MBM tests to use megabytes to represent span. CMT test uses
> >> bytes. The difference requires run_benchmark() to size the buffer
> >> differently based on the test name, which in turn requires passing the
> >> test name into run_benchmark().
> >>
> >> Convert MBA and MBM tests to use internally bytes like CMT test to
> >> remove the internal inconsistency between the tests. Remove the test
> >> dependent buffer sizing from run_benchmark().
> > 
> > If I understand correctly the intention is to always use bytes internally
> > and only convert to megabytes when displayed to user space. The above
> > implies that this takes care of the conversion but there still seems
> > to be places that that do not follow my understanding. For example,
> > resctrl_val.c:measure_vals() converts to megabytes before proceeding.
> 
> Doesn't the use case inside resctrl_val.c:measure_vals() satisfy
> the idea of only displaying data to the user space? From my
> understanding it reads the number of bytes and only converts to
> MB when printing the value. Or did I miss some detail there?

It's for printing there yes.

But it's not about span in the first place so I'm not sure why it is 
related.

-- 
 i.

> > While MBA, MBM, and CMT tests use resctrl_val() for testing it seems
> > as though the function still exits with the MBA/MBM data recorded in
> > megabytes with the CMT data recorded in bytes. That seems to be why
> > show_mba_info() needs no conversion when displaying the data.
> > 
> > Reinette
> 
> Kind regards
> Maciej Wieczór-Retman
> 

Re: [PATCH v4 10/19] selftests/resctrl: Express span internally in bytes

[Reinette Chatre]

Hi Ilpo,

On 7/14/2023 3:22 AM, Ilpo Järvinen wrote:
> On Fri, 14 Jul 2023, Wieczor-Retman, Maciej wrote:
>> On 14.07.2023 01:00, Reinette Chatre wrote:
>>> Hi Ilpo,
>>>
>>> On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
>>>> MBA and MBM tests to use megabytes to represent span. CMT test uses
>>>> bytes. The difference requires run_benchmark() to size the buffer
>>>> differently based on the test name, which in turn requires passing the
>>>> test name into run_benchmark().
>>>>
>>>> Convert MBA and MBM tests to use internally bytes like CMT test to
>>>> remove the internal inconsistency between the tests. Remove the test
>>>> dependent buffer sizing from run_benchmark().
>>>
>>> If I understand correctly the intention is to always use bytes internally
>>> and only convert to megabytes when displayed to user space. The above
>>> implies that this takes care of the conversion but there still seems
>>> to be places that that do not follow my understanding. For example,
>>> resctrl_val.c:measure_vals() converts to megabytes before proceeding.
>>
>> Doesn't the use case inside resctrl_val.c:measure_vals() satisfy
>> the idea of only displaying data to the user space? From my
>> understanding it reads the number of bytes and only converts to
>> MB when printing the value. Or did I miss some detail there?
> 
> It's for printing there yes.
> 
> But it's not about span in the first place so I'm not sure why it is 
> related.
> 

If this change is just about how "span" is interpreted by the different
tests then the changelog could be more specific to not create expectation
that with this change there are no longer "bytes vs megabytes" internal
inconsistency between MBA, MBM, and CMT tests.

Reinette

Re: [PATCH v4 10/19] selftests/resctrl: Express span internally in bytes

[Ilpo Järvinen]

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

On Fri, 14 Jul 2023, Reinette Chatre wrote:
> On 7/14/2023 3:22 AM, Ilpo Järvinen wrote:
> > On Fri, 14 Jul 2023, Wieczor-Retman, Maciej wrote:
> >> On 14.07.2023 01:00, Reinette Chatre wrote:
> >>> Hi Ilpo,
> >>>
> >>> On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> >>>> MBA and MBM tests to use megabytes to represent span. CMT test uses
> >>>> bytes. The difference requires run_benchmark() to size the buffer
> >>>> differently based on the test name, which in turn requires passing the
> >>>> test name into run_benchmark().
> >>>>
> >>>> Convert MBA and MBM tests to use internally bytes like CMT test to
> >>>> remove the internal inconsistency between the tests. Remove the test
> >>>> dependent buffer sizing from run_benchmark().
> >>>
> >>> If I understand correctly the intention is to always use bytes internally
> >>> and only convert to megabytes when displayed to user space. The above
> >>> implies that this takes care of the conversion but there still seems
> >>> to be places that that do not follow my understanding. For example,
> >>> resctrl_val.c:measure_vals() converts to megabytes before proceeding.
> >>
> >> Doesn't the use case inside resctrl_val.c:measure_vals() satisfy
> >> the idea of only displaying data to the user space? From my
> >> understanding it reads the number of bytes and only converts to
> >> MB when printing the value. Or did I miss some detail there?
> > 
> > It's for printing there yes.
> > 
> > But it's not about span in the first place so I'm not sure why it is 
> > related.
> > 
> 
> If this change is just about how "span" is interpreted by the different
> tests then the changelog could be more specific to not create expectation
> that with this change there are no longer "bytes vs megabytes" internal
> inconsistency between MBA, MBM, and CMT tests.

The shortlog and changelog are already pretty specific in mentioning 
"span" a few times :-). I added yet another "span" into the changelog's 
2nd paragraph.

Your general observation about the other MB/bytes inconsistency is still
a good one so I added it also to my todo list, it just doesn't belong to 
this patch (IMHO).


-- 
 i.

Re: [PATCH v4 10/19] selftests/resctrl: Express span internally in bytes

[Reinette Chatre]

Hi Ilpo,

On 7/17/2023 5:30 AM, Ilpo Järvinen wrote:
> On Fri, 14 Jul 2023, Reinette Chatre wrote:
>> On 7/14/2023 3:22 AM, Ilpo Järvinen wrote:
>>> On Fri, 14 Jul 2023, Wieczor-Retman, Maciej wrote:
>>>> On 14.07.2023 01:00, Reinette Chatre wrote:
>>>>> Hi Ilpo,
>>>>>
>>>>> On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
>>>>>> MBA and MBM tests to use megabytes to represent span. CMT test uses
>>>>>> bytes. The difference requires run_benchmark() to size the buffer
>>>>>> differently based on the test name, which in turn requires passing the
>>>>>> test name into run_benchmark().
>>>>>>
>>>>>> Convert MBA and MBM tests to use internally bytes like CMT test to
>>>>>> remove the internal inconsistency between the tests. Remove the test
>>>>>> dependent buffer sizing from run_benchmark().
>>>>>
>>>>> If I understand correctly the intention is to always use bytes internally
>>>>> and only convert to megabytes when displayed to user space. The above
>>>>> implies that this takes care of the conversion but there still seems
>>>>> to be places that that do not follow my understanding. For example,
>>>>> resctrl_val.c:measure_vals() converts to megabytes before proceeding.
>>>>
>>>> Doesn't the use case inside resctrl_val.c:measure_vals() satisfy
>>>> the idea of only displaying data to the user space? From my
>>>> understanding it reads the number of bytes and only converts to
>>>> MB when printing the value. Or did I miss some detail there?
>>>
>>> It's for printing there yes.
>>>
>>> But it's not about span in the first place so I'm not sure why it is 
>>> related.
>>>
>>
>> If this change is just about how "span" is interpreted by the different
>> tests then the changelog could be more specific to not create expectation
>> that with this change there are no longer "bytes vs megabytes" internal
>> inconsistency between MBA, MBM, and CMT tests.
> 
> The shortlog and changelog are already pretty specific in mentioning 
> "span" a few times :-). I added yet another "span" into the changelog's 
> 2nd paragraph.

There are many things to consider when reviewing a patch. There is the code
changes in the patch itself that can be reviewed for correctness but there is
also the review of the changelog's solution statement to review if the code
changes in the patch accomplishes the stated solution. In one sense the review
considers the code in the patch and in another the review considers what
code may be missing from the patch.

I looked at the v5 changelog and I am still left with the same impression:
The changelog claims that this change removes internal consistency between
the tests, which it does not. Since you posted the next version before this
discussion completed (which is not ideal) I'll respond further to that patch.
In the end it may just be better to drop the "remove the internal inconsistency
between the tests" claim.

> Your general observation about the other MB/bytes inconsistency is still
> a good one so I added it also to my todo list, it just doesn't belong to 
> this patch (IMHO).

ok

Reinette
 

Re: [PATCH v4 10/19] selftests/resctrl: Express span internally in bytes

[Ilpo Järvinen]

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

On Mon, 17 Jul 2023, Reinette Chatre wrote:
> On 7/17/2023 5:30 AM, Ilpo Järvinen wrote:
> > On Fri, 14 Jul 2023, Reinette Chatre wrote:
> >> On 7/14/2023 3:22 AM, Ilpo Järvinen wrote:
> >>> On Fri, 14 Jul 2023, Wieczor-Retman, Maciej wrote:
> >>>> On 14.07.2023 01:00, Reinette Chatre wrote:
> >>>>> Hi Ilpo,
> >>>>>
> >>>>> On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> >>>>>> MBA and MBM tests to use megabytes to represent span. CMT test uses
> >>>>>> bytes. The difference requires run_benchmark() to size the buffer
> >>>>>> differently based on the test name, which in turn requires passing the
> >>>>>> test name into run_benchmark().
> >>>>>>
> >>>>>> Convert MBA and MBM tests to use internally bytes like CMT test to
> >>>>>> remove the internal inconsistency between the tests. Remove the test
> >>>>>> dependent buffer sizing from run_benchmark().
> >>>>>
> >>>>> If I understand correctly the intention is to always use bytes internally
> >>>>> and only convert to megabytes when displayed to user space. The above
> >>>>> implies that this takes care of the conversion but there still seems
> >>>>> to be places that that do not follow my understanding. For example,
> >>>>> resctrl_val.c:measure_vals() converts to megabytes before proceeding.
> >>>>
> >>>> Doesn't the use case inside resctrl_val.c:measure_vals() satisfy
> >>>> the idea of only displaying data to the user space? From my
> >>>> understanding it reads the number of bytes and only converts to
> >>>> MB when printing the value. Or did I miss some detail there?
> >>>
> >>> It's for printing there yes.
> >>>
> >>> But it's not about span in the first place so I'm not sure why it is 
> >>> related.
> >>>
> >>
> >> If this change is just about how "span" is interpreted by the different
> >> tests then the changelog could be more specific to not create expectation
> >> that with this change there are no longer "bytes vs megabytes" internal
> >> inconsistency between MBA, MBM, and CMT tests.
> > 
> > The shortlog and changelog are already pretty specific in mentioning 
> > "span" a few times :-). I added yet another "span" into the changelog's 
> > 2nd paragraph.
> 
> There are many things to consider when reviewing a patch. There is the code
> changes in the patch itself that can be reviewed for correctness but there is
> also the review of the changelog's solution statement to review if the code
> changes in the patch accomplishes the stated solution. In one sense the review
> considers the code in the patch and in another the review considers what
> code may be missing from the patch.

Sure review can enlarge the scope, however, if I add some other things 
unrelated to span into this patch, it no longer is in the minimal form.

> I looked at the v5 changelog and I am still left with the same impression:
> The changelog claims that this change removes internal consistency between
> the tests, which it does not. Since you posted the next version before this
> discussion completed (which is not ideal) I'll respond further to that patch.
> In the end it may just be better to drop the "remove the internal inconsistency
> between the tests" claim.

I'm sorry, it was unintentional to send it like that. ...I honestly 
thought I had addressed all the concerns you had by adding yet another 
"span" (even if the scope was already pretty clear even w/o it, IMHO).

I see nowhere in the changelog mentioned that this change removes all 
internal inconsistencies between the test types. But I'm not attached to 
any particular wording, if the text is good enough for you w/o the 
internal inconsistency wording, I can cut that out.


-- 
 i.

Re: [PATCH v4 10/19] selftests/resctrl: Express span internally in bytes

[Reinette Chatre]

Hi Maciej,

On 7/13/2023 11:43 PM, Wieczor-Retman, Maciej wrote:
> Hi Reinette!
> 
> On 14.07.2023 01:00, Reinette Chatre wrote:
>> Hi Ilpo,
>>
>> On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
>>> MBA and MBM tests to use megabytes to represent span. CMT test uses
>>> bytes. The difference requires run_benchmark() to size the buffer
>>> differently based on the test name, which in turn requires passing the
>>> test name into run_benchmark().
>>>
>>> Convert MBA and MBM tests to use internally bytes like CMT test to
>>> remove the internal inconsistency between the tests. Remove the test
>>> dependent buffer sizing from run_benchmark().
>>
>> If I understand correctly the intention is to always use bytes internally
>> and only convert to megabytes when displayed to user space. The above
>> implies that this takes care of the conversion but there still seems
>> to be places that that do not follow my understanding. For example,
>> resctrl_val.c:measure_vals() converts to megabytes before proceeding.
> 
> Doesn't the use case inside resctrl_val.c:measure_vals() satisfy
> the idea of only displaying data to the user space? From my
> understanding it reads the number of bytes and only converts to
> MB when printing the value. Or did I miss some detail there?

For MBM and MBA tests resctrl_val.c:measure_vals() converts data
to MB before it writes the raw data to the file. Compare to CMT
test that uses cache.c:measure_cache_vals() to write raw data
in bytes to file.

On a high level I see the MBA, MBM, and CMT tests in three stages:
(1) A "benchmark" is run in background, 
(2) raw data is collected at intervals and written to a file,
(3) finally, data from file is processed and results displayed to user.

From what I can tell MBA and MBM tests continue to write data to file
in MB while CMT writes data in bytes.

The changelog claims "Convert MBA and MBM tests to use internally
bytes like CMT test to remove the internal inconsistency between
the tests.". This claim is not clear to me because from what I can tell
the three tests continue to handle data inconsistently after this change.

Reinette

[PATCH v4 11/19] selftests/resctrl: Remove duplicated preparation for span arg

[Ilpo Järvinen]

When no benchmark_cmd is given, benchmark_cmd[1] is set to span in
main(). There's no need to do it again in run_mba_test().

Remove the duplicated preparation for span argument into
benchmark_cmd[1] from run_mba_test(). After this, the has_ben and span
arguments to run_mba_test() can be removed.

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/resctrl_tests.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index 5205a13ed250..6e693f381a6e 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -99,8 +99,7 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, size_t span,
 	umount_resctrlfs();
 }
 
-static void run_mba_test(bool has_ben, char **benchmark_cmd, size_t span,
-			 int cpu_no, char *bw_report)
+static void run_mba_test(char **benchmark_cmd, int cpu_no, char *bw_report)
 {
 	int res;
 
@@ -117,8 +116,6 @@ static void run_mba_test(bool has_ben, char **benchmark_cmd, size_t span,
 		goto umount;
 	}
 
-	if (!has_ben)
-		sprintf(benchmark_cmd[1], "%zu", span);
 	res = mba_schemata_change(cpu_no, bw_report, benchmark_cmd);
 	ksft_test_result(!res, "MBA: schemata change\n");
 
@@ -298,7 +295,7 @@ int main(int argc, char **argv)
 		run_mbm_test(has_ben, benchmark_cmd, span, cpu_no, bw_report);
 
 	if (mba_test)
-		run_mba_test(has_ben, benchmark_cmd, span, cpu_no, bw_report);
+		run_mba_test(benchmark_cmd, cpu_no, bw_report);
 
 	if (cmt_test)
 		run_cmt_test(has_ben, benchmark_cmd, cpu_no);
-- 
2.30.2

Re: [PATCH v4 11/19] selftests/resctrl: Remove duplicated preparation for span arg

[Reinette Chatre]

Hi Ilpo,

On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> When no benchmark_cmd is given, benchmark_cmd[1] is set to span in
> main(). There's no need to do it again in run_mba_test().
> 
> Remove the duplicated preparation for span argument into
> benchmark_cmd[1] from run_mba_test(). After this, the has_ben and span
> arguments to run_mba_test() can be removed.
> 
> 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>
> ---

Thank you.

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

[PATCH v4 12/19] selftests/resctrl: Remove "malloc_and_init_memory" param from run_fill_buf()

[Ilpo Järvinen]

run_fill_buf()'s malloc_and_init_memory parameter is always 1. There's
also duplicated memory init code for malloc_and_init_memory == 0 case
in fill_buf() which is unused.

Remove the malloc_and_init_memory parameter and the duplicated mem init
code.

While at it, fix also a typo in run_fill_buf() prototype's argument.

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       |  6 ++--
 tools/testing/selftests/resctrl/fill_buf.c    | 28 +++----------------
 tools/testing/selftests/resctrl/resctrl.h     |  3 +-
 .../testing/selftests/resctrl/resctrl_tests.c | 13 ++++-----
 tools/testing/selftests/resctrl/resctrlfs.c   | 12 ++++----
 5 files changed, 19 insertions(+), 43 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
index f2c7d322bd76..0db97b6a542f 100644
--- a/tools/testing/selftests/resctrl/cache.c
+++ b/tools/testing/selftests/resctrl/cache.c
@@ -211,7 +211,7 @@ int measure_cache_vals(struct resctrl_val_param *param, int bm_pid)
  */
 int cat_val(struct resctrl_val_param *param)
 {
-	int malloc_and_init_memory = 1, memflush = 1, operation = 0, ret = 0;
+	int memflush = 1, operation = 0, ret = 0;
 	char *resctrl_val = param->resctrl_val;
 	pid_t bm_pid;
 
@@ -248,8 +248,8 @@ int cat_val(struct resctrl_val_param *param)
 			if (ret)
 				break;
 
-			if (run_fill_buf(param->span, malloc_and_init_memory,
-					 memflush, operation, resctrl_val)) {
+			if (run_fill_buf(param->span, memflush, operation,
+					 resctrl_val)) {
 				fprintf(stderr, "Error-running fill buffer\n");
 				ret = -1;
 				close(fd_lm);
diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c
index 785cbd8d0148..d8f5505eb9e6 100644
--- a/tools/testing/selftests/resctrl/fill_buf.c
+++ b/tools/testing/selftests/resctrl/fill_buf.c
@@ -138,36 +138,18 @@ static int fill_cache_write(unsigned char *start_ptr, unsigned char *end_ptr,
 	return 0;
 }
 
-static int
-fill_cache(size_t buf_size, int malloc_and_init, int memflush,
-	   int op, char *resctrl_val)
+static int fill_cache(size_t buf_size, int memflush, int op, char *resctrl_val)
 {
 	unsigned char *start_ptr, *end_ptr;
-	unsigned long long i;
 	int ret;
 
-	if (malloc_and_init)
-		start_ptr = malloc_and_init_memory(buf_size);
-	else
-		start_ptr = malloc(buf_size);
-
+	start_ptr = malloc_and_init_memory(buf_size);
 	if (!start_ptr)
 		return -1;
 
 	startptr = start_ptr;
 	end_ptr = start_ptr + buf_size;
 
-	/*
-	 * It's better to touch the memory once to avoid any compiler
-	 * optimizations
-	 */
-	if (!malloc_and_init) {
-		for (i = 0; i < buf_size; i++)
-			*start_ptr++ = (unsigned char)rand();
-	}
-
-	start_ptr = startptr;
-
 	/* Flush the memory before using to avoid "cache hot pages" effect */
 	if (memflush)
 		mem_flush(start_ptr, buf_size);
@@ -188,14 +170,12 @@ fill_cache(size_t buf_size, int malloc_and_init, int memflush,
 	return 0;
 }
 
-int run_fill_buf(size_t span, int malloc_and_init_memory, int memflush, int op,
-		 char *resctrl_val)
+int run_fill_buf(size_t span, int memflush, int op, char *resctrl_val)
 {
 	size_t cache_size = span;
 	int ret;
 
-	ret = fill_cache(cache_size, malloc_and_init_memory, memflush, op,
-			 resctrl_val);
+	ret = fill_cache(cache_size, memflush, op, resctrl_val);
 	if (ret) {
 		printf("\n Error in fill cache\n");
 		return -1;
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index 52068ceea956..3054cc4ef0e3 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -97,8 +97,7 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp,
 			    char *resctrl_val);
 int perf_event_open(struct perf_event_attr *hw_event, pid_t pid, int cpu,
 		    int group_fd, unsigned long flags);
-int run_fill_buf(size_t span, int malloc_and_init_memory, int memflush, int op,
-		 char *resctrl_va);
+int run_fill_buf(size_t span, int memflush, int op, char *resctrl_val);
 int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param);
 int mbm_bw_change(size_t span, int cpu_no, char *bw_report, char **benchmark_cmd);
 void tests_cleanup(void);
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index 6e693f381a6e..0c313054326d 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -89,7 +89,7 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, size_t span,
 	}
 
 	if (!has_ben)
-		sprintf(benchmark_cmd[5], "%s", MBA_STR);
+		sprintf(benchmark_cmd[4], "%s", MBA_STR);
 	res = mbm_bw_change(span, cpu_no, bw_report, benchmark_cmd);
 	ksft_test_result(!res, "MBM: bw change\n");
 	if ((get_vendor() == ARCH_INTEL) && res)
@@ -141,7 +141,7 @@ static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no)
 	}
 
 	if (!has_ben)
-		sprintf(benchmark_cmd[5], "%s", CMT_STR);
+		sprintf(benchmark_cmd[4], "%s", CMT_STR);
 	res = cmt_resctrl_val(cpu_no, 5, benchmark_cmd);
 	ksft_test_result(!res, "CMT: test\n");
 	if ((get_vendor() == ARCH_INTEL) && res)
@@ -267,16 +267,15 @@ int main(int argc, char **argv)
 		benchmark_cmd[ben_count] = NULL;
 	} else {
 		/* If no benchmark is given by "-b" argument, use fill_buf. */
-		for (i = 0; i < 6; i++)
+		for (i = 0; i < 5; i++)
 			benchmark_cmd[i] = benchmark_cmd_area[i];
 
 		strcpy(benchmark_cmd[0], "fill_buf");
 		sprintf(benchmark_cmd[1], "%zu", span);
 		strcpy(benchmark_cmd[2], "1");
-		strcpy(benchmark_cmd[3], "1");
-		strcpy(benchmark_cmd[4], "0");
-		strcpy(benchmark_cmd[5], "");
-		benchmark_cmd[6] = NULL;
+		strcpy(benchmark_cmd[3], "0");
+		strcpy(benchmark_cmd[4], "");
+		benchmark_cmd[5] = NULL;
 	}
 
 	sprintf(bw_report, "reads");
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index 847e0a80c6cd..5f41ce03c470 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -297,7 +297,7 @@ int taskset_benchmark(pid_t bm_pid, int cpu_no)
  */
 void run_benchmark(int signum, siginfo_t *info, void *ucontext)
 {
-	int operation, ret, malloc_and_init_memory, memflush;
+	int operation, ret, memflush;
 	char **benchmark_cmd;
 	char resctrl_val[64];
 	size_t span;
@@ -316,13 +316,11 @@ void run_benchmark(int signum, siginfo_t *info, void *ucontext)
 	if (strcmp(benchmark_cmd[0], "fill_buf") == 0) {
 		/* Execute default fill_buf benchmark */
 		span = strtoul(benchmark_cmd[1], NULL, 10);
-		malloc_and_init_memory = atoi(benchmark_cmd[2]);
-		memflush =  atoi(benchmark_cmd[3]);
-		operation = atoi(benchmark_cmd[4]);
-		sprintf(resctrl_val, "%s", benchmark_cmd[5]);
+		memflush =  atoi(benchmark_cmd[2]);
+		operation = atoi(benchmark_cmd[3]);
+		sprintf(resctrl_val, "%s", benchmark_cmd[4]);
 
-		if (run_fill_buf(span, malloc_and_init_memory, memflush,
-				 operation, resctrl_val))
+		if (run_fill_buf(span, memflush, operation, resctrl_val))
 			fprintf(stderr, "Error in running fill buffer\n");
 	} else {
 		/* Execute specified benchmark */
-- 
2.30.2

Re: [PATCH v4 12/19] selftests/resctrl: Remove "malloc_and_init_memory" param from run_fill_buf()

[Reinette Chatre]

Hi Ilpo,

On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> run_fill_buf()'s malloc_and_init_memory parameter is always 1. There's
> also duplicated memory init code for malloc_and_init_memory == 0 case
> in fill_buf() which is unused.
> 
> Remove the malloc_and_init_memory parameter and the duplicated mem init
> code.
> 
> While at it, fix also a typo in run_fill_buf() prototype's argument.
> 
> 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>

Thank you.

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

[PATCH v4 13/19] selftests/resctrl: Remove unnecessary startptr global from fill_buf

[Ilpo Järvinen]

fill_buf stores buffer pointer into global variable startptr that is
only used in fill_cache().

Remove startptr as global variable, the local variable in fill_cache()
is enough to keep the pointer.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/fill_buf.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c
index d8f5505eb9e6..a5ec9c82a960 100644
--- a/tools/testing/selftests/resctrl/fill_buf.c
+++ b/tools/testing/selftests/resctrl/fill_buf.c
@@ -22,8 +22,6 @@
 #define PAGE_SIZE		(4 * 1024)
 #define MB			(1024 * 1024)
 
-static unsigned char *startptr;
-
 static void sb(void)
 {
 #if defined(__i386) || defined(__x86_64)
@@ -147,7 +145,6 @@ static int fill_cache(size_t buf_size, int memflush, int op, char *resctrl_val)
 	if (!start_ptr)
 		return -1;
 
-	startptr = start_ptr;
 	end_ptr = start_ptr + buf_size;
 
 	/* Flush the memory before using to avoid "cache hot pages" effect */
@@ -159,7 +156,7 @@ static int fill_cache(size_t buf_size, int memflush, int op, char *resctrl_val)
 	else
 		ret = fill_cache_write(start_ptr, end_ptr, resctrl_val);
 
-	free(startptr);
+	free(start_ptr);
 
 	if (ret) {
 		printf("\n Error in fill cache read/write...\n");
-- 
2.30.2

Re: [PATCH v4 13/19] selftests/resctrl: Remove unnecessary startptr global from fill_buf

[Reinette Chatre]

Hi Ilpo,

On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> fill_buf stores buffer pointer into global variable startptr that is
> only used in fill_cache().
> 
> Remove startptr as global variable, the local variable in fill_cache()
> is enough to keep the pointer.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---

Thank you.

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

[PATCH v4 14/19] selftests/resctrl: Improve parameter consistency in fill_buf

[Ilpo Järvinen]

fill_buf's arguments can be improved in multiple ways:

  - Multiple functions in fill_buf have start_ptr as one of their
    argument which is a bit long and the extra "start" is pretty
    obvious when it comes to pointers.

  - Some of the functions take end_ptr and others size_t to indicate
    the end of the buffer.

  - Some arguments meaning buffer size are called just 's'

  - mem_flush() takes void * but immediately converts it to char *

Cleanup the parameters to make things simpler and more consistent:

  - Rename start_ptr to simply buf as it's shorter.

  - Replace end_ptr and s parameters with buf_size and only calculate
    end_ptr in the functions that truly use it.

  - Make mem_flush() parameters to follow the same convention as the
    other functions in fill_buf.

  - convert mem_flush() char * to unsigned char *.

While at it, fix also a typo in a comment.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/fill_buf.c | 49 +++++++++++-----------
 1 file changed, 24 insertions(+), 25 deletions(-)

diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c
index a5ec9c82a960..5f16c4f5dfbf 100644
--- a/tools/testing/selftests/resctrl/fill_buf.c
+++ b/tools/testing/selftests/resctrl/fill_buf.c
@@ -38,32 +38,32 @@ static void cl_flush(void *p)
 #endif
 }
 
-static void mem_flush(void *p, size_t s)
+static void mem_flush(unsigned char *buf, size_t buf_size)
 {
-	char *cp = (char *)p;
+	unsigned char *cp = buf;
 	size_t i = 0;
 
-	s = s / CL_SIZE; /* mem size in cache llines */
+	buf_size = buf_size / CL_SIZE; /* mem size in cache lines */
 
-	for (i = 0; i < s; i++)
+	for (i = 0; i < buf_size; i++)
 		cl_flush(&cp[i * CL_SIZE]);
 
 	sb();
 }
 
-static void *malloc_and_init_memory(size_t s)
+static void *malloc_and_init_memory(size_t buf_size)
 {
 	void *p = NULL;
 	uint64_t *p64;
 	size_t s64;
 	int ret;
 
-	ret = posix_memalign(&p, PAGE_SIZE, s);
+	ret = posix_memalign(&p, PAGE_SIZE, buf_size);
 	if (ret < 0)
 		return NULL;
 
 	p64 = (uint64_t *)p;
-	s64 = s / sizeof(uint64_t);
+	s64 = buf_size / sizeof(uint64_t);
 
 	while (s64 > 0) {
 		*p64 = (uint64_t)rand();
@@ -74,12 +74,13 @@ static void *malloc_and_init_memory(size_t s)
 	return p;
 }
 
-static int fill_one_span_read(unsigned char *start_ptr, unsigned char *end_ptr)
+static int fill_one_span_read(unsigned char *buf, size_t buf_size)
 {
+	unsigned char *end_ptr = buf + buf_size;
 	unsigned char sum, *p;
 
 	sum = 0;
-	p = start_ptr;
+	p = buf;
 	while (p < end_ptr) {
 		sum += *p;
 		p += (CL_SIZE / 2);
@@ -88,26 +89,26 @@ static int fill_one_span_read(unsigned char *start_ptr, unsigned char *end_ptr)
 	return sum;
 }
 
-static
-void fill_one_span_write(unsigned char *start_ptr, unsigned char *end_ptr)
+static void fill_one_span_write(unsigned char *buf, size_t buf_size)
 {
+	unsigned char *end_ptr = buf + buf_size;
 	unsigned char *p;
 
-	p = start_ptr;
+	p = buf;
 	while (p < end_ptr) {
 		*p = '1';
 		p += (CL_SIZE / 2);
 	}
 }
 
-static int fill_cache_read(unsigned char *start_ptr, unsigned char *end_ptr,
+static int fill_cache_read(unsigned char *buf, size_t buf_size,
 			   char *resctrl_val)
 {
 	int ret = 0;
 	FILE *fp;
 
 	while (1) {
-		ret = fill_one_span_read(start_ptr, end_ptr);
+		ret = fill_one_span_read(buf, buf_size);
 		if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)))
 			break;
 	}
@@ -124,11 +125,11 @@ static int fill_cache_read(unsigned char *start_ptr, unsigned char *end_ptr,
 	return 0;
 }
 
-static int fill_cache_write(unsigned char *start_ptr, unsigned char *end_ptr,
+static int fill_cache_write(unsigned char *buf, size_t buf_size,
 			    char *resctrl_val)
 {
 	while (1) {
-		fill_one_span_write(start_ptr, end_ptr);
+		fill_one_span_write(buf, buf_size);
 		if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)))
 			break;
 	}
@@ -138,25 +139,23 @@ static int fill_cache_write(unsigned char *start_ptr, unsigned char *end_ptr,
 
 static int fill_cache(size_t buf_size, int memflush, int op, char *resctrl_val)
 {
-	unsigned char *start_ptr, *end_ptr;
+	unsigned char *buf;
 	int ret;
 
-	start_ptr = malloc_and_init_memory(buf_size);
-	if (!start_ptr)
+	buf = malloc_and_init_memory(buf_size);
+	if (!buf)
 		return -1;
 
-	end_ptr = start_ptr + buf_size;
-
 	/* Flush the memory before using to avoid "cache hot pages" effect */
 	if (memflush)
-		mem_flush(start_ptr, buf_size);
+		mem_flush(buf, buf_size);
 
 	if (op == 0)
-		ret = fill_cache_read(start_ptr, end_ptr, resctrl_val);
+		ret = fill_cache_read(buf, buf_size, resctrl_val);
 	else
-		ret = fill_cache_write(start_ptr, end_ptr, resctrl_val);
+		ret = fill_cache_write(buf, buf_size, resctrl_val);
 
-	free(start_ptr);
+	free(buf);
 
 	if (ret) {
 		printf("\n Error in fill cache read/write...\n");
-- 
2.30.2

Re: [PATCH v4 14/19] selftests/resctrl: Improve parameter consistency in fill_buf

[Reinette Chatre]

Hi Ilpo,

On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> fill_buf's arguments can be improved in multiple ways:
> 
>   - Multiple functions in fill_buf have start_ptr as one of their
>     argument which is a bit long and the extra "start" is pretty
>     obvious when it comes to pointers.
> 
>   - Some of the functions take end_ptr and others size_t to indicate
>     the end of the buffer.
> 
>   - Some arguments meaning buffer size are called just 's'
> 
>   - mem_flush() takes void * but immediately converts it to char *
> 
> Cleanup the parameters to make things simpler and more consistent:
> 
>   - Rename start_ptr to simply buf as it's shorter.
> 
>   - Replace end_ptr and s parameters with buf_size and only calculate
>     end_ptr in the functions that truly use it.
> 
>   - Make mem_flush() parameters to follow the same convention as the
>     other functions in fill_buf.
> 
>   - convert mem_flush() char * to unsigned char *.
> 
> While at it, fix also a typo in a comment.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

This makes the code significantly easier to read. Thank you.

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

[PATCH v4 15/19] selftests/resctrl: Don't pass test name to fill_buf

[Ilpo Järvinen]

Test name is passed to fill_buf functions so that they can loop around
buffer only once. This is required for CAT test case.

To loop around buffer only once, caller doesn't need to let fill_buf
know which test case it is. Instead, pass a boolean argument 'once'
which makes fill_buf more generic.

As run_benchmark() no longer needs to pass the test name to
run_fill_buf(), a few test running functions can be simplified to not
write the test name into the default benchmark_cmd. The has_ben
argument can also be removed now from those test running functions.

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       |  3 +--
 tools/testing/selftests/resctrl/fill_buf.c    | 20 +++++++++----------
 tools/testing/selftests/resctrl/resctrl.h     |  2 +-
 .../testing/selftests/resctrl/resctrl_tests.c | 14 +++++--------
 tools/testing/selftests/resctrl/resctrlfs.c   | 11 +++++++---
 5 files changed, 24 insertions(+), 26 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
index 0db97b6a542f..bf758f2e5578 100644
--- a/tools/testing/selftests/resctrl/cache.c
+++ b/tools/testing/selftests/resctrl/cache.c
@@ -248,8 +248,7 @@ int cat_val(struct resctrl_val_param *param)
 			if (ret)
 				break;
 
-			if (run_fill_buf(param->span, memflush, operation,
-					 resctrl_val)) {
+			if (run_fill_buf(param->span, memflush, operation, true)) {
 				fprintf(stderr, "Error-running fill buffer\n");
 				ret = -1;
 				close(fd_lm);
diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c
index 5f16c4f5dfbf..0d425f26583a 100644
--- a/tools/testing/selftests/resctrl/fill_buf.c
+++ b/tools/testing/selftests/resctrl/fill_buf.c
@@ -101,15 +101,14 @@ static void fill_one_span_write(unsigned char *buf, size_t buf_size)
 	}
 }
 
-static int fill_cache_read(unsigned char *buf, size_t buf_size,
-			   char *resctrl_val)
+static int fill_cache_read(unsigned char *buf, size_t buf_size, bool once)
 {
 	int ret = 0;
 	FILE *fp;
 
 	while (1) {
 		ret = fill_one_span_read(buf, buf_size);
-		if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)))
+		if (once)
 			break;
 	}
 
@@ -125,19 +124,18 @@ static int fill_cache_read(unsigned char *buf, size_t buf_size,
 	return 0;
 }
 
-static int fill_cache_write(unsigned char *buf, size_t buf_size,
-			    char *resctrl_val)
+static int fill_cache_write(unsigned char *buf, size_t buf_size, bool once)
 {
 	while (1) {
 		fill_one_span_write(buf, buf_size);
-		if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)))
+		if (once)
 			break;
 	}
 
 	return 0;
 }
 
-static int fill_cache(size_t buf_size, int memflush, int op, char *resctrl_val)
+static int fill_cache(size_t buf_size, int memflush, int op, bool once)
 {
 	unsigned char *buf;
 	int ret;
@@ -151,9 +149,9 @@ static int fill_cache(size_t buf_size, int memflush, int op, char *resctrl_val)
 		mem_flush(buf, buf_size);
 
 	if (op == 0)
-		ret = fill_cache_read(buf, buf_size, resctrl_val);
+		ret = fill_cache_read(buf, buf_size, once);
 	else
-		ret = fill_cache_write(buf, buf_size, resctrl_val);
+		ret = fill_cache_write(buf, buf_size, once);
 
 	free(buf);
 
@@ -166,12 +164,12 @@ static int fill_cache(size_t buf_size, int memflush, int op, char *resctrl_val)
 	return 0;
 }
 
-int run_fill_buf(size_t span, int memflush, int op, char *resctrl_val)
+int run_fill_buf(size_t span, int memflush, int op, bool once)
 {
 	size_t cache_size = span;
 	int ret;
 
-	ret = fill_cache(cache_size, memflush, op, resctrl_val);
+	ret = fill_cache(cache_size, memflush, op, once);
 	if (ret) {
 		printf("\n Error in fill cache\n");
 		return -1;
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index 3054cc4ef0e3..645ad407bd8d 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -97,7 +97,7 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp,
 			    char *resctrl_val);
 int perf_event_open(struct perf_event_attr *hw_event, pid_t pid, int cpu,
 		    int group_fd, unsigned long flags);
-int run_fill_buf(size_t span, int memflush, int op, char *resctrl_val);
+int run_fill_buf(size_t span, int memflush, int op, bool once);
 int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param);
 int mbm_bw_change(size_t span, int cpu_no, char *bw_report, char **benchmark_cmd);
 void tests_cleanup(void);
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index 0c313054326d..417698488595 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -70,7 +70,7 @@ void tests_cleanup(void)
 	cat_test_cleanup();
 }
 
-static void run_mbm_test(bool has_ben, char **benchmark_cmd, size_t span,
+static void run_mbm_test(char **benchmark_cmd, size_t span,
 			 int cpu_no, char *bw_report)
 {
 	int res;
@@ -88,8 +88,6 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, size_t span,
 		goto umount;
 	}
 
-	if (!has_ben)
-		sprintf(benchmark_cmd[4], "%s", MBA_STR);
 	res = mbm_bw_change(span, cpu_no, bw_report, benchmark_cmd);
 	ksft_test_result(!res, "MBM: bw change\n");
 	if ((get_vendor() == ARCH_INTEL) && res)
@@ -123,7 +121,7 @@ static void run_mba_test(char **benchmark_cmd, int cpu_no, char *bw_report)
 	umount_resctrlfs();
 }
 
-static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no)
+static void run_cmt_test(char **benchmark_cmd, int cpu_no)
 {
 	int res;
 
@@ -140,8 +138,6 @@ static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no)
 		goto umount;
 	}
 
-	if (!has_ben)
-		sprintf(benchmark_cmd[4], "%s", CMT_STR);
 	res = cmt_resctrl_val(cpu_no, 5, benchmark_cmd);
 	ksft_test_result(!res, "CMT: test\n");
 	if ((get_vendor() == ARCH_INTEL) && res)
@@ -274,7 +270,7 @@ int main(int argc, char **argv)
 		sprintf(benchmark_cmd[1], "%zu", span);
 		strcpy(benchmark_cmd[2], "1");
 		strcpy(benchmark_cmd[3], "0");
-		strcpy(benchmark_cmd[4], "");
+		strcpy(benchmark_cmd[4], "false");
 		benchmark_cmd[5] = NULL;
 	}
 
@@ -291,13 +287,13 @@ int main(int argc, char **argv)
 	ksft_set_plan(tests ? : 4);
 
 	if (mbm_test)
-		run_mbm_test(has_ben, benchmark_cmd, span, cpu_no, bw_report);
+		run_mbm_test(benchmark_cmd, span, cpu_no, bw_report);
 
 	if (mba_test)
 		run_mba_test(benchmark_cmd, cpu_no, bw_report);
 
 	if (cmt_test)
-		run_cmt_test(has_ben, benchmark_cmd, cpu_no);
+		run_cmt_test(benchmark_cmd, cpu_no);
 
 	if (cat_test)
 		run_cat_test(cpu_no, no_of_bits);
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index 5f41ce03c470..4373483cc1d6 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -299,8 +299,8 @@ void run_benchmark(int signum, siginfo_t *info, void *ucontext)
 {
 	int operation, ret, memflush;
 	char **benchmark_cmd;
-	char resctrl_val[64];
 	size_t span;
+	bool once;
 	FILE *fp;
 
 	benchmark_cmd = info->si_ptr;
@@ -318,9 +318,14 @@ void run_benchmark(int signum, siginfo_t *info, void *ucontext)
 		span = strtoul(benchmark_cmd[1], NULL, 10);
 		memflush =  atoi(benchmark_cmd[2]);
 		operation = atoi(benchmark_cmd[3]);
-		sprintf(resctrl_val, "%s", benchmark_cmd[4]);
+		if (!strcmp(benchmark_cmd[4], "true"))
+			once = true;
+		else if (!strcmp(benchmark_cmd[4], "false"))
+			once = false;
+		else
+			PARENT_EXIT("Invalid once parameter");
 
-		if (run_fill_buf(span, memflush, operation, resctrl_val))
+		if (run_fill_buf(span, memflush, operation, once))
 			fprintf(stderr, "Error in running fill buffer\n");
 	} else {
 		/* Execute specified benchmark */
-- 
2.30.2

Re: [PATCH v4 15/19] selftests/resctrl: Don't pass test name to fill_buf

[Reinette Chatre]

Hi Ilpo,

On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> Test name is passed to fill_buf functions so that they can loop around
> buffer only once. This is required for CAT test case.
> 
> To loop around buffer only once, caller doesn't need to let fill_buf
> know which test case it is. Instead, pass a boolean argument 'once'
> which makes fill_buf more generic.
> 
> As run_benchmark() no longer needs to pass the test name to
> run_fill_buf(), a few test running functions can be simplified to not
> write the test name into the default benchmark_cmd. The has_ben
> argument can also be removed now from those test running functions.
> 
> 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>

Thank you.

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

[PATCH v4 16/19] selftests/resctrl: Don't use variable argument list for ->setup()

[Ilpo Järvinen]

struct resctrl_val_param has ->setup() function that accepts variable
argument list. All test cases use only 1 argument as input and it's
the struct resctrl_val_param pointer.

Instead of variable argument list, directly pass struct
resctrl_val_param pointer as the only parameter to ->setup().

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       | 2 +-
 tools/testing/selftests/resctrl/cat_test.c    | 8 +-------
 tools/testing/selftests/resctrl/cmt_test.c    | 9 +--------
 tools/testing/selftests/resctrl/mba_test.c    | 8 +-------
 tools/testing/selftests/resctrl/mbm_test.c    | 8 +-------
 tools/testing/selftests/resctrl/resctrl.h     | 3 +--
 tools/testing/selftests/resctrl/resctrl_val.c | 2 +-
 7 files changed, 7 insertions(+), 33 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
index bf758f2e5578..385c01dd3ec6 100644
--- a/tools/testing/selftests/resctrl/cache.c
+++ b/tools/testing/selftests/resctrl/cache.c
@@ -237,7 +237,7 @@ int cat_val(struct resctrl_val_param *param)
 	/* Test runs until the callback setup() tells the test to stop. */
 	while (1) {
 		if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR))) {
-			ret = param->setup(1, param);
+			ret = param->setup(param);
 			if (ret == END_OF_TESTS) {
 				ret = 0;
 				break;
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
index 480db0dc4e0e..42e35269d8b6 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -27,17 +27,11 @@ static unsigned long cache_size;
  * con_mon grp, mon_grp in resctrl FS.
  * Run 5 times in order to get average values.
  */
-static int cat_setup(int num, ...)
+static int cat_setup(struct resctrl_val_param *p)
 {
-	struct resctrl_val_param *p;
 	char schemata[64];
-	va_list param;
 	int ret = 0;
 
-	va_start(param, num);
-	p = va_arg(param, struct resctrl_val_param *);
-	va_end(param);
-
 	/* Run NUM_OF_RUNS times */
 	if (p->num_of_runs >= NUM_OF_RUNS)
 		return END_OF_TESTS;
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index beb0f0687c6d..7214aefb55ed 100644
--- a/tools/testing/selftests/resctrl/cmt_test.c
+++ b/tools/testing/selftests/resctrl/cmt_test.c
@@ -21,15 +21,8 @@ static char cbm_mask[256];
 static unsigned long long_mask;
 static unsigned long cache_size;
 
-static int cmt_setup(int num, ...)
+static int cmt_setup(struct resctrl_val_param *p)
 {
-	struct resctrl_val_param *p;
-	va_list param;
-
-	va_start(param, num);
-	p = va_arg(param, struct resctrl_val_param *);
-	va_end(param);
-
 	/* Run NUM_OF_RUNS times */
 	if (p->num_of_runs >= NUM_OF_RUNS)
 		return END_OF_TESTS;
diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
index 3d53c6c9b9ce..4d2f145804b8 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -22,18 +22,12 @@
  * con_mon grp, mon_grp in resctrl FS.
  * For each allocation, run 5 times in order to get average values.
  */
-static int mba_setup(int num, ...)
+static int mba_setup(struct resctrl_val_param *p)
 {
 	static int runs_per_allocation, allocation = 100;
-	struct resctrl_val_param *p;
 	char allocation_str[64];
-	va_list param;
 	int ret;
 
-	va_start(param, num);
-	p = va_arg(param, struct resctrl_val_param *);
-	va_end(param);
-
 	if (runs_per_allocation >= NUM_OF_RUNS)
 		runs_per_allocation = 0;
 
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index 2d58d4b8a918..c7de6f5977f6 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -86,16 +86,10 @@ static int check_results(size_t span)
 	return ret;
 }
 
-static int mbm_setup(int num, ...)
+static int mbm_setup(struct resctrl_val_param *p)
 {
-	struct resctrl_val_param *p;
-	va_list param;
 	int ret = 0;
 
-	va_start(param, num);
-	p = va_arg(param, struct resctrl_val_param *);
-	va_end(param);
-
 	/* Run NUM_OF_RUNS times */
 	if (p->num_of_runs >= NUM_OF_RUNS)
 		return END_OF_TESTS;
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index 645ad407bd8d..838d1a438f33 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -3,7 +3,6 @@
 #ifndef RESCTRL_H
 #define RESCTRL_H
 #include <stdio.h>
-#include <stdarg.h>
 #include <math.h>
 #include <errno.h>
 #include <sched.h>
@@ -68,7 +67,7 @@ struct resctrl_val_param {
 	char		*bw_report;
 	unsigned long	mask;
 	int		num_of_runs;
-	int		(*setup)(int num, ...);
+	int		(*setup)(struct resctrl_val_param *param);
 };
 
 #define MBM_STR			"mbm"
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index e8f1e6a59f4a..f0f6c5f6e98b 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -759,7 +759,7 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param)
 
 	/* Test runs until the callback setup() tells the test to stop. */
 	while (1) {
-		ret = param->setup(1, param);
+		ret = param->setup(param);
 		if (ret == END_OF_TESTS) {
 			ret = 0;
 			break;
-- 
2.30.2

Re: [PATCH v4 16/19] selftests/resctrl: Don't use variable argument list for ->setup()

[Reinette Chatre]

Hi Ilpo,

On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> struct resctrl_val_param has ->setup() function that accepts variable
> argument list. All test cases use only 1 argument as input and it's
> the struct resctrl_val_param pointer.
> 
> Instead of variable argument list, directly pass struct
> resctrl_val_param pointer as the only parameter to ->setup().
> 
> 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>

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

[PATCH v4 17/19] selftests/resctrl: Move CAT/CMT test global vars to function they are used in

[Ilpo Järvinen]

CAT and CMT tests have count_of_bits, long_mask, cbm_mask, and
cache_size global variables that can be moved into the sole using
function.

Make the global variables local variables of the relevant function to
scope them better.

While at it, move cache_size initialization into the declaration line.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/cat_test.c | 11 ++++-------
 tools/testing/selftests/resctrl/cmt_test.c | 11 ++++-------
 2 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
index 42e35269d8b6..ed6c8e64ad11 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -17,11 +17,6 @@
 #define MAX_DIFF_PERCENT	4
 #define MAX_DIFF		1000000
 
-static int count_of_bits;
-static char cbm_mask[256];
-static unsigned long long_mask;
-static unsigned long cache_size;
-
 /*
  * Change schemata. Write schemata to specified
  * con_mon grp, mon_grp in resctrl FS.
@@ -96,10 +91,12 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
 {
 	unsigned long l_mask, l_mask_1;
 	int ret, pipefd[2], sibling_cpu_no;
+	unsigned long cache_size = 0;
+	unsigned long long_mask;
+	char cbm_mask[256];
+	int count_of_bits;
 	char pipe_message;
 
-	cache_size = 0;
-
 	/* Get default cbm mask for L3/L2 cache */
 	ret = get_cbm_mask(cache_type, cbm_mask);
 	if (ret)
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index 7214aefb55ed..0ac9d6bbd13d 100644
--- a/tools/testing/selftests/resctrl/cmt_test.c
+++ b/tools/testing/selftests/resctrl/cmt_test.c
@@ -16,11 +16,6 @@
 #define MAX_DIFF		2000000
 #define MAX_DIFF_PERCENT	15
 
-static int count_of_bits;
-static char cbm_mask[256];
-static unsigned long long_mask;
-static unsigned long cache_size;
-
 static int cmt_setup(struct resctrl_val_param *p)
 {
 	/* Run NUM_OF_RUNS times */
@@ -75,10 +70,12 @@ void cmt_test_cleanup(void)
 
 int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
 {
+	unsigned long cache_size = 0;
+	unsigned long long_mask;
+	char cbm_mask[256];
+	int count_of_bits;
 	int ret;
 
-	cache_size = 0;
-
 	if (!validate_resctrl_feature_request(CMT_STR))
 		return -1;
 
-- 
2.30.2

Re: [PATCH v4 17/19] selftests/resctrl: Move CAT/CMT test global vars to function they are used in

[Reinette Chatre]

Hi Ilpo,

On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> CAT and CMT tests have count_of_bits, long_mask, cbm_mask, and
> cache_size global variables that can be moved into the sole using
> function.
> 
> Make the global variables local variables of the relevant function to
> scope them better.
> 
> While at it, move cache_size initialization into the declaration line.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---

Thank you.

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

[PATCH v4 18/19] selftests/resctrl: Pass the real number of tests to show_cache_info()

[Ilpo Järvinen]

Results include warm-up test which is discarded before passing the sum
to show_cache_info(). show_cache_info() handles this by subtracting one
from the number of tests in divisor. It is a trappy construct to have
sum and number of tests parameters to disagree like this.

A more logical place for subtracting the skipped tests is where the sum
is calculated so move it there. Pass the correct number of tests to
show_cache_info() so it can be used directly as the divisor for
calculating the average.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/cache.c    | 2 +-
 tools/testing/selftests/resctrl/cat_test.c | 2 +-
 tools/testing/selftests/resctrl/cmt_test.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
index 385c01dd3ec6..5aa112e5fdd3 100644
--- a/tools/testing/selftests/resctrl/cache.c
+++ b/tools/testing/selftests/resctrl/cache.c
@@ -290,7 +290,7 @@ int show_cache_info(unsigned long sum_llc_val, int no_of_bits,
 	long avg_diff = 0;
 	int ret;
 
-	avg_llc_val = sum_llc_val / (num_of_runs - 1);
+	avg_llc_val = sum_llc_val / num_of_runs;
 	avg_diff = (long)abs(cache_span - avg_llc_val);
 	diff_percent = ((float)cache_span - avg_llc_val) / cache_span * 100;
 
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
index ed6c8e64ad11..3848dfb46aba 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -77,7 +77,7 @@ static int check_results(struct resctrl_val_param *param)
 	no_of_bits = count_bits(param->mask);
 
 	return show_cache_info(sum_llc_perf_miss, no_of_bits, param->span / 64,
-			       MAX_DIFF, MAX_DIFF_PERCENT, NUM_OF_RUNS,
+			       MAX_DIFF, MAX_DIFF_PERCENT, runs - 1,
 			       get_vendor() == ARCH_INTEL, false);
 }
 
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index 0ac9d6bbd13d..cb2197647c6c 100644
--- a/tools/testing/selftests/resctrl/cmt_test.c
+++ b/tools/testing/selftests/resctrl/cmt_test.c
@@ -59,7 +59,7 @@ static int check_results(struct resctrl_val_param *param, int no_of_bits)
 	fclose(fp);
 
 	return show_cache_info(sum_llc_occu_resc, no_of_bits, param->span,
-			       MAX_DIFF, MAX_DIFF_PERCENT, NUM_OF_RUNS,
+			       MAX_DIFF, MAX_DIFF_PERCENT, runs - 1,
 			       true, true);
 }
 
-- 
2.30.2

Re: [PATCH v4 18/19] selftests/resctrl: Pass the real number of tests to show_cache_info()

[Reinette Chatre]

Hi Ilpo,

On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> Results include warm-up test which is discarded before passing the sum
> to show_cache_info(). show_cache_info() handles this by subtracting one
> from the number of tests in divisor. It is a trappy construct to have
> sum and number of tests parameters to disagree like this.
> 
> A more logical place for subtracting the skipped tests is where the sum
> is calculated so move it there. Pass the correct number of tests to
> show_cache_info() so it can be used directly as the divisor for
> calculating the average.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---

Thank you.

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

[PATCH v4 19/19] selftests/resctrl: Remove test type checks from cat_val()

[Ilpo Järvinen]

cat_val() is only used during CAT test but it checks for test type.

Remove test type checks and the unused else branch from cat_val().

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/cache.c | 47 +++++++++++--------------
 1 file changed, 21 insertions(+), 26 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
index 5aa112e5fdd3..2758e1a3b255 100644
--- a/tools/testing/selftests/resctrl/cache.c
+++ b/tools/testing/selftests/resctrl/cache.c
@@ -231,37 +231,32 @@ int cat_val(struct resctrl_val_param *param)
 	if (ret)
 		return ret;
 
-	if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)))
-		initialize_llc_perf();
+	initialize_llc_perf();
 
 	/* Test runs until the callback setup() tells the test to stop. */
 	while (1) {
-		if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR))) {
-			ret = param->setup(param);
-			if (ret == END_OF_TESTS) {
-				ret = 0;
-				break;
-			}
-			if (ret < 0)
-				break;
-			ret = reset_enable_llc_perf(bm_pid, param->cpu_no);
-			if (ret)
-				break;
-
-			if (run_fill_buf(param->span, memflush, operation, true)) {
-				fprintf(stderr, "Error-running fill buffer\n");
-				ret = -1;
-				close(fd_lm);
-				break;
-			}
-
-			sleep(1);
-			ret = measure_cache_vals(param, bm_pid);
-			if (ret)
-				break;
-		} else {
+		ret = param->setup(param);
+		if (ret == END_OF_TESTS) {
+			ret = 0;
 			break;
 		}
+		if (ret < 0)
+			break;
+		ret = reset_enable_llc_perf(bm_pid, param->cpu_no);
+		if (ret)
+			break;
+
+		if (run_fill_buf(param->span, memflush, operation, true)) {
+			fprintf(stderr, "Error-running fill buffer\n");
+			ret = -1;
+			close(fd_lm);
+			break;
+		}
+
+		sleep(1);
+		ret = measure_cache_vals(param, bm_pid);
+		if (ret)
+			break;
 	}
 
 	return ret;
-- 
2.30.2

Re: [PATCH v4 19/19] selftests/resctrl: Remove test type checks from cat_val()

[Reinette Chatre]

Hi Ilpo,

On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> cat_val() is only used during CAT test but it checks for test type.
> 
> Remove test type checks and the unused else branch from cat_val().
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Thank you.

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette