<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> <p>I tested it that it saves a lot of vram on KFD big buffer stress test. I think there are two reasons:</p> <p>1. Calling <font size="2"><span style="font-size:11pt">amdgpu_vm_update_ptes() during unmapping will allocate unnecessary pts, because there is no flag to determine if the VA is mapping or unmapping in function </span></font><br> <font size="2"><span style="font-size:11pt"><font size="2"><span style="font-size:11pt">amdgpu_vm_update_ptes(). It saves the most of memory.</span></font></span></font></p> <p><font size="2"><span style="font-size:11pt"><font size="2"><span style="font-size:11pt">2. Intentionally removing those unmapping pts is logical expectation, although it is not removing so much pts.<br> </span></font></span></font></p> <p><font size="2"><span style="font-size:11pt"><font size="2"><span style="font-size:11pt">Regards,</span></font></span></font></p> <p><font size="2"><span style="font-size:11pt"><font size="2"><span style="font-size:11pt">Eric<br> </span></font></span></font></p> <div class="moz-cite-prefix">On 2019-10-30 11:57 a.m., Koenig, Christian wrote:<br> </div> <blockquote type="cite" cite="mid:3b01b638-a678-42e6-900e-bff3593874c4@email.android.com"> <div dir="auto"> <div><br> <div class="gmail_extra"><br> <div class="gmail_quote">Am 30.10.2019 16:47 schrieb "Kuehling, Felix" <a class="moz-txt-link-rfc2396E" href="mailto:Felix.Kuehling@amd.com"> <Felix.Kuehling@amd.com></a>:<br type="attribution"> <blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><font size="2"><span style="font-size:11pt"> <div>On 2019-10-30 9:52 a.m., Christian König wrote:<br> > Am 29.10.19 um 21:06 schrieb Huang, JinHuiEric:<br> >> The issue is PT BOs are not freed when unmapping VA,<br> >> which causes vram usage accumulated is huge in some<br> >> memory stress test, such as kfd big buffer stress test.<br> >> Function amdgpu_vm_bo_update_mapping() is called by both<br> >> amdgpu_vm_bo_update() and amdgpu_vm_clear_freed(). The<br> >> solution is replacing amdgpu_vm_bo_update_mapping() in<br> >> amdgpu_vm_clear_freed() with removing PT BOs function<br> >> to save vram usage.<br> ><br> > NAK, that is intentional behavior.<br> ><br> > Otherwise we can run into out of memory situations when page tables <br> > need to be allocated again under stress.<br> <br> That's a bit arbitrary and inconsistent. We are freeing page tables in <br> other situations, when a mapping uses huge pages in <br> amdgpu_vm_update_ptes. Why not when a mapping is destroyed completely?<br> <br> I'm actually a bit surprised that the huge-page handling in <br> amdgpu_vm_update_ptes isn't kicking in to free up lower-level page <br> tables when a BO is unmapped.<br> </div> </span></font></div> </blockquote> </div> </div> </div> <div dir="auto"><br> </div> <div dir="auto">Well it does free the lower level, and that is already causing problems (that's why I added the reserved space).</div> <div dir="auto"><br> </div> <div dir="auto">What we don't do is freeing the higher levels.</div> <div dir="auto"><br> </div> <div dir="auto">E.g. when you free a 2MB BO we free the lowest level, if we free a 1GB BO we free the two lowest levels etc...</div> <div dir="auto"><br> </div> <div dir="auto">The problem with freeing the higher levels is that you don't know who is also using this. E.g. we would need to check all entries when we unmap one.</div> <div dir="auto"><br> </div> <div dir="auto">It's simply not worth it for a maximum saving of 2MB per VM.</div> <div dir="auto"><br> </div> <div dir="auto">Writing this I'm actually wondering how you ended up in this issue? There shouldn't be much savings from this.</div> <div dir="auto"><br> </div> <div dir="auto">Regards,</div> <div dir="auto">Christian.</div> <div dir="auto"><br> </div> <div dir="auto"> <div class="gmail_extra"> <div class="gmail_quote"> <blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><font size="2"><span style="font-size:11pt"> <div><br> Regards,<br> Felix<br> <br> <br> ><br> > Regards,<br> > Christian.<br> ><br> >><br> >> Change-Id: Ic24e35bff8ca85265b418a642373f189d972a924<br> >> Signed-off-by: Eric Huang <a class="moz-txt-link-rfc2396E" href="mailto:JinhuiEric.Huang@amd.com"> <JinhuiEric.Huang@amd.com></a><br> >> ---<br> >> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 56 <br> >> +++++++++++++++++++++++++++++-----<br> >> 1 file changed, 48 insertions(+), 8 deletions(-)<br> >><br> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c <br> >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c<br> >> index 0f4c3b2..8a480c7 100644<br> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c<br> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c<br> >> @@ -1930,6 +1930,51 @@ static void amdgpu_vm_prt_fini(struct <br> >> amdgpu_device *adev, struct amdgpu_vm *vm)<br> >> }<br> >> /**<br> >> + * amdgpu_vm_remove_ptes - free PT BOs<br> >> + *<br> >> + * @adev: amdgpu device structure<br> >> + * @vm: amdgpu vm structure<br> >> + * @start: start of mapped range<br> >> + * @end: end of mapped entry<br> >> + *<br> >> + * Free the page table level.<br> >> + */<br> >> +static int amdgpu_vm_remove_ptes(struct amdgpu_device *adev,<br> >> + struct amdgpu_vm *vm, uint64_t start, uint64_t end)<br> >> +{<br> >> + struct amdgpu_vm_pt_cursor cursor;<br> >> + unsigned shift, num_entries;<br> >> +<br> >> + amdgpu_vm_pt_start(adev, vm, start, &cursor);<br> >> + while (cursor.level < AMDGPU_VM_PTB) {<br> >> + if (!amdgpu_vm_pt_descendant(adev, &cursor))<br> >> + return -ENOENT;<br> >> + }<br> >> +<br> >> + while (cursor.pfn < end) {<br> >> + amdgpu_vm_free_table(cursor.entry);<br> >> + num_entries = amdgpu_vm_num_entries(adev, cursor.level - 1);<br> >> +<br> >> + if (cursor.entry != &cursor.parent->entries[num_entries - 1]) {<br> >> + /* Next ptb entry */<br> >> + shift = amdgpu_vm_level_shift(adev, cursor.level - 1);<br> >> + cursor.pfn += 1ULL << shift;<br> >> + cursor.pfn &= ~((1ULL << shift) - 1);<br> >> + cursor.entry++;<br> >> + } else {<br> >> + /* Next ptb entry in next pd0 entry */<br> >> + amdgpu_vm_pt_ancestor(&cursor);<br> >> + shift = amdgpu_vm_level_shift(adev, cursor.level - 1);<br> >> + cursor.pfn += 1ULL << shift;<br> >> + cursor.pfn &= ~((1ULL << shift) - 1);<br> >> + amdgpu_vm_pt_descendant(adev, &cursor);<br> >> + }<br> >> + }<br> >> +<br> >> + return 0;<br> >> +}<br> >> +<br> >> +/**<br> >> * amdgpu_vm_clear_freed - clear freed BOs in the PT<br> >> *<br> >> * @adev: amdgpu_device pointer<br> >> @@ -1949,7 +1994,6 @@ int amdgpu_vm_clear_freed(struct amdgpu_device <br> >> *adev,<br> >> struct dma_fence **fence)<br> >> {<br> >> struct amdgpu_bo_va_mapping *mapping;<br> >> - uint64_t init_pte_value = 0;<br> >> struct dma_fence *f = NULL;<br> >> int r;<br> >> @@ -1958,13 +2002,10 @@ int amdgpu_vm_clear_freed(struct <br> >> amdgpu_device *adev,<br> >> struct amdgpu_bo_va_mapping, list);<br> >> list_del(&mapping->list);<br> >> - if (vm->pte_support_ats &&<br> >> - mapping->start < AMDGPU_GMC_HOLE_START)<br> >> - init_pte_value = AMDGPU_PTE_DEFAULT_ATC;<br> >> + r = amdgpu_vm_remove_ptes(adev, vm,<br> >> + (mapping->start + 0x1ff) & (~0x1ffll),<br> >> + (mapping->last + 1) & (~0x1ffll));<br> >> - r = amdgpu_vm_bo_update_mapping(adev, vm, false, NULL,<br> >> - mapping->start, mapping->last,<br> >> - init_pte_value, 0, NULL, &f);<br> >> amdgpu_vm_free_mapping(adev, vm, mapping, f);<br> >> if (r) {<br> >> dma_fence_put(f);<br> >> @@ -1980,7 +2021,6 @@ int amdgpu_vm_clear_freed(struct amdgpu_device <br> >> *adev,<br> >> }<br> >> return 0;<br> >> -<br> >> }<br> >> /**<br> ><br> > _______________________________________________<br> > amd-gfx mailing list<br> > <a class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org"> amd-gfx@lists.freedesktop.org</a><br> > <a href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx" moz-do-not-send="true"> https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a><br> </div> </span></font></div> </blockquote> </div> <br> </div> </div> </div> </blockquote> </body> </html>