* [PATCH v3 0/3] drm/sched: Introduce more locking to entity
@ 2026-09-10 7:59 Philipp Stanner
2026-09-10 7:59 ` [PATCH v3 1/3] drm/sched: Lock drm_sched_rq_pop_entity() externally Philipp Stanner
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Philipp Stanner @ 2026-09-10 7:59 UTC (permalink / raw)
To: Danilo Krummrich, Philipp Stanner, Christian König,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Simona Vetter, Tvrtko Ursulin
Cc: dri-devel, linux-kernel
Changes since v2:
- Fix ordering bug between spsc_queue_pop() and
drm_sched_rq_pop_entity().
Changes since v1:
- Remove a bunch of patches; make this series only about locking
entity->last_scheduled. The rest shall be done in separate patches
and series. Consequently, also do not lock spsc_queue, yet.
- Move lock-cycle patch to first position. (Tvrtko)
Both Tvrtko [1] and I [2] have recently proposed some improvals for
drm_sched.
While taking Tvrtko's feedback into account for my patch, I realized
that both his and my patch can be fully replaced with a bigger and far
more beautiful series.
If I am not mistaken, it turns out that the entire entity->entity_idle
completion is also nothing but a workaround around the grave mistake of
not using the greatest helper with parallel programming that exists in
computer science: Locking.
This series adds locking to the last_scheduled field and all checks
related to detect the idleness of the entity. As before, the
job_scheduled event queue causes the periodic checks.
This way, we can get rid of memory barriers, RCU, a few lines of code,
make things more readable, understandable...
Greetings,
Philipp
[1] https://lore.kernel.org/dri-devel/20260611123423.39819-1-tvrtko.ursulin@igalia.com/
[2] https://lore.kernel.org/dri-devel/20260626081942.2122144-2-phasta@kernel.org/
Philipp Stanner (3):
drm/sched: Lock drm_sched_rq_pop_entity() externally
drm/sched: Lock spsc_queue_pop() in drm_sched_entity_pop_job()
drm/sched: Protect entity->last_scheduled with spinlock
drivers/gpu/drm/scheduler/sched_entity.c | 53 ++++++++++--------------
drivers/gpu/drm/scheduler/sched_rq.c | 4 +-
include/drm/gpu_scheduler.h | 10 ++---
3 files changed, 28 insertions(+), 39 deletions(-)
--
2.55.0
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v3 1/3] drm/sched: Lock drm_sched_rq_pop_entity() externally 2026-09-10 7:59 [PATCH v3 0/3] drm/sched: Introduce more locking to entity Philipp Stanner @ 2026-09-10 7:59 ` Philipp Stanner 2026-09-10 7:59 ` [PATCH v3 2/3] drm/sched: Lock spsc_queue_pop() in drm_sched_entity_pop_job() Philipp Stanner ` (2 subsequent siblings) 3 siblings, 0 replies; 7+ messages in thread From: Philipp Stanner @ 2026-09-10 7:59 UTC (permalink / raw) To: Danilo Krummrich, Philipp Stanner, Christian König, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter, Tvrtko Ursulin Cc: dri-devel, linux-kernel In order to protect entity->last_scheduled with a spinlock, adding locking to drm_sched_entity_pop_job() is necessary. This would lead to a slightly suboptimal lock-unlock-relock pattern with drm_sched_rq_pop_entity(). As a preparational step for adding the locking, lock drm_sched_rq_pop_entity() externally. Signed-off-by: Philipp Stanner <phasta@kernel.org> --- drivers/gpu/drm/scheduler/sched_entity.c | 2 ++ drivers/gpu/drm/scheduler/sched_rq.c | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index bf97508a45b9..274d7a702298 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -565,7 +565,9 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity) spsc_queue_pop(&entity->job_queue); + spin_lock(&entity->lock); drm_sched_rq_pop_entity(entity); + spin_unlock(&entity->lock); /* Jobs and entities might have different lifecycles. Since we're * removing the job from the entities queue, set the jobs entity pointer diff --git a/drivers/gpu/drm/scheduler/sched_rq.c b/drivers/gpu/drm/scheduler/sched_rq.c index 0464d324d98d..696792a18708 100644 --- a/drivers/gpu/drm/scheduler/sched_rq.c +++ b/drivers/gpu/drm/scheduler/sched_rq.c @@ -346,11 +346,12 @@ void drm_sched_rq_pop_entity(struct drm_sched_entity *entity) struct drm_sched_job *next_job; struct drm_sched_rq *rq; + lockdep_assert_held(&entity->lock); + /* * Update the entity's location in the min heap according to * the timestamp of the next job, if any. */ - spin_lock(&entity->lock); rq = entity->rq; spin_lock(&rq->lock); next_job = drm_sched_entity_queue_peek(entity); @@ -376,7 +377,6 @@ void drm_sched_rq_pop_entity(struct drm_sched_entity *entity) } } spin_unlock(&rq->lock); - spin_unlock(&entity->lock); } /** -- 2.55.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3 2/3] drm/sched: Lock spsc_queue_pop() in drm_sched_entity_pop_job() 2026-09-10 7:59 [PATCH v3 0/3] drm/sched: Introduce more locking to entity Philipp Stanner 2026-09-10 7:59 ` [PATCH v3 1/3] drm/sched: Lock drm_sched_rq_pop_entity() externally Philipp Stanner @ 2026-09-10 7:59 ` Philipp Stanner 2026-09-10 7:59 ` [PATCH v3 3/3] drm/sched: Protect entity->last_scheduled with spinlock Philipp Stanner 2026-09-11 8:19 ` [PATCH v3 0/3] drm/sched: Introduce more locking to entity Tvrtko Ursulin 3 siblings, 0 replies; 7+ messages in thread From: Philipp Stanner @ 2026-09-10 7:59 UTC (permalink / raw) To: Danilo Krummrich, Philipp Stanner, Christian König, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter, Tvrtko Ursulin Cc: dri-devel, linux-kernel As a preparational step to lock the access of entity->last_scheduled, it is necessary to lock the call to spsc_queue_pop() in drm_sched_entity_pop_job(). The reason is that later the existing lock needs to be moved upward and the relative order between spsc_queue_pop() and drm_sched_rq_pop_entity() needs to be preserved. Guard spsc_queue_pop() with the existing spinlock. Signed-off-by: Philipp Stanner <phasta@kernel.org> --- drivers/gpu/drm/scheduler/sched_entity.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index 274d7a702298..e168f445f2ab 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -563,9 +563,8 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity) */ smp_wmb(); - spsc_queue_pop(&entity->job_queue); - spin_lock(&entity->lock); + spsc_queue_pop(&entity->job_queue); drm_sched_rq_pop_entity(entity); spin_unlock(&entity->lock); -- 2.55.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3 3/3] drm/sched: Protect entity->last_scheduled with spinlock 2026-09-10 7:59 [PATCH v3 0/3] drm/sched: Introduce more locking to entity Philipp Stanner 2026-09-10 7:59 ` [PATCH v3 1/3] drm/sched: Lock drm_sched_rq_pop_entity() externally Philipp Stanner 2026-09-10 7:59 ` [PATCH v3 2/3] drm/sched: Lock spsc_queue_pop() in drm_sched_entity_pop_job() Philipp Stanner @ 2026-09-10 7:59 ` Philipp Stanner 2026-09-11 8:17 ` Tvrtko Ursulin 2026-09-11 8:19 ` [PATCH v3 0/3] drm/sched: Introduce more locking to entity Tvrtko Ursulin 3 siblings, 1 reply; 7+ messages in thread From: Philipp Stanner @ 2026-09-10 7:59 UTC (permalink / raw) To: Danilo Krummrich, Philipp Stanner, Christian König, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter, Tvrtko Ursulin Cc: dri-devel, linux-kernel The entity->last_scheduled field has always been set and read with special RCU functions in addition to memory barriers. This was added in commit 70102d77ff22 ("drm/scheduler: add drm_sched_entity_error and use rcu for last_scheduled") however, no proper justification for that mechanism was provided. There seems to be no obvious reason, since the entity lock is available and taken at all places that evaluate the last_scheduled field. The only exception is drm_sched_entity_error(), which is not performance critical in any way. Improve robustness, readability and maintainability by replacing RCU and barriers with the lock. Signed-off-by: Philipp Stanner <phasta@kernel.org> --- drivers/gpu/drm/scheduler/sched_entity.c | 50 ++++++++++-------------- include/drm/gpu_scheduler.h | 10 ++--- 2 files changed, 24 insertions(+), 36 deletions(-) diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index e168f445f2ab..fa4a9388e5c9 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -136,7 +136,6 @@ int drm_sched_entity_init(struct drm_sched_entity *entity, DRM_SCHED_PRIORITY_KERNEL : priority; entity->num_sched_list = num_sched_list; entity->sched_list = num_sched_list > 1 ? sched_list : NULL; - RCU_INIT_POINTER(entity->last_scheduled, NULL); RB_CLEAR_NODE(&entity->rb_tree_node); if (!sched_list[0]->sched_rq) { @@ -233,10 +232,10 @@ int drm_sched_entity_error(struct drm_sched_entity *entity) struct dma_fence *fence; int r; - rcu_read_lock(); - fence = rcu_dereference(entity->last_scheduled); + spin_lock(&entity->lock); + fence = entity->last_scheduled; r = fence ? fence->error : 0; - rcu_read_unlock(); + spin_unlock(&entity->lock); return r; } @@ -319,9 +318,10 @@ void drm_sched_entity_kill(struct drm_sched_entity *entity) /* Make sure this entity is not used by the scheduler at the moment */ wait_for_completion(&entity->entity_idle); - /* The entity is guaranteed to not be used by the scheduler */ - prev = rcu_dereference_check(entity->last_scheduled, true); + spin_lock(&entity->lock); + prev = entity->last_scheduled; dma_fence_get(prev); + spin_unlock(&entity->lock); while ((job = drm_sched_entity_queue_pop(entity))) { struct drm_sched_fence *s_fence = job->s_fence; @@ -413,8 +413,7 @@ void drm_sched_entity_fini(struct drm_sched_entity *entity) entity->dependency = NULL; } - dma_fence_put(rcu_dereference_check(entity->last_scheduled, true)); - RCU_INIT_POINTER(entity->last_scheduled, NULL); + dma_fence_put(entity->last_scheduled); drm_sched_entity_stats_put(entity->stats); } EXPORT_SYMBOL(drm_sched_entity_fini); @@ -536,6 +535,10 @@ drm_sched_job_dependency(struct drm_sched_job *job, struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity) { + /* Helper to avoid dropping the reference while the entity lock is held, + * just to have some more robustness. + */ + struct dma_fence *prev_last_scheduled; struct drm_sched_job *sched_job; sched_job = drm_sched_entity_queue_peek(entity); @@ -552,22 +555,15 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity) if (entity->guilty && atomic_read(entity->guilty)) dma_fence_set_error(&sched_job->s_fence->finished, -ECANCELED); - dma_fence_put(rcu_dereference_check(entity->last_scheduled, true)); - rcu_assign_pointer(entity->last_scheduled, - dma_fence_get(&sched_job->s_fence->finished)); - - /* - * If the queue is empty we allow drm_sched_entity_select_rq() to - * locklessly access ->last_scheduled. This only works if we set the - * pointer before we dequeue and if we a write barrier here. - */ - smp_wmb(); - spin_lock(&entity->lock); + prev_last_scheduled = entity->last_scheduled; + entity->last_scheduled = dma_fence_get(&sched_job->s_fence->finished); spsc_queue_pop(&entity->job_queue); drm_sched_rq_pop_entity(entity); spin_unlock(&entity->lock); + dma_fence_put(prev_last_scheduled); + /* Jobs and entities might have different lifecycles. Since we're * removing the job from the entities queue, set the jobs entity pointer * to NULL to prevent any future access of the entity through this job. @@ -591,21 +587,15 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity) if (spsc_queue_count(&entity->job_queue)) return; - /* - * Only when the queue is empty are we guaranteed that - * drm_sched_run_job_work() cannot change entity->last_scheduled. To - * enforce ordering we need a read barrier here. See - * drm_sched_entity_pop_job() for the other side. - */ - smp_rmb(); - - fence = rcu_dereference_check(entity->last_scheduled, true); + spin_lock(&entity->lock); + fence = entity->last_scheduled; /* stay on the same engine if the previous job hasn't finished */ - if (fence && !dma_fence_is_signaled(fence)) + if (fence && !dma_fence_is_signaled(fence)) { + spin_unlock(&entity->lock); return; + } - spin_lock(&entity->lock); sched = drm_sched_pick_best(entity->sched_list, entity->num_sched_list); rq = sched ? sched->sched_rq[entity->rq_priority] : NULL; if (rq != entity->rq) { diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index 7a64cc11de08..aeeea6efc623 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -100,8 +100,8 @@ struct drm_sched_entity { * @lock: * * Lock protecting the run-queue (@rq) to which this entity belongs, - * @priority, the list of schedulers (@sched_list, @num_sched_list) and - * the @rr_ts field. + * @priority, @last_scheduled and the list of schedulers (@sched_list, + * @num_sched_list). */ spinlock_t lock; @@ -215,11 +215,9 @@ struct drm_sched_entity { /** * @last_scheduled: * - * Points to the finished fence of the last scheduled job. Only written - * by drm_sched_entity_pop_job(). Can be accessed locklessly from - * drm_sched_job_arm() if the queue is empty. + * Points to the finished fence of the last scheduled job. */ - struct dma_fence __rcu *last_scheduled; + struct dma_fence *last_scheduled; /** * @last_user: last group leader pushing a job into the entity. -- 2.55.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 3/3] drm/sched: Protect entity->last_scheduled with spinlock 2026-09-10 7:59 ` [PATCH v3 3/3] drm/sched: Protect entity->last_scheduled with spinlock Philipp Stanner @ 2026-09-11 8:17 ` Tvrtko Ursulin 0 siblings, 0 replies; 7+ messages in thread From: Tvrtko Ursulin @ 2026-09-11 8:17 UTC (permalink / raw) To: Philipp Stanner, Danilo Krummrich, Christian König, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter, Tvrtko Ursulin, Christian König Cc: dri-devel, linux-kernel + Christian's e-mail which he reads :) On 10/09/2026 08:59, Philipp Stanner wrote: > The entity->last_scheduled field has always been set and read with > special RCU functions in addition to memory barriers. > > This was added in > > commit 70102d77ff22 ("drm/scheduler: add drm_sched_entity_error and use rcu for last_scheduled") > > however, no proper justification for that mechanism was provided. There > seems to be no obvious reason, since the entity lock is available and > taken at all places that evaluate the last_scheduled field. The only > exception is drm_sched_entity_error(), which is not performance critical > in any way. FWIW the entry paths that I found from the amdgpu side are from the job submission side where it is called a few times for each job via amdgpu_ctx_get_entity(), amdgpu_job_prepare_job() and amdgpu_vm_generation(). Last one being both in amdgpu_job_alloc() and amdgpu_job_run(). If I did not miss any, the change effectively adds four lock-unlock cycles to the job submission path. Plus one more for schedulers which use load balancing in some cases. I don't know if avoiding that was the reason RCU was chosen, perhaps Christian remembers. Also, to repeat what I said before, I benchmarked this a bit under extreme circumstances (4x non vsynced vkgears) and did not find a regression in fps. > Improve robustness, readability and maintainability by replacing RCU and > barriers with the lock. > > Signed-off-by: Philipp Stanner <phasta@kernel.org> > --- > drivers/gpu/drm/scheduler/sched_entity.c | 50 ++++++++++-------------- > include/drm/gpu_scheduler.h | 10 ++--- > 2 files changed, 24 insertions(+), 36 deletions(-) > > diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c > index e168f445f2ab..fa4a9388e5c9 100644 > --- a/drivers/gpu/drm/scheduler/sched_entity.c > +++ b/drivers/gpu/drm/scheduler/sched_entity.c > @@ -136,7 +136,6 @@ int drm_sched_entity_init(struct drm_sched_entity *entity, > DRM_SCHED_PRIORITY_KERNEL : priority; > entity->num_sched_list = num_sched_list; > entity->sched_list = num_sched_list > 1 ? sched_list : NULL; > - RCU_INIT_POINTER(entity->last_scheduled, NULL); > RB_CLEAR_NODE(&entity->rb_tree_node); > > if (!sched_list[0]->sched_rq) { > @@ -233,10 +232,10 @@ int drm_sched_entity_error(struct drm_sched_entity *entity) > struct dma_fence *fence; > int r; > > - rcu_read_lock(); > - fence = rcu_dereference(entity->last_scheduled); > + spin_lock(&entity->lock); > + fence = entity->last_scheduled; > r = fence ? fence->error : 0; > - rcu_read_unlock(); > + spin_unlock(&entity->lock); > > return r; > } > @@ -319,9 +318,10 @@ void drm_sched_entity_kill(struct drm_sched_entity *entity) > /* Make sure this entity is not used by the scheduler at the moment */ > wait_for_completion(&entity->entity_idle); > > - /* The entity is guaranteed to not be used by the scheduler */ > - prev = rcu_dereference_check(entity->last_scheduled, true); > + spin_lock(&entity->lock); > + prev = entity->last_scheduled; > dma_fence_get(prev); Here you can compact with the idiomatic: prev = dma_fence_get(entity->last_scheduled); Otherwise looks plausible to me. Lets see what sashiko will have to say this time round. Regards, Tvrtko > + spin_unlock(&entity->lock); > while ((job = drm_sched_entity_queue_pop(entity))) { > struct drm_sched_fence *s_fence = job->s_fence; > > @@ -413,8 +413,7 @@ void drm_sched_entity_fini(struct drm_sched_entity *entity) > entity->dependency = NULL; > } > > - dma_fence_put(rcu_dereference_check(entity->last_scheduled, true)); > - RCU_INIT_POINTER(entity->last_scheduled, NULL); > + dma_fence_put(entity->last_scheduled); > drm_sched_entity_stats_put(entity->stats); > } > EXPORT_SYMBOL(drm_sched_entity_fini); > @@ -536,6 +535,10 @@ drm_sched_job_dependency(struct drm_sched_job *job, > > struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity) > { > + /* Helper to avoid dropping the reference while the entity lock is held, > + * just to have some more robustness. > + */ > + struct dma_fence *prev_last_scheduled; > struct drm_sched_job *sched_job; > > sched_job = drm_sched_entity_queue_peek(entity); > @@ -552,22 +555,15 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity) > if (entity->guilty && atomic_read(entity->guilty)) > dma_fence_set_error(&sched_job->s_fence->finished, -ECANCELED); > > - dma_fence_put(rcu_dereference_check(entity->last_scheduled, true)); > - rcu_assign_pointer(entity->last_scheduled, > - dma_fence_get(&sched_job->s_fence->finished)); > - > - /* > - * If the queue is empty we allow drm_sched_entity_select_rq() to > - * locklessly access ->last_scheduled. This only works if we set the > - * pointer before we dequeue and if we a write barrier here. > - */ > - smp_wmb(); > - > spin_lock(&entity->lock); > + prev_last_scheduled = entity->last_scheduled; > + entity->last_scheduled = dma_fence_get(&sched_job->s_fence->finished); > spsc_queue_pop(&entity->job_queue); > drm_sched_rq_pop_entity(entity); > spin_unlock(&entity->lock); > > + dma_fence_put(prev_last_scheduled); > + > /* Jobs and entities might have different lifecycles. Since we're > * removing the job from the entities queue, set the jobs entity pointer > * to NULL to prevent any future access of the entity through this job. > @@ -591,21 +587,15 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity) > if (spsc_queue_count(&entity->job_queue)) > return; > > - /* > - * Only when the queue is empty are we guaranteed that > - * drm_sched_run_job_work() cannot change entity->last_scheduled. To > - * enforce ordering we need a read barrier here. See > - * drm_sched_entity_pop_job() for the other side. > - */ > - smp_rmb(); > - > - fence = rcu_dereference_check(entity->last_scheduled, true); > + spin_lock(&entity->lock); > + fence = entity->last_scheduled; > > /* stay on the same engine if the previous job hasn't finished */ > - if (fence && !dma_fence_is_signaled(fence)) > + if (fence && !dma_fence_is_signaled(fence)) { > + spin_unlock(&entity->lock); > return; > + } > > - spin_lock(&entity->lock); > sched = drm_sched_pick_best(entity->sched_list, entity->num_sched_list); > rq = sched ? sched->sched_rq[entity->rq_priority] : NULL; > if (rq != entity->rq) { > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h > index 7a64cc11de08..aeeea6efc623 100644 > --- a/include/drm/gpu_scheduler.h > +++ b/include/drm/gpu_scheduler.h > @@ -100,8 +100,8 @@ struct drm_sched_entity { > * @lock: > * > * Lock protecting the run-queue (@rq) to which this entity belongs, > - * @priority, the list of schedulers (@sched_list, @num_sched_list) and > - * the @rr_ts field. > + * @priority, @last_scheduled and the list of schedulers (@sched_list, > + * @num_sched_list). > */ > spinlock_t lock; > > @@ -215,11 +215,9 @@ struct drm_sched_entity { > /** > * @last_scheduled: > * > - * Points to the finished fence of the last scheduled job. Only written > - * by drm_sched_entity_pop_job(). Can be accessed locklessly from > - * drm_sched_job_arm() if the queue is empty. > + * Points to the finished fence of the last scheduled job. > */ > - struct dma_fence __rcu *last_scheduled; > + struct dma_fence *last_scheduled; > > /** > * @last_user: last group leader pushing a job into the entity. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 0/3] drm/sched: Introduce more locking to entity 2026-09-10 7:59 [PATCH v3 0/3] drm/sched: Introduce more locking to entity Philipp Stanner ` (2 preceding siblings ...) 2026-09-10 7:59 ` [PATCH v3 3/3] drm/sched: Protect entity->last_scheduled with spinlock Philipp Stanner @ 2026-09-11 8:19 ` Tvrtko Ursulin 2026-09-11 10:34 ` Philipp Stanner 3 siblings, 1 reply; 7+ messages in thread From: Tvrtko Ursulin @ 2026-09-11 8:19 UTC (permalink / raw) To: Philipp Stanner, Danilo Krummrich, Christian König, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter, Tvrtko Ursulin Cc: dri-devel, linux-kernel On 10/09/2026 08:59, Philipp Stanner wrote: > Changes since v2: > - Fix ordering bug between spsc_queue_pop() and > drm_sched_rq_pop_entity(). > > Changes since v1: > - Remove a bunch of patches; make this series only about locking > entity->last_scheduled. The rest shall be done in separate patches > and series. Consequently, also do not lock spsc_queue, yet. > - Move lock-cycle patch to first position. (Tvrtko) > > > Both Tvrtko [1] and I [2] have recently proposed some improvals for > drm_sched. > > While taking Tvrtko's feedback into account for my patch, I realized > that both his and my patch can be fully replaced with a bigger and far > more beautiful series. > > If I am not mistaken, it turns out that the entire entity->entity_idle > completion is also nothing but a workaround around the grave mistake of > not using the greatest helper with parallel programming that exists in > computer science: Locking. > > This series adds locking to the last_scheduled field and all checks > related to detect the idleness of the entity. As before, the > job_scheduled event queue causes the periodic checks. > > This way, we can get rid of memory barriers, RCU, a few lines of code, > make things more readable, understandable... > > Greetings, > Philipp > > [1] https://lore.kernel.org/dri-devel/20260611123423.39819-1-tvrtko.ursulin@igalia.com/ > [2] https://lore.kernel.org/dri-devel/20260626081942.2122144-2-phasta@kernel.org/ > > Philipp Stanner (3): > drm/sched: Lock drm_sched_rq_pop_entity() externally > drm/sched: Lock spsc_queue_pop() in drm_sched_entity_pop_job() FWIW if you could review https://lore.kernel.org/dri-devel/20260907130527.52530-1-tvrtko.ursulin@igalia.com/ you could drop the first two patches from your series. Extra benefit is that patch fixes a bug and has been tested by the user. Regards, Tvrtko > drm/sched: Protect entity->last_scheduled with spinlock > > drivers/gpu/drm/scheduler/sched_entity.c | 53 ++++++++++-------------- > drivers/gpu/drm/scheduler/sched_rq.c | 4 +- > include/drm/gpu_scheduler.h | 10 ++--- > 3 files changed, 28 insertions(+), 39 deletions(-) > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 0/3] drm/sched: Introduce more locking to entity 2026-09-11 8:19 ` [PATCH v3 0/3] drm/sched: Introduce more locking to entity Tvrtko Ursulin @ 2026-09-11 10:34 ` Philipp Stanner 0 siblings, 0 replies; 7+ messages in thread From: Philipp Stanner @ 2026-09-11 10:34 UTC (permalink / raw) To: Tvrtko Ursulin, Philipp Stanner, Danilo Krummrich, Christian König, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter, Tvrtko Ursulin Cc: dri-devel, linux-kernel On Fri, 2026-09-11 at 09:19 +0100, Tvrtko Ursulin wrote: > FWIW if you could review > https://lore.kernel.org/dri-devel/20260907130527.52530-1-tvrtko.ursulin@igalia.com/ > you could drop the first two patches from your series. Extra benefit is > that patch fixes a bug and has been tested by the user. See the other thread; I'm not super-hard opposed, but if we can agree on getting CFS to a non-experimental status by first improving spsc_queue, then it would be nicer from the diff / git log perspective to have these little patches as a preparational base-line for the spsc rework :) Greetings Philipp ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2026-09-11 10:35 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2026-09-10 7:59 [PATCH v3 0/3] drm/sched: Introduce more locking to entity Philipp Stanner 2026-09-10 7:59 ` [PATCH v3 1/3] drm/sched: Lock drm_sched_rq_pop_entity() externally Philipp Stanner 2026-09-10 7:59 ` [PATCH v3 2/3] drm/sched: Lock spsc_queue_pop() in drm_sched_entity_pop_job() Philipp Stanner 2026-09-10 7:59 ` [PATCH v3 3/3] drm/sched: Protect entity->last_scheduled with spinlock Philipp Stanner 2026-09-11 8:17 ` Tvrtko Ursulin 2026-09-11 8:19 ` [PATCH v3 0/3] drm/sched: Introduce more locking to entity Tvrtko Ursulin 2026-09-11 10:34 ` Philipp Stanner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®