Am 08.03.23 um 17:44 schrieb Asahi Lina:
On 09/03/2023 00.30, Christian König wrote:
Am 08.03.23 um 15:53 schrieb Asahi Lina:
[SNIP]
The background is that core memory management requires that signaling a fence only depends on signaling other fences and hardware progress and nothing else. Otherwise you immediately run into problems because of circle dependencies or what we call infinite fences.
And hardware progress is exactly the only dependency here...
Well then you should have a fence for that hardware progress.
I do, it's the prior job hardware completion fences that drm_sched already knows about!
Yes, I could return those in the prepare callback, it just means I need to start stashing fence references in the underlying firmware job queue command objects so I can find out what is the oldest pending fence is, and return it when a queue is full. As long as drm_sched doesn't mind if I keep giving it fences (since multiple commands can have to complete before there is space) or the occasional already signaled fence (because this process is inherently racy), it should work fine.
Well this handling is intentional and necessary, but see below for a more in deep explanation.
If you think this is the better way, I'll do it that way and drop this patch. It just seemed simpler to do it with another callback, since drm_sched is already tracking those fences and doing a hardware queue limit check anyway, and that way I can avoid tracking the fences down into the hardware queue code... *
Well it's not the better way, it's the only way that works.
I have to admit that my bet on your intentions was wrong, but even that use case doesn't work correctly.
See when your callback returns false it is perfectly possible that all hw fences are signaled between returning that information and processing it.
The result would be that the scheduler goes to sleep and never wakes up again.
That's why we have that rule that all dependencies need to be expressed by those dma_fence objects, cause those are design with such races in mind.
(But I still maintain what I'm trying to do here is entirely correct and deadlock-free! If you prefer I use prepare_job and return prior job fences from that instead, that's very different from NAKing the patch saying it's broken...)
As I said we exercised those ideas before and yes this approach here came up before as well and no it doesn't work.
- If you're wondering how the fences get signaled at all then: callback
closures that capture a reference to the fence when firmware commands are constructed and submitted. I know, I know, fancy Rust stuff... ^^ If you'd rather have me use the fences for the blocking, I'll probably just drop the signaling bit from the closures so we don't need to keep two redundant fence references in different places per command. I still need the closures for command completion processing though, since I use them to process statistics too...
I see that we have a disconnection here. As far as I can see you can use the can_run callback in only three ways:
- To check for some userspace dependency (We don't need to discuss
that, it's evil and we both know it).
- You check for some hw resource availability. Similar to VMID on
amdgpu hw.
This is what I think you do here (but I might be wrong).
It isn't... I agree, it would be problematic. It doesn't make any sense to check for global resources this way, not just because you might deadlock but also because there might be nothing to signal to the scheduler that a resource was freed at all once it is!
But this would be extremely problematic because you can then live lock. E.g. queue A keeps submitting jobs which take only a few resources and by doing so delays submitting jobs from queue B indefinitely.
This particular issue aside, fairness in global resource allocation is a conversation I'd love to have! Right now the driver doesn't try to ensure that, a queue can easily monopolize certain hardware resources (though one queue can only monopolize one of each, so you'd need something like 63 queues with 63 distinct VMs all submitting free-running jobs back to back in order to starve other queues of resources forever). For starters, one thing I'm thinking of doing is reserving certain subsets of hardware resources for queues with a given priority, so you can at least guarantee forward progress of higher-priority queues when faced with misbehaving lower-priority queues. But if we want to guarantee proper fairness, I think I'll have to start doing things like switching to a CPU-roundtrip submission model when resources become scarce (to guarantee that queues actually release the resources once in a while) and then figure out how to add fairness to the allocation code...
But let's have that conversation when we talk about the driver (or maybe on IRC or something?), right now I'm more interested in getting the abstractions reviewed ^^
Well that stuff is highly problematic as well. The fairness aside you risk starvation which in turn breaks the guarantee of forward progress.
In this particular case you can catch this with a timeout for the hw operation, but you should consider blocking that from the sw side as well.
- You have an intra queue dependency. E.g. you have jobs which take X
amount of resources, you can submit only to a specific limit. But in this case you should be able to return fences from the same queue as dependency and won't need that callback.
Yes, I can do this. I can just do the same check can_run_job() does and if it fails, pick the oldest job in the full firmware queue and return its fence (it just means I need to keep track of those fences there, as I said above).
We would just need to adjust drm_sched_entity_add_dependency_cb() a bit because dependencies from the same queue are currently filtered out because it assumes a pipeline nature of submission (e.g. previous submissions are finished before new submissions start).
Actually that should be fine, because I'd be returning the underlying hardware completion fences (what the run() callback returns) which the driver owns, and wouldn't be recognized as belonging to the sched.
I actually know I have a different theoretical deadlock issue along these lines in the driver because right now we grab actually global resources (including a VMID) before job submission to drm_sched. This is a known issue, and to fix it without reducing performance I need to introduce some kind of "patching/fixup" system for firmware commands (because we need to inject those identifiers in dozens of places, but we don't want to construct those commands from scratch at job run time because that introduces latency at the wrong time and makes error handling/validation more complicated and error-prone), and that is exactly what should happen in prepare_job, as you say. And yes, at that point that should use fences to block when those resources are exhausted. But that's a different discussion we should have when reviewing the driver, it has nothing to do with the DRM abstractions nor the can_run_job callback I'm adding here nor the firmware queue length limit issue! (And also the global hardware devices are plentiful enough that I would be very surprised if anyone ever deadlocks it in practice even with the current code, so I honestly don't think that should be a blocker for driver submission either, I can and will fix it later...)
Well this is what I thought about those problems in amdgpu as well and it totally shipwrecked.
We still have memory allocations in the VMID code path which I'm still not sure how to remove.
We don't even have a shrinker yet, and I'm sure that's going to be a lot of fun when we add it too... but yes, if we can't do any memory allocations in some of these callbacks (is this documented anywhere?), that's going to be interesting...
Yes, that is all part of the dma_fence documentation. It's just absolutely not obvious what all this means.
It's not all bad news though! All memory allocations are fallible in kernel Rust (and therefore explicit, and also failures have to be explicitly handled or propagated), so it's pretty easy to point out where they are, and there are already discussions of higher-level tooling to enforce rules like that (and things like wait contexts). Also, Rust makes it a lot easier to refactor code in general and not be scared that you're going to regress everything, so I'm not really worried if I need to turn a chunk of the driver on its head to solve some of these problems in the future ^^ (I already did that when I switched it from the "demo" synchronous submission model to the proper explicit sync + fences one.)
Yeah, well the problem isn't that you run into memory allocation failure.
The problem is rather something like this: 1. You try to allocate memory to signal your fence. 2. This memory allocation can't be fulfilled and goes to sleep to wait for reclaim. 3. On another CPU reclaim is running and through the general purpose shrinker, page fault or MMU notifier ends up wait for your dma_fence.
You don't even need to implement the shrinker for this to go boom extremely easy.
So everything involved with signaling the fence can allocate memory only with GFP_ATOMIC and only if you absolutely have to.
Christian.
~~ Lina