Re: [PATCH v2 03/24] selftests/resctrl: Move resctrl FS mount/umount to higher level

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3302 bytes --]

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.