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

[Jihan LIN <linjh22s@gmail.com>]

Hi Kairui,

Thanks for this series! I noticed a few issues in this patch:

[...]

>+static void swap_queue_install_readers(struct swap_queue_reader __percpu *readers)
>+{
>+	int ring_idx, cpu;
>+	struct swap_prio_ring *ring;
>+
>+	free_percpu(swap_queue_readers);
>+	swap_queue_readers = readers;
>+	if (!readers)
>+		return;
>+
>+	/* Distribute each CPU's swap IO fairly across devices. */
>+	for_each_possible_cpu(cpu) {
>+		local_lock_init(&per_cpu_ptr(readers, cpu)->lock);
>+
>+		for (ring_idx = 0; ring_idx < swap_queue_len; ring_idx++) {
>+			ring = swap_queue[ring_idx];
>+			per_cpu_ptr(readers, cpu)->ri[ring_idx].offset =
>+				    cpu % ring->size;
>+		}
>+	}
>+}
>+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?

[...]

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

[...]

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

---
Thanks!
Jihan LIN