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

[KunWu Chan]

On Mon, Oct 12, 2026 at 3:23 PM Baoquan He <baoquan.he@xxxxxxxxx> wrote:
>
> On 10/08/26 at 05:10pm, KunWu Chan wrote:
> > On Sat, Oct 3, 2026 at 8:32 AM Baoquan He <hebaoquan@xxxxxxxxxx> wrote:
> ...snip...
> > >  /*
> > >   * Try to shrink the cluster_info tail: unmap contiguous free clusters
> > >   * at the end of the mapped range.
> > > @@ -4164,14 +4187,20 @@ 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, keep, idx;
> > > +       unsigned long nr_mapped, nr_tail, keep, nr_unmap, start_idx, i;
> > >
> > >         if (!(si->flags & SWP_XSWAP))
> > >                 return;
> > >
> > > +       mutex_lock(&si->xswap_lock);
> > > +
> > > +       /* A swapoff raced us and is about to walk this mapping. */
> > > +       if (!(READ_ONCE(si->flags) & SWP_WRITEOK))
> > > +               goto out_unlock;
> > > +
> > >         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
> > > @@ -4181,40 +4210,73 @@ static void xswap_try_shrink(struct swap_info_struct *si)
> > >          */
> > >         if (swap_usage_in_pages(si) * 100 >
> > >             nr_mapped * SWAPFILE_CLUSTER * XSWAP_SHRINK_WHEN)
> > > -               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 (last == nr_mapped)
> > > -               return; /* nothing to shrink */
> > > -
> > > -       /* Below `last` has to stay mapped: the free ones in between are
> > > -        * not part of the tail, and unmapping them orphans what is above.
> > > -        */
> > > -       if (nr_mapped - last < XSWAP_SHRINK_SLACK + XSWAP_SHRINK_MIN)
> > > -               return;
> > > +       if (nr_tail < XSWAP_SHRINK_SLACK + XSWAP_SHRINK_MIN)
> > > +               goto out_unlock;
> > >
> > >         /*
> > >          * Stop at SHRINK_UNTIL rather than at the end of the tail, or the
> > > -        * range comes out full enough for the grow to be woken.
> > > +        * range comes out full enough for the grow to be woken.  Below the
> > > +        * tail everything stays mapped, and one chunk of tail with it.
> > >          */
> > >         keep = DIV_ROUND_UP(swap_usage_in_pages(si) * 100,
> > >                             XSWAP_SHRINK_UNTIL * SWAPFILE_CLUSTER);
> > > -       if (keep < last + XSWAP_SHRINK_SLACK)
> > > -               keep = last + XSWAP_SHRINK_SLACK;
> > > +       if (keep < nr_mapped - nr_tail + XSWAP_SHRINK_SLACK)
> > > +               keep = nr_mapped - nr_tail + XSWAP_SHRINK_SLACK;
> > >
> > >         if (nr_mapped < keep + XSWAP_SHRINK_MIN)
> > > -               return;
> > > +               goto out_unlock;
> > > +
> > > +       nr_unmap = rounddown(nr_mapped - keep, XSWAP_GROW_CLUSTERS);
> > > +       if (!nr_unmap)
> > > +               goto out_unlock;
> > > +       start_idx = nr_mapped - nr_unmap;
> > > +
> > > +       /*
> > > +        * 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;
> > > +       }
> > > +
> > > +       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, keep, nr_mapped - keep);
> > > +       xswap_unmap_clusters_locked(si, start_idx, nr_unmap);
> > > +
> > > +out_unlock:
> > > +       mutex_unlock(&si->xswap_lock);
> > >  }
> > >  #endif /* CONFIG_XSWAP */
> > >
> > > @@ -4278,6 +4340,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
> > >
> >
> > Hi Baoquan,
> >
> > While following the shrink path across these patches, I wanted to
> > clarify the lifetime guarantee for the `cluster_info` backing pages.
> >
> > P07 flushes the per-CPU swap cluster cache and calls
> > `synchronize_rcu()` before unmapping the tail. P09 clears
> > `SWP_WRITEOK` and cancels the shrink work before proceeding with
> > `wait_for_allocation()` and `try_to_unuse()`.
> >
> > Could you clarify how the shrink path ensures that, before the tail
> > is unmapped, no allocation-side reader can still access the tail or
> > republish a stale per-CPU cache entry that points into it, and no
> > swapoff-side walker can still access the unmapped range?
>
> Hi Kunwu,
>
> Thanks a lot for your careful review, and for following the shrink path
> across patches. Assume P07 in your saying means patch 07, and P09 means
> patch 09. For the questions,
>
>
> 1. Allocation-side readers of the tail
>
> xswap_try_shrink() removes the clusters to unmap from their lists while
> holding si->lock, and only proceeds if spin_trylock(&ci->lock) succeeds for
> each of them. After that no allocator can get these clusters from a list.
>
> The flag check alone is not enough. move_cluster() writes ci->flags after it
> drops si->lock, so spin_trylock() is what actually blocks an allocator that
> just took the cluster.
>
> Then, before the pages are unmapped, xswap_unmap_clusters_locked() does
> flush_percpu_swap_cluster() and synchronize_rcu(). That drains the four
> readers that use cluster_info[] under rcu_read_lock():
> alloc_swap_scan_cluster(), swap_reclaim_full_clusters(), swap_alloc_fast(),
> and swap_alloc_hibernation_slot().
>
> 2. Republishing a stale per-CPU cache entry
>
> The per-CPU cache is written in only one place: the end of
> alloc_swap_scan_cluster(), and the value is an offset inside the cluster that
> just got the allocation. That cluster cannot be one the shrink unmaps,
> because the shrink only unmaps CLUSTER_FLAG_FREE clusters and a cluster that
> just took an allocation is not free. So a racing cache write can only point
> to a cluster that stays mapped. The flush plus synchronize_rcu() handles the
> entries written before it.
>
> 3. Swapoff-side walkers
>
> The swapoff walkers are not RCU readers. swapoff first stops the shrink:
> cancel_work_sync(&p->xswap_shrink_work), then waits on p->xswap_lock (which
> grow and shrink also hold). SWP_WRITEOK is cleared, so xswap_try_shrink() bails
> out too. Then wait_for_allocation() drains any in-flight allocation, and
> try_to_unuse() runs. Only after percpu_ref_kill(), synchronize_rcu(), and
> flush_work() of the discard and reclaim works, flush_percpu_swap_cluster()
> run and the mapping get freed.
>
> Not sure if I answer your questions clearly.
>

Hi Baoquan,

Thanks for the detailed explanation. This clarifies my questions
about the cluster_info lifetime and the synchronization between
shrink and swapoff.

I’ll check these paths again against v5.

Thanks,
Kunwu

> Thanks,
> Baoquan