| Message ID | cover.1714073751.git.babu.moger@amd.com |
|---|---|
| Headers | show |
| Series | selftests/resctrl: Enable MBM and MBA tests on AMD | expand |
Hi Babu, On 4/25/2024 1:16 PM, Babu Moger wrote: > > The MBM (Memory Bandwidth Monitoring) and MBA (Memory Bandwidth Allocation) > features are not enabled for AMD systems. The reason was lack of perf > counters to compare the resctrl test results. > > Starting with the commit > 25e56847821f ("perf/x86/amd/uncore: Add memory controller support"), AMD > now supports the UMC (Unified Memory Controller) perf events. These events > can be used to compare the test results. > > This series adds the support to detect the UMC events and enable MBM/MBA > tests for AMD systems. > > v2: Changes. > a. Rebased on top of tip/master (Apr 25, 2024) Please note that resctrl selftest changes flow upstream via the kselftest repo. The latest resctrl selftest changes can be found on the "next" branch there. Reinette
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.
Hi Babu, On 4/25/2024 1:17 PM, Babu Moger wrote: > Add support to read UMC (Unified Memory Controller) perf events to compare > the numbers with QoS monitor for AMD. > > Signed-off-by: Babu Moger <babu.moger@amd.com> > --- > tools/testing/selftests/resctrl/resctrl_val.c | 67 ++++++++++++++++--- > 1 file changed, 59 insertions(+), 8 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c > index e3b09128ec3d..d90d3196d7b5 100644 > --- a/tools/testing/selftests/resctrl/resctrl_val.c > +++ b/tools/testing/selftests/resctrl/resctrl_val.c > @@ -11,6 +11,7 @@ > #include "resctrl.h" > > #define UNCORE_IMC "uncore_imc" > +#define AMD_UMC "amd_umc" > #define READ_FILE_NAME "events/cas_count_read" > #define WRITE_FILE_NAME "events/cas_count_write" > #define DYN_PMU_PATH "/sys/bus/event_source/devices" > @@ -146,6 +147,47 @@ static int open_perf_event(int i, int cpu_no, int j) > return 0; > } > > +/* Get type and config (read and write) of an UMC counter */ > +static int read_from_umc_dir(char *umc_dir, int count) > +{ > + char umc_counter_type[PATH_MAX]; > + FILE *fp; > + > + /* Get type of iMC counter */ iMC counter? > + sprintf(umc_counter_type, "%s%s", umc_dir, "type"); > + fp = fopen(umc_counter_type, "r"); > + if (!fp) { > + ksft_perror("Failed to open imc counter type file"); Why go through effort of changing to generic names and then follow by using Intel naming in AMD specific code? > + return -1; > + } > + > + if (fscanf(fp, "%u", &imc_counters_config[count][READ].type) <= 0) { > + ksft_perror("Could not get imc type"); Same here. > + fclose(fp); > + return -1; > + } > + > + fclose(fp); > + > + imc_counters_config[count][WRITE].type = > + imc_counters_config[count][READ].type; > + Up to here seems to be a copy&paste of read_from_imc_dir(). Could you instead split read_from_imc_dir() so that AMD and Intel can share the code to determine type? Reinette
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@amd.com> >> --- >> 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@linux.intel.com/ >
The MBM (Memory Bandwidth Monitoring) and MBA (Memory Bandwidth Allocation) features are not enabled for AMD systems. The reason was lack of perf counters to compare the resctrl test results. Starting with the commit 25e56847821f ("perf/x86/amd/uncore: Add memory controller support"), AMD now supports the UMC (Unified Memory Controller) perf events. These events can be used to compare the test results. This series adds the support to detect the UMC events and enable MBM/MBA tests for AMD systems. v2: Changes. a. Rebased on top of tip/master (Apr 25, 2024) b. Addressed Ilpo comments except the one about close call. It seems more clear to keep READ and WRITE separate. https://lore.kernel.org/lkml/8e4badb7-6cc5-61f1-e041-d902209a90d5@linux.intel.com/ c. Used ksft_perror call when applicable. d. Added vendor check for non contiguous CBM check. v1: https://lore.kernel.org/lkml/cover.1708637563.git.babu.moger@amd.com/ Babu Moger (4): selftests/resctrl: Rename variable imcs and num_of_imcs() to generic names selftests/resctrl: Pass sysfs controller name of the vendor selftests/resctrl: Add support for MBM and MBA tests on AMD selftests/resctrl: Enable MBA/MBA tests on AMD tools/testing/selftests/resctrl/cat_test.c | 2 +- tools/testing/selftests/resctrl/mba_test.c | 1 - tools/testing/selftests/resctrl/mbm_test.c | 1 - tools/testing/selftests/resctrl/resctrl_val.c | 142 +++++++++++++----- 4 files changed, 103 insertions(+), 43 deletions(-)