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? > 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/[email protected]?part=2
