Re: [igt-dev] [PATCH i-g-t] lib: Don't leak children in igt_waitchildren_timeout

Daniel Vetter
2019-02-13T10:20:13Z

On Tue, Feb 12, 2019 at 04:08:59PM +0000, Chris Wilson wrote:
> Quoting Daniel Vetter (2019-02-12 16:02:12)
> > Instead of cleaning up the mess in igt_exit make sure we don't even
> > let it out of the container. See also
> >
> > commit 754876378d6c9b2775e8c07b4d16f9878c55949f
> > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > Date: Fri Feb 26 22:11:10 2016 +0000
> >
> > igt/gem_sync: Enforce a timeout of 20s
> >
> > which added this helper.
> >
> > To make sure that everyone follows the rules, add an assert.
> >
> > We're keeping the cleanup code as a failsafe, and because it speeds
> > up the testcase I'm following up with.
> >
> > v2: Chris pointed out that my original patch did nothing. Which I
> > didn't catch because my testcase was also broken. Unfortunately this
> > means we need to open code part of the waiting.
> >
> > v3: The 2nd __igt_waitchildren() isn't necessary, __igt_waitchildren
> > recovers from EINTR already and keeps waiting (Chris Wilson).
> >
> > v4: Change the timeout signal vs waitchildren logic to be race-free
> > (Chris). This changes the exit code for a timeout from
> > IGT_EXIT_FAILURE to SIGKILL + 128.
> >
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> > lib/igt_core.c | 30 +++++++++++++++++++++++++-----
> > 1 file changed, 25 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > index cbbe79f88070..57f757421309 100644
> > --- a/lib/igt_core.c
> > +++ b/lib/igt_core.c
> > @@ -1525,6 +1525,7 @@ void igt_exit(void)
> >
> > for (int c = 0; c < num_test_children; c++)
> > kill(test_children[c], SIGKILL);
> > + assert(!num_test_children);
> >
> > if (!test_with_subtests) {
> > struct timespec now;
> > @@ -1832,20 +1833,39 @@ void igt_waitchildren(void)
> > igt_fail(err);
> > }
> >
> > +static void igt_alarm_killchildren(int signal)
> > +{
> > + igt_info("Timed out waiting for children\n");
> > +
> > + for (int c = 0; c < num_test_children; c++)
> > + kill(test_children[c], SIGKILL);
> > +}
> > +
> > /**
> > * igt_waitchildren_timeout:
> > * @seconds: timeout in seconds to wait
> > * @reason: debug string explaining what timedout
> > *
> > - * Wait for all children forked with igt_fork, for a maximum of @seconds.
> > - *
> > - * Wraps igt_waitchildren() and igt_set_timeout()
> > + * Wait for all children forked with igt_fork, for a maximum of @seconds. If the
> > + * timeout expires, kills all children and cleans them up.
>
> ...before reporting the fail with igt_fail
>
> Or whatever is the common language for hitting igt_fail(). As it stands,
> it sounds like it should gracefully clean up and return control on
> timeout.

Yeah, clarified.
>
> > */
> > void igt_waitchildren_timeout(int seconds, const char *reason)
> > {
> > - igt_set_timeout(seconds, reason);
> > - igt_waitchildren();
> > + struct sigaction sa;
> > + int ret;
> > +
> > + sa.sa_handler = igt_alarm_killchildren;
> > + sigemptyset(&sa.sa_mask);
> > + sa.sa_flags = 0;
>
> struct sigaction sa = { .sa_handler = igt_alarm_killchildren };
> ?

In userspace I'm much less aggressive with assume clearing to 0 is the
right default, unlike kernel and kzalloc. I guess sigemptyset isn't
anything but clearing anywhere we even remotely care about, but posix is
fun. It's also open-coded in the other places, so I think I'll leave it.
>
> > +
> > + sigaction(SIGALRM, &sa, NULL);
> > +
> > + alarm(seconds);
> > +
> > + ret = __igt_waitchildren();
> > igt_reset_timeout();
> > + if (ret)
> > + igt_fail(ret);
>
> Lgtm,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks for reviewing, will respin the entire series.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation