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

[Chris Wilson]

Quoting Daniel Vetter (2019-02-12 12:04:29)
> 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.
> 
> v3: The 2nd __igt_waitchildren() isn't necessary, __igt_waitchildren
> recovers from EINTR already and keeps waiting (Chris Wilson).
> 
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> ---
>  lib/igt_core.c | 38 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index cbbe79f88070..fa73712ced91 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,47 @@ 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");
> +
> +       igt_killchidren_timed_out = true;
> +
> +       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.
>   */
>  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();

Experience says to cancel the alarm as soon as you no longer need it.

> +       if (igt_killchidren_timed_out)
> +               igt_fail(IGT_EXIT_FAILURE);

Something like

ret = __igt_waitchildren();
igt_reset_timeout();
if (igt_killchildren_timed_out && !ret) // this should be impossible!
	ret = IGT_EXIT_FAILURE;
if (ret)
	igt_fail(ret);

Just to have only one path to the igt_fail. I'm not even sure about
needing the igt_killchildren_timed_out; if no child was killed then we
didn't exactly timeout :)
-Chris