| Message ID | 0367b45170fbc7411aad12c28802284a298c1c6f.1714073751.git.babu.moger@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/resctrl: Enable MBM and MBA tests on AMD | expand |
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, On 5/9/24 16:11, Reinette Chatre wrote: > 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? Yes. Will fix it. > >> + 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? My bad. > >> + return -1; >> + } >> + >> + if (fscanf(fp, "%u", &imc_counters_config[count][READ].type) <= 0) { >> + ksft_perror("Could not get imc type"); > > Same here. Sure. > >> + 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? Sure. Will address this in next revision. > > Reinette >
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 */ + 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"); + return -1; + } + + if (fscanf(fp, "%u", &imc_counters_config[count][READ].type) <= 0) { + ksft_perror("Could not get imc type"); + fclose(fp); + return -1; + } + + fclose(fp); + + imc_counters_config[count][WRITE].type = + imc_counters_config[count][READ].type; + + /* + * Setup the event and umasks for UMC events + * Number of CAS commands sent for reads: + * EventCode = 0x0a, umask = 0x1 + * Number of CAS commands sent for writes: + * EventCode = 0x0a, umask = 0x2 + */ + imc_counters_config[count][READ].event = 0xa; + imc_counters_config[count][READ].umask = 0x1; + + imc_counters_config[count][WRITE].event = 0xa; + imc_counters_config[count][WRITE].umask = 0x2; + + return 0; +} + /* Get type and config (read and write) of an iMC counter */ static int read_from_imc_dir(char *imc_dir, int count) { @@ -233,6 +275,9 @@ static int num_of_mem_controllers(void) if (vendor == ARCH_INTEL) { sysfs_name = UNCORE_IMC; size = sizeof(UNCORE_IMC); + } else if (vendor == ARCH_AMD) { + sysfs_name = AMD_UMC; + size = sizeof(AMD_UMC); } else { perror("Unsupported Vendor!\n"); return -1; @@ -246,11 +291,12 @@ static int num_of_mem_controllers(void) continue; /* - * imc counters are named as "uncore_imc_<n>", hence - * increment the pointer to point to <n>. Note that - * sizeof(UNCORE_IMC) would count for null character as - * well and hence the last underscore character in - * uncore_imc'_' need not be counted. + * Intel imc counters are named as "uncore_imc_<n>", + * hence increment the pointer to point to <n>. + * Note that sizeof(UNCORE_IMC) would count for null + * character as well and hence the last underscore + * character in uncore_imc'_' need not be counted. + * For AMD, it will be amd_umc_<n>. */ temp = temp + size; @@ -262,7 +308,11 @@ static int num_of_mem_controllers(void) if (temp[0] >= '0' && temp[0] <= '9') { sprintf(imc_dir, "%s/%s/", DYN_PMU_PATH, ep->d_name); - ret = read_from_imc_dir(imc_dir, count); + if (vendor == ARCH_INTEL) + ret = read_from_imc_dir(imc_dir, count); + else + ret = read_from_umc_dir(imc_dir, count); + if (ret) { closedir(dp); @@ -273,8 +323,9 @@ static int num_of_mem_controllers(void) } closedir(dp); if (count == 0) { - ksft_print_msg("Unable to find iMC counters\n"); - + ksft_print_msg("Unable to find iMC/UMC counters\n"); + if (vendor == ARCH_AMD) + ksft_print_msg("Try loading amd_uncore module\n"); return -1; } } else {
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(-)