Re: [PATCH v6 0/6] mm/vmalloc: Speed up ioremap, vmalloc and vmap with contiguous memory

[Uladzislau Rezki]

On Mon, Jul 13, 2026 at 08:43:14PM +0530, Dev Jain wrote:
> 
> 
> On 10/07/26 2:24 pm, Wen Jiang wrote:
> > On Fri, 10 Jul 2026 at 07:08, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> >>
> >> On Thu,  9 Jul 2026 15:38:17 +0800 Wen Jiang <jiangwenxiaomi@xxxxxxxxx> wrote:
> >>
> >>> This patchset accelerates ioremap, vmalloc, and vmap when the memory
> >>> is physically fully or partially contiguous.
> >>
> >> Thanks, I added this to mm.git's mm-new branch for wider testing.
> >>
> >> AI review asked some questions, and some of them are new since the v5
> >> series:
> >>         https://sashiko.dev/#/patchset/20260709073823.6643-1-jiangwen6@xxxxxxxxxx
> > 
> > Hi Andrew,
> > 
> > I've gone through the Sashiko findings:
> > 
> > - Patch 1 (find_num_contig): Over-interpretation. No new hugetlbfs hstate
> >   is added. The extra sizes are only used by init_mm kernel mappings via.
> > 
> > - Patch 5/6 (NULL page): Invalid input. vmap() expects a fully populated
> >     array of valid struct page pointers.
> 
> Correct, but vmap_pages_pte_range has !page and !pfn_valid checks.
> 
> I really hate those checks - if those checks have any remote possibility of
> firing, then we already have a bug at
> 
> vm_map_ram -> vmap_pages_range -> vmap_pages_range_noflush -> kmsan_vmap_pages_range_noflush
> 
> because the last function dereferences the struct page pointers.
> 
> It is painful to do the page array sanity check deep into vmap - it implies
> we simply cannot play with the page array before that.
> 
> But since vmap is an exported function, doing a sanity check for the page array
> in the vmap code makes sense.
> 
> So how about the following:
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index afaa14ebf17bb..0c44bb7a45b5d 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -566,14 +566,6 @@ static int vmap_pages_pte_range(pmd_t *pmd, unsigned long addr,
>  			err = -EBUSY;
>  			break;
>  		}
> -		if (WARN_ON(!page)) {
> -			err = -ENOMEM;
> -			break;
> -		}
> -		if (WARN_ON(!pfn_valid(page_to_pfn(page)))) {
> -			err = -EINVAL;
> -			break;
> -		}
> 
>  		pfn = page_to_pfn(page);
>  		size = vmap_set_ptes(pte, addr, end, pfn, prot, shift);
> @@ -603,11 +595,6 @@ static int vmap_pages_pmd_range(pud_t *pud, unsigned long addr,
>  			struct page *page = pages[*nr];
>  			phys_addr_t phys_addr;
> 
> -			if (WARN_ON(!page))
> -				return -ENOMEM;
> -			if (WARN_ON(!pfn_valid(page_to_pfn(page))))
> -				return -EINVAL;
> -
>  			phys_addr = page_to_phys(page);
> 
>  			if (vmap_try_huge_pmd(pmd, addr, next, phys_addr, prot,
> @@ -3663,6 +3650,19 @@ static struct vm_struct *vmap_get_aligned_vm_area(unsigned long size,
>  	return __get_vm_area_node_aligned_caller(size, PAGE_SIZE, flags, caller);
>  }
> 
> +static inline bool vmap_page_sanity_checks(struct page **pages, unsigned int count)
> +{
> +	for (int i = 0; i < count; ++i) {
> +		if (WARN_ON(!pages[i]))
> +			return true;
> +
> +		if (WARN_ON(!pfn_valid(page_to_pfn(pages[i]))))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>  /**
>   * vmap - map an array of pages into virtually contiguous space
>   * @pages: array of page pointers
> @@ -3706,6 +3706,9 @@ void *vmap(struct page **pages, unsigned int count,
>  	if (!area)
>  		return NULL;
> 
> +	if (unlikely(vmap_page_sanity_checks(pages, count)))
> +		return NULL;
> +
>  	addr = (unsigned long)area->addr;
>  	if (vmap_pages_range_batched(addr, addr + size, pgprot_nx(prot),
>  				pages) < 0) {
> 
> 
> 
> Reasoning for calling vmap_page_sanity_checks() before vmap_pages_range_batched,
> and not at the start of vmap: I am worried that since vmap() is already very
> fast, we may cause a regression:
> 
> vmap() -> scan page array with linear map pointers -> vmap_get_aligned_vm_area (does
> memory allocation, throwing out the linear map VAs from cache and TLB) -> walk
> the pgtables and again access cold page array.
> 
> Perhaps I am being very pedantic here. What do you think?
> 
Sanity check adds extra CPU cycles and it adds overhead. The concern about cache
to be cold on second iteration looks valid. You can get some perf figures to see
the cost.

I would just keep the original approach. But no strong opinion here.

--
Uladzislau Rezki