[PATCH] drm/sched: Do not restore unsaved virtual runtime

[PATCH] drm/sched: Do not restore unsaved virtual runtime

[Tvrtko Ursulin]

Prevent pushing a new job to an entity seeing it being the first in the
queue, and hence entering the drm_sched_rq_add_entity() path, if the pop
side in drm_sched_entity_pop_job() has de-queued the job but not yet
updated the saved virtual time.

We do this by pulling the locked sections out to encompass both the queue
push/pop and corresponding rbtree management.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Fixes: 2fa4d8e2c109 ("drm/sched: Add fair scheduling policy")
Suggested-by: Luke.Wildhardt@proton.me # via Claude Opus
Tested-by: Luke.Wildhardt@proton.me
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Philipp Stanner <phasta@kernel.org>
Cc: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Vitaly Prosyak <vitaly.prosyak@amd.com>
Cc: <stable@vger.kernel.org> # v7.2+
---
 drivers/gpu/drm/scheduler/sched_entity.c |  8 +++++++-
 drivers/gpu/drm/scheduler/sched_rq.c     | 20 +++++++++-----------
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index bf97508a45b9..e4069fcb0272 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -563,9 +563,10 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity)
 	 */
 	smp_wmb();
 
+	spin_lock(&entity->lock);
 	spsc_queue_pop(&entity->job_queue);
-
 	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
@@ -651,6 +652,9 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
 	 * Make sure to set the submit_ts first, to avoid a race.
 	 */
 	sched_job->submit_ts = submit_ts = ktime_get();
+
+	spin_lock(&entity->lock);
+
 	first = spsc_queue_push(&entity->job_queue, &sched_job->queue_node);
 
 	/* first job wakes up scheduler */
@@ -661,5 +665,7 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
 		if (sched)
 			drm_sched_wakeup(sched);
 	}
+
+	spin_unlock(&entity->lock);
 }
 EXPORT_SYMBOL(drm_sched_entity_push_job);
diff --git a/drivers/gpu/drm/scheduler/sched_rq.c b/drivers/gpu/drm/scheduler/sched_rq.c
index 0464d324d98d..23f46ec610e7 100644
--- a/drivers/gpu/drm/scheduler/sched_rq.c
+++ b/drivers/gpu/drm/scheduler/sched_rq.c
@@ -257,19 +257,17 @@ static ktime_t drm_sched_entity_get_job_ts(struct drm_sched_entity *entity)
 struct drm_gpu_scheduler *
 drm_sched_rq_add_entity(struct drm_sched_entity *entity, ktime_t ts)
 {
+	struct drm_sched_rq *rq = entity->rq;
 	struct drm_gpu_scheduler *sched;
-	struct drm_sched_rq *rq;
 
 	/* Add the entity to the run queue */
-	spin_lock(&entity->lock);
+	lockdep_assert_held(&entity->lock);
+
 	if (entity->stopped) {
-		spin_unlock(&entity->lock);
-
 		DRM_ERROR("Trying to push to a killed entity\n");
 		return NULL;
 	}
 
-	rq = entity->rq;
 	spin_lock(&rq->lock);
 	sched = rq->sched;
 
@@ -289,7 +287,6 @@ drm_sched_rq_add_entity(struct drm_sched_entity *entity, ktime_t ts)
 	drm_sched_rq_update_fifo_locked(entity, rq, ts);
 
 	spin_unlock(&rq->lock);
-	spin_unlock(&entity->lock);
 
 	return sched;
 }
@@ -343,16 +340,17 @@ drm_sched_rq_next_rr_ts(struct drm_sched_rq *rq,
  */
 void drm_sched_rq_pop_entity(struct drm_sched_entity *entity)
 {
+	struct drm_sched_rq *rq = entity->rq;
 	struct drm_sched_job *next_job;
-	struct drm_sched_rq *rq;
+
+	lockdep_assert_held(&entity->lock);
+
+	spin_lock(&rq->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);
 	if (next_job) {
 		ktime_t ts;
@@ -375,8 +373,8 @@ void drm_sched_rq_pop_entity(struct drm_sched_entity *entity)
 			drm_sched_entity_save_vruntime(entity, min_vruntime);
 		}
 	}
+
 	spin_unlock(&rq->lock);
-	spin_unlock(&entity->lock);
 }
 
 /**
-- 
2.55.0

Re: [PATCH] drm/sched: Do not restore unsaved virtual runtime

[Philipp Stanner]

On Mon, 2026-09-07 at 14:05 +0100, Tvrtko Ursulin wrote:
> Prevent pushing a new job to an entity seeing it being the first in the
> queue, and hence entering the drm_sched_rq_add_entity() path, if the pop
> side in drm_sched_entity_pop_job() has de-queued the job but not yet
> updated the saved virtual time.

I think I don't fully get the bug from that message. The bug (is it
even one?) you address is the performance bottleneck for CFS under high
load, right?

Anyways, I think I got a better idea:

> 
> We do this by pulling the locked sections out to encompass both the queue
> push/pop and corresponding rbtree management.

So basically you move for locking the spsc-queue, the lockless queue
which is usually used with the locks next to it anyways. That's cool
stuff. I've suggested this for a while already

https://docs.kernel.org/gpu/todo.html#replace-the-lockless-queue-with-a-locked-list

So from a maintainability perspective, it would be far cooler if you go
down this road one step further and kill the spsc_queue for good,
replacing it with a fully locked list. AFAIR Christian and Danilo also
have agreed to this.


[…]

> @@ -343,16 +340,17 @@ drm_sched_rq_next_rr_ts(struct drm_sched_rq *rq,
>   */
>  void drm_sched_rq_pop_entity(struct drm_sched_entity *entity)
>  {
> +	struct drm_sched_rq *rq = entity->rq;
>  	struct drm_sched_job *next_job;
> -	struct drm_sched_rq *rq;
> +
> +	lockdep_assert_held(&entity->lock);

btw. I think it *reads* a bit safer if the rq initalization happens
below the lockdep assertion. Also might make the diff nicer.


P.

Re: [PATCH] drm/sched: Do not restore unsaved virtual runtime

[Tvrtko Ursulin]

On 11/09/2026 09:56, Philipp Stanner wrote:
> On Mon, 2026-09-07 at 14:05 +0100, Tvrtko Ursulin wrote:
>> Prevent pushing a new job to an entity seeing it being the first in the
>> queue, and hence entering the drm_sched_rq_add_entity() path, if the pop
>> side in drm_sched_entity_pop_job() has de-queued the job but not yet
>> updated the saved virtual time.
> 
> I think I don't fully get the bug from that message. The bug (is it
> even one?) you address is the performance bottleneck for CFS under high
> load, right?

Not high load but a race condition between the save and restore of 
virtual time when entities enter/leave a run queue.

> Anyways, I think I got a better idea:
> 
>>
>> We do this by pulling the locked sections out to encompass both the queue
>> push/pop and corresponding rbtree management.
> 
> So basically you move for locking the spsc-queue, the lockless queue
> which is usually used with the locks next to it anyways. That's cool
> stuff. I've suggested this for a while already
> 
> https://docs.kernel.org/gpu/todo.html#replace-the-lockless-queue-with-a-locked-list
> 
> So from a maintainability perspective, it would be far cooler if you go
> down this road one step further and kill the spsc_queue for good,
> replacing it with a fully locked list. AFAIR Christian and Danilo also
> have agreed to this.

I am happy to work on that just that a) I thought some time back you 
said someone from RedHat will tackle it, and b) lets please not gate 
this bugfix on that.
> 
> […]
> 
>> @@ -343,16 +340,17 @@ drm_sched_rq_next_rr_ts(struct drm_sched_rq *rq,
>>    */
>>   void drm_sched_rq_pop_entity(struct drm_sched_entity *entity)
>>   {
>> +	struct drm_sched_rq *rq = entity->rq;
>>   	struct drm_sched_job *next_job;
>> -	struct drm_sched_rq *rq;
>> +
>> +	lockdep_assert_held(&entity->lock);
> 
> btw. I think it *reads* a bit safer if the rq initalization happens
> below the lockdep assertion. Also might make the diff nicer.

It makes not practical difference but sure, I agree it reads better so I 
can respin once you clarify if you are blocking the bugfix until the 
spsc removal or what.

Regards,

Tvrtko

Re: [PATCH] drm/sched: Do not restore unsaved virtual runtime

[Matthew Brost]

On Fri, Sep 11, 2026 at 10:18:29AM +0100, Tvrtko Ursulin wrote:
> 
> On 11/09/2026 09:56, Philipp Stanner wrote:
> > On Mon, 2026-09-07 at 14:05 +0100, Tvrtko Ursulin wrote:
> > > Prevent pushing a new job to an entity seeing it being the first in the
> > > queue, and hence entering the drm_sched_rq_add_entity() path, if the pop
> > > side in drm_sched_entity_pop_job() has de-queued the job but not yet
> > > updated the saved virtual time.
> > 
> > I think I don't fully get the bug from that message. The bug (is it
> > even one?) you address is the performance bottleneck for CFS under high
> > load, right?
> 
> Not high load but a race condition between the save and restore of virtual
> time when entities enter/leave a run queue.
> 
> > Anyways, I think I got a better idea:
> > 
> > > 
> > > We do this by pulling the locked sections out to encompass both the queue
> > > push/pop and corresponding rbtree management.
> > 
> > So basically you move for locking the spsc-queue, the lockless queue
> > which is usually used with the locks next to it anyways. That's cool
> > stuff. I've suggested this for a while already
> > 
> > https://docs.kernel.org/gpu/todo.html#replace-the-lockless-queue-with-a-locked-list
> > 
> > So from a maintainability perspective, it would be far cooler if you go
> > down this road one step further and kill the spsc_queue for good,
> > replacing it with a fully locked list. AFAIR Christian and Danilo also
> > have agreed to this.
> 
> I am happy to work on that just that a) I thought some time back you said
> someone from RedHat will tackle it, and b) lets please not gate this bugfix
> on that.
> > 
> > […]
> > 
> > > @@ -343,16 +340,17 @@ drm_sched_rq_next_rr_ts(struct drm_sched_rq *rq,
> > >    */
> > >   void drm_sched_rq_pop_entity(struct drm_sched_entity *entity)
> > >   {
> > > +	struct drm_sched_rq *rq = entity->rq;
> > >   	struct drm_sched_job *next_job;
> > > -	struct drm_sched_rq *rq;
> > > +
> > > +	lockdep_assert_held(&entity->lock);
> > 
> > btw. I think it *reads* a bit safer if the rq initalization happens
> > below the lockdep assertion. Also might make the diff nicer.

No. This style is used throughout the kernel. Your suggestion makes the
code harder to read, and it scales poorly as the number of stack
variables increases.

> 
> It makes not practical difference but sure, I agree it reads better so I can
> respin once you clarify if you are blocking the bugfix until the spsc
> removal or what.

Yes. The lockdep invarient holds regardless.

Matt

> 
> Regards,
> 
> Tvrtko

Re: [PATCH] drm/sched: Do not restore unsaved virtual runtime

[Philipp Stanner]

On Fri, 2026-09-11 at 10:18 +0100, Tvrtko Ursulin wrote:
> > So basically you move for locking the spsc-queue, the lockless queue
> > which is usually used with the locks next to it anyways. That's cool
> > stuff. I've suggested this for a while already
> > 
> > https://docs.kernel.org/gpu/todo.html#replace-the-lockless-queue-with-a-locked-list
> > 
> > So from a maintainability perspective, it would be far cooler if you go
> > down this road one step further and kill the spsc_queue for good,
> > replacing it with a fully locked list. AFAIR Christian and Danilo also
> > have agreed to this.
> 
> I am happy to work on that just that a) I thought some time back you 
> said someone from RedHat will tackle it, and b) lets please not gate 
> this bugfix on that.

Someone from RH might or might not work on that, and perhaps that would
take until 2027. I'm keeping an eye open for people to pick it up, but
everyone's always busy.

Regarding the bugfix, this was reported as a performance issue on high
load, that only seems to be reachable for CFS users, a brand-new policy
that is marked as experimental and is not used by anyone in production
yet. So IMO we can move on on drm-misc-next with spsc-queue, which will
also make CFS usable then.

Is there a plan btw as to how you would like to proceed with CFS?


P.