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

[Chris Wilson]

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.

> 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.
-Chris