On Tue, Mar 07, 2023 at 11:25:36PM +0900, Asahi Lina wrote:
drm_sched_fini() currently leaves any pending jobs dangling, which causes segfaults and other badness when job completion fences are signaled after the scheduler is torn down.
Explicitly detach all jobs from their completion callbacks and free them. This makes it possible to write a sensible safe abstraction for drm_sched, without having to externally duplicate the tracking of in-flight jobs.
This shouldn't regress any existing drivers, since calling drm_sched_fini() with any pending jobs is broken and this change should be a no-op if there are no pending jobs.
Signed-off-by: Asahi Lina lina@asahilina.net
drivers/gpu/drm/scheduler/sched_main.c | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 5c0add2c7546..0aab1e0aebdd 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -1119,10 +1119,33 @@ EXPORT_SYMBOL(drm_sched_init); void drm_sched_fini(struct drm_gpu_scheduler *sched) { struct drm_sched_entity *s_entity;
- struct drm_sched_job *s_job, *tmp; int i;
- if (sched->thread)
kthread_stop(sched->thread);
- if (!sched->thread)
return;- /*
* Stop the scheduler, detaching all jobs from their hardware callbacks* and cleaning up complete jobs.*/- drm_sched_stop(sched, NULL);
- /*
* Iterate through the pending job list and free all jobs.* This assumes the driver has either guaranteed jobs are already stopped, or that* otherwise it is responsible for keeping any necessary data structures for* in-progress jobs alive even when the free_job() callback is called early (e.g. by* putting them in its own queue or doing its own refcounting).*/
This comment makes me wonder whether we shouldn't go one step further and have a drm_sched_quiescent, which waits for any in-flight jobs to complete and cancels everything else. Because even if rust guarantees that you don't have any memory bugs, if you just leak things by sprinkling reference-counted pointer wrappers everywhere you still have a semantic bug.
Except now it's much harder to realize that because there's no Oops and KASAN doesn't tell you about it either. I think it would be much better if the scheduler code and rust abstraction provider drivers the correct lifetimes and very strongly encourage them to only have borrowed references and not additional refcounting of their own.
I think Christian mentioned that this would block in close() or context destruction, which is no good at all. And with the 1:1 drm_scheduler:drm_sched_entity design for there's no other place. This is way I've suggested in the Xe threads that we should make the current drm_scheduler an implementation detail hidden from drivers, with a new drm_scheduler which is always per-engine for all cases as the driver api interface. And the internal scheduler attached to either that (for current drivers) or drm_sched_entity (for fw scheduling drivers) as needed. With that - the sched_entity cleanup could take care of this code here for the fw scheduler case - the drm_sched_fini could take care of blocking appropriately before the driver is unloaded for any lagging in-flight jobs, without blocking userspace - drivers should not end up with any need to reference-count either per-ctx/drm_sched_entity or per-drm_sched_job data, ever
Because any comment that's along the lines of "drivers need to refcount" is bad business, because it either means leaks (rust) or crashes (C). I much prefer when drivers have to put in extra effort to get things wrong because by default the lifetimes are Just Right(tm). -Daniel
- list_for_each_entry_safe(s_job, tmp, &sched->pending_list, list) {
spin_lock(&sched->job_list_lock);list_del_init(&s_job->list);spin_unlock(&sched->job_list_lock);sched->ops->free_job(s_job);- }
- kthread_stop(sched->thread);
for (i = DRM_SCHED_PRIORITY_COUNT - 1; i >= DRM_SCHED_PRIORITY_MIN; i--) { struct drm_sched_rq *rq = &sched->sched_rq[i];
-- 2.35.1