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

[Chris Li]

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().

I feel that we should provide more generic support for dynamically
growing an array of element size A to size N. Cluster array is just a
user of such an API. There might be more users. This patch hardcodes
the map/umpa of clusters. What about other users of such a dynamic
array?
Another consideration is that the map/unmap logic, while more generic,
adds quite a bit of complexity to an approach intended only for
growth. Any reason we want map/unmap rathern than simple grow from the
current si->max?

I read a bit more about the patch. It seems you need the unmap for the
error cleanup path. In that case I suspect just make cluster a
kmem_cache allocation, cluster array just a pointer. The cluster
pointer array should just grow, not shrink. That might be overall
simpler.
I'm worried someone might abuse the cluster unmap causing unwanted side effects.

I see the other reason might be you want the cluster code to be
exactly the same when XSWAP is off. For that I think it is acceptiale
to turn cluster into kmem_cache allocated and cluster array as
pointers array as seperate patch as the base even when XSWAP is not
configed.

Chris


> Signed-off-by: Baoquan He <baoquan.he@xxxxxxxxx>
> ---
>  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;
> +       }
> +
> +       kfree(pages);
> +
> +       /* Initialize spinlocks for newly mapped clusters */
> +       for (i = start_idx; i < start_idx + nr; i++)
> +               spin_lock_init(&si->cluster_info[i].lock);
> +
> +       /*
> +        * Pairs with READ_ONCE() in shrink/grow paths.
> +        */
> +       WRITE_ONCE(si->nr_clusters_mapped, start_idx + nr);
> +       return 0;
> +
> +fail:
> +       while (i > 0) {
> +               i--;
> +               if (pages[i])
> +                       __free_page(pages[i]);
> +       }
> +       kfree(pages);
> +       return -ENOMEM;
> +}
> +
> +static void xswap_unmap_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);
> +       /*
> +        * Round to page boundaries: start up (skip partial page that may
> +        * contain clusters still in use before start_idx), end up so the
> +        * entire range is covered.  vm_area_unmap_pages() operates on
> +        * whole pages.
> +        */
> +       unsigned long vm_start = PAGE_ALIGN(start_addr);
> +       unsigned long vm_end = PAGE_ALIGN(end_addr);
> +
> +       if (vm_start >= vm_end)
> +               goto out;
> +
> +       vm_area_unmap_pages(si->cluster_vm, vm_start, vm_end);
> +       /*
> +        * vm_area_unmap_pages() only clears PTEs; it does not free the
> +        * physical pages.  Walk the page table to find and free them.
> +        */
> +       /* TODO: free backing pages via page table walk or tracking bitmap */
> +       /*
> +        * Pairs with READ_ONCE() in shrink/grow paths.
> +        */
> +       out:
> +       WRITE_ONCE(si->nr_clusters_mapped, start_idx);
> +}
> +#endif /* CONFIG_XSWAP */
> +
>  static int setup_swap_clusters_info(struct swap_info_struct *si,
>                                     union swap_header *swap_header,
>                                     unsigned long maxpages)
> @@ -3573,6 +3720,63 @@ static int setup_swap_clusters_info(struct swap_info_struct *si,
>         int err = -ENOMEM;
>         unsigned long i;
>
> +#ifdef CONFIG_XSWAP
> +       if (si->flags & SWP_XSWAP) {
> +               unsigned long size = PAGE_ALIGN(nr_clusters * sizeof(*cluster_info));
> +               struct vm_struct *vm;
> +
> +               vm = get_vm_area(size, VM_SPARSE);
> +               if (!vm)
> +                       goto err;
> +
> +               cluster_info = vm->addr;
> +               si->cluster_vm = vm;
> +               si->nr_clusters = nr_clusters;
> +               si->cluster_info = cluster_info;
> +
> +               /* Map the initial chunk (at least cluster 0) */
> +               if (xswap_map_clusters(si, 0, min_t(unsigned long,
> +                                       XSWAP_GROW_CLUSTERS, nr_clusters)))
> +                       goto err_free_vm;
> +
> +               /* xswap: only cluster 0 slot 0 is bad */
> +               err = swap_cluster_setup_bad_slot(si, cluster_info, 0, false);
> +               if (err)
> +                       goto err_unmap;
> +
> +               INIT_LIST_HEAD(&si->free_clusters);
> +               INIT_LIST_HEAD(&si->full_clusters);
> +               INIT_LIST_HEAD(&si->discard_clusters);
> +               for (i = 0; i < SWAP_NR_ORDERS; i++) {
> +                       INIT_LIST_HEAD(&si->nonfull_clusters[i]);
> +                       INIT_LIST_HEAD(&si->frag_clusters[i]);
> +               }
> +
> +               /* Mark mapped clusters: cluster 0 has 1 bad slot, rest free */
> +               for (i = 0; i < si->nr_clusters_mapped; i++) {
> +                       struct swap_cluster_info *ci = &cluster_info[i];
> +
> +                       if (i == 0) {
> +                               ci->flags = CLUSTER_FLAG_NONFULL;
> +                               list_add_tail(&ci->list, &si->nonfull_clusters[0]);
> +                       } else {
> +                               ci->flags = CLUSTER_FLAG_FREE;
> +                               list_add_tail(&ci->list, &si->free_clusters);
> +                       }
> +               }
> +
> +               return 0;
> +
> +err_unmap:
> +               xswap_unmap_clusters(si, 0, si->nr_clusters_mapped);
> +err_free_vm:
> +               free_vm_area(si->cluster_vm);
> +               si->cluster_vm = NULL;
> +               si->cluster_info = NULL;
> +               return err;
> +       }
> +#endif /* CONFIG_XSWAP */
> +
>         cluster_info = kvzalloc_objs(*cluster_info, nr_clusters);
>         if (!cluster_info)
>                 goto err;
> @@ -3640,7 +3844,7 @@ static int setup_swap_clusters_info(struct swap_info_struct *si,
>         si->cluster_info = cluster_info;
>         return 0;
>  err:
> -       free_swap_cluster_info(cluster_info, maxpages);
> +       free_swap_cluster_info(si);
>         return err;
>  }
>
> @@ -3852,7 +4056,7 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
>         si->global_cluster = NULL;
>         inode = NULL;
>         destroy_swap_extents(si, swap_file);
> -       free_swap_cluster_info(si->cluster_info, si->max);
> +       free_swap_cluster_info(si);
>         si->cluster_info = NULL;
>         /*
>          * Clear the SWP_USED flag after all resources are freed so
> --
> 2.54.0
>