On Thu, May 20, 2021 at 4:01 PM Peter Xu <peterx@xxxxxxxxxx> wrote: > > 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.. > It may have coincidentally handled the huge PUD case, but I hadn't considered huge PUDs when implementing the HAVE_MOVE_PUD patchset. Or as Zi suggested, huge PUD mremap may be unused atm, I haven't seen any related breakages since enabling HAVE_MOVE_PUD for x86 and arm64 > > 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 >
