[PATCH 1/6] drm/ttm: Add unampping of the entire device address space

[Thomas Hellström (Intel)]

On 6/10/20 5:30 PM, Daniel Vetter wrote:
> On Wed, Jun 10, 2020 at 04:05:04PM +0200, Christian König wrote:
>> Am 10.06.20 um 15:54 schrieb Andrey Grodzovsky:
>>>
>>> On 6/10/20 6:15 AM, Thomas Hellström (Intel) wrote:
>>>>
>>>> On 6/9/20 7:21 PM, Koenig, Christian wrote:
>>>>>
>>>>> Am 09.06.2020 18:37 schrieb "Grodzovsky, Andrey"
>>>>> <Andrey.Grodzovsky at amd.com>:
>>>>>
>>>>>
>>>>>      On 6/5/20 2:40 PM, Christian König wrote:
>>>>>      > Am 05.06.20 um 16:29 schrieb Andrey Grodzovsky:
>>>>>      >>
>>>>>      >> On 5/11/20 2:45 AM, Christian König wrote:
>>>>>      >>> Am 09.05.20 um 20:51 schrieb Andrey Grodzovsky:
>>>>>      >>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky at amd.com>
>>>>>      >>>> ---
>>>>>      >>>> drivers/gpu/drm/ttm/ttm_bo.c    | 22 +++++++++++++++++++++-
>>>>>      >>>> include/drm/ttm/ttm_bo_driver.h |  2 ++
>>>>>      >>>>   2 files changed, 23 insertions(+), 1 deletion(-)
>>>>>      >>>>
>>>>>      >>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c
>>>>>      >>>> b/drivers/gpu/drm/ttm/ttm_bo.c
>>>>>      >>>> index c5b516f..eae61cc 100644
>>>>>      >>>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>>>>>      >>>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>>>>>      >>>> @@ -1750,9 +1750,29 @@ void ttm_bo_unmap_virtual(struct
>>>>>      >>>> ttm_buffer_object *bo)
>>>>>      >>>> ttm_bo_unmap_virtual_locked(bo);
>>>>>      >>>> ttm_mem_io_unlock(man);
>>>>>      >>>>   }
>>>>>      >>>> +EXPORT_SYMBOL(ttm_bo_unmap_virtual);
>>>>>      >>>>   +void ttm_bo_unmap_virtual_address_space(struct
>>>>>      ttm_bo_device *bdev)
>>>>>      >>>> +{
>>>>>      >>>> +    struct ttm_mem_type_manager *man;
>>>>>      >>>> +    int i;
>>>>>      >>>> -EXPORT_SYMBOL(ttm_bo_unmap_virtual);
>>>>>      >>>
>>>>>      >>>> +    for (i = 0; i < TTM_NUM_MEM_TYPES; i++) {
>>>>>      >>>> +        man = &bdev->man[i];
>>>>>      >>>> +        if (man->has_type && man->use_type)
>>>>>      >>>> + ttm_mem_io_lock(man, false);
>>>>>      >>>> +    }
>>>>>      >>>
>>>>>      >>> You should drop that it will just result in a deadlock
>>>>>      warning for
>>>>>      >>> Nouveau and has no effect at all.
>>>>>      >>>
>>>>>      >>> Apart from that looks good to me,
>>>>>      >>> Christian.
>>>>>      >>
>>>>>      >>
>>>>>      >> As I am considering to re-include this in V2 of the
>>>>>      patchsets, can
>>>>>      >> you clarify please why this will have no effect at all ?
>>>>>      >
>>>>>      > The locks are exclusive for Nouveau to allocate/free the io
>>>>>      address
>>>>>      > space.
>>>>>      >
>>>>>      > Since we don't do this here we don't need the locks.
>>>>>      >
>>>>>      > Christian.
>>>>>
>>>>>
>>>>>      So basically calling unmap_mapping_range doesn't require any extra
>>>>>      locking around it and whatever locks are taken within the function
>>>>>      should be enough ?
>>>>>
>>>>>
>>>>>
>>>>> I think so, yes.
>>>>>
>>>>> Christian.
>>>> Yes, that's true. However, without the bo reservation, nothing stops
>>>> a PTE from being immediately re-faulted back again. Even while
>>>> unmap_mapping_range() is running.
>>>>
>>> Can you explain more on this - specifically, which function to reserve
>>> the BO, why BO reservation would prevent re-fault of the PTE ?
>>>
>> Thomas is talking about ttm_bo_reserver()/ttm_bo_unreserve(), but we don't
>> need this because we unmap everything because the whole device is gone and
>> not just manipulate a single BO.
>>
>>>> So the device removed flag needs to be advertized before this
>>>> function is run,
>>>>
>>> I indeed intend to call this  right after calling drm_dev_unplug from
>>> amdgpu_pci_remove while adding drm_dev_enter/exit in ttm_bo_vm_fault (or
>>> in amdgpu specific wrapper since I don't see how can I access struct
>>> drm_device from ttm_bo_vm_fault) and this in my understanding should
>>> stop a PTE from being re-faulted back as you pointed out - so again I
>>> don't see how  bo reservation would prevent it so it looks like I am
>>> missing something...
>>>
>>>
>>>> (perhaps with a memory barrier pair).
>>>>
>>> drm_dev_unplug and drm_dev_enter/exit are RCU synchronized and so I
>>> don't think require any extra memory barriers for visibility of the
>>> removed flag being set
>>>
>> As far as I can see that should be perfectly sufficient.
> Only if you have a drm_dev_enter/exit pair in your fault handler.
> Otherwise you're still open to the races Thomas described. But aside from
> that the drm_dev_unplug stuff has all the barriers and stuff to make sure
> nothing escapes.
>
> Failure to drm_dev_enter could then also trigger the special case where we
> put a dummy page in place.
> -Daniel

Hmm, Yes, indeed advertizing the flag before the call to 
unmap_mapping_range isn't enough, since there might be fault handlers 
running that haven't picked up the flag when unmap_mapping_range is 
launched.

For the special case of syncing a full address-space 
unmap_mapping_range() with fault handlers regardless of the reason for 
the full address-space unmap_mapping_range() one could either traverse 
the address space (drm_vma_manager) and grab *all* bo reservations 
around the unmap_mapping_range(), or grab the i_mmap_lock in read mode 
in the fault handler. (It's taken in write mode in unmap_mapping_range). 
While the latter may seem like a simple solution, one should probably 
consider the overhead both in run-time and scaling ability.

/Thomas