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

From: "Thomas Hellström (VMware)" <thomas_os@shipmail.org>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Thomas Hellstrom <thellstrom@vmware.com>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Huang Rui <ray.huang@amd.com>,
	VMware Graphics <linux-graphics-maintainer@vmware.com>,
	Gerd Hoffmann <kraxel@redhat.com>,
	Daniel Vetter <daniel.vetter@intel.com>,
	Christian Koenig <christian.koenig@amd.com>
Subject: Re: [PATCH 3/3] drm/ttm: remove ttm_bo_wait_unreserved
Date: Wed, 21 Aug 2019 16:27:39 +0200	[thread overview]
Message-ID: <2b1ac3ab-4a21-88cc-e518-90f675528ab4@shipmail.org> (raw)
In-Reply-To: <CAKMK7uE0Zp86jbGyWnNBOsGA=tiNAwYP8WqdH-AHEYthWPf7jw@mail.gmail.com>

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@shipmail.org> 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@intel.com>
>>>> Cc: Christian Koenig <christian.koenig@amd.com>
>>>> Cc: Huang Rui <ray.huang@amd.com>
>>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>>> Cc: "VMware Graphics" <linux-graphics-maintainer@vmware.com>
>>>> Cc: Thomas Hellstrom <thellstrom@vmware.com>
>>>> ---
>>>>    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,

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


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;
         }


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
