Re: [PATCH 3/3] drm/ttm: remove ttm_bo_wait_unreserved

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

 



On Wed, Aug 21, 2019 at 4:27 PM Thomas Hellström (VMware)
<thomas_os@xxxxxxxxxxxx> wrote:
> On 8/21/19 4:09 PM, Daniel Vetter wrote:
> > On Wed, Aug 21, 2019 at 2:47 PM Thomas Hellström (VMware)
> > <thomas_os@xxxxxxxxxxxx> wrote:
> >> On 8/21/19 2:40 PM, Thomas Hellström (VMware) wrote:
> >>> On 8/20/19 4:53 PM, Daniel Vetter wrote:
> >>>> With nouveau fixed all ttm-using drives have the correct nesting of
> >>>> mmap_sem vs dma_resv, and we can just lock the buffer.
> >>>>
> >>>> Assuming I didn't screw up anything with my audit of course.
> >>>>
> >>>> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> >>>> Cc: Christian Koenig <christian.koenig@xxxxxxx>
> >>>> Cc: Huang Rui <ray.huang@xxxxxxx>
> >>>> Cc: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> >>>> Cc: "VMware Graphics" <linux-graphics-maintainer@xxxxxxxxxx>
> >>>> Cc: Thomas Hellstrom <thellstrom@xxxxxxxxxx>
> >>>> ---
> >>>>    drivers/gpu/drm/ttm/ttm_bo.c    | 34 ---------------------------------
> >>>>    drivers/gpu/drm/ttm/ttm_bo_vm.c | 26 +------------------------
> >>>>    include/drm/ttm/ttm_bo_api.h    |  1 -
> >>>>    3 files changed, 1 insertion(+), 60 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> >>>> index 20ff56f27aa4..a952dd624b06 100644
> >>>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> >>>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> >>>> @@ -1954,37 +1954,3 @@ void ttm_bo_swapout_all(struct ttm_bo_device
> >>>> *bdev)
> >>>>            ;
> >>>>    }
> >>>>    EXPORT_SYMBOL(ttm_bo_swapout_all);
> >>>> -
> >>>> -/**
> >>>> - * ttm_bo_wait_unreserved - interruptible wait for a buffer object
> >>>> to become
> >>>> - * unreserved
> >>>> - *
> >>>> - * @bo: Pointer to buffer
> >>>> - */
> >>>> -int ttm_bo_wait_unreserved(struct ttm_buffer_object *bo)
> >>>> -{
> >>>> -    int ret;
> >>>> -
> >>>> -    /*
> >>>> -     * In the absense of a wait_unlocked API,
> >>>> -     * Use the bo::wu_mutex to avoid triggering livelocks due to
> >>>> -     * concurrent use of this function. Note that this use of
> >>>> -     * bo::wu_mutex can go away if we change locking order to
> >>>> -     * mmap_sem -> bo::reserve.
> >>>> -     */
> >>>> -    ret = mutex_lock_interruptible(&bo->wu_mutex);
> >>>> -    if (unlikely(ret != 0))
> >>>> -        return -ERESTARTSYS;
> >>>> -    if (!dma_resv_is_locked(bo->base.resv))
> >>>> -        goto out_unlock;
> >>>> -    ret = dma_resv_lock_interruptible(bo->base.resv, NULL);
> >>>> -    if (ret == -EINTR)
> >>>> -        ret = -ERESTARTSYS;
> >>>> -    if (unlikely(ret != 0))
> >>>> -        goto out_unlock;
> >>>> -    dma_resv_unlock(bo->base.resv);
> >>>> -
> >>>> -out_unlock:
> >>>> -    mutex_unlock(&bo->wu_mutex);
> >>>> -    return ret;
> >>>> -}
> >>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c
> >>>> b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> >>>> index 76eedb963693..505e1787aeea 100644
> >>>> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
> >>>> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> >>>> @@ -125,31 +125,7 @@ static vm_fault_t ttm_bo_vm_fault(struct
> >>>> vm_fault *vmf)
> >>>>            &bdev->man[bo->mem.mem_type];
> >>>>        struct vm_area_struct cvma;
> >>>>    -    /*
> >>>> -     * Work around locking order reversal in fault / nopfn
> >>>> -     * between mmap_sem and bo_reserve: Perform a trylock operation
> >>>> -     * for reserve, and if it fails, retry the fault after waiting
> >>>> -     * for the buffer to become unreserved.
> >>>> -     */
> >>>> -    if (unlikely(!dma_resv_trylock(bo->base.resv))) {
> >>>> -        if (vmf->flags & FAULT_FLAG_ALLOW_RETRY) {
> >>>> -            if (!(vmf->flags & FAULT_FLAG_RETRY_NOWAIT)) {
> >>>> -                ttm_bo_get(bo);
> >>>> - up_read(&vmf->vma->vm_mm->mmap_sem);
> >>>> -                (void) ttm_bo_wait_unreserved(bo);
> >>>> -                ttm_bo_put(bo);
> >>>> -            }
> >>>> -
> >>>> -            return VM_FAULT_RETRY;
> >>>> -        }
> >>>> -
> >>>> -        /*
> >>>> -         * If we'd want to change locking order to
> >>>> -         * mmap_sem -> bo::reserve, we'd use a blocking reserve here
> >>>> -         * instead of retrying the fault...
> >>>> -         */
> >>> I think you should justify why the above code is removed, since the
> >>> comments actually outlines what to do if we change locking order.
> >>>
> >>> The code that's removed above is not for adjusting locking orders but
> >>> to decrease the mm latency by releasing the mmap_sem while waiting for
> >>> bo reserve which in turn might be waiting for GPU. At a minimum we
> >>> should have a separate patch with justification.
> >>>
> >>> Note that the caller here ensures locking progress by adjusting the
> >>> RETRY flags after a retry.
> > That would be patches 1&2 in this series.
> >
> Hmm? Those seem to touch only dma-buf and nouveau not ttm?  I mean this
> patch should look along the lines of (based on an older tree) to
> implement the new locking-order mmap_sem->reservation,

Only nouveau was breaking was doing copy_*_user or get_user_pages
while holding dma_resv locks, no one else. So nothing else needed to
be changed. But patch 1 contains the full audit. I might have missed
something.

> but to keep the mm latency optimization using the RETRY functionality:

Still no idea why this is needed? All the comments here and the code
and history seem like they've been about the mmap_sem vs dma_resv
inversion between driver ioctls and fault handling here. Once that's
officially fixed there's no reason to play games here and retry loops
- previously that was necessary because the old ttm_bo_vm_fault had a
busy spin and that's definitely not nice. If it's needed I think it
should be a second patch on top, to keep this all clear. I had to
audit an enormous amount of code, I'd like to make sure I didn't miss
anything before we start to make this super fancy again. Further
patches on top is obviously all fine with me.
-Daniel

> Thanks,
> Thomas
>
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> index 85f5bcbe0c76..68482c67b9f7 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> @@ -125,30 +125,20 @@ static vm_fault_t ttm_bo_vm_fault(struct vm_fault *vmf)
>                  &bdev->man[bo->mem.mem_type];
>          struct vm_area_struct cvma;
>
> -       /*
> -        * Work around locking order reversal in fault / nopfn
> -        * between mmap_sem and bo_reserve: Perform a trylock operation
> -        * for reserve, and if it fails, retry the fault after waiting
> -        * for the buffer to become unreserved.
> -        */
> +       /* Avoid blocking on reservation with mmap_sem held, if possible */
>          if (unlikely(!reservation_object_trylock(bo->base.resv))) {
> -               if (vmf->flags & FAULT_FLAG_ALLOW_RETRY) {
> -                       if (!(vmf->flags & FAULT_FLAG_RETRY_NOWAIT)) {
> -                               ttm_bo_get(bo);
> -                               up_read(&vmf->vma->vm_mm->mmap_sem);
> -                               (void) ttm_bo_wait_unreserved(bo);
> -                               ttm_bo_put(bo);
> -                       }
> +               if ((vmf->flags & FAULT_FLAG_ALLOW_RETRY) &&
> +                   !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT)) {
> +                       ttm_bo_get(bo);
> +                       up_read(&vmf->vma->vm_mm->mmap_sem);
> +                       (void) ttm_bo_wait_unreserved(bo);
> +                       ttm_bo_put(bo);
>
>                          return VM_FAULT_RETRY;
>                  }
>
> -               /*
> -                * If we'd want to change locking order to
> -                * mmap_sem -> bo::reserve, we'd use a blocking reserve here
> -                * instead of retrying the fault...
> -                */
> -               return VM_FAULT_NOPAGE;
> +               if (reservation_object_lock_interruptible(bo->base.resv, NULL))
> +                       return VM_FAULT_NOPAGE;
>          }
>
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux