[PATCH 0/2] drm/sched: Fair policy fixups

[PATCH 0/2] drm/sched: Fair policy fixups

[Tvrtko Ursulin]

Two fixes to address the two issues found in:
https://lore.kernel.org/dri-devel/TfhgV1W0W5LI6RWUO6J35B3R8QIYH_FN3Eihzdo_9PH39hfn1AVByT-QBPHmWiAR-L0Kqi2sppM_EhFPKwZPGmb5pFgpF2MrzeFzkZDmpG8=@proton.me/

Many thanks to Luke for reporting, sharing one AI proposed fix, and the super
quick testing turnaround!

After these two we can hopefully work at promoting the "fair" policy back to
the default in an upcoming kernel release, followed by removing the FIFO and
round-robin, and then re-applying the code base simplifications which we had to
temporarily revert.

Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Luke.Wildhardt@proton.me
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <phasta@kernel.org>
Cc: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
Cc: Vitaly Prosyak <vitaly.prosyak@amd.com>

Tvrtko Ursulin (2):
  drm/sched: Do not restore unsaved virtual runtime
  drm/sched: Ensure monotonic min_vruntime

 drivers/gpu/drm/scheduler/sched_entity.c |  8 ++-
 drivers/gpu/drm/scheduler/sched_rq.c     | 85 ++++++++----------------
 include/drm/gpu_scheduler.h              |  2 +
 3 files changed, 37 insertions(+), 58 deletions(-)

-- 
2.54.0

[PATCH 1/2] 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>
---
 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 a4a7efdbf229..673ca9cbf362 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -559,9 +559,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
@@ -647,6 +648,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 */
@@ -657,5 +661,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.54.0

[PATCH 2/2] drm/sched: Ensure monotonic min_vruntime

[Tvrtko Ursulin]

min_vruntime handling had a bug where if an entity never exited the run-
queue it could get penalised by its virtual runtime only ever growing,
while the periodically exiting and re-joining entities could repeatedly
get pulled ahead of it.

Fix it by making the min_vruntime strictly monotonic by tracking it
separately (an keeping it always up to date) instead of fetching what
happens to be the top of the tree sorted by virtual runtime.

While at it replace open coded ktime_t comparison with the correct
ktime_after() helper.

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

diff --git a/drivers/gpu/drm/scheduler/sched_rq.c b/drivers/gpu/drm/scheduler/sched_rq.c
index 23f46ec610e7..bc8363974a27 100644
--- a/drivers/gpu/drm/scheduler/sched_rq.c
+++ b/drivers/gpu/drm/scheduler/sched_rq.c
@@ -98,6 +98,7 @@ void drm_sched_rq_init(struct drm_gpu_scheduler *sched,
 	rq->rb_tree_root = RB_ROOT_CACHED;
 	rq->sched = sched;
 	rq->head_prio = DRM_SCHED_PRIORITY_INVALID;
+	rq->min_vruntime = 0;
 }
 
 /*
@@ -120,28 +121,6 @@ static const unsigned int vruntime_shift[] = {
 	[DRM_SCHED_PRIORITY_LOW]    = 7,
 };
 
-static ktime_t
-drm_sched_rq_get_min_vruntime(struct drm_sched_rq *rq)
-{
-	ktime_t vruntime = 0;
-	struct rb_node *rb;
-
-	lockdep_assert_held(&rq->lock);
-
-	rb = rb_first_cached(&rq->rb_tree_root);
-	if (rb) {
-		struct drm_sched_entity *entity =
-			rb_entry(rb, typeof(*entity), rb_tree_node);
-		struct drm_sched_entity_stats *stats = entity->stats;
-
-		spin_lock(&stats->lock);
-		vruntime = stats->vruntime;
-		spin_unlock(&stats->lock);
-	}
-
-	return vruntime;
-}
-
 static void
 drm_sched_entity_save_vruntime(struct drm_sched_entity *entity,
 			       ktime_t min_vruntime)
@@ -151,7 +130,7 @@ drm_sched_entity_save_vruntime(struct drm_sched_entity *entity,
 
 	spin_lock(&stats->lock);
 	vruntime = stats->vruntime;
-	if (min_vruntime && vruntime > min_vruntime)
+	if (ktime_after(vruntime, min_vruntime))
 		vruntime = ktime_sub(vruntime, min_vruntime);
 	else
 		vruntime = 0;
@@ -239,11 +218,6 @@ static ktime_t drm_sched_entity_update_vruntime(struct drm_sched_entity *entity)
 	return runtime;
 }
 
-static ktime_t drm_sched_entity_get_job_ts(struct drm_sched_entity *entity)
-{
-	return drm_sched_entity_update_vruntime(entity);
-}
-
 /**
  * drm_sched_rq_add_entity - add an entity
  * @entity: scheduler entity
@@ -276,13 +250,11 @@ drm_sched_rq_add_entity(struct drm_sched_entity *entity, ktime_t ts)
 		list_add_tail(&entity->list, &rq->entities);
 	}
 
-	if (drm_sched_policy == DRM_SCHED_POLICY_FAIR) {
-		ts = drm_sched_rq_get_min_vruntime(rq);
-		ts = drm_sched_entity_restore_vruntime(entity, ts,
+	if (drm_sched_policy == DRM_SCHED_POLICY_FAIR)
+		ts = drm_sched_entity_restore_vruntime(entity, rq->min_vruntime,
 						       rq->head_prio);
-	} else if (drm_sched_policy == DRM_SCHED_POLICY_RR) {
+	else if (drm_sched_policy == DRM_SCHED_POLICY_RR)
 		ts = entity->rr_ts;
-	}
 
 	drm_sched_rq_update_fifo_locked(entity, rq, ts);
 
@@ -342,6 +314,7 @@ void drm_sched_rq_pop_entity(struct drm_sched_entity *entity)
 {
 	struct drm_sched_rq *rq = entity->rq;
 	struct drm_sched_job *next_job;
+	ktime_t ts;
 
 	lockdep_assert_held(&entity->lock);
 
@@ -351,27 +324,27 @@ void drm_sched_rq_pop_entity(struct drm_sched_entity *entity)
 	 * Update the entity's location in the min heap according to
 	 * the timestamp of the next job, if any.
 	 */
+
+	if (drm_sched_policy == DRM_SCHED_POLICY_FAIR) {
+		ts = drm_sched_entity_update_vruntime(entity);
+		if (ktime_after(ts, rq->min_vruntime))
+			rq->min_vruntime = ts;
+	}
+
 	next_job = drm_sched_entity_queue_peek(entity);
 	if (next_job) {
-		ktime_t ts;
-
-		if (drm_sched_policy == DRM_SCHED_POLICY_FAIR)
-			ts = drm_sched_entity_get_job_ts(entity);
-		else if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
+		if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
 			ts = next_job->submit_ts;
-		else
+		else if (drm_sched_policy == DRM_SCHED_POLICY_RR)
 			ts = drm_sched_rq_next_rr_ts(rq, entity);
 
 		drm_sched_rq_update_fifo_locked(entity, rq, ts);
 	} else {
+		if (drm_sched_policy == DRM_SCHED_POLICY_FAIR)
+			drm_sched_entity_save_vruntime(entity,
+						       rq->min_vruntime);
+
 		drm_sched_rq_remove_fifo_locked(entity, rq);
-
-		if (drm_sched_policy == DRM_SCHED_POLICY_FAIR) {
-			ktime_t min_vruntime;
-
-			min_vruntime = drm_sched_rq_get_min_vruntime(rq);
-			drm_sched_entity_save_vruntime(entity, min_vruntime);
-		}
 	}
 
 	spin_unlock(&rq->lock);
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 7a64cc11de08..92d1a6ff9ac0 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -267,6 +267,7 @@ struct drm_sched_entity {
  * @entities: list of the entities to be scheduled.
  * @rb_tree_root: root of time based priority queue of entities for FIFO scheduling
  * @head_prio: priority of the top tree element.
+ * @min_vruntime: Minimum virtual runtime for the run-queue.
  *
  * Run queue is a set of entities scheduling command submissions for
  * one specific ring. It implements the scheduling policy that selects
@@ -281,6 +282,7 @@ struct drm_sched_rq {
 	struct list_head		entities;
 	struct rb_root_cached		rb_tree_root;
 	enum drm_sched_priority		head_prio;
+	ktime_t				min_vruntime;
 };
 
 /**
-- 
2.54.0

Re: [PATCH 2/2] drm/sched: Ensure monotonic min_vruntime

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm_sched_entity_save_vruntime() always saves a virtual runtime lag of 0 due to an impossible condition, permanently destroying the entity's relative lag state.
- [Medium] rq->min_vruntime incorrectly tracks the maximum updated vruntime of the executing entity rather than the true minimum of the runqueue, penalizing newly enqueued entities.
--

--- Patch [2]: [PATCH 2/2] drm/sched: Ensure monotonic min_vruntime ---
commit 840e24cc75ac9a836c633b23cb95dda0e0dc3b75
Author: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

drm/sched: Ensure monotonic min_vruntime

min_vruntime handling had a bug where if an entity never exited the run-
queue it could get penalised by its virtual runtime only ever growing,
while the periodically exiting and re-joining entities could repeatedly
get pulled ahead of it.

Fix it by making the min_vruntime strictly monotonic by tracking it
separately (an keeping it always up to date) instead of fetching what
happens to be the top of the tree sorted by virtual runtime.

While at it replace open coded ktime_t comparison with the correct
ktime_after() helper.

Fixes: 2fa4d8e2c109 ("drm/sched: Add fair scheduling policy")
Tested-by: Luke.Wildhardt@proton.me
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Luke.Wildhardt@proton.me
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <phasta@kernel.org>
Cc: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
Cc: Vitaly Prosyak <vitaly.prosyak@amd.com>

> diff --git a/drivers/gpu/drm/scheduler/sched_rq.c b/drivers/gpu/drm/scheduler/sched_rq.c
> index 23f46ec610e78..bc8363974a276 100644
> --- a/drivers/gpu/drm/scheduler/sched_rq.c
> +++ b/drivers/gpu/drm/scheduler/sched_rq.c

[ ... ]

> @@ -151,7 +130,7 @@ drm_sched_entity_save_vruntime(struct drm_sched_entity *entity,
>  
>  	spin_lock(&stats->lock);
>  	vruntime = stats->vruntime;
> -	if (min_vruntime && vruntime > min_vruntime)
> +	if (ktime_after(vruntime, min_vruntime))

[Severity: High]
Will this condition always evaluate to false and unconditionally reset the 
saved lag to 0?

In drm_sched_rq_pop_entity(), the runqueue's min_vruntime is updated to be
at least the entity's current vruntime:

drm_sched_rq_pop_entity() {
    ...
    ts = drm_sched_entity_update_vruntime(entity);
    if (ktime_after(ts, rq->min_vruntime))
        rq->min_vruntime = ts;

Later in the same function, if the queue is empty, we call:

    drm_sched_entity_save_vruntime(entity, rq->min_vruntime);

Since min_vruntime is guaranteed to be greater than or equal to the entity's
vruntime at this point, the ktime_after(vruntime, min_vruntime) check here 
seems mathematically impossible to satisfy. 

Does this mean entities that empty their runqueue will permanently lose 
their accumulated vruntime penalty?

>  		vruntime = ktime_sub(vruntime, min_vruntime);
>  	else
>  		vruntime = 0;

[ ... ]

> @@ -351,27 +324,27 @@ void drm_sched_rq_pop_entity(struct drm_sched_entity *entity)
>  	 * Update the entity's location in the min heap according to
>  	 * the timestamp of the next job, if any.
>  	 */
> +
> +	if (drm_sched_policy == DRM_SCHED_POLICY_FAIR) {
> +		ts = drm_sched_entity_update_vruntime(entity);
> +		if (ktime_after(ts, rq->min_vruntime))
> +			rq->min_vruntime = ts;
> +	}
> +

[Severity: Medium]
Does this unintentionally advance the runqueue's min_vruntime past other
entities still waiting in the RB tree?

By setting rq->min_vruntime to the newly updated vruntime of the currently
executing entity, it seems we might ignore the vruntimes of other entities
that are still queued. 

When new entities join the runqueue, won't their initial vruntime be based 
on this artificially inflated min_vruntime, penalizing them relative to the 
existing waiting entities?

>  	next_job = drm_sched_entity_queue_peek(entity);
>  	if (next_job) {
> -		ktime_t ts;
> -
> -		if (drm_sched_policy == DRM_SCHED_POLICY_FAIR)
> -			ts = drm_sched_entity_get_job_ts(entity);
> -		else if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
> +		if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
>  			ts = next_job->submit_ts;
> -		else
> +		else if (drm_sched_policy == DRM_SCHED_POLICY_RR)
>  			ts = drm_sched_rq_next_rr_ts(rq, entity);
>  
>  		drm_sched_rq_update_fifo_locked(entity, rq, ts);
>  	} else {
> -		drm_sched_rq_remove_fifo_locked(entity, rq);
> -
> -		if (drm_sched_policy == DRM_SCHED_POLICY_FAIR) {
> -			ktime_t min_vruntime;
> +		if (drm_sched_policy == DRM_SCHED_POLICY_FAIR)
> +			drm_sched_entity_save_vruntime(entity,
> +						       rq->min_vruntime);
>  
> -			min_vruntime = drm_sched_rq_get_min_vruntime(rq);
> -			drm_sched_entity_save_vruntime(entity, min_vruntime);
> -		}
> +		drm_sched_rq_remove_fifo_locked(entity, rq);
>  	}
>  
>  	spin_unlock(&rq->lock);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814075739.32481-1-tvrtko.ursulin@igalia.com?part=2

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] A killed and stopped scheduler entity can be unconditionally added back to the runqueue's rbtree, leading to a use-after-free when the entity is subsequently freed by the driver.
--

--- Patch [1]: [PATCH 1/2] drm/sched: Do not restore unsaved virtual runtime ---
commit 8feac77c9420bd8a6d250c6a988250fe755b6870
Author: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Subject: drm/sched: Do not restore unsaved virtual runtime

This patch modifies the queue push/pop and runqueue management paths in the
drm scheduler. It extends the scope of entity->lock to encompass both queue
management and rbtree updates. This prevents a race condition where a new job
could be pushed to an entity before its virtual runtime was correctly saved.

> diff --git a/drivers/gpu/drm/scheduler/sched_rq.c b/drivers/gpu/drm/scheduler/sched_rq.c
> index 0464d324d98d5..23f46ec610e78 100644
> --- a/drivers/gpu/drm/scheduler/sched_rq.c
> +++ b/drivers/gpu/drm/scheduler/sched_rq.c

[ ... ]

> @@ -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) {

[Severity: High]
This is a pre-existing issue, but can this lead to a use-after-free if the
entity is killed concurrently?

If drm_sched_entity_kill() in drivers/gpu/drm/scheduler/sched_entity.c is
called on an entity with multiple pending jobs concurrently with the scheduler
thread popping a job:

drm_sched_entity_kill() {
    ...
    spin_lock(&entity->lock);
    entity->stopped = true;
    drm_sched_rq_remove_entity(entity->rq, entity);
    spin_unlock(&entity->lock);
    ...
}

Meanwhile, the scheduler thread popping a job calls drm_sched_rq_pop_entity()
in drivers/gpu/drm/scheduler/sched_rq.c. Since next_job is not NULL, the code
takes this path and calls drm_sched_rq_update_fifo_locked(), which
unconditionally adds the entity back to the runqueue's rbtree, ignoring
entity->stopped.

When the killer thread wakes up and finishes, the entity is freed but still
linked in the rbtree, which corrupts the rbtree.

Should drm_sched_rq_pop_entity() check entity->stopped before re-adding the
entity to the runqueue, similar to the check in drm_sched_rq_add_entity()?

>  		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);
>  }
>  
>  /**

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814075739.32481-1-tvrtko.ursulin@igalia.com?part=1

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

[Matthew Brost]

On Fri, Aug 14, 2026 at 08:57:38AM +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.
> 
> 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>

Ugh. Idiom:

"How much longer are we going to keep polishing this turd?"

The revert of FAIR IMO shows it basically time for a rewrite, cough -
DRM dep.

If we can agree, I'll prioritize, if not we'll be here seemingly
forever.

Matt

> Cc: Vitaly Prosyak <vitaly.prosyak@amd.com>
> ---
>  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 a4a7efdbf229..673ca9cbf362 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -559,9 +559,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
> @@ -647,6 +648,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 */
> @@ -657,5 +661,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.54.0
> 

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

[Tvrtko Ursulin]

On 14/08/2026 09:53, Matthew Brost wrote:
> On Fri, Aug 14, 2026 at 08:57:38AM +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.
>>
>> 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>
> 
> Ugh. Idiom:
> 
> "How much longer are we going to keep polishing this turd?"
> 
> The revert of FAIR IMO shows it basically time for a rewrite, cough -
> DRM dep.

To give any external readers clarity - hypothetical new DRM dep and fair 
policy for the current scheduler address separate and distinct kernel 
drivers.

Or to put it differently, the regression which warranted a revert does 
not apply at all to any of the drivers DRM dep would be the new solution 
for. Better scheduler for drivers not covered by the DRM dep proposal is 
still required should DRM dep happen or not.

Having clarified that...

> If we can agree, I'll prioritize, if not we'll be here seemingly
> forever.

...for me that's fine albeit unfortunate. I proposed refactoring the 
existing scheduler to split it at the backend submission level by sched 
ops and rq ops, with a future route to split the data structures as 
well, but that was rejected as a direction. So it seems some flavour of 
two schedulers will happen, but will it be DRM dep, or jobq in Rust it's 
up to the (more) interested parties to agree upon.

Regards,

Tvrtko

>> Cc: Vitaly Prosyak <vitaly.prosyak@amd.com>
>> ---
>>   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 a4a7efdbf229..673ca9cbf362 100644
>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>> @@ -559,9 +559,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
>> @@ -647,6 +648,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 */
>> @@ -657,5 +661,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.54.0
>>

Re: [PATCH 2/2] drm/sched: Ensure monotonic min_vruntime

[Tvrtko Ursulin]

On 14/08/2026 09:09, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] drm_sched_entity_save_vruntime() always saves a virtual runtime lag of 0 due to an impossible condition, permanently destroying the entity's relative lag state.
> - [Medium] rq->min_vruntime incorrectly tracks the maximum updated vruntime of the executing entity rather than the true minimum of the runqueue, penalizing newly enqueued entities.
> --
> 
> --- Patch [2]: [PATCH 2/2] drm/sched: Ensure monotonic min_vruntime ---
> commit 840e24cc75ac9a836c633b23cb95dda0e0dc3b75
> Author: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> 
> drm/sched: Ensure monotonic min_vruntime
> 
> min_vruntime handling had a bug where if an entity never exited the run-
> queue it could get penalised by its virtual runtime only ever growing,
> while the periodically exiting and re-joining entities could repeatedly
> get pulled ahead of it.
> 
> Fix it by making the min_vruntime strictly monotonic by tracking it
> separately (an keeping it always up to date) instead of fetching what
> happens to be the top of the tree sorted by virtual runtime.
> 
> While at it replace open coded ktime_t comparison with the correct
> ktime_after() helper.
> 
> Fixes: 2fa4d8e2c109 ("drm/sched: Add fair scheduling policy")
> Tested-by: Luke.Wildhardt@proton.me
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Luke.Wildhardt@proton.me
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Philipp Stanner <phasta@kernel.org>
> Cc: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
> Cc: Vitaly Prosyak <vitaly.prosyak@amd.com>
> 
>> diff --git a/drivers/gpu/drm/scheduler/sched_rq.c b/drivers/gpu/drm/scheduler/sched_rq.c
>> index 23f46ec610e78..bc8363974a276 100644
>> --- a/drivers/gpu/drm/scheduler/sched_rq.c
>> +++ b/drivers/gpu/drm/scheduler/sched_rq.c
> 
> [ ... ]
> 
>> @@ -151,7 +130,7 @@ drm_sched_entity_save_vruntime(struct drm_sched_entity *entity,
>>   
>>   	spin_lock(&stats->lock);
>>   	vruntime = stats->vruntime;
>> -	if (min_vruntime && vruntime > min_vruntime)
>> +	if (ktime_after(vruntime, min_vruntime))
> 
> [Severity: High]
> Will this condition always evaluate to false and unconditionally reset the
> saved lag to 0?
> 
> In drm_sched_rq_pop_entity(), the runqueue's min_vruntime is updated to be
> at least the entity's current vruntime:
> 
> drm_sched_rq_pop_entity() {
>      ...
>      ts = drm_sched_entity_update_vruntime(entity);
>      if (ktime_after(ts, rq->min_vruntime))
>          rq->min_vruntime = ts;
> 
> Later in the same function, if the queue is empty, we call:
> 
>      drm_sched_entity_save_vruntime(entity, rq->min_vruntime);
> 
> Since min_vruntime is guaranteed to be greater than or equal to the entity's
> vruntime at this point, the ktime_after(vruntime, min_vruntime) check here
> seems mathematically impossible to satisfy.
> 
> Does this mean entities that empty their runqueue will permanently lose
> their accumulated vruntime penalty?

Okay, it's not a quick fix so I'll drop 2/2 for now and come back to it 
later. 1/2 stands on it's own anyway.

Regards,

Tvrtko

> 
>>   		vruntime = ktime_sub(vruntime, min_vruntime);
>>   	else
>>   		vruntime = 0;
> 
> [ ... ]
> 
>> @@ -351,27 +324,27 @@ void drm_sched_rq_pop_entity(struct drm_sched_entity *entity)
>>   	 * Update the entity's location in the min heap according to
>>   	 * the timestamp of the next job, if any.
>>   	 */
>> +
>> +	if (drm_sched_policy == DRM_SCHED_POLICY_FAIR) {
>> +		ts = drm_sched_entity_update_vruntime(entity);
>> +		if (ktime_after(ts, rq->min_vruntime))
>> +			rq->min_vruntime = ts;
>> +	}
>> +
> 
> [Severity: Medium]
> Does this unintentionally advance the runqueue's min_vruntime past other
> entities still waiting in the RB tree?
> 
> By setting rq->min_vruntime to the newly updated vruntime of the currently
> executing entity, it seems we might ignore the vruntimes of other entities
> that are still queued.
> 
> When new entities join the runqueue, won't their initial vruntime be based
> on this artificially inflated min_vruntime, penalizing them relative to the
> existing waiting entities?
> 
>>   	next_job = drm_sched_entity_queue_peek(entity);
>>   	if (next_job) {
>> -		ktime_t ts;
>> -
>> -		if (drm_sched_policy == DRM_SCHED_POLICY_FAIR)
>> -			ts = drm_sched_entity_get_job_ts(entity);
>> -		else if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
>> +		if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
>>   			ts = next_job->submit_ts;
>> -		else
>> +		else if (drm_sched_policy == DRM_SCHED_POLICY_RR)
>>   			ts = drm_sched_rq_next_rr_ts(rq, entity);
>>   
>>   		drm_sched_rq_update_fifo_locked(entity, rq, ts);
>>   	} else {
>> -		drm_sched_rq_remove_fifo_locked(entity, rq);
>> -
>> -		if (drm_sched_policy == DRM_SCHED_POLICY_FAIR) {
>> -			ktime_t min_vruntime;
>> +		if (drm_sched_policy == DRM_SCHED_POLICY_FAIR)
>> +			drm_sched_entity_save_vruntime(entity,
>> +						       rq->min_vruntime);
>>   
>> -			min_vruntime = drm_sched_rq_get_min_vruntime(rq);
>> -			drm_sched_entity_save_vruntime(entity, min_vruntime);
>> -		}
>> +		drm_sched_rq_remove_fifo_locked(entity, rq);
>>   	}
>>   
>>   	spin_unlock(&rq->lock);
>