Re: [PATCH v3 09/14] mm, swap: defer xswap shrink to workqueue to avoid lock recursion

[Chris Li]

On Wed, Sep 16, 2026 at 12:21 AM Baoquan He <hebaoquan@xxxxxxxxxx> wrote:
>
> __free_cluster() called xswap_try_shrink() while holding ci->lock, but
> shrinking unmaps the backing pages and the subsequent unlock faults on
> the unmapped address. Run the shrink via schedule_work() instead, so no
> cluster lock is held. The work is only scheduled for xswap devices and
> is cancelled on swapoff.
>
> Signed-off-by: Baoquan He <hebaoquan@xxxxxxxxxx>

I will skip the review for shrinking related patches for now.

Chris


> ---
>  include/linux/swap.h |   1 +
>  mm/swapfile.c        | 123 +++++++++++++++++++++++++++++++++----------
>  2 files changed, 97 insertions(+), 27 deletions(-)
>
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index 8c62a53667bb..30642bb481df 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -246,6 +246,7 @@ struct swap_info_struct {
>         struct vm_struct        *cluster_vm;    /* VM_SPARSE area for cluster_info */
>         unsigned long           nr_clusters_max;/* total clusters in the xswap address space */
>         unsigned long           nr_clusters_mapped; /* currently mapped cluster count */
> +       struct work_struct      xswap_shrink_work; /* deferred shrink trigger */
>         struct mutex            xswap_lock;     /* serialize map/unmap operations */
>  #endif
>         struct list_head free_clusters; /* free clusters list */
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 5b31aacb3ec5..351c68bcd70b 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -699,7 +699,9 @@ static void __free_cluster(struct swap_info_struct *si, struct swap_cluster_info
>         move_cluster(si, ci, &si->free_clusters, CLUSTER_FLAG_FREE);
>         ci->order = 0;
>  #ifdef CONFIG_XSWAP
> -       xswap_try_shrink(si);
> +       /* Only xswap devices, and not while the device is being torn down. */
> +       if ((si->flags & SWP_XSWAP) && (si->flags & SWP_WRITEOK))
> +               schedule_work(&si->xswap_shrink_work);
>  #endif
>  }
>
> @@ -3328,6 +3330,7 @@ static void free_swap_cluster_info(struct swap_info_struct *si)
>         if (si->flags & SWP_XSWAP) {
>                 unsigned long nr_mapped;
>
> +               cancel_work_sync(&si->xswap_shrink_work);
>                 /*
>                  * Cluster 0 keeps the bad header slot, so it never empties
>                  * and __free_cluster() never frees its table.
> @@ -3452,6 +3455,11 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile)
>         spin_unlock(&p->lock);
>         spin_unlock(&swap_lock);
>
> +#ifdef CONFIG_XSWAP
> +       if (p->flags & SWP_XSWAP)
> +               cancel_work_sync(&p->xswap_shrink_work);
> +#endif
> +
>         wait_for_allocation(p);
>
>         set_current_oom_origin();
> @@ -4011,8 +4019,9 @@ static int xswap_collect_page(pte_t *pte, unsigned long addr, void *data)
>         return 0;
>  }
>
> -static int xswap_unmap_clusters(struct swap_info_struct *si,
> -                               unsigned long start_idx, unsigned long nr)
> +/* Caller must hold si->xswap_lock; -ENOMEM leaves the mapping intact. */
> +static int xswap_unmap_clusters_locked(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);
> @@ -4025,11 +4034,8 @@ static int xswap_unmap_clusters(struct swap_info_struct *si,
>         unsigned int noreclaim_flags;
>         int i;
>
> -       mutex_lock(&si->xswap_lock);
> -
>         if (vm_start >= vm_end) {
>                 WRITE_ONCE(si->nr_clusters_mapped, start_idx);
> -               mutex_unlock(&si->xswap_lock);
>                 return 0;
>         }
>
> @@ -4049,10 +4055,8 @@ static int xswap_unmap_clusters(struct swap_info_struct *si,
>         xpd.pages = kmalloc_array(npages, sizeof(*xpd.pages),
>                                   __GFP_HIGH | __GFP_NOMEMALLOC | GFP_KERNEL);
>         memalloc_noreclaim_restore(noreclaim_flags);
> -       if (!xpd.pages) {
> -               mutex_unlock(&si->xswap_lock);
> +       if (!xpd.pages)
>                 return -ENOMEM;
> -       }
>
>         xpd.nr = 0;
>         xpd.max = npages;
> @@ -4066,10 +4070,20 @@ static int xswap_unmap_clusters(struct swap_info_struct *si,
>         kfree(xpd.pages);
>
>         WRITE_ONCE(si->nr_clusters_mapped, start_idx);
> -       mutex_unlock(&si->xswap_lock);
>         return 0;
>  }
>
> +static int xswap_unmap_clusters(struct swap_info_struct *si,
> +                               unsigned long start_idx, unsigned long nr)
> +{
> +       int ret;
> +
> +       mutex_lock(&si->xswap_lock);
> +       ret = xswap_unmap_clusters_locked(si, start_idx, nr);
> +       mutex_unlock(&si->xswap_lock);
> +       return ret;
> +}
> +
>  /* Track the end of the run of pages that is already mapped. */
>  static int xswap_mapped_end(pte_t *pte, unsigned long addr, void *data)
>  {
> @@ -4090,6 +4104,16 @@ static int xswap_mapped_end(pte_t *pte, unsigned long addr, void *data)
>  #define XSWAP_SHRINK_SLACK     XSWAP_GROW_CLUSTERS
>  #define XSWAP_SHRINK_MIN       (XSWAP_GROW_CLUSTERS * 4)
>
> +static void xswap_shrink_work_fn(struct work_struct *work)
> +{
> +       struct swap_info_struct *si = container_of(work,
> +                       struct swap_info_struct, xswap_shrink_work);
> +
> +       if (!(READ_ONCE(si->flags) & SWP_WRITEOK))
> +               return;
> +       xswap_try_shrink(si);
> +}
> +
>  /*
>   * Try to shrink the cluster_info tail: unmap contiguous free clusters
>   * at the end of the mapped range.
> @@ -4097,14 +4121,16 @@ static int xswap_mapped_end(pte_t *pte, unsigned long addr, void *data)
>  static void xswap_try_shrink(struct swap_info_struct *si)
>  {
>         struct swap_cluster_info *ci;
> -       unsigned long nr_mapped, last, idx;
> +       unsigned long nr_mapped, nr_tail, nr_unmap, start_idx, i;
>
>         if (!(si->flags & SWP_XSWAP))
>                 return;
>
> +       mutex_lock(&si->xswap_lock);
> +
>         nr_mapped = READ_ONCE(si->nr_clusters_mapped);
> -       if (nr_mapped <= 1) /* keep cluster 0 */
> -               return;
> +       if (nr_mapped <= 1)     /* keep cluster 0 */
> +               goto out_unlock;
>
>         /*
>          * Reclaim on our own, but only once the mapped range is at most
> @@ -4113,27 +4139,69 @@ static void xswap_try_shrink(struct swap_info_struct *si)
>          * an RCU grace period.
>          */
>         if (swap_usage_in_pages(si) * 2 > nr_mapped * SWAPFILE_CLUSTER)
> -               return;
> +               goto out_unlock;
>
> -       /* Find the last non-free cluster from the tail */
> -       last = nr_mapped;
> -       while (last > 1) {
> -               idx = last - 1;
> -               ci = &si->cluster_info[idx];
> -               if (ci->count || ci->flags != CLUSTER_FLAG_FREE)
> +       /*
> +        * Count the free clusters at the tail of the mapped range.  Scanned,
> +        * not tracked: the count must be exact to size the unmap, and an
> +        * incremental count falls behind on out-of-order frees.
> +        */
> +       nr_tail = 0;
> +       while (nr_mapped - nr_tail > 1) {
> +               ci = &si->cluster_info[nr_mapped - nr_tail - 1];
> +               if (READ_ONCE(ci->count) ||
> +                   READ_ONCE(ci->flags) != CLUSTER_FLAG_FREE)
>                         break;
> -               last = idx;
> +               nr_tail++;
>         }
> +       if (nr_tail < XSWAP_SHRINK_SLACK + XSWAP_SHRINK_MIN)
> +               goto out_unlock;
>
> -       if (last == nr_mapped)
> -               return; /* nothing to shrink */
> +       nr_unmap = rounddown(nr_tail - XSWAP_SHRINK_SLACK, XSWAP_GROW_CLUSTERS);
> +       if (!nr_unmap)
> +               goto out_unlock;
> +       start_idx = nr_mapped - nr_unmap;
>
> -       if (nr_mapped - last < XSWAP_SHRINK_SLACK + XSWAP_SHRINK_MIN)
> -               return;
> +       /*
> +        * Only shrink a run that reaches the mapped end; otherwise
> +        * truncating nr_clusters_mapped would orphan the active tail.
> +        */
> +       spin_lock(&si->lock);
> +       for (i = start_idx; i < nr_mapped; i++) {
> +               ci = &si->cluster_info[i];
> +               if (READ_ONCE(ci->flags) != CLUSTER_FLAG_FREE)
> +                       break;
> +               if (!spin_trylock(&ci->lock)) {
> +                       spin_unlock(&si->lock);
> +                       goto out_unlock;
> +               }
> +               spin_unlock(&ci->lock);
> +       }
> +       if (i != nr_mapped) {
> +               spin_unlock(&si->lock);
> +               goto out_unlock;
> +       }
>
> -       last += XSWAP_SHRINK_SLACK;
> +       for (i = start_idx; i < nr_mapped; i++) {
> +               ci = &si->cluster_info[i];
> +               list_del_init(&ci->list);
> +               WRITE_ONCE(ci->flags, CLUSTER_FLAG_NONE);
> +       }
> +       spin_unlock(&si->lock);
>
> -       xswap_unmap_clusters(si, last, nr_mapped - last);
> +       if (xswap_unmap_clusters_locked(si, start_idx, nr_unmap)) {
> +               spin_lock(&si->lock);
> +               for (i = start_idx; i < nr_mapped; i++) {
> +                       ci = &si->cluster_info[i];
> +                       WRITE_ONCE(ci->flags, CLUSTER_FLAG_FREE);
> +                       list_add_tail(&ci->list, &si->free_clusters);
> +               }
> +               spin_unlock(&si->lock);
> +               goto out_unlock;
> +       }
> +
> +out_unlock:
> +       mutex_unlock(&si->xswap_lock);
>  }
>  #endif /* CONFIG_XSWAP */
>
> @@ -4197,6 +4265,7 @@ static int setup_swap_clusters_info(struct swap_info_struct *si,
>                         }
>                 }
>
> +               INIT_WORK(&si->xswap_shrink_work, xswap_shrink_work_fn);
>                 return 0;
>
>  err_unmap:
> --
> 2.54.0
>