Re: [PATCH v3 1/4] selftests/resctrl: Rename variables and functions to generic names

[Moger, Babu]

Hi Reinette,


On 6/14/24 13:37, Reinette Chatre wrote:
> Hi Babu,
> 
> On 6/5/24 3:45 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
> 
> Changelog usually starts with some context and then problem to be solved.
> What
> the patch does follows that. Maybe just something like:
> 
>     For Intel, the memory controller is called Integrated Memory
>     Controller (IMC). For AMD, it is called Unified Memory Controller (UMC).
>                                                                                     Change the names of variables and functions from imc to mc in preparation
>     for support of MBM and MBA tests for AMD.
>                                                                                     No functional change.

Sure.

> 
>> Integrated Memory Controllers (IMC). For AMD, it is called Unified
>> Memory Controller (UMC).
>>
>> Change the names of variables and functions from imc (Integrated memory
>> controller) to mc(Memory Controller). No functional change.
>>
>> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
>> ---
> 
> ...
> 
>> @@ -349,10 +350,10 @@ static void do_imc_mem_bw_test(void)
>>    *
>>    * Return: = 0 on success. < 0 on failure.
>>    */
>> -static int get_mem_bw_imc(const char *bw_report, float *bw_imc)
>> +static int get_mem_bw_mc(const char *bw_report, float *bw_mc)
> 
> The name of the function is expected to be changed in the function comments
> also.

Ok.

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

Yes.

> 
>>       reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
>> @@ -361,21 +362,21 @@ static int get_mem_bw_imc(const char *bw_report,
>> float *bw_imc)
>>        * Get results which are stored in struct type imc_counter_config
>>        * Take overflow into consideration before calculating total
>> bandwidth.
>>        */
> 
> In below snippet imc_counter_config is renamed to mc_counter_config yet the
> comment above was not changed to match.

Will correct it.

> 
>> -    for (imc = 0; imc < imcs; imc++) {
>> -        struct imc_counter_config *r =
>> -            &imc_counters_config[imc][READ];
>> -        struct imc_counter_config *w =
>> -            &imc_counters_config[imc][WRITE];
>> +    for (mc = 0; mc < mcs; mc++) {
>> +        struct mc_counter_config *r =
>> +            &mc_counters_config[mc][READ];
>> +        struct mc_counter_config *w =
>> +            &mc_counters_config[mc][WRITE];
>>   
> 
> I noticed a couple of misses by just looking at this patch. You can
> use grep to ensure that this patch does what you intend.

Sure.--
Thanks
Babu Moger