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
