Re: [PATCH 07/19] mm/fork: Accept huge pfnmap entries

David Hildenbrand
2024-08-12T18:50:23Z

On 12.08.24 20:29, Peter Xu wrote:
> On Fri, Aug 09, 2024 at 07:59:58PM +0200, David Hildenbrand wrote:
>> On 09.08.24 19:15, Peter Xu wrote:
>>> On Fri, Aug 09, 2024 at 06:32:44PM +0200, David Hildenbrand wrote:
>>>> On 09.08.24 18:08, Peter Xu wrote:
>>>>> Teach the fork code to properly copy pfnmaps for pmd/pud levels. Pud is
>>>>> much easier, the write bit needs to be persisted though for writable and
>>>>> shared pud mappings like PFNMAP ones, otherwise a follow up write in either
>>>>> parent or child process will trigger a write fault.
>>>>>
>>>>> Do the same for pmd level.
>>>>>
>>>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>>>> ---
>>>>> mm/huge_memory.c | 27 ++++++++++++++++++++++++---
>>>>> 1 file changed, 24 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>>>> index 6568586b21ab..015c9468eed5 100644
>>>>> --- a/mm/huge_memory.c
>>>>> +++ b/mm/huge_memory.c
>>>>> @@ -1375,6 +1375,22 @@ int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
>>>>> pgtable_t pgtable = NULL;
>>>>> int ret = -ENOMEM;
>>>>> + pmd = pmdp_get_lockless(src_pmd);
>>>>> + if (unlikely(pmd_special(pmd))) {
>>>>> + dst_ptl = pmd_lock(dst_mm, dst_pmd);
>>>>> + src_ptl = pmd_lockptr(src_mm, src_pmd);
>>>>> + spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
>>>>> + /*
>>>>> + * No need to recheck the pmd, it can't change with write
>>>>> + * mmap lock held here.
>>>>> + */
>>>>> + if (is_cow_mapping(src_vma->vm_flags) && pmd_write(pmd)) {
>>>>> + pmdp_set_wrprotect(src_mm, addr, src_pmd);
>>>>> + pmd = pmd_wrprotect(pmd);
>>>>> + }
>>>>> + goto set_pmd;
>>>>> + }
>>>>> +
>>>>
>>>> I strongly assume we should be using using vm_normal_page_pmd() instead of
>>>> pmd_page() further below. pmd_special() should be mostly limited to GUP-fast
>>>> and vm_normal_page_pmd().
>>>
>>> One thing to mention that it has this:
>>>
>>> if (!vma_is_anonymous(dst_vma))
>>> return 0;
>>
>> Another obscure thing in this function. It's not the job of copy_huge_pmd()
>> to make the decision whether to copy, it's the job of vma_needs_copy() in
>> copy_page_range().
>>
>> And now I have to suspect that uffd-wp is broken with this function, because
>> as vma_needs_copy() clearly states, we must copy, and we don't do that for
>> PMDs. Ugh.
>>
>> What a mess, we should just do what we do for PTEs and we will be fine ;)
>
> IIUC it's not a problem: file uffd-wp is different from anonymous, in that
> it pushes everything down to ptes.
>
> It means if we skipped one huge pmd here for file, then it's destined to
> have nothing to do with uffd-wp, otherwise it should have already been
> split at the first attempt to wr-protect.

Is that also true for UFFD_FEATURE_WP_ASYNC, when we call
pagemap_scan_thp_entry()->make_uffd_wp_pmd() ?

I'm not immediately finding the code that does the "pushes everything
down to ptes", so I might miss that part.

>
>>
>> Also, we call copy_huge_pmd() only if "is_swap_pmd(*src_pmd) ||
>> pmd_trans_huge(*src_pmd) || pmd_devmap(*src_pmd)"
>>
>> Would that even be the case with PFNMAP? I suspect that pmd_trans_huge()
>> would return "true" for special pfnmap, which is rather "surprising", but
>> fortunate for us.
>
> It's definitely not surprising to me as that's the plan.. and I thought it
> shoulidn't be surprising to you - if you remember before I sent this one, I
> tried to decouple that here with the "thp agnostic" series:
>
> https://lore.kernel.org/r/20240717220219.3743374-1-peterx@redhat.com
>
> in which you reviewed it (which I appreciated).
>
> So yes, pfnmap on pmd so far will report pmd_trans_huge==true.

I review way to much stuff to remember everything :) That certainly
screams for a cleanup ...

>
>>
>> Likely we should be calling copy_huge_pmd() if pmd_leaf() ... cleanup for
>> another day.
>
> Yes, ultimately it should really be a pmd_leaf(), but since I didn't get
> much feedback there, and that can further postpone this series from being
> posted I'm afraid, then I decided to just move on with "taking pfnmap as
> THPs". The corresponding change on this path is here in that series:
>
> https://lore.kernel.org/all/20240717220219.3743374-7-peterx@redhat.com/
>
> @@ -1235,8 +1235,7 @@ copy_pmd_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
> src_pmd = pmd_offset(src_pud, addr);
> do {
> next = pmd_addr_end(addr, end);
> - if (is_swap_pmd(*src_pmd) || pmd_trans_huge(*src_pmd)
> - || pmd_devmap(*src_pmd)) {
> + if (is_swap_pmd(*src_pmd) || pmd_is_leaf(*src_pmd)) {
> int err;
> VM_BUG_ON_VMA(next-addr != HPAGE_PMD_SIZE, src_vma);
> err = copy_huge_pmd(dst_mm, src_mm, dst_pmd, src_pmd,
>

Ah, good.

[...]

>> Yes, as stated above, likely broken with UFFD-WP ...
>>
>> I really think we should make this code just behave like it would with PTEs,
>> instead of throwing in more "different" handling.
>
> So it could simply because file / anon uffd-wp work very differently.

Or because nobody wants to clean up that code ;)

--
Cheers,

David / dhildenb