| Message ID | e4b1887682e8262a3416dbd7bc63695af0d63ee0.1707130307.git.maciej.wieczor-retman@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/resctrl: Add non-contiguous CBMs in Intel CAT selftest | expand |
On Mon, 5 Feb 2024, Maciej Wieczor-Retman wrote: > validate_resctrl_feature_request() is used to test both if a resource is > present in the info directory, and if a passed monitoring feature is > present in the mon_features file. > > Refactor validate_resctrl_feature_request() into two smaller functions > that each accomplish one check to give feature checking more > granularity: > - Resource directory presence in the /sys/fs/resctrl/info directory. > - Feature name presence in the /sys/fs/resctrl/info/L3_MON/mon_features > file. > > Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com> > --- > Changelog v4: > - Roll back to using test_resource_feature_check() for CMT and MBA. > (Ilpo). > > Changelog v3: > - Move new function to a separate patch. (Reinette) > - Rewrite resctrl_mon_feature_exists() only for L3_MON. > > Changelog v2: > - Add this patch. > > tools/testing/selftests/resctrl/cmt_test.c | 2 +- > tools/testing/selftests/resctrl/mba_test.c | 2 +- > tools/testing/selftests/resctrl/mbm_test.c | 6 ++-- > tools/testing/selftests/resctrl/resctrl.h | 3 +- > tools/testing/selftests/resctrl/resctrlfs.c | 33 +++++++++++++-------- > 5 files changed, 28 insertions(+), 18 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c > index dd5ca343c469..c1157917a814 100644 > --- a/tools/testing/selftests/resctrl/cmt_test.c > +++ b/tools/testing/selftests/resctrl/cmt_test.c > @@ -170,7 +170,7 @@ static int cmt_run_test(const struct resctrl_test *test, const struct user_param > static bool cmt_feature_check(const struct resctrl_test *test) > { > return test_resource_feature_check(test) && > - validate_resctrl_feature_request("L3_MON", "llc_occupancy"); > + resctrl_resource_exists("L3"); This not correctly transformed. > +/* > + * resctrl_mon_feature_exists - Check if requested monitoring L3_MON feature is valid. > + * @feature: Required monitor feature (in mon_features file). > + * > + * Return: True if the feature is supported, else false. > + */ > +bool resctrl_mon_feature_exists(const char *feature) > +{ > + char *res; > + FILE *inf; > + > if (!feature) > - return true; > + return false; > > - snprintf(res_path, sizeof(res_path), "%s/%s/mon_features", INFO_PATH, resource); > - inf = fopen(res_path, "r"); > + inf = fopen("/sys/fs/resctrl/info/L3_MON/mon_features", "r"); This became less generic? Could there be other MON resource besides L3 one? Perhaps there aren't today but why remove the ability give it as a parameter?
On 2024-02-05 at 14:41:30 +0200, Ilpo Järvinen wrote: >On Mon, 5 Feb 2024, Maciej Wieczor-Retman wrote: > >> validate_resctrl_feature_request() is used to test both if a resource is >> present in the info directory, and if a passed monitoring feature is >> present in the mon_features file. >> >> Refactor validate_resctrl_feature_request() into two smaller functions >> that each accomplish one check to give feature checking more >> granularity: >> - Resource directory presence in the /sys/fs/resctrl/info directory. >> - Feature name presence in the /sys/fs/resctrl/info/L3_MON/mon_features >> file. >> >> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com> >> --- >> Changelog v4: >> - Roll back to using test_resource_feature_check() for CMT and MBA. >> (Ilpo). >> >> Changelog v3: >> - Move new function to a separate patch. (Reinette) >> - Rewrite resctrl_mon_feature_exists() only for L3_MON. >> >> Changelog v2: >> - Add this patch. >> >> tools/testing/selftests/resctrl/cmt_test.c | 2 +- >> tools/testing/selftests/resctrl/mba_test.c | 2 +- >> tools/testing/selftests/resctrl/mbm_test.c | 6 ++-- >> tools/testing/selftests/resctrl/resctrl.h | 3 +- >> tools/testing/selftests/resctrl/resctrlfs.c | 33 +++++++++++++-------- >> 5 files changed, 28 insertions(+), 18 deletions(-) >> > >> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c >> index dd5ca343c469..c1157917a814 100644 >> --- a/tools/testing/selftests/resctrl/cmt_test.c >> +++ b/tools/testing/selftests/resctrl/cmt_test.c >> @@ -170,7 +170,7 @@ static int cmt_run_test(const struct resctrl_test *test, const struct user_param >> static bool cmt_feature_check(const struct resctrl_test *test) >> { >> return test_resource_feature_check(test) && >> - validate_resctrl_feature_request("L3_MON", "llc_occupancy"); >> + resctrl_resource_exists("L3"); > >This not correctly transformed. Oops, sorry, I'll fix it for the next version. > >> +/* >> + * resctrl_mon_feature_exists - Check if requested monitoring L3_MON feature is valid. >> + * @feature: Required monitor feature (in mon_features file). >> + * >> + * Return: True if the feature is supported, else false. >> + */ >> +bool resctrl_mon_feature_exists(const char *feature) >> +{ >> + char *res; >> + FILE *inf; >> + >> if (!feature) >> - return true; >> + return false; >> >> - snprintf(res_path, sizeof(res_path), "%s/%s/mon_features", INFO_PATH, resource); >> - inf = fopen(res_path, "r"); >> + inf = fopen("/sys/fs/resctrl/info/L3_MON/mon_features", "r"); > >This became less generic? Could there be other MON resource besides L3 >one? Perhaps there aren't today but why remove the ability give it as a >parameter? During v2 discussion [1] Reinette made me realize this functionality only interfaces with L3_MON/mon_features file and the 'resource' parameter isn't needed. The 'mon_features' file is only mentioned for L3_MON and I don't know of any plans for other MON resources so I assumed it doesn't need to be generic. But sure, I can make it use a parameter if Reinette doesn't mind. [1] https://lore.kernel.org/all/2o7adr2cos6qcikcu7oop4ss7vib2n6ue33djgfeds3v6gj53f@uu45lomrp5qv/ > > >-- > i. >
On 2/5/2024 5:24 AM, Maciej Wieczor-Retman wrote: > On 2024-02-05 at 14:41:30 +0200, Ilpo Järvinen wrote: >> On Mon, 5 Feb 2024, Maciej Wieczor-Retman wrote: >> >>> validate_resctrl_feature_request() is used to test both if a resource is >>> present in the info directory, and if a passed monitoring feature is >>> present in the mon_features file. >>> >>> Refactor validate_resctrl_feature_request() into two smaller functions >>> that each accomplish one check to give feature checking more >>> granularity: >>> - Resource directory presence in the /sys/fs/resctrl/info directory. >>> - Feature name presence in the /sys/fs/resctrl/info/L3_MON/mon_features >>> file. >>> >>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com> >>> --- >>> Changelog v4: >>> - Roll back to using test_resource_feature_check() for CMT and MBA. >>> (Ilpo). >>> >>> Changelog v3: >>> - Move new function to a separate patch. (Reinette) >>> - Rewrite resctrl_mon_feature_exists() only for L3_MON. >>> >>> Changelog v2: >>> - Add this patch. >>> >>> tools/testing/selftests/resctrl/cmt_test.c | 2 +- >>> tools/testing/selftests/resctrl/mba_test.c | 2 +- >>> tools/testing/selftests/resctrl/mbm_test.c | 6 ++-- >>> tools/testing/selftests/resctrl/resctrl.h | 3 +- >>> tools/testing/selftests/resctrl/resctrlfs.c | 33 +++++++++++++-------- >>> 5 files changed, 28 insertions(+), 18 deletions(-) >>> >> >>> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c >>> index dd5ca343c469..c1157917a814 100644 >>> --- a/tools/testing/selftests/resctrl/cmt_test.c >>> +++ b/tools/testing/selftests/resctrl/cmt_test.c >>> @@ -170,7 +170,7 @@ static int cmt_run_test(const struct resctrl_test *test, const struct user_param >>> static bool cmt_feature_check(const struct resctrl_test *test) >>> { >>> return test_resource_feature_check(test) && >>> - validate_resctrl_feature_request("L3_MON", "llc_occupancy"); >>> + resctrl_resource_exists("L3"); >> >> This not correctly transformed. > > Oops, sorry, I'll fix it for the next version. > >> >>> +/* >>> + * resctrl_mon_feature_exists - Check if requested monitoring L3_MON feature is valid. >>> + * @feature: Required monitor feature (in mon_features file). >>> + * >>> + * Return: True if the feature is supported, else false. >>> + */ >>> +bool resctrl_mon_feature_exists(const char *feature) >>> +{ >>> + char *res; >>> + FILE *inf; >>> + >>> if (!feature) >>> - return true; >>> + return false; >>> >>> - snprintf(res_path, sizeof(res_path), "%s/%s/mon_features", INFO_PATH, resource); >>> - inf = fopen(res_path, "r"); >>> + inf = fopen("/sys/fs/resctrl/info/L3_MON/mon_features", "r"); >> >> This became less generic? Could there be other MON resource besides L3 >> one? Perhaps there aren't today but why remove the ability give it as a >> parameter? This does make the function less generic but the benefit is that four copies of the same hardcoded parameter is no longer needed. In my opinion this patch thus makes the code cleaner but it is not a requirement that I will use to hold this series back. > During v2 discussion [1] Reinette made me realize this functionality only > interfaces with L3_MON/mon_features file and the 'resource' parameter isn't > needed. The 'mon_features' file is only mentioned for L3_MON and I don't know of > any plans for other MON resources so I assumed it doesn't need to be generic. > > But sure, I can make it use a parameter if Reinette doesn't mind. I prefer what is in this patch, but I will not object if the function is changed to take the resource as parameter. > > [1] https://lore.kernel.org/all/2o7adr2cos6qcikcu7oop4ss7vib2n6ue33djgfeds3v6gj53f@uu45lomrp5qv/ > Reinette
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c index dd5ca343c469..c1157917a814 100644 --- a/tools/testing/selftests/resctrl/cmt_test.c +++ b/tools/testing/selftests/resctrl/cmt_test.c @@ -170,7 +170,7 @@ static int cmt_run_test(const struct resctrl_test *test, const struct user_param static bool cmt_feature_check(const struct resctrl_test *test) { return test_resource_feature_check(test) && - validate_resctrl_feature_request("L3_MON", "llc_occupancy"); + resctrl_resource_exists("L3"); } struct resctrl_test cmt_test = { diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c index da256d2dbe5c..fa99a91c8ab7 100644 --- a/tools/testing/selftests/resctrl/mba_test.c +++ b/tools/testing/selftests/resctrl/mba_test.c @@ -171,7 +171,7 @@ static int mba_run_test(const struct resctrl_test *test, const struct user_param static bool mba_feature_check(const struct resctrl_test *test) { return test_resource_feature_check(test) && - validate_resctrl_feature_request("L3_MON", "mbm_local_bytes"); + resctrl_mon_feature_exists("mbm_local_bytes"); } struct resctrl_test mba_test = { diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c index 34879e7b71a0..9c885bc427ca 100644 --- a/tools/testing/selftests/resctrl/mbm_test.c +++ b/tools/testing/selftests/resctrl/mbm_test.c @@ -97,7 +97,7 @@ static int mbm_setup(const struct resctrl_test *test, return END_OF_TESTS; /* Set up shemata with 100% allocation on the first run. */ - if (p->num_of_runs == 0 && validate_resctrl_feature_request("MB", NULL)) + if (p->num_of_runs == 0 && resctrl_resource_exists("MB")) ret = write_schemata(p->ctrlgrp, "100", uparams->cpu, test->resource); p->num_of_runs++; @@ -140,8 +140,8 @@ static int mbm_run_test(const struct resctrl_test *test, const struct user_param static bool mbm_feature_check(const struct resctrl_test *test) { - return validate_resctrl_feature_request("L3_MON", "mbm_total_bytes") && - validate_resctrl_feature_request("L3_MON", "mbm_local_bytes"); + return resctrl_mon_feature_exists("mbm_total_bytes") && + resctrl_mon_feature_exists("mbm_local_bytes"); } struct resctrl_test mbm_test = { diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index 5116ea082d03..4603b215b97e 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -136,7 +136,8 @@ int get_domain_id(const char *resource, int cpu_no, int *domain_id); int mount_resctrlfs(void); int umount_resctrlfs(void); int validate_bw_report_request(char *bw_report); -bool validate_resctrl_feature_request(const char *resource, const char *feature); +bool resctrl_resource_exists(const char *resource); +bool resctrl_mon_feature_exists(const char *feature); bool test_resource_feature_check(const struct resctrl_test *test); char *fgrep(FILE *inf, const char *str); int taskset_benchmark(pid_t bm_pid, int cpu_no, cpu_set_t *old_affinity); diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index e0fbc46a917a..0cfec8bb23fd 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -708,20 +708,16 @@ char *fgrep(FILE *inf, const char *str) } /* - * validate_resctrl_feature_request - Check if requested feature is valid. - * @resource: Required resource (e.g., MB, L3, L2, L3_MON, etc.) - * @feature: Required monitor feature (in mon_features file). Can only be - * set for L3_MON. Must be NULL for all other resources. + * resctrl_resource_exists - Check if a resource is supported. + * @resource: Resctrl resource (e.g., MB, L3, L2, L3_MON, etc.) * - * Return: True if the resource/feature is supported, else false. False is + * Return: True if the resource is supported, else false. False is * also returned if resctrl FS is not mounted. */ -bool validate_resctrl_feature_request(const char *resource, const char *feature) +bool resctrl_resource_exists(const char *resource) { char res_path[PATH_MAX]; struct stat statbuf; - char *res; - FILE *inf; int ret; if (!resource) @@ -736,11 +732,24 @@ bool validate_resctrl_feature_request(const char *resource, const char *feature) if (stat(res_path, &statbuf)) return false; + return true; +} + +/* + * resctrl_mon_feature_exists - Check if requested monitoring L3_MON feature is valid. + * @feature: Required monitor feature (in mon_features file). + * + * Return: True if the feature is supported, else false. + */ +bool resctrl_mon_feature_exists(const char *feature) +{ + char *res; + FILE *inf; + if (!feature) - return true; + return false; - snprintf(res_path, sizeof(res_path), "%s/%s/mon_features", INFO_PATH, resource); - inf = fopen(res_path, "r"); + inf = fopen("/sys/fs/resctrl/info/L3_MON/mon_features", "r"); if (!inf) return false; @@ -753,7 +762,7 @@ bool validate_resctrl_feature_request(const char *resource, const char *feature) bool test_resource_feature_check(const struct resctrl_test *test) { - return validate_resctrl_feature_request(test->resource, NULL); + return resctrl_resource_exists(test->resource); } int filter_dmesg(void)
validate_resctrl_feature_request() is used to test both if a resource is present in the info directory, and if a passed monitoring feature is present in the mon_features file. Refactor validate_resctrl_feature_request() into two smaller functions that each accomplish one check to give feature checking more granularity: - Resource directory presence in the /sys/fs/resctrl/info directory. - Feature name presence in the /sys/fs/resctrl/info/L3_MON/mon_features file. Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com> --- Changelog v4: - Roll back to using test_resource_feature_check() for CMT and MBA. (Ilpo). Changelog v3: - Move new function to a separate patch. (Reinette) - Rewrite resctrl_mon_feature_exists() only for L3_MON. Changelog v2: - Add this patch. tools/testing/selftests/resctrl/cmt_test.c | 2 +- tools/testing/selftests/resctrl/mba_test.c | 2 +- tools/testing/selftests/resctrl/mbm_test.c | 6 ++-- tools/testing/selftests/resctrl/resctrl.h | 3 +- tools/testing/selftests/resctrl/resctrlfs.c | 33 +++++++++++++-------- 5 files changed, 28 insertions(+), 18 deletions(-)