drm/sched: Fix dynamic job-flow control race
Subsystems (3)
PATCH 1/1 drm/sched: Fix dynamic job-flow control race
From: Rob Clark <[email protected]>
Fixes a race condition reported here: https://github.com/AsahiLinux/linux/issues/309#issuecomment-2238968609
The whole premise of lockless access to a single-producer-single- consumer queue is that there is just a single producer and single consumer. That means we can't call drm_sched_can_queue() (which is about queueing more work to the hw, not to the spsc queue) from anywhere other than the consumer (wq).
This call in the producer is just an optimization to avoid scheduling the consuming worker if it cannot yet queue more work to the hw. It is safe to drop this optimization to avoid the race condition.
--- drivers/gpu/drm/scheduler/sched_main.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/gpu/drm/scheduler/sched_main.c +1 -2
@@ -1020,8 +1020,7 @@ EXPORT_SYMBOL(drm_sched_job_cleanup);void drm_sched_wakeup (struct drm_gpu_scheduler *sched,struct drm_sched_entity *entity) { -if (drm_sched_can_queue (sched, entity)) -drm_sched_run_job_queue (sched); +drm_sched_run_job_queue (sched); }/**
Signature · 1 line
2.46.0
Quoted 26 lines · Rob Clark
From: Rob Clark <[email protected]>
Fixes a race condition reported here: https://github.com/AsahiLinux/linux/issues/309#issuecomment-2238968609
The whole premise of lockless access to a single-producer-single- consumer queue is that there is just a single producer and single consumer. That means we can't call
drm_sched_can_queue()(which is about queueing more work to the hw, not to the spsc queue) from anywhere other than the consumer (wq).This call in the producer is just an optimization to avoid scheduling the consuming worker if it cannot yet queue more work to the hw. It is safe to drop this optimization to avoid the race condition.
Suggested-by Asahi Lina <[email protected]>Fixes a78422e9dff3 ("drm/sched: implement dynamic job-flow control")Signed-off-by Rob Clark <[email protected]>--- drivers/gpu/drm/scheduler/sched_main.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/gpu/drm/scheduler/sched_main.c +0
-0
drivers/gpu/drm/scheduler/sched_main.c EXPORT_SYMBOL(drm_sched_job_cleanup); · line 1026
Quoted 9 lines
drivers/gpu/drm/scheduler/sched_main.c +1
-2@@ -1020,8 +1020,7 @@ EXPORT_SYMBOL(drm_sched_job_cleanup);void drm_sched_wakeup (struct drm_gpu_scheduler *sched,struct drm_sched_entity *entity) { -if (drm_sched_can_queue (sched, entity)) -drm_sched_run_job_queue (sched); +drm_sched_run_job_queue (sched); }/**
The entity parameter is unused now.
Signature · 2 lines
Earthling Michel Dänzer \ GNOME / Xwayland / Mesa developer https://redhat.com \ Libre software enthusiast
Quoted 36 lines · Michel Dänzer
Quoted 34 lines · Rob Clark
From: Rob Clark <[email protected]>
Fixes a race condition reported here: https://github.com/AsahiLinux/linux/issues/309#issuecomment-2238968609
The whole premise of lockless access to a single-producer-single- consumer queue is that there is just a single producer and single consumer. That means we can't call
drm_sched_can_queue()(which is about queueing more work to the hw, not to the spsc queue) from anywhere other than the consumer (wq).This call in the producer is just an optimization to avoid scheduling the consuming worker if it cannot yet queue more work to the hw. It is safe to drop this optimization to avoid the race condition.
Suggested-by Asahi Lina <[email protected]>Fixes a78422e9dff3 ("drm/sched: implement dynamic job-flow control")Signed-off-by Rob Clark <[email protected]>--- drivers/gpu/drm/scheduler/sched_main.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/gpu/drm/scheduler/sched_main.c +1
-2@@ -1020,8 +1020,7 @@ EXPORT_SYMBOL(drm_sched_job_cleanup);void drm_sched_wakeup (struct drm_gpu_scheduler *sched,struct drm_sched_entity *entity) { -if (drm_sched_can_queue (sched, entity)) -drm_sched_run_job_queue (sched); +drm_sched_run_job_queue (sched); }
Quoted 3 lines
drivers/gpu/drm/scheduler/sched_main.c EXPORT_SYMBOL(drm_sched_job_cleanup); · line 1026
Quoted 1 line
drivers/gpu/drm/scheduler/sched_main.c +0
-0/** The entity parameter is unused now.
Right.. and we probably should collapse drm_sched_wakeup() and drm_sched_run_job_queue()..
But this fix needs to be cherry picked back to a bunch of release branches, so I intentionally avoided refactoring as part of the fix.
BR,
-R
Quoted 5 lines
--
Earthling Michel Dänzer \ GNOME / Xwayland / Mesa developer
https://redhat.com \ Libre software enthusiast
Quoted 38 lines · Rob Clark
Quoted 36 lines · Michel Dänzer
Quoted 34 lines · Rob Clark
From: Rob Clark <[email protected]>
Fixes a race condition reported here: https://github.com/AsahiLinux/linux/issues/309#issuecomment-2238968609
The whole premise of lockless access to a single-producer-single- consumer queue is that there is just a single producer and single consumer. That means we can't call
drm_sched_can_queue()(which is about queueing more work to the hw, not to the spsc queue) from anywhere other than the consumer (wq).This call in the producer is just an optimization to avoid scheduling the consuming worker if it cannot yet queue more work to the hw. It is safe to drop this optimization to avoid the race condition.
Suggested-by Asahi Lina <[email protected]>Fixes a78422e9dff3 ("drm/sched: implement dynamic job-flow control")Signed-off-by Rob Clark <[email protected]>--- drivers/gpu/drm/scheduler/sched_main.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/gpu/drm/scheduler/sched_main.c +1
-2@@ -1020,8 +1020,7 @@ EXPORT_SYMBOL(drm_sched_job_cleanup);void drm_sched_wakeup (struct drm_gpu_scheduler *sched,struct drm_sched_entity *entity) { -if (drm_sched_can_queue (sched, entity)) -drm_sched_run_job_queue (sched); +drm_sched_run_job_queue (sched); }
Quoted 6 lines
Quoted 3 lines
drivers/gpu/drm/scheduler/sched_main.c EXPORT_SYMBOL(drm_sched_job_cleanup); · line 1026
Quoted 1 line
drivers/gpu/drm/scheduler/sched_main.c +0
-0/** The entity parameter is unused now.
Right.. and we probably should collapse
drm_sched_wakeup()anddrm_sched_run_job_queue()..
I looked into that as well, seems fine to leave them separate for now though.
Quoted 2 lines
But this fix needs to be cherry picked back to a bunch of release branches, so I intentionally avoided refactoring as part of the fix.
Fixing up the drm_sched_wakeup caller(s) when backporting doesn't seem like a big deal.
Signature · 2 lines
Earthling Michel Dänzer \ GNOME / Xwayland / Mesa developer https://redhat.com \ Libre software enthusiast
Quoted 3 lines · Rob Clark
From: Rob Clark <[email protected]>
Fixes a race condition reported here: https://github.com/AsahiLinux/linux/issues/309#issuecomment-2238968609
Good catch! Please add a 'Closes' tag with this link.
Quoted 7 lines
The whole premise of lockless access to a single-producer-single- consumer queue is that there is just a single producer and single consumer. That means we can't call
drm_sched_can_queue()(which is about queueing more work to the hw, not to the spsc queue) from anywhere other than the consumer (wq).
Quoted 6 lines
This call in the producer is just an optimization to avoid scheduling the consuming worker if it cannot yet queue more work to the hw. It is safe to drop this optimization to avoid the race condition.
Suggested-by Asahi Lina <[email protected]>Fixes a78422e9dff3 ("drm/sched: implement dynamic job-flow control")
You may want to explicitly CC stable.
drivers/gpu/drm/scheduler/sched_main.c EXPORT_SYMBOL(drm_sched_job_cleanup); · line 1021
Quoted 12 lines
Signed-off-by Rob Clark <[email protected]>--- drivers/gpu/drm/scheduler/sched_main.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/gpu/drm/scheduler/sched_main.c +0
-0@@ -1020,8 +1020,7 @@ EXPORT_SYMBOL(drm_sched_job_cleanup);void drm_sched_wakeup (struct drm_gpu_scheduler *sched,struct drm_sched_entity *entity)
Please also remove the entity parameter. For the other refactoring, I agree it should be in a different patch.
With that,
Thanks for fixing this.
- Danilo
Quoted 10 lines
drivers/gpu/drm/scheduler/sched_main.c +1
-2{ -if (drm_sched_can_queue (sched, entity)) -drm_sched_run_job_queue (sched); +drm_sched_run_job_queue (sched); }/** Signature · 1 line
2.46.0
Quoted 4 lines · Rob Clark
From: Rob Clark <[email protected]>
Fixes a race condition reported here:
https://github.com/AsahiLinux/linux/issues/309#issuecomment-2238968609
As Danilo suggested before, I'd put this in a Fixes: section at the bottom and instead have a sentence here detailing what the race consists of, i.e., who is racing with whom.
P.
Quoted 33 lines
The whole premise of lockless access to a single-producer-single- consumer queue is that there is just a single producer and single consumer. That means we can't call
drm_sched_can_queue()(which is about queueing more work to the hw, not to the spsc queue) from anywhere other than the consumer (wq).This call in the producer is just an optimization to avoid scheduling the consuming worker if it cannot yet queue more work to the hw. It is safe to drop this optimization to avoid the race condition.
Suggested-by Asahi Lina <[email protected]>Fixes a78422e9dff3 ("drm/sched: implement dynamic job-flow control")Signed-off-by Rob Clark <[email protected]>---
drivers/gpu/drm/scheduler/sched_main.c | 3 +--1 file changed, 1 insertion(+), 2 deletions(-)drivers/gpu/drm/scheduler/sched_main.c +1
-2@@ -1020,8 +1020,7 @@ EXPORT_SYMBOL(drm_sched_job_cleanup); void drm_sched_wakeup(struct drm_gpu_scheduler *sched, struct drm_sched_entity *entity) { -if (drm_sched_can_queue (sched, entity)) -drm_sched_run_job_queue (sched); +drm_sched_run_job_queue (sched); }
/**
Danilo Krummrich
maintainerQuoted 7 lines · Philipp Stanner
Quoted 4 lines · Rob Clark
From: Rob Clark <[email protected]>
Fixes a race condition reported here:
https://github.com/AsahiLinux/linux/issues/309#issuecomment-2238968609As Danilo suggested before, I'd put this in a Fixes: section at the
I think you mean 'Closes'.
Please note that there's a v2 [1] already, which does that.
[1] https://lore.kernel.org/dri-devel/[email protected]/
Quoted 2 lines
bottom and instead have a sentence here detailing what the race consists of, i.e., who is racing with whom.
That's written right below, isn't it?
Quoted 37 lines
P.
Quoted 33 lines
The whole premise of lockless access to a single-producer-single- consumer queue is that there is just a single producer and single consumer. That means we can't call
drm_sched_can_queue()(which is about queueing more work to the hw, not to the spsc queue) from anywhere other than the consumer (wq).This call in the producer is just an optimization to avoid scheduling the consuming worker if it cannot yet queue more work to the hw. It is safe to drop this optimization to avoid the race condition.
Suggested-by Asahi Lina <[email protected]>Fixes a78422e9dff3 ("drm/sched: implement dynamic job-flow control")Signed-off-by Rob Clark <[email protected]>---
drivers/gpu/drm/scheduler/sched_main.c | 3 +--1 file changed, 1 insertion(+), 2 deletions(-)drivers/gpu/drm/scheduler/sched_main.c +1
-2@@ -1020,8 +1020,7 @@ EXPORT_SYMBOL(drm_sched_job_cleanup); void drm_sched_wakeup(struct drm_gpu_scheduler *sched, struct drm_sched_entity *entity) { -if (drm_sched_can_queue (sched, entity)) -drm_sched_run_job_queue (sched); +drm_sched_run_job_queue (sched); }
/**