Re: [PATCH v2 2/2] drm/sched: Protect entity->last_scheduled with spinlock

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