Re: [PATCH v8 6/7] mm, folio_zero_user: support clearing page ranges

["David Hildenbrand (Red Hat)" <david@kernel.org>]

On 27.10.25 21:21, Ankur Arora wrote:
> Clear contiguous page ranges in folio_zero_user() instead of clearing
> a page-at-a-time. This enables CPU specific optimizations based on
> the length of the region.
> 
> Operating on arbitrarily large regions can lead to high preemption
> latency under cooperative preemption models. So, limit the worst
> case preemption latency via architecture specified PAGE_CONTIG_NR
> units.
> 
> The resultant performance depends on the kinds of optimizations
> available to the CPU for the region being cleared. Two classes of
> of optimizations:
> 
>    - clearing iteration costs can be amortized over a range larger
>      than a single page.
>    - cacheline allocation elision (seen on AMD Zen models).
> 
> Testing a demand fault workload shows an improved baseline from the
> first optimization and a larger improvement when the region being
> cleared is large enough for the second optimization.
> 
> AMD Milan (EPYC 7J13, boost=0, region=64GB on the local NUMA node):
> 
>   $ perf bench mem map -p $pg-sz -f demand -s 64GB -l 5
> 
>                      page-at-a-time     contiguous clearing      change
> 
>                    (GB/s  +- %stdev)     (GB/s  +- %stdev)
> 
>     pg-sz=2MB       12.92  +- 2.55%        17.03  +-  0.70%       + 31.8%	preempt=*
> 
>     pg-sz=1GB       17.14  +- 2.27%        18.04  +-  1.05% [#]   +  5.2%	preempt=none|voluntary
>     pg-sz=1GB       17.26  +- 1.24%        42.17  +-  4.21%       +144.3%	preempt=full|lazy
> 
> [#] AMD Milan uses a threshold of LLC-size (~32MB) for eliding cacheline
> allocation, which is larger than ARCH_PAGE_CONTIG_NR, so
> preempt=none|voluntary see no improvement on the pg-sz=1GB.
> 
> Also as mentioned earlier, the baseline improvement is not specific to
> AMD Zen platforms. Intel Icelakex (pg-sz=2MB|1GB) sees a similar
> improvement as the Milan pg-sz=2MB workload above (~30%).
> 
> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
> Reviewed-by: Raghavendra K T <raghavendra.kt@amd.com>
> Tested-by: Raghavendra K T <raghavendra.kt@amd.com>
> ---
>   include/linux/mm.h |  6 ++++++
>   mm/memory.c        | 42 +++++++++++++++++++++---------------------
>   2 files changed, 27 insertions(+), 21 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index ecbcb76df9de..02db84667f97 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3872,6 +3872,12 @@ static inline void clear_page_guard(struct zone *zone, struct page *page,
>   				unsigned int order) {}
>   #endif	/* CONFIG_DEBUG_PAGEALLOC */
>   
> +#ifndef ARCH_PAGE_CONTIG_NR
> +#define PAGE_CONTIG_NR	1
> +#else
> +#define PAGE_CONTIG_NR	ARCH_PAGE_CONTIG_NR
> +#endif

The name is a bit misleading. We need something that tells us that this 
is for patch-processing (clearing? maybe alter copying?) contig pages. 
Likely spelling out that this is for the non-preemptible case only.

I assume we can drop the "CONTIG", just like clear_pages() doesn't 
contain it etc.

CLEAR_PAGES_NON_PREEMPT_BATCH

PROCESS_PAGES_NON_PREEMPT_BATCH

Can you remind me again why this is arch specific, and why the default 
is 1 instead of, say 2,4,8 ... ?

> +
>   #ifndef __HAVE_ARCH_CLEAR_PAGES
>   /**
>    * clear_pages() - clear a page range for kernel-internal use.
> diff --git a/mm/memory.c b/mm/memory.c
> index 74b45e258323..7781b2aa18a8 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -7144,40 +7144,40 @@ static inline int process_huge_page(
>   	return 0;
>   }
>   
> -static void clear_gigantic_page(struct folio *folio, unsigned long addr_hint,
> -				unsigned int nr_pages)
> +/*
> + * Clear contiguous pages chunking them up when running under
> + * non-preemptible models.
> + */
> +static void clear_contig_highpages(struct page *page, unsigned long addr,
> +				   unsigned int npages)
>   {
> -	unsigned long addr = ALIGN_DOWN(addr_hint, folio_size(folio));
> -	int i;
> +	unsigned int i, count, unit;
>   
> -	might_sleep();
> -	for (i = 0; i < nr_pages; i++) {
> +	unit = preempt_model_preemptible() ? npages : PAGE_CONTIG_NR;
> +
> +	for (i = 0; i < npages; ) {
> +		count = min(unit, npages - i);
> +		clear_user_highpages(page + i,
> +				     addr + i * PAGE_SIZE, count);
> +		i += count;

Why not

for (i = 0; i < nr_pages; i += count) {

Also, I would leave the cond_resched(); where it was (before the 
invocation) to perform as little change as possible.

>   		cond_resched();
> -		clear_user_highpage(folio_page(folio, i), addr + i * PAGE_SIZE);
>   	}
>   }
>   
> -static int clear_subpage(unsigned long addr, int idx, void *arg)
> -{
> -	struct folio *folio = arg;
> -
> -	clear_user_highpage(folio_page(folio, idx), addr);
> -	return 0;
> -}
> -
>   /**
>    * folio_zero_user - Zero a folio which will be mapped to userspace.
>    * @folio: The folio to zero.
> - * @addr_hint: The address will be accessed or the base address if uncelar.
> + * @addr_hint: The address accessed by the user or the base address.
> + *
> + * Uses architectural support for clear_pages() to zero page extents
> + * instead of clearing page-at-a-time.
>    */
>   void folio_zero_user(struct folio *folio, unsigned long addr_hint)
>   {
> -	unsigned int nr_pages = folio_nr_pages(folio);
> +	unsigned long base_addr = ALIGN_DOWN(addr_hint, folio_size(folio));
>   
> -	if (unlikely(nr_pages > MAX_ORDER_NR_PAGES))
> -		clear_gigantic_page(folio, addr_hint, nr_pages);
> -	else
> -		process_huge_page(addr_hint, nr_pages, clear_subpage, folio);
> +	clear_contig_highpages(folio_page(folio, 0),
> +				base_addr, folio_nr_pages(folio));
>   }
>   
>   static int copy_user_gigantic_page(struct folio *dst, struct folio *src,


-- 
Cheers

David