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

[sashiko-bot@kernel.org]

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