Re: [PATCH RFC 09/13] mm/swap: add priority queue for swap device allocation

[Kairui Song]

On Wed, Jul 15, 2026 at 2:10 AM Youngjun Park <youngjun.park@xxxxxxx> wrote:
>
> On Tue, Jul 14, 2026 at 01:25:47AM +0800, Kairui Song via B4 Relay wrote:
> > From: Kairui Song <kasong@xxxxxxxxxxx>
>
> Hi Kairui,
>
> Thanks for the patch :)
> I like the idea. A few questions and comments on the core implementation below.
>
> I'll share my higher-level thoughts (e.g. on a tunable interface for the round-robin policy)
> separately in a follow-up.

Thanks for the review!

> > +/*
> > + * All available swap_info_structs are grouped by priority rings, the rings
> > + * are ordered in a queue by priority (lower prio value = higher priority).
>
> looks like comment updated needed :)

Nice catch :), I got confused by the priority defination again, will fix it.

> [...]
> > +
> > +/*
> > + * The ring is protected by swapon_rwsem so updating it is costly. To make
> > + * the allocator and other users skip full devices faster, the lowest bit of
> > + * a device pointer is used to mark it disabled (temporarily unavailable).
> > + * This relies on struct swap_info_struct being sufficiently
> > + * aligned (guaranteed by kmalloc).
> > + */
> > +#define SWAP_DEVICE_MASKED_SHIFT     0
> > +#define SWAP_DEVICE_MASKED_BIT               BIT(SWAP_DEVICE_MASKED_SHIFT)
>
> Unlike the old plist, full devices stay in the ring, so every
> allocation keeps walking past them. When the higher priority
> devices are all full, each swap out pays one masked pointer check
> per full device before reaching a usable one. That is also the
> steady state for swap tiering, so I want to be sure it stays cheap.
> It looks like a deliberate trade-off of the static ring design.
> What do you think?

Right, this should be super cheap as modern processors are really good
at walking arrays. We have at most 32 swap devices in most cases, only
4 cache lines if they are in the same top ring and I think the CPU
will only spend a dozen cycles on them. And rings are mostly
read-only; they will stay cached for hot access.

If they fallback to secondary rings... I think these devices are
mostly performance insensitive anyway, and CPUs are still pretty good
at reading the readonly arrays.

I did plan to add a ring->avail to skip completely empty rings, or use
cmpxchg & a seq style lock (don't shrink the ring, just shift the
content on full device removal) to update the ring content, to archive
0 overhead if we got more full devices, it works but that's ends up
super complex with no measurable gain.

Maybe we can implement these if this percpu reader queue can be used
as a generic infrastructure for other components, or if we grow to
support hundreds of swaps someday, not really a issue for now I think.

>
> [...]
>
> > +static struct swap_info_struct *swap_queue_get_device(long nr_alloc, int nr_iter)
> > +{
> > +     bool rotate = false;
> > +     struct swap_info_struct *si;
> > +     struct swap_ring_iterator *ri;
> > +     struct swap_prio_ring *ring;
> > +     unsigned int queue_idx;
> > +
> > +     if (!swap_queue_len)
> > +             return ERR_PTR(-ENOENT);
> > +
> > +     queue_idx = 0;
> > +     while (nr_iter >= swap_queue[queue_idx]->size) {
> > +             nr_iter -= swap_queue[queue_idx]->size;
> > +             if (++queue_idx >= swap_queue_len)
> > +                     return ERR_PTR(-ENOENT);
> > +     }
> > +
> > +     ring = swap_queue[queue_idx];
> > +     local_lock(&swap_queue_readers->lock);
> > +     ri = this_cpu_ptr(&swap_queue_readers->ri[queue_idx]);
> > +     /* Rotate while iterating the ring, just not on the first try */
> > +     if (nr_iter)
> > +             rotate = true;
> > +     else if (ri->rr_counter < nr_alloc)
> > +             rotate = true;
> > +     else if (ri->offset >= ring->size)
> > +             rotate = true;
> > +     if (rotate) {
> > +             ri->offset++;
> > +             ri->offset %= ring->size;
> > +             ri->rr_counter = SWAP_ROUND_ROBIN_QUOTA;
> > +     }
> > +     ri->rr_counter -= nr_alloc;
> > +     si = READ_ONCE(ring->dev[ri->offset]);
> > +     local_unlock(&swap_queue_readers->lock);
> > +
> > +     if (swap_device_masked(si))
> > +             return ERR_PTR(-EBUSY);
> > +
> > +     si = swap_device_unmask_ptr(si);
> > +     return si;
> > +}
>
> Tasks interleaving on the same CPU can rotate the ring more than
> once for a single failure, since the lock is dropped between two
> get_device() calls of one allocation. A retry walk can then visit
> one device twice and skip another. It is transient and looks
> harmless, and every fix I could think of costs more than it is
> worth....

You mean dropping the local lock? Rotating it a bit more might be
harmless I think, if that happens randomly, then the rotation is still
fair.

I did think about keep holding the local lock during the whole
process, but making this unsleepable is a trade-off and reduce future
flexibility... the local lock of the cluster below is already causing
some trouble for swap table allocation so I hoped we could avoid that.
So far, stress test didn't show any fragmentation issue.

>
> Unless you have a better idea, how about noting it in a
> comment?

Yeah, we can definitely doc that more cleanly.

> > -/* Rotate the device and switch to a new cluster */
> > -static void swap_alloc_entry(struct folio *folio)
> > +static int swap_alloc_entry(struct folio *folio)
> >  {
> > -     struct swap_info_struct *si, *next;
> > +     long nr_pages = folio_nr_pages(folio);
> > +     struct swap_info_struct *si;
> > +     int nr_iter, ret;
> >
> > -     spin_lock(&swap_avail_lock);
> > -start_over:
> > -     plist_for_each_entry_safe(si, next, &swap_avail_head, avail_list) {
> > -             /* Rotate the device and switch to a new cluster */
> > -             plist_requeue(&si->avail_list, &swap_avail_head);
> > -             spin_unlock(&swap_avail_lock);
> > -             if (get_swap_device_info(si)) {
> > -                     cluster_alloc_swap_entry(si, folio);
> > -                     put_swap_device(si);
> > -                     if (folio_test_swapcache(folio))
> > -                             return;
> > -                     if (folio_test_large(folio))
> > -                             return;
> > +     percpu_down_read(&swapon_rwsem);
>
> The local lock is only held inside swap_queue_get_device() now, so
> the task can migrate between device selection and
> cluster_alloc_swap_entry(). Then rr_counter is charged on one CPU
> while the pages land in another CPU's cluster, so the pacing of
> one cluster per visit does not hold anymore.
>
> Since you want the loop to stay sleepable, how about
> migrate_disable() around the loop?

That sounds like a good idea to avoid a potential race. I think these
races are harmless though.

>
> > +     for (nr_iter = 0;; nr_iter++) {
> > +             si = swap_queue_get_device(nr_pages, nr_iter);
>
> When a ring has a single device, which is probably the most common
> setup, the iterator does nothing useful. Would a fast path that
> skips the local_lock and just reads ring->dev[0] for size == 1
> rings sequentially be worth it? swapon_rwsem keeps the queue stable, so choosing
> between the two paths should be race free, I think.

Ah, that's a good idea indeed.

> >       del_from_avail_list(si, true);
> > +     percpu_up_write(&swapon_rwsem);
> >
> >       /*
> >        * Swap allocator doesn't touch si lock, so looping through all
> > @@ -3042,6 +3375,9 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile)
> >               return err;
> >       }
> >
> > +     percpu_down_write(&swapon_rwsem);
> > +     swap_queue_del(p);
> > +
> >       /*
> >        * Wait for swap operations protected by get/put_swap_device()
> >        * to complete.  Because of synchronize_rcu() here, all swap
> > @@ -3062,7 +3398,6 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile)
> >       if (!(p->flags & SWP_SOLIDSTATE))
> >               atomic_dec(&nr_rotate_swap);
> >
> > -     percpu_down_write(&swapon_rwsem);
> >       spin_lock(&p->lock);
> >       drain_mmlist();
>
> synchronize_rcu() can take a while and it runs here with the write
> lock held, together with wait_for_completion() and flush_work(), so
> a single swapoff can stall all swap allocation during a little bit long time.
>
> Only swap_queue_del() and the final teardown seem to need the write
> lock. If my assumption is right, how about dropping it after
> swap_queue_del() and re-taking it for the teardown?

Good observation! I noticed we could tidy the locking scope, I skipped
that optimization for the RFC :P. Swapoff is very performance
insensitive IMO, it will put a huge pressure to the system and IO, so
an RCU sync / flush hardly matters. The design is heavily optimized
toward readers (ordinary swapin/swapout). But a tidier implementation
and optimizations are definitely great to have.

Thanks again for the detailed review! I think this can be rebased on
top of your tiering work, there are some conflicts but nothing
fundamental, and I think the ring design actually fits well. I've been
experiement this for a while so I think better send it out to plan out
the next steps, and this series can be used to optimize the tiering
alloc as follow up.