[PATCH] drm/amdgpu: use HMM mirror callback to replace mmu notifier v4

[Kuehling, Felix]

I had a chat with Jerome yesterday. He pointed out that the new blockable parameter can be used to infer whether the MMU notifier is being called  in a reclaim operation. So if blockable==true, it should even be safe to take the BO reservation lock without problems. I think with that we should be able to remove the read-write locking completely and go back to locking (or try-locking for blockable==false) the reservation locks in the MMU notifier?

Regards,
  Felix

-----Original Message-----
From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of Christian König
Sent: Saturday, September 15, 2018 3:47 AM
To: Kuehling, Felix <Felix.Kuehling at amd.com>; Yang, Philip <Philip.Yang at amd.com>; amd-gfx at lists.freedesktop.org; Jerome Glisse <j.glisse at gmail.com>
Subject: Re: [PATCH] drm/amdgpu: use HMM mirror callback to replace mmu notifier v4

Am 14.09.2018 um 22:21 schrieb Felix Kuehling:
> On 2018-09-14 01:52 PM, Christian König wrote:
>> Am 14.09.2018 um 19:47 schrieb Philip Yang:
>>> On 2018-09-14 03:51 AM, Christian König wrote:
>>>> Am 13.09.2018 um 23:51 schrieb Felix Kuehling:
>>>>> On 2018-09-13 04:52 PM, Philip Yang wrote:
>>>>> [SNIP]
>>>>>>    +    amdgpu_mn_read_unlock(amn);
>>>>>> +
>>>>> amdgpu_mn_read_lock/unlock support recursive locking for multiple 
>>>>> overlapping or nested invalidation ranges. But if you'r locking 
>>>>> and unlocking in the same function. Is that still a concern?
>>> I don't understand the possible recursive case, but
>>> amdgpu_mn_read_lock() still support recursive locking.
>>>> Well the real problem is that unlocking them here won't work.
>>>>
>>>> We need to hold the lock until we are sure that the operation which 
>>>> updates the page tables is completed.
>>>>
>>> The reason for this change is because hmm mirror has 
>>> invalidate_start callback, no invalidate_end callback
>>>
>>> Check mmu_notifier.c and hmm.c again, below is entire logic to 
>>> update CPU page tables and callback:
>>>
>>> mn lock amn->lock is used to protect interval tree access because 
>>> user may submit/register new userptr anytime.
>>> This is same for old and new way.
>>>
>>> step 2 guarantee the GPU operation is done before updating CPU page 
>>> table.
>>>
>>> So I think the change is safe. We don't need hold mn lock until the 
>>> CPU page tables update is completed.
>> No, that isn't even remotely correct. The lock doesn't protects the 
>> interval tree.
>>
>>> Old:
>>>     1. down_read_non_owner(&amn->lock)
>>>     2. loop to handle BOs from node->bos through interval tree
>>> amn->object nodes
>>>         gfx: wait for pending BOs fence operation done, mark user 
>>> pages dirty
>>>         kfd: evict user queues of the process, wait for queue 
>>> unmap/map operation done
>>>     3. update CPU page tables
>>>     4. up_read(&amn->lock)
>>>
>>> New, switch step 3 and 4
>>>     1. down_read_non_owner(&amn->lock)
>>>     2. loop to handle BOs from node->bos through interval tree
>>> amn->object nodes
>>>         gfx: wait for pending BOs fence operation done, mark user 
>>> pages dirty
>>>         kfd: evict user queues of the process, wait for queue 
>>> unmap/map operation done
>>>     3. up_read(&amn->lock)
>>>     4. update CPU page tables
>> The lock is there to make sure that we serialize page table updates 
>> with command submission.
> As I understand it, the idea is to prevent command submission (adding 
> new fences to BOs) while a page table invalidation is in progress.

Yes, exactly.

> But do we really need another lock for this? Wouldn't the 
> re-validation of userptr BOs (currently calling get_user_pages) force 
> synchronization with the ongoing page table invalidation through the 
> mmap_sem or other MM locks?

No and yes. We don't hold any other locks while doing command submission, but I expect that HMM has its own mechanism to prevent that.

Since we don't modify amdgpu_mn_lock()/amdgpu_mn_unlock() we are certainly not using this mechanism correctly.

Regards,
Christian.
_______________________________________________
amd-gfx mailing list
amd-gfx at lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx