From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Koenig, Christian" Subject: RE: [PATCH] drm/amdgpu: use HMM mirror callback to replace mmu notifier v4 Date: Thu, 27 Sep 2018 06:59:32 +0000 Message-ID: References: <1536871954-8451-1-git-send-email-Philip.Yang@amd.com> <9d6717ac-23f0-7beb-6e41-58c6e32acdf8@amd.com> <58bc3bb9-b7b1-a32f-e355-c78a23d95215@gmail.com> <383388c8-1bff-48d9-1044-f16e66bcbfa5@amd.com> <3850fbeb-5d91-9c14-43c9-45d5d058e15b@amd.com> , Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0932822097==" Return-path: In-Reply-To: Content-Language: de-DE List-Id: Discussion list for AMD gfx List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Sender: "amd-gfx" To: "Kuehling, Felix" Cc: Jerome Glisse , "Yang, Philip" , "amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org" --===============0932822097== Content-Language: de-DE Content-Type: multipart/alternative; boundary="_000_a76b71ac4b5b45d7b48b6d0e4a7e7524emailandroidcom_" --_000_a76b71ac4b5b45d7b48b6d0e4a7e7524emailandroidcom_ Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable No, that won't work. We would still run into lock inversion problems. What we could do with the scheduler is to turn submissions into dummies if = we find that the page tables are now outdated. But that would be really hacky and I'm not sure if that would really work i= n all cases. Christian. Am 27.09.2018 08:53 schrieb "Kuehling, Felix" : I had a chat with Jerome yesterday. He pointed out that the new blockable p= arameter can be used to infer whether the MMU notifier is being called in = a reclaim operation. So if blockable=3D=3Dtrue, it should even be safe to t= ake the BO reservation lock without problems. I think with that we should b= e able to remove the read-write locking completely and go back to locking (= or try-locking for blockable=3D=3Dfalse) the reservation locks in the MMU n= otifier? Regards, Felix -----Original Message----- From: amd-gfx On Behalf Of Christia= n K=F6nig Sent: Saturday, September 15, 2018 3:47 AM To: Kuehling, Felix ; Yang, Philip ; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org; Jerome Glisse Subject: Re: [PATCH] drm/amdgpu: use HMM mirror callback to replace mmu not= ifier v4 Am 14.09.2018 um 22:21 schrieb Felix Kuehling: > On 2018-09-14 01:52 PM, Christian K=F6nig wrote: >> Am 14.09.2018 um 19:47 schrieb Philip Yang: >>> On 2018-09-14 03:51 AM, Christian K=F6nig 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, b= ut 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-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx --_000_a76b71ac4b5b45d7b48b6d0e4a7e7524emailandroidcom_ Content-Type: text/html; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable
No, that won't work. We would still run into lock inversi= on problems.

What we could do with the scheduler is to turn submission= s into dummies if we find that the page tables are now outdated.

But that would be really hacky and I'm not sure if that w= ould really work in all cases.

Christian.

Am 27.09.2018 08:53 schrieb "Kuehling, Fe= lix" <Felix.Kuehling-5C7GfCeVMHo@public.gmane.org>:
I had a chat with Jerome yesterday. He pointed out= that the new blockable parameter can be used to infer whether the MMU noti= fier is being called  in a reclaim operation. So if blockable=3D=3Dtru= e, it should even be safe to take the BO reservation lock without problems. I think with that we should be able to remove the r= ead-write locking completely and go back to locking (or try-locking for blo= ckable=3D=3Dfalse) the reservation locks in the MMU notifier?

Regards,
  Felix

-----Original Message-----
From: amd-gfx <amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org> On Behalf Of Ch= ristian K=F6nig
Sent: Saturday, September 15, 2018 3:47 AM
To: Kuehling, Felix <Felix.Kuehling-5C7GfCeVMHo@public.gmane.org>; Yang, Philip <Philip= .Yang-5C7GfCeVMHo@public.gmane.org>; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org; Jerome Glisse <j.gliss= e@gmail.com>
Subject: Re: [PATCH] drm/amdgpu: use HMM mirror callback to replace mmu not= ifier v4

Am 14.09.2018 um 22:21 schrieb Felix Kuehling:
> On 2018-09-14 01:52 PM, Christian K=F6nig wrote:
>> Am 14.09.2018 um 19:47 schrieb Philip Yang:
>>> On 2018-09-14 03:51 AM, Christian K=F6nig wrote:
>>>> Am 13.09.2018 um 23:51 schrieb Felix Kuehling:
>>>>> On 2018-09-13 04:52 PM, Philip Yang wrote:
>>>>> [SNIP]
>>>>>>    +    amdgpu_mn_rea= d_unlock(amn);
>>>>>> +
>>>>> amdgpu_mn_read_lock/unlock support recursive locking f= or multiple
>>>>> overlapping or nested invalidation ranges. But if you'= r locking
>>>>> and unlocking in the same function. Is that still a co= ncern?
>>> 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 wo= rk.
>>>>
>>>> We need to hold the lock until we are sure that the operat= ion 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 b= ecause
>>> 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 unti= l the
>>> CPU page tables update is completed.
>> No, that isn't even remotely correct. The lock doesn't protects th= e
>> interval tree.
>>
>>> Old:
>>>     1. down_read_non_owner(&amn->lock) >>>     2. loop to handle BOs from node->bos thr= ough interval tree
>>> amn->object nodes
>>>         gfx: wait for pendi= ng BOs fence operation done, mark user
>>> pages dirty
>>>         kfd: evict user que= ues 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 thr= ough interval tree
>>> amn->object nodes
>>>         gfx: wait for pendi= ng BOs fence operation done, mark user
>>> pages dirty
>>>         kfd: evict user que= ues 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 update= s
>> 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 <= br> > mmap_sem or other MM locks?

No and yes. We don't hold any other locks while doing command submission, b= ut 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-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
https://= lists.freedesktop.org/mailman/listinfo/amd-gfx
 --_000_a76b71ac4b5b45d7b48b6d0e4a7e7524emailandroidcom_-- --===============0932822097== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KYW1kLWdmeCBt YWlsaW5nIGxpc3QKYW1kLWdmeEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5m cmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9hbWQtZ2Z4Cg== --===============0932822097==--