Re: [PATCH v5 13/19] mm, swap: add xswap grow trigger on cluster allocation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] mm, swap: premature nr_clusters_mapped update in
  xswap_map_clusters()
- [Medium] mm, swap: cluster exhaustion below 85% usage in
  xswap_should_grow()
--

commit 691004e6412209ac9359059a41013d8433ee5087
Author: Baoquan He <hebaoquan@kylinos.cn>

mm, swap: add xswap grow trigger on cluster allocation

This commit adds on-demand growth of the mapped xswap cluster_info range
from cluster_alloc_swap_entry() when swap usage reaches 85% of the
currently mapped range, growing it back down to 65% usage.

> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 7b1d16226b7bb..ba730fa19194c 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c

[ ... ]

> @@ -1359,6 +1370,18 @@ static unsigned long cluster_alloc_swap_entry(struct swap_info_struct *si,
>  	if (order && !(si->flags & SWP_BLKDEV))
>  		return 0;
>  
> +#ifdef CONFIG_XSWAP
> +	/*
> +	 * Top the range up early.  Every path below leaves through `done`,
> +	 * so this has to come first; mapping pages can sleep, and doing it
> +	 * from the allocation that would otherwise fail leaves nothing to
> +	 * fall back on.
> +	 */
> +	if ((si->flags & SWP_XSWAP) && (si->flags & SWP_WRITEOK) &&
> +	    xswap_should_grow(si))
> +		xswap_grow(si);
> +#endif

[ ... ]

> @@ -1435,6 +1458,12 @@ static unsigned long cluster_alloc_swap_entry(struct swap_info_struct *si,
>  		if (found)
>  			goto done;
>  	}
> +
> +#ifdef CONFIG_XSWAP
> +	/* A concurrent free or grow may have added clusters; retry once. */
> +	if (!found && (si->flags & SWP_XSWAP))
> +		found = alloc_swap_scan_list(si, &si->free_clusters, folio, false);
> +#endif
>  done:
>  	if (!(si->flags & SWP_SOLIDSTATE))
>  		spin_unlock(&si->global_cluster_lock);
> @@ -4168,6 +4197,77 @@ static void xswap_unmap_range(struct swap_info_struct *si,
>  		__free_page(pages[i]);
>  }
>  
> +static bool xswap_should_grow(struct swap_info_struct *si)
> +{
> +	unsigned long mapped = READ_ONCE(si->nr_clusters_mapped);
> +
> +	if (mapped >= READ_ONCE(si->nr_clusters_max))
> +		return false;
> +
> +	return swap_usage_in_pages(si) * 100 >=
> +	       mapped * SWAPFILE_CLUSTER * XSWAP_GROW_WHEN;
> +}

[Severity: Medium]
Can cluster_alloc_swap_entry() run out of usable clusters and fail with
SWAP_ENTRY_INVALID while swap_usage_in_pages(si) is still below 85% of the
mapped range?

Each CPU that misses in swap_alloc_fast() isolates a whole 512-page cluster
from si->free_clusters or si->nonfull_clusters, allocates a single slot,
moves the cluster to si->frag_clusters, and caches it in percpu_swap_cluster.
When the number of concurrently allocating CPUs reaches nr_clusters_mapped
(initially at most XSWAP_GROW_CLUSTERS, i.e. 16 to 85 clusters), or when more
than 15% of mapped clusters are held in per-CPU caches, si->free_clusters and
si->nonfull_clusters are drained while every cluster in si->frag_clusters is
isolated or locked by its owning CPU.

Because total page usage can still be well below 85% (and below the 65%
XSWAP_GROW_UNTIL check in xswap_grow()), xswap_should_grow() returns false
and the fallback at the end of cluster_alloc_swap_entry() only retries
si->free_clusters without growing the mapped range, causing folio_alloc_swap()
to fail prematurely even though unmapped clusters remain available.

> +
> +/*
> + * Map more of the cluster_info array, up to GROW_UNTIL in use.  The caller
> + * holds percpu_swap_cluster.lock, which this drops while it sleeps.
> + */
> +static unsigned long xswap_grow(struct swap_info_struct *si)
> +{
> +	unsigned long mapped = READ_ONCE(si->nr_clusters_mapped);
> +	unsigned long nr_new, want, i;
> +	int ret;
> +
> +	/* The unmap below is paired with the caller's lock. */
> +	lockdep_assert_held(&this_cpu_ptr(&percpu_swap_cluster)->lock);
> +
> +	want = DIV_ROUND_UP(swap_usage_in_pages(si) * 100,
> +			    XSWAP_GROW_UNTIL * SWAPFILE_CLUSTER);
> +	if (want <= mapped)
> +		return 0;
> +
> +	nr_new = rounddown(want - mapped, XSWAP_GROW_CLUSTERS);
> +	if (!nr_new)
> +		nr_new = XSWAP_GROW_CLUSTERS;
> +	if (nr_new > XSWAP_GROW_CHUNKS * XSWAP_GROW_CLUSTERS)
> +		nr_new = XSWAP_GROW_CHUNKS * XSWAP_GROW_CLUSTERS;
> +	nr_new = min(nr_new, READ_ONCE(si->nr_clusters_max) - mapped);
> +	if (!nr_new)
> +		return 0;
> +
> +	/*
> +	 * Mapping pages can sleep.  The lock only guards the per-cpu
> +	 * cluster cache, which this path does not touch.
> +	 */
> +	local_unlock(&percpu_swap_cluster.lock);
> +	ret = xswap_map_clusters(si, mapped, nr_new);
> +	local_lock(&percpu_swap_cluster.lock);
> +	if (ret)
> +		return 0;
> +
> +	/*
> +	 * Link the clusters even if swapoff cleared SWP_WRITEOK while we
> +	 * mapped. The caller keeps the device alive, and nothing takes a
> +	 * cluster while WRITEOK is clear. If try_to_unuse() fails, the
> +	 * reinserted device needs them.
> +	 */
> +	for (i = mapped; i < mapped + nr_new; i++) {
> +		struct swap_cluster_info *ci = &si->cluster_info[i];
> +
> +		/*
> +		 * A concurrent grower may have taken these already;
> +		 * only add the off-list ones.
> +		 */
> +		spin_lock(&ci->lock);
> +		if (ci->flags == CLUSTER_FLAG_NONE)
> +			move_cluster(si, ci, &si->free_clusters,
> +				     CLUSTER_FLAG_FREE);
> +		spin_unlock(&ci->lock);
> +	}
> +
> +	return nr_new;
> +}

[Severity: Medium]
Can concurrent swap allocations fail in cluster_alloc_swap_entry() between
xswap_map_clusters() updating si->nr_clusters_mapped and xswap_grow() linking
the new clusters onto si->free_clusters?

xswap_map_clusters() publishes the new si->nr_clusters_mapped and drops
si->xswap_lock before returning to xswap_grow():

mm/swapfile.c:xswap_map_clusters() {
    ...
	smp_store_release(&si->nr_clusters_mapped, start_idx + nr);
	mutex_unlock(&si->xswap_lock);
	return 0;
    ...
}

Until the winning grower re-acquires percpu_swap_cluster.lock and runs the
move_cluster() loop, the newly mapped clusters remain off-list with
CLUSTER_FLAG_NONE:

CPU0                              CPU1
cluster_alloc_swap_entry()        cluster_alloc_swap_entry()
  xswap_grow()
    xswap_map_clusters()
      mutex_lock(&si->xswap_lock)
      ...
      smp_store_release(
        &si->nr_clusters_mapped, ...)
      mutex_unlock(&si->xswap_lock)
                                    xswap_should_grow() -> false
                                    /* scans cluster lists: empty */
                                    alloc_swap_scan_list(
                                      &si->free_clusters) -> 0
                                      <-- returns SWAP_ENTRY_INVALID
    local_lock(...)
    move_cluster(...,
      &si->free_clusters, ...)

Similarly, a concurrent caller that already entered xswap_grow() and waited
on si->xswap_lock wakes up, sees start_idx != si->nr_clusters_mapped, gets
-EAGAIN from xswap_map_clusters(), and returns 0 from xswap_grow() before CPU0
links the clusters.  When existing clusters are exhausted during the grow,
these concurrent allocations return SWAP_ENTRY_INVALID despite the newly
mapped capacity.  Linking the new clusters onto si->free_clusters (or
updating si->nr_clusters_mapped after linking) while still serialized by
si->xswap_lock would avoid this window.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109055455.536925-1-baoquan.he@linux.dev?part=13