Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Incorrect ordering of `spsc_queue_pop()` and `drm_sched_rq_pop_entity()` causes runqueue corruption, empty entity leakage, and breaks FIFO scheduling. --
--- Patch [2]: [PATCH v2 2/2] drm/sched: Protect entity->last_scheduled with spinlock --- commit fe95a95b4cbec301e14107b82365837fc67259e7 Author: Philipp Stanner <[email protected]> drm/sched: Protect entity->last_scheduled with spinlock This commit replaces the RCU and memory barrier mechanisms used to protect entity->last_scheduled with a spinlock. It aims to improve robustness and readability since the entity lock is already taken in most places that evaluate this field. > diff --git a/drivers/gpu/drm/scheduler/sched_entity.c > b/drivers/gpu/drm/scheduler/sched_entity.c > --- a/drivers/gpu/drm/scheduler/sched_entity.c > +++ b/drivers/gpu/drm/scheduler/sched_entity.c [ ... ] > @@ -555,23 +558,16 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct > drm_sched_entity *entity) > if (entity->guilty && atomic_read(entity->guilty)) > dma_fence_set_error(&sched_job->s_fence->finished, -ECANCELED); > > - dma_fence_put(rcu_dereference_check(entity->last_scheduled, true)); > - rcu_assign_pointer(entity->last_scheduled, > - dma_fence_get(&sched_job->s_fence->finished)); > - > - /* > - * If the queue is empty we allow drm_sched_entity_select_rq() to > - * locklessly access ->last_scheduled. This only works if we set the > - * pointer before we dequeue and if we a write barrier here. > - */ > - smp_wmb(); > - > - spsc_queue_pop(&entity->job_queue); > - > spin_lock(&entity->lock); > + prev_last_scheduled = entity->last_scheduled; > + entity->last_scheduled = dma_fence_get(&sched_job->s_fence->finished); > drm_sched_rq_pop_entity(entity); > spin_unlock(&entity->lock); > > + spsc_queue_pop(&entity->job_queue); [Severity: High] Does moving spsc_queue_pop() after drm_sched_rq_pop_entity() cause runqueue corruption? Looking at drm_sched_rq_pop_entity(), it uses drm_sched_entity_queue_peek() to check the next job in the queue: drivers/gpu/drm/scheduler/sched_rq.c:drm_sched_rq_pop_entity() { ... next_job = drm_sched_entity_queue_peek(entity); if (next_job) { ... drm_sched_rq_update_fifo_locked(entity, rq, ts); } else { drm_sched_rq_remove_fifo_locked(entity, rq); ... } Because the current job hasn't been popped from entity->job_queue yet, won't drm_sched_entity_queue_peek() return the job we are currently processing instead of the subsequent job (or NULL)? It appears this structural inversion causes the runqueue update to evaluate the current job's timestamp rather than the next job, and prevents empty entities from being correctly removed from the runqueue. > + > + dma_fence_put(prev_last_scheduled); [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
