Re: [PATCH v7 6/7] mm/vmalloc: map contiguous pages in batches for vmap() if possible

[Wen Jiang]

On Mon, 20 Jul 2026 at 15:50, Dev Jain <dev.jain@xxxxxxx> wrote:
>
> [------]
>
> >> +
> >> +    nr_contig = num_pages_contiguous(&pages[idx], max_steps);
> >> +    if (nr_contig < 2)
> >> +            return 0;
> >> +
> >> +    order = ilog2(nr_contig);
> >> +    pfn = page_to_pfn(pages[idx]);
> >> +
> >> +    /* Limit order by pfn alignment */
> >> +    if (pfn > 0)
> >> +            order = min_t(int, order, __ffs(pfn));
> >
> > Wouldn't it make sense to determine that before you call num_pages_contiguous?
> > Because then, you can just scan to that maximum instead of the given nr_pages.
>
> Right!
>
>

Agreed. I will check the PFN alignment limit first before calling
num_pages_contiguous() to avoid unnecessary scanning.

> >> +    unsigned int prev_shift = 0, idx = 0;
> >> +    unsigned long map_addr = addr, batch_end = addr;
> >> +    int err;
> >> +
> >> +    err = kmsan_vmap_pages_range_noflush(addr, end, prot, pages,
> >> +                                         PAGE_SHIFT, GFP_KERNEL);
> >
> > Is the indentation on the second parameter line off?
> >

Will check this, thanks.

> >> +    if (err)
> >> +            goto out;
> >> +
> >> +    for (unsigned int i = 0; i < count; ) {
> >> +            unsigned int shift = PAGE_SHIFT +
> >> +                    get_vmap_batch_order(pages, prot, count - i, i);
> >
> > Having two indices, i and idx, is just confusing.
> >

How about renaming idx to batch_start? I think using i and batch_start
will make the logic much clearer.

> > The whole function is a bit overly complicated. Does it really buy us much to
> > batch over vmap_pages_range_noflush_walk() calling with the same shift?
>
> It will buy us more in the sense that, vmap() speed is sensitive to the number
> of pagetable walks. I think that is reflected by the ioremap(1MB) 1.35x speedup
> mentioned in the cover letter.
>
> In terms of callers, not sure. I would expect a caller to get the highest
> power of 2 from the total needed nr_pages, then get the next highest power
> of 2 from the remaining, and so on.
>
> For the arm64 TRBE usecase, the relevant code is the rb_alloc_aux_page() caller in
> kernel/events/ring_buffer.c. We may get multiple same order requests if can't
> get the first requested higher order.
>
> Since on the cover letter I see Wen and team have tested on boards, I lean
> towards saying it is reasonable to assume a usecase where such order
> fallback can happen.
>

Thanks. The batching is necessary because it significantly reduces
page table walks and effectively handles fallback scenarios (such as
the arm64 TRBE use case).

> But yeah the code for this currently is not so nice I agree.
>
> >
> >> +
> >> +            if (!i)
> >> +                    prev_shift = shift;
> >> +
> >> +            if (shift != prev_shift) {
> >> +                    err = vmap_pages_range_noflush_walk(map_addr, batch_end,
> >> +                                    prot, pages + idx, prev_shift);
> >> +                    if (err)
> >> +                            goto out;
> >> +                    prev_shift = shift;
> >> +                    map_addr = batch_end;
> >> +                    idx = i;
> >> +            }
> >> +
> >> +            /*
> >> +             * Once small pages are encountered, the remaining pages
> >> +             * are likely small as well.
> >
> > "small pages" is odd. Maybe
> >
> > "Once we fail to batch pages, we expect to fail batching for all remaining
> > pages, so just give up."
> >

Will update the comment.

> >> +             */
> >> +            if (shift == PAGE_SHIFT)
> >> +                    break;
> >> +
> >> +            batch_end += 1UL << shift;
> >> +            i += 1U << (shift - PAGE_SHIFT);
> >> +    }
> >> +
> >> +    /* Remaining */
> >> +    if (map_addr < end)
> >> +            err = vmap_pages_range_noflush_walk(map_addr, end,
> >> +                            prot, pages + idx, prev_shift);
> >> +
> >> +out:
> >> +    flush_cache_vmap(addr, end);
> >> +    return err;
> >> +}
> >> +
> >>  /**
> >>   * vmap - map an array of pages into virtually contiguous space
> >>   * @pages: array of page pointers
> >> @@ -3600,8 +3678,8 @@ void *vmap(struct page **pages, unsigned int count,
> >>              return NULL;
> >>
> >>      addr = (unsigned long)area->addr;
> >> -    if (vmap_pages_range(addr, addr + size, pgprot_nx(prot),
> >> -                            pages, PAGE_SHIFT) < 0) {
> >> +    if (vmap_pages_range_batched(addr, addr + size, pgprot_nx(prot),
> >> +                            pages) < 0) {
> >
> > Nit: single line would make that nicer to read.
> >
>

Once you confirm, I will send out a diff to address all the points
mentioned above. Thanks again for the review!

Best regards,
Wen