Re: [PATCH] drm/scheduler: signal fences of jobs pushed to a stopped entity

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] drm/sched: out-of-order fence signaling in drm_sched_entity_push_job()
--

commit 94b284ecd7cfe529710e74fba166d9065b9197ff
Author: Olivier Kaloudoff <olivier.kaloudoff@gmail.com>

drm/scheduler: signal fences of jobs pushed to a stopped entity

This commit checks entity->stopped under entity->lock in
drm_sched_entity_push_job() before pushing a job to the entity queue. When
the entity is stopped, it decrements the scheduler score and schedules
drm_sched_entity_kill_jobs_work() to signal the job fences.

> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> index 7be0387bac3ce..37adb82292088 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -644,6 +644,27 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
>  
>  	spin_lock(&entity->lock);
>  
> +	/*
> +	 * A stopped entity is never put back on a run queue, so a job
> +	 * queued on it would be stranded forever and its fences would
> +	 * never signal, hanging any task waiting on them. Don't queue
> +	 * the job at all; run the normal kill-jobs work on it so its
> +	 * finished fence completes with -ESRCH.
> +	 */
> +	if (unlikely(entity->stopped)) {
> +		spin_unlock(&entity->lock);
> +		/*
> +		 * Balance the atomic_inc(&sched->score) above; the
> +		 * decrement in drm_sched_job_done() only happens when
> +		 * the job actually runs on a scheduler.
> +		 */
> +		atomic_dec(entity->rq->sched->score);
> +		DRM_ERROR("Trying to push to a killed entity\n");
> +		INIT_WORK(&sched_job->work, drm_sched_entity_kill_jobs_work);
> +		schedule_work(&sched_job->work);
> +		return;
> +	}
> +
>  	first = spsc_queue_push(&entity->job_queue, &sched_job->queue_node);
>  
>  	/* first job wakes up scheduler */

[Severity: Critical]
Can scheduling drm_sched_entity_kill_jobs_work() directly here cause
s_fence->scheduled and s_fence->finished to signal out of order relative to
earlier jobs on the same entity?

Before calling drm_sched_entity_push_job(), the caller arms the job via
drm_sched_job_arm() -> drm_sched_fence_init(), which assigns the next
sequence number on entity->fence_context and entity->fence_context + 1.

In drivers/gpu/drm/scheduler/sched_entity.c:drm_sched_entity_kill(), after
setting entity->stopped = true, the function waits for entity->entity_idle
and chains each drained job behind entity->last_scheduled and prior queued
jobs:

	spin_lock(&entity->lock);
	entity->stopped = true;
	drm_sched_rq_remove_entity(entity->rq, entity);
	spin_unlock(&entity->lock);

	/* Make sure this entity is not used by the scheduler at the moment */
	wait_for_completion(&entity->entity_idle);

	spin_lock(&entity->lock);
	prev = dma_fence_get(entity->last_scheduled);
	spin_unlock(&entity->lock);
	while ((job = drm_sched_entity_queue_pop(entity))) {
		struct drm_sched_fence *s_fence = job->s_fence;

		dma_fence_get(&s_fence->finished);
		if (!prev ||
		    dma_fence_add_callback(prev, &job->finish_cb,
					   drm_sched_entity_kill_jobs_cb)) {
			...
			drm_sched_entity_kill_jobs_cb(NULL, &job->finish_cb);
		}

		prev = &s_fence->finished;
	}
	dma_fence_put(prev);

In contrast, when drm_sched_entity_push_job() sees entity->stopped, it
immediately schedules drm_sched_entity_kill_jobs_work() without waiting for
entity->entity_idle, entity->last_scheduled, or the tail of the chain in
drm_sched_entity_kill() (which is only kept in the local variable prev).

Since drm_sched_entity_kill_jobs_work() only waits on fences in
sched_job->dependencies, and jobs on the same entity normally rely on the
entity queue for ordering rather than explicit same-entity dependencies,
this job's fences can signal while an earlier job from the same entity is
still executing on the hardware.

For example, drivers/dma-buf/dma-resv.c:dma_resv_add_fence() replaces an
earlier unsignaled fence from the same context when a later sequence number
is added:

	for (i = 0; i < count; ++i) {
		enum dma_resv_usage old_usage;

		dma_resv_list_entry(fobj, i, obj, &old, &old_usage);
		if ((old->context == fence->context && old_usage >= usage &&
		     dma_fence_is_later_or_same(fence, old)) ||
		    dma_fence_is_signaled(old)) {
			dma_resv_list_set(fobj, i, fence, usage);
			dma_fence_put(old);
			return;
		}
	}

Similarly, in the amdkfd migration path
svm_migrate_vma_to_ram() -> svm_migrate_copy_memory_gart() in
drivers/gpu/drm/amd/amdkfd/kfd_migrate.c, multiple copy jobs are submitted
to the same entity and *mfence is overwritten with the latest job's fence:

		r = amdgpu_copy_buffer(adev, entity,
				       gart_s, gart_d, size * PAGE_SIZE,
				       NULL, &next, true, 0);
		if (r) {
			dev_err(adev->dev, "fail %d to copy memory\n", r);
			goto out_unlock;
		}

		dma_fence_put(*mfence);
		*mfence = next;

If the entity is killed while an earlier job is still in flight and a
subsequent job hits the entity->stopped path in drm_sched_entity_push_job(),
wouldn't the later job's fence signal first and cause callers waiting on
*mfence or dma_resv to unmap and free memory while the earlier job is still
accessing it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022105454.60267-1-olivier.kaloudoff@gmail.com?part=1