[Intel-gfx] [PATCH i-g-t v2] i915/pm_rps: install SIGTERM handler for load_helper process

[Liu, Chuansheng]

> -----Original Message-----
> From: Chris Wilson <chris at chris-wilson.co.uk>
> Sent: Thursday, November 21, 2019 3:47 PM
> To: Liu, Chuansheng <chuansheng.liu at intel.com>;
> igt-dev at lists.freedesktop.org
> Cc: intel-gfx at lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH i-g-t v2] i915/pm_rps: install SIGTERM handler
> for load_helper process
> 
> Quoting Liu, Chuansheng (2019-11-21 01:34:24)
> > Thanks for reviewing the patch, please see below comments.
> >
> > > > So here we install the proper handler for signal SIGTERM in the
> > > > similar way. Without this patch, the GT may keep busy after
> > > > running this subtest. Enabling rps should be tracked on the
> > > > other side.
> > > >
> > > > Signed-off-by: Chuansheng Liu <chuansheng.liu at intel.com>
> > > > ---
> > > >  tests/i915/i915_pm_rps.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/tests/i915/i915_pm_rps.c b/tests/i915/i915_pm_rps.c
> > > > index ef627c0b..8c71c1a1 100644
> > > > --- a/tests/i915/i915_pm_rps.c
> > > > +++ b/tests/i915/i915_pm_rps.c
> > > > @@ -252,6 +252,7 @@ static void load_helper_run(enum load load)
> > > >
> > > >                 signal(SIGUSR1, load_helper_signal_handler);
> > > >                 signal(SIGUSR2, load_helper_signal_handler);
> > > > +               signal(SIGTERM, load_helper_signal_handler);
> > >
> > > I don't see any behaviour changes to igt to cause it to send SIGTERM on
> > > exit_subtest.
> >
> > Yes, exit_subtest() will not send SIGTERM out. But when main process calls
> > igt_exit() to exit, it hits the below assertion, then goes to fatal_sig_handler()
> with SIGABORT.
> > (i915_pm_rps:1680) igt_core-CRITICAL: Exiting with status code 98
> > i915_pm_rps: ../lib/igt_core.c:1775: igt_exit: Assertion `waitpid(-1, &tmp,
> WNOHANG) == -1 && errno == ECHILD' failed.
> > Received signal SIGABRT.
> 
> Ok, but that's not a huge concern, since we are already in an error state.
> My concern is for fixing whatever got us into that state.
Agree. In this case, we need to enable rps completely. Here I would like this quick
patch to unblock the following test cases.

Without this quick fix, it can mislead guys to catch the real root cause:)
Would you mind to get this patch merged at first?

> 
> > In fatal_sig_handler(), the installed exit handler fork_helper_exit_handler()
> > will send out the SIGTERM to all children process.
> >
> > >
> > > But you might as well just s/SIGUSR2/SIGTERM/ for clearer and common
> > > intentions.
> > Don't get your real point, SIGUSR1 is for actively stopping load_helper,
> SIGUSR2 is for
> > switching high and low load, the SIGTERM is for passively exiting.
> 
> I think the design of having a persistent helper process that leaks
> between subtests is broken. Then using three signals for essentially only
> 2 commands is aesthetically unpleasing.
Yes, to be honest, the main process should not receive SIGABRT according
to the initial code intention. Since the children processes should be cleaned
up, no matter it is load_helper or other created children process.