Re: [PATCH] drm/scheduler: fix inconsistent locking of job_list_lock

["Christian König" <christian.koenig@amd.com>]

Am 20.01.20 um 11:51 schrieb Lucas Stach:
> 1db8c142b6c5 (drm/scheduler: Add drm_sched_suspend/resume_timeout()) made
> the job_list_lock IRQ safe in as the suspend/resume calls were expected to
> be called from IRQ context. This usage never materialized in upstream.
> Instead amdgpu started locking the job_list_lock in an IRQ unsafe way in
> amdgpu_ib_preempt_mark_partial_job() and amdgpu_ib_preempt_job_recovery(),
> which leads to potential deadlock if one would actually start to call the
> drm_sched_suspend/resume_timeout functions from IRQ context.
>
> As no current user needs the locking to be IRQ safe, the local IRQ
> disable/enable is pure overhead. Fix the inconsistent locking by changing
> all uses of job_list_lock to use the IRQ unsafe locking primitives.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Christian König <christian.koenig@amd.com>, do you want to 
merge through drm-misc-next or should I push it to amd-staging-drm-next?

Regards,
Christian.

> ---
>   drivers/gpu/drm/scheduler/sched_main.c | 38 ++++++++++----------------
>   1 file changed, 15 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index 3c57e84222ca..d1b24444dd99 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -220,8 +220,7 @@ EXPORT_SYMBOL(drm_sched_fault);
>    *
>    * Suspend the delayed work timeout for the scheduler. This is done by
>    * modifying the delayed work timeout to an arbitrary large value,
> - * MAX_SCHEDULE_TIMEOUT in this case. Note that this function can be
> - * called from an IRQ context.
> + * MAX_SCHEDULE_TIMEOUT in this case.
>    *
>    * Returns the timeout remaining
>    *
> @@ -250,43 +249,39 @@ EXPORT_SYMBOL(drm_sched_suspend_timeout);
>    * @sched: scheduler instance for which to resume the timeout
>    * @remaining: remaining timeout
>    *
> - * Resume the delayed work timeout for the scheduler. Note that
> - * this function can be called from an IRQ context.
> + * Resume the delayed work timeout for the scheduler.
>    */
>   void drm_sched_resume_timeout(struct drm_gpu_scheduler *sched,
>   		unsigned long remaining)
>   {
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&sched->job_list_lock, flags);
> +	spin_lock(&sched->job_list_lock);
>   
>   	if (list_empty(&sched->ring_mirror_list))
>   		cancel_delayed_work(&sched->work_tdr);
>   	else
>   		mod_delayed_work(system_wq, &sched->work_tdr, remaining);
>   
> -	spin_unlock_irqrestore(&sched->job_list_lock, flags);
> +	spin_unlock(&sched->job_list_lock);
>   }
>   EXPORT_SYMBOL(drm_sched_resume_timeout);
>   
>   static void drm_sched_job_begin(struct drm_sched_job *s_job)
>   {
>   	struct drm_gpu_scheduler *sched = s_job->sched;
> -	unsigned long flags;
>   
> -	spin_lock_irqsave(&sched->job_list_lock, flags);
> +	spin_lock(&sched->job_list_lock);
>   	list_add_tail(&s_job->node, &sched->ring_mirror_list);
>   	drm_sched_start_timeout(sched);
> -	spin_unlock_irqrestore(&sched->job_list_lock, flags);
> +	spin_unlock(&sched->job_list_lock);
>   }
>   
>   static void drm_sched_job_timedout(struct work_struct *work)
>   {
>   	struct drm_gpu_scheduler *sched;
>   	struct drm_sched_job *job;
> -	unsigned long flags;
>   
>   	sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work);
> +
>   	job = list_first_entry_or_null(&sched->ring_mirror_list,
>   				       struct drm_sched_job, node);
>   
> @@ -303,9 +298,9 @@ static void drm_sched_job_timedout(struct work_struct *work)
>   		}
>   	}
>   
> -	spin_lock_irqsave(&sched->job_list_lock, flags);
> +	spin_lock(&sched->job_list_lock);
>   	drm_sched_start_timeout(sched);
> -	spin_unlock_irqrestore(&sched->job_list_lock, flags);
> +	spin_unlock(&sched->job_list_lock);
>   }
>   
>    /**
> @@ -368,7 +363,6 @@ EXPORT_SYMBOL(drm_sched_increase_karma);
>   void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad)
>   {
>   	struct drm_sched_job *s_job, *tmp;
> -	unsigned long flags;
>   
>   	kthread_park(sched->thread);
>   
> @@ -388,9 +382,9 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad)
>   			 * remove job from ring_mirror_list.
>   			 * Locking here is for concurrent resume timeout
>   			 */
> -			spin_lock_irqsave(&sched->job_list_lock, flags);
> +			spin_lock(&sched->job_list_lock);
>   			list_del_init(&s_job->node);
> -			spin_unlock_irqrestore(&sched->job_list_lock, flags);
> +			spin_unlock(&sched->job_list_lock);
>   
>   			/*
>   			 * Wait for job's HW fence callback to finish using s_job
> @@ -433,7 +427,6 @@ EXPORT_SYMBOL(drm_sched_stop);
>   void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery)
>   {
>   	struct drm_sched_job *s_job, *tmp;
> -	unsigned long flags;
>   	int r;
>   
>   	/*
> @@ -462,9 +455,9 @@ void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery)
>   	}
>   
>   	if (full_recovery) {
> -		spin_lock_irqsave(&sched->job_list_lock, flags);
> +		spin_lock(&sched->job_list_lock);
>   		drm_sched_start_timeout(sched);
> -		spin_unlock_irqrestore(&sched->job_list_lock, flags);
> +		spin_unlock(&sched->job_list_lock);
>   	}
>   
>   	kthread_unpark(sched->thread);
> @@ -648,7 +641,6 @@ static struct drm_sched_job *
>   drm_sched_get_cleanup_job(struct drm_gpu_scheduler *sched)
>   {
>   	struct drm_sched_job *job;
> -	unsigned long flags;
>   
>   	/*
>   	 * Don't destroy jobs while the timeout worker is running  OR thread
> @@ -659,7 +651,7 @@ drm_sched_get_cleanup_job(struct drm_gpu_scheduler *sched)
>   	    __kthread_should_park(sched->thread))
>   		return NULL;
>   
> -	spin_lock_irqsave(&sched->job_list_lock, flags);
> +	spin_lock(&sched->job_list_lock);
>   
>   	job = list_first_entry_or_null(&sched->ring_mirror_list,
>   				       struct drm_sched_job, node);
> @@ -673,7 +665,7 @@ drm_sched_get_cleanup_job(struct drm_gpu_scheduler *sched)
>   		drm_sched_start_timeout(sched);
>   	}
>   
> -	spin_unlock_irqrestore(&sched->job_list_lock, flags);
> +	spin_unlock(&sched->job_list_lock);
>   
>   	return job;
>   }

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel