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
