Re: [PATCH v1 14/16] mm/vmalloc: Batch arch_sync_kernel_mappings() more efficiently

[Anshuman Khandual <anshuman.khandual@arm.com>]

On 2/5/25 20:39, Ryan Roberts wrote:
> When page_shift is greater than PAGE_SIZE, __vmap_pages_range_noflush()
> will call vmap_range_noflush() for each individual huge page. But
> vmap_range_noflush() would previously call arch_sync_kernel_mappings()
> directly so this would end up being called for every huge page.
> 
> We can do better than this; refactor the call into the outer
> __vmap_pages_range_noflush() so that it is only called once for the
> entire batch operation.

This makes sense.

> 
> This will benefit performance for arm64 which is about to opt-in to
> using the hook.
> 
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> ---
>  mm/vmalloc.c | 60 ++++++++++++++++++++++++++--------------------------
>  1 file changed, 30 insertions(+), 30 deletions(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 68950b1824d0..50fd44439875 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -285,40 +285,38 @@ static int vmap_p4d_range(pgd_t *pgd, unsigned long addr, unsigned long end,
>  
>  static int vmap_range_noflush(unsigned long addr, unsigned long end,
>  			phys_addr_t phys_addr, pgprot_t prot,
> -			unsigned int max_page_shift)
> +			unsigned int max_page_shift, pgtbl_mod_mask *mask)
>  {
>  	pgd_t *pgd;
> -	unsigned long start;
>  	unsigned long next;
>  	int err;
> -	pgtbl_mod_mask mask = 0;
>  
>  	might_sleep();
>  	BUG_ON(addr >= end);
>  
> -	start = addr;
>  	pgd = pgd_offset_k(addr);
>  	do {
>  		next = pgd_addr_end(addr, end);
>  		err = vmap_p4d_range(pgd, addr, next, phys_addr, prot,
> -					max_page_shift, &mask);
> +					max_page_shift, mask);
>  		if (err)
>  			break;
>  	} while (pgd++, phys_addr += (next - addr), addr = next, addr != end);
>  
> -	if (mask & ARCH_PAGE_TABLE_SYNC_MASK)
> -		arch_sync_kernel_mappings(start, end);
> -
>  	return err;
>  }

arch_sync_kernel_mappings() gets dropped here and moved to existing
vmap_range_noflush() callers instead.

>  
>  int vmap_page_range(unsigned long addr, unsigned long end,
>  		    phys_addr_t phys_addr, pgprot_t prot)
>  {
> +	pgtbl_mod_mask mask = 0;
>  	int err;
>  
>  	err = vmap_range_noflush(addr, end, phys_addr, pgprot_nx(prot),
> -				 ioremap_max_page_shift);
> +				 ioremap_max_page_shift, &mask);
> +	if (mask & ARCH_PAGE_TABLE_SYNC_MASK)
> +		arch_sync_kernel_mappings(addr, end);
> +

arch_sync_kernel_mappings() gets moved here.

>  	flush_cache_vmap(addr, end);
>  	if (!err)
>  		err = kmsan_ioremap_page_range(addr, end, phys_addr, prot,
> @@ -587,29 +585,24 @@ static int vmap_pages_p4d_range(pgd_t *pgd, unsigned long addr,
>  }
>  
>  static int vmap_small_pages_range_noflush(unsigned long addr, unsigned long end,
> -		pgprot_t prot, struct page **pages)
> +		pgprot_t prot, struct page **pages, pgtbl_mod_mask *mask)
>  {
> -	unsigned long start = addr;
>  	pgd_t *pgd;
>  	unsigned long next;
>  	int err = 0;
>  	int nr = 0;
> -	pgtbl_mod_mask mask = 0;
>  
>  	BUG_ON(addr >= end);
>  	pgd = pgd_offset_k(addr);
>  	do {
>  		next = pgd_addr_end(addr, end);
>  		if (pgd_bad(*pgd))
> -			mask |= PGTBL_PGD_MODIFIED;
> -		err = vmap_pages_p4d_range(pgd, addr, next, prot, pages, &nr, &mask);
> +			*mask |= PGTBL_PGD_MODIFIED;
> +		err = vmap_pages_p4d_range(pgd, addr, next, prot, pages, &nr, mask);
>  		if (err)
>  			break;
>  	} while (pgd++, addr = next, addr != end);
>  
> -	if (mask & ARCH_PAGE_TABLE_SYNC_MASK)
> -		arch_sync_kernel_mappings(start, end);
> -
>  	return err;
>  }
>  
> @@ -626,26 +619,33 @@ int __vmap_pages_range_noflush(unsigned long addr, unsigned long end,
>  		pgprot_t prot, struct page **pages, unsigned int page_shift)
>  {
>  	unsigned int i, nr = (end - addr) >> PAGE_SHIFT;
> +	unsigned long start = addr;
> +	pgtbl_mod_mask mask = 0;
> +	int err = 0;
>  
>  	WARN_ON(page_shift < PAGE_SHIFT);
>  
>  	if (!IS_ENABLED(CONFIG_HAVE_ARCH_HUGE_VMALLOC) ||
> -			page_shift == PAGE_SHIFT)
> -		return vmap_small_pages_range_noflush(addr, end, prot, pages);
> -
> -	for (i = 0; i < nr; i += 1U << (page_shift - PAGE_SHIFT)) {
> -		int err;
> -
> -		err = vmap_range_noflush(addr, addr + (1UL << page_shift),
> -					page_to_phys(pages[i]), prot,
> -					page_shift);
> -		if (err)
> -			return err;
> +			page_shift == PAGE_SHIFT) {
> +		err = vmap_small_pages_range_noflush(addr, end, prot, pages,
> +						&mask);

Unlike earlier don't return here until arch_sync_kernel_mappings()
gets covered later.

> +	} else {
> +		for (i = 0; i < nr; i += 1U << (page_shift - PAGE_SHIFT)) {
> +			err = vmap_range_noflush(addr,
> +						addr + (1UL << page_shift),
> +						page_to_phys(pages[i]), prot,
> +						page_shift, &mask);
> +			if (err)
> +				break;
>  
> -		addr += 1UL << page_shift;
> +			addr += 1UL << page_shift;
> +		}
>  	}
>  
> -	return 0;
> +	if (mask & ARCH_PAGE_TABLE_SYNC_MASK)
> +		arch_sync_kernel_mappings(start, end);

arch_sync_kernel_mappings() gets moved here after getting dropped
from both vmap_range_noflush() and vmap_small_pages_range_noflush().

> +
> +	return err;
>  }
>  
>  int vmap_pages_range_noflush(unsigned long addr, unsigned long end,

LGTM, and this can stand on its own as well.

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>