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

[Reinette Chatre <reinette.chatre@intel.com>]

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