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

[Kairui Song]

On Wed, Jul 15, 2026 at 11:58 PM Jihan LIN <linjh22s@xxxxxxxxx> wrote:
>
> Hi Kairui,
>
> Thanks for this series! I noticed a few issues in this patch:

Thanks for the review!

> >+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;
>
> ri->rr_counter is left at 0 so the first allocation on each CPU hits
> ri->rr_counter < nr_alloc and rotates, shifting offset away from
> intended cpu % ring->size start. Initializing rr_counter to
> SWAP_ROUND_ROBIN_QUOTA would avoid this, or is this initial rotation
> perhaps intentional?

Yeah, that's true, it hardly matters as we will do million of
allocation and rotates a lot randomly, so the first allocation is
really trivial. But initlalize it as SWAP_ROUND_ROBIN_QUOTA definately
make it more consistent. Agree on that.

> [...]
>
> > @@ -1245,12 +1580,12 @@ static void add_to_avail_list(struct swap_info_struct *si)
> >       unsigned long pages;
> >
> >       spin_lock(&swap_avail_lock);
> >+      spin_lock(&swap_queue_update_lock);
> >
> >       /*
> >-       * Add the device to the avail list if SWP_WRITEOK is set and
> >-       * SWAP_USAGE_OFFLIST_BIT is still set. Swapoff clears
> >-       * SWP_WRITEOK first, so the device won't be re-added after
> >-       * swapoff starts unless swap_device_enable resurrects it.
> >+       * Mark the device as avail if SWP_WRITEOK is set. Swapoff clears
> >+       * SWP_WRITEOK first, so check that first so the device won't be
> >+       * re-added after swapoff started.
> >        */
> >       if (!(si->flags & SWP_WRITEOK))
> >               goto skip;
> >@@ -1261,9 +1596,10 @@ static void add_to_avail_list(struct swap_info_struct *si)
> >       val = atomic_long_fetch_and_relaxed(~SWAP_USAGE_OFFLIST_BIT, &si->inuse_pages);
> >
> >       /*
> >-       * When device is full and device is on the plist, only one updater will
> >-       * see (inuse_pages == si->pages) and will call del_from_avail_list. If
> >-       * that updater happen to be here, just skip adding.
> >+       * When device is full and marked as available, one reader will see
> >+       * (inuse_pages == si->pages) and should mark it as unavailable and
> >+       * set SWAP_USAGE_OFFLIST_BIT. If that updater happens to be here, just
> >+       * skip the rest.
> >        */
> >       pages = si->pages;
> >       if (val == pages) {
> >@@ -1273,10 +1609,10 @@ static void add_to_avail_list(struct swap_info_struct *si)
> >                       goto skip;
> >       }
> >
> >+      swap_queue_unmask(si);
> >       plist_add(&si->avail_list, &swap_avail_head);
> >-
> > skip:
> >-      spin_unlock(&swap_avail_lock);
> >+      spin_unlock(&swap_queue_update_lock);
> > }
> swap_avail_lock is acquired but never released when add_to_avail_list()
> returns.

Ah, that really a silly error of mine, in the following commit
swap_avail_lock is completely removed, so I didn't notice that during
review or test. Need to fix this intermeidate commit. Thanks!

>
> [...]
>
> >@@ -3700,6 +4035,12 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
> >
> >       /* Sets SWP_WRITEOK, resurrect the percpu ref, expose the swap device */
> >       percpu_ref_resurrect(&si->users);
> >+      percpu_down_write(&swapon_rwsem);
> >+      error = swap_queue_add(si);
> >+      percpu_up_write(&swapon_rwsem);
> >+      if (error)
> >+              goto free_swap_zswap;
> >+
> >       swap_device_enable(si);
> The swapfile is inserted into the ring unmasked before SWP_WRITEOK was set in
> swap_device_enable(). Allocators can pick it and get -EBUSY from
> get_swap_device_info(). How about adding masked and then letting
> swap_device_enable() unmask the new swapfile?

Yeah, that's a good idea. I considered it but thought it wasn't
necessary for the RFC, so I skipped that part. We can definitely do
that, it will also help for some other synchronization issues that are
still present.

Thanks again!