Re: [PATCH v5 3/9] mm/mremap: Use pmd/pud_poplulate to update page table entries

[Peter Xu <peterx@redhat.com>]

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@linux.ibm.com> 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