On Fri, 21 Apr 2023, Reinette Chatre wrote:
On 4/18/2023 4:44 AM, Ilpo Järvinen wrote:
A few places currently lack umounting resctrl FS on error paths.
You mention "A few places" (plural). In the patch I do see that cmt_resctrl_val() is missing an unmount. Where are the other places?
- cmt_resctrl_val() has multiple error paths with direct return. - cat_perf_miss_val() has multiple error paths with direct return.
In addition, validate_resctrl_feature_request() is called by run_mbm_test() and run_mba_test(). Neither MBA nor MBM test tries to umount resctrl FS.
I improved the changelog accordingly.
While doing this, I took a more careful look into how it can result in problems and I think the only way is through PARENT_EXIT() because main has the umount in the end (and the remounting trickery kinda seems to work even if it was hard to track).
Fixing the PARENT_EXIT() problem required yet another change which I add in v3.
As the only failure I could think of is because of PARENT_EXIT(), I removed Fixes tags from this change and put one into the PARENT_EXIT() umount fix. So this change will just be part of the move towards more tractable resctrl FS handling, not a fix anymore.
In the end, after some reshuffling, I ended up having 5 changes related to this:
selftests/resctrl: Remove mum_resctrlfs from struct resctrl_val_param selftests/resctrl: Refactor remount_resctrl(bool mum_resctrlfs) to mount_resctrl() selftests/resctrl: Move resctrl FS mount/umount to higher level selftests/resctrl: Unmount resctrl FS before starting the first test selftests/resctrl: Unmount resctrl FS if child fails to run benchmark
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c index af71b2141271..426d11189a99 100644 --- a/tools/testing/selftests/resctrl/cmt_test.c +++ b/tools/testing/selftests/resctrl/cmt_test.c @@ -86,10 +86,6 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd) cache_size = 0;
- ret = remount_resctrlfs(true);
- if (ret)
return ret;- if (!validate_resctrl_feature_request(CMT_STR)) return -1;
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index 9b9751206e1c..5c9ed52b69f2 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -77,9 +77,15 @@ 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);
I think that should be remount_resctrlfs(true).
Please note that any of the tests could be run separately from the command line and thus each test need to ensure a clean environment, it cannot assume that (a) user space provided it with a clean and/or unmounted resctrl or (b) that any test was run before it.
I think I got tripped by the level of complexity here and trying to split patch to minimal parts. I was somehow thinking that remount_resctrlfs(false) would return error if resctrl fs is already mounted.
I've now changed this to pass true instead even if the argument is removed by the other change.