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.
--
i.
