| Message ID | 20230911111930.16088-4-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/resctrl: Fixes to failing tests | expand |
Hi Ilpo, On 9/11/2023 4:19 AM, Ilpo Järvinen wrote: > Feature check in validate_resctrl_feature_request() takes in the test > name string and maps that to what to check per test. > > Pass resource and feature names to validate_resctrl_feature_request() > directly rather than deriving them from the test name inside the > function which makes the feature check easier to extend for new test > cases. > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > Cc: <stable@vger.kernel.org> This does not seem to be stable material. > --- > tools/testing/selftests/resctrl/resctrl.h | 6 +- > .../testing/selftests/resctrl/resctrl_tests.c | 10 +-- > tools/testing/selftests/resctrl/resctrlfs.c | 69 ++++++++----------- > 3 files changed, 34 insertions(+), 51 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h > index dd07463cdf48..89ced4152933 100644 > --- a/tools/testing/selftests/resctrl/resctrl.h > +++ b/tools/testing/selftests/resctrl/resctrl.h > @@ -28,10 +28,6 @@ > #define RESCTRL_PATH "/sys/fs/resctrl" > #define PHYS_ID_PATH "/sys/devices/system/cpu/cpu" > #define INFO_PATH "/sys/fs/resctrl/info" > -#define L3_PATH "/sys/fs/resctrl/info/L3" > -#define MB_PATH "/sys/fs/resctrl/info/MB" > -#define L3_MON_PATH "/sys/fs/resctrl/info/L3_MON" > -#define L3_MON_FEATURES_PATH "/sys/fs/resctrl/info/L3_MON/mon_features" > > #define ARCH_INTEL 1 > #define ARCH_AMD 2 > @@ -88,7 +84,7 @@ int get_resource_id(int cpu_no, int *resource_id); > int mount_resctrlfs(void); > int umount_resctrlfs(void); > int validate_bw_report_request(char *bw_report); > -bool validate_resctrl_feature_request(const char *resctrl_val); > +bool validate_resctrl_feature_request(const char *resource, const char *feature); > char *fgrep(FILE *inf, const char *str); > int taskset_benchmark(pid_t bm_pid, int cpu_no); > void run_benchmark(int signum, siginfo_t *info, void *ucontext); > diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c > index 3d66fbdc2df3..3052394ca884 100644 > --- a/tools/testing/selftests/resctrl/resctrl_tests.c > +++ b/tools/testing/selftests/resctrl/resctrl_tests.c > @@ -84,7 +84,9 @@ static void run_mbm_test(const char * const *benchmark_cmd, int cpu_no) > return; > } > > - if (!validate_resctrl_feature_request(MBM_STR) || (get_vendor() != ARCH_INTEL)) { > + if (!validate_resctrl_feature_request("L3_MON", "mbm_total_bytes") || > + !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes") || > + (get_vendor() != ARCH_INTEL)) { > ksft_test_result_skip("Hardware does not support MBM or MBM is disabled\n"); > goto umount; > } > @@ -116,7 +118,7 @@ static void run_mba_test(const char * const *benchmark_cmd, int cpu_no) > return; > } > > - if (!validate_resctrl_feature_request(MBA_STR) || (get_vendor() != ARCH_INTEL)) { > + if (!validate_resctrl_feature_request("MB", NULL) || (get_vendor() != ARCH_INTEL)) { > ksft_test_result_skip("Hardware does not support MBA or MBA is disabled\n"); > goto umount; > } > @@ -146,7 +148,7 @@ static void run_cmt_test(const char * const *benchmark_cmd, int cpu_no) > return; > } > > - if (!validate_resctrl_feature_request(CMT_STR)) { > + if (!validate_resctrl_feature_request("L3_MON", "llc_occupancy")) { > ksft_test_result_skip("Hardware does not support CMT or CMT is disabled\n"); > goto umount; > } > @@ -178,7 +180,7 @@ static void run_cat_test(int cpu_no, int no_of_bits) > return; > } > > - if (!validate_resctrl_feature_request(CAT_STR)) { > + if (!validate_resctrl_feature_request("L3", NULL)) { > ksft_test_result_skip("Hardware does not support CAT or CAT is disabled\n"); > goto umount; > } > diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c > index bd36ee206602..bd547a10791c 100644 > --- a/tools/testing/selftests/resctrl/resctrlfs.c > +++ b/tools/testing/selftests/resctrl/resctrlfs.c > @@ -10,6 +10,8 @@ > */ > #include "resctrl.h" > > +#include <limits.h> > + Could you please include <limits.h> before the local resctrl.h? > static int find_resctrl_mount(char *buffer) > { > FILE *mounts; > @@ -604,63 +606,46 @@ char *fgrep(FILE *inf, const char *str) > > /* > * validate_resctrl_feature_request - Check if requested feature is valid. > - * @resctrl_val: Requested feature > + * @resource: Required resource (e.g., MB, L3, L2, L3_MON, etc.) > + * @feature: Feature to be checked under resource (can be NULL). This path > + * is relative to the resource path. I do not think "this path" is accurate. @feature is not a path but an entry within the mon_features file. Also please note that mon_features only exists for L3_MON, none of the other listed resources have an associated mon_features file in resctrl. This function is created to be generic has specific requirements on what valid (never checked) parameters should be. This may be ok with the usage but it should not pretend to be generic. > * > - * Return: True if the feature is supported, else false. False is also > - * returned if resctrl FS is not mounted. > + * Return: True if the resource/feature is supported, else false. False is > + * also returned if resctrl FS is not mounted. > */ > -bool validate_resctrl_feature_request(const char *resctrl_val) > +bool validate_resctrl_feature_request(const char *resource, const char *feature) > { > struct stat statbuf; > - bool found = false; > + char res_path[PATH_MAX]; Please maintain reverse fir order. > char *res; > FILE *inf; > int ret; > > - if (!resctrl_val) > + if (!resource) > return false; > > ret = find_resctrl_mount(NULL); > if (ret) > return false; > > - if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR))) { > - if (!stat(L3_PATH, &statbuf)) > - return true; > - } else if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) { > - if (!stat(MB_PATH, &statbuf)) > - return true; > - } else if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) || > - !strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) { > - if (!stat(L3_MON_PATH, &statbuf)) { > - inf = fopen(L3_MON_FEATURES_PATH, "r"); > - if (!inf) > - return false; > - > - if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) { > - res = fgrep(inf, "llc_occupancy"); > - if (res) { > - found = true; > - free(res); > - } > - } > - > - if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR))) { > - res = fgrep(inf, "mbm_total_bytes"); > - if (res) { > - free(res); > - res = fgrep(inf, "mbm_local_bytes"); > - if (res) { > - found = true; > - free(res); > - } > - } > - } > - fclose(inf); > - } > - } > + snprintf(res_path, sizeof(res_path), "%s/%s", INFO_PATH, resource); > + > + if (stat(res_path, &statbuf)) > + return false; > + > + if (!feature) > + return true; > + > + snprintf(res_path, sizeof(res_path), "%s/%s/mon_features", INFO_PATH, resource); > + inf = fopen(res_path, "r"); > + if (!inf) > + return false; > + > + res = fgrep(inf, feature); > + free(res); > + fclose(inf); > > - return found; > + return res; This is unexpected. Function should return bool but instead returns a char * that has been freed? > } > > int filter_dmesg(void) Reinette
On Tue, 12 Sep 2023, Reinette Chatre wrote: > On 9/11/2023 4:19 AM, Ilpo Järvinen wrote: > > Feature check in validate_resctrl_feature_request() takes in the test > > name string and maps that to what to check per test. > > > > Pass resource and feature names to validate_resctrl_feature_request() > > directly rather than deriving them from the test name inside the > > function which makes the feature check easier to extend for new test > > cases. > > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > Cc: <stable@vger.kernel.org> > > This does not seem to be stable material. Alone it isn't, but both 2/5 and this 3/5 are prerequisites for 4/5 as shown by the tags there. > > --- > > tools/testing/selftests/resctrl/resctrl.h | 6 +- > > .../testing/selftests/resctrl/resctrl_tests.c | 10 +-- > > tools/testing/selftests/resctrl/resctrlfs.c | 69 ++++++++----------- > > 3 files changed, 34 insertions(+), 51 deletions(-) > > > > diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h > > index dd07463cdf48..89ced4152933 100644 > > --- a/tools/testing/selftests/resctrl/resctrl.h > > +++ b/tools/testing/selftests/resctrl/resctrl.h > > diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c > > index bd36ee206602..bd547a10791c 100644 > > --- a/tools/testing/selftests/resctrl/resctrlfs.c > > +++ b/tools/testing/selftests/resctrl/resctrlfs.c > > @@ -10,6 +10,8 @@ > > */ > > #include "resctrl.h" > > > > +#include <limits.h> > > + > > Could you please include <limits.h> before the local resctrl.h? Believe me I tried that first but it did not work. So this intentionally in the current order as resctrl.h defines _GNU_SOURCE which is among things that tends to alter many things. If I reorder them, the build gives me these issues: resctrlfs.c: In function ‘taskset_benchmark’: resctrlfs.c:284:2: warning: implicit declaration of function ‘CPU_ZERO’; did you mean ‘FP_ZERO’? [-Wimplicit-function-declaration] 284 | CPU_ZERO(&my_set); | ^~~~~~~~ | FP_ZERO resctrlfs.c:285:2: warning: implicit declaration of function ‘CPU_SET’ [-Wimplicit-function-declaration] 285 | CPU_SET(cpu_no, &my_set); | ^~~~~~~ resctrlfs.c:287:6: warning: implicit declaration of function ‘sched_setaffinity’ [-Wimplicit-function-declaration] 287 | if (sched_setaffinity(bm_pid, sizeof(cpu_set_t), &my_set)) { | ^~~~~~~~~~~~~~~~~ It might be useful to move _GNU_SOURCE define into Makefile though to avoid these kind of issues (but that's not material for this patch). > > static int find_resctrl_mount(char *buffer) > > { > > FILE *mounts; > > @@ -604,63 +606,46 @@ char *fgrep(FILE *inf, const char *str) > > > > /* > > * validate_resctrl_feature_request - Check if requested feature is valid. > > - * @resctrl_val: Requested feature > > + * @resource: Required resource (e.g., MB, L3, L2, L3_MON, etc.) > > + * @feature: Feature to be checked under resource (can be NULL). This path > > + * is relative to the resource path. > > I do not think "this path" is accurate. @feature is not a path but an entry > within the mon_features file. Yes, agreed. > Also please note that mon_features only exists for L3_MON, none of the other > listed resources have an associated mon_features file in resctrl. This > function is created to be generic has specific requirements on what > valid (never checked) parameters should be. This may be ok with the usage > but it should not pretend to be generic. So are you recommending I split this function into two where the new one would do the mon_features check? > > char *res; > > FILE *inf; > > int ret; > > > > - if (!resctrl_val) > > + if (!resource) > > return false; > > > > ret = find_resctrl_mount(NULL); > > if (ret) > > return false; > > > > - if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR))) { > > - if (!stat(L3_PATH, &statbuf)) > > - return true; > > - } else if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) { > > - if (!stat(MB_PATH, &statbuf)) > > - return true; > > - } else if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) || > > - !strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) { > > - if (!stat(L3_MON_PATH, &statbuf)) { > > - inf = fopen(L3_MON_FEATURES_PATH, "r"); > > - if (!inf) > > - return false; > > - > > - if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) { > > - res = fgrep(inf, "llc_occupancy"); > > - if (res) { > > - found = true; > > - free(res); > > - } > > - } > > - > > - if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR))) { > > - res = fgrep(inf, "mbm_total_bytes"); > > - if (res) { > > - free(res); > > - res = fgrep(inf, "mbm_local_bytes"); > > - if (res) { > > - found = true; > > - free(res); > > - } > > - } > > - } > > - fclose(inf); > > - } > > - } > > + snprintf(res_path, sizeof(res_path), "%s/%s", INFO_PATH, resource); > > + > > + if (stat(res_path, &statbuf)) > > + return false; > > + > > + if (!feature) > > + return true; > > + > > + snprintf(res_path, sizeof(res_path), "%s/%s/mon_features", INFO_PATH, resource); > > + inf = fopen(res_path, "r"); > > + if (!inf) > > + return false; > > + > > + res = fgrep(inf, feature); > > + free(res); > > + fclose(inf); > > > > - return found; > > + return res; > > This is unexpected. Function should return bool but instead returns a char * that > has been freed? Okay, I understand it looks confusing when relying on implicit conversion to boolean (despite being functionally correct). I can do this instead to make it more obvious the intention is to convert the result to boolean and not return a pointer: return !!res;
Hi Ilpo, On 9/13/2023 4:02 AM, Ilpo Järvinen wrote: > On Tue, 12 Sep 2023, Reinette Chatre wrote: >> On 9/11/2023 4:19 AM, Ilpo Järvinen wrote: >>> Feature check in validate_resctrl_feature_request() takes in the test >>> name string and maps that to what to check per test. >>> >>> Pass resource and feature names to validate_resctrl_feature_request() >>> directly rather than deriving them from the test name inside the >>> function which makes the feature check easier to extend for new test >>> cases. >>> >>> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> >>> Cc: <stable@vger.kernel.org> >> >> This does not seem to be stable material. > > Alone it isn't, but both 2/5 and this 3/5 are prerequisites for 4/5 as > shown by the tags there. > >>> --- >>> tools/testing/selftests/resctrl/resctrl.h | 6 +- >>> .../testing/selftests/resctrl/resctrl_tests.c | 10 +-- >>> tools/testing/selftests/resctrl/resctrlfs.c | 69 ++++++++----------- >>> 3 files changed, 34 insertions(+), 51 deletions(-) >>> >>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h >>> index dd07463cdf48..89ced4152933 100644 >>> --- a/tools/testing/selftests/resctrl/resctrl.h >>> +++ b/tools/testing/selftests/resctrl/resctrl.h > >>> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c >>> index bd36ee206602..bd547a10791c 100644 >>> --- a/tools/testing/selftests/resctrl/resctrlfs.c >>> +++ b/tools/testing/selftests/resctrl/resctrlfs.c >>> @@ -10,6 +10,8 @@ >>> */ >>> #include "resctrl.h" >>> >>> +#include <limits.h> >>> + >> >> Could you please include <limits.h> before the local resctrl.h? > > Believe me I tried that first but it did not work. So this intentionally > in the current order as resctrl.h defines _GNU_SOURCE which is among > things that tends to alter many things. If I reorder them, the build gives > me these issues: > > resctrlfs.c: In function ‘taskset_benchmark’: > resctrlfs.c:284:2: warning: implicit declaration of function ‘CPU_ZERO’; > did you mean ‘FP_ZERO’? [-Wimplicit-function-declaration] > 284 | CPU_ZERO(&my_set); > | ^~~~~~~~ > | FP_ZERO > resctrlfs.c:285:2: warning: implicit declaration of function ‘CPU_SET’ > [-Wimplicit-function-declaration] > 285 | CPU_SET(cpu_no, &my_set); > | ^~~~~~~ > resctrlfs.c:287:6: warning: implicit declaration of function > ‘sched_setaffinity’ [-Wimplicit-function-declaration] > 287 | if (sched_setaffinity(bm_pid, sizeof(cpu_set_t), &my_set)) { > | ^~~~~~~~~~~~~~~~~ > > It might be useful to move _GNU_SOURCE define into Makefile though to > avoid these kind of issues (but that's not material for this patch). How about a #define _GNU_SOURCE in this file as an intermediate step? I did see your patch making this change but cannot see how it is coordinated with fixing the include order in this file. > >>> static int find_resctrl_mount(char *buffer) >>> { >>> FILE *mounts; >>> @@ -604,63 +606,46 @@ char *fgrep(FILE *inf, const char *str) >>> >>> /* >>> * validate_resctrl_feature_request - Check if requested feature is valid. >>> - * @resctrl_val: Requested feature >>> + * @resource: Required resource (e.g., MB, L3, L2, L3_MON, etc.) >>> + * @feature: Feature to be checked under resource (can be NULL). This path >>> + * is relative to the resource path. >> >> I do not think "this path" is accurate. @feature is not a path but an entry >> within the mon_features file. > > Yes, agreed. > >> Also please note that mon_features only exists for L3_MON, none of the other >> listed resources have an associated mon_features file in resctrl. This >> function is created to be generic has specific requirements on what >> valid (never checked) parameters should be. This may be ok with the usage >> but it should not pretend to be generic. > > So are you recommending I split this function into two where the new one > would do the mon_features check? No need to split the function. That seems overkill considering its captive usage. I think a snippet making its usage clear will be helpful. Something like: @feature: <description>. Can only be set for L3_MON. Must be NULL for all other resources. Please feel free to improve. > >>> char *res; >>> FILE *inf; >>> int ret; >>> >>> - if (!resctrl_val) >>> + if (!resource) >>> return false; >>> >>> ret = find_resctrl_mount(NULL); >>> if (ret) >>> return false; >>> >>> - if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR))) { >>> - if (!stat(L3_PATH, &statbuf)) >>> - return true; >>> - } else if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) { >>> - if (!stat(MB_PATH, &statbuf)) >>> - return true; >>> - } else if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) || >>> - !strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) { >>> - if (!stat(L3_MON_PATH, &statbuf)) { >>> - inf = fopen(L3_MON_FEATURES_PATH, "r"); >>> - if (!inf) >>> - return false; >>> - >>> - if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) { >>> - res = fgrep(inf, "llc_occupancy"); >>> - if (res) { >>> - found = true; >>> - free(res); >>> - } >>> - } >>> - >>> - if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR))) { >>> - res = fgrep(inf, "mbm_total_bytes"); >>> - if (res) { >>> - free(res); >>> - res = fgrep(inf, "mbm_local_bytes"); >>> - if (res) { >>> - found = true; >>> - free(res); >>> - } >>> - } >>> - } >>> - fclose(inf); >>> - } >>> - } >>> + snprintf(res_path, sizeof(res_path), "%s/%s", INFO_PATH, resource); >>> + >>> + if (stat(res_path, &statbuf)) >>> + return false; >>> + >>> + if (!feature) >>> + return true; >>> + >>> + snprintf(res_path, sizeof(res_path), "%s/%s/mon_features", INFO_PATH, resource); >>> + inf = fopen(res_path, "r"); >>> + if (!inf) >>> + return false; >>> + >>> + res = fgrep(inf, feature); >>> + free(res); >>> + fclose(inf); >>> >>> - return found; >>> + return res; >> >> This is unexpected. Function should return bool but instead returns a char * that >> has been freed? > > Okay, I understand it looks confusing when relying on implicit conversion > to boolean (despite being functionally correct). > > I can do this instead to make it more obvious the intention is to convert > the result to boolean and not return a pointer: > return !!res; > Looks good to me. Reinette
On Wed, 13 Sep 2023, Reinette Chatre wrote: > On 9/13/2023 4:02 AM, Ilpo Järvinen wrote: > > On Tue, 12 Sep 2023, Reinette Chatre wrote: > >> On 9/11/2023 4:19 AM, Ilpo Järvinen wrote: > >>> Feature check in validate_resctrl_feature_request() takes in the test > >>> name string and maps that to what to check per test. > >>> > >>> Pass resource and feature names to validate_resctrl_feature_request() > >>> directly rather than deriving them from the test name inside the > >>> function which makes the feature check easier to extend for new test > >>> cases. > >>> > >>> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > >>> Cc: <stable@vger.kernel.org> > >> > >> This does not seem to be stable material. > > > > Alone it isn't, but both 2/5 and this 3/5 are prerequisites for 4/5 as > > shown by the tags there. > > > >>> --- > >>> tools/testing/selftests/resctrl/resctrl.h | 6 +- > >>> .../testing/selftests/resctrl/resctrl_tests.c | 10 +-- > >>> tools/testing/selftests/resctrl/resctrlfs.c | 69 ++++++++----------- > >>> 3 files changed, 34 insertions(+), 51 deletions(-) > >>> > >>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h > >>> index dd07463cdf48..89ced4152933 100644 > >>> --- a/tools/testing/selftests/resctrl/resctrl.h > >>> +++ b/tools/testing/selftests/resctrl/resctrl.h > > > >>> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c > >>> index bd36ee206602..bd547a10791c 100644 > >>> --- a/tools/testing/selftests/resctrl/resctrlfs.c > >>> +++ b/tools/testing/selftests/resctrl/resctrlfs.c > >>> @@ -10,6 +10,8 @@ > >>> */ > >>> #include "resctrl.h" > >>> > >>> +#include <limits.h> > >>> + > >> > >> Could you please include <limits.h> before the local resctrl.h? > > > > Believe me I tried that first but it did not work. So this intentionally > > in the current order as resctrl.h defines _GNU_SOURCE which is among > > things that tends to alter many things. If I reorder them, the build gives > > me these issues: > > > > resctrlfs.c: In function ‘taskset_benchmark’: > > resctrlfs.c:284:2: warning: implicit declaration of function ‘CPU_ZERO’; > > did you mean ‘FP_ZERO’? [-Wimplicit-function-declaration] > > 284 | CPU_ZERO(&my_set); > > | ^~~~~~~~ > > | FP_ZERO > > resctrlfs.c:285:2: warning: implicit declaration of function ‘CPU_SET’ > > [-Wimplicit-function-declaration] > > 285 | CPU_SET(cpu_no, &my_set); > > | ^~~~~~~ > > resctrlfs.c:287:6: warning: implicit declaration of function > > ‘sched_setaffinity’ [-Wimplicit-function-declaration] > > 287 | if (sched_setaffinity(bm_pid, sizeof(cpu_set_t), &my_set)) { > > | ^~~~~~~~~~~~~~~~~ > > > > It might be useful to move _GNU_SOURCE define into Makefile though to > > avoid these kind of issues (but that's not material for this patch). > > How about a #define _GNU_SOURCE in this file as an intermediate step? > I did see your patch making this change but cannot see how it is > coordinated with fixing the include order in this file. I'll just make that change part of this series and use also it as dependency. Making an intermediate step just for stable that is going to immediately removed in mainline would just causing the code to diverge unnecessarily, IMO. There's also a small risk for some other bug that does not cause compile to fail due to differences because of a late define for _GNU_SOURCE. I don't find it very likely but seems possible due to differences in some constant values (not that the resctrl selftest code is very good at using those defined constants in the first place, there are plenty of literals still to cleanup). > >>> static int find_resctrl_mount(char *buffer) > >>> { > >>> FILE *mounts; > >>> @@ -604,63 +606,46 @@ char *fgrep(FILE *inf, const char *str) > >>> > >>> /* > >>> * validate_resctrl_feature_request - Check if requested feature is valid. > >>> - * @resctrl_val: Requested feature > >>> + * @resource: Required resource (e.g., MB, L3, L2, L3_MON, etc.) > >>> + * @feature: Feature to be checked under resource (can be NULL). This path > >>> + * is relative to the resource path. > >> > >> I do not think "this path" is accurate. @feature is not a path but an entry > >> within the mon_features file. > > > > Yes, agreed. > > > >> Also please note that mon_features only exists for L3_MON, none of the other > >> listed resources have an associated mon_features file in resctrl. This > >> function is created to be generic has specific requirements on what > >> valid (never checked) parameters should be. This may be ok with the usage > >> but it should not pretend to be generic. > > > > So are you recommending I split this function into two where the new one > > would do the mon_features check? > > No need to split the function. That seems overkill considering its > captive usage. I think a snippet making its usage clear will be helpful. > Something like: > > @feature: <description>. Can only be set for L3_MON. Must be > NULL for all other resources. > > Please feel free to improve. Thanks, I'll do that.
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index dd07463cdf48..89ced4152933 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -28,10 +28,6 @@ #define RESCTRL_PATH "/sys/fs/resctrl" #define PHYS_ID_PATH "/sys/devices/system/cpu/cpu" #define INFO_PATH "/sys/fs/resctrl/info" -#define L3_PATH "/sys/fs/resctrl/info/L3" -#define MB_PATH "/sys/fs/resctrl/info/MB" -#define L3_MON_PATH "/sys/fs/resctrl/info/L3_MON" -#define L3_MON_FEATURES_PATH "/sys/fs/resctrl/info/L3_MON/mon_features" #define ARCH_INTEL 1 #define ARCH_AMD 2 @@ -88,7 +84,7 @@ int get_resource_id(int cpu_no, int *resource_id); int mount_resctrlfs(void); int umount_resctrlfs(void); int validate_bw_report_request(char *bw_report); -bool validate_resctrl_feature_request(const char *resctrl_val); +bool validate_resctrl_feature_request(const char *resource, const char *feature); char *fgrep(FILE *inf, const char *str); int taskset_benchmark(pid_t bm_pid, int cpu_no); void run_benchmark(int signum, siginfo_t *info, void *ucontext); diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index 3d66fbdc2df3..3052394ca884 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -84,7 +84,9 @@ static void run_mbm_test(const char * const *benchmark_cmd, int cpu_no) return; } - if (!validate_resctrl_feature_request(MBM_STR) || (get_vendor() != ARCH_INTEL)) { + if (!validate_resctrl_feature_request("L3_MON", "mbm_total_bytes") || + !validate_resctrl_feature_request("L3_MON", "mbm_local_bytes") || + (get_vendor() != ARCH_INTEL)) { ksft_test_result_skip("Hardware does not support MBM or MBM is disabled\n"); goto umount; } @@ -116,7 +118,7 @@ static void run_mba_test(const char * const *benchmark_cmd, int cpu_no) return; } - if (!validate_resctrl_feature_request(MBA_STR) || (get_vendor() != ARCH_INTEL)) { + if (!validate_resctrl_feature_request("MB", NULL) || (get_vendor() != ARCH_INTEL)) { ksft_test_result_skip("Hardware does not support MBA or MBA is disabled\n"); goto umount; } @@ -146,7 +148,7 @@ static void run_cmt_test(const char * const *benchmark_cmd, int cpu_no) return; } - if (!validate_resctrl_feature_request(CMT_STR)) { + if (!validate_resctrl_feature_request("L3_MON", "llc_occupancy")) { ksft_test_result_skip("Hardware does not support CMT or CMT is disabled\n"); goto umount; } @@ -178,7 +180,7 @@ static void run_cat_test(int cpu_no, int no_of_bits) return; } - if (!validate_resctrl_feature_request(CAT_STR)) { + if (!validate_resctrl_feature_request("L3", NULL)) { ksft_test_result_skip("Hardware does not support CAT or CAT is disabled\n"); goto umount; } diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index bd36ee206602..bd547a10791c 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -10,6 +10,8 @@ */ #include "resctrl.h" +#include <limits.h> + static int find_resctrl_mount(char *buffer) { FILE *mounts; @@ -604,63 +606,46 @@ char *fgrep(FILE *inf, const char *str) /* * validate_resctrl_feature_request - Check if requested feature is valid. - * @resctrl_val: Requested feature + * @resource: Required resource (e.g., MB, L3, L2, L3_MON, etc.) + * @feature: Feature to be checked under resource (can be NULL). This path + * is relative to the resource path. * - * Return: True if the feature is supported, else false. False is also - * returned if resctrl FS is not mounted. + * Return: True if the resource/feature is supported, else false. False is + * also returned if resctrl FS is not mounted. */ -bool validate_resctrl_feature_request(const char *resctrl_val) +bool validate_resctrl_feature_request(const char *resource, const char *feature) { struct stat statbuf; - bool found = false; + char res_path[PATH_MAX]; char *res; FILE *inf; int ret; - if (!resctrl_val) + if (!resource) return false; ret = find_resctrl_mount(NULL); if (ret) return false; - if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR))) { - if (!stat(L3_PATH, &statbuf)) - return true; - } else if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) { - if (!stat(MB_PATH, &statbuf)) - return true; - } else if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) || - !strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) { - if (!stat(L3_MON_PATH, &statbuf)) { - inf = fopen(L3_MON_FEATURES_PATH, "r"); - if (!inf) - return false; - - if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) { - res = fgrep(inf, "llc_occupancy"); - if (res) { - found = true; - free(res); - } - } - - if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR))) { - res = fgrep(inf, "mbm_total_bytes"); - if (res) { - free(res); - res = fgrep(inf, "mbm_local_bytes"); - if (res) { - found = true; - free(res); - } - } - } - fclose(inf); - } - } + snprintf(res_path, sizeof(res_path), "%s/%s", INFO_PATH, resource); + + if (stat(res_path, &statbuf)) + return false; + + if (!feature) + return true; + + snprintf(res_path, sizeof(res_path), "%s/%s/mon_features", INFO_PATH, resource); + inf = fopen(res_path, "r"); + if (!inf) + return false; + + res = fgrep(inf, feature); + free(res); + fclose(inf); - return found; + return res; } int filter_dmesg(void)
Feature check in validate_resctrl_feature_request() takes in the test name string and maps that to what to check per test. Pass resource and feature names to validate_resctrl_feature_request() directly rather than deriving them from the test name inside the function which makes the feature check easier to extend for new test cases. Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Cc: <stable@vger.kernel.org> --- tools/testing/selftests/resctrl/resctrl.h | 6 +- .../testing/selftests/resctrl/resctrl_tests.c | 10 +-- tools/testing/selftests/resctrl/resctrlfs.c | 69 ++++++++----------- 3 files changed, 34 insertions(+), 51 deletions(-)