Re: [PATCH v5 07/38] mm: add mmap action for discontiguous kernel page mapping

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Mon, Oct 12, 2026 at 05:03:02PM +0200, David Hildenbrand (Arm) wrote:
> On 10/9/26 20:04, Lorenzo Stoakes (ARM) wrote:
> > The existing kernel page mapping mmap actions allow for partial and full
> > mapping of an array of struct page pointers.
> >
> > However some drivers require the mapping of discontiguous ranges. Permit
> > this by providing discontig_kernel_page_ops which allows a driver to
> > specify how the operation should begin and how batches of pages should be
> > retrieved.
> >
> > It uses the minimum exposed interface to do so, providing address, page
> > offset and both vm_private_data state and a local private state object.
> >
> > ops->init can establish state for the operation, and ops->get outputs the
> > pages to map and their count. Should an error arise the core unmaps the
> > VMA, invoking vm_ops->close, which is therefore where any state established
> > by ops->init is released.
> >
> > Also update the compatibility layer to do this too via
> > compat_mmap_action_abort().
> >
> > Batches may not exceed the VMA, but may map less than its full range in
> > case the driver wishes to allow the user to map an area larger than the
> > available data.
> >
> > To use it, users invoke mmap_action_map_discontig_kernel_pages() with
> > initial local private state and a set of operations.
> >
> > Users can then use one of the provided helper functions to perform an
> > action:
> >
> > * discontig_kernel_map_abort() - Abort and leave the mapping as it has
> >   been accumulated so far.
> > * discontig_kernel_map_page() - Map a single page, or a compound page given
> >   its head page.
> > * discontig_kernel_map_page_range() - Maps a struct page ** array of a
> >   specified count, mapping only those pages which fit in the VMA.
> >
> > While here, update vm_insert_page() and vm_insert_pages() to assert the
> > mmap write lock is held rather than the bizarre assert !mmap_read_trylock()
> > and use the new VMA API so all code paths are consistent.
> >
> > Also convert a BUG_ON() to a VM_WARN_ON_ONCE() in vm_insert_page().
> >
> > The userland VMA tests are updated accordingly.
> >
> > Acked-by: Zi Yan <ziy@nvidia.com>
> > Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
>
>
> Finally finding time to go over this. In the future I want to have more time to
> review such massive series, as it causes severe stress to be forced to review
> this last minute before it goes upstream.
>
> So if I push back in order to give me some more room to breathe in the future
> it's nothing personal, I just have to keep my sanity somehow.
>
> I hate being forced to review because we pick up stuff way to early. There is
> just way too much other stuff going on.
>
> I'm planning on picking up most stuff I maintain in the next cycle. For
> cross-component stuff (esp. VMA) I expect Andrew to pick stuff up until we
> tackle this once cycle later.

OK I understand, sorry for sending such a large series.

I already delayed it from last cycle and sent early in rc2, but I understand
there's been a lot of volume in this one.

I've been working evenings and weekends myself to keep up with review (and
failing), so I get how thankless it can be (LPC was also unhelpfully timed!)

I will try to split things out more in future, this had a lot of dependencies so
it wasn't clear that made sense + wouldn't just cause more stress over a longer
period otherwise.

>
>
> > +static inline
> > +void mmap_action_map_discontig_kernel_pages(struct vm_area_desc *desc,
> > +		void *init_private, const struct discontig_kernel_page_ops *ops)
> > +{
> > +	struct mmap_action *action = &desc->action;
> > +
> > +	action->type = MMAP_DISCONTIG_KERNEL_PAGES;
> > +	action->map_kernel_discontig.init_private = init_private;
> > +	action->map_kernel_discontig.ops = ops;
> > +}
> > +
> >  int mmap_action_prepare(struct vm_area_desc *desc);
> >  int mmap_action_complete(struct vm_area_struct *vma,
> >  			 struct mmap_action *action, bool is_compat);
>
> There is a mixture of two tab vs. different in your changes.

OK. mm is fairly inconsistent about this, I will convert all to 2 tabs.

>
> >
> > +static inline void
> > +discontig_kernel_map_abort(struct discontig_kernel_page_state *state)
> > +{
> > +	state->action = DISCONTIG_KERNEL_PAGE_ABORT;
> > +}
> > +
> > +static inline void
> > +discontig_kernel_map_page(struct discontig_kernel_page_state *state,
> > +			  struct page *page)
> > +{
> > +	struct folio *folio = page_folio(page);
> > +
> > +	if (folio_test_large(folio)) {
> > +		VM_WARN_ON_ONCE(page != folio_page(folio, 0));
> > +		state->action = DISCONTIG_KERNEL_PAGE_MAP_COMPOUND_PAGE;
> > +		state->__folio = folio;
> > +		state->__nr_pages = min(state->nr_pages_remain,
> > +					folio_nr_pages(folio));
> > +	} else {
> > +		state->action = DISCONTIG_KERNEL_PAGE_MAP_PAGE;
> > +		state->__page = page;
> > +		state->__nr_pages = 1;
> > +	}
>
> Why are both separate? Isn't it sufficient to represent that through _page +
> _nr_pages > 1 ? See below.

Ack yup that works.

>
> > +}
> > +
> > +static inline void
> > +discontig_kernel_map_page_range(struct discontig_kernel_page_state *state,
> > +				struct page **page_arr, unsigned long nr_pages)
>
> Is this actually "map_pages"? A range is rather a number of consectuive pages
> part of the same folio/compound page, no?
>
> (we call insert_pages() later ... )

Sure, can change.

>
> > +{
> > +	state->action = DISCONTIG_KERNEL_PAGE_MAP_PAGE_RANGE;
> > +	state->__page_arr = page_arr;
> > +	state->__nr_pages = min(nr_pages, state->nr_pages_remain);
>
> So we expect all of them to be non-compound?

Yup this matches existing insert_pages() behaviour.

>
>
> [...]
>
> > diff --git a/mm/internal.h b/mm/internal.h
> > index fc4e249e2343..557e629db8c5 100644
> > --- a/mm/internal.h
> > +++ b/mm/internal.h
> > @@ -1515,6 +1515,9 @@ int simple_ioremap_prepare(struct vm_area_desc *desc);
> >  int map_kernel_pages_prepare(struct vm_area_desc *desc);
> >  int map_kernel_pages_complete(struct vm_area_struct *vma,
> >  			      struct mmap_action *action);
> > +int map_discontig_kernel_pages_prepare(struct vm_area_desc *desc);
> > +int map_discontig_kernel_pages_complete(struct vm_area_struct *vma,
> > +					struct mmap_action *action);
> >
> >  static inline int io_remap_pfn_range_prepare(struct vm_area_desc *desc)
> >  {
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 448342883e9d..e5c1d1c35bc7 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -2599,27 +2599,34 @@ int vm_insert_pages(struct vm_area_struct *vma, unsigned long addr,
> >
> >  	if (!range_in_vma(vma, addr, end))
> >  		return -EFAULT;
> > -	if (!(vma->vm_flags & VM_MIXEDMAP)) {
> > -		VM_WARN_ON_ONCE(mmap_read_trylock(vma->vm_mm));
> > -		VM_WARN_ON_ONCE(vma->vm_flags & VM_PFNMAP);
> > -		vm_flags_set(vma, VM_MIXEDMAP);
> > +	if (!vma_test(vma, VMA_MIXEDMAP_BIT)) {
> > +		mmap_assert_write_locked(vma->vm_mm);
> > +		VM_WARN_ON_ONCE(vma_test(vma, VMA_PFNMAP_BIT));
>
> I would prefer to have these changes in a separate patch. Or is it strictly
> required to have this in this very patch?

That was because Suren raised the 'assert on mmap_read_trylock()' thing and I
wanted to get the other cases here to be consistent.

I will split it out into another patch.

>
>
> > +		vma_start_write(vma);
> > +		vma_set_flags(vma, VMA_MIXEDMAP_BIT);
> >  	}
> >  	/* Defer page refcount checking till we're about to map that page. */
> >  	return insert_pages(vma, addr, pages, num, vma->vm_page_prot);
> >  }
> >  EXPORT_SYMBOL(vm_insert_pages);
> >
> > +static void __map_kernel_pages_prepare(struct vm_area_desc *desc)
> > +{
> > +	if (vma_desc_test(desc, VMA_MIXEDMAP_BIT))
> > +		return;
> > +
> > +	mmap_assert_write_locked(desc->mm);
> > +	VM_WARN_ON_ONCE(vma_desc_test(desc, VMA_PFNMAP_BIT));
> > +	vma_desc_set_flags(desc, VMA_MIXEDMAP_BIT);
> > +}
> > +
> >  int map_kernel_pages_prepare(struct vm_area_desc *desc)
> >  {
> >  	const struct mmap_action *action = &desc->action;
> >  	const unsigned long addr = action->map_kernel.start;
> >  	unsigned long nr_pages, end;
> >
> > -	if (!vma_desc_test(desc, VMA_MIXEDMAP_BIT)) {
> > -		VM_WARN_ON_ONCE(mmap_read_trylock(desc->mm));
> > -		VM_WARN_ON_ONCE(vma_desc_test(desc, VMA_PFNMAP_BIT));
> > -		vma_desc_set_flags(desc, VMA_MIXEDMAP_BIT);
> > -	}
> > +	__map_kernel_pages_prepare(desc);
> >
> >  	nr_pages = action->map_kernel.nr_pages;
> >  	end = addr + PAGE_SIZE * nr_pages;
> > @@ -2640,6 +2647,98 @@ int map_kernel_pages_complete(struct vm_area_struct *vma,
> >  			    &nr_pages, vma->vm_page_prot);
> >  }
> >
> > +int map_discontig_kernel_pages_prepare(struct vm_area_desc *desc)
> > +{
> > +	const struct mmap_action *action = &desc->action;
> > +	const struct discontig_kernel_page_ops *ops =
> > +		action->map_kernel_discontig.ops;
> > +
> > +	/* At minimum need to be able to get pages. */
> > +	if (WARN_ON_ONCE(!ops || !ops->get))
> > +		return -EINVAL;
> > +
> > +	__map_kernel_pages_prepare(desc);
> > +	return 0;
> > +}
> > +
> > +static int apply_discontig_action(struct vm_area_struct *vma,
> > +				  struct discontig_kernel_page_state *state)
> > +{
> > +	unsigned long nr_pages = state->__nr_pages;
> > +	unsigned long addr = state->addr;
> > +	unsigned long i;
> > +
> > +	if (state->action == DISCONTIG_KERNEL_PAGE_MAP_PAGE)
> > +		return insert_page(vma, addr, state->__page,
> > +				   vma->vm_page_prot, /*mkwrite=*/false);
> > +	if (state->action == DISCONTIG_KERNEL_PAGE_MAP_PAGE_RANGE)
> > +		return insert_pages(vma, addr, state->__page_arr,
> > +				    &nr_pages, vma->vm_page_prot);
> > +
> > +	/* Compound folio - have to iterate through each page. */
> > +	for (i = 0; i < nr_pages; i++, addr += PAGE_SIZE) {
> > +		struct page *page = folio_page(state->__folio, i);
> > +		int err;
> > +
> > +		err = insert_page(vma, addr, page, vma->vm_page_prot,
> > +				  /*mkwrite=*/false);
> > +		if (err)
> > +			return err;
> > +	}
>
> Summarizing some of my comments below, I wonder if this could be:
>
> DISCONTIG_KERNEL_PAGE_MAP_PAGES - array of individual apges
> DISCONTIG_KERNEL_PAGE_MAP_CONTIG_PAGES - 1..N contiguous pages

OK.

>
>
> if (state->action == DISCONTIG_KERNEL_PAGE_MAP_PAGES)
> 	return insert_pages(vma, addr, state->__page_arr,
> 			    &nr_pages, vma->vm_page_prot);
>
> VM_WARN_ON_ONCE(state->action != DISCONTIG_KERNEL_PAGE_MAP_CONTIG_PAGES)
> for (i = 0; i < nr_pages; i++, addr += PAGE_SIZE) {
> 	struct page *page = state->__page + i;
> 	int err;
>
> 	err = insert_page(vma, addr, page, vma->vm_page_prot,
> 			  /*mkwrite=*/false);
> 	if (err)
> 		return err;
> }
> ...

OK. Will respin for v6.

>
> --
> Cheers,
>
> David

--
Cheers, Lorenzo