<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"> <meta name="Generator" content="Microsoft Exchange Server"> <style></style> </head> <body> <div> <div dir="auto">No, that won't work. We would still run into lock inversion problems. <div dir="auto"><br> </div> <div dir="auto">What we could do with the scheduler is to turn submissions into dummies if we find that the page tables are now outdated.</div> <div dir="auto"><br> </div> <div dir="auto">But that would be really hacky and I'm not sure if that would really work in all cases.</div> <div dir="auto"><br> </div> <div dir="auto">Christian.</div> </div> <div class="x_gmail_extra"><br> <div class="x_gmail_quote">Am 27.09.2018 08:53 schrieb "Kuehling, Felix" <Felix.Kuehling@amd.com>:<br type="attribution"> </div> </div> </div> <font size="2"><span style="font-size:11pt;"> <div class="PlainText">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?<br> <br> Regards,<br> Felix<br> <br> -----Original Message-----<br> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Christian König<br> Sent: Saturday, September 15, 2018 3:47 AM<br> To: Kuehling, Felix <Felix.Kuehling@amd.com>; Yang, Philip <Philip.Yang@amd.com>; amd-gfx@lists.freedesktop.org; Jerome Glisse <j.glisse@gmail.com><br> Subject: Re: [PATCH] drm/amdgpu: use HMM mirror callback to replace mmu notifier v4<br> <br> Am 14.09.2018 um 22:21 schrieb Felix Kuehling:<br> > On 2018-09-14 01:52 PM, Christian König wrote:<br> >> Am 14.09.2018 um 19:47 schrieb Philip Yang:<br> >>> On 2018-09-14 03:51 AM, Christian König wrote:<br> >>>> Am 13.09.2018 um 23:51 schrieb Felix Kuehling:<br> >>>>> On 2018-09-13 04:52 PM, Philip Yang wrote:<br> >>>>> [SNIP]<br> >>>>>> + amdgpu_mn_read_unlock(amn);<br> >>>>>> +<br> >>>>> amdgpu_mn_read_lock/unlock support recursive locking for multiple <br> >>>>> overlapping or nested invalidation ranges. But if you'r locking <br> >>>>> and unlocking in the same function. Is that still a concern?<br> >>> I don't understand the possible recursive case, but<br> >>> amdgpu_mn_read_lock() still support recursive locking.<br> >>>> Well the real problem is that unlocking them here won't work.<br> >>>><br> >>>> We need to hold the lock until we are sure that the operation which <br> >>>> updates the page tables is completed.<br> >>>><br> >>> The reason for this change is because hmm mirror has <br> >>> invalidate_start callback, no invalidate_end callback<br> >>><br> >>> Check mmu_notifier.c and hmm.c again, below is entire logic to <br> >>> update CPU page tables and callback:<br> >>><br> >>> mn lock amn->lock is used to protect interval tree access because <br> >>> user may submit/register new userptr anytime.<br> >>> This is same for old and new way.<br> >>><br> >>> step 2 guarantee the GPU operation is done before updating CPU page <br> >>> table.<br> >>><br> >>> So I think the change is safe. We don't need hold mn lock until the <br> >>> CPU page tables update is completed.<br> >> No, that isn't even remotely correct. The lock doesn't protects the <br> >> interval tree.<br> >><br> >>> Old:<br> >>> 1. down_read_non_owner(&amn->lock)<br> >>> 2. loop to handle BOs from node->bos through interval tree<br> >>> amn->object nodes<br> >>> gfx: wait for pending BOs fence operation done, mark user <br> >>> pages dirty<br> >>> kfd: evict user queues of the process, wait for queue <br> >>> unmap/map operation done<br> >>> 3. update CPU page tables<br> >>> 4. up_read(&amn->lock)<br> >>><br> >>> New, switch step 3 and 4<br> >>> 1. down_read_non_owner(&amn->lock)<br> >>> 2. loop to handle BOs from node->bos through interval tree<br> >>> amn->object nodes<br> >>> gfx: wait for pending BOs fence operation done, mark user <br> >>> pages dirty<br> >>> kfd: evict user queues of the process, wait for queue <br> >>> unmap/map operation done<br> >>> 3. up_read(&amn->lock)<br> >>> 4. update CPU page tables<br> >> The lock is there to make sure that we serialize page table updates <br> >> with command submission.<br> > As I understand it, the idea is to prevent command submission (adding <br> > new fences to BOs) while a page table invalidation is in progress.<br> <br> Yes, exactly.<br> <br> > But do we really need another lock for this? Wouldn't the <br> > re-validation of userptr BOs (currently calling get_user_pages) force <br> > synchronization with the ongoing page table invalidation through the <br> > mmap_sem or other MM locks?<br> <br> 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.<br> <br> Since we don't modify amdgpu_mn_lock()/amdgpu_mn_unlock() we are certainly not using this mechanism correctly.<br> <br> Regards,<br> Christian.<br> _______________________________________________<br> amd-gfx mailing list<br> amd-gfx@lists.freedesktop.org<br> <a href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx">https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a><br> </div> </span></font> </body> </html>