Re: [PATCH 1/4] selftests/resctrl: Rename variable imcs and num_of_imcs() to generic names

[Moger, Babu]

Hi Ilpo,

On 2/23/24 04:38, Ilpo Järvinen wrote:
> On Thu, 22 Feb 2024, Babu Moger wrote:
> 
>> In an effort to support MBM and MBA tests for AMD, renaming for variable
>> and functions to generic names. For Intel, the memory controller is called
>> Integrated Memory Controllers (IMC). For AMD, it is called Unified
>> Memory Controller (UMC). No functional change.
>>
>> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
>> ---
>>  tools/testing/selftests/resctrl/resctrl_val.c | 25 ++++++++++---------
>>  1 file changed, 13 insertions(+), 12 deletions(-)
>>
>> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
>> index 88789678917b..52e23a8076d3 100644
>> --- a/tools/testing/selftests/resctrl/resctrl_val.c
>> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
>> @@ -60,7 +60,7 @@ struct imc_counter_config {
>>  };
>>  
>>  static char mbm_total_path[1024];
>> -static int imcs;
>> +static int number_of_mem_ctrls;
>>  static struct imc_counter_config imc_counters_config[MAX_IMCS][2];
>>  
>>  void membw_initialize_perf_event_attr(int i, int j)
>> @@ -211,15 +211,16 @@ static int read_from_imc_dir(char *imc_dir, int count)
>>  }
>>  
>>  /*
>> - * A system can have 'n' number of iMC (Integrated Memory Controller)
>> - * counters, get that 'n'. For each iMC counter get it's type and config.
>> + * A system can have 'n' number of iMC (Integrated Memory Controller for
>> + * Intel) counters, get that 'n'. In case of AMD it is called UMC (Unified
>> + * Memory Controller). For each iMC/UMC counter get it's type and config.
>>   * Also, each counter has two configs, one for read and the other for write.
>>   * A config again has two parts, event and umask.
>>   * Enumerate all these details into an array of structures.
>>   *
>>   * Return: >= 0 on success. < 0 on failure.
>>   */
>> -static int num_of_imcs(void)
>> +static int get_number_of_mem_ctrls(void)
> 
> It's a bit odd to shorten "memory" and "controller" but not "number". In 
> fact, I'd prefer to use "controllers" in the longer form because ctrls 
> is ambiguous ("control" vs "controller").
> 
> So perhaps num_of_mem_controllers(void) (or that with get_ prefix if you 
> insist).

Sure. num_of_mem_controllers looks good.

> 
>>  {
>>  	char imc_dir[512], *temp;
>>  	unsigned int count = 0;
>> @@ -279,12 +280,12 @@ static int initialize_mem_bw_imc(void)
>>  {
>>  	int imc, j;
>>  
>> -	imcs = num_of_imcs();
>> -	if (imcs <= 0)
>> -		return imcs;
>> +	number_of_mem_ctrls = get_number_of_mem_ctrls();
>> +	if (number_of_mem_ctrls <= 0)
>> +		return number_of_mem_ctrls;
>>  
>>  	/* Initialize perf_event_attr structures for all iMC's */
>> -	for (imc = 0; imc < imcs; imc++) {
>> +	for (imc = 0; imc < number_of_mem_ctrls; imc++) {
> 
> So you renamed imcs, but not imc. Perhaps the variable names could just be 
> "mc" and "mcs"?

How about mem_ctrls ?
> 
>>  		for (j = 0; j < 2; j++)
>>  			membw_initialize_perf_event_attr(imc, j);
>>  	}
>> @@ -309,7 +310,7 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
>>  
>>  	/* 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 (imc = 0; imc < number_of_mem_ctrls; imc++) {
>>  		for (j = 0; j < 2; j++) {
>>  			ret = open_perf_event(imc, cpu_no, j);
>>  			if (ret)
>> @@ -322,7 +323,7 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
>>  	sleep(1);
>>  
>>  	/* Stop counters after a second to get results (both read and write) */
>> -	for (imc = 0; imc < imcs; imc++) {
>> +	for (imc = 0; imc < number_of_mem_ctrls; imc++) {
>>  		for (j = 0; j < 2; j++)
>>  			membw_ioctl_perf_event_ioc_disable(imc, j);
>>  	}
>> @@ -331,7 +332,7 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
>>  	 * Get results which are stored in struct type imc_counter_config
>>  	 * Take over flow into consideration before calculating total b/w
>>  	 */
>> -	for (imc = 0; imc < imcs; imc++) {
>> +	for (imc = 0; imc < number_of_mem_ctrls; imc++) {
>>  		struct imc_counter_config *r =
>>  			&imc_counters_config[imc][READ];
>>  		struct imc_counter_config *w =
>> @@ -368,7 +369,7 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
>>  		writes += w->return_value.value * of_mul_write * SCALE;
>>  	}
>>  
>> -	for (imc = 0; imc < imcs; imc++) {
>> +	for (imc = 0; imc < number_of_mem_ctrls; imc++) {
>>  		close(imc_counters_config[imc][READ].fd);
>>  		close(imc_counters_config[imc][WRITE].fd);
> 
> I've a yet submitted patch to convert these close() calls to use the 
> loop approach which is consistent with the rest of the code, since you
> will end up touching this when you do imc -> mc rename, it would be 
> preferrable if you add one patch into your series which converts them to:
> 
> 		for (j = 0; j < 2; j++)
> 			close(imc_counters_config[mc][j].fd);

Sure. Will do.

> 
> (Given how long kselftest patches tend to linger unapplied, it would 
> make things a lot easier since those changes will obviously conflict 
> otherwise).
> 

Actually, I can wait for you to submit your patches.  Let me know.

-- 
Thanks
Babu Moger