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

On 07/29/26 at 11:12pm, Baoquan He wrote:
> On 07/27/26 at 08:05am, Nhat Pham wrote:
> > On Mon, Jul 27, 2026 at 7:05 AM Baoquan He <[email protected]> wrote:
...snip...
> > > +#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.?

Hi Chris,

I gave the kmem_cache approach a try, but ended up keeping VM_SPARSE.
The kmem_cache approach has two structural problems:

1. With individually kmem_cache_alloc()'d clusters, ci - si->cluster_info
   no longer works — cluster_info is a pointer array, not a contiguous
   array.  Each struct swap_cluster_info would need a new field to store
   its own index. VM_SPARSE preserves the contiguous array layout, so
   ci - si->cluster_info continues to work for O(1) index lookup everywhere.

2. The struct swap_cluster_info ** pointer array itself costs extra
   memory: 8 bytes per cluster.  That's 8 KB for a 1 GB swap device,
   and 8 MB for a 1 TB device — paid upfront at swapon, regardless of
   how many clusters are actually used.  VM_SPARSE needs no such
   indirection array; the cluster_info is addressed directly through
   the vmalloc area.

By comparison, the introduced change in struct swap_cluster_info isn't
that great.

How the GFP concern is addressed
--------------------------------
Hi Nhat,

You were right about the three layers of hardcoded GFP_KERNEL in v1.
In the current code (the version I'll post as RFC v2), each is fixed:

1. alloc_page() and kmalloc_array() now use:

     __GFP_HIGH | __GFP_NOMEMALLOC | GFP_KERNEL

   This matches the pattern Kairui established in
   swap_cluster_alloc_table().  __GFP_NOMEMALLOC prevents the
   pfmemalloc reserve bypass during reclaim.

2. The internal page-table allocations inside vmap_pages_range()
   use GFP_PGTABLE_KERNEL and are not directly controllable from
   the caller.  To prevent these from recursing into swap reclaim,
   the entire allocation block is wrapped with:

     noreclaim_flags = memalloc_noreclaim_save();
     ... alloc_page / kmalloc_array / vm_area_map_pages ...
     memalloc_noreclaim_restore(noreclaim_flags);

   memalloc_noreclaim_save() ensures __GFP_FS and __GFP_IO are
   cleared for all allocations within the scope, so the page-table
   allocation cannot recurse into filesystem or swap reclaim.

3. The residual risk is that the page-table allocations still lack
   __GFP_NOMEMALLOC, meaning they could theoretically dip into
   emergency reserves under PF_MEMALLOC.  However, each grow
   operation maps at most one PTE page (XSWAP_GROW_CLUSTERS
   clusters per page), so the total order-0 allocation is tiny —
   typically a single page-table page.  This seems acceptable
   compared to the complexity of plumbing a GFP parameter through
   the entire vmap_pages_range() call chain.

A variant vm_area_map_pages_gfp that passes the caller's GFP context
down to the page-table allocations. That cleanly solves the problem
for all VM_SPARSE users, not just xswap.  I'm happy to work on that
if people think it's the right direction, but I'd prefer to keep it
as a separate improvement rather than blocking this series on it.

The shrink path also avoids this problem entirely: it runs in a
workqueue context where PF_MEMALLOC is never set, so GFP_KERNEL
is correct there.

Thanks
Baoquan

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