[PATCH v2 7/8] drm/sched: Replace completion with a flush

[PATCH v2 7/8] drm/sched: Replace completion with a flush

[Tvrtko Ursulin]

Due the scheduler locking design, and the inability to always lock both
the entity and the run-queue in the consistent order, a completion exists
which effectively marks the entity as in use from a call path which is not
able to lock it.

When entity is selected from the run job worker, its completion is marked
as non-idle all until the code is sure it will not be dereferencing it any
more, at which point it signals it as idle, releasing the potential
parallel cleanup path.

We can remove the need for this completion by implementing the identical
guarantee by simply flushing the run job work from the cleanup path, after
having removed the entity from the run queue.

We then know that the entity is no longer reachable by the run queue
selection logic, so as soon as any pending work is done the cleanup can
safely proceed. And because we have marked the entity as stopped, we also
know that the entity cannot re-enter the run queue.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <phasta@kernel.org>
Cc: amd-gfx@lists.freedesktop.org
Cc: intel-xe@lists.freedesktop.org
---
 drivers/gpu/drm/scheduler/sched_entity.c   | 18 ++++++++++--------
 drivers/gpu/drm/scheduler/sched_internal.h | 14 ++++++++++++--
 drivers/gpu/drm/scheduler/sched_main.c     |  2 --
 drivers/gpu/drm/scheduler/sched_rq.c       | 19 +++++++++++++------
 include/drm/gpu_scheduler.h                |  9 ---------
 5 files changed, 35 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 3ca671af02ed..79ea20838328 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -161,11 +161,6 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
 			      DRM_SCHED_PRIORITY_KERNEL : priority;
 	entity->rq = sched_list[0]->sched_rq[entity->rq_priority];
 
-	init_completion(&entity->entity_idle);
-
-	/* We start in an idle state. */
-	complete_all(&entity->entity_idle);
-
 	spin_lock_init(&entity->lock);
 	spsc_queue_init(&entity->job_queue);
 
@@ -304,16 +299,23 @@ static void drm_sched_entity_kill_jobs_cb(struct dma_fence *f,
  */
 void drm_sched_entity_kill(struct drm_sched_entity *entity)
 {
+	struct drm_gpu_scheduler *sched;
 	struct drm_sched_job *job;
 	struct dma_fence *prev;
 
 	spin_lock(&entity->lock);
 	entity->stopped = true;
-	drm_sched_rq_remove_entity(entity->rq, entity);
+	sched = 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);
+	/*
+	 * Make sure this entity is not used by the scheduler at the moment.
+	 *
+	 * Scheduler is guaranteed to be stable after the entity was stopped and
+	 * removed from the run-queue.
+	 */
+	if (sched)
+		drm_sched_flush_run_work(sched);
 
 	spin_lock(&entity->lock);
 	prev = dma_fence_get(entity->last_scheduled);
diff --git a/drivers/gpu/drm/scheduler/sched_internal.h b/drivers/gpu/drm/scheduler/sched_internal.h
index 32d3ddb820be..ca724a8a4433 100644
--- a/drivers/gpu/drm/scheduler/sched_internal.h
+++ b/drivers/gpu/drm/scheduler/sched_internal.h
@@ -42,13 +42,23 @@ bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
 			 struct drm_sched_entity *entity);
 void drm_sched_wakeup(struct drm_gpu_scheduler *sched);
 
+/**
+ * drm_sched_flush_run_work - flush the run-job work
+ * @sched: scheduler instance
+ */
+static inline void drm_sched_flush_run_work(struct drm_gpu_scheduler *sched)
+{
+	flush_work(&sched->work_run_job);
+}
+
 void drm_sched_rq_init(struct drm_gpu_scheduler *sched,
 		       struct drm_sched_rq *rq);
 
 struct drm_gpu_scheduler *
 drm_sched_rq_add_entity(struct drm_sched_entity *entity, ktime_t ts);
-void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
-				struct drm_sched_entity *entity);
+struct drm_gpu_scheduler *
+drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
+			   struct drm_sched_entity *entity);
 void drm_sched_rq_pop_entity(struct drm_sched_entity *entity);
 
 struct drm_sched_entity *
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 85a49e219837..281a1e7f89a9 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1033,7 +1033,6 @@ static void drm_sched_run_job_work(struct work_struct *w)
 
 	sched_job = drm_sched_entity_pop_job(entity);
 	if (!sched_job) {
-		complete_all(&entity->entity_idle);
 		drm_sched_run_job_queue(sched);
 		return;
 	}
@@ -1049,7 +1048,6 @@ static void drm_sched_run_job_work(struct work_struct *w)
 	 * refcount has been incremented for the scheduler already.
 	 */
 	fence = sched->ops->run_job(sched_job);
-	complete_all(&entity->entity_idle);
 	drm_sched_fence_scheduled(s_fence, fence);
 
 	if (!IS_ERR_OR_NULL(fence)) {
diff --git a/drivers/gpu/drm/scheduler/sched_rq.c b/drivers/gpu/drm/scheduler/sched_rq.c
index 23f46ec610e7..79f7bdb5b3f0 100644
--- a/drivers/gpu/drm/scheduler/sched_rq.c
+++ b/drivers/gpu/drm/scheduler/sched_rq.c
@@ -297,23 +297,32 @@ drm_sched_rq_add_entity(struct drm_sched_entity *entity, ktime_t ts)
  * @entity: scheduler entity
  *
  * Removes a scheduler entity from the run queue.
+ *
+ * Return: DRM scheduler selected to handle this entity or NULL if entity has
+ * already been removed.
  */
-void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
-				struct drm_sched_entity *entity)
+struct drm_gpu_scheduler *
+drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
+			   struct drm_sched_entity *entity)
 {
+	struct drm_gpu_scheduler *sched;
+
 	lockdep_assert_held(&entity->lock);
 
 	if (list_empty(&entity->list))
-		return;
+		return NULL;
 
 	spin_lock(&rq->lock);
 
-	atomic_dec(rq->sched->score);
+	sched = rq->sched;
+	atomic_dec(sched->score);
 	list_del_init(&entity->list);
 
 	drm_sched_rq_remove_fifo_locked(entity, rq);
 
 	spin_unlock(&rq->lock);
+
+	return sched;
 }
 
 static ktime_t
@@ -407,8 +416,6 @@ drm_sched_rq_select_entity(struct drm_gpu_scheduler *sched,
 				spin_unlock(&rq->lock);
 				return ERR_PTR(-ENOSPC);
 			}
-
-			reinit_completion(&entity->entity_idle);
 			break;
 		}
 	}
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 3b0978fa8cf0..c89a581f54bf 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -27,7 +27,6 @@
 #include <drm/spsc_queue.h>
 #include <linux/average.h>
 #include <linux/dma-fence.h>
-#include <linux/completion.h>
 #include <linux/xarray.h>
 #include <linux/workqueue.h>
 
@@ -232,14 +231,6 @@ struct drm_sched_entity {
 	 */
 	bool 				stopped;
 
-	/**
-	 * @entity_idle:
-	 *
-	 * Signals when entity is not in use, used to sequence entity cleanup in
-	 * drm_sched_entity_fini().
-	 */
-	struct completion		entity_idle;
-
 	/**
 	 * @oldest_job_waiting:
 	 *
-- 
2.56.0

Re: [PATCH v2 7/8] drm/sched: Replace completion with a flush

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] drm/sched: stopped entity re-enters rbtree in drm_sched_rq_pop_entity()
- [Critical] drm/sched: concurrent drm_sched_entity_kill() skips run_work flush
- [High] drm/sched: workqueue deadlock in drm_sched_entity_kill() from free_job
--

commit 271930dfdc1a5516f290d150182ce8d6b1d2358b
Author: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

drm/sched: Replace completion with a flush

This commit replaces struct drm_sched_entity.entity_idle with a flush of
the scheduler's work_run_job work item in drm_sched_entity_kill() after
removing the entity from the run-queue.

> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> index 3ca671af02ed5..79ea20838328d 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
[ ... ]
> @@ -304,16 +299,23 @@ static void drm_sched_entity_kill_jobs_cb(struct dma_fence *f,
>   */
>  void drm_sched_entity_kill(struct drm_sched_entity *entity)
>  {
> +	struct drm_gpu_scheduler *sched;
>  	struct drm_sched_job *job;
>  	struct dma_fence *prev;
>  
>  	spin_lock(&entity->lock);
>  	entity->stopped = true;
> -	drm_sched_rq_remove_entity(entity->rq, entity);
> +	sched = 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);
> +	/*
> +	 * Make sure this entity is not used by the scheduler at the moment.
> +	 *
> +	 * Scheduler is guaranteed to be stable after the entity was stopped and
> +	 * removed from the run-queue.
> +	 */
> +	if (sched)
> +		drm_sched_flush_run_work(sched);

[Severity: Critical]
Can an in-flight drm_sched_run_job_work() re-insert the stopped entity
back into rq->rb_tree_root while drm_sched_entity_kill() is waiting inside
drm_sched_flush_run_work()?

When an entity has two or more queued jobs and drm_sched_run_job_work()
selects it via drm_sched_select_entity() -> drm_sched_rq_select_entity(),
rq->lock is dropped before calling drm_sched_entity_pop_job(entity), which
peeks the first job and checks its dependencies without holding rq->lock
or entity->lock.

If drm_sched_entity_kill() runs during that unlocked window, it sets
entity->stopped = true, removes entity->list and entity->rb_tree_node via
drm_sched_rq_remove_entity(), and blocks in drm_sched_flush_run_work()
before draining entity->job_queue.

The in-flight drm_sched_entity_pop_job() then acquires entity->lock, pops
the first job, and calls drm_sched_rq_pop_entity(entity):

drivers/gpu/drm/scheduler/sched_entity.c:drm_sched_entity_pop_job() {
	...
	spin_lock(&entity->lock);
	prev_last_scheduled = entity->last_scheduled;
	entity->last_scheduled = dma_fence_get(&sched_job->s_fence->finished);
	spsc_queue_pop(&entity->job_queue);
	drm_sched_rq_pop_entity(entity);
	spin_unlock(&entity->lock);
	...
}

In drm_sched_rq_pop_entity(), neither entity->stopped nor
list_empty(&entity->list) is checked, and because the second job is still
in entity->job_queue, next_job is non-NULL:

drivers/gpu/drm/scheduler/sched_rq.c:drm_sched_rq_pop_entity() {
	...
	next_job = drm_sched_entity_queue_peek(entity);
	if (next_job) {
		ktime_t ts;
		...
		drm_sched_rq_update_fifo_locked(entity, rq, ts);
	}
	...
}

This re-inserts entity->rb_tree_node into rq->rb_tree_root before
drm_sched_run_job_work() re-queues sched->work_run_job and finishes.

Once drm_sched_flush_run_work() returns, won't the re-queued
drm_sched_run_job_work() be able to select the entity again concurrently
with drm_sched_entity_kill() draining entity->job_queue and
drm_sched_entity_fini() freeing the entity, or leave a dangling
entity->rb_tree_node in rq->rb_tree_root because subsequent calls to
drm_sched_rq_remove_entity() see list_empty(&entity->list) and return NULL
early?

[Severity: High]
Can calling drm_sched_flush_run_work(sched) here deadlock when
drm_sched_entity_kill() is invoked from a driver's free_job() callback on
sched->submit_wq?

When an entity pushes a job, drm_sched_rq_add_entity() links entity->list
onto rq->entities, and entity->list stays linked even after all jobs have
been popped and completed until drm_sched_rq_remove_entity() is called in
drm_sched_entity_kill(). As a result, drm_sched_rq_remove_entity() returns
a non-NULL sched for any entity that has ever submitted a job, causing
drm_sched_entity_kill() to call flush_work(&sched->work_run_job).

By default, sched->submit_wq is allocated via drm_sched_alloc_wq() as an
ordered workqueue (max_active = 1) shared by sched->work_run_job and
sched->work_free_job. In drivers such as msm and panfrost, freeing a job in
drm_sched_free_job_work() -> sched->ops->free_job() can drop the last
reference to a closed context or submitqueue and tear down its entities:

drm_sched_free_job_work()
  -> msm_job_free()
    -> msm_gem_submit_put()
      -> __msm_gem_submit_destroy()
        -> msm_submitqueue_put() / msm_context_put()
          -> __msm_context_destroy()
            -> drm_sched_entity_destroy()
              -> drm_sched_entity_fini()
                -> drm_sched_entity_kill()
                  -> drm_sched_flush_run_work()
                    -> flush_work(&sched->work_run_job)

and similarly via panfrost_job_free() -> panfrost_job_put() ->
panfrost_job_cleanup() -> panfrost_jm_ctx_put() ->
panfrost_jm_ctx_release() -> drm_sched_entity_destroy().

If sched->work_run_job is queued on the ordered sched->submit_wq when
drm_sched_free_job_work() runs this path, won't flush_work(&sched->work_run_job)
deadlock waiting for work_run_job, which cannot start until
drm_sched_free_job_work() finishes (and trigger a recursive lockdep
warning on drm_sched_lockdep_map)?

>  
>  	spin_lock(&entity->lock);
>  	prev = dma_fence_get(entity->last_scheduled);
[ ... ]
> diff --git a/drivers/gpu/drm/scheduler/sched_rq.c b/drivers/gpu/drm/scheduler/sched_rq.c
> index 23f46ec610e78..79f7bdb5b3f0a 100644
> --- a/drivers/gpu/drm/scheduler/sched_rq.c
> +++ b/drivers/gpu/drm/scheduler/sched_rq.c
> @@ -297,23 +297,32 @@ drm_sched_rq_add_entity(struct drm_sched_entity *entity, ktime_t ts)
>   * @entity: scheduler entity
>   *
>   * Removes a scheduler entity from the run queue.
> + *
> + * Return: DRM scheduler selected to handle this entity or NULL if entity has
> + * already been removed.
>   */
> -void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
> -				struct drm_sched_entity *entity)
> +struct drm_gpu_scheduler *
> +drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
> +			   struct drm_sched_entity *entity)
>  {
> +	struct drm_gpu_scheduler *sched;
> +
>  	lockdep_assert_held(&entity->lock);
>  
>  	if (list_empty(&entity->list))
> -		return;
> +		return NULL;
>  
>  	spin_lock(&rq->lock);
>  
> -	atomic_dec(rq->sched->score);
> +	sched = rq->sched;
> +	atomic_dec(sched->score);
>  	list_del_init(&entity->list);
>  
>  	drm_sched_rq_remove_fifo_locked(entity, rq);
>  
>  	spin_unlock(&rq->lock);
> +
> +	return sched;
>  }

[Severity: Critical]
What happens if two threads call drm_sched_entity_kill() on the same
entity concurrently (for instance, one thread in file_operations->flush
calling drm_sched_entity_flush() -> drm_sched_entity_kill() upon SIGKILL,
and another thread closing the context and calling drm_sched_entity_fini()
-> drm_sched_entity_kill()) while drm_sched_run_job_work() is using the
entity?

Previously, both callers waited on wait_for_completion(&entity->entity_idle)
until drm_sched_run_job_work() called complete_all(&entity->entity_idle).

With this change, the first caller into drm_sched_rq_remove_entity() runs
list_del_init(&entity->list), receives the non-NULL sched pointer, drops
entity->lock, and blocks in drm_sched_flush_run_work(sched).

When the second caller enters drm_sched_rq_remove_entity(), it sees
list_empty(&entity->list) and gets NULL back, so in drm_sched_entity_kill()
it skips drm_sched_flush_run_work(sched) and proceeds directly to draining
entity->job_queue and freeing the entity in drm_sched_entity_fini() while
drm_sched_run_job_work() is still running. Can this cause a use-after-free
and concurrent SPSC queue access on entity->job_queue?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020154123.33631-1-tvrtko.ursulin@igalia.com?part=7