On Thu, May 20, 2021 at 03:06:30PM -0400, Zi Yan wrote:

On 20 May 2021, at 10:57, Peter Xu wrote:

quoted

On Thu, May 20, 2021 at 07:07:57PM +0530, Aneesh Kumar K.V wrote:

quoted

"Aneesh Kumar K.V" [off-list ref] writes:

quoted

On 5/20/21 6:16 PM, Peter Xu wrote:

quoted

On Thu, May 20, 2021 at 01:56:54PM +0530, Aneesh Kumar K.V wrote:

quoted

quoted

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