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

[Chris Wilson]

Quoting Daniel Vetter (2019-02-11 18:02:02)
> 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 at 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.
> 
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> ---
>  lib/igt_core.c | 39 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index cbbe79f88070..3053697da58c 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,48 @@ void igt_waitchildren(void)
>                 igt_fail(err);
>  }
>  
> +static bool igt_killchidren_timed_out;
> +
> +static void igt_alarm_killchildren(int signal)
> +{
> +       igt_info("Timed out waiting for children\n");

We used to print the caller supplied reason. Although that appears to
have never been used, so might as well drop it from the API.

> +
> +       igt_killchidren_timed_out = true;
> +
> +       for (int c = 0; c < num_test_children; c++)
> +               kill(test_children[c], SIGKILL);

Ok, kill() is signal-safe.

> +}
> +
>  /**
>   * 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.
>   */
>  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;
> +
> +       igt_killchidren_timed_out = false;
> +
> +       sigaction(SIGALRM, &sa, NULL);
> +
> +       alarm(seconds);
> +
> +       ret = __igt_waitchildren();
> +       if (!igt_killchidren_timed_out && ret)
> +               igt_fail(ret);
>         igt_reset_timeout();
> +       __igt_waitchildren();
> +       if (igt_killchidren_timed_out)
> +               igt_fail(IGT_EXIT_FAILURE);

Double __igt_waitchildren()? My reading of __igt_waitchildren() is that
it will continue on after the alarm() wakes wait() up with SIGINT and
repeat the wait() until all children die. And now those children will
die, rather than the parent as before.
-Chris