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

Chris Wilson
2019-02-12T16:09:23Z

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.

> */
> 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 };
?

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