[v2,10/24] selftests/resctrl: Split run_fill_buf() to alloc, work, and dealloc helpers

  MBM, MBA and CMT test cases use run_fill_buf() to loop indefinitely
loop around the buffer. CAT test case is different and doesn't want to
loop around the buffer continuously.

Split run_fill_buf() into helpers so that both the use cases are easier
to control by creating separate functions for buffer allocation,
looping around the buffer and the deallocation. Make those functions
available for tests.

Co-developed-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 tools/testing/selftests/resctrl/fill_buf.c | 46 ++++++++++++++++------
 tools/testing/selftests/resctrl/resctrl.h  |  3 ++
 2 files changed, 37 insertions(+), 12 deletions(-)
  

Hi Ilpo,

On 4/18/2023 4:44 AM, Ilpo Järvinen wrote:
> MBM, MBA and CMT test cases use run_fill_buf() to loop indefinitely
> loop around the buffer. CAT test case is different and doesn't want to

"loop indefinitely loop" -> "loop indefinitely" ?

> loop around the buffer continuously.
> 
> Split run_fill_buf() into helpers so that both the use cases are easier
> to control by creating separate functions for buffer allocation,
> looping around the buffer and the deallocation. Make those functions
> available for tests.
> 
> Co-developed-by: Fenghua Yu <fenghua.yu@intel.com>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  tools/testing/selftests/resctrl/fill_buf.c | 46 ++++++++++++++++------
>  tools/testing/selftests/resctrl/resctrl.h  |  3 ++
>  2 files changed, 37 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c
> index 5cdb421a2f6c..6f0438aa71a6 100644
> --- a/tools/testing/selftests/resctrl/fill_buf.c
> +++ b/tools/testing/selftests/resctrl/fill_buf.c
> @@ -24,6 +24,11 @@
>  
>  static unsigned char *startptr;
>  
> +void free_buffer(void)
> +{
> +	free(startptr);
> +}
> +

From what I understand startptr is a global variable because there used
to be a signal handler that attempted to free the buffer as part of
its cleanup. This was not necessary and this behavior no longer exists,
yet the global buffer pointer remains.
See commit 73c55fa5ab55 ("selftests/resctrl: Commonize the signal handler
register/unregister for all tests")

I do not see why a global buffer pointer with all these indirections
are needed. Why not just use a local pointer and pass it to functions
as needed? In the above case, just call free(pointer) directly from the
test.

>  static void sb(void)
>  {
>  #if defined(__i386) || defined(__x86_64)
> @@ -138,36 +143,53 @@ static int fill_cache_write(unsigned char *start_ptr, unsigned char *end_ptr,
>  	return 0;
>  }
>  
> -static int
> -fill_cache(unsigned long long buf_size, int memflush, int op, char *resctrl_val)
> +int alloc_buffer(unsigned long long buf_size, int memflush)
>  {

This can be an allocation function that returns a pointer to
allocated buffer, NULL if error.

> -	unsigned char *start_ptr, *end_ptr;
> -	int ret;
> +	unsigned char *start_ptr;
>  
>  	start_ptr = malloc_and_init_memory(buf_size);
>  	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 */
>  	if (memflush)
>  		mem_flush(start_ptr, buf_size);
>  
> +	return 0;
> +}
> +
> +int use_buffer(unsigned long long buf_size, int op, char *resctrl_val)
> +{
> +	unsigned char *end_ptr;
> +	int ret;
> +
> +	end_ptr = startptr + buf_size;
>  	if (op == 0)
> -		ret = fill_cache_read(start_ptr, end_ptr, resctrl_val);
> +		ret = fill_cache_read(startptr, end_ptr, resctrl_val);
>  	else
> -		ret = fill_cache_write(start_ptr, end_ptr, resctrl_val);
> +		ret = fill_cache_write(startptr, end_ptr, resctrl_val);
>  
> -	if (ret) {
> +	if (ret)
>  		printf("\n Error in fill cache read/write...\n");
> -		return -1;
> -	}
>  
> -	free(startptr);
> +	return ret;
> +}
>  

This seems like an unnecessary level of abstraction to me. Could
callers not just call fill_cache_read()/fill_cache_write() directly?
I think doing so will make tests easier to understand. Looking ahead
at how cat_val() turns out in the final patch I do think a call
to fill_cache_read() is easier to follow than this abstraction.


> -	return 0;
> +static int
> +fill_cache(unsigned long long buf_size, int memflush, int op, char *resctrl_val)
> +{
> +	int ret;
> +
> +	ret = alloc_buffer(buf_size, memflush);
> +	if (ret)
> +		return ret;
> +
> +	ret = use_buffer(buf_size, op, resctrl_val);
> +	free_buffer();
> +
> +	return ret;
>  }
>  
>  int run_fill_buf(unsigned long span, int memflush, int op, char *resctrl_val)
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index 574adac8932d..75bfa2b2746d 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -95,6 +95,9 @@ 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);
> +void free_buffer(void);
> +int alloc_buffer(unsigned long long buf_size, int memflush);
> +int use_buffer(unsigned long long buf_size, int op, char *resctrl_val);
>  int run_fill_buf(unsigned long span, int memflush, int op, char *resctrl_val);
>  int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param);
>  int mbm_bw_change(unsigned long span, int cpu_no, char *bw_report, char **benchmark_cmd);

Reinette
  

On Fri, 21 Apr 2023, Reinette Chatre wrote:
> On 4/18/2023 4:44 AM, Ilpo Järvinen wrote:
> > 
> > diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c
> > index 5cdb421a2f6c..6f0438aa71a6 100644
> > --- a/tools/testing/selftests/resctrl/fill_buf.c
> > +++ b/tools/testing/selftests/resctrl/fill_buf.c
> > @@ -24,6 +24,11 @@
> >  
> >  static unsigned char *startptr;
> >  
> > +void free_buffer(void)
> > +{
> > +	free(startptr);
> > +}
> > +
> 
> >From what I understand startptr is a global variable because there used
> to be a signal handler that attempted to free the buffer as part of
> its cleanup. This was not necessary and this behavior no longer exists,
> yet the global buffer pointer remains.
> See commit 73c55fa5ab55 ("selftests/resctrl: Commonize the signal handler
> register/unregister for all tests")
> 
> I do not see why a global buffer pointer with all these indirections
> are needed. Why not just use a local pointer and pass it to functions
> as needed? In the above case, just call free(pointer) directly from the
> test.

OK, I'll try to convert all this into using non-global pointers then. It 
requires a bit refactoring but, IIRC, it is doable.

> >  static void sb(void)
> >  {
> >  #if defined(__i386) || defined(__x86_64)
> > @@ -138,36 +143,53 @@ static int fill_cache_write(unsigned char *start_ptr, unsigned char *end_ptr,
> >  	return 0;
> >  }
> >  
> > -static int
> > -fill_cache(unsigned long long buf_size, int memflush, int op, char *resctrl_val)
> > +int alloc_buffer(unsigned long long buf_size, int memflush)
> >  {
> 
> This can be an allocation function that returns a pointer to
> allocated buffer, NULL if error.
>
> > -	unsigned char *start_ptr, *end_ptr;
> > -	int ret;
> > +	unsigned char *start_ptr;
> >  
> >  	start_ptr = malloc_and_init_memory(buf_size);
> >  	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 */
> >  	if (memflush)
> >  		mem_flush(start_ptr, buf_size);
> >  
> > +	return 0;
> > +}
> > +
> > +int use_buffer(unsigned long long buf_size, int op, char *resctrl_val)
> > +{
> > +	unsigned char *end_ptr;
> > +	int ret;
> > +
> > +	end_ptr = startptr + buf_size;
> >  	if (op == 0)
> > -		ret = fill_cache_read(start_ptr, end_ptr, resctrl_val);
> > +		ret = fill_cache_read(startptr, end_ptr, resctrl_val);
> >  	else
> > -		ret = fill_cache_write(start_ptr, end_ptr, resctrl_val);
> > +		ret = fill_cache_write(startptr, end_ptr, resctrl_val);
> >  
> > -	if (ret) {
> > +	if (ret)
> >  		printf("\n Error in fill cache read/write...\n");
> > -		return -1;
> > -	}
> >  
> > -	free(startptr);
> > +	return ret;
> > +}
> >  
> 
> This seems like an unnecessary level of abstraction to me. Could
> callers not just call fill_cache_read()/fill_cache_write() directly?
> I think doing so will make tests easier to understand. Looking ahead
> at how cat_val() turns out in the final patch I do think a call
> to fill_cache_read() is easier to follow than this abstraction.

Passing a custom benchmark command with -b would lose some functionality 
if this abstraction is removed. CAT test could make a direct call though 
as it doesn't care about the benchmark command.

How useful that -b functionality is for selftesting is somewhat 
questionable though.
  

Hi Ilpo,

On 4/24/2023 9:01 AM, Ilpo Järvinen wrote:
> On Fri, 21 Apr 2023, Reinette Chatre wrote:
>> On 4/18/2023 4:44 AM, Ilpo Järvinen wrote:
>>>

...

>>>  static void sb(void)
>>>  {
>>>  #if defined(__i386) || defined(__x86_64)
>>> @@ -138,36 +143,53 @@ static int fill_cache_write(unsigned char *start_ptr, unsigned char *end_ptr,
>>>  	return 0;
>>>  }
>>>  
>>> -static int
>>> -fill_cache(unsigned long long buf_size, int memflush, int op, char *resctrl_val)
>>> +int alloc_buffer(unsigned long long buf_size, int memflush)
>>>  {
>>
>> This can be an allocation function that returns a pointer to
>> allocated buffer, NULL if error.
>>
>>> -	unsigned char *start_ptr, *end_ptr;
>>> -	int ret;
>>> +	unsigned char *start_ptr;
>>>  
>>>  	start_ptr = malloc_and_init_memory(buf_size);
>>>  	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 */
>>>  	if (memflush)
>>>  		mem_flush(start_ptr, buf_size);
>>>  
>>> +	return 0;
>>> +}
>>> +
>>> +int use_buffer(unsigned long long buf_size, int op, char *resctrl_val)
>>> +{
>>> +	unsigned char *end_ptr;
>>> +	int ret;
>>> +
>>> +	end_ptr = startptr + buf_size;
>>>  	if (op == 0)
>>> -		ret = fill_cache_read(start_ptr, end_ptr, resctrl_val);
>>> +		ret = fill_cache_read(startptr, end_ptr, resctrl_val);
>>>  	else
>>> -		ret = fill_cache_write(start_ptr, end_ptr, resctrl_val);
>>> +		ret = fill_cache_write(startptr, end_ptr, resctrl_val);
>>>  
>>> -	if (ret) {
>>> +	if (ret)
>>>  		printf("\n Error in fill cache read/write...\n");
>>> -		return -1;
>>> -	}
>>>  
>>> -	free(startptr);
>>> +	return ret;
>>> +}
>>>  
>>
>> This seems like an unnecessary level of abstraction to me. Could
>> callers not just call fill_cache_read()/fill_cache_write() directly?
>> I think doing so will make tests easier to understand. Looking ahead
>> at how cat_val() turns out in the final patch I do think a call
>> to fill_cache_read() is easier to follow than this abstraction.
> 
> Passing a custom benchmark command with -b would lose some functionality 
> if this abstraction is removed. CAT test could make a direct call though 
> as it doesn't care about the benchmark command.
> 
> How useful that -b functionality is for selftesting is somewhat 
> questionable though.

I do not think we are speaking about the same thing here. I think that
use_buffer() is unnecessary. fill_cache() can just call fill_cache_read()
or fill_cache_write() directly, depending on the op value. Could you please
elaborate how that impacts the custom benchmark?

Looking ahead at patch 24/24: "selftests/resctrl: Rewrite Cache Allocation
Technology (CAT) test" I feel more strongly that use_buffer() is unnecessary
since it adds an unnecessary layer and makes it harder to see what the test
does.  

Reinette