Re: [RFC PATCH 03/11] mm, swap: add xswap cluster grow via VM_SPARSE vmalloc

[Baoquan He]

On 07/27/26 at 08:05am, Nhat Pham wrote:
> On Mon, Jul 27, 2026 at 7:05 AM Baoquan He <baoquan.he@xxxxxxxxx> wrote:
> >
> > Implement dynamic cluster_info array growth for xswap devices using a
> > VM_SPARSE vmalloc area:
> >
> > 1. xswap_map_clusters(): Allocate physical pages and map them into
> >    the pre-reserved VM_SPARSE KVA region via vm_area_map_pages().
> >
> > 2. xswap_unmap_clusters(): Unmap pages from the VM_SPARSE area via
> >    vm_area_unmap_pages() (used by the error/teardown paths, shrink
> >    comes later).
> >
> > 3. setup_swap_clusters_info() xswap path: Use get_vm_area(VM_SPARSE)
> >    for the cluster_info array, lazily mapping only the initial chunk.
> >
> > 4. free_swap_cluster_info(): Refactor to take swap_info_struct*.
> >    For xswap, unmap all clusters and free_vm_area().
> >
> > 5. swapoff: Remove snapshot locals; move p->max/p->cluster_info
> >    clearing after free_swap_cluster_info().
> >
> > Signed-off-by: Baoquan He <baoquan.he@xxxxxxxxx>
> 
> I'll give my 2 cents on the direction we're pursuing later. I've done
> a lot of thinking over the past weeks, and I have some new concerns
> now regarding how we plan to land this joint venture :)
> 
> That said, this is a new piece of extension, done per my request
> (kernel-driven dynamic swap address space growth - thanks for taking
> my concerns seriously), and I've also evaluated this vmalloc-based
> data structure in the past week, so I'll put in my inquiries/concerns.
> 
> > ---
> >  mm/swapfile.c | 228 +++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 216 insertions(+), 12 deletions(-)
> >
> > diff --git a/mm/swapfile.c b/mm/swapfile.c
> > index 143088dae07d..6d9c95ed09bd 100644
> > --- a/mm/swapfile.c
> > +++ b/mm/swapfile.c
> > @@ -49,6 +49,24 @@
> >  #include "internal.h"
> >  #include "swap.h"
> >
> > +#ifdef CONFIG_XSWAP
> > +/*
> > + * xswap: dynamically grow the cluster_info array via a VM_SPARSE area.
> > + *
> > + * XSWAP_GROW_CLUSTERS is the number of clusters to map in one grow
> > + * operation.  It is set to the number of cluster_info structs that
> > + * fit in a single page (at least 16), so that the vmalloc page table
> > + * overhead is proportional to the number of clusters mapped.
> > + */
> > +#define XSWAP_GROW_CLUSTERS \
> > +       max_t(unsigned long, PAGE_SIZE / sizeof(struct swap_cluster_info), 16)
> > +
> > +static int xswap_map_clusters(struct swap_info_struct *si,
> > +                             unsigned long start_idx, unsigned long nr);
> > +static void xswap_unmap_clusters(struct swap_info_struct *si,
> > +                                unsigned long start_idx, unsigned long nr);
> > +#endif
> > +
> >  static void swap_range_alloc(struct swap_info_struct *si,
> >                              unsigned int nr_entries);
> >  static bool folio_swapcache_freeable(struct folio *folio);
> > @@ -3041,20 +3059,47 @@ static void wait_for_allocation(struct swap_info_struct *si)
> >
> >         BUG_ON(si->flags & SWP_WRITEOK);
> >
> > +#ifdef CONFIG_XSWAP
> > +       /*
> > +        * xswap clusters beyond nr_clusters_mapped have been unmapped
> > +        * by the shrinker and their vmalloc pages are no longer
> > +        * accessible.  Only iterate over currently mapped clusters.
> > +        */
> > +       if (si->flags & SWP_XSWAP)
> > +               end = min(end, READ_ONCE(si->nr_clusters_mapped) *
> > +                         SWAPFILE_CLUSTER);
> > +#endif
> > +
> >         for (offset = 0; offset < end; offset += SWAPFILE_CLUSTER) {
> >                 ci = swap_cluster_lock(si, offset);
> >                 swap_cluster_unlock(ci);
> >         }
> >  }
> >
> > -static void free_swap_cluster_info(struct swap_cluster_info *cluster_info,
> > -                                  unsigned long maxpages)
> > +static void free_swap_cluster_info(struct swap_info_struct *si)
> >  {
> > +       struct swap_cluster_info *cluster_info = si->cluster_info;
> > +       unsigned long maxpages = si->max;
> >         struct swap_cluster_info *ci;
> > -       int i, nr_clusters = DIV_ROUND_UP(maxpages, SWAPFILE_CLUSTER);
> > +       int i, nr_clusters;
> >
> >         if (!cluster_info)
> >                 return;
> > +
> > +#ifdef CONFIG_XSWAP
> > +       if (si->flags & SWP_XSWAP) {
> > +               /* Unmap all mapped clusters and free the VM_SPARSE area */
> > +               if (si->nr_clusters_mapped > 0)
> > +                       xswap_unmap_clusters(si, 0, si->nr_clusters_mapped);
> > +               free_vm_area(si->cluster_vm);
> > +               si->cluster_vm = NULL;
> > +               si->nr_clusters = 0;
> > +               si->nr_clusters_mapped = 0;
> > +               return;
> > +       }
> > +#endif
> > +
> > +       nr_clusters = DIV_ROUND_UP(maxpages, SWAPFILE_CLUSTER);
> >         for (i = 0; i < nr_clusters; i++) {
> >                 ci = cluster_info + i;
> >                 /* Cluster with bad marks count will have a remaining table */
> > @@ -3093,11 +3138,9 @@ static void flush_percpu_swap_cluster(struct swap_info_struct *si)
> >  SYSCALL_DEFINE1(swapoff, const char __user *, specialfile)
> >  {
> >         struct swap_info_struct *p = NULL;
> > -       struct swap_cluster_info *cluster_info;
> >         struct file *swap_file, *victim;
> >         struct address_space *mapping;
> >         struct inode *inode;
> > -       unsigned int maxpages;
> >         int err, found = 0;
> >
> >         if (!capable(CAP_SYS_ADMIN))
> > @@ -3189,10 +3232,6 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile)
> >
> >         swap_file = p->swap_file;
> >         p->swap_file = NULL;
> > -       maxpages = p->max;
> > -       cluster_info = p->cluster_info;
> > -       p->max = 0;
> > -       p->cluster_info = NULL;
> >         spin_unlock(&p->lock);
> >         spin_unlock(&swap_lock);
> >         arch_swap_invalidate_area(p->type);
> > @@ -3200,7 +3239,9 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile)
> >         mutex_unlock(&swapon_mutex);
> >         kfree(p->global_cluster);
> >         p->global_cluster = NULL;
> > -       free_swap_cluster_info(cluster_info, maxpages);
> > +       free_swap_cluster_info(p);
> > +       p->max = 0;
> > +       p->cluster_info = NULL;
> >
> >         inode = mapping->host;
> >
> > @@ -3564,6 +3605,112 @@ static unsigned long read_swap_header(struct swap_info_struct *si,
> >         return maxpages;
> >  }
> >
> > +#ifdef CONFIG_XSWAP
> > +static int xswap_map_clusters(struct swap_info_struct *si,
> > +                             unsigned long start_idx, unsigned long nr)
> > +{
> > +       unsigned long start_addr = (unsigned long)si->cluster_info +
> > +                                  (size_t)start_idx * sizeof(struct swap_cluster_info);
> > +       unsigned long end_addr = start_addr + (size_t)nr * sizeof(struct swap_cluster_info);
> > +       /*
> > +        * vm_area_map_pages() requires that start and end be page-aligned.
> > +        * If start_addr falls within a page that was already mapped by a
> > +        * previous batch (grow path), round it up to skip the already-mapped
> > +        * partial page.  Always round end_addr up so the vmap page table walk
> > +        * terminates correctly (the walk loop exits when addr == end, and addr
> > +        * advances by PAGE_SIZE each iteration).
> > +        */
> > +       unsigned long vm_start = PAGE_ALIGN(start_addr);
> > +       unsigned long vm_end = PAGE_ALIGN(end_addr);
> > +       unsigned long npages;
> > +       struct page **pages;
> > +       unsigned long i;
> > +
> > +       if (vm_start >= vm_end) {
> > +               /* All requested clusters fall within already-mapped pages. */
> > +               for (i = start_idx; i < start_idx + nr; i++)
> > +                       spin_lock_init(&si->cluster_info[i].lock);
> > +               WRITE_ONCE(si->nr_clusters_mapped, start_idx + nr);
> > +               return 0;
> > +       }
> > +
> > +       npages = (vm_end - vm_start) >> PAGE_SHIFT;
> > +
> > +       pages = kmalloc_array(npages, sizeof(*pages), GFP_KERNEL);
> > +       if (!pages)
> > +               return -ENOMEM;
> > +
> > +       for (i = 0; i < npages; i++) {
> > +               /*
> > +                * __GFP_ZERO is critical: cluster_info structs contain pointer
> > +                * fields (extend_table, zero_bitmap, memcg_table, table) that
> > +                * must start as NULL.  Without zeroing, stale data from a
> > +                * previous user of the page would look like valid pointers.
> > +                */
> > +               pages[i] = alloc_page(GFP_KERNEL | __GFP_ZERO);
> > +               if (!pages[i])
> > +                       goto fail;
> > +       }
> > +
> > +       if (vm_area_map_pages(si->cluster_vm, vm_start, vm_end, pages)) {
> > +               i = npages; /* free all pages on failure */
> > +               goto fail;
> 
> I was evaluating whether your vmalloc array can be extended to support
> kernel-driven dynamic growth at least (either as a slot-in replacement
> for xarray, or as a follow-up optimization if it's too complicated),
> and I stumble this mapping action.
> 
> Seems like it does not take any GFP flag argument, and under the hood
> it calls GFP_KERNEL. Would this be safe in the swap allocation path
> (where you slotted it in in patch 4)? Kairui used a more precise set
> of flags in this path, for e.g for swap table allocation:
> 
> ret = swap_cluster_alloc_table(ci, __GFP_HIGH | __GFP_NOMEMALLOC |
>    GFP_KERNEL);
> 
> 
> I'm guessing this has to do with the fact that we often entered
> swapping out paths with PF_MEMALLOC... Is there any risk of deadlock
> etc.?

Thanks for catching this.  You're right — there are actually three
layers of hardcoded GFP_KERNEL in the VM_SPARSE path:

1. alloc_page(GFP_KERNEL | __GFP_ZERO) in xswap_map_clusters() for the
backing pages
2. vmap_pages_range() hardcodes GFP_KERNEL (passed only to KMSAN, but
still semantically wrong)
3. The page-table allocations inside vmap_pages_range_noflush_walk()
(pte_alloc_kernel_track etc.) use GFP_PGTABLE_KERNEL, which is
GFP_KERNEL | __GFP_ZERO — also lacking __GFP_NOMEMALLOC, and
completely invisible to the caller

When PF_MEMALLOC is set (as it is during reclaim), __gfp_pfmemalloc_flags()
returns ALLOC_NO_WATERMARKS for any allocation without __GFP_NOMEMALLOC,
meaning those allocations can consume all emergency reserves — including
the reserves that the reclaim machinery itself needs to make forward
progress.  That is a real deadlock risk.

One way is to fix them one by one. Another way is to drop the VM_SPARSE
approach entirely and implemented Chris's suggestion instead: cluster_info
is now a struct swap_cluster_info ** pointer array, with each cluster
individually kmem_cache_zalloc()'d. The grow path (xswap_grow_clusters)
now uses:

kmem_cache_zalloc(swap_cluster_cachep,
        __GFP_HIGH | __GFP_NOMEMALLOC | GFP_KERNEL)
matching the pattern Kairui established in swap_cluster_alloc_table().
All allocations are fully GFP-controllable — no hidden vmap page-table
allocations, no hardcoded GFP_PGTABLE_KERNEL.

With kmem_cache, shrink is just kmem_cache_free() — each cluster is
independently allocated and freed. Seems the logic is simpler. If no
objection, I will give it a shot soon.


Thanks
Baoquan