CI was extremely sporadically (once a month or two) reporting an issue
with unexpected premature completion of a spin supposed to be still
running.  It occurred possible to reproduce the issue in a few hours by
running the test in a loop on one of affected platforms.

Having a closer look at the logs, it became clear that the test was
blocking somewhere before the check for the spin still busy, and the spin
was meanwhile aborted by a request watchdog timer after a default period
of 20s of its activity.

Deeper debugging revealed that one of the test subprocesses was trying to
evict a VMA and was waiting infinitely for its idleness.  Since the scope
of affected platforms was limited to Gen12, where no kernel side
relocations are used, Chris suggested that must have been a userspace bug,
i.e., the test was apparently providing conflicting object offsets.
Further analysis also shown that the main process of the test was looping
infinitely around a page fault that occurred by reading another, mmapped
GEM object shared with subprocesses, apparently locked by the subprocess
waiting on eviction.

Based on all those observations, Zbigniew noticed incorrect use of
intel_allocator library, leading to unexpected reuse of a just closed
object handle, and its already allocated offset, by another child process
before the allocator drops a link of the offset to the handle.  That
theory was then confirmed by:
a) forcing a single fixed offset to be used by each child process, which
   resulted in immediate reproduction of the issue on first attempt, and
b) extending a potential race window by introducing a short delay between
   GEM closing the object of the VMA and the allocator forgetting the link
   between its handle and offset, which also resulted in prompt
   reproduction of the issue.

Close the race window by reordering those two cleanup operations inside an
IGT store function: call put_offset() first, then gem_close(), so other
test processes have no chance to pick up the same offset when reusing the
just closed GEM object handle.

Correctness of the proposed solution was confirmed by running the fixed
test in a loop for 12 hours with no issues.

Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/15263
Suggested-by: Chris Wilson <[email protected]>
Suggested-by: Zbigniew Kempczyński <[email protected]>
Signed-off-by: Janusz Krzysztofik <[email protected]>
---
 lib/igt_store.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/igt_store.c b/lib/igt_store.c
index 42ffdc5cdb..ce29355c3d 100644
--- a/lib/igt_store.c
+++ b/lib/igt_store.c
@@ -95,6 +95,6 @@ void igt_store_word(int fd, uint64_t ahnd, const intel_ctx_t 
*ctx,
        batch[++i] = MI_BATCH_BUFFER_END;
        gem_write(fd, obj[BATCH].handle, 0, batch, sizeof(batch));
        gem_execbuf(fd, &execbuf);
-       gem_close(fd, obj[BATCH].handle);
        put_offset(ahnd, obj[BATCH].handle);
+       gem_close(fd, obj[BATCH].handle);
 }
-- 
2.54.0

Reply via email to