On 09/03/2023 00.42, Christian König wrote:
Am 08.03.23 um 16:18 schrieb Asahi Lina:
On 08/03/2023 19.03, Christian König wrote:
Am 08.03.23 um 10:57 schrieb Maarten Lankhorst:
On 2023-03-07 15:25, 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). + */ + 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); + }
I would stop the kthread first, then delete all jobs without spinlock since nothing else can race against sched_fini?
If you do need the spinlock, It would need to guard list_for_each_entry too.
Well this case here actually should not happen in the first place.
"This should not happen in the first place" is how you end up with C APIs that have corner cases that lead to kernel oopses...
The idea with Rust abstractions is that it needs to be actually impossible to create memory safety problems for the user of the abstraction, you can't impose arbitrary constraints like "you must wait for all jobs to finish before destroying the scheduler"... it needs to be intrinsically safe.
Jobs depend on their device, so as long as there are jobs there should also be a reference to the scheduler.
These schedulers are created dynamically per userspace queue. The memory management and reference counting involved make it safe to destroy the scheduler even when behind the scenes hardware jobs are still running, as long as drm_sched itself doesn't crash on fences firing without a scheduler (which is what this patch fixes).
We have originally rejected that approach, but I still think it might work if done right.
This is the power of Rust: it forces you to architect your code in a way that you don't have complex high-level dependencies that span the entire driver and are difficult to prove hold. In my driver, you can kill a process and that destroys the drm_sched, closes all GEM objects, everything, even if the GPU is still running jobs from that process. The worst that can happen is that the GPU faults as in-use userspace buffers are unmapped out from under the running user job, but that's fine (GPU faults are recoverable). The actual firmware resources, queues, etc. in use are all kept alive until the commands finish executing (or fault, which is just an abnormal completion), even if the userspace process that owned them is long gone. I've tested this extensively by doing things like large-resolution glmark runs in a loop that get `kill -9`'d repeatedly, and it works very well! Tons of GPU faults but no firmware crashes, no oopses, nothing. And the firmware *will* crash irrecoverably if anything goes wrong with its shared memory structures, so that it doesn't is pretty good evidence that all this works!
Well testing is no prove at all of a correct design.
Well, I'm guessing you don't have a formal correctness proof for amdgpu either... ^^
There's actually no way to prove my design is correct, since this is a reverse engineered driver that talks to proprietary firmware and I don't have the benefit of both open and internal docs like you AMD people have, never mind access to firmware source code... all I can do is try to understand how it should work based on how macOS does things and running tests, and then design something that should work with it. I spent months writing a prototype Python driver before even starting on the real DRM driver (long story...), and I keep going back to it to test little details of the firmware interface. There's over 3300 lines of just firmware structure definitions, it's kind of crazy...
But even with all that... this driver has no right to be as stable as it is, considering I wrote it in just a few months. It hasn't even been a year since I started working on AGX at all! As I mentioned in the cover letter, we've gotten zero reports of oopses in production. I tried fuzzing the UAPI and all I managed to do was crash the firmware after a lot of GPU faults (that was a subtle firmware data cache coherency issue, now fixed), the driver itself was fine. I didn't have to debug the OOM error codepaths when we first started running Xonotic on 8GB RAM machines with no texture compression support on high quality texture settings (bad combination...), it all just worked even though all those error/cleanup paths had never been tested before at all. The only memory leaks I managed to cause were due to circular references between VMs and GEM objects (tricky to avoid, I did manage to miss one special case object in the first driver release...), everything else just cleans itself up by design. And it's not because I'm a genius or anything like that... it's because Rust just makes getting all this right *so* much easier than C.
So I can at least say I'm quite confident that, as long as my understanding of the firmware structure lifetimes is correct and I encode it in the Rust object model the driver uses to represent them, things will work without crashing without relying on high-level invariants like "you must wait for all job completions before tearing down the top-level scheduler for a user queue" ^^
What could be is that you have allocated a scheduler instance dynamically, but even then you should first tear down all entities and then the scheduler.
This is about creating a safe Rust abstraction, so we can't impose requirements on users like that, the abstraction has to take care of it. Unfortunately, the jobs cannot depend on the scheduler at the abstraction level. I tried that (putting a reference counted reference to the scheduler in the job abstraction), but it doesn't work because a job completing can end up dropping the last reference to the scheduler, and then you end up trying to stop and clean up the scheduler from a callback called from the scheduler kthread itself, which deadlocks. We could throw those cleanups into a workqueue or something, but that's just adding bandages around the problem that the drm_sched interface today is just not safe without this patch...
Well that won't work like this. The scheduler has a pretty clear tear down procedure.
Well... I wouldn't call it "clear". I had to reverse engineer this from reading drm_sched source code, the docs don't tell you. The entire documentation of "drm_sched_fini()" is as follows:
"Tears down and cleans up the scheduler."
That's it.
This is why I had so much trouble writing this abstraction, and I spent hours reading the drm_sched code to understand how it worked in order to use the API correctly... and yet...
And that procedure implies that all entities which might provide jobs are destroyed before the scheduler is destroyed.
Yes, I do this: the entity abstraction holds a reference to the scheduler for this reason, so the scheduler can only be destroyed once all entities are destroyed. But...
Destroying the entities in turn cleans up the pending jobs inside of them.
Yes but... none of this cleans up jobs that are already submitted by the scheduler and in its pending list, with registered completion callbacks, which were already popped off of the entities.
*That* is the problem this patch fixes!
We could add a warning when users of this API doesn't do this correctly, but cleaning up incorrect API use is clearly something we don't want here.
It is the job of the Rust abstractions to make incorrect API use that leads to memory unsafety impossible. So even if you don't want that in C, it's my job to do that for Rust... and right now, I just can't because drm_sched doesn't provide an API that can be safely wrapped without weird bits of babysitting functionality on top (like tracking jobs outside or awkwardly making jobs hold a reference to the scheduler and defer dropping it to another thread).
Right now, it is not possible to create a safe Rust abstraction for drm_sched without doing something like duplicating all job tracking in the abstraction, or the above backreference + deferred cleanup mess, or something equally silly. So let's just fix the C side please ^^
Nope, as far as I can see this is just not correctly tearing down the objects in the right order.
There's no API to clean up in-flight jobs in a drm_sched at all. Destroying an entity won't do it. So there is no reasonable way to do this at all...
So you are trying to do something which is not supposed to work in the first place.
I need to make things that aren't supposed to work impossible to do in the first place, or at least fail gracefully instead of just oopsing like drm_sched does today...
If you're convinced there's a way to do this, can you tell me exactly what code sequence I need to run to safely shut down a scheduler assuming all entities are already destroyed? You can't ask me for a list of pending jobs (the scheduler knows this, it doesn't make any sense to duplicate that outside), and you can't ask me to just not do this until all jobs complete execution (because then we either end up with the messy deadlock situation I described if I take a reference, or more duplicative in-flight job count tracking and blocking in the free path of the Rust abstraction, which doesn't make any sense either).
~~ Lina