[igt-dev] [PATCH i-g-t 1/9] lib/tests: Check that igt_assert forwards correctly through igt_fork

[igt-dev] [PATCH i-g-t 1/9] lib/tests: Check that igt_assert forwards correctly through igt_fork

[Daniel Vetter]

Note that without the igt_waitchildren nothing at all gets forwarded,
maybe we should check for left-behind children somewhere on subtest
exit.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 lib/tests/igt_fork.c  | 87 +++++++++++++++++++++++++++++++++++++++++++
 lib/tests/meson.build |  1 +
 2 files changed, 88 insertions(+)
 create mode 100644 lib/tests/igt_fork.c

diff --git a/lib/tests/igt_fork.c b/lib/tests/igt_fork.c
new file mode 100644
index 000000000000..d495c7815e06
--- /dev/null
+++ b/lib/tests/igt_fork.c
@@ -0,0 +1,87 @@
+/*
+ * Copyright © 2019 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+
+#include <assert.h>
+#include <errno.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+
+#include "igt_core.h"
+
+/*
+ * We need to hide assert from the cocci igt test refactor spatch.
+ *
+ * IMPORTANT: Test infrastructure tests are the only valid places where using
+ * assert is allowed.
+ */
+#define internal_assert assert
+
+char test[] = "test";
+char *argv_run[] = { test };
+
+static void igt_fork_vs_assert(void)
+{
+	igt_fork(i, 1) {
+		igt_assert(0);
+	}
+
+	igt_waitchildren();
+}
+
+static int do_fork(void (*test_to_run)(void))
+{
+	int pid, status;
+	int argc;
+
+	switch (pid = fork()) {
+	case -1:
+		internal_assert(0);
+	case 0:
+		argc = 1;
+		igt_simple_init(argc, argv_run);
+		test_to_run();
+		igt_exit();
+	default:
+		while (waitpid(pid, &status, 0) == -1 &&
+		       errno == EINTR)
+			;
+
+		if(WIFSIGNALED(status))
+			return WTERMSIG(status) + 128;
+
+		return WEXITSTATUS(status);
+	}
+}
+
+
+int main(int argc, char **argv)
+{
+	int ret;
+
+	ret = do_fork(igt_fork_vs_assert);
+	internal_assert(ret == IGT_EXIT_FAILURE);
+}
diff --git a/lib/tests/meson.build b/lib/tests/meson.build
index 55ab2b3d2dff..665ad4a0fbcc 100644
--- a/lib/tests/meson.build
+++ b/lib/tests/meson.build
@@ -1,5 +1,6 @@
 lib_tests = [
 	'igt_fork_helper',
+	'igt_fork',
 	'igt_list_only',
 	'igt_simulation',
 	'igt_stats',
-- 
2.20.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 2/9] lib/tests: make sure igt_skip in igt_fork is forbidden

[Daniel Vetter]

Another corner case to check.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 lib/tests/igt_fork.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/lib/tests/igt_fork.c b/lib/tests/igt_fork.c
index d495c7815e06..5ff2956dd0ab 100644
--- a/lib/tests/igt_fork.c
+++ b/lib/tests/igt_fork.c
@@ -43,6 +43,15 @@
 char test[] = "test";
 char *argv_run[] = { test };
 
+static void igt_fork_vs_skip(void)
+{
+	igt_fork(i, 1) {
+		igt_skip("skipping");
+	}
+
+	igt_waitchildren();
+}
+
 static void igt_fork_vs_assert(void)
 {
 	igt_fork(i, 1) {
@@ -82,6 +91,11 @@ int main(int argc, char **argv)
 {
 	int ret;
 
+	/* check that igt_assert is forwarded */
 	ret = do_fork(igt_fork_vs_assert);
 	internal_assert(ret == IGT_EXIT_FAILURE);
+
+	/* check that igt_skip within a fork blows up */
+	ret = do_fork(igt_fork_vs_skip);
+	internal_assert(ret == SIGABRT + 128);
 }
-- 
2.20.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Daniel Vetter]

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.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@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");
+
+	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();
+	__igt_waitchildren();
+	if (igt_killchidren_timed_out)
+		igt_fail(IGT_EXIT_FAILURE);
 }
 
 /* exit handler code */
-- 
2.20.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 4/9] tests/gem_exec_reuse: Don't leak the hang detector

[Daniel Vetter]

My new "are there any child processes left?" check in igt_exit catched
this one.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 tests/i915/gem_exec_reuse.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/i915/gem_exec_reuse.c b/tests/i915/gem_exec_reuse.c
index df220be7bab8..559047188fbd 100644
--- a/tests/i915/gem_exec_reuse.c
+++ b/tests/i915/gem_exec_reuse.c
@@ -213,4 +213,7 @@ igt_main
 		for (n = 0; n < ncontexts; n++)
 			gem_context_destroy(no.fd, contexts[n]);
 	}
+
+	igt_fixture
+		igt_stop_hang_detector();
 }
-- 
2.20.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 5/9] tests/i915_missed_irq: Don't leave the hang detector hanging

[Daniel Vetter]

Spotted by my new "are there any child processes left?" check in
igt_exit - we need to put all the igt_require before we start any real
test logic.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 tests/i915/missed_irq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/i915/missed_irq.c b/tests/i915/missed_irq.c
index cade3f371401..302da0e8d071 100644
--- a/tests/i915/missed_irq.c
+++ b/tests/i915/missed_irq.c
@@ -108,13 +108,13 @@ igt_simple_main
 	igt_require_gem(device);
 	igt_skip_on(gem_has_guc_submission(device)); /* irq forced for guc */
 	gem_require_mmap_wc(device);
-	igt_fork_hang_detector(device);
-
 	debugfs = igt_debugfs_dir(device);
 
 	expect_rings = engine_mask(debugfs);
 	igt_require(expect_rings);
 
+	igt_fork_hang_detector(device);
+
 	igt_debug("Clearing rings %x\n", expect_rings);
 	intel_detect_and_clear_missed_interrupts(device);
 	for (e = intel_execution_engines; e->name; e++) {
-- 
2.20.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 6/9] lib/tests: make sure we catch igt_fork leaks

[Daniel Vetter]

Tests have to call igt_waitchildren(_timeout) before they finish
(either with success or an igt_fail/assert/skip/whatever).

Make sure we catch this.

v2: Another testcase to make sure igt_waitchildren_timeout cleans up.

v3: Actually test that igt_waitchildren_timeout kills children - we
want a proper IGT_EXIT_FAILURE, not some assert.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 lib/tests/igt_fork.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/lib/tests/igt_fork.c b/lib/tests/igt_fork.c
index 5ff2956dd0ab..e5b0ab016b23 100644
--- a/lib/tests/igt_fork.c
+++ b/lib/tests/igt_fork.c
@@ -61,6 +61,22 @@ static void igt_fork_vs_assert(void)
 	igt_waitchildren();
 }
 
+static void igt_fork_leak(void)
+{
+	igt_fork(i, 1) {
+		sleep(10);
+	}
+}
+
+static void igt_fork_timeout_leak(void)
+{
+	igt_fork(i, 1) {
+		sleep(10);
+	}
+
+	igt_waitchildren_timeout(1, "library test");
+}
+
 static int do_fork(void (*test_to_run)(void))
 {
 	int pid, status;
@@ -98,4 +114,12 @@ int main(int argc, char **argv)
 	/* check that igt_skip within a fork blows up */
 	ret = do_fork(igt_fork_vs_skip);
 	internal_assert(ret == SIGABRT + 128);
+
+	/* check that failure to clean up fails */
+	ret = do_fork(igt_fork_leak);
+	internal_assert(ret == SIGABRT + 128);
+
+	/* check that igt_waitchildren_timeout cleans up*/
+	ret = do_fork(igt_fork_timeout_leak);
+	internal_assert(ret == IGT_EXIT_FAILURE);
 }
-- 
2.20.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 7/9] lib: Make sure we leak no child processes

[Daniel Vetter]

There's a lot more ways to leak children than igt_fork, some even
handrolled. So check for that. Also have a nice littel testcase for
that too.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 lib/igt_core.c       |  4 ++++
 lib/tests/igt_fork.c | 18 ++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index 3053697da58c..f0e186244bc3 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -1492,6 +1492,8 @@ void __igt_fail_assert(const char *domain, const char *file, const int line,
  */
 void igt_exit(void)
 {
+	int tmp;
+
 	igt_exit_called = true;
 
 	if (igt_key_file)
@@ -1527,6 +1529,8 @@ void igt_exit(void)
 		kill(test_children[c], SIGKILL);
 	assert(!num_test_children);
 
+	assert(wait(&tmp) == -1 && errno == ECHILD);
+
 	if (!test_with_subtests) {
 		struct timespec now;
 		const char *result;
diff --git a/lib/tests/igt_fork.c b/lib/tests/igt_fork.c
index e5b0ab016b23..b486d07000bb 100644
--- a/lib/tests/igt_fork.c
+++ b/lib/tests/igt_fork.c
@@ -68,6 +68,20 @@ static void igt_fork_leak(void)
 	}
 }
 
+static void plain_fork_leak(void)
+{
+	int pid;
+
+	switch (pid = fork()) {
+	case -1:
+		internal_assert(0);
+	case 0:
+		sleep(1);
+	default:
+		exit(0);
+	}
+}
+
 static void igt_fork_timeout_leak(void)
 {
 	igt_fork(i, 1) {
@@ -122,4 +136,8 @@ int main(int argc, char **argv)
 	/* check that igt_waitchildren_timeout cleans up*/
 	ret = do_fork(igt_fork_timeout_leak);
 	internal_assert(ret == IGT_EXIT_FAILURE);
+
+	/* check that any other process leaks are caught*/
+	ret = do_fork(plain_fork_leak);
+	internal_assert(ret == SIGABRT + 128);
 }
-- 
2.20.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 8/9] lib: Drop igt_child_done

[Daniel Vetter]

Added in

commit 054eb1abecd1cce2e4ee0516f3ff8a67a35dca22
Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Date:   Thu Mar 30 14:32:29 2017 +0100

    benchmarks/gem_wsim: Command submission workload simulator

but since then the only user was lost.

Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 lib/igt_core.c | 26 --------------------------
 lib/igt_core.h |  1 -
 2 files changed, 27 deletions(-)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index f0e186244bc3..0732e9996819 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -1742,32 +1742,6 @@ bool __igt_fork(void)
 
 }
 
-/**
- * igt_child_done:
- *
- * Lets the IGT core know that one of the children has exited.
- */
-void igt_child_done(pid_t pid)
-{
-	int i = 0;
-	int found = -1;
-
-	igt_assert(num_test_children > 1);
-
-	for (i = 0; i < num_test_children; i++) {
-		if (pid == test_children[i]) {
-			found = i;
-			break;
-		}
-	}
-
-	igt_assert(found >= 0);
-
-	num_test_children--;
-	for (i = found; i < num_test_children; i++)
-		test_children[i] = test_children[i + 1];
-}
-
 int __igt_waitchildren(void)
 {
 	int err = 0;
diff --git a/lib/igt_core.h b/lib/igt_core.h
index 0d02c90beec1..46bc935a428c 100644
--- a/lib/igt_core.h
+++ b/lib/igt_core.h
@@ -775,7 +775,6 @@ bool __igt_fork(void);
 #define igt_fork(child, num_children) \
 	for (int child = 0; child < (num_children); child++) \
 		for (; __igt_fork(); exit(0))
-void igt_child_done(pid_t pid);
 int __igt_waitchildren(void);
 void igt_waitchildren(void);
 void igt_waitchildren_timeout(int seconds, const char *reason);
-- 
2.20.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 9/9] lib: Drop IGT_EXIT_TIMEOUT

[Daniel Vetter]

We use the timeout status for when the runner had to kill a testcase,
which indicates a more sever issue than an operation failing that we
expected to complete within seconds.

Since it's unused, drop it.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 docs/reference/igt-gpu-tools/igt_test_programs.xml | 5 -----
 lib/igt_core.c                                     | 8 +-------
 lib/igt_core.h                                     | 7 -------
 runner/resultgen.c                                 | 3 ---
 4 files changed, 1 insertion(+), 22 deletions(-)

diff --git a/docs/reference/igt-gpu-tools/igt_test_programs.xml b/docs/reference/igt-gpu-tools/igt_test_programs.xml
index 2487da79588b..b64ba474a5f7 100644
--- a/docs/reference/igt-gpu-tools/igt_test_programs.xml
+++ b/docs/reference/igt-gpu-tools/igt_test_programs.xml
@@ -81,11 +81,6 @@
               <entry>77</entry>
               <entry>The test was skipped</entry>
             </row>
-            <row>
-              <entry>#IGT_EXIT_TIMEOUT</entry>
-              <entry>78</entry>
-              <entry>The test took longer than expected and was stopped</entry>
-            </row>
             <row>
               <entry>#IGT_EXIT_INVALID</entry>
               <entry>79</entry>
diff --git a/lib/igt_core.c b/lib/igt_core.c
index 0732e9996819..06f8947e3dfa 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -1135,10 +1135,7 @@ void igt_fail(int exitcode)
 	_igt_log_buffer_dump();
 
 	if (in_subtest) {
-		if (exitcode == IGT_EXIT_TIMEOUT)
-			exit_subtest("TIMEOUT");
-		else
-			exit_subtest("FAIL");
+		exit_subtest("FAIL");
 	} else {
 		assert(igt_can_fail());
 
@@ -1541,9 +1538,6 @@ void igt_exit(void)
 			case IGT_EXIT_SUCCESS:
 				result = "SUCCESS";
 				break;
-			case IGT_EXIT_TIMEOUT:
-				result = "TIMEOUT";
-				break;
 			case IGT_EXIT_SKIP:
 				result = "SKIP";
 				break;
diff --git a/lib/igt_core.h b/lib/igt_core.h
index 46bc935a428c..47ffd9e77308 100644
--- a/lib/igt_core.h
+++ b/lib/igt_core.h
@@ -93,13 +93,6 @@ extern char *igt_frame_dump_path;
  */
 #define IGT_TEST_DESCRIPTION(str) const char* __igt_test_description = str
 
-/**
- * IGT_EXIT_TIMEOUT:
- *
- * Exit status indicating a timeout occurred.
- */
-#define IGT_EXIT_TIMEOUT 78
-
 /**
  * IGT_EXIT_SKIP:
  *
diff --git a/runner/resultgen.c b/runner/resultgen.c
index be884955e86c..32b59d59aad2 100644
--- a/runner/resultgen.c
+++ b/runner/resultgen.c
@@ -18,7 +18,6 @@
 
 #define INCOMPLETE_EXITCODE -1
 
-_Static_assert(INCOMPLETE_EXITCODE != IGT_EXIT_TIMEOUT, "exit code clash");
 _Static_assert(INCOMPLETE_EXITCODE != IGT_EXIT_SKIP, "exit code clash");
 _Static_assert(INCOMPLETE_EXITCODE != IGT_EXIT_SUCCESS, "exit code clash");
 _Static_assert(INCOMPLETE_EXITCODE != IGT_EXIT_INVALID, "exit code clash");
@@ -731,8 +730,6 @@ static bool fill_from_dmesg(int fd,
 static const char *result_from_exitcode(int exitcode)
 {
 	switch (exitcode) {
-	case IGT_EXIT_TIMEOUT:
-		return "timeout";
 	case IGT_EXIT_SKIP:
 		return "skip";
 	case IGT_EXIT_SUCCESS:
-- 
2.20.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 4/9] tests/gem_exec_reuse: Don't leak the hang detector

[Chris Wilson]

Quoting Daniel Vetter (2019-02-11 18:02:03)
> My new "are there any child processes left?" check in igt_exit catched
> this one.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  tests/i915/gem_exec_reuse.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tests/i915/gem_exec_reuse.c b/tests/i915/gem_exec_reuse.c
> index df220be7bab8..559047188fbd 100644
> --- a/tests/i915/gem_exec_reuse.c
> +++ b/tests/i915/gem_exec_reuse.c
> @@ -213,4 +213,7 @@ igt_main
>                 for (n = 0; n < ncontexts; n++)
>                         gem_context_destroy(no.fd, contexts[n]);
>         }
> +
> +       igt_fixture
> +               igt_stop_hang_detector();

Better than my attempt which didn't remember it took void.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 5/9] tests/i915_missed_irq: Don't leave the hang detector hanging

[Chris Wilson]

Quoting Daniel Vetter (2019-02-11 18:02:04)
> Spotted by my new "are there any child processes left?" check in
> igt_exit - we need to put all the igt_require before we start any real
> test logic.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  tests/i915/missed_irq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/i915/missed_irq.c b/tests/i915/missed_irq.c
> index cade3f371401..302da0e8d071 100644
> --- a/tests/i915/missed_irq.c
> +++ b/tests/i915/missed_irq.c
> @@ -108,13 +108,13 @@ igt_simple_main
>         igt_require_gem(device);
>         igt_skip_on(gem_has_guc_submission(device)); /* irq forced for guc */
>         gem_require_mmap_wc(device);
> -       igt_fork_hang_detector(device);
> -
>         debugfs = igt_debugfs_dir(device);
>  
>         expect_rings = engine_mask(debugfs);
>         igt_require(expect_rings);
>  
> +       igt_fork_hang_detector(device);

We don't really need hang detector here either, I think I might have
been a bit to quick to copy it here. Test is defunct, but
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
nevertheless.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/9] lib/tests: Check that igt_assert forwards correctly through igt_fork

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t,1/9] lib/tests: Check that igt_assert forwards correctly through igt_fork
URL   : https://patchwork.freedesktop.org/series/56507/
State : success

== Summary ==

CI Bug Log - changes from IGT_4817 -> IGTPW_2376
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/56507/revisions/1/mbox/

Known issues
------------

  Here are the changes found in IGTPW_2376 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_module_load@reload:
    - fi-blb-e6850:       PASS -> INCOMPLETE [fdo#107718]

  * igt@i915_selftest@live_execlists:
    - fi-apl-guc:         PASS -> INCOMPLETE [fdo#103927]

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-kbl-7567u:       PASS -> WARN [fdo#109380]

  * igt@kms_flip@basic-flip-vs-dpms:
    - fi-skl-6700hq:      PASS -> DMESG-WARN [fdo#105998]

  
#### Possible fixes ####

  * igt@i915_selftest@live_hangcheck:
    - fi-kbl-7560u:       INCOMPLETE [fdo#108044] / [fdo#108744] -> PASS

  * igt@kms_busy@basic-flip-a:
    - fi-kbl-7567u:       {SKIP} [fdo#109271] / [fdo#109278] -> PASS +2
    - fi-gdg-551:         FAIL [fdo#103182] -> PASS

  * igt@kms_frontbuffer_tracking@basic:
    - fi-byt-clapper:     FAIL [fdo#103167] -> PASS

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b:
    - fi-byt-clapper:     FAIL [fdo#107362] -> PASS

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - fi-byt-clapper:     FAIL [fdo#103191] / [fdo#107362] -> PASS +1

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103182]: https://bugs.freedesktop.org/show_bug.cgi?id=103182
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#105998]: https://bugs.freedesktop.org/show_bug.cgi?id=105998
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108044]: https://bugs.freedesktop.org/show_bug.cgi?id=108044
  [fdo#108622]: https://bugs.freedesktop.org/show_bug.cgi?id=108622
  [fdo#108744]: https://bugs.freedesktop.org/show_bug.cgi?id=108744
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109380]: https://bugs.freedesktop.org/show_bug.cgi?id=109380


Participating hosts (48 -> 41)
------------------------------

  Additional (1): fi-glk-j4005 
  Missing    (8): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-icl-y fi-bdw-samus 


Build changes
-------------

    * IGT: IGT_4817 -> IGTPW_2376

  CI_DRM_5589: 7a1e565f0c6d003c5990380c6e3da8719ac33eec @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2376: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2376/
  IGT_4817: 08cdb644686629dcf968c6cc00e054ed5f5aae6a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2376/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [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@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@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 7/9] lib: Make sure we leak no child processes

[Chris Wilson]

Quoting Daniel Vetter (2019-02-11 18:02:06)
> @@ -1527,6 +1529,8 @@ void igt_exit(void)
>                 kill(test_children[c], SIGKILL);
>         assert(!num_test_children);
>  
> +       assert(wait(&tmp) == -1 && errno == ECHILD);

Turning a previous failedsafe "bug" into an infinite wait.
assert(waitpid(-1, &tmp, WNOHANG) == -1 && errno == ECHILD);
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.IGT: success for series starting with [i-g-t,1/9] lib/tests: Check that igt_assert forwards correctly through igt_fork

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t,1/9] lib/tests: Check that igt_assert forwards correctly through igt_fork
URL   : https://patchwork.freedesktop.org/series/56507/
State : success

== Summary ==

CI Bug Log - changes from IGT_4817_full -> IGTPW_2376_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/56507/revisions/1/mbox/

Known issues
------------

  Here are the changes found in IGTPW_2376_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_eio@reset-stress:
    - shard-snb:          PASS -> FAIL [fdo#107799]

  * igt@gem_mmap_gtt@hang:
    - shard-kbl:          PASS -> INCOMPLETE [fdo#103665] / [fdo#109605 ]
    - shard-apl:          PASS -> INCOMPLETE [fdo#103927] / [fdo#109605 ]

  * igt@kms_busy@extended-modeset-hang-newfb-render-a:
    - shard-kbl:          PASS -> DMESG-WARN [fdo#107956]

  * igt@kms_busy@extended-modeset-hang-newfb-render-c:
    - shard-glk:          NOTRUN -> DMESG-WARN [fdo#107956]

  * igt@kms_ccs@pipe-b-crc-sprite-planes-basic:
    - shard-apl:          PASS -> FAIL [fdo#106510] / [fdo#108145]

  * igt@kms_color@pipe-c-ctm-max:
    - shard-apl:          PASS -> FAIL [fdo#108147]

  * igt@kms_cursor_crc@cursor-256x85-sliding:
    - shard-kbl:          PASS -> FAIL [fdo#103232] +1
    - shard-apl:          PASS -> FAIL [fdo#103232] +2
    - shard-glk:          NOTRUN -> FAIL [fdo#103232] +2

  * igt@kms_cursor_crc@cursor-alpha-opaque:
    - shard-kbl:          PASS -> FAIL [fdo#109350]
    - shard-apl:          PASS -> FAIL [fdo#109350]

  * igt@kms_flip@2x-flip-vs-expired-vblank:
    - shard-glk:          PASS -> FAIL [fdo#105363]

  * igt@kms_flip@2x-flip-vs-fences-interruptible:
    - shard-hsw:          NOTRUN -> INCOMPLETE [fdo#103540]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-cpu:
    - shard-apl:          PASS -> FAIL [fdo#103167] +1

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move:
    - shard-glk:          NOTRUN -> FAIL [fdo#103167] +1

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-mmap-wc:
    - shard-glk:          PASS -> FAIL [fdo#103167] +5

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-c-frame-sequence:
    - shard-kbl:          PASS -> DMESG-WARN [fdo#103313] / [fdo#105345]

  * igt@kms_plane_alpha_blend@pipe-c-alpha-basic:
    - shard-apl:          NOTRUN -> FAIL [fdo#108145]

  * igt@kms_plane_alpha_blend@pipe-c-alpha-transparant-fb:
    - shard-kbl:          NOTRUN -> FAIL [fdo#108145]

  * igt@kms_plane_multiple@atomic-pipe-a-tiling-none:
    - shard-apl:          PASS -> FAIL [fdo#103166]

  * igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
    - shard-glk:          NOTRUN -> FAIL [fdo#103166]

  * igt@kms_setmode@basic:
    - shard-apl:          PASS -> FAIL [fdo#99912]
    - shard-kbl:          PASS -> FAIL [fdo#99912]

  * igt@kms_universal_plane@universal-plane-pipe-c-functional:
    - shard-glk:          PASS -> FAIL [fdo#103166] +5

  * igt@perf_pmu@rc6-runtime-pm-long:
    - shard-glk:          PASS -> FAIL [fdo#105010]

  
#### Possible fixes ####

  * igt@kms_color@pipe-a-degamma:
    - shard-kbl:          FAIL [fdo#104782] / [fdo#108145] -> PASS

  * igt@kms_cursor_crc@cursor-128x128-dpms:
    - shard-kbl:          FAIL [fdo#103232] -> PASS

  * igt@kms_cursor_crc@cursor-128x128-suspend:
    - shard-apl:          FAIL [fdo#103191] / [fdo#103232] -> PASS +1

  * igt@kms_cursor_crc@cursor-256x256-random:
    - shard-glk:          FAIL [fdo#103232] -> PASS +3
    - shard-apl:          FAIL [fdo#103232] -> PASS +3

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-mmap-wc:
    - shard-glk:          FAIL [fdo#103167] -> PASS +1
    - shard-kbl:          FAIL [fdo#103167] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-wc:
    - shard-apl:          FAIL [fdo#103167] -> PASS +1

  * igt@kms_plane@pixel-format-pipe-b-planes-source-clamping:
    - shard-apl:          FAIL [fdo#108948] -> PASS
    - shard-glk:          FAIL [fdo#108948] -> PASS
    - shard-kbl:          FAIL [fdo#108948] -> PASS

  * igt@kms_plane@plane-position-covered-pipe-b-planes:
    - shard-glk:          FAIL [fdo#103166] -> PASS +2

  * igt@kms_plane@plane-position-covered-pipe-c-planes:
    - shard-apl:          FAIL [fdo#103166] -> PASS +3
    - shard-kbl:          FAIL [fdo#103166] -> PASS

  * igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend:
    - shard-kbl:          INCOMPLETE [fdo#103665] -> PASS

  * igt@pm_rc6_residency@rc6-accuracy:
    - shard-snb:          {SKIP} [fdo#109271] -> PASS

  * igt@tools_test@tools_test:
    - shard-apl:          INCOMPLETE [fdo#103927] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103313]: https://bugs.freedesktop.org/show_bug.cgi?id=103313
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104782]: https://bugs.freedesktop.org/show_bug.cgi?id=104782
  [fdo#105010]: https://bugs.freedesktop.org/show_bug.cgi?id=105010
  [fdo#105345]: https://bugs.freedesktop.org/show_bug.cgi?id=105345
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#106510]: https://bugs.freedesktop.org/show_bug.cgi?id=106510
  [fdo#107799]: https://bugs.freedesktop.org/show_bug.cgi?id=107799
  [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108147]: https://bugs.freedesktop.org/show_bug.cgi?id=108147
  [fdo#108948]: https://bugs.freedesktop.org/show_bug.cgi?id=108948
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109350]: https://bugs.freedesktop.org/show_bug.cgi?id=109350
  [fdo#109605 ]: https://bugs.freedesktop.org/show_bug.cgi?id=109605 
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (7 -> 5)
------------------------------

  Missing    (2): shard-skl shard-iclb 


Build changes
-------------

    * IGT: IGT_4817 -> IGTPW_2376

  CI_DRM_5589: 7a1e565f0c6d003c5990380c6e3da8719ac33eec @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2376: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2376/
  IGT_4817: 08cdb644686629dcf968c6cc00e054ed5f5aae6a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2376/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Daniel Vetter]

On Mon, Feb 11, 2019 at 09:03:12PM +0000, Chris Wilson wrote:
> 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@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@chris-wilson.co.uk>
> > Signed-off-by: Daniel Vetter <daniel.vetter@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.

igt_waitchildren_timeout before this patch wouldn't work if it did that.
The pid == wait(); if (pid == -1) continue; bails out to the next child in
case of interrupts. Hence the wait; kill; wait sequence here. I discovered
this through testcases :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t] lib: Make sure we leak no child processes

[Daniel Vetter]

There's a lot more ways to leak children than igt_fork, some even
handrolled. So check for that. Also have a nice littel testcase for
that too.

v2: Don't hang if there's a leaked child process (Chris). Has the
added benefit that my library unit test also gets faster!

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 lib/igt_core.c       |  4 ++++
 lib/tests/igt_fork.c | 18 ++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index 3053697da58c..d2cfc8e6da20 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -1492,6 +1492,8 @@ void __igt_fail_assert(const char *domain, const char *file, const int line,
  */
 void igt_exit(void)
 {
+	int tmp;
+
 	igt_exit_called = true;
 
 	if (igt_key_file)
@@ -1527,6 +1529,8 @@ void igt_exit(void)
 		kill(test_children[c], SIGKILL);
 	assert(!num_test_children);
 
+	assert(waitpid(-1, &tmp, WNOHANG) == -1 && errno == ECHILD);
+
 	if (!test_with_subtests) {
 		struct timespec now;
 		const char *result;
diff --git a/lib/tests/igt_fork.c b/lib/tests/igt_fork.c
index e5b0ab016b23..b486d07000bb 100644
--- a/lib/tests/igt_fork.c
+++ b/lib/tests/igt_fork.c
@@ -68,6 +68,20 @@ static void igt_fork_leak(void)
 	}
 }
 
+static void plain_fork_leak(void)
+{
+	int pid;
+
+	switch (pid = fork()) {
+	case -1:
+		internal_assert(0);
+	case 0:
+		sleep(1);
+	default:
+		exit(0);
+	}
+}
+
 static void igt_fork_timeout_leak(void)
 {
 	igt_fork(i, 1) {
@@ -122,4 +136,8 @@ int main(int argc, char **argv)
 	/* check that igt_waitchildren_timeout cleans up*/
 	ret = do_fork(igt_fork_timeout_leak);
 	internal_assert(ret == IGT_EXIT_FAILURE);
+
+	/* check that any other process leaks are caught*/
+	ret = do_fork(plain_fork_leak);
+	internal_assert(ret == SIGABRT + 128);
 }
-- 
2.20.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [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 22:38:25)
> On Mon, Feb 11, 2019 at 09:03:12PM +0000, Chris Wilson wrote:
> > 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@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@chris-wilson.co.uk>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@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.
> 
> igt_waitchildren_timeout before this patch wouldn't work if it did that.

Before this patch, the alarm handler did igt_fail -> exit(1) in the
parent.

> The pid == wait(); if (pid == -1) continue; bails out to the next child in
> case of interrupts. Hence the wait; kill; wait sequence here. I discovered
> this through testcases :-)

Now alarm -> wait() returning -1 + errno=EINTR, right? Then the
sighandler does killall -9, so on the next wait it sees that all the
children are dead.

__igt_waitchildren() even sets num_test_children = 0 on return, so the
second __igt_waitchildren() must do nothing. Or I am very confused.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Daniel Vetter]

On Mon, Feb 11, 2019 at 11:01:16PM +0000, Chris Wilson wrote:
> Quoting Daniel Vetter (2019-02-11 22:38:25)
> > On Mon, Feb 11, 2019 at 09:03:12PM +0000, Chris Wilson wrote:
> > > 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@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@chris-wilson.co.uk>
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@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.
> > 
> > igt_waitchildren_timeout before this patch wouldn't work if it did that.
> 
> Before this patch, the alarm handler did igt_fail -> exit(1) in the
> parent.
> 
> > The pid == wait(); if (pid == -1) continue; bails out to the next child in
> > case of interrupts. Hence the wait; kill; wait sequence here. I discovered
> > this through testcases :-)
> 
> Now alarm -> wait() returning -1 + errno=EINTR, right? Then the
> sighandler does killall -9, so on the next wait it sees that all the
> children are dead.
> 
> __igt_waitchildren() even sets num_test_children = 0 on return, so the
> second __igt_waitchildren() must do nothing. Or I am very confused.

Hm right I think I got myself totally confused with some older version of
this (which was broken), in combination with also broken testcases.

Looking at the code again it does indeed loop until it has them all, plus
there is even a loop to kill all the children if one failed. I'll see what
happens when I drop the 2nd waitchildren, and double-check my tests do
tests what I want to test.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Daniel Vetter]

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

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@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();
+	if (igt_killchidren_timed_out)
+		igt_fail(IGT_EXIT_FAILURE);
 }
 
 /* exit handler code */
-- 
2.20.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [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@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@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/9] lib/tests: Check that igt_assert forwards correctly through igt_fork (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t,1/9] lib/tests: Check that igt_assert forwards correctly through igt_fork (rev2)
URL   : https://patchwork.freedesktop.org/series/56507/
State : success

== Summary ==

CI Bug Log - changes from IGT_4818 -> IGTPW_2380
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/56507/revisions/2/mbox/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_2380:

### IGT changes ###

#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@i915_selftest@live_workarounds:
    - {fi-icl-u3}:        PASS -> INCOMPLETE
    - {fi-icl-u2}:        NOTRUN -> INCOMPLETE

  
Known issues
------------

  Here are the changes found in IGTPW_2380 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-skl-6700hq:      PASS -> FAIL [fdo#103375]

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - fi-blb-e6850:       PASS -> INCOMPLETE [fdo#107718]

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109309]: https://bugs.freedesktop.org/show_bug.cgi?id=109309
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#109316]: https://bugs.freedesktop.org/show_bug.cgi?id=109316


Participating hosts (43 -> 37)
------------------------------

  Additional (3): fi-skl-guc fi-icl-u2 fi-skl-6600u 
  Missing    (9): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-kbl-guc fi-ctg-p8600 fi-gdg-551 fi-icl-y fi-bdw-samus 


Build changes
-------------

    * IGT: IGT_4818 -> IGTPW_2380

  CI_DRM_5591: 1c5934e14f717ab09d832a7200113b02e3196e98 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2380: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2380/
  IGT_4818: ed32029e6a630d9322bd544b626eea38b4785e68 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2380/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Daniel Vetter]

On Tue, Feb 12, 2019 at 12:09:25PM +0000, Chris Wilson wrote:
> 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@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@chris-wilson.co.uk>
> > Signed-off-by: Daniel Vetter <daniel.vetter@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 :)

Hm right ordered this way we should either get a SIGKILL status in ret, or
it all worked out. I'll respin.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.IGT: success for series starting with [i-g-t,1/9] lib/tests: Check that igt_assert forwards correctly through igt_fork (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t,1/9] lib/tests: Check that igt_assert forwards correctly through igt_fork (rev2)
URL   : https://patchwork.freedesktop.org/series/56507/
State : success

== Summary ==

CI Bug Log - changes from IGT_4818_full -> IGTPW_2380_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/56507/revisions/2/mbox/

Known issues
------------

  Here are the changes found in IGTPW_2380_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-c:
    - shard-apl:          PASS -> DMESG-WARN [fdo#107956]

  * igt@kms_ccs@pipe-b-crc-sprite-planes-basic:
    - shard-glk:          PASS -> FAIL [fdo#108145]

  * igt@kms_color@pipe-a-gamma:
    - shard-kbl:          PASS -> FAIL [fdo#104782]
    - shard-apl:          PASS -> FAIL [fdo#104782] +1

  * igt@kms_cursor_crc@cursor-128x42-random:
    - shard-apl:          PASS -> FAIL [fdo#103232] +3

  * igt@kms_cursor_crc@cursor-256x256-suspend:
    - shard-kbl:          PASS -> FAIL [fdo#103191] / [fdo#103232]
    - shard-apl:          PASS -> FAIL [fdo#103191] / [fdo#103232]

  * igt@kms_cursor_crc@cursor-64x64-random:
    - shard-kbl:          PASS -> FAIL [fdo#103232]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render:
    - shard-apl:          NOTRUN -> FAIL [fdo#103167]
    - shard-kbl:          PASS -> FAIL [fdo#103167] +1

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-cpu:
    - shard-apl:          PASS -> FAIL [fdo#103167] +4

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-mmap-cpu:
    - shard-glk:          PASS -> FAIL [fdo#103167] +5

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
    - shard-kbl:          PASS -> INCOMPLETE [fdo#103665]

  * igt@kms_plane_alpha_blend@pipe-a-alpha-opaque-fb:
    - shard-apl:          NOTRUN -> FAIL [fdo#108145] +1

  * igt@kms_plane_multiple@atomic-pipe-c-tiling-none:
    - shard-glk:          PASS -> FAIL [fdo#103166] +1
    - shard-apl:          PASS -> FAIL [fdo#103166] +1

  * igt@kms_universal_plane@universal-plane-pipe-b-functional:
    - shard-kbl:          PASS -> FAIL [fdo#103166]

  
#### Possible fixes ####

  * igt@gem_mmap_gtt@hang:
    - shard-apl:          INCOMPLETE [fdo#103927] / [fdo#109605 ] -> PASS

  * igt@kms_ccs@pipe-a-crc-sprite-planes-basic:
    - shard-apl:          FAIL [fdo#106510] / [fdo#108145] -> PASS

  * igt@kms_cursor_crc@cursor-64x21-onscreen:
    - shard-glk:          FAIL [fdo#103232] -> PASS +2

  * igt@kms_cursor_crc@cursor-64x21-random:
    - shard-apl:          FAIL [fdo#103232] -> PASS +2

  * igt@kms_cursor_crc@cursor-64x64-onscreen:
    - shard-kbl:          FAIL [fdo#103232] -> PASS +2

  * igt@kms_cursor_crc@cursor-64x64-suspend:
    - shard-apl:          FAIL [fdo#103191] / [fdo#103232] -> PASS

  * igt@kms_cursor_crc@cursor-alpha-opaque:
    - shard-kbl:          FAIL [fdo#109350] -> PASS
    - shard-apl:          FAIL [fdo#109350] -> PASS
    - shard-glk:          FAIL [fdo#109350] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-gtt:
    - shard-apl:          FAIL [fdo#103167] -> PASS +1

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
    - shard-kbl:          FAIL [fdo#103167] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-onoff:
    - shard-glk:          FAIL [fdo#103167] -> PASS +5

  * igt@kms_plane@plane-position-covered-pipe-c-planes:
    - shard-apl:          FAIL [fdo#103166] -> PASS +3

  * igt@kms_plane_lowres@pipe-c-tiling-yf:
    - shard-kbl:          DMESG-WARN [fdo#105345] -> PASS

  * igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
    - shard-glk:          FAIL [fdo#103166] -> PASS +3

  * igt@kms_plane_multiple@atomic-pipe-b-tiling-y:
    - shard-kbl:          FAIL [fdo#103166] -> PASS +1

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104782]: https://bugs.freedesktop.org/show_bug.cgi?id=104782
  [fdo#105345]: https://bugs.freedesktop.org/show_bug.cgi?id=105345
  [fdo#106510]: https://bugs.freedesktop.org/show_bug.cgi?id=106510
  [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109350]: https://bugs.freedesktop.org/show_bug.cgi?id=109350
  [fdo#109605 ]: https://bugs.freedesktop.org/show_bug.cgi?id=109605 


Participating hosts (7 -> 5)
------------------------------

  Missing    (2): shard-skl shard-iclb 


Build changes
-------------

    * IGT: IGT_4818 -> IGTPW_2380

  CI_DRM_5591: 1c5934e14f717ab09d832a7200113b02e3196e98 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2380: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2380/
  IGT_4818: ed32029e6a630d9322bd544b626eea38b4785e68 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2380/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t] lib/tests: make sure we catch igt_fork leaks

[Daniel Vetter]

Tests have to call igt_waitchildren(_timeout) before they finish
(either with success or an igt_fail/assert/skip/whatever).

Make sure we catch this.

v2: Another testcase to make sure igt_waitchildren_timeout cleans up.

v3: Actually test that igt_waitchildren_timeout kills children - we
want a proper IGT_EXIT_FAILURE, not some assert.

v4: Adjust testcase, exit code is back to SIGKILL + 128.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 lib/tests/igt_fork.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/lib/tests/igt_fork.c b/lib/tests/igt_fork.c
index 5ff2956dd0ab..8ff4cbfad36b 100644
--- a/lib/tests/igt_fork.c
+++ b/lib/tests/igt_fork.c
@@ -61,6 +61,22 @@ static void igt_fork_vs_assert(void)
 	igt_waitchildren();
 }
 
+static void igt_fork_leak(void)
+{
+	igt_fork(i, 1) {
+		sleep(10);
+	}
+}
+
+static void igt_fork_timeout_leak(void)
+{
+	igt_fork(i, 1) {
+		sleep(10);
+	}
+
+	igt_waitchildren_timeout(1, "library test");
+}
+
 static int do_fork(void (*test_to_run)(void))
 {
 	int pid, status;
@@ -98,4 +114,12 @@ int main(int argc, char **argv)
 	/* check that igt_skip within a fork blows up */
 	ret = do_fork(igt_fork_vs_skip);
 	internal_assert(ret == SIGABRT + 128);
+
+	/* check that failure to clean up fails */
+	ret = do_fork(igt_fork_leak);
+	internal_assert(ret == SIGABRT + 128);
+
+	/* check that igt_waitchildren_timeout cleans up*/
+	ret = do_fork(igt_fork_timeout_leak);
+	internal_assert(ret == SIGKILL + 128);
 }
-- 
2.20.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Daniel Vetter]

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.
  */
 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;
+
+	sigaction(SIGALRM, &sa, NULL);
+
+	alarm(seconds);
+
+	ret = __igt_waitchildren();
 	igt_reset_timeout();
+	if (ret)
+		igt_fail(ret);
 }
 
 /* exit handler code */
-- 
2.20.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Chris Wilson]

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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✗ Fi.CI.BAT: failure for series starting with [i-g-t,1/9] lib/tests: Check that igt_assert forwards correctly through igt_fork (rev4)

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t,1/9] lib/tests: Check that igt_assert forwards correctly through igt_fork (rev4)
URL   : https://patchwork.freedesktop.org/series/56507/
State : failure

== Summary ==

Applying: lib/tests: Check that igt_assert forwards correctly through igt_fork
Applying: lib/tests: make sure igt_skip in igt_fork is forbidden
Applying: lib: Don't leak children in igt_waitchildren_timeout
Applying: tests/gem_exec_reuse: Don't leak the hang detector
Applying: tests/i915_missed_irq: Don't leave the hang detector hanging
Applying: lib/tests: make sure we catch igt_fork leaks
Applying: lib: Make sure we leak no child processes
Using index info to reconstruct a base tree...
M	lib/igt_core.c
M	lib/tests/igt_fork.c
Falling back to patching base and 3-way merge...
Auto-merging lib/tests/igt_fork.c
CONFLICT (content): Merge conflict in lib/tests/igt_fork.c
Auto-merging lib/igt_core.c
Patch failed at 0007 lib: Make sure we leak no child processes
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 1/9] lib/tests: Check that igt_assert forwards correctly through igt_fork

[Chris Wilson]

Quoting Daniel Vetter (2019-02-11 18:02:00)
> Note that without the igt_waitchildren nothing at all gets forwarded,
> maybe we should check for left-behind children somewhere on subtest
> exit.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  lib/tests/igt_fork.c  | 87 +++++++++++++++++++++++++++++++++++++++++++
>  lib/tests/meson.build |  1 +
>  2 files changed, 88 insertions(+)
>  create mode 100644 lib/tests/igt_fork.c
> 
> diff --git a/lib/tests/igt_fork.c b/lib/tests/igt_fork.c
> new file mode 100644
> index 000000000000..d495c7815e06
> --- /dev/null
> +++ b/lib/tests/igt_fork.c
> @@ -0,0 +1,87 @@
> +/*
> + * Copyright © 2019 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */
> +
> +#include <assert.h>
> +#include <errno.h>
> +#include <stdint.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
> +
> +#include "igt_core.h"
> +
> +/*
> + * We need to hide assert from the cocci igt test refactor spatch.
> + *
> + * IMPORTANT: Test infrastructure tests are the only valid places where using
> + * assert is allowed.
> + */
> +#define internal_assert assert
> +
> +char test[] = "test";
> +char *argv_run[] = { test };
> +
> +static void igt_fork_vs_assert(void)
> +{
> +       igt_fork(i, 1) {
> +               igt_assert(0);
> +       }
> +
> +       igt_waitchildren();
> +}
> +
> +static int do_fork(void (*test_to_run)(void))
> +{
> +       int pid, status;
> +       int argc;
> +
> +       switch (pid = fork()) {
> +       case -1:
> +               internal_assert(0);
> +       case 0:
> +               argc = 1;
> +               igt_simple_init(argc, argv_run);
> +               test_to_run();
> +               igt_exit();
> +       default:
> +               while (waitpid(pid, &status, 0) == -1 &&
> +                      errno == EINTR)
> +                       ;
> +
> +               if(WIFSIGNALED(status))
> +                       return WTERMSIG(status) + 128;
> +
> +               return WEXITSTATUS(status);

Rather than reinventing status/exitcode, why not just return status?
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] lib: Make sure we leak no child processes

[Chris Wilson]

Quoting Daniel Vetter (2019-02-11 22:43:15)
> There's a lot more ways to leak children than igt_fork, some even
> handrolled. So check for that. Also have a nice littel testcase for
> that too.
> 
> v2: Don't hang if there's a leaked child process (Chris). Has the
> added benefit that my library unit test also gets faster!
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

> ---
>  lib/igt_core.c       |  4 ++++
>  lib/tests/igt_fork.c | 18 ++++++++++++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index 3053697da58c..d2cfc8e6da20 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -1492,6 +1492,8 @@ void __igt_fail_assert(const char *domain, const char *file, const int line,
>   */
>  void igt_exit(void)
>  {
> +       int tmp;
> +
>         igt_exit_called = true;
>  
>         if (igt_key_file)
> @@ -1527,6 +1529,8 @@ void igt_exit(void)
>                 kill(test_children[c], SIGKILL);
>         assert(!num_test_children);
>  
> +       assert(waitpid(-1, &tmp, WNOHANG) == -1 && errno == ECHILD);
> +
>         if (!test_with_subtests) {
>                 struct timespec now;
>                 const char *result;
> diff --git a/lib/tests/igt_fork.c b/lib/tests/igt_fork.c
> index e5b0ab016b23..b486d07000bb 100644
> --- a/lib/tests/igt_fork.c
> +++ b/lib/tests/igt_fork.c
> @@ -68,6 +68,20 @@ static void igt_fork_leak(void)
>         }
>  }
>  
> +static void plain_fork_leak(void)
> +{
> +       int pid;
> +
> +       switch (pid = fork()) {
> +       case -1:
> +               internal_assert(0);
> +       case 0:
> +               sleep(1);
> +       default:
> +               exit(0);
> +       }
> +}
> +
>  static void igt_fork_timeout_leak(void)
>  {
>         igt_fork(i, 1) {
> @@ -122,4 +136,8 @@ int main(int argc, char **argv)
>         /* check that igt_waitchildren_timeout cleans up*/
>         ret = do_fork(igt_fork_timeout_leak);
>         internal_assert(ret == IGT_EXIT_FAILURE);
> +
> +       /* check that any other process leaks are caught*/
> +       ret = do_fork(plain_fork_leak);
> +       internal_assert(ret == SIGABRT + 128);

I'm mulling over
internal_assert(signaled(status, SIGABRT));

static bool signaled(int status, int sig)
{
	return WIFSIGNALED(status) && WTERMSIG(status) == sig;
}

static bool exited(int status, int code)
{
	return WIFEXITED(status) && WEXITSTATUS(status) == code;
}
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✗ Fi.CI.BAT: failure for series starting with [i-g-t,1/9] lib/tests: Check that igt_assert forwards correctly through igt_fork (rev5)

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t,1/9] lib/tests: Check that igt_assert forwards correctly through igt_fork (rev5)
URL   : https://patchwork.freedesktop.org/series/56507/
State : failure

== Summary ==

Applying: lib/tests: Check that igt_assert forwards correctly through igt_fork
Applying: lib/tests: make sure igt_skip in igt_fork is forbidden
Applying: lib: Don't leak children in igt_waitchildren_timeout
Applying: tests/gem_exec_reuse: Don't leak the hang detector
Applying: tests/i915_missed_irq: Don't leave the hang detector hanging
Applying: lib/tests: make sure we catch igt_fork leaks
Applying: lib: Make sure we leak no child processes
Using index info to reconstruct a base tree...
M	lib/igt_core.c
M	lib/tests/igt_fork.c
Falling back to patching base and 3-way merge...
Auto-merging lib/tests/igt_fork.c
CONFLICT (content): Merge conflict in lib/tests/igt_fork.c
Auto-merging lib/igt_core.c
Patch failed at 0007 lib: Make sure we leak no child processes
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 1/9] lib/tests: Check that igt_assert forwards correctly through igt_fork

[Daniel Vetter]

On Tue, Feb 12, 2019 at 04:44:09PM +0000, Chris Wilson wrote:
> Quoting Daniel Vetter (2019-02-11 18:02:00)
> > Note that without the igt_waitchildren nothing at all gets forwarded,
> > maybe we should check for left-behind children somewhere on subtest
> > exit.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  lib/tests/igt_fork.c  | 87 +++++++++++++++++++++++++++++++++++++++++++
> >  lib/tests/meson.build |  1 +
> >  2 files changed, 88 insertions(+)
> >  create mode 100644 lib/tests/igt_fork.c
> > 
> > diff --git a/lib/tests/igt_fork.c b/lib/tests/igt_fork.c
> > new file mode 100644
> > index 000000000000..d495c7815e06
> > --- /dev/null
> > +++ b/lib/tests/igt_fork.c
> > @@ -0,0 +1,87 @@
> > +/*
> > + * Copyright © 2019 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the next
> > + * paragraph) shall be included in all copies or substantial portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + *
> > + */
> > +
> > +#include <assert.h>
> > +#include <errno.h>
> > +#include <stdint.h>
> > +#include <stdlib.h>
> > +#include <unistd.h>
> > +#include <sys/types.h>
> > +#include <sys/wait.h>
> > +
> > +#include "igt_core.h"
> > +
> > +/*
> > + * We need to hide assert from the cocci igt test refactor spatch.
> > + *
> > + * IMPORTANT: Test infrastructure tests are the only valid places where using
> > + * assert is allowed.
> > + */
> > +#define internal_assert assert
> > +
> > +char test[] = "test";
> > +char *argv_run[] = { test };
> > +
> > +static void igt_fork_vs_assert(void)
> > +{
> > +       igt_fork(i, 1) {
> > +               igt_assert(0);
> > +       }
> > +
> > +       igt_waitchildren();
> > +}
> > +
> > +static int do_fork(void (*test_to_run)(void))
> > +{
> > +       int pid, status;
> > +       int argc;
> > +
> > +       switch (pid = fork()) {
> > +       case -1:
> > +               internal_assert(0);
> > +       case 0:
> > +               argc = 1;
> > +               igt_simple_init(argc, argv_run);
> > +               test_to_run();
> > +               igt_exit();
> > +       default:
> > +               while (waitpid(pid, &status, 0) == -1 &&
> > +                      errno == EINTR)
> > +                       ;
> > +
> > +               if(WIFSIGNALED(status))
> > +                       return WTERMSIG(status) + 128;
> > +
> > +               return WEXITSTATUS(status);
> 
> Rather than reinventing status/exitcode, why not just return status?

Copypasta from other testcases. I guess we could clean up all of them?
I'll grab some coffee and work up the motivation to fight all the
conflicts :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Daniel Vetter]

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
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev