| Message ID | cc22790173de9978a6485baca5bc96a8ad8e05a4.1708637563.git.babu.moger@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/resctrl: Enable MBM and MBA tests on AMD | expand |
On Thu, 22 Feb 2024, 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> > --- > .../testing/selftests/resctrl/resctrl_tests.c | 6 +- > tools/testing/selftests/resctrl/resctrl_val.c | 62 +++++++++++++++++-- > 2 files changed, 58 insertions(+), 10 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c > index 2bbe3045a018..231233b8d354 100644 > --- a/tools/testing/selftests/resctrl/resctrl_tests.c > +++ b/tools/testing/selftests/resctrl/resctrl_tests.c > @@ -104,8 +104,7 @@ static void run_mbm_test(const char * const *benchmark_cmd, int cpu_no) > } > > if (!validate_resctrl_feature_request("L3_MON", "mbm_total_bytes") || > - !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes") || > - (get_vendor() != ARCH_INTEL)) { > + !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes")) { > ksft_test_result_skip("Hardware does not support MBM or MBM is disabled\n"); > goto cleanup; > } > @@ -131,8 +130,7 @@ static void run_mba_test(const char * const *benchmark_cmd, int cpu_no) > } > > if (!validate_resctrl_feature_request("MB", NULL) || > - !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes") || > - (get_vendor() != ARCH_INTEL)) { > + !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes")) { > ksft_test_result_skip("Hardware does not support MBA or MBA is disabled\n"); > goto cleanup; > } These need to be rebased as this code moved into per test files with the generic test framework. The .vendor_specific field is the new way to make tests limited to particular vendors. > diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c > index 792116d3565f..c5a4607aa9d9 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[1024]; 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) { > + perror("Failed to open imc counter type file"); Please don't add new perror() anymore, I just managed to clean up those in favor of ksft_perror(). > + Drop this newline (to slowly move towards more concise error handling blocks w/o all those extra newlines). > + return -1; > + } > + if (fscanf(fp, "%u", &imc_counters_config[count][READ].type) <= 0) { > + perror("Could not get imc type"); Ditto. > + 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) > {
Hi Ilpo, On 2/23/24 04:53, Ilpo Järvinen wrote: > On Thu, 22 Feb 2024, 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> >> --- >> .../testing/selftests/resctrl/resctrl_tests.c | 6 +- >> tools/testing/selftests/resctrl/resctrl_val.c | 62 +++++++++++++++++-- >> 2 files changed, 58 insertions(+), 10 deletions(-) >> >> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c >> index 2bbe3045a018..231233b8d354 100644 >> --- a/tools/testing/selftests/resctrl/resctrl_tests.c >> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c >> @@ -104,8 +104,7 @@ static void run_mbm_test(const char * const *benchmark_cmd, int cpu_no) >> } >> >> if (!validate_resctrl_feature_request("L3_MON", "mbm_total_bytes") || >> - !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes") || >> - (get_vendor() != ARCH_INTEL)) { >> + !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes")) { >> ksft_test_result_skip("Hardware does not support MBM or MBM is disabled\n"); >> goto cleanup; >> } >> @@ -131,8 +130,7 @@ static void run_mba_test(const char * const *benchmark_cmd, int cpu_no) >> } >> >> if (!validate_resctrl_feature_request("MB", NULL) || >> - !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes") || >> - (get_vendor() != ARCH_INTEL)) { >> + !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes")) { >> ksft_test_result_skip("Hardware does not support MBA or MBA is disabled\n"); >> goto cleanup; >> } > > These need to be rebased as this code moved into per test files with the > generic test framework. The .vendor_specific field is the new way to make > tests limited to particular vendors. Hmm. I picked the latest code from tip/master. Where is the latest code now? Is it yet to be submitted? I can wait for your code to merge first. > >> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c >> index 792116d3565f..c5a4607aa9d9 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[1024]; > > PATH_MAX ok. > >> + FILE *fp; >> + >> + /* Get type of iMC counter */ >> + sprintf(umc_counter_type, "%s%s", umc_dir, "type"); >> + fp = fopen(umc_counter_type, "r"); >> + if (!fp) { >> + perror("Failed to open imc counter type file"); > > Please don't add new perror() anymore, I just managed to clean up those > in favor of ksft_perror(). Ok. Will look into it. > >> + > > Drop this newline (to slowly move towards more concise error handling > blocks w/o all those extra newlines). ok. > >> + return -1; > > >> + } >> + if (fscanf(fp, "%u", &imc_counters_config[count][READ].type) <= 0) { >> + perror("Could not get imc type"); > > Ditto. ok > >> + 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) >> { >
On 2/23/24 13:30, Moger, Babu wrote: > Hi Ilpo, > > On 2/23/24 04:53, Ilpo Järvinen wrote: >> On Thu, 22 Feb 2024, 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> >>> --- >>> .../testing/selftests/resctrl/resctrl_tests.c | 6 +- >>> tools/testing/selftests/resctrl/resctrl_val.c | 62 +++++++++++++++++-- >>> 2 files changed, 58 insertions(+), 10 deletions(-) >>> >>> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c >>> index 2bbe3045a018..231233b8d354 100644 >>> --- a/tools/testing/selftests/resctrl/resctrl_tests.c >>> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c >>> @@ -104,8 +104,7 @@ static void run_mbm_test(const char * const *benchmark_cmd, int cpu_no) >>> } >>> >>> if (!validate_resctrl_feature_request("L3_MON", "mbm_total_bytes") || >>> - !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes") || >>> - (get_vendor() != ARCH_INTEL)) { >>> + !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes")) { >>> ksft_test_result_skip("Hardware does not support MBM or MBM is disabled\n"); >>> goto cleanup; >>> } >>> @@ -131,8 +130,7 @@ static void run_mba_test(const char * const *benchmark_cmd, int cpu_no) >>> } >>> >>> if (!validate_resctrl_feature_request("MB", NULL) || >>> - !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes") || >>> - (get_vendor() != ARCH_INTEL)) { >>> + !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes")) { >>> ksft_test_result_skip("Hardware does not support MBA or MBA is disabled\n"); >>> goto cleanup; >>> } >> >> These need to be rebased as this code moved into per test files with the >> generic test framework. The .vendor_specific field is the new way to make >> tests limited to particular vendors. > > Hmm. I picked the latest code from tip/master. Where is the latest code > now? Is it yet to be submitted? > > I can wait for your code to merge first. Oh. ok. Here it is https://lore.kernel.org/lkml/cover.1708072203.git.maciej.wieczor-retman@intel.com/ I will rebase it on top of this next revision. Thanks Babu > >> >>> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c >>> index 792116d3565f..c5a4607aa9d9 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[1024]; >> >> PATH_MAX > > ok. >> >>> + FILE *fp; >>> + >>> + /* Get type of iMC counter */ >>> + sprintf(umc_counter_type, "%s%s", umc_dir, "type"); >>> + fp = fopen(umc_counter_type, "r"); >>> + if (!fp) { >>> + perror("Failed to open imc counter type file"); >> >> Please don't add new perror() anymore, I just managed to clean up those >> in favor of ksft_perror(). > > Ok. Will look into it. >> >>> + >> >> Drop this newline (to slowly move towards more concise error handling >> blocks w/o all those extra newlines). > > ok. > >> >>> + return -1; >> >> >>> + } >>> + if (fscanf(fp, "%u", &imc_counters_config[count][READ].type) <= 0) { >>> + perror("Could not get imc type"); >> >> Ditto. > > ok > >> >>> + 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) >>> { >> >
Hi Babu, On 2/23/2024 11:47 AM, Moger, Babu wrote: > On 2/23/24 13:30, Moger, Babu wrote: >> On 2/23/24 04:53, Ilpo Järvinen wrote: >>> On Thu, 22 Feb 2024, Babu Moger wrote: >>>> @@ -131,8 +130,7 @@ static void run_mba_test(const char * const *benchmark_cmd, int cpu_no) >>>> } >>>> >>>> if (!validate_resctrl_feature_request("MB", NULL) || >>>> - !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes") || >>>> - (get_vendor() != ARCH_INTEL)) { >>>> + !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes")) { >>>> ksft_test_result_skip("Hardware does not support MBA or MBA is disabled\n"); >>>> goto cleanup; >>>> } >>> >>> These need to be rebased as this code moved into per test files with the >>> generic test framework. The .vendor_specific field is the new way to make >>> tests limited to particular vendors. >> >> Hmm. I picked the latest code from tip/master. Where is the latest code >> now? Is it yet to be submitted? >> >> I can wait for your code to merge first. > > Oh. ok. Here it is > https://lore.kernel.org/lkml/cover.1708072203.git.maciej.wieczor-retman@intel.com/ > > I will rebase it on top of this next revision. The resctrl selftest changes are routed through the kselftest repo. Shuah just picked up this series you noticed but it is based on the framework changes from Ilpo that is already merged. I recommend that you base your next series on the "next" branch of the kselftest repo [1]. Reinette [1] git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git
Hi Reinette On 2/23/24 16:39, Reinette Chatre wrote: > Hi Babu, > > On 2/23/2024 11:47 AM, Moger, Babu wrote: >> On 2/23/24 13:30, Moger, Babu wrote: >>> On 2/23/24 04:53, Ilpo Järvinen wrote: >>>> On Thu, 22 Feb 2024, Babu Moger wrote: >>>>> @@ -131,8 +130,7 @@ static void run_mba_test(const char * const *benchmark_cmd, int cpu_no) >>>>> } >>>>> >>>>> if (!validate_resctrl_feature_request("MB", NULL) || >>>>> - !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes") || >>>>> - (get_vendor() != ARCH_INTEL)) { >>>>> + !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes")) { >>>>> ksft_test_result_skip("Hardware does not support MBA or MBA is disabled\n"); >>>>> goto cleanup; >>>>> } >>>> >>>> These need to be rebased as this code moved into per test files with the >>>> generic test framework. The .vendor_specific field is the new way to make >>>> tests limited to particular vendors. >>> >>> Hmm. I picked the latest code from tip/master. Where is the latest code >>> now? Is it yet to be submitted? >>> >>> I can wait for your code to merge first. >> >> Oh. ok. Here it is >> https://lore.kernel.org/lkml/cover.1708072203.git.maciej.wieczor-retman@intel.com/ >> >> I will rebase it on top of this next revision. > > The resctrl selftest changes are routed through the kselftest repo. Shuah just picked > up this series you noticed but it is based on the framework changes from Ilpo that > is already merged. I recommend that you base your next series on the "next" branch > of the kselftest repo [1]. Sure. Will do. > > Reinette > > [1] git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index 2bbe3045a018..231233b8d354 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -104,8 +104,7 @@ static void run_mbm_test(const char * const *benchmark_cmd, int cpu_no) } if (!validate_resctrl_feature_request("L3_MON", "mbm_total_bytes") || - !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes") || - (get_vendor() != ARCH_INTEL)) { + !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes")) { ksft_test_result_skip("Hardware does not support MBM or MBM is disabled\n"); goto cleanup; } @@ -131,8 +130,7 @@ static void run_mba_test(const char * const *benchmark_cmd, int cpu_no) } if (!validate_resctrl_feature_request("MB", NULL) || - !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes") || - (get_vendor() != ARCH_INTEL)) { + !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes")) { ksft_test_result_skip("Hardware does not support MBA or MBA is disabled\n"); goto cleanup; } diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c index 792116d3565f..c5a4607aa9d9 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[1024]; + FILE *fp; + + /* Get type of iMC counter */ + sprintf(umc_counter_type, "%s%s", umc_dir, "type"); + fp = fopen(umc_counter_type, "r"); + if (!fp) { + perror("Failed to open imc counter type file"); + + return -1; + } + if (fscanf(fp, "%u", &imc_counters_config[count][READ].type) <= 0) { + 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 get_number_of_mem_ctrls(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 get_number_of_mem_ctrls(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 get_number_of_mem_ctrls(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);
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> --- .../testing/selftests/resctrl/resctrl_tests.c | 6 +- tools/testing/selftests/resctrl/resctrl_val.c | 62 +++++++++++++++++-- 2 files changed, 58 insertions(+), 10 deletions(-)