Re: [PATCH v4 05/19] selftests/resctrl: Unmount resctrl FS before starting the first test

[Reinette Chatre]

Hi Ilpo,

On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> Resctrl FS mount/remount/umount code is hard to track. Better approach
> is to use mount/umount pair for each test but that assumes resctrl FS
> is not mounted beforehand.
> 
> Change umount_resctrlfs() so that it can unmount resctrl FS from any
> path, and enable further simplifications into mount/remount/umount
> logic by unmounting resctrl FS at the start if a pre-existing
> mountpoint is found.
> 
> Suggested-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> ---
>  tools/testing/selftests/resctrl/resctrl_tests.c | 2 ++
>  tools/testing/selftests/resctrl/resctrlfs.c     | 6 ++++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index 9b9751206e1c..b1b2d28b52f7 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -250,6 +250,8 @@ int main(int argc, char **argv)
>  	if (!check_resctrlfs_support())
>  		return ksft_exit_skip("resctrl FS does not exist. Enable X86_CPU_RESCTRL config option.\n");
>  
> +	umount_resctrlfs();
> +

umount_resctrlfs() can fail. I think there should be error checking
here and no tests should be run if resctrl cannot be unmounted since
the hardware and resctrl state is not known (without more effort).

>  	filter_dmesg();
>  
>  	ksft_set_plan(tests ? : 4);
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index fb00245dee92..23f75aeaa198 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -82,10 +82,12 @@ int remount_resctrlfs(bool mum_resctrlfs)
>  
>  int umount_resctrlfs(void)
>  {
> -	if (find_resctrl_mount(NULL))
> +	char mountpoint[256];
> +
> +	if (find_resctrl_mount(mountpoint))
>  		return 0;

It looks like the intent is to return 0 if find_resctrl_mount()
returns -ENOENT. It is not clear to me that it should also
return 0 if find_resctrl_mount() returns -ENXIO.

>  
> -	if (umount(RESCTRL_PATH)) {
> +	if (umount(mountpoint)) {
>  		perror("# Unable to umount resctrl");
>  
>  		return errno;

Reinette