| Message ID | f45784f8882fa41d454ac3b35b203ee6d04ecc25.1717626661.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, Subject and changelog mentions how controller name is "passed" but the patch does not seem to "pass" anything new. On 6/5/24 3:45 PM, Babu Moger wrote: > The test detects number of memory controllers by looking at the sysfs > file system. Detect the vendor to pass the controller name appropriately. > So, that rest of the code will be generic and can be used to support other > vendors. > > Also change the search to look for the full string "uncore_imc_". Replace > the sizeof with strlen. > > Signed-off-by: Babu Moger <babu.moger@amd.com> > --- > v3: Change the search string to "uncore_imc_". > > v2: No changes > --- > tools/testing/selftests/resctrl/resctrl_val.c | 22 ++++++++++++------- > 1 file changed, 14 insertions(+), 8 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c > index 2d5e961b3a68..23c0e0a1d845 100644 > --- a/tools/testing/selftests/resctrl/resctrl_val.c > +++ b/tools/testing/selftests/resctrl/resctrl_val.c > @@ -10,7 +10,7 @@ > */ > #include "resctrl.h" > > -#define UNCORE_IMC "uncore_imc" > +#define UNCORE_IMC "uncore_imc_" > #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" > @@ -206,24 +206,30 @@ static int num_of_mem_controllers(void) > char mc_dir[512], *temp; > unsigned int count = 0; > struct dirent *ep; > - int ret; > + char *sysfs_name; > + int ret, vendor; > DIR *dp; > > + vendor = get_vendor(); get_vendor() is already optimized to only need to do actual detection once. I thus do not see a need for a local variable. > + if (vendor == ARCH_INTEL) { > + sysfs_name = UNCORE_IMC; > + } else { > + ksft_perror("Unsupported vendor!\n"); ksft_perror() also adds a "\n" so adding it here is not necessary. Also please drop the exclamation. > + return -1; > + } > + > dp = opendir(DYN_PMU_PATH); > if (dp) { > while ((ep = readdir(dp))) { > - temp = strstr(ep->d_name, UNCORE_IMC); > + temp = strstr(ep->d_name, sysfs_name); > if (!temp) > 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. > + * increment the pointer to point to <n>. > */ > - temp = temp + sizeof(UNCORE_IMC); > + temp = temp + strlen(sysfs_name); > > /* > * Some directories under "DYN_PMU_PATH" could have Reinette
Hi Reinette, On 6/14/24 13:38, Reinette Chatre wrote: > Hi Babu, > > Subject and changelog mentions how controller name is "passed" but the > patch does not seem to "pass" anything new. Sure. Will change the subject to this. Also will update the commit message. selftests/resctrl: Dynamically detect sysfs controller name of the vendor > > On 6/5/24 3:45 PM, Babu Moger wrote: >> The test detects number of memory controllers by looking at the sysfs >> file system. Detect the vendor to pass the controller name appropriately. >> So, that rest of the code will be generic and can be used to support other >> vendors. >> >> Also change the search to look for the full string "uncore_imc_". Replace >> the sizeof with strlen. >> >> Signed-off-by: Babu Moger <babu.moger@amd.com> >> --- >> v3: Change the search string to "uncore_imc_". >> >> v2: No changes >> --- >> tools/testing/selftests/resctrl/resctrl_val.c | 22 ++++++++++++------- >> 1 file changed, 14 insertions(+), 8 deletions(-) >> >> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c >> b/tools/testing/selftests/resctrl/resctrl_val.c >> index 2d5e961b3a68..23c0e0a1d845 100644 >> --- a/tools/testing/selftests/resctrl/resctrl_val.c >> +++ b/tools/testing/selftests/resctrl/resctrl_val.c >> @@ -10,7 +10,7 @@ >> */ >> #include "resctrl.h" >> -#define UNCORE_IMC "uncore_imc" >> +#define UNCORE_IMC "uncore_imc_" >> #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" >> @@ -206,24 +206,30 @@ static int num_of_mem_controllers(void) >> char mc_dir[512], *temp; >> unsigned int count = 0; >> struct dirent *ep; >> - int ret; >> + char *sysfs_name; >> + int ret, vendor; >> DIR *dp; >> + vendor = get_vendor(); > > get_vendor() is already optimized to only need to do actual detection once. > I thus do not see a need for a local variable. Sure. > >> + if (vendor == ARCH_INTEL) { >> + sysfs_name = UNCORE_IMC; >> + } else { >> + ksft_perror("Unsupported vendor!\n"); > > ksft_perror() also adds a "\n" so adding it here is not necessary. Also > please drop the exclamation. Sure. > >> + return -1; >> + } >> + >> dp = opendir(DYN_PMU_PATH); >> if (dp) { >> while ((ep = readdir(dp))) { >> - temp = strstr(ep->d_name, UNCORE_IMC); >> + temp = strstr(ep->d_name, sysfs_name); >> if (!temp) >> 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. >> + * increment the pointer to point to <n>. >> */ >> - temp = temp + sizeof(UNCORE_IMC); >> + temp = temp + strlen(sysfs_name); >> /* >> * Some directories under "DYN_PMU_PATH" could have > > Reinette >
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c index 2d5e961b3a68..23c0e0a1d845 100644 --- a/tools/testing/selftests/resctrl/resctrl_val.c +++ b/tools/testing/selftests/resctrl/resctrl_val.c @@ -10,7 +10,7 @@ */ #include "resctrl.h" -#define UNCORE_IMC "uncore_imc" +#define UNCORE_IMC "uncore_imc_" #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" @@ -206,24 +206,30 @@ static int num_of_mem_controllers(void) char mc_dir[512], *temp; unsigned int count = 0; struct dirent *ep; - int ret; + char *sysfs_name; + int ret, vendor; DIR *dp; + vendor = get_vendor(); + if (vendor == ARCH_INTEL) { + sysfs_name = UNCORE_IMC; + } else { + ksft_perror("Unsupported vendor!\n"); + return -1; + } + dp = opendir(DYN_PMU_PATH); if (dp) { while ((ep = readdir(dp))) { - temp = strstr(ep->d_name, UNCORE_IMC); + temp = strstr(ep->d_name, sysfs_name); if (!temp) 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. + * increment the pointer to point to <n>. */ - temp = temp + sizeof(UNCORE_IMC); + temp = temp + strlen(sysfs_name); /* * Some directories under "DYN_PMU_PATH" could have
The test detects number of memory controllers by looking at the sysfs file system. Detect the vendor to pass the controller name appropriately. So, that rest of the code will be generic and can be used to support other vendors. Also change the search to look for the full string "uncore_imc_". Replace the sizeof with strlen. Signed-off-by: Babu Moger <babu.moger@amd.com> --- v3: Change the search string to "uncore_imc_". v2: No changes --- tools/testing/selftests/resctrl/resctrl_val.c | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-)