Re: [PATCH v2 02/13] selftests/resctrl: Calculate resctrl FS derived mem bw over sleep(1) only

[Reinette Chatre]

Hi Ilpo,

On 3/22/2024 5:11 AM, Ilpo Järvinen wrote:
> On Tue, 19 Mar 2024, Reinette Chatre wrote:
>> On 3/11/2024 6:52 AM, Ilpo Järvinen wrote:


>>> -static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
>>> +static int perf_open_imc_mem_bw(int cpu_no)
>>>  {
>>> -	float reads, writes, of_mul_read, of_mul_write;
>>>  	int imc, j, ret;
>>>  
>>> -	/* Start all iMC counters to log values (both read and write) */
>>> -	reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
>>>  	for (imc = 0; imc < imcs; imc++) {
>>>  		for (j = 0; j < 2; j++) {
>>>  			ret = open_perf_event(imc, cpu_no, j);
>>>  			if (ret)
>>>  				return -1;
>>>  		}
>>
>> I'm feeling more strongly that this inner loop makes the code harder to
>> understand and unwinding it would make it easier to understand.
> 
> Okay, I'll unwind them in the first patch.

Thank you very much.

> 
>>>  	}
>>> +}
>>> +
>>> +/*
>>> + * get_mem_bw_imc - Memory band width as reported by iMC counters
>>> + * @bw_report:		Bandwidth report type (reads, writes)
>>> + *
>>> + * Memory B/W utilized by a process on a socket can be calculated using
>>> + * iMC counters. Perf events are used to read these counters.
>>
>> In the above there are three variations of the same: "band width", "Bandwidth",
>> and "B/W". Please just use one and use it consistently.
> 
> Okay but I'll do that in a separate patch because these are just the 
> "removed" lines above, the diff is more messy than the actual change 
> here as is often the case with this kind of split function refactoring 
> because the diff algorithm fails to pair the lines optimally from 
> human-readed PoV.

I have not used these much myself bit I've heard folks having success
getting more readable diffs when using the --patience or --histogram
algorithms.

> 
>>> + * Return: = 0 on success. < 0 on failure.
>>> + */
>>> +static int get_mem_bw_imc(char *bw_report, float *bw_imc)
>>> +{
>>> +	float reads, writes, of_mul_read, of_mul_write;
>>> +	int imc, j;
>>> +
>>> +	/* Start all iMC counters to log values (both read and write) */
>>> +	reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
>>>  
>>>  	/*
>>>  	 * Get results which are stored in struct type imc_counter_config
> 
>>> @@ -696,7 +725,6 @@ int resctrl_val(const struct resctrl_test *test,
>>>  		struct resctrl_val_param *param)
>>>  {
>>>  	char *resctrl_val = param->resctrl_val;
>>> -	unsigned long bw_resc_start = 0;
>>
>> In the current implementation the first iteration's starting measurement
>> is, as seen above, 0 ... which makes the first measurement unreliable
>> and dropped for both the MBA and MBM tests. In this enhancement, the
>> first measurement is no longer skewed so much so I wonder if this enhancement
>> can be expanded to the analysis phase where first measurement no longer
>> needs to be dropped?
> 
> In ideal world, yes, but I'll have to check the raw numbers. My general 
> feel is that the numbers tend to converge slowly with more iterations 
> being run so the first iteration might still be "off" by quite much (this 
> is definitely the case with CAT tests iterations but I'm not entirely sure 
> any more how it is with other selftests).
> 

>From what I can tell the CAT test is not dropping any results. It looks
to me that any "settling" is and should be handled in the test before
the data collection starts.

Reinette