| Message ID | 20230713131932.133258-8-ilpo.jarvinen@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | selftests/resctrl: Fixes and cleanups | expand |
On Thu, 13 Jul 2023, Reinette Chatre wrote: > On 7/13/2023 6:19 AM, Ilpo Järvinen wrote: > > Mount/umount of the resctrl FS is now paired nicely per test. > > > > Rename remount_resctrl(bool mum_resctrlfs) to mount_resctrl(). Make > > it unconditionally try to mount the resctrl FS and return error if > > resctrl FS was mounted already. > > > > While at it, group the mount/umount prototypes in the header. > > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > --- > > tools/testing/selftests/resctrl/resctrl.h | 2 +- > > .../testing/selftests/resctrl/resctrl_tests.c | 8 ++++---- > > tools/testing/selftests/resctrl/resctrlfs.c | 20 +++++-------------- > > 3 files changed, 10 insertions(+), 20 deletions(-) > > > > diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h > > index f455f0b7e314..23af3fb73cb4 100644 > > --- a/tools/testing/selftests/resctrl/resctrl.h > > +++ b/tools/testing/selftests/resctrl/resctrl.h > > @@ -85,8 +85,8 @@ extern char llc_occup_path[1024]; > > int get_vendor(void); > > bool check_resctrlfs_support(void); > > int filter_dmesg(void); > > -int remount_resctrlfs(bool mum_resctrlfs); > > 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); > > diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c > > index a421d045de08..3f26d2279f75 100644 > > --- a/tools/testing/selftests/resctrl/resctrl_tests.c > > +++ b/tools/testing/selftests/resctrl/resctrl_tests.c > > @@ -77,7 +77,7 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, int span, > > > > ksft_print_msg("Starting MBM BW change ...\n"); > > > > - res = remount_resctrlfs(true); > > + res = mount_resctrlfs(); > > if (res) { > > ksft_exit_fail_msg("Failed to mount resctrl FS\n"); > > return; > > @@ -106,7 +106,7 @@ static void run_mba_test(bool has_ben, char **benchmark_cmd, int span, > > > > ksft_print_msg("Starting MBA Schemata change ...\n"); > > > > - res = remount_resctrlfs(true); > > + res = mount_resctrlfs(); > > if (res) { > > ksft_exit_fail_msg("Failed to mount resctrl FS\n"); > > return; > > @@ -132,7 +132,7 @@ static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no) > > > > ksft_print_msg("Starting CMT test ...\n"); > > > > - res = remount_resctrlfs(true); > > + res = mount_resctrlfs(); > > if (res) { > > ksft_exit_fail_msg("Failed to mount resctrl FS\n"); > > return; > > @@ -160,7 +160,7 @@ static void run_cat_test(int cpu_no, int no_of_bits) > > > > ksft_print_msg("Starting CAT test ...\n"); > > > > - res = remount_resctrlfs(true); > > + res = mount_resctrlfs(); > > if (res) { > > ksft_exit_fail_msg("Failed to mount resctrl FS\n"); > > return; > > diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c > > index b3a05488d360..f622245adafe 100644 > > --- a/tools/testing/selftests/resctrl/resctrlfs.c > > +++ b/tools/testing/selftests/resctrl/resctrlfs.c > > @@ -48,29 +48,19 @@ static int find_resctrl_mount(char *buffer) > > } > > > > /* > > - * remount_resctrlfs - Remount resctrl FS at /sys/fs/resctrl > > - * @mum_resctrlfs: Should the resctrl FS be remounted? > > + * mount_resctrlfs - Mount resctrl FS at /sys/fs/resctrl > > * > > * If not mounted, mount it. > > - * If mounted and mum_resctrlfs then remount resctrl FS. > > - * If mounted and !mum_resctrlfs then noop > > * > > * Return: 0 on success, non-zero on failure > > */ > > Since it is not obviously a "failure" I do think it will help to > add to the comments that resctrl already being mounted is treated as > a failure. > > > -int remount_resctrlfs(bool mum_resctrlfs) > > +int mount_resctrlfs(void) > > { > > - char mountpoint[256]; > > int ret; > > > > - ret = find_resctrl_mount(mountpoint); > > - if (ret) > > - strcpy(mountpoint, RESCTRL_PATH); > > - > > - if (!ret && mum_resctrlfs && umount(mountpoint)) > > - ksft_print_msg("Fail: unmounting \"%s\"\n", mountpoint); > > - > > - if (!ret && !mum_resctrlfs) > > - return 0; > > + ret = find_resctrl_mount(NULL); > > + if (!ret) > > + return -1; > > This treats "ret == 0" as a failure. What about -ENXIO? It seems to > me that only "ret == -ENOENT" is "success". Yes, it's a good catch.
On Mon, 24 Jul 2023, Wieczor-Retman, Maciej wrote: > On 14.07.2023 13:03, Ilpo Järvinen wrote: > > On Thu, 13 Jul 2023, Reinette Chatre wrote: > >> On 7/13/2023 6:19 AM, Ilpo Järvinen wrote: > >>> -int remount_resctrlfs(bool mum_resctrlfs) > >>> +int mount_resctrlfs(void) > >>> { > >>> - char mountpoint[256]; > >>> int ret; > >>> > >>> - ret = find_resctrl_mount(mountpoint); > >>> - if (ret) > >>> - strcpy(mountpoint, RESCTRL_PATH); > >>> - > >>> - if (!ret && mum_resctrlfs && umount(mountpoint)) > >>> - ksft_print_msg("Fail: unmounting \"%s\"\n", mountpoint); > >>> - > >>> - if (!ret && !mum_resctrlfs) > >>> - return 0; > >>> + ret = find_resctrl_mount(NULL); > >>> + if (!ret) > >>> + return -1; > >> > >> This treats "ret == 0" as a failure. What about -ENXIO? It seems to > >> me that only "ret == -ENOENT" is "success". > > > > Yes, it's a good catch. > > > > I had an idea about a small redesign of find_resctrl_mount > return values so it is easier to see what the function tries > to accomplish. > > When there is an error (-ENXIO for example) it could > return the negative error value. When no mount is found > it could return a zero (instead of the -ENOENT error code). > Finally when a mount point was found it could return a positive > value (for example return 1). This way errors could be > separate from regular return values and in my opinion the > function logic would be more transparent. > > What do you think about it? Yes, it's a great idea. It would be more logical and thus easier to comprehend. Since this already wast applied, it has to be done in another change.
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index f455f0b7e314..23af3fb73cb4 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -85,8 +85,8 @@ extern char llc_occup_path[1024]; int get_vendor(void); bool check_resctrlfs_support(void); int filter_dmesg(void); -int remount_resctrlfs(bool mum_resctrlfs); 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); diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index a421d045de08..3f26d2279f75 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -77,7 +77,7 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, int span, ksft_print_msg("Starting MBM BW change ...\n"); - res = remount_resctrlfs(true); + res = mount_resctrlfs(); if (res) { ksft_exit_fail_msg("Failed to mount resctrl FS\n"); return; @@ -106,7 +106,7 @@ static void run_mba_test(bool has_ben, char **benchmark_cmd, int span, ksft_print_msg("Starting MBA Schemata change ...\n"); - res = remount_resctrlfs(true); + res = mount_resctrlfs(); if (res) { ksft_exit_fail_msg("Failed to mount resctrl FS\n"); return; @@ -132,7 +132,7 @@ static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no) ksft_print_msg("Starting CMT test ...\n"); - res = remount_resctrlfs(true); + res = mount_resctrlfs(); if (res) { ksft_exit_fail_msg("Failed to mount resctrl FS\n"); return; @@ -160,7 +160,7 @@ static void run_cat_test(int cpu_no, int no_of_bits) ksft_print_msg("Starting CAT test ...\n"); - res = remount_resctrlfs(true); + res = mount_resctrlfs(); if (res) { ksft_exit_fail_msg("Failed to mount resctrl FS\n"); return; diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index b3a05488d360..f622245adafe 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -48,29 +48,19 @@ static int find_resctrl_mount(char *buffer) } /* - * remount_resctrlfs - Remount resctrl FS at /sys/fs/resctrl - * @mum_resctrlfs: Should the resctrl FS be remounted? + * mount_resctrlfs - Mount resctrl FS at /sys/fs/resctrl * * If not mounted, mount it. - * If mounted and mum_resctrlfs then remount resctrl FS. - * If mounted and !mum_resctrlfs then noop * * Return: 0 on success, non-zero on failure */ -int remount_resctrlfs(bool mum_resctrlfs) +int mount_resctrlfs(void) { - char mountpoint[256]; int ret; - ret = find_resctrl_mount(mountpoint); - if (ret) - strcpy(mountpoint, RESCTRL_PATH); - - if (!ret && mum_resctrlfs && umount(mountpoint)) - ksft_print_msg("Fail: unmounting \"%s\"\n", mountpoint); - - if (!ret && !mum_resctrlfs) - return 0; + ret = find_resctrl_mount(NULL); + if (!ret) + return -1; ksft_print_msg("Mounting resctrl to \"%s\"\n", RESCTRL_PATH); ret = mount("resctrl", RESCTRL_PATH, "resctrl", 0, NULL);
Mount/umount of the resctrl FS is now paired nicely per test. Rename remount_resctrl(bool mum_resctrlfs) to mount_resctrl(). Make it unconditionally try to mount the resctrl FS and return error if resctrl FS was mounted already. While at it, group the mount/umount prototypes in the header. Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> --- tools/testing/selftests/resctrl/resctrl.h | 2 +- .../testing/selftests/resctrl/resctrl_tests.c | 8 ++++---- tools/testing/selftests/resctrl/resctrlfs.c | 20 +++++-------------- 3 files changed, 10 insertions(+), 20 deletions(-)