[PATCH v3 0/5] Fix memory leaks in drm_sched_fini()

[Philipp Stanner]

Changelog below.

I experimented with the alternative solution (cancel_job() callback) and
maintain the position that this approach is more stable and robust,
despite more code added. I feel more comfortable with it regarding
stability and the possiblity of porting more drivers.

Changes in v3:
  - Fix and simplify cleanup callback in unit tests. Behave like a
    driver would: cancel interrupts (hrtimer), then simply run into
    drm_sched_fini().
  - Use drm_mock_sched_complete() as a centralized position to signal
    fences.
  - Reorder pending_list-is-empty warning patch for correct behavior. (Tvrtko)
  - Rename cleanup callback so that it becomes clear that it's about
    signaling all in-flight fences. (Tvrtko, Danilo, Me)
  - Add more documentation for the new functions.
  - Fix some typos.

Changes in v2:
  - Port kunit tests to new cleanup approach (Tvrtko), however so far
    still with the fence context-kill approach (Me. to be discussed.)
  - Improve len(pending_list) > 0 warning print. (Me)
  - Remove forgotten RFC comments. (Me)
  - Remove timeout boolean, since it's surplus. (Me)
  - Fix misunderstandings in the cover letter. (Tvrtko)

Changes since the RFC:
  - (None)

Howdy,

as many of you know, we have potential memory leaks in drm_sched_fini()
which have been tried to be solved by various parties with various
methods in the past.

In our past discussions, we came to the conclusion, that the simplest
solution, blocking in drm_sched_fini(), is not possible because it could
cause processes ignoring SIGKILL and blocking for too long (which could
turn out to be an effective way to generate a funny email from Linus,
though :) )

Another idea was to have submitted jobs refcount the scheduler. I
investigated this and we found that this then *additionally* would
require us to have *the scheduler* refcount everything *in the driver*
that is accessed through the still running callbacks; since the driver
would want to unload possibly after a non-blocking drm_sched_fini()
call. So that's also no solution.

This RFC here is a new approach, somewhat based on the original
waitque-idea. It looks as follows:

1. Have drm_sched_fini() block until the pending_list becomes empty with
   a waitque, as a first step.
2. Provide the scheduler with a callback with which it can instruct the
   driver to kill the associated fence context. This will cause all
   pending hardware fences to get signalled. (Credit to Danilo, whose
   idea this was)
3. In drm_sched_fini(), first switch off submission of new jobs and
   timeouts (the latter might not be strictly necessary, but is probably
   cleaner).
4. Then, call the aformentioned callback, ensuring that free_job() will
   be called for all remaining jobs relatively quickly. This has the
   great advantage that the jobs get cleaned up through the standard
   mechanism.
5. Once all jobs are gone, also switch off the free_job() work item and
   then proceed as usual.

Furthermore, since there is now such a callback, we can provide an
if-branch checking for its existence. If the driver doesn't provide it,
drm_sched_fini() operates in "legacy mode". So none of the existing
drivers should notice a difference and we remain fully backwards
compatible.

Our glorious beta-tester is Nouveau, which so far had its own waitque
solution, which is now obsolete. The last two patches port Nouveau and
remove that waitque.

I've tested this on a desktop environment with Nouveau. Works fine and
solves the problem (though we did discover an unrelated problem inside
Nouveau in the process).

Tvrtko's unit tests also run as expected (except for the new warning
print in patch 3), which is not surprising since they don't provide the
callback.

I'm looking forward to your input and feedback. I really hope we can
work this RFC into something that can provide users with a more
reliable, clean scheduler API.

Philipp

Philipp Stanner (5):
  drm/sched: Fix teardown leaks with waitqueue
  drm/sched/tests: Port tests to new cleanup method
  drm/sched: Warn if pending list is not empty
  drm/nouveau: Add new callback for scheduler teardown
  drm/nouveau: Remove waitque for sched teardown

 drivers/gpu/drm/nouveau/nouveau_abi16.c       |   4 +-
 drivers/gpu/drm/nouveau/nouveau_drm.c         |   2 +-
 drivers/gpu/drm/nouveau/nouveau_sched.c       |  39 ++++---
 drivers/gpu/drm/nouveau/nouveau_sched.h       |  12 +-
 drivers/gpu/drm/nouveau/nouveau_uvmm.c        |   8 +-
 drivers/gpu/drm/scheduler/sched_main.c        | 108 +++++++++++++++---
 .../gpu/drm/scheduler/tests/mock_scheduler.c  |  67 ++++-------
 drivers/gpu/drm/scheduler/tests/sched_tests.h |   4 +-
 include/drm/gpu_scheduler.h                   |  19 +++
 9 files changed, 170 insertions(+), 93 deletions(-)

-- 
2.49.0