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

On 14/08/2026 09:09, [email protected] 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 <[email protected]>
> 
> 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: [email protected]
> Cc: Christian König <[email protected]>
> Cc: Danilo Krummrich <[email protected]>
> Cc: [email protected]
> Cc: Matthew Brost <[email protected]>
> Cc: Philipp Stanner <[email protected]>
> Cc: Pierre-Eric Pelloux-Prayer <[email protected]>
> Cc: Vitaly Prosyak <[email protected]>
> 
>> 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);
>