Re: [PATCH v4 2/4] mm/huge_memory: copy file THPs at fork like copy_present_ptes() does

[David Hildenbrand (Arm)]

On 10/21/26 22:36, Konstantin Khorenko wrote:
> This fixes no bug: it makes fork copy a PMD the way copy_present_ptes()
> copies a PTE. Until now copy_huge_pmd() skipped every folio PMD of a
> non-anonymous VMA and left the child to fault them in again, while the
> PTEs of the same VMA are copied.
> 
> Treat file folios as the PTE path does: take a reference, duplicate the
> rmap and account them. fork then pays a reference and an rmap update
> per PMD, and saves the child a fault per PMD it touches. The next patch
> sends PFN mappings through the same tail.
> 
> That covers the VMAs vma_needs_copy() sends here:
> 
> - private file mappings with an anon_vma;
> - VM_COPY_ON_FORK ones: uffd-wp and uffd-rwp, VM_MIXEDMAP (fuse DAX),
>   and VM_MAYBE_GUARD, which shared shmem, file and DAX mappings get
>   from a guard region.
> 
> The DAX huge zero PMD is copied too, with a reference taken by
> mm_get_huge_zero_folio() as for anonymous VMAs. A migration entry of a
> file folio is copied and accounted like copy_nonpresent_pte() does, and
> made read-only in both processes only in a CoW mapping.
> 
> The copy then gets the same tail as __copy_present_ptes(): write
> protect both sides in a CoW mapping, mark the child's copy clean in a
> shared one, and drop the uffd-wp bit where the child is not armed. An
> anonymous THP is now write protected only when its PMD is writable, as
> a PTE is.
> 
> A page table is deposited under the copy where the fault paths deposit
> one: in anonymous VMAs, and wherever the architecture needs one.
> 
> Suggested-by: David Hildenbrand <david@xxxxxxxxxx>
> Signed-off-by: Konstantin Khorenko <khorenko@xxxxxxxxxxxxx>
> ---
>  mm/huge_memory.c | 60 ++++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 37 insertions(+), 23 deletions(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 1e5d68acf62a5..13e3674fb6115 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1898,12 +1898,13 @@ static void copy_huge_non_present_pmd(
>  		pmd_t pmd, pgtable_t pgtable)
>  {
>  	softleaf_t entry = softleaf_from_pmd(pmd);
> -	struct folio *src_folio;
> +	struct folio *src_folio = softleaf_to_folio(entry);
>  
>  	VM_WARN_ON_ONCE(!pmd_is_valid_softleaf(pmd));
>  
> -	if (softleaf_is_migration_write(entry) ||
> -	    softleaf_is_migration_read_exclusive(entry)) {
> +	if ((softleaf_is_migration_write(entry) ||
> +	     softleaf_is_migration_read_exclusive(entry)) &&
> +	    vma_is_cow_mapping(src_vma)) {
>  		entry = make_readable_migration_entry(swp_offset(entry));
>  		pmd = softleaf_to_pmd(entry);
>  		if (pmd_swp_soft_dirty(*src_pmd))


Any reason we're not adding the vma_is_cow_mapping() check on the
softleaf_is_device_private_write() path as well?

(I think they are all anonymous ... just for consistency)


Is the fallthrough:

	add_mm_counter(dst_mm, MM_ANONPAGES, HPAGE_PMD_NR)

really expected?


> @@ -1927,7 +1928,6 @@ static void copy_huge_non_present_pmd(
>  			set_pmd_at(src_mm, addr, src_pmd, pmd);
>  		}
>  
> -		src_folio = softleaf_to_folio(entry);
>  		VM_WARN_ON(!folio_test_large(src_folio));
>  
>  		folio_get(src_folio);
> @@ -1939,9 +1939,11 @@ static void copy_huge_non_present_pmd(
>  					    dst_vma, src_vma);
>  	}
>  
> -	add_mm_counter(dst_mm, MM_ANONPAGES, HPAGE_PMD_NR);
> -	mm_inc_nr_ptes(dst_mm);
> -	pgtable_trans_huge_deposit(dst_mm, dst_pmd, pgtable);
> +	add_mm_counter(dst_mm, mm_counter(src_folio), HPAGE_PMD_NR);
> +	if (pgtable) {
> +		mm_inc_nr_ptes(dst_mm);
> +		pgtable_trans_huge_deposit(dst_mm, dst_pmd, pgtable);
> +	}
>  	if (!userfaultfd_protected(dst_vma))
>  		pmd = pmd_swp_clear_uffd(pmd);
>  	set_pmd_at(dst_mm, addr, dst_pmd, pmd);
> @@ -1957,6 +1959,7 @@ int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
>  	pmd_t pmd;
>  	pgtable_t pgtable = NULL;
>  	int ret = -ENOMEM;
> +	bool writable;

Why do we need this variable?

[...]

> +		goto copy_pmd;
>  	}

Likely we should clean this up later to avoid the gotos.



-- 
Cheers,

David