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

[Youngjun Park <youngjun.park@lge.com>]

On Tue, Jul 14, 2026 at 01:25:47AM +0800, Kairui Song via B4 Relay wrote:
> From: Kairui Song <kasong@tencent.com>

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.

> +/*
> + * 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 :)

[...]
> +
> +/*
> + * 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?

[...]

> +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.... 

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

> -/* 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?

> +	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.

> +		if (IS_ERR(si)) {
> +			ret = PTR_ERR(si);
> +			if (ret == -EBUSY)
> +				continue;
> +			break;
> +		}
> +		cluster_alloc_swap_entry(si, folio);
> +
> +		if (folio_test_swapcache(folio)) {
> +			ret = 0;
> +			break;
>		}

[...]

>	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? 

> @@ -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);

Thanks!
Youngjun Park