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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Done.  There was a bit of fuzz when applying it.  Please double check it:
https://cgit.freedesktop.org/~agd5f/linux/commit/?h=drm-next&id=a7fbb630c5485f5095146df46f04c2ca1a24c299

Thanks,

Alex


Alex

On Fri, Mar 13, 2020 at 10:51 AM Lucas Stach <l.stach@xxxxxxxxxxxxxx> wrote:
>
> Hi Alex,
>
> since you seem to be picking up scheduler patches, can I ask you to
> take this one through your tree?
>
> Regards,
> Lucas
>
> On Mo, 2020-01-20 at 11:59 +0100, Christian König wrote:
> > 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@xxxxxxxxxxxxxx>
> >
> > Reviewed-by: Christian König <christian.koenig@xxxxxxx>, 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux