Re: [PATCH mm-unstable v3 1/5] mm: consolidate anonymous folio PTE mapping into helpers

Nico Pache
2026-03-18T16:49:10Z

On 3/16/26 12:17 PM, Lorenzo Stoakes (Oracle) wrote:
> On Wed, Mar 11, 2026 at 03:13:11PM -0600, Nico Pache wrote:
>> The anonymous page fault handler in do_anonymous_page() open-codes the
>> sequence to map a newly allocated anonymous folio at the PTE level:
>> - construct the PTE entry
>> - add rmap
>> - add to LRU
>> - set the PTEs
>> - update the MMU cache.
>
> Yikes yeah this all needs work. Thanks for looking at this!

np! I believe it looks much cleaner now, and it has the added benefit of
cleaning up some of the mTHP patchset.

I also believe you suggested this so I'll add your SB when I add your RB tag :)

>
>>
>> Introduce a two helpers to consolidate this duplicated logic, mirroring the
>
> NIT: 'Introduce a two helpers' -> "introduce two helpers'

ack!

>
>> existing map_anon_folio_pmd_nopf() pattern for PMD-level mappings:
>>
>> map_anon_folio_pte_nopf(): constructs the PTE entry, takes folio
>> references, adds anon rmap and LRU. This function also handles the
>> uffd_wp that can occur in the pf variant.
>>
>> map_anon_folio_pte_pf(): extends the nopf variant to handle MM_ANONPAGES
>> counter updates, and mTHP fault allocation statistics for the page fault
>> path.
>
> MEGA nit, not sure why you're not just putting this in a bullet list, just
> weird to see not-code indented here :)

Ill drop the indentation!

>
>>
>> The zero-page read path in do_anonymous_page() is also untangled from the
>> shared setpte label, since it does not allocate a folio and should not
>> share the same mapping sequence as the write path. Make nr_pages = 1
>> rather than relying on the variable. This makes it more clear that we
>> are operating on the zero page only.
>>
>> This refactoring will also help reduce code duplication between mm/memory.c
>> and mm/khugepaged.c, and provides a clean API for PTE-level anonymous folio
>> mapping that can be reused by future callers.
>
> Maybe worth mentioning subseqent patches that will use what you set up
> here?

ok ill add something like "... that will be used when adding mTHP support to
khugepaged, and may find future reuse by other callers." Speaking of which I
tried to leverage this elsewhere and I believe that will take extra focus and
time, but may be doable.

>
> Also you split things out into _nopf() and _pf() variants, it might be
> worth saying exactly why you're doing that or what you are preparing to do?

ok ill expand on this in the "bullet list" you reference above.

>
>>
>> Reviewed-by: Dev Jain <dev.jain@arm.com>
>> Reviewed-by: Lance Yang <lance.yang@linux.dev>
>> Acked-by: David Hildenbrand (Arm) <david@kernel.org>
>> Signed-off-by: Nico Pache <npache@redhat.com>
>
> There's nits above and below, but overall the logic looks good, so with
> nits addressed/reasonably responded to:
>
> Reviewed-by: Lorenzo Stoakes (Oracle) <ljs@kernel.org>

Thanks :) Ill take care of those

>
>> ---
>> include/linux/mm.h | 4 ++++
>> mm/memory.c | 60 +++++++++++++++++++++++++++++++---------------
>> 2 files changed, 45 insertions(+), 19 deletions(-)
>>
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index 4c4fd55fc823..9fea354bd17f 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -4903,4 +4903,8 @@ static inline bool snapshot_page_is_faithful(const struct page_snapshot *ps)
>>
>> void snapshot_page(struct page_snapshot *ps, const struct page *page);
>>
>> +void map_anon_folio_pte_nopf(struct folio *folio, pte_t *pte,
>> + struct vm_area_struct *vma, unsigned long addr,
>> + bool uffd_wp);
>
> How I hate how uffd infiltrates all our code like this.
>
> Not your fault :)
>
>> +
>> #endif /* _LINUX_MM_H */
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 6aa0ea4af1fc..5c8bf1eb55f5 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -5197,6 +5197,37 @@ static struct folio *alloc_anon_folio(struct vm_fault *vmf)
>> return folio_prealloc(vma->vm_mm, vma, vmf->address, true);
>> }
>>
>> +void map_anon_folio_pte_nopf(struct folio *folio, pte_t *pte,
>> + struct vm_area_struct *vma, unsigned long addr,
>> + bool uffd_wp)
>> +{
>> + unsigned int nr_pages = folio_nr_pages(folio);
>
> const would be good
>
>> + pte_t entry = folio_mk_pte(folio, vma->vm_page_prot);
>> +
>> + entry = pte_sw_mkyoung(entry);
>> +
>> + if (vma->vm_flags & VM_WRITE)
>> + entry = pte_mkwrite(pte_mkdirty(entry), vma);
>> + if (uffd_wp)
>> + entry = pte_mkuffd_wp(entry);
>> +
>> + folio_ref_add(folio, nr_pages - 1);
>> + folio_add_new_anon_rmap(folio, vma, addr, RMAP_EXCLUSIVE);
>> + folio_add_lru_vma(folio, vma);
>> + set_ptes(vma->vm_mm, addr, pte, entry, nr_pages);
>> + update_mmu_cache_range(NULL, vma, addr, pte, nr_pages);
>> +}
>> +
>> +static void map_anon_folio_pte_pf(struct folio *folio, pte_t *pte,
>> + struct vm_area_struct *vma, unsigned long addr, bool uffd_wp)
>> +{
>> + unsigned int order = folio_order(folio);
>
> const would be good here also!

ack on the const's

>
>> +
>> + map_anon_folio_pte_nopf(folio, pte, vma, addr, uffd_wp);
>> + add_mm_counter(vma->vm_mm, MM_ANONPAGES, 1 << order);
>
> Is 1 << order strictly right here? This field is a long value, so 1L <<
> order maybe? I get nervous about these shifts...
>
> Note that folio_large_nr_pages() uses 1L << order so that does seem
> preferable.

Ok sounds good thanks!

>
>> + count_mthp_stat(order, MTHP_STAT_ANON_FAULT_ALLOC);
>> +}
>> +
>> /*
>> * We enter with non-exclusive mmap_lock (to exclude vma changes,
>> * but allow concurrent faults), and pte mapped but not yet locked.
>> @@ -5243,7 +5274,14 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
>> pte_unmap_unlock(vmf->pte, vmf->ptl);
>> return handle_userfault(vmf, VM_UFFD_MISSING);
>> }
>> - goto setpte;
>> + if (vmf_orig_pte_uffd_wp(vmf))
>> + entry = pte_mkuffd_wp(entry);
>> + set_pte_at(vma->vm_mm, addr, vmf->pte, entry);
>
> How I _despise_ how uffd is implemented in mm. Feels like open coded
> nonsense spills out everywhere.
>
> Not your fault obviously :)
>
>> +
>> + /* No need to invalidate - it was non-present before */
>> + update_mmu_cache_range(vmf, vma, addr, vmf->pte,
>> + /*nr_pages=*/ 1);
>
> Is there any point in passing vmf here given you pass NULL above, and it
> appears that nobody actually uses this? I guess it doesn't matter but
> seeing this immediately made we question why you set it in one, and not the
> other?
>
> Maybe I'm mistaken and some arch uses it? Don't think so though.
>
> Also can't we then just use update_mmu_cache() which is the single-page
> wrapper of this AFAICT? That'd make it even simpler.

>
> Having done this, is there any reason to keep the annoying and confusing
> initial assignment of nr_pages = 1 at declaration time?
>
> It seems that nr_pages is unconditionally assigned before it's used
> anywhere now at line 5298:
>
> nr_pages = folio_nr_pages(folio);
> addr = ALIGN_DOWN(vmf->address, nr_pages * PAGE_SIZE);
> ...
>
> It's kinda weird to use nr_pages again after you go out of your way to
> avoid it using folio_nr_pages() in map_anon_folio_pte_nopf() and
> folio_order() in map_anon_folio_pte_pf().

Yes I believe so, thank you that is cleaner! In the past I had nr_pages as a
variable but David suggested being explicit with the "1" so it would be obvious
its a single page... update_mmu_cache() should indicate the same thing.

>
> But yeah, ok we align the address and it's yucky maybe leave for now (but
> we can definitely stop defaulting nr_pages to 1 :)
>
>> + goto unlock;
>> }
>>
>> /* Allocate our own private page. */
>> @@ -5267,11 +5305,6 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
>> */
>> __folio_mark_uptodate(folio);
>>
>> - entry = folio_mk_pte(folio, vma->vm_page_prot);
>> - entry = pte_sw_mkyoung(entry);
>> - if (vma->vm_flags & VM_WRITE)
>> - entry = pte_mkwrite(pte_mkdirty(entry), vma);
>> -
>> vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, addr, &vmf->ptl);
>> if (!vmf->pte)
>> goto release;
>> @@ -5293,19 +5326,8 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
>> folio_put(folio);
>> return handle_userfault(vmf, VM_UFFD_MISSING);
>> }
>> -
>> - folio_ref_add(folio, nr_pages - 1);
>> - add_mm_counter(vma->vm_mm, MM_ANONPAGES, nr_pages);
>> - count_mthp_stat(folio_order(folio), MTHP_STAT_ANON_FAULT_ALLOC);
>> - folio_add_new_anon_rmap(folio, vma, addr, RMAP_EXCLUSIVE);
>> - folio_add_lru_vma(folio, vma);
>> -setpte:
>> - if (vmf_orig_pte_uffd_wp(vmf))
>> - entry = pte_mkuffd_wp(entry);
>> - set_ptes(vma->vm_mm, addr, vmf->pte, entry, nr_pages);
>> -
>> - /* No need to invalidate - it was non-present before */
>> - update_mmu_cache_range(vmf, vma, addr, vmf->pte, nr_pages);
>> + map_anon_folio_pte_pf(folio, vmf->pte, vma, addr,
>> + vmf_orig_pte_uffd_wp(vmf));
>
> So we're going from:
>
> entry = folio_mk_pte(...)
> entry = pte_sw_mkyoung(...)
> if (write)
> entry = pte_mkwrite(also dirty...)
> folio_ref_add(nr_pages - 1)
> add_mm_counter(... MM_ANON_PAGES, nr_pages)
> count_mthp_stat(folio_order(folio), MTHP_STAT_ANON_FAULT_ALLOC)
> folio_add_new_anon_rmap(.., RMAP_EXCLUSIVE)
> folio_add_lru_vma(folio, vma)
> if (vmf_orig_pte_uffd_wp(vmf))
> entry = pte_mkuffd_wp(entry)
> set_ptes(mm, addr, vmf->pte, entry, nr_pages)
> update_mmu_cache_range(vmf, vma, addr, vmf->pte, nr_pages)
>
> To:
>
> <map_anon_folio_pte_nopf>
> entry = folio_mk_pte(...)
> entry = pte_sw_mkyoung(...)
> if (write)
> entry = pte_mkwrite(also dirty...)
> if (vmf_orig_pte_uffd_wp(vmf) <passed in via uffd_wp>) <-- reordered
> entry = pte_mkuffd_wp(entry)
> folio_ref_add(nr_pages - 1)
> folio_add_new_anon_rmap(.., RMAP_EXCLUSIVE)
> folio_add_lru_vma(folio, vma)
> set_ptes(mm, addr, pte, entry, nr_pages)
> update_mmu_cache_range(NULL, vma, addr, pte, nr_pages)
>
> <map_anon_folio_pte_pf>
> add_mm_counter(... MM_ANON_PAGES, nr_pages) <-- reordered
> count_mthp_stat(folio_order(folio), MTHP_STAT_ANON_FAULT_ALLOC) <-- reodrdered
>
> But the reorderings seem fine, and it is achieving the same thing.
>
> All the parameters being passed seem correct too.

Thanks for the review and verifying the logic :) I was particularly scared of
changing the page fault handler, so im glad multiple people have confirmed this
seems fine.

Cheers,
-- Nico

>
>> unlock:
>> if (vmf->pte)
>> pte_unmap_unlock(vmf->pte, vmf->ptl);
>> --
>> 2.53.0
>>
>
> Cheers, Lorenzo
>