| Message ID | 20230418114506.46788-5-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/resctrl: Fixes, cleanups, and rewritten CAT test | expand |
Hi Ilpo, On 4/18/2023 4:44 AM, Ilpo Järvinen wrote: > Resctrl FS mount/umount are now cleanly paired. > > Removed mum_resctrlfs that is what is left of the earlier remount Removed -> Remove? I am not sure what is meant with "that is what is left" ... > trickery to make the code cleaner. Rename remount_resctrlfs() to > mount_resctrlfs() to match the reduced functionality. These two logical changes would be easier to review if done in two patches. ... > diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c > index 5c9ed52b69f2..f3303459136d 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(false); > + res = mount_resctrlfs(); As mentioned in previous patch the way I see it remount is still needed. > 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(false); > + 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(false); > + 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(false); > + 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 42f547a81e25..45f785213143 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; This seems to assume that resctrl is always unmounted. Should the main program thus start by unmounting resctrl before it runs any test in case it is mounted when user space starts the tests? Reinette
On Fri, 21 Apr 2023, Reinette Chatre 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 seems to assume that resctrl is always unmounted. Should the main > program thus start by unmounting resctrl before it runs any test in case > it is mounted when user space starts the tests? I thought that was the wanted functionality. I've now added a change to the series which does this umount before starting a tests.
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index dec3151cf888..86c62dfd8e78 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -140,7 +140,6 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) struct resctrl_val_param param = { .resctrl_val = CAT_STR, .cpu_no = cpu_no, - .mum_resctrlfs = false, .setup = cat_setup, }; diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c index 426d11189a99..d31e28416bb7 100644 --- a/tools/testing/selftests/resctrl/cmt_test.c +++ b/tools/testing/selftests/resctrl/cmt_test.c @@ -113,7 +113,6 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd) .ctrlgrp = "c1", .mongrp = "m1", .cpu_no = cpu_no, - .mum_resctrlfs = false, .filename = RESULT_FILE_NAME, .mask = ~(long_mask << n) & long_mask, .span = cache_size * n / count_of_bits, diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c index cde3781a9ab0..3d53c6c9b9ce 100644 --- a/tools/testing/selftests/resctrl/mba_test.c +++ b/tools/testing/selftests/resctrl/mba_test.c @@ -154,7 +154,6 @@ int mba_schemata_change(int cpu_no, char *bw_report, char **benchmark_cmd) .ctrlgrp = "c1", .mongrp = "m1", .cpu_no = cpu_no, - .mum_resctrlfs = true, .filename = RESULT_FILE_NAME, .bw_report = bw_report, .setup = mba_setup diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c index 538d35a6485a..24326cb7bc21 100644 --- a/tools/testing/selftests/resctrl/mbm_test.c +++ b/tools/testing/selftests/resctrl/mbm_test.c @@ -123,7 +123,6 @@ int mbm_bw_change(int span, int cpu_no, char *bw_report, char **benchmark_cmd) .mongrp = "m1", .span = span, .cpu_no = cpu_no, - .mum_resctrlfs = true, .filename = RESULT_FILE_NAME, .bw_report = bw_report, .setup = mbm_setup diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index 87e39456dee0..c737eb47eacc 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -53,7 +53,6 @@ * @mongrp: Name of the monitor group (mon grp) * @cpu_no: CPU number to which the benchmark would be binded * @span: Memory bytes accessed in each benchmark iteration - * @mum_resctrlfs: Should the resctrl FS be remounted? * @filename: Name of file to which the o/p should be written * @bw_report: Bandwidth report type (reads vs writes) * @setup: Call back function to setup test environment @@ -64,7 +63,6 @@ struct resctrl_val_param { char mongrp[64]; int cpu_no; unsigned long span; - bool mum_resctrlfs; char filename[64]; char *bw_report; unsigned long mask; @@ -84,8 +82,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 5c9ed52b69f2..f3303459136d 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(false); + 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(false); + 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(false); + 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(false); + 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 42f547a81e25..45f785213143 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);
Resctrl FS mount/umount are now cleanly paired. Removed mum_resctrlfs that is what is left of the earlier remount trickery to make the code cleaner. Rename remount_resctrlfs() to mount_resctrlfs() to match the reduced functionality. 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/cat_test.c | 1 - tools/testing/selftests/resctrl/cmt_test.c | 1 - tools/testing/selftests/resctrl/mba_test.c | 1 - tools/testing/selftests/resctrl/mbm_test.c | 1 - tools/testing/selftests/resctrl/resctrl.h | 4 +--- .../testing/selftests/resctrl/resctrl_tests.c | 8 ++++---- tools/testing/selftests/resctrl/resctrlfs.c | 20 +++++-------------- 7 files changed, 10 insertions(+), 26 deletions(-)