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

[Moger, Babu]

Hi Reinette,
Was doing few other things. Sorry for the delay.

On 5/9/24 16:10, Reinette Chatre wrote:
> Hi Babu,
> 
> On 4/25/2024 1:16 PM, 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.
> 
> This is a resonable change yet the actual changes seem inconsistent to me.
> Per the changelog the goal is to switch from "IMC" specific naming to generic
> "MC" naming in all the code that will be shared between AMD and Intel.
>>From what I can tell this patch only changes *some* of the shared variables,
> functions, and data structures and it is not obvious to me why some are
> changed and some are not. This makes the code inconsistent.

Agree. Will address it in next version.

> 
> There are many examples of the inconsistencies in this patch alone that
> I will try to highlight what I mean without considering areas untouched by
> this patch.
>  
>> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
>> ---
>>  tools/testing/selftests/resctrl/resctrl_val.c | 59 ++++++++++---------
>>  1 file changed, 30 insertions(+), 29 deletions(-)
>>
>> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
>> index 5a49f07a6c85..a30cfcff605f 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 mcs;
>>  static struct imc_counter_config imc_counters_config[MAX_IMCS][2];
> 
> Global "imcs" is changed to "mcs" ... but why are
> global imc_counters_config[][] and its struct imc_counter_config
> not changed?

Yes. Will address it.
> 
>>  
>>  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 num_of_mem_controllers(void)
>>  {
>>  	char imc_dir[512], *temp;
> 
> Similarly, what about imc_dir[]?

Yes. Sure.

> 
>>  	unsigned int count = 0;
>> @@ -275,25 +276,25 @@ static int num_of_imcs(void)
>>  	return count;
>>  }
>>  
>> -static int initialize_mem_bw_imc(void)
>> +static int initialize_mem_bw_mc(void)
>>  {
>> -	int imc, j;
>> +	int mc, j;
>>  
>> -	imcs = num_of_imcs();
>> -	if (imcs <= 0)
>> -		return imcs;
>> +	mcs = num_of_mem_controllers();
>> +	if (mcs <= 0)
>> +		return mcs;
>>  
>>  	/* Initialize perf_event_attr structures for all iMC's */
> 
> Note comment still refers to iMC

Yes.
> 
>> -	for (imc = 0; imc < imcs; imc++) {
>> +	for (mc = 0; mc < mcs; mc++) {
>>  		for (j = 0; j < 2; j++)
>> -			membw_initialize_perf_event_attr(imc, j);
>> +			membw_initialize_perf_event_attr(mc, j);
>>  	}
>>  
>>  	return 0;
>>  }
>>  
>>  /*
>> - * get_mem_bw_imc:	Memory band width as reported by iMC counters
>> + * get_mem_bw_mc:	Memory band width as reported by iMC counters
> 
> Comment still refers to iMC

Will address it.

> 
>>   * @cpu_no:		CPU number that the benchmark PID is binded to
>>   * @bw_report:		Bandwidth report type (reads, writes)
>>   *
>> @@ -302,40 +303,40 @@ static int initialize_mem_bw_imc(void)
>>   *
>>   * Return: = 0 on success. < 0 on failure.
>>   */
>> -static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
>> +static int get_mem_bw_mc(int cpu_no, char *bw_report, float *bw_imc)
> 
> The intent of the function is to "get" bw_mc ... so not renaming "bw_imc"
> seems like a miss. Especially when considering that its caller does just this.

Yes. Will take care of this.

> 
>>  {
>>  	float reads, writes, of_mul_read, of_mul_write;
>> -	int imc, j, ret;
>> +	int mc, j, ret;
>>  
>>  	/* Start all iMC counters to log values (both read and write) */
> 
> iMC?

Sure.
> 
>>  	reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
>> -	for (imc = 0; imc < imcs; imc++) {
>> +	for (mc = 0; mc < mcs; mc++) {
>>  		for (j = 0; j < 2; j++) {
>> -			ret = open_perf_event(imc, cpu_no, j);
>> +			ret = open_perf_event(mc, cpu_no, j);
>>  			if (ret)
>>  				return -1;
>>  		}
>>  		for (j = 0; j < 2; j++)
>> -			membw_ioctl_perf_event_ioc_reset_enable(imc, j);
>> +			membw_ioctl_perf_event_ioc_reset_enable(mc, j);
>>  	}
>>  
>>  	sleep(1);
>>  
>>  	/* Stop counters after a second to get results (both read and write) */
>> -	for (imc = 0; imc < imcs; imc++) {
>> +	for (mc = 0; mc < mcs; mc++) {
>>  		for (j = 0; j < 2; j++)
>> -			membw_ioctl_perf_event_ioc_disable(imc, j);
>> +			membw_ioctl_perf_event_ioc_disable(mc, j);
>>  	}
>>  
>>  	/*
>>  	 * 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 (mc = 0; mc < mcs; mc++) {
>>  		struct imc_counter_config *r =
>> -			&imc_counters_config[imc][READ];
>> +			&imc_counters_config[mc][READ];
>>  		struct imc_counter_config *w =
>> -			&imc_counters_config[imc][WRITE];
>> +			&imc_counters_config[mc][WRITE];
>>  
>>  		if (read(r->fd, &r->return_value,
>>  			 sizeof(struct membw_read_format)) == -1) {
>> @@ -368,9 +369,9 @@ 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++) {
>> -		close(imc_counters_config[imc][READ].fd);
>> -		close(imc_counters_config[imc][WRITE].fd);
>> +	for (mc = 0; mc < mcs; mc++) {
>> +		close(imc_counters_config[mc][READ].fd);
>> +		close(imc_counters_config[mc][WRITE].fd);
>>  	}
>>  
>>  	if (strcmp(bw_report, "reads") == 0) {
>> @@ -598,7 +599,7 @@ static int measure_vals(const struct user_params *uparams,
>>  			unsigned long *bw_resc_start)
>>  {
>>  	unsigned long bw_resc, bw_resc_end;
>> -	float bw_imc;
>> +	float bw_mc;
>>  	int ret;
>>  
>>  	/*
>> @@ -608,7 +609,7 @@ static int measure_vals(const struct user_params *uparams,
>>  	 * Compare the two values to validate resctrl value.
>>  	 * It takes 1sec to measure the data.
>>  	 */
>> -	ret = get_mem_bw_imc(uparams->cpu, param->bw_report, &bw_imc);
>> +	ret = get_mem_bw_mc(uparams->cpu, param->bw_report, &bw_mc);
>>  	if (ret < 0)
>>  		return ret;
>>  
>> @@ -617,7 +618,7 @@ static int measure_vals(const struct user_params *uparams,
>>  		return ret;
>>  
>>  	bw_resc = (bw_resc_end - *bw_resc_start) / MB;
>> -	ret = print_results_bw(param->filename, bm_pid, bw_imc, bw_resc);
>> +	ret = print_results_bw(param->filename, bm_pid, bw_mc, bw_resc);
>>  	if (ret)
>>  		return ret;
>>  
>> @@ -795,7 +796,7 @@ int resctrl_val(const struct resctrl_test *test,
>>  
>>  	if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
>>  	    !strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
>> -		ret = initialize_mem_bw_imc();
>> +		ret = initialize_mem_bw_mc();
>>  		if (ret)
>>  			goto out;
>>  
> 
> Please note that this patch conflicts with other in-progress work [1].

Yes. Noted.

> 
> Reinette
> 
> [1] https://lore.kernel.org/lkml/20240408163247.3224-1-ilpo.jarvinen@xxxxxxxxxxxxxxx/
> 

-- 
Thanks
Babu Moger