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

On Mon, Oct 12, 2026 at 05:03:02PM +0200, David Hildenbrand (Arm) wrote:

On 10/9/26 20:04, Lorenzo Stoakes (ARM) wrote:

quoted

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.

quoted

+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.

quoted

+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.

quoted

+}
+
+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.

quoted

+{
+	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.

[...]

quoted

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.

quoted

+		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