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 <[email protected]>
> Signed-off-by: Lorenzo Stoakes (ARM) <[email protected]>
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.
> +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.
>
> +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.
> +}
> +
> +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 ... )
> +{
> + 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?
[...]
> 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?
> + 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
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;
}
...
--
Cheers,
David
