Date:   Wed, 8 Feb 2023 09:54:19 +0200 (EET)
From:   =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
To:     "Shaopeng Tan (Fujitsu)" <tan.shaopeng@fujitsu.com>
cc:     Fenghua Yu <fenghua.yu@intel.com>,
        Reinette Chatre <reinette.chatre@intel.com>,
        Shuah Khan <shuah@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        "linux-kselftest@vger.kernel.org" <linux-kselftest@vger.kernel.org>
Subject: RE: [PATCH v6 4/5] selftests/resctrl: Cleanup properly when an error
 occurs in CAT test
In-Reply-To: <OSZPR01MB6328AE8F77449F7D6C7DE8958BD89@OSZPR01MB6328.jpnprd01.prod.outlook.com>
Message-ID: <c3acfc41-44f8-263e-379e-f8825194a6e@linux.intel.com>
References: <20230131054655.396270-1-tan.shaopeng@jp.fujitsu.com> <20230131054655.396270-5-tan.shaopeng@jp.fujitsu.com> <83e1de31-b448-1a51-ba39-faec794694f@linux.intel.com> <OSZPR01MB6328AE8F77449F7D6C7DE8958BD89@OSZPR01MB6328.jpnprd01.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Wed, 8 Feb 2023, Shaopeng Tan (Fujitsu) wrote:
> > On Tue, 31 Jan 2023, Shaopeng Tan wrote:
> > 
> > > After creating a child process with fork() in CAT test, if an error
> > > occurs or a signal such as SIGINT is received, the parent process will
> > > be terminated immediately, and therefor the child process will not be
> > > killed and also resctrlfs is not unmounted.
> > >
> > > There is a signal handler registered in CMT/MBM/MBA tests, which kills
> > > child process, unmount resctrlfs, cleanups result files, etc., if a
> > > signal such as SIGINT is received.
> > >
> > > Commonize the signal handler registered for CMT/MBM/MBA tests and
> > > reuse it in CAT too.
> > >
> > > To reuse the signal handler, make the child process in CAT wait to be
> > > killed by parent process in any case (an error occurred or a signal
> > > was received), and when killing child process use global bm_pid
> > > instead of local bm_pid.
> > >
> > > Also, since the MBA/MBA/CMT/CAT are run in order, unregister the
> > > signal handler at the end of each test so that the signal handler
> > > cannot be inherited by other tests.
> > >
> > > Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> > > ---
> > 
> > >  	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");
> > 
> > If the write error is ignored here, won't it just lead to parent hanging forever
> > waiting for the child to send the message through the pipe which will never
> > come?
> 
> If the write error is ignored here, the pipe will be closed by "close(pipefd[1]);" and child process will wait to be killed by "while(1)".
> ---
> -			return errno;
> -		}
> 
>  		close(pipefd[1]);
>  		while (1)
> ---
> 
> If all file descriptors referring to the write end of a pipe have been closed, 
> then an attempt to read(2) from the pipe will see end-of-file (read(2) will return 0).
> Then, "perror("# failed reading from child process");" occurs.
> ---
>         } else {
>                 /* Parent waits for child to be ready. */
>                 close(pipefd[1]);
>                 pipe_message = 0;
>                 while (pipe_message != 1) {
>                         if (read(pipefd[0], &pipe_message,
>                                  sizeof(pipe_message)) < sizeof(pipe_message)) {
>                                 perror("# failed reading from child process");
>                                 break;
>                         }
>                 }
>                 close(pipefd[0]);
>                 kill(bm_pid, SIGKILL);
>                 signal_handler_unregister();
>         }

Ah, indeed read() will pick up the close event. So your code seem fine 
after all.

-- 
 i.