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

[David Hildenbrand <david@redhat.com>]

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