Re: [RFC PATCH 1/1] mm/swap: allocate contiguous swap slots for a reclaim batch

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mm/swap: stack use-after-return on skipped list in alloc_swap_scan_list_contig()
- [Medium] mm/swap: folio_alloc_swap_split() skips cursor check and order accounting
- [Medium] mm/swap: swap_alloc_prepare() and swap_alloc_fast() strand usable clusters on frag_clusters
--

commit 0160f7d5d1e92861c5a388fc552e5871f0f57f93
Author: Alexandre Ghiti <alex@ghiti.fr>

mm/swap: allocate contiguous swap slots for a reclaim batch

This commit records the number of pages per small order in a reclaim batch
before swapping them out and updates the swap cluster allocator to seek
contiguous runs of up to SWAP_CLUSTER_MAX free slots in nonfull or free
clusters.

> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index c3288910b3e35..67161e1849052 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c

[ ... ]

> @@ -1042,6 +1079,59 @@ static unsigned int alloc_swap_scan_list(struct swap_info_struct *si,
>  	return found;
>  }
>  
> +/* Larger folios already make large enough write requests on their own */
> +#define SWAP_BATCH_MAX_ORDER	(ilog2(SWAP_CLUSTER_MAX) - 1)
> +
> +static inline unsigned int swap_batch_contig(unsigned int left)
> +{
> +	return min_t(unsigned int, left, SWAP_CLUSTER_MAX);
> +}
> +
> +/*
> + * Like alloc_swap_scan_list(), but only allocate at the start of @nr_contig
> + * free slots. Order 0 clusters without them move to the frag list until a
> + * slot is freed, larger orders stay nonfull as the folio may still fit.
> + */
> +static unsigned int alloc_swap_scan_list_contig(struct swap_info_struct *si,
> +						struct list_head *list,
> +						struct folio *folio,
> +						unsigned int nr_contig)
> +{
> +	unsigned int found = SWAP_ENTRY_INVALID, order = folio_order(folio);
> +	struct swap_cluster_info *ci;
> +	unsigned long start, offset;
> +	LIST_HEAD(skipped);
> +
> +	while ((ci = isolate_lock_cluster(si, list))) {
> +		start = cluster_offset(si, ci);
> +		offset = SWAP_ENTRY_INVALID;
> +		if (SWAPFILE_CLUSTER - ci->count >= nr_contig)
> +			offset = cluster_find_free_contig(ci, start,
> +							  start + SWAPFILE_CLUSTER,
> +							  1 << order, nr_contig);
> +		if (offset == SWAP_ENTRY_INVALID) {
> +			if (order)
> +				move_cluster(si, ci, &skipped,
> +					     CLUSTER_FLAG_NONFULL);
> +			else
> +				relocate_cluster(si, ci);
> +			swap_cluster_unlock(ci);
> +			continue;
> +		}
> +		found = alloc_swap_scan_cluster(si, ci, folio, offset);
> +		if (found)
> +			break;
> +	}
> +
> +	if (!list_empty(&skipped)) {
> +		spin_lock(&si->lock);
> +		list_splice_tail(&skipped, list);
> +		spin_unlock(&si->lock);
> +	}
> +
> +	return found;
> +}

[Severity: High]
Can moving an order > 0 cluster onto the stack-local skipped list with
CLUSTER_FLAG_NONFULL and then dropping ci->lock cause a stack
use-after-return on skipped?

When shrink_folio_list() reclaims a batch of mTHP folios (order > 0, up to
SWAP_BATCH_MAX_ORDER), folio_alloc_swap() -> swap_alloc_slow() ->
cluster_alloc_swap_entry() -> alloc_swap_scan_list_contig() places any
cluster from si->nonfull_clusters[order] lacking nr_contig contiguous free
slots onto skipped with ci->flags set to CLUSTER_FLAG_NONFULL, and then
unlocks ci->lock.

While ci is unlocked on skipped, another CPU can lock ci by offset (for
example, when freeing its remaining entries via
__swap_cluster_free_entries() -> free_cluster(), or through a cached
percpu_swap_cluster / global_cluster cursor in swap_alloc_fast() /
cluster_alloc_swap_entry() -> alloc_swap_scan_cluster() ->
relocate_cluster()). Because ci->flags != CLUSTER_FLAG_NONE, move_cluster()
removes ci from skipped under si->lock:

mm/swapfile.c:move_cluster() {
	spin_lock(&si->lock);
	if (ci->flags == CLUSTER_FLAG_NONE)
		list_add_tail(&ci->list, list);
	else
		list_move_tail(&ci->list, list);
	spin_unlock(&si->lock);
	ci->flags = new_flags;
}

When ci is the last entry on skipped, __list_del(&skipped, &skipped) in
include/linux/list.h updates skipped.prev and skipped.next without a
memory barrier:

static inline void __list_del(struct list_head * prev, struct list_head * next)
{
	next->prev = prev;
	WRITE_ONCE(prev->next, next);
}

Because alloc_swap_scan_list_contig() tests !list_empty(&skipped)
(READ_ONCE(skipped.next) == &skipped) outside si->lock before returning,
can it observe skipped.next == &skipped, skip spin_lock(&si->lock), and
return while the other CPU is still writing skipped.prev on the popped
stack frame?

Also, while those order > 0 clusters sit on the local skipped list, they
are hidden from concurrent alloc_swap_scan_list() callers scanning
si->nonfull_clusters[order], which could cause premature -E2BIG splits.

[ ... ]

> @@ -1390,7 +1503,15 @@ static bool swap_alloc_fast(struct folio *folio)
>  	if (cluster_is_usable(ci, order)) {
>  		if (cluster_is_empty(ci))
>  			offset = cluster_offset(si, ci);
> -		alloc_swap_scan_cluster(si, ci, folio, offset);
> +		/* The batch's contiguous slots are used up */
> +		if (this_cpu_read(percpu_swap_cluster.batch_left[order]) &&
> +		    !cluster_slots_free(ci, offset, 1 << order)) {
> +			swap_cluster_unlock(ci);
> +			this_cpu_write(percpu_swap_cluster.offset[order],
> +				       SWAP_ENTRY_INVALID);
> +		} else {
> +			alloc_swap_scan_cluster(si, ci, folio, offset);
> +		}
>  	} else {
>  		swap_cluster_unlock(ci);
>  	}
> @@ -1729,6 +1850,86 @@ static int swap_dup_entries_cluster(struct swap_info_struct *si,
>  	return err;
>  }
>  
> +static void swap_alloc_prepare(unsigned int order, unsigned int nr)
> +{
> +	unsigned int nr_contig = swap_batch_contig(nr);
> +	struct swap_cluster_info *ci;
> +	struct swap_info_struct *si;
> +	unsigned long offset;
> +	bool keep = false;
> +
> +	local_lock(&percpu_swap_cluster.lock);
> +	if (nr <= (1 << order)) {
> +		this_cpu_write(percpu_swap_cluster.batch_left[order], 0);
> +		goto out;
> +	}
> +	this_cpu_write(percpu_swap_cluster.batch_left[order], nr);
> +
> +	/* Keep the cursor if enough free slots follow it */
> +	si = this_cpu_read(percpu_swap_cluster.si[order]);
> +	offset = this_cpu_read(percpu_swap_cluster.offset[order]);
> +	if (si && offset && get_swap_device_info(si)) {
> +		ci = swap_cluster_lock(si, offset);
> +		if (cluster_is_usable(ci, order)) {
> +			if (cluster_is_empty(ci))
> +				offset = cluster_offset(si, ci);
> +			keep = offset % SWAPFILE_CLUSTER + nr_contig <=
> +			       SWAPFILE_CLUSTER &&
> +			       cluster_find_free_contig(ci, offset,
> +							offset + nr_contig,
> +							1 << order,
> +							nr_contig) == offset;
> +		}
> +		swap_cluster_unlock(ci);
> +		put_swap_device(si);
> +	}
> +	if (!keep)
> +		this_cpu_write(percpu_swap_cluster.offset[order],
> +			       SWAP_ENTRY_INVALID);
> +out:
> +	local_unlock(&percpu_swap_cluster.lock);
> +}

[Severity: Medium]
When swap_alloc_prepare() or swap_alloc_fast() clears
percpu_swap_cluster.offset[order] to SWAP_ENTRY_INVALID without calling
alloc_swap_scan_cluster(), does that strand usable clusters on
si->frag_clusters[order]?

On the first allocation from a cluster, alloc_swap_scan_cluster() calls
relocate_cluster(), which moves any partially populated cluster
(0 < ci->count < SWAPFILE_CLUSTER) to si->frag_clusters[ci->order]:

mm/swapfile.c:relocate_cluster() {
	...
	} else if (ci->count != SWAPFILE_CLUSTER) {
		if (ci->flags != CLUSTER_FLAG_FRAG)
			move_cluster(si, ci, &si->frag_clusters[ci->order],
				     CLUSTER_FLAG_FRAG);
	}
}

Previously, the per-CPU cursor kept allocating from that cluster until the
end of the cluster was reached. With this change, if swap_alloc_prepare()
finds fewer than nr_contig free slots immediately at offset, or if
swap_alloc_fast() (or cluster_alloc_swap_entry() for rotating storage)
encounters an occupied slot at offset while batch_left[order] is non-zero,
the cursor is abandoned while ci remains on si->frag_clusters[order], even
if large contiguous free regions remain later in the same cluster.

Since si->frag_clusters[order] is only scanned after si->nonfull_clusters
and si->free_clusters are exhausted (and only one cluster at a time), could
abandoning the cursor without returning ci to si->nonfull_clusters[order]
or scanning the rest of ci cause premature cluster exhaustion and -E2BIG
mTHP swapout fallbacks?

> +
> +/**
> + * folio_alloc_swap_prepare - prepare swap allocation for a reclaim batch
> + * @folios: list of folios reclaim is about to swap out
> + *
> + * Record how many pages of each small order the batch will swap out, so
> + * that their slots are allocated contiguously. Nothing is reserved.
> + */
> +void folio_alloc_swap_prepare(struct list_head *folios)
> +{
> +	unsigned int max_order = min(SWAP_BATCH_MAX_ORDER, SWAP_NR_ORDERS - 1);
> +	unsigned int nr[SWAP_BATCH_MAX_ORDER + 1] = {};
> +	struct folio *folio;
> +	unsigned int order;
> +
> +	if (!total_swap_pages)
> +		return;
> +
> +	list_for_each_entry(folio, folios, lru) {
> +		if (!folio_test_swapbacked(folio) || folio_test_swapcache(folio))
> +			continue;
> +		order = folio_order(folio);
> +		if (order <= max_order)
> +			nr[order] += 1 << order;
> +	}
> +
> +	for (order = 0; order <= max_order; order++)
> +		swap_alloc_prepare(order, nr[order]);
> +}
> +
> +/**
> + * folio_alloc_swap_split - account for a large folio split by reclaim
> + * @nr_pages: number of order-0 folios the split produced
> + */
> +void folio_alloc_swap_split(unsigned int nr_pages)
> +{
> +	local_lock(&percpu_swap_cluster.lock);
> +	this_cpu_add(percpu_swap_cluster.batch_left[0], nr_pages);
> +	local_unlock(&percpu_swap_cluster.lock);
> +}

[Severity: Medium]
Should folio_alloc_swap_split() also validate percpu_swap_cluster.offset[0]
for contiguous free slots and decrement batch_left[order] for the split
folio's original order?

When folio_alloc_swap_prepare() sees 0 or 1 order-0 folios in the batch,
swap_alloc_prepare(0, nr[0]) sets batch_left[0] to 0 and leaves the
existing percpu_swap_cluster.offset[0] cursor untouched. If a large folio
is subsequently split in shrink_folio_list() or shmem_writeout(),
folio_alloc_swap_split() adds nr_pages to batch_left[0] without checking
whether percpu_swap_cluster.offset[0] has swap_batch_contig(batch_left[0])
contiguous free slots following it.

Because swap_alloc_fast() only checks cluster_slots_free(ci, offset, 1) for
order-0 allocations, if percpu_swap_cluster.offset[0] points into a small
free hole, the first subpages of the split folio will be allocated into
that hole until an occupied slot is reached.

In addition, if the split folio had order <= max_order,
folio_alloc_swap_prepare() already added 1 << order to batch_left[order].
When that folio is split before folio_alloc_swap() (such as a partially
mapped folio in shrink_folio_list() or a folio in shmem_writeout()) or
after folio_alloc_swap() fails without clearing batch_left[order],
batch_left[order] is not decremented, leaving it inflated for subsequent
folios of that order.

титься

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022145615.2184343-1-alex@ghiti.fr?part=1