On Thu, May 20, 2021 at 03:06:30PM -0400, Zi Yan wrote: > On 20 May 2021, at 10:57, Peter Xu wrote: > > > On Thu, May 20, 2021 at 07:07:57PM +0530, Aneesh Kumar K.V wrote: > >> "Aneesh Kumar K.V" <aneesh.kumar@xxxxxxxxxxxxx> writes: > >> > >>> On 5/20/21 6:16 PM, Peter Xu wrote: > >>>> On Thu, May 20, 2021 at 01:56:54PM +0530, Aneesh Kumar K.V wrote: > >>>>>> This seems to work at least for my userfaultfd test on shmem, however I don't > >>>>>> fully understand the commit message [1] on: How do we guarantee we're not > >>>>>> moving a thp pte? > >>>>>> > >>>>> > >>>>> move_page_tables() checks for pmd_trans_huge() and ends up calling > >>>>> move_huge_pmd if it is a THP entry. > >>>> > >>>> Sorry to be unclear: what if a huge pud thp? > >>>> > >>> > >>> I am still checking. Looking at the code before commit > >>> c49dd340180260c6239e453263a9a244da9a7c85, I don't see kernel handling > >>> huge pud thp. I haven't studied huge pud thp enough to understand > >>> whether c49dd340180260c6239e453263a9a244da9a7c85 intent to add that > >>> support. > >>> > >>> We can do a move_huge_pud() like we do for huge pmd thp. But I am not > >>> sure whether we handle those VMA's earlier and restrict mremap on them? > >> > >> something like this? (not even compile tested). I am still not sure > >> whether this is really needed or we handle DAX VMA's in some other form. > > > > Yeah maybe (you may want to at least drop that extra "case HPAGE_PUD"). > > > > It's just that if with CONFIG_HAVE_MOVE_PUD (x86 and arm64 enables it by > > default so far) it does seem to work even with huge pud, while after this patch > > it seems to be not working anymore, even with your follow up fix. > > > > Indeed I saw CONFIG_HAVE_MOVE_PUD is introduced a few months ago so breaking > > someone seems to be unlikely, perhaps no real user yet to mremap() a huge pud > > for dax or whatever backend? > > > > Ideally maybe rework this patch (or series?) and repost it for a better review? > > Agree the risk seems low. I'll leave that to you and Andrew to decide.. > > It seems that the mremap function for 1GB DAX THP was not added when 1GB DAX THP > was implemented[1]. Yes, but trickily as I mentioned it seems Android's CONFIG_HAVE_MOVE_PUD has done this right (with no intention I guess) with the set_pud_at() before this patch is merged, so we might have a short period that this might start to work.. > I guess no one is using mremap on 1GB DAX THP. Maybe we want > to at least add a warning or VM_BUG_ON to catch this or use Aneesh’s move_huge_pud() > to handle the situation properly? Agreed, if we decide to go with the patches, some warning (or even VM_BUG_ON, which iiuc should be very not-suggested in most cases) looks better than pgtable corruption reports. -- Peter Xu
