[PATCH v2 0/2] drm/sched: Introduce more locking to entity

[PATCH v2 0/2] drm/sched: Introduce more locking to entity

[Philipp Stanner]

This is for now based on drm-misc-fixes. I send it out so we can get
consensus for our work in the upcoming cycle.

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...


Tested with drm-sched-unit tests. I'm a bit busy right now, but wanted
to show you guys the idea. Before merging I'd test it more exhaustively
with Nouveau.

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 (2):
  drm/sched: Lock drm_sched_rq_pop_entity() externally
  drm/sched: Protect entity->last_scheduled with spinlock

 drivers/gpu/drm/scheduler/sched_entity.c | 52 ++++++++++--------------
 drivers/gpu/drm/scheduler/sched_rq.c     |  4 +-
 include/drm/gpu_scheduler.h              | 10 ++---
 3 files changed, 28 insertions(+), 38 deletions(-)


base-commit: 9a11db68872055e6ead919bad04d6330851c522d
-- 
2.55.0

[PATCH v2 1/2] drm/sched: Lock drm_sched_rq_pop_entity() externally

[Philipp Stanner]

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 672b5c57ed8e..7fe47fc86c0e 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -568,7 +568,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

[PATCH v2 2/2] drm/sched: Protect entity->last_scheduled with spinlock

[Philipp Stanner]

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 | 54 ++++++++++--------------
 include/drm/gpu_scheduler.h              | 10 ++---
 2 files changed, 26 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 7fe47fc86c0e..2c903feca230 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -137,7 +137,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) {
@@ -230,10 +229,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;
 
@@ -416,8 +416,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);
@@ -539,6 +538,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);
@@ -555,22 +558,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);
+	drm_sched_rq_pop_entity(entity);
+	spin_unlock(&entity->lock);
 
 	spsc_queue_pop(&entity->job_queue);
 
-	spin_lock(&entity->lock);
-	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
@@ -595,21 +591,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 790f7ecb6b85..b638065a11cb 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

Re: [PATCH v2 2/2] drm/sched: Protect entity->last_scheduled with spinlock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incorrect ordering of `spsc_queue_pop()` and `drm_sched_rq_pop_entity()` causes runqueue corruption, empty entity leakage, and breaks FIFO scheduling.
--

--- Patch [2]: [PATCH v2 2/2] drm/sched: Protect entity->last_scheduled with spinlock ---
commit fe95a95b4cbec301e14107b82365837fc67259e7
Author: Philipp Stanner <phasta@kernel.org>

drm/sched: Protect entity->last_scheduled with spinlock

This commit replaces the RCU and memory barrier mechanisms used to protect
entity->last_scheduled with a spinlock. It aims to improve robustness and
readability since the entity lock is already taken in most places that
evaluate this field.

> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
[ ... ]
> @@ -555,23 +558,16 @@ 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();
> -
> -	spsc_queue_pop(&entity->job_queue);
> -
>  	spin_lock(&entity->lock);
> +	prev_last_scheduled = entity->last_scheduled;
> +	entity->last_scheduled = dma_fence_get(&sched_job->s_fence->finished);
>  	drm_sched_rq_pop_entity(entity);
>  	spin_unlock(&entity->lock);
>  
> +	spsc_queue_pop(&entity->job_queue);

[Severity: High]
Does moving spsc_queue_pop() after drm_sched_rq_pop_entity() cause runqueue
corruption?

Looking at drm_sched_rq_pop_entity(), it uses drm_sched_entity_queue_peek()
to check the next job in the queue:

drivers/gpu/drm/scheduler/sched_rq.c:drm_sched_rq_pop_entity() {
    ...
    next_job = drm_sched_entity_queue_peek(entity);
    if (next_job) {
        ...
        drm_sched_rq_update_fifo_locked(entity, rq, ts);
    } else {
        drm_sched_rq_remove_fifo_locked(entity, rq);
        ...
}

Because the current job hasn't been popped from entity->job_queue yet,
won't drm_sched_entity_queue_peek() return the job we are currently
processing instead of the subsequent job (or NULL)?

It appears this structural inversion causes the runqueue update to evaluate
the current job's timestamp rather than the next job, and prevents empty
entities from being correctly removed from the runqueue.

> +
> +	dma_fence_put(prev_last_scheduled);
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813092510.2385962-4-phasta@kernel.org?part=2

Re: [PATCH v2 2/2] drm/sched: Protect entity->last_scheduled with spinlock

[Philipp Stanner]

On Thu, 2026-08-13 at 11:25 +0200, Philipp Stanner wrote:
> 
> +	drm_sched_rq_pop_entity(entity);
> +	spin_unlock(&entity->lock);
>  
>  	spsc_queue_pop(&entity->job_queue);
>  
> -	spin_lock(&entity->lock);
> -	drm_sched_rq_pop_entity(entity);
> -	spin_unlock(&entity->lock);
> +	dma_fence_put(prev_last_scheduled);


The relative order between these must not be changed. My bad.

So unfortunately it looks as if at least locking spsc_queue here is
necessary. I really wished someone could pick up our spsc_queue locking
TODO.


P.