| Message ID | 20230713131932.133258-8-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/resctrl: Fixes and cleanups | expand |
Hi Ilpo, 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". > > ksft_print_msg("Mounting resctrl to \"%s\"\n", RESCTRL_PATH); > ret = mount("resctrl", RESCTRL_PATH, "resctrl", 0, NULL); Reinette
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.
Hi! 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? Kind regards Maciej Wieczór-Retman
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(-)