Date:   Tue, 7 Feb 2023 10:50:23 +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: <TYAPR01MB6330A24D0B7B52567453FF388BDB9@TYAPR01MB6330.jpnprd01.prod.outlook.com>
Message-ID: <406496b7-877b-d9e7-10e-d7b31da51add@linux.intel.com>
References: <20230131054655.396270-1-tan.shaopeng@jp.fujitsu.com> <20230131054655.396270-5-tan.shaopeng@jp.fujitsu.com> <a9ab65a6-f750-7fd9-99ba-1cbd15427d2c@linux.intel.com> <TYAPR01MB6330A24D0B7B52567453FF388BDB9@TYAPR01MB6330.jpnprd01.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Tue, 7 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>
> > > ---

> > >  	ret = cat_val(&param);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > > -	ret = check_results(&param);
> > > -	if (ret)
> > > -		return ret;
> > > +	if (ret == 0)
> > > +		ret = check_results(&param);
> > 
> > It would be take this program flow fix out of the signal handler change into a
> > separate change.
> 
> Do you mean this fix should be separated into two patches?

Yes.

Currently, I see your patch doing (mainly) two things:
1) cleaning up the messy signal handler logic
2) fixing the early return in case of error from cat_val() or 
   check_results()

Both are good changes and both are needed to fully fix things. But (IMHO) 
those are indepedent enough that it would warrant to split this change 
into two.

-- 
 i.

> To make the child process wait to be killed by parent process
> in any case(an error occurred or a signal was received),
> I fixed it like this.
> 
> This fix was discussed here.
> https://lore.kernel.org/lkml/2ab9ca20-c757-7dd8-b770-2b84d171cbfb@intel.com/