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

[Kalesh Singh <kaleshsingh@google.com>]

On Thu, May 20, 2021 at 4:01 PM Peter Xu <peterx@redhat.com> 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@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..
>
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
>