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

From: Zi Yan <ziy@nvidia.com>
To: Peter Xu <peterx@redhat.com>
Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>,
	Nathan Chancellor <nathan@kernel.org>,
	linux-mm@kvack.org, akpm@linux-foundation.org,
	mpe@ellerman.id.au, linuxppc-dev@lists.ozlabs.org,
	kaleshsingh@google.com, npiggin@gmail.com,
	joel@joelfernandes.org,
	Christophe Leroy <christophe.leroy@csgroup.eu>
Subject: Re: [PATCH v5 3/9] mm/mremap: Use pmd/pud_poplulate to update page table entries
Date: Thu, 20 May 2021 15:06:30 -0400	[thread overview]
Message-ID: <4CE7132C-3800-456B-91DA-613391361B94@nvidia.com> (raw)
In-Reply-To: <YKZ49Nrz2OojQUBR@t490s>

[-- Attachment #1: Type: text/plain, Size: 2393 bytes --]

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]. 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?

[1] https://lore.kernel.org/linux-ext4/148545012634.17912.13951763606410303827.stgit@djiang5-desk3.ch.intel.com/

—
Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]