Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
On Wed, Apr 05, 2023 at 04:14:11PM +0200, Christian König wrote:
> Am 05.04.23 um 15:40 schrieb Daniel Vetter:
> > On Tue, Mar 07, 2023 at 11:25:35PM +0900, Asahi Lina wrote:
> > > Some hardware may require more complex resource utilization accounting
> > > than the simple job count supported by drm_sched internally. Add a
> > > can_run_job callback to allow drivers to implement more logic before
> > > deciding whether to run a GPU job.
> > >
> > > Signed-off-by: Asahi Lina
> > Ok scheduler rules, or trying to summarize the entire discussion:
> >
> > dma_fence rules are very tricky. The two main chapters in the docs are
> >
> > https://dri.freedesktop.org/docs/drm/driver-api/dma-buf.html?highlight=dma_buf#dma-fence-cross-driver-contract
> > https://dri.freedesktop.org/docs/drm/driver-api/dma-buf.html?highlight=dma_buf#indefinite-dma-fences
> >
> > Unforutunately I don't think it's possible to check this at compile time,
> > thus far all we can do is validate at runtime. I've posted two patches for
> > this:
> >
> > https://lore.kernel.org/dri-devel/20201023122216.2373294-17-daniel.vet...@ffwll.ch/
> > https://lore.kernel.org/dri-devel/20201023122216.2373294-20-daniel.vet...@ffwll.ch/
> >
> > Unfortunately most drivers are buggy and get this completely wrong, so
> > realistically we'd need to make this a per-driver opt-out and annotate all
> > current drivers. Well except amdgpu is correct by now I think (they'd
> > still need to test that).
>
> There is still one potential memory allocation in the run_job callback in
> amdgpu which I wasn't able to fix yet.
>
> But that one is purely academic and could potentially be trivially replaced
> with using GFP_ATOMIC if we ever have to.
I think the modeset in the tdr code was more scary, and I'm not sure you
really managed to get rid of absolutely everything in there yet.
-Daniel
>
> Christian.
>
> > And Rob Clark is working on patches to fix up
> > msm.
> >
> > I think best here is if you work together with Rob to make sure these
> > annotations are mandatory for any rust drivers (I don't want new buggy
> > drivers at least). Would also be great to improve the kerneldoc for all
> > the driver hooks to explain these restrictions and link to the relevant
> > kerneldocs (there's also one for the dma_fence signalling annotations
> > which might be worth linking too).
> >
> > I don't see any way to make this explicit in rust types, it's really only
> > something runtime tests (using lockdep) can catch. Somewhat disappointing.
> >
> > For the other things discussed here:
> >
> > - Option as the return value for ->prepare_job makes sense to
> >me.
> >
> > - I don't see any way a driver can use ->can_run_job without breaking the
> >above rules, that really doesn't sound like a good idea to me.
> >
> > Cheers, Daniel
> >
> > > ---
> > > drivers/gpu/drm/scheduler/sched_main.c | 10 ++
> > > include/drm/gpu_scheduler.h| 8
> > > 2 files changed, 18 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> > > b/drivers/gpu/drm/scheduler/sched_main.c
> > > index 4e6ad6e122bc..5c0add2c7546 100644
> > > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > > @@ -1001,6 +1001,16 @@ static int drm_sched_main(void *param)
> > > if (!entity)
> > > continue;
> > > + if (sched->ops->can_run_job) {
> > > + sched_job =
> > > to_drm_sched_job(spsc_queue_peek(>job_queue));
> > > + if (!sched_job) {
> > > + complete_all(>entity_idle);
> > > + continue;
> > > + }
> > > + if (!sched->ops->can_run_job(sched_job))
> > > + continue;
> > > + }
> > > +
> > > sched_job = drm_sched_entity_pop_job(entity);
> > > if (!sched_job) {
> > > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> > > index 9db9e5e504ee..bd89ea9507b9 100644
> > > --- a/include/drm/gpu_scheduler.h
> > > +++ b/include/drm/gpu_scheduler.h
> > > @@ -396,6 +396,14 @@ struct drm_sched_backend_ops {
> > > struct dma_fence *(*prepare_job)(struct drm_sched_job
> > > *sched_job,
> > >struct drm_sched_entity
> > > *s_entity);
> > > + /**
> > > + * @can_run_job: Called before job execution to check whether the
> > > + * hardware is free enough to run the job. This can be used to
> > > + * implement more complex hardware resource policies than the
> > > + * hw_submission limit.
> > > + */
> > > + bool (*can_run_job)(struct drm_sched_job *sched_job);
> > > +
> > > /**
> > >* @run_job: Called to execute the job once all of the
> > > dependencies
> > >* have been resolved. This may be called multiple times, if
> > >
> > > --
> > > 2.35.1
> > >
>
--
Daniel
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
Am 05.04.23 um 15:40 schrieb Daniel Vetter:
On Tue, Mar 07, 2023 at 11:25:35PM +0900, Asahi Lina wrote:
Some hardware may require more complex resource utilization accounting
than the simple job count supported by drm_sched internally. Add a
can_run_job callback to allow drivers to implement more logic before
deciding whether to run a GPU job.
Signed-off-by: Asahi Lina
Ok scheduler rules, or trying to summarize the entire discussion:
dma_fence rules are very tricky. The two main chapters in the docs are
https://dri.freedesktop.org/docs/drm/driver-api/dma-buf.html?highlight=dma_buf#dma-fence-cross-driver-contract
https://dri.freedesktop.org/docs/drm/driver-api/dma-buf.html?highlight=dma_buf#indefinite-dma-fences
Unforutunately I don't think it's possible to check this at compile time,
thus far all we can do is validate at runtime. I've posted two patches for
this:
https://lore.kernel.org/dri-devel/20201023122216.2373294-17-daniel.vet...@ffwll.ch/
https://lore.kernel.org/dri-devel/20201023122216.2373294-20-daniel.vet...@ffwll.ch/
Unfortunately most drivers are buggy and get this completely wrong, so
realistically we'd need to make this a per-driver opt-out and annotate all
current drivers. Well except amdgpu is correct by now I think (they'd
still need to test that).
There is still one potential memory allocation in the run_job callback
in amdgpu which I wasn't able to fix yet.
But that one is purely academic and could potentially be trivially
replaced with using GFP_ATOMIC if we ever have to.
Christian.
And Rob Clark is working on patches to fix up
msm.
I think best here is if you work together with Rob to make sure these
annotations are mandatory for any rust drivers (I don't want new buggy
drivers at least). Would also be great to improve the kerneldoc for all
the driver hooks to explain these restrictions and link to the relevant
kerneldocs (there's also one for the dma_fence signalling annotations
which might be worth linking too).
I don't see any way to make this explicit in rust types, it's really only
something runtime tests (using lockdep) can catch. Somewhat disappointing.
For the other things discussed here:
- Option as the return value for ->prepare_job makes sense to
me.
- I don't see any way a driver can use ->can_run_job without breaking the
above rules, that really doesn't sound like a good idea to me.
Cheers, Daniel
---
drivers/gpu/drm/scheduler/sched_main.c | 10 ++
include/drm/gpu_scheduler.h| 8
2 files changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
index 4e6ad6e122bc..5c0add2c7546 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1001,6 +1001,16 @@ static int drm_sched_main(void *param)
if (!entity)
continue;
+ if (sched->ops->can_run_job) {
+ sched_job =
to_drm_sched_job(spsc_queue_peek(>job_queue));
+ if (!sched_job) {
+ complete_all(>entity_idle);
+ continue;
+ }
+ if (!sched->ops->can_run_job(sched_job))
+ continue;
+ }
+
sched_job = drm_sched_entity_pop_job(entity);
if (!sched_job) {
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 9db9e5e504ee..bd89ea9507b9 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -396,6 +396,14 @@ struct drm_sched_backend_ops {
struct dma_fence *(*prepare_job)(struct drm_sched_job *sched_job,
struct drm_sched_entity *s_entity);
+ /**
+* @can_run_job: Called before job execution to check whether the
+* hardware is free enough to run the job. This can be used to
+* implement more complex hardware resource policies than the
+* hw_submission limit.
+*/
+ bool (*can_run_job)(struct drm_sched_job *sched_job);
+
/**
* @run_job: Called to execute the job once all of the dependencies
* have been resolved. This may be called multiple times, if
--
2.35.1
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
On Tue, Mar 07, 2023 at 11:25:35PM +0900, Asahi Lina wrote:
> Some hardware may require more complex resource utilization accounting
> than the simple job count supported by drm_sched internally. Add a
> can_run_job callback to allow drivers to implement more logic before
> deciding whether to run a GPU job.
>
> Signed-off-by: Asahi Lina
Ok scheduler rules, or trying to summarize the entire discussion:
dma_fence rules are very tricky. The two main chapters in the docs are
https://dri.freedesktop.org/docs/drm/driver-api/dma-buf.html?highlight=dma_buf#dma-fence-cross-driver-contract
https://dri.freedesktop.org/docs/drm/driver-api/dma-buf.html?highlight=dma_buf#indefinite-dma-fences
Unforutunately I don't think it's possible to check this at compile time,
thus far all we can do is validate at runtime. I've posted two patches for
this:
https://lore.kernel.org/dri-devel/20201023122216.2373294-17-daniel.vet...@ffwll.ch/
https://lore.kernel.org/dri-devel/20201023122216.2373294-20-daniel.vet...@ffwll.ch/
Unfortunately most drivers are buggy and get this completely wrong, so
realistically we'd need to make this a per-driver opt-out and annotate all
current drivers. Well except amdgpu is correct by now I think (they'd
still need to test that). And Rob Clark is working on patches to fix up
msm.
I think best here is if you work together with Rob to make sure these
annotations are mandatory for any rust drivers (I don't want new buggy
drivers at least). Would also be great to improve the kerneldoc for all
the driver hooks to explain these restrictions and link to the relevant
kerneldocs (there's also one for the dma_fence signalling annotations
which might be worth linking too).
I don't see any way to make this explicit in rust types, it's really only
something runtime tests (using lockdep) can catch. Somewhat disappointing.
For the other things discussed here:
- Option as the return value for ->prepare_job makes sense to
me.
- I don't see any way a driver can use ->can_run_job without breaking the
above rules, that really doesn't sound like a good idea to me.
Cheers, Daniel
> ---
> drivers/gpu/drm/scheduler/sched_main.c | 10 ++
> include/drm/gpu_scheduler.h| 8
> 2 files changed, 18 insertions(+)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> b/drivers/gpu/drm/scheduler/sched_main.c
> index 4e6ad6e122bc..5c0add2c7546 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -1001,6 +1001,16 @@ static int drm_sched_main(void *param)
> if (!entity)
> continue;
>
> + if (sched->ops->can_run_job) {
> + sched_job =
> to_drm_sched_job(spsc_queue_peek(>job_queue));
> + if (!sched_job) {
> + complete_all(>entity_idle);
> + continue;
> + }
> + if (!sched->ops->can_run_job(sched_job))
> + continue;
> + }
> +
> sched_job = drm_sched_entity_pop_job(entity);
>
> if (!sched_job) {
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 9db9e5e504ee..bd89ea9507b9 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -396,6 +396,14 @@ struct drm_sched_backend_ops {
> struct dma_fence *(*prepare_job)(struct drm_sched_job *sched_job,
>struct drm_sched_entity *s_entity);
>
> + /**
> + * @can_run_job: Called before job execution to check whether the
> + * hardware is free enough to run the job. This can be used to
> + * implement more complex hardware resource policies than the
> + * hw_submission limit.
> + */
> + bool (*can_run_job)(struct drm_sched_job *sched_job);
> +
> /**
> * @run_job: Called to execute the job once all of the dependencies
> * have been resolved. This may be called multiple times, if
>
> --
> 2.35.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
On Wed, Mar 08, 2023 at 04:19:17PM +0100, Karol Herbst wrote: > On Wed, Mar 8, 2023 at 4:09 PM Christian König > wrote: > > > > Am 08.03.23 um 15:43 schrieb Karol Herbst: > > > [SNIP] > > > "further"? There was no discussion at all, > > > > Yeah, well that is exactly what I wanted to archive. > > > > > you just started off like > > > that. If you think somebody misses that connection, you can point out > > > to documentation/videos whatever so the contributor can understand > > > what's wrong with an approach. You did that, so that's fine. It's just > > > starting off _any_ discussion with a "Well complete NAK" is terrible > > > style. I'd feel uncomfortable if that happened to me and I'm sure > > > there are enough people like that that we should be more reasonable > > > with our replies. Just.. don't. > > > > > > We are all humans here and people react negatively to such things. And > > > if people do it on purpose it just makes it worse. > > > > I completely see your point, I just don't know how to improve it. > > > > I don't stop people like this because I want to make them uncomfortable > > but because I want to prevent further discussions on that topic. > > > > In other words how can I make people notice that this is something > > fundamental while still being polite? Ask them to improve the docs. Gets them on board, and for bonus point you - can check they actually get it when you review the doc patch - get scheduler docs for free - have an easily pasteable link for next time around instead of just an aggressive NAK that helps no one really (aside from getting people boiling). It's not really about being polite but making sure that efficient communiction happens and that you don't have to repeat yourself. In rare cases you get to type the docs themself when people are too dense to learn (like what I had to do with the various dma_fence docs). > I think a little improvement over this would be to at least wait a few > replies before resorting to those strong statements. Just before it > becomes a risk in just wasting time. See above what I'm trying to do. When the message doesn't sink in as either a proper doc patch or when linking to the doc patch for next time around (because let's face it, this entire concept of "dma_fence committed for execution" is extremely trick, there will be repeations of this question until we've sunset dma_fence, which is probably decades away). If the learning does not happen, then it's the time to whack the big hammer (and if people don't get it, you can escalate to Dave, we have tools to make sure people get the message). But this really should be the end, not the start of the escalation chain :-) Cheers, Daniel > > > This is clearly going against the idea of having jobs only depend on > > fences and nothing else which is mandatory for correct memory > > management. > > > > >>> I'm sure it's all documented and there is a design document on how > > >>> things have to look like you can point out? Might help to get a better > > >>> understanding on how things should be. > > >> Yeah, that's the problematic part. We have documented this very > > >> extensively: > > >> https://www.kernel.org/doc/html/v5.9/driver-api/dma-buf.html#indefinite-dma-fences > > >> > > >> And both Jason and Daniel gave talks about the underlying problem and > > > fyi: > > > s/Jason/Faith/g > > > > +1. I wasn't aware of that. > > > > >> try to come up with patches to raise warnings when that happens, but > > >> people still keep coming up with the same idea over and over again. > > >> > > > Yes, and we'll have to tell them over and over again. Nothing wrong > > > with that. That's just part of maintaining such a big subsystem. And > > > that's definitely not a valid reason to phrase things like above. > > > > > >> It's just that the technical relationship between preventing jobs from > > >> running and with that preventing dma_fences from signaling and the core > > >> memory management with page faults and shrinkers waiting for those > > >> fences is absolutely not obvious. > > >> > > >> We had at least 10 different teams from different companies falling into > > >> the same trap already and either the patches were rejected of hand or > > >> had to painfully reverted or mitigated later on. > > >> > > > Sure, but that's just part of the job. And pointing out fundamental > > > mistakes early on is important, but the situation won't get any better > > > by being like that. Yes, we'll have to repeat the same words over and > > > over again, and yes that might be annoying, but that's just how it is. > > > > Well I have no problem explaining people why a solution doesn't work. > > > > But what usually happens is that people don't realize that they need to > > back of from a design and completely start over. > > > > Regards, > > Christian. > > > > > > > >> Regards, > > >> Christian. > > >> > > If the hw is busy with something you need to return the fence for this > >
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
On 10/03/2023 03.50, Faith Ekstrand wrote: > Jumping in here quick... (Sorry, I was out yesterday and was ignoring > my e-mail on Tuesday so I could finally type some compiler code.) > > On Thu, 2023-03-09 at 18:14 +0900, Asahi Lina wrote: >> On 09/03/2023 17.05, Christian König wrote: >>> Am 09.03.23 um 07:30 schrieb Asahi Lina: On 09/03/2023 05.14, Christian König wrote: >> I think you mean wake_up_interruptible(). That would be >> drm_sched_job_done(), on the fence callback when a job >> completes, which >> as I keep saying is the same logic used for >> hw_rq_count/hw_submission_limit tracking. > As the documentation to wait_event says: > > * wake_up() has to be called after changing any variable > that could > * change the result of the wait condition. > > So what you essentially try to do here is to skip that and say > drm_sched_job_done() would call that anyway, but when you read > any > variable to determine that state then as far as I can see > nothing is > guarantying that order. The driver needs to guarantee that any changes to that state precede a job completion fence signal of course, that's the entire idea of the API. It's supposed to represent a check for per-scheduler (or more specific, but not more global) resources that are released on job completion. Of course if you misuse the API you could cause a problem, but what I'm trying to say is that the API as designed and when used as intended does work properly. Put another way: job completions always need to cause the sched main loop to run an iteration anyway (otherwise we wouldn't make forward progress), and job completions are exactly the signal that the can_run_job() condition may have changed. > The only other possibility how you could use the callback > correctly > would be to call drm_fence_is_signaled() to query the state of > your hw > submission from the same fence which is then signaled. But then > the > question is once more why you don't give that fence directly to > the > scheduler? But the driver is supposed to guarantee that the ordering is always 1. resources freed, 2. fence signaled. So you don't need to check for the fence, you can just check for the resource state. >>> >>> Yeah, but this is exactly what the dma_fence framework tried to >>> prevent. >>> We try very hard to avoid such side channel signaling :) >> >> Right, and it's fine, I can use the fences directly easily enough. >> I'm >> just trying to explain why my original idea works too, even if it's >> not >> the best solution for other reasons! >> >> Of course I don't have the context of what other drivers are doing or >> did historically and what the pitfalls are, so I can't know what the >> "right" solution for any of this is in that context. I did my best to >> understand the drm_sched code and come up with a solution that works >> (which it does) without any more info. When I saw the hw submission >> limit stuff, I thought "okay, I need the same thing but with slightly >> more complex logic, so let's add a callback so the driver can >> customize >> it and do its own inflight counting". > > So, I think there's a difference here between "impossible to implement > correctly", "likely to be implemented correctly", and "impossible to > implement incorrectly". It's obviously possible to implement > correctly. You can just always return true or do exactly the same > check or do some simple thing where you can guarantee that it will only > ever return false when there's a bunch of other stuff in the queue. > That doesn't mean that it's likely to be implemented correctly by some > other driver. Some idiot will come along and try to take advantage of > it and cause themselves horrible problems. > > And, to be clear, for the purposes of this discussion, we're ALL > idiots, myself included. If there's one thing the DRM community has > learned over the years, it's that drivers are so complex that we all > turn into idiots at some point, relative to the complexity of the code > and hardware behavior. That's why things like dma_fence are written so > incredibly defensively and why we're so harsh about the rules. It's > the rules and not our individual smarts that keep us from making > mistakes. (Kinda like Rust, in a way.) So while I appreciate the > frustration of "I'm just trying to do something that's clearly correct > here", that doesn't mean that then next person to come by and fix a bug > by tweaking that callback isn't going to screw it up irreparably. That > person may even be you in 6 to 12 months after this e-mail thread is a > distant memory. > > So, yes, does the implementation you have today work without deadlocks > or starvation? Maybe it does. I've not verified. Is the suggested > callback a giant foot-gun
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
Jumping in here quick... (Sorry, I was out yesterday and was ignoring my e-mail on Tuesday so I could finally type some compiler code.) On Thu, 2023-03-09 at 18:14 +0900, Asahi Lina wrote: > On 09/03/2023 17.05, Christian König wrote: > > Am 09.03.23 um 07:30 schrieb Asahi Lina: > > > On 09/03/2023 05.14, Christian König wrote: > > > > > I think you mean wake_up_interruptible(). That would be > > > > > drm_sched_job_done(), on the fence callback when a job > > > > > completes, which > > > > > as I keep saying is the same logic used for > > > > > hw_rq_count/hw_submission_limit tracking. > > > > As the documentation to wait_event says: > > > > > > > > * wake_up() has to be called after changing any variable > > > > that could > > > > * change the result of the wait condition. > > > > > > > > So what you essentially try to do here is to skip that and say > > > > drm_sched_job_done() would call that anyway, but when you read > > > > any > > > > variable to determine that state then as far as I can see > > > > nothing is > > > > guarantying that order. > > > The driver needs to guarantee that any changes to that state > > > precede a > > > job completion fence signal of course, that's the entire idea of > > > the > > > API. It's supposed to represent a check for per-scheduler (or > > > more > > > specific, but not more global) resources that are released on job > > > completion. Of course if you misuse the API you could cause a > > > problem, > > > but what I'm trying to say is that the API as designed and when > > > used as > > > intended does work properly. > > > > > > Put another way: job completions always need to cause the sched > > > main > > > loop to run an iteration anyway (otherwise we wouldn't make > > > forward > > > progress), and job completions are exactly the signal that the > > > can_run_job() condition may have changed. > > > > > > > The only other possibility how you could use the callback > > > > correctly > > > > would be to call drm_fence_is_signaled() to query the state of > > > > your hw > > > > submission from the same fence which is then signaled. But then > > > > the > > > > question is once more why you don't give that fence directly to > > > > the > > > > scheduler? > > > But the driver is supposed to guarantee that the ordering is > > > always 1. > > > resources freed, 2. fence signaled. So you don't need to check > > > for the > > > fence, you can just check for the resource state. > > > > Yeah, but this is exactly what the dma_fence framework tried to > > prevent. > > We try very hard to avoid such side channel signaling :) > > Right, and it's fine, I can use the fences directly easily enough. > I'm > just trying to explain why my original idea works too, even if it's > not > the best solution for other reasons! > > Of course I don't have the context of what other drivers are doing or > did historically and what the pitfalls are, so I can't know what the > "right" solution for any of this is in that context. I did my best to > understand the drm_sched code and come up with a solution that works > (which it does) without any more info. When I saw the hw submission > limit stuff, I thought "okay, I need the same thing but with slightly > more complex logic, so let's add a callback so the driver can > customize > it and do its own inflight counting". So, I think there's a difference here between "impossible to implement correctly", "likely to be implemented correctly", and "impossible to implement incorrectly". It's obviously possible to implement correctly. You can just always return true or do exactly the same check or do some simple thing where you can guarantee that it will only ever return false when there's a bunch of other stuff in the queue. That doesn't mean that it's likely to be implemented correctly by some other driver. Some idiot will come along and try to take advantage of it and cause themselves horrible problems. And, to be clear, for the purposes of this discussion, we're ALL idiots, myself included. If there's one thing the DRM community has learned over the years, it's that drivers are so complex that we all turn into idiots at some point, relative to the complexity of the code and hardware behavior. That's why things like dma_fence are written so incredibly defensively and why we're so harsh about the rules. It's the rules and not our individual smarts that keep us from making mistakes. (Kinda like Rust, in a way.) So while I appreciate the frustration of "I'm just trying to do something that's clearly correct here", that doesn't mean that then next person to come by and fix a bug by tweaking that callback isn't going to screw it up irreparably. That person may even be you in 6 to 12 months after this e-mail thread is a distant memory. So, yes, does the implementation you have today work without deadlocks or starvation? Maybe it does. I've not verified. Is the suggested callback a giant foot-gun in the already treacherous
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
On 09/03/2023 17.05, Christian König wrote: > Am 09.03.23 um 07:30 schrieb Asahi Lina: >> On 09/03/2023 05.14, Christian König wrote: I think you mean wake_up_interruptible(). That would be drm_sched_job_done(), on the fence callback when a job completes, which as I keep saying is the same logic used for hw_rq_count/hw_submission_limit tracking. >>> As the documentation to wait_event says: >>> >>> * wake_up() has to be called after changing any variable that could >>> * change the result of the wait condition. >>> >>> So what you essentially try to do here is to skip that and say >>> drm_sched_job_done() would call that anyway, but when you read any >>> variable to determine that state then as far as I can see nothing is >>> guarantying that order. >> The driver needs to guarantee that any changes to that state precede a >> job completion fence signal of course, that's the entire idea of the >> API. It's supposed to represent a check for per-scheduler (or more >> specific, but not more global) resources that are released on job >> completion. Of course if you misuse the API you could cause a problem, >> but what I'm trying to say is that the API as designed and when used as >> intended does work properly. >> >> Put another way: job completions always need to cause the sched main >> loop to run an iteration anyway (otherwise we wouldn't make forward >> progress), and job completions are exactly the signal that the >> can_run_job() condition may have changed. >> >>> The only other possibility how you could use the callback correctly >>> would be to call drm_fence_is_signaled() to query the state of your hw >>> submission from the same fence which is then signaled. But then the >>> question is once more why you don't give that fence directly to the >>> scheduler? >> But the driver is supposed to guarantee that the ordering is always 1. >> resources freed, 2. fence signaled. So you don't need to check for the >> fence, you can just check for the resource state. > > Yeah, but this is exactly what the dma_fence framework tried to prevent. > We try very hard to avoid such side channel signaling :) Right, and it's fine, I can use the fences directly easily enough. I'm just trying to explain why my original idea works too, even if it's not the best solution for other reasons! Of course I don't have the context of what other drivers are doing or did historically and what the pitfalls are, so I can't know what the "right" solution for any of this is in that context. I did my best to understand the drm_sched code and come up with a solution that works (which it does) without any more info. When I saw the hw submission limit stuff, I thought "okay, I need the same thing but with slightly more complex logic, so let's add a callback so the driver can customize it and do its own inflight counting". After this discussion, I can see that this is equivalent to doing the same check in prepare_job() followed by returning the oldest running job's fence (as long as there's no race there... it should be fine if the fence reference is taken first, before the resource check, or if everything is done within the same critical section taking the firmware queue lock), so I'm happy to switch to that and drop this patch. But keep in mind none of this is documented, and there's no way for us driver authors to understand what we're supposed to do without documentation. As I said I spent a long time trying to understand drm_sched, and then my original attempt missed the drm_sched_fini() issue with dangling jobs and Alyssa managed to hit an oops on the test branch, I guessed what the problem was from her trace, figured out a way to reproduce it (the kill-loop glmark2 thing), and fixed it in the next patch in this series. So even trying my best to figure out how to do this, reading the code and what scarce docs there are, I managed to miss something that caused a potential oops on the first try. If I can't even get the API usage right after spending hours on it trying really hard not to (because it's not just about my driver, I need the Rust abstraction to be safe for any driver), there's no way I'm going to divine what approaches to resource/dependency signaling are problematic/easy to abuse... the most I can hope for is "I got the wrapper right and the API/driver interaction is correct and guarantees forward progress if the driver follows the rules". So when I submit something, and you reply with "Well complete NAK", that's just not nice. Honestly, I was kind of upset when I got that email. It sounded as if you were saying my solution was completely broken and couldn't work, but no matter how I looked at it I couldn't figure out how it's broken. And then it took several emails to even understand what you were suggesting with the prepare_job callback (and yes, that works too and is probably harder to abuse than a new callback). I'm trying really hard to make this all work and be correct, and of course I make
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
Am 09.03.23 um 07:30 schrieb Asahi Lina: On 09/03/2023 05.14, Christian König wrote: I think you mean wake_up_interruptible(). That would be drm_sched_job_done(), on the fence callback when a job completes, which as I keep saying is the same logic used for hw_rq_count/hw_submission_limit tracking. As the documentation to wait_event says: * wake_up() has to be called after changing any variable that could * change the result of the wait condition. So what you essentially try to do here is to skip that and say drm_sched_job_done() would call that anyway, but when you read any variable to determine that state then as far as I can see nothing is guarantying that order. The driver needs to guarantee that any changes to that state precede a job completion fence signal of course, that's the entire idea of the API. It's supposed to represent a check for per-scheduler (or more specific, but not more global) resources that are released on job completion. Of course if you misuse the API you could cause a problem, but what I'm trying to say is that the API as designed and when used as intended does work properly. Put another way: job completions always need to cause the sched main loop to run an iteration anyway (otherwise we wouldn't make forward progress), and job completions are exactly the signal that the can_run_job() condition may have changed. The only other possibility how you could use the callback correctly would be to call drm_fence_is_signaled() to query the state of your hw submission from the same fence which is then signaled. But then the question is once more why you don't give that fence directly to the scheduler? But the driver is supposed to guarantee that the ordering is always 1. resources freed, 2. fence signaled. So you don't need to check for the fence, you can just check for the resource state. Yeah, but this is exactly what the dma_fence framework tried to prevent. We try very hard to avoid such side channel signaling :) But putting that issue aside for a moment. What I don't get is when you have such intra queue dependencies, then why can't you check that at a much higher level? In other words even userspace should be able to predict that for it's submissions X amount of resources are needed and when all of my submissions run in parallel that won't work. Asking the firmware for a status is usually a magnitudes slower than just computing it before submission. Regards, Christian. If the callback returns false then by definition the fence wasn't yet signaled at some point during its execution (because the resources weren't yet freed), and since it would be in the wait_event_interruptible() check path, by definition the fence signaling at any point during or after the check would cause the thread to wake up again and re-check. Thread 1 Thread 2 1. wait_event_interruptible() arms wq 1. Free resources 2. can_run_job() checks resources 2. Signal fence 3. wait_event_interruptible() sleeps on wq3. Fence wakes up wq 4. loop There is no possible interleaving of those sequences that leads to a lost event and the thread not waking up: - If T2.3 happens before T1.1, that means T2.1 happened earlier and T1.2 must return true. - If T2.3 happens after T1.1 but before T1.3, the wq code will ensure the wq does not sleep (or immediately wakes up) at T1.3 since it was signaled during the condition check, after the wq was armed. At the next check loop, T1.2 will then return true, since T2.1 already happened before T2.3. - If T2.3 happens during T1.3, the wq wakes up normally and does another check, and at that point T1.2 returns true. QED. ~~ Lina
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
On 09/03/2023 05.14, Christian König wrote: >> I think you mean wake_up_interruptible(). That would be >> drm_sched_job_done(), on the fence callback when a job completes, which >> as I keep saying is the same logic used for >> hw_rq_count/hw_submission_limit tracking. > > As the documentation to wait_event says: > > * wake_up() has to be called after changing any variable that could > * change the result of the wait condition. > > So what you essentially try to do here is to skip that and say > drm_sched_job_done() would call that anyway, but when you read any > variable to determine that state then as far as I can see nothing is > guarantying that order. The driver needs to guarantee that any changes to that state precede a job completion fence signal of course, that's the entire idea of the API. It's supposed to represent a check for per-scheduler (or more specific, but not more global) resources that are released on job completion. Of course if you misuse the API you could cause a problem, but what I'm trying to say is that the API as designed and when used as intended does work properly. Put another way: job completions always need to cause the sched main loop to run an iteration anyway (otherwise we wouldn't make forward progress), and job completions are exactly the signal that the can_run_job() condition may have changed. > The only other possibility how you could use the callback correctly > would be to call drm_fence_is_signaled() to query the state of your hw > submission from the same fence which is then signaled. But then the > question is once more why you don't give that fence directly to the > scheduler? But the driver is supposed to guarantee that the ordering is always 1. resources freed, 2. fence signaled. So you don't need to check for the fence, you can just check for the resource state. If the callback returns false then by definition the fence wasn't yet signaled at some point during its execution (because the resources weren't yet freed), and since it would be in the wait_event_interruptible() check path, by definition the fence signaling at any point during or after the check would cause the thread to wake up again and re-check. Thread 1 Thread 2 1. wait_event_interruptible() arms wq 1. Free resources 2. can_run_job() checks resources 2. Signal fence 3. wait_event_interruptible() sleeps on wq3. Fence wakes up wq 4. loop There is no possible interleaving of those sequences that leads to a lost event and the thread not waking up: - If T2.3 happens before T1.1, that means T2.1 happened earlier and T1.2 must return true. - If T2.3 happens after T1.1 but before T1.3, the wq code will ensure the wq does not sleep (or immediately wakes up) at T1.3 since it was signaled during the condition check, after the wq was armed. At the next check loop, T1.2 will then return true, since T2.1 already happened before T2.3. - If T2.3 happens during T1.3, the wq wakes up normally and does another check, and at that point T1.2 returns true. QED. ~~ Lina
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
Am 08.03.23 um 20:45 schrieb Asahi Lina: On 09/03/2023 04.12, Christian König wrote: Am 08.03.23 um 20:05 schrieb Asahi Lina: [SNIP] 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 can't happen, because it will just go into another iteration of the drm_sched main loop since there is an entity available still. Rather there is probably the opposite bug in this patch: the can_run_job logic should be moved into the wait_event_interruptible() condition check, otherwise I think it can end up busy-looping since the condition itself can be true even when the can_run_job check blocks it. But there is no risk of it going to sleep and never waking up because job completions will wake up the waitqueue by definition, and that happens after the driver-side queues are popped. If this problem could happen, then the existing hw_submission_limit logic would be broken in the same way. It is logically equivalent in how it works. Basically, if properly done in wait_event_interruptible, it is exactly the logic of that macro that prevents this race condition and makes everything work at all. Without it, drm_sched would be completely 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. It can never deadlock with this patch as it stands (though it could busy loop), and if properly moved into the wait_event_interruptible(), it would also never busy loop and work entirely as intended. The actual API change is sound. I don't know why you're trying so hard to convince everyone that this approach is fundamentally broken... It might be a bad idea for other reasons, it might encourage incorrect usage, it might not be the best option, there are plenty of arguments you can make... but you just keep trying to make an argument that it just can't work at all for some reason. Why? I already said I'm happy dropping it in favor of the fences... Well because it is broken. When you move the check into the wait_event_interruptible condition then who is going to call wait_event_interruptible when the condition changes? I think you mean wake_up_interruptible(). That would be drm_sched_job_done(), on the fence callback when a job completes, which as I keep saying is the same logic used for hw_rq_count/hw_submission_limit tracking. As the documentation to wait_event says: * wake_up() has to be called after changing any variable that could * change the result of the wait condition. So what you essentially try to do here is to skip that and say drm_sched_job_done() would call that anyway, but when you read any variable to determine that state then as far as I can see nothing is guarantying that order. The only other possibility how you could use the callback correctly would be to call drm_fence_is_signaled() to query the state of your hw submission from the same fence which is then signaled. But then the question is once more why you don't give that fence directly to the scheduler? Please think about it for a second, Yeah, I'm trying to really follow your intentions here. But that doesn't really makes sense. Either you are trying to do something invalid or you are trying to circumvent the object model somehow and add a shortcut for the signaling API. Both would be more than fishy. Regards, Christian. it's really not that complicated to see why it works: - Driver pops off completed commands <-- can_run_job condition satisfied - Driver signals fence - drm_sched_job_done_cb() - drm_sched_job_done() - atomic_dec(>hw_rq_count); <-- hw_submission_limit satisfied - ... - wake_up_interruptible(>wake_up_worker); ^- happens after both conditions are potentially satisfied It really is completely equivalent to just making the hw_rq_count logic customizable by the driver. The actual flow is the same. As long as the driver guarantees it satisfies the can_run_job() condition before signaling the completion fence that triggered that change, it works fine. As I said this idea came up before and was rejected multiple times. Maybe it was a different idea, or maybe it was rejected for other reasons, or maybe it was wrongly rejected for being broken when it isn't ^^ ~~ Lina
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
On 09/03/2023 04.12, Christian König wrote: > Am 08.03.23 um 20:05 schrieb Asahi Lina: >> [SNIP] >>> 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 can't happen, because it will just go into another iteration of the >> drm_sched main loop since there is an entity available still. >> >> Rather there is probably the opposite bug in this patch: the can_run_job >> logic should be moved into the wait_event_interruptible() condition >> check, otherwise I think it can end up busy-looping since the condition >> itself can be true even when the can_run_job check blocks it. >> >> But there is no risk of it going to sleep and never waking up because >> job completions will wake up the waitqueue by definition, and that >> happens after the driver-side queues are popped. If this problem could >> happen, then the existing hw_submission_limit logic would be broken in >> the same way. It is logically equivalent in how it works. >> >> Basically, if properly done in wait_event_interruptible, it is exactly >> the logic of that macro that prevents this race condition and makes >> everything work at all. Without it, drm_sched would be completely 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. >> It can never deadlock with this patch as it stands (though it could busy >> loop), and if properly moved into the wait_event_interruptible(), it >> would also never busy loop and work entirely as intended. The actual API >> change is sound. >> >> I don't know why you're trying so hard to convince everyone that this >> approach is fundamentally broken... It might be a bad idea for other >> reasons, it might encourage incorrect usage, it might not be the best >> option, there are plenty of arguments you can make... but you just keep >> trying to make an argument that it just can't work at all for some >> reason. Why? I already said I'm happy dropping it in favor of the fences... > > Well because it is broken. > > When you move the check into the wait_event_interruptible condition then > who is going to call wait_event_interruptible when the condition changes? I think you mean wake_up_interruptible(). That would be drm_sched_job_done(), on the fence callback when a job completes, which as I keep saying is the same logic used for hw_rq_count/hw_submission_limit tracking. Please think about it for a second, it's really not that complicated to see why it works: - Driver pops off completed commands <-- can_run_job condition satisfied - Driver signals fence - drm_sched_job_done_cb() - drm_sched_job_done() - atomic_dec(>hw_rq_count); <-- hw_submission_limit satisfied - ... - wake_up_interruptible(>wake_up_worker); ^- happens after both conditions are potentially satisfied It really is completely equivalent to just making the hw_rq_count logic customizable by the driver. The actual flow is the same. As long as the driver guarantees it satisfies the can_run_job() condition before signaling the completion fence that triggered that change, it works fine. > As I said this idea came up before and was rejected multiple times. Maybe it was a different idea, or maybe it was rejected for other reasons, or maybe it was wrongly rejected for being broken when it isn't ^^ ~~ Lina
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
Am 08.03.23 um 20:05 schrieb Asahi Lina: [SNIP] 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 can't happen, because it will just go into another iteration of the drm_sched main loop since there is an entity available still. Rather there is probably the opposite bug in this patch: the can_run_job logic should be moved into the wait_event_interruptible() condition check, otherwise I think it can end up busy-looping since the condition itself can be true even when the can_run_job check blocks it. But there is no risk of it going to sleep and never waking up because job completions will wake up the waitqueue by definition, and that happens after the driver-side queues are popped. If this problem could happen, then the existing hw_submission_limit logic would be broken in the same way. It is logically equivalent in how it works. Basically, if properly done in wait_event_interruptible, it is exactly the logic of that macro that prevents this race condition and makes everything work at all. Without it, drm_sched would be completely 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. It can never deadlock with this patch as it stands (though it could busy loop), and if properly moved into the wait_event_interruptible(), it would also never busy loop and work entirely as intended. The actual API change is sound. I don't know why you're trying so hard to convince everyone that this approach is fundamentally broken... It might be a bad idea for other reasons, it might encourage incorrect usage, it might not be the best option, there are plenty of arguments you can make... but you just keep trying to make an argument that it just can't work at all for some reason. Why? I already said I'm happy dropping it in favor of the fences... Well because it is broken. When you move the check into the wait_event_interruptible condition then who is going to call wait_event_interruptible when the condition changes? As I said this idea came up before and was rejected multiple times. Regards, Christian. It's intended to mirror the hw_submission_limit logic. If you think this is broken, then that's broken too. They are equivalent mechanisms. 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. In the current state I actually think it's not really that problematic, because the resources are acquired directly in the ioctl path. So that can block if starved, but if that can cause overall forward progress to stop because some fence doesn't get signaled, then so can just not doing the ioctl in the first place, so there's not much point (userspace can always misbehave with its fence usage...). By the time anything gets submitted to drm_sched, the resources are already guaranteed to be acquired, we never block in the run callback. It needs to be fixed of course, but if the threat model is a malicious GPU process, well, there are many other ways to DoS your system... and I don't think it's very likely that 63+ queues (which usually means 63+ processes with OpenGL) will end up accidentally starving the GPU in a tight loop at the same time. I'd love to hear about real-world scenarios where this kind of thing has been a real problem and not just
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
On 09/03/2023 02.57, Christian König wrote: > 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 can't happen, because it will just go into another iteration of the drm_sched main loop since there is an entity available still. Rather there is probably the opposite bug in this patch: the can_run_job logic should be moved into the wait_event_interruptible() condition check, otherwise I think it can end up busy-looping since the condition itself can be true even when the can_run_job check blocks it. But there is no risk of it going to sleep and never waking up because job completions will wake up the waitqueue by definition, and that happens after the driver-side queues are popped. If this problem could happen, then the existing hw_submission_limit logic would be broken in the same way. It is logically equivalent in how it works. Basically, if properly done in wait_event_interruptible, it is exactly the logic of that macro that prevents this race condition and makes everything work at all. Without it, drm_sched would be completely 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. It can never deadlock with this patch as it stands (though it could busy loop), and if properly moved into the wait_event_interruptible(), it would also never busy loop and work entirely as intended. The actual API change is sound. I don't know why you're trying so hard to convince everyone that this approach is fundamentally broken... It might be a bad idea for other reasons, it might encourage incorrect usage, it might not be the best option, there are plenty of arguments you can make... but you just keep trying to make an argument that it just can't work at all for some reason. Why? I already said I'm happy dropping it in favor of the fences... It's intended to mirror the hw_submission_limit logic. If you think this is broken, then that's broken too. They are equivalent mechanisms. >> 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
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
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: 1. To check for some userspace dependency (We don't need to discuss that, it's evil and we both know it). 2. 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
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
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. 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... * (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...) * 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... >>> Jason Ekstrand gave a create presentation on that problem a few years >>> ago on LPC. I strongly suggest you google that one up. >> Faith Ekstrand (it looks like you mistyped that name...) > > My fault I was really just mistyping that :) It's all good ^^ > > 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: > > 1. To check for some userspace dependency (We don't need to discuss > that, it's evil and we both know it). > > 2. 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 ^^ > 3. 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
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
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. Jason Ekstrand gave a create presentation on that problem a few years ago on LPC. I strongly suggest you google that one up. Faith Ekstrand (it looks like you mistyped that name...) My fault I was really just mistyping that :) is the person who proposed that I should use drm_sched in this way (see below), we've had a few private meetings about this design ^^ You can see the driver implementation of that callback in drivers/gpu/drm/asahi/queue/mod.rs (QueueJob::can_run()), which then calls into drivers/gpu/drm/asahi/workqueue.rs (Job::can_submit()) that does the actual available slot count checks. The can_run_job logic is written to mirror the hw_submission_limit logic (just a bit later in the sched main loop since we need to actually pick a job to do the check), and just like for that case, completion of any job in the same scheduler will cause another run of the main loop and another check (which is exactly what we want here). Yeah and that hw_submission_limit is based on a fence signaling again. I don't think so...? It's just an atomic that gets checked in drm_sched_ready(). There are no extra fences involved (other than the job completion fences that trigger another scheduler run). The idea is that when the hardware queue makes forward progress you check against the limit again and submit more jobs as needed. I'm doing the same exact thing, I'm just using more complex logic for the notion of in-flight queue limits! Then why can't you express that logic in a dependency fence? When you have some firmware limitation that a job needs resources which are currently in use by other submissions then those other submissions have fences as well and you can return those in the prepare_job callback. If those other submissions don't have fences, then you have a major design problem inside your driver and we need to get back to square one and talk about that dependency handling. I think we have a disconnect in our views of what is going on here... This hardware has firmware-side scheduling with an arbitrary (as far as I know) number of queues. There is one scheduler instance and one entity per userspace queue (not global!). These queues process jobs in some logical sequence, though at the firmware level they get split into up to three queues each (and there is some parallelism allowed). The limitation here is in the number of in-flight jobs per firmware queue, not global. So far I'm familiar with that design. There is no way for things to deadlock. If jobs have been submitted to the firmware queue, that means their dependencies were signaled already. Jobs have intra-job dependencies via driver barriers (which drm_sched knows nothing about), but the submission code in the driver guarantees that they are deadlock-free since you can only barrier on past commands, which by definition submit first. If a firmware queue is full, drm_sched blocks. Since it is full, that means it will run those commands (since they have no outside dependencies and they are already queued and ready to run by the firmware), eventually space will be freed, and each time a job completes drm_sched will do the can_run_job check again and decide whether to run a new job. Since the firmware queues contain commands which only have past-facing barriers on other already submitted commands, by definition they will become empty at some point as long as the firmware is making forward progress. And therefore, by definition, can_run_job will eventually return true at some point after a job completion fence is signaled (the one for the last job submitted prior). There is a check in the driver to ensure that we do not allow submissions which, by themselves, would exceed the queued command limit (we actually just limit to 64 commands overall right now, which is conservative but seems reasonable given the 128-per-firmware-queue limit). Well then again why don't you give that fence out as dependency? Is it because the scheduler tries to optimize those away? I get the feeling that you are conflating pending jobs with submitted jobs. This isn't about how many jobs you can have pending in drm_sched before running them or anything like that. Of course, at that point, arbitrary dependencies come into play and you can end up with deadlocks on dependency fences. But that's not the case here. What can_run_job is waiting on is guaranteed to make forward progress. 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: 1. To check for some
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
On Wed, Mar 8, 2023 at 4:09 PM Christian König wrote:
>
> Am 08.03.23 um 15:43 schrieb Karol Herbst:
> > [SNIP]
> > "further"? There was no discussion at all,
>
> Yeah, well that is exactly what I wanted to archive.
>
> > you just started off like
> > that. If you think somebody misses that connection, you can point out
> > to documentation/videos whatever so the contributor can understand
> > what's wrong with an approach. You did that, so that's fine. It's just
> > starting off _any_ discussion with a "Well complete NAK" is terrible
> > style. I'd feel uncomfortable if that happened to me and I'm sure
> > there are enough people like that that we should be more reasonable
> > with our replies. Just.. don't.
> >
> > We are all humans here and people react negatively to such things. And
> > if people do it on purpose it just makes it worse.
>
> I completely see your point, I just don't know how to improve it.
>
> I don't stop people like this because I want to make them uncomfortable
> but because I want to prevent further discussions on that topic.
>
> In other words how can I make people notice that this is something
> fundamental while still being polite?
>
I think a little improvement over this would be to at least wait a few
replies before resorting to those strong statements. Just before it
becomes a risk in just wasting time.
> This is clearly going against the idea of having jobs only depend on
> fences and nothing else which is mandatory for correct memory management.
>
> >>> I'm sure it's all documented and there is a design document on how
> >>> things have to look like you can point out? Might help to get a better
> >>> understanding on how things should be.
> >> Yeah, that's the problematic part. We have documented this very
> >> extensively:
> >> https://www.kernel.org/doc/html/v5.9/driver-api/dma-buf.html#indefinite-dma-fences
> >>
> >> And both Jason and Daniel gave talks about the underlying problem and
> > fyi:
> > s/Jason/Faith/g
>
> +1. I wasn't aware of that.
>
> >> try to come up with patches to raise warnings when that happens, but
> >> people still keep coming up with the same idea over and over again.
> >>
> > Yes, and we'll have to tell them over and over again. Nothing wrong
> > with that. That's just part of maintaining such a big subsystem. And
> > that's definitely not a valid reason to phrase things like above.
> >
> >> It's just that the technical relationship between preventing jobs from
> >> running and with that preventing dma_fences from signaling and the core
> >> memory management with page faults and shrinkers waiting for those
> >> fences is absolutely not obvious.
> >>
> >> We had at least 10 different teams from different companies falling into
> >> the same trap already and either the patches were rejected of hand or
> >> had to painfully reverted or mitigated later on.
> >>
> > Sure, but that's just part of the job. And pointing out fundamental
> > mistakes early on is important, but the situation won't get any better
> > by being like that. Yes, we'll have to repeat the same words over and
> > over again, and yes that might be annoying, but that's just how it is.
>
> Well I have no problem explaining people why a solution doesn't work.
>
> But what usually happens is that people don't realize that they need to
> back of from a design and completely start over.
>
> Regards,
> Christian.
>
> >
> >> Regards,
> >> Christian.
> >>
> If the hw is busy with something you need to return the fence for this
> from the prepare_job callback so that the scheduler can be notified when
> the hw is available again.
>
> Regards,
> Christian.
>
> > Signed-off-by: Asahi Lina
> > ---
> > drivers/gpu/drm/scheduler/sched_main.c | 10 ++
> > include/drm/gpu_scheduler.h| 8
> > 2 files changed, 18 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> > b/drivers/gpu/drm/scheduler/sched_main.c
> > index 4e6ad6e122bc..5c0add2c7546 100644
> > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > @@ -1001,6 +1001,16 @@ static int drm_sched_main(void *param)
> > if (!entity)
> > continue;
> >
> > + if (sched->ops->can_run_job) {
> > + sched_job =
> > to_drm_sched_job(spsc_queue_peek(>job_queue));
> > + if (!sched_job) {
> > + complete_all(>entity_idle);
> > + continue;
> > + }
> > + if (!sched->ops->can_run_job(sched_job))
> > + continue;
> > + }
> > +
> > sched_job = drm_sched_entity_pop_job(entity);
> >
> > if (!sched_job) {
> > diff --git
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
Am 08.03.23 um 15:43 schrieb Karol Herbst:
[SNIP]
"further"? There was no discussion at all,
Yeah, well that is exactly what I wanted to archive.
you just started off like
that. If you think somebody misses that connection, you can point out
to documentation/videos whatever so the contributor can understand
what's wrong with an approach. You did that, so that's fine. It's just
starting off _any_ discussion with a "Well complete NAK" is terrible
style. I'd feel uncomfortable if that happened to me and I'm sure
there are enough people like that that we should be more reasonable
with our replies. Just.. don't.
We are all humans here and people react negatively to such things. And
if people do it on purpose it just makes it worse.
I completely see your point, I just don't know how to improve it.
I don't stop people like this because I want to make them uncomfortable
but because I want to prevent further discussions on that topic.
In other words how can I make people notice that this is something
fundamental while still being polite?
This is clearly going against the idea of having jobs only depend on
fences and nothing else which is mandatory for correct memory management.
I'm sure it's all documented and there is a design document on how
things have to look like you can point out? Might help to get a better
understanding on how things should be.
Yeah, that's the problematic part. We have documented this very
extensively:
https://www.kernel.org/doc/html/v5.9/driver-api/dma-buf.html#indefinite-dma-fences
And both Jason and Daniel gave talks about the underlying problem and
fyi:
s/Jason/Faith/g
+1. I wasn't aware of that.
try to come up with patches to raise warnings when that happens, but
people still keep coming up with the same idea over and over again.
Yes, and we'll have to tell them over and over again. Nothing wrong
with that. That's just part of maintaining such a big subsystem. And
that's definitely not a valid reason to phrase things like above.
It's just that the technical relationship between preventing jobs from
running and with that preventing dma_fences from signaling and the core
memory management with page faults and shrinkers waiting for those
fences is absolutely not obvious.
We had at least 10 different teams from different companies falling into
the same trap already and either the patches were rejected of hand or
had to painfully reverted or mitigated later on.
Sure, but that's just part of the job. And pointing out fundamental
mistakes early on is important, but the situation won't get any better
by being like that. Yes, we'll have to repeat the same words over and
over again, and yes that might be annoying, but that's just how it is.
Well I have no problem explaining people why a solution doesn't work.
But what usually happens is that people don't realize that they need to
back of from a design and completely start over.
Regards,
Christian.
Regards,
Christian.
If the hw is busy with something you need to return the fence for this
from the prepare_job callback so that the scheduler can be notified when
the hw is available again.
Regards,
Christian.
Signed-off-by: Asahi Lina
---
drivers/gpu/drm/scheduler/sched_main.c | 10 ++
include/drm/gpu_scheduler.h| 8
2 files changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
index 4e6ad6e122bc..5c0add2c7546 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1001,6 +1001,16 @@ static int drm_sched_main(void *param)
if (!entity)
continue;
+ if (sched->ops->can_run_job) {
+ sched_job =
to_drm_sched_job(spsc_queue_peek(>job_queue));
+ if (!sched_job) {
+ complete_all(>entity_idle);
+ continue;
+ }
+ if (!sched->ops->can_run_job(sched_job))
+ continue;
+ }
+
sched_job = drm_sched_entity_pop_job(entity);
if (!sched_job) {
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 9db9e5e504ee..bd89ea9507b9 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -396,6 +396,14 @@ struct drm_sched_backend_ops {
struct dma_fence *(*prepare_job)(struct drm_sched_job *sched_job,
struct drm_sched_entity *s_entity);
+ /**
+ * @can_run_job: Called before job execution to check whether the
+ * hardware is free enough to run the job. This can be used to
+ * implement more complex hardware resource policies than the
+ * hw_submission limit.
+ */
+ bool (*can_run_job)(struct drm_sched_job *sched_job);
+
/**
* @run_job: Called to execute the job once all of the
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
On 08/03/2023 19.00, Christian König wrote: > Am 08.03.23 um 10:41 schrieb Asahi Lina: >> On 08/03/2023 17.46, Christian König wrote: >>> Am 07.03.23 um 15:25 schrieb Asahi Lina: Some hardware may require more complex resource utilization accounting than the simple job count supported by drm_sched internally. Add a can_run_job callback to allow drivers to implement more logic before deciding whether to run a GPU job. >>> Well complete NAK. >>> >>> This is clearly going against the idea of having jobs only depend on >>> fences and nothing else which is mandatory for correct memory management. >>> >>> If the hw is busy with something you need to return the fence for this >>> from the prepare_job callback so that the scheduler can be notified when >>> the hw is available again. >> I think you misunderstood the intent here... This isn't about job >> dependencies, it's about in-flight resource limits. >> >> drm_sched already has a hw_submission_limit that specifies the number of >> submissions that can be in flight, but that doesn't work for us because >> each job from drm_sched's point of view consists of multiple commands >> split among 3 firmware queues. The firmware can only support up to 128 >> work commands in flight per queue (barriers don't count), otherwise it >> overflows a fixed-size buffer. >> >> So we need more complex accounting of how many underlying commands are >> in flight per queue to determine whether it is safe to run a new job, >> and that is what this callback accomplishes. This has to happen even >> when individual jobs have no buffer/resource dependencies between them >> (which is what the fences would express). > > Yeah, I already assumed that you have something like this. > > And to make it clear this is unfortunately a complete NAK to this > approach! You can't do this! I think you still have some significant misconception about how this driver works and uses drm_sched... I would appreciate it if you listen and try to understand the design before giving hard NAKs... (this isn't a Radeon) > 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... > Jason Ekstrand gave a create presentation on that problem a few years > ago on LPC. I strongly suggest you google that one up. Faith Ekstrand (it looks like you mistyped that name...) is the person who proposed that I should use drm_sched in this way (see below), we've had a few private meetings about this design ^^ >> You can see the driver implementation of that callback in >> drivers/gpu/drm/asahi/queue/mod.rs (QueueJob::can_run()), which then >> calls into drivers/gpu/drm/asahi/workqueue.rs (Job::can_submit()) that >> does the actual available slot count checks. >> >> The can_run_job logic is written to mirror the hw_submission_limit logic >> (just a bit later in the sched main loop since we need to actually pick >> a job to do the check), and just like for that case, completion of any >> job in the same scheduler will cause another run of the main loop and >> another check (which is exactly what we want here). > > Yeah and that hw_submission_limit is based on a fence signaling again. I don't think so...? It's just an atomic that gets checked in drm_sched_ready(). There are no extra fences involved (other than the job completion fences that trigger another scheduler run). The idea is that when the hardware queue makes forward progress you check against the limit again and submit more jobs as needed. I'm doing the same exact thing, I'm just using more complex logic for the notion of in-flight queue limits! > When you have some firmware limitation that a job needs resources which > are currently in use by other submissions then those other submissions > have fences as well and you can return those in the prepare_job callback. > > If those other submissions don't have fences, then you have a major > design problem inside your driver and we need to get back to square one > and talk about that dependency handling. I think we have a disconnect in our views of what is going on here... This hardware has firmware-side scheduling with an arbitrary (as far as I know) number of queues. There is one scheduler instance and one entity per userspace queue (not global!). These queues process jobs in some logical sequence, though at the firmware level they get split into up to three queues each (and there is some parallelism allowed). The limitation here is in the number of in-flight jobs per firmware queue, not global. There is no way for things to deadlock. If jobs have been submitted to the firmware queue, that means their dependencies were signaled already. Jobs have intra-job dependencies via driver barriers (which drm_sched knows
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
On Wed, Mar 8, 2023 at 2:47 PM Christian König wrote: > > Am 08.03.23 um 13:39 schrieb Karol Herbst: > > On Wed, Mar 8, 2023 at 9:46 AM Christian König > > wrote: > >> Am 07.03.23 um 15:25 schrieb Asahi Lina: > >>> Some hardware may require more complex resource utilization accounting > >>> than the simple job count supported by drm_sched internally. Add a > >>> can_run_job callback to allow drivers to implement more logic before > >>> deciding whether to run a GPU job. > >> Well complete NAK. > >> > > There hasn't even been any kind of discussion yet you already come > > around with a "Well complete NAK" > > > > First, this can be seen as rude behavior and me being part of the drm > > community I don't want to have to see this kind of thing. > > > > Obviously, any kind of strong "technical" review point is a nak until > > people settle with an agreement on what to land, there is no point in > > pointing out a "NAK", especially if that's the first thing you say. If > > you want to express your strong disagreement with the proposed > > solution, then state what your pain points are directly. > > > > If there is a long discussion and a maintainer feels it's going > > nowhere and no conclusion will be reached it might be this kind of > > "speaking with authority" point has to be made. But not as the starter > > into a discussion. This is unnecessarily hostile towards the > > contributor. And I wished we wouldn't have to see this kind of > > behavior here. > > > > Yes, some kernel maintainers do this a lot, but kernel maintainers > > also have this kind of reputation and people don't want to have to > > deal with this nonsense and decide to not contribute at all. So please > > just drop this attitude. > > Yes, you are completely right with that, but getting this message to the > recipient is intentional on my side. > > I give completely NAKs when the author of a patch has missed such a > fundamental technical connection that further discussion doesn't make sense. > > It's not meant to be in any way rude or offending. I can put a smiley > behind it if it somehow helps, but we still need a way to raise this big > red stop sign. > "further"? There was no discussion at all, you just started off like that. If you think somebody misses that connection, you can point out to documentation/videos whatever so the contributor can understand what's wrong with an approach. You did that, so that's fine. It's just starting off _any_ discussion with a "Well complete NAK" is terrible style. I'd feel uncomfortable if that happened to me and I'm sure there are enough people like that that we should be more reasonable with our replies. Just.. don't. We are all humans here and people react negatively to such things. And if people do it on purpose it just makes it worse. > >> This is clearly going against the idea of having jobs only depend on > >> fences and nothing else which is mandatory for correct memory management. > >> > > I'm sure it's all documented and there is a design document on how > > things have to look like you can point out? Might help to get a better > > understanding on how things should be. > > Yeah, that's the problematic part. We have documented this very > extensively: > https://www.kernel.org/doc/html/v5.9/driver-api/dma-buf.html#indefinite-dma-fences > > And both Jason and Daniel gave talks about the underlying problem and fyi: s/Jason/Faith/g > try to come up with patches to raise warnings when that happens, but > people still keep coming up with the same idea over and over again. > Yes, and we'll have to tell them over and over again. Nothing wrong with that. That's just part of maintaining such a big subsystem. And that's definitely not a valid reason to phrase things like above. > It's just that the technical relationship between preventing jobs from > running and with that preventing dma_fences from signaling and the core > memory management with page faults and shrinkers waiting for those > fences is absolutely not obvious. > > We had at least 10 different teams from different companies falling into > the same trap already and either the patches were rejected of hand or > had to painfully reverted or mitigated later on. > Sure, but that's just part of the job. And pointing out fundamental mistakes early on is important, but the situation won't get any better by being like that. Yes, we'll have to repeat the same words over and over again, and yes that might be annoying, but that's just how it is. > Regards, > Christian. > > > > >> If the hw is busy with something you need to return the fence for this > >> from the prepare_job callback so that the scheduler can be notified when > >> the hw is available again. > >> > >> Regards, > >> Christian. > >> > >>> Signed-off-by: Asahi Lina > >>> --- > >>>drivers/gpu/drm/scheduler/sched_main.c | 10 ++ > >>>include/drm/gpu_scheduler.h| 8 > >>>2 files changed, 18 insertions(+) > >>> > >>> diff --git
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
Am 08.03.23 um 13:39 schrieb Karol Herbst:
On Wed, Mar 8, 2023 at 9:46 AM Christian König wrote:
Am 07.03.23 um 15:25 schrieb Asahi Lina:
Some hardware may require more complex resource utilization accounting
than the simple job count supported by drm_sched internally. Add a
can_run_job callback to allow drivers to implement more logic before
deciding whether to run a GPU job.
Well complete NAK.
There hasn't even been any kind of discussion yet you already come
around with a "Well complete NAK"
First, this can be seen as rude behavior and me being part of the drm
community I don't want to have to see this kind of thing.
Obviously, any kind of strong "technical" review point is a nak until
people settle with an agreement on what to land, there is no point in
pointing out a "NAK", especially if that's the first thing you say. If
you want to express your strong disagreement with the proposed
solution, then state what your pain points are directly.
If there is a long discussion and a maintainer feels it's going
nowhere and no conclusion will be reached it might be this kind of
"speaking with authority" point has to be made. But not as the starter
into a discussion. This is unnecessarily hostile towards the
contributor. And I wished we wouldn't have to see this kind of
behavior here.
Yes, some kernel maintainers do this a lot, but kernel maintainers
also have this kind of reputation and people don't want to have to
deal with this nonsense and decide to not contribute at all. So please
just drop this attitude.
Yes, you are completely right with that, but getting this message to the
recipient is intentional on my side.
I give completely NAKs when the author of a patch has missed such a
fundamental technical connection that further discussion doesn't make sense.
It's not meant to be in any way rude or offending. I can put a smiley
behind it if it somehow helps, but we still need a way to raise this big
red stop sign.
This is clearly going against the idea of having jobs only depend on
fences and nothing else which is mandatory for correct memory management.
I'm sure it's all documented and there is a design document on how
things have to look like you can point out? Might help to get a better
understanding on how things should be.
Yeah, that's the problematic part. We have documented this very
extensively:
https://www.kernel.org/doc/html/v5.9/driver-api/dma-buf.html#indefinite-dma-fences
And both Jason and Daniel gave talks about the underlying problem and
try to come up with patches to raise warnings when that happens, but
people still keep coming up with the same idea over and over again.
It's just that the technical relationship between preventing jobs from
running and with that preventing dma_fences from signaling and the core
memory management with page faults and shrinkers waiting for those
fences is absolutely not obvious.
We had at least 10 different teams from different companies falling into
the same trap already and either the patches were rejected of hand or
had to painfully reverted or mitigated later on.
Regards,
Christian.
If the hw is busy with something you need to return the fence for this
from the prepare_job callback so that the scheduler can be notified when
the hw is available again.
Regards,
Christian.
Signed-off-by: Asahi Lina
---
drivers/gpu/drm/scheduler/sched_main.c | 10 ++
include/drm/gpu_scheduler.h| 8
2 files changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
index 4e6ad6e122bc..5c0add2c7546 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1001,6 +1001,16 @@ static int drm_sched_main(void *param)
if (!entity)
continue;
+ if (sched->ops->can_run_job) {
+ sched_job =
to_drm_sched_job(spsc_queue_peek(>job_queue));
+ if (!sched_job) {
+ complete_all(>entity_idle);
+ continue;
+ }
+ if (!sched->ops->can_run_job(sched_job))
+ continue;
+ }
+
sched_job = drm_sched_entity_pop_job(entity);
if (!sched_job) {
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 9db9e5e504ee..bd89ea9507b9 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -396,6 +396,14 @@ struct drm_sched_backend_ops {
struct dma_fence *(*prepare_job)(struct drm_sched_job *sched_job,
struct drm_sched_entity *s_entity);
+ /**
+ * @can_run_job: Called before job execution to check whether the
+ * hardware is free enough to run the job. This can be used to
+ * implement more complex hardware resource policies than the
+ * hw_submission limit.
+
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
On Wed, Mar 8, 2023 at 9:46 AM Christian König wrote:
>
> Am 07.03.23 um 15:25 schrieb Asahi Lina:
> > Some hardware may require more complex resource utilization accounting
> > than the simple job count supported by drm_sched internally. Add a
> > can_run_job callback to allow drivers to implement more logic before
> > deciding whether to run a GPU job.
>
> Well complete NAK.
>
There hasn't even been any kind of discussion yet you already come
around with a "Well complete NAK"
First, this can be seen as rude behavior and me being part of the drm
community I don't want to have to see this kind of thing.
Obviously, any kind of strong "technical" review point is a nak until
people settle with an agreement on what to land, there is no point in
pointing out a "NAK", especially if that's the first thing you say. If
you want to express your strong disagreement with the proposed
solution, then state what your pain points are directly.
If there is a long discussion and a maintainer feels it's going
nowhere and no conclusion will be reached it might be this kind of
"speaking with authority" point has to be made. But not as the starter
into a discussion. This is unnecessarily hostile towards the
contributor. And I wished we wouldn't have to see this kind of
behavior here.
Yes, some kernel maintainers do this a lot, but kernel maintainers
also have this kind of reputation and people don't want to have to
deal with this nonsense and decide to not contribute at all. So please
just drop this attitude.
> This is clearly going against the idea of having jobs only depend on
> fences and nothing else which is mandatory for correct memory management.
>
I'm sure it's all documented and there is a design document on how
things have to look like you can point out? Might help to get a better
understanding on how things should be.
> If the hw is busy with something you need to return the fence for this
> from the prepare_job callback so that the scheduler can be notified when
> the hw is available again.
>
> Regards,
> Christian.
>
> >
> > Signed-off-by: Asahi Lina
> > ---
> > drivers/gpu/drm/scheduler/sched_main.c | 10 ++
> > include/drm/gpu_scheduler.h| 8
> > 2 files changed, 18 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> > b/drivers/gpu/drm/scheduler/sched_main.c
> > index 4e6ad6e122bc..5c0add2c7546 100644
> > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > @@ -1001,6 +1001,16 @@ static int drm_sched_main(void *param)
> > if (!entity)
> > continue;
> >
> > + if (sched->ops->can_run_job) {
> > + sched_job =
> > to_drm_sched_job(spsc_queue_peek(>job_queue));
> > + if (!sched_job) {
> > + complete_all(>entity_idle);
> > + continue;
> > + }
> > + if (!sched->ops->can_run_job(sched_job))
> > + continue;
> > + }
> > +
> > sched_job = drm_sched_entity_pop_job(entity);
> >
> > if (!sched_job) {
> > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> > index 9db9e5e504ee..bd89ea9507b9 100644
> > --- a/include/drm/gpu_scheduler.h
> > +++ b/include/drm/gpu_scheduler.h
> > @@ -396,6 +396,14 @@ struct drm_sched_backend_ops {
> > struct dma_fence *(*prepare_job)(struct drm_sched_job *sched_job,
> >struct drm_sched_entity *s_entity);
> >
> > + /**
> > + * @can_run_job: Called before job execution to check whether the
> > + * hardware is free enough to run the job. This can be used to
> > + * implement more complex hardware resource policies than the
> > + * hw_submission limit.
> > + */
> > + bool (*can_run_job)(struct drm_sched_job *sched_job);
> > +
> > /**
> >* @run_job: Called to execute the job once all of the
> > dependencies
> >* have been resolved. This may be called multiple times, if
> >
>
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
Am 08.03.23 um 10:41 schrieb Asahi Lina: On 08/03/2023 17.46, Christian König wrote: Am 07.03.23 um 15:25 schrieb Asahi Lina: Some hardware may require more complex resource utilization accounting than the simple job count supported by drm_sched internally. Add a can_run_job callback to allow drivers to implement more logic before deciding whether to run a GPU job. Well complete NAK. This is clearly going against the idea of having jobs only depend on fences and nothing else which is mandatory for correct memory management. If the hw is busy with something you need to return the fence for this from the prepare_job callback so that the scheduler can be notified when the hw is available again. I think you misunderstood the intent here... This isn't about job dependencies, it's about in-flight resource limits. drm_sched already has a hw_submission_limit that specifies the number of submissions that can be in flight, but that doesn't work for us because each job from drm_sched's point of view consists of multiple commands split among 3 firmware queues. The firmware can only support up to 128 work commands in flight per queue (barriers don't count), otherwise it overflows a fixed-size buffer. So we need more complex accounting of how many underlying commands are in flight per queue to determine whether it is safe to run a new job, and that is what this callback accomplishes. This has to happen even when individual jobs have no buffer/resource dependencies between them (which is what the fences would express). Yeah, I already assumed that you have something like this. And to make it clear this is unfortunately a complete NAK to this approach! You can't do this! 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. Jason Ekstrand gave a create presentation on that problem a few years ago on LPC. I strongly suggest you google that one up. You can see the driver implementation of that callback in drivers/gpu/drm/asahi/queue/mod.rs (QueueJob::can_run()), which then calls into drivers/gpu/drm/asahi/workqueue.rs (Job::can_submit()) that does the actual available slot count checks. The can_run_job logic is written to mirror the hw_submission_limit logic (just a bit later in the sched main loop since we need to actually pick a job to do the check), and just like for that case, completion of any job in the same scheduler will cause another run of the main loop and another check (which is exactly what we want here). Yeah and that hw_submission_limit is based on a fence signaling again. When you have some firmware limitation that a job needs resources which are currently in use by other submissions then those other submissions have fences as well and you can return those in the prepare_job callback. If those other submissions don't have fences, then you have a major design problem inside your driver and we need to get back to square one and talk about that dependency handling. This case (potentially scheduling more than the FW job limit) is rare but handling it is necessary, since otherwise the entire job completion/tracking logic gets screwed up on the firmware end and queues end up stuck (I've managed to trigger this before). Actually that's a pretty normal use case. I've have rejected similar requirements like this before as well. For an example how this can work see amdgpu_job_prepare_job(): https://elixir.bootlin.com/linux/v6.3-rc1/source/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c#L251 The gang submit gives and example of a global fence lock and the VMIDs are an example of a global shared firmware resource. Regards, Christian. ~~ Lina
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
On 08/03/2023 17.46, Christian König wrote: > Am 07.03.23 um 15:25 schrieb Asahi Lina: >> Some hardware may require more complex resource utilization accounting >> than the simple job count supported by drm_sched internally. Add a >> can_run_job callback to allow drivers to implement more logic before >> deciding whether to run a GPU job. > > Well complete NAK. > > This is clearly going against the idea of having jobs only depend on > fences and nothing else which is mandatory for correct memory management. > > If the hw is busy with something you need to return the fence for this > from the prepare_job callback so that the scheduler can be notified when > the hw is available again. I think you misunderstood the intent here... This isn't about job dependencies, it's about in-flight resource limits. drm_sched already has a hw_submission_limit that specifies the number of submissions that can be in flight, but that doesn't work for us because each job from drm_sched's point of view consists of multiple commands split among 3 firmware queues. The firmware can only support up to 128 work commands in flight per queue (barriers don't count), otherwise it overflows a fixed-size buffer. So we need more complex accounting of how many underlying commands are in flight per queue to determine whether it is safe to run a new job, and that is what this callback accomplishes. This has to happen even when individual jobs have no buffer/resource dependencies between them (which is what the fences would express). You can see the driver implementation of that callback in drivers/gpu/drm/asahi/queue/mod.rs (QueueJob::can_run()), which then calls into drivers/gpu/drm/asahi/workqueue.rs (Job::can_submit()) that does the actual available slot count checks. The can_run_job logic is written to mirror the hw_submission_limit logic (just a bit later in the sched main loop since we need to actually pick a job to do the check), and just like for that case, completion of any job in the same scheduler will cause another run of the main loop and another check (which is exactly what we want here). This case (potentially scheduling more than the FW job limit) is rare but handling it is necessary, since otherwise the entire job completion/tracking logic gets screwed up on the firmware end and queues end up stuck (I've managed to trigger this before). ~~ Lina
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
Am 07.03.23 um 15:25 schrieb Asahi Lina:
Some hardware may require more complex resource utilization accounting
than the simple job count supported by drm_sched internally. Add a
can_run_job callback to allow drivers to implement more logic before
deciding whether to run a GPU job.
Well complete NAK.
This is clearly going against the idea of having jobs only depend on
fences and nothing else which is mandatory for correct memory management.
If the hw is busy with something you need to return the fence for this
from the prepare_job callback so that the scheduler can be notified when
the hw is available again.
Regards,
Christian.
Signed-off-by: Asahi Lina
---
drivers/gpu/drm/scheduler/sched_main.c | 10 ++
include/drm/gpu_scheduler.h| 8
2 files changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
index 4e6ad6e122bc..5c0add2c7546 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1001,6 +1001,16 @@ static int drm_sched_main(void *param)
if (!entity)
continue;
+ if (sched->ops->can_run_job) {
+ sched_job =
to_drm_sched_job(spsc_queue_peek(>job_queue));
+ if (!sched_job) {
+ complete_all(>entity_idle);
+ continue;
+ }
+ if (!sched->ops->can_run_job(sched_job))
+ continue;
+ }
+
sched_job = drm_sched_entity_pop_job(entity);
if (!sched_job) {
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 9db9e5e504ee..bd89ea9507b9 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -396,6 +396,14 @@ struct drm_sched_backend_ops {
struct dma_fence *(*prepare_job)(struct drm_sched_job *sched_job,
struct drm_sched_entity *s_entity);
+ /**
+* @can_run_job: Called before job execution to check whether the
+* hardware is free enough to run the job. This can be used to
+* implement more complex hardware resource policies than the
+* hw_submission limit.
+*/
+ bool (*can_run_job)(struct drm_sched_job *sched_job);
+
/**
* @run_job: Called to execute the job once all of the dependencies
* have been resolved. This may be called multiple times, if
[PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
Some hardware may require more complex resource utilization accounting
than the simple job count supported by drm_sched internally. Add a
can_run_job callback to allow drivers to implement more logic before
deciding whether to run a GPU job.
Signed-off-by: Asahi Lina
---
drivers/gpu/drm/scheduler/sched_main.c | 10 ++
include/drm/gpu_scheduler.h| 8
2 files changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
index 4e6ad6e122bc..5c0add2c7546 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1001,6 +1001,16 @@ static int drm_sched_main(void *param)
if (!entity)
continue;
+ if (sched->ops->can_run_job) {
+ sched_job =
to_drm_sched_job(spsc_queue_peek(>job_queue));
+ if (!sched_job) {
+ complete_all(>entity_idle);
+ continue;
+ }
+ if (!sched->ops->can_run_job(sched_job))
+ continue;
+ }
+
sched_job = drm_sched_entity_pop_job(entity);
if (!sched_job) {
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 9db9e5e504ee..bd89ea9507b9 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -396,6 +396,14 @@ struct drm_sched_backend_ops {
struct dma_fence *(*prepare_job)(struct drm_sched_job *sched_job,
struct drm_sched_entity *s_entity);
+ /**
+* @can_run_job: Called before job execution to check whether the
+* hardware is free enough to run the job. This can be used to
+* implement more complex hardware resource policies than the
+* hw_submission limit.
+*/
+ bool (*can_run_job)(struct drm_sched_job *sched_job);
+
/**
* @run_job: Called to execute the job once all of the dependencies
* have been resolved. This may be called multiple times, if
--
2.35.1
