Re: [PATCH v7 4/6] selftests/resctrl: Cleanup properly when an error occurs in CAT test

[Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>]

On Mon, 13 Feb 2023, Shaopeng Tan wrote:

> After creating a child process with fork() in CAT test, if an error
> occurs when parent process runs cat_val() or check_results(), the child

I'd replace the rest of the paragraph with this:

"returns early. The parent will wait pipe message from child which will 
never be sent by the child and the parent cannot proceeed to unmount 
resctrlfs."

> process will not be killed and also resctrlfs is not unmounted. Also if
> an error occurs when child process runs cat_val() or check_results(),
> the child process is returned, but the parent process will wait pipe
> message from child.
>
> Synchronize the exits between the parent and child. An error occurs
> whether in parents process or child process, the parents process
> always kills child process and runs umount_resctrlfs(), and the
> child process always waits to be killed by the parent process.
> 
> Signed-off-by: Shaopeng Tan <tan.shaopeng@xxxxxxxxxxxxxx>
> ---
>  tools/testing/selftests/resctrl/cat_test.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
> index 6a8306b0a109..477b62dac546 100644
> --- a/tools/testing/selftests/resctrl/cat_test.c
> +++ b/tools/testing/selftests/resctrl/cat_test.c
> @@ -186,23 +186,20 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
>  	remove(param.filename);
>  
>  	ret = cat_val(&param);
> -	if (ret)
> -		return ret;
> -
> -	ret = check_results(&param);
> -	if (ret)
> -		return ret;
> +	if (ret == 0)
> +		ret = check_results(&param);
>  
>  	if (bm_pid == 0) {
>  		/* Tell parent that child is ready */
>  		close(pipefd[0]);
>  		pipe_message = 1;
>  		if (write(pipefd[1], &pipe_message, sizeof(pipe_message)) <
> -		    sizeof(pipe_message)) {
> -			close(pipefd[1]);
> +		    sizeof(pipe_message))
> +			/*
> +			 * Just print the error message.
> +			 * Let while(1) run and wait for itself to be killed.
> +			 */
>  			perror("# failed signaling parent process");
> -			return errno;
> -		}
>  
>  		close(pipefd[1]);
>  		while (1)
> @@ -226,5 +223,5 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
>  	if (bm_pid)
>  		umount_resctrlfs();
>  
> -	return 0;
> +	return ret;
>  }
> 

The code change looks good.

-- 
 i.