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@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Christian König Sent: Saturday, September 15, 2018 3:47 AM To: Kuehling, Felix <Felix.Kuehling@xxxxxxx>; Yang, Philip <Philip.Yang@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Jerome Glisse <j.glisse@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
