From: Kairui Song <kasong@tencent.com>
Almost all callers of the cluster scan helper require the: lock -> check
usefulness/emptiness check -> allocate -> unlock routine. So merge them
into the same helper to simplify the code.
Signed-off-by: Kairui Song <kasong@tencent.com>
---
mm/swapfile.c | 30 ++++++++----------------------
1 file changed, 8 insertions(+), 22 deletions(-)
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 0d1b17c99221..68dbbbd0dd24 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -923,11 +923,14 @@ static unsigned int alloc_swap_scan_cluster(struct swap_info_struct *si,
bool need_reclaim, ret, usable;
lockdep_assert_held(&ci->lock);
- VM_WARN_ON(!cluster_is_usable(ci, order));
- if (end < nr_pages || ci->count + nr_pages > SWAPFILE_CLUSTER)
+ if (!cluster_is_usable(ci, order) || end < nr_pages ||
+ ci->count + nr_pages > SWAPFILE_CLUSTER)
goto out;
+ if (cluster_is_empty(ci))
+ offset = cluster_offset(si, ci);
+
for (end -= nr_pages; offset <= end; offset += nr_pages) {
need_reclaim = false;
if (!cluster_scan_range(si, ci, offset, nr_pages, &need_reclaim))
@@ -1060,14 +1063,7 @@ static unsigned long cluster_alloc_swap_entry(struct swap_info_struct *si,
goto new_cluster;
ci = swap_cluster_lock(si, offset);
- /* Cluster could have been used by another order */
- if (cluster_is_usable(ci, order)) {
- if (cluster_is_empty(ci))
- offset = cluster_offset(si, ci);
- found = alloc_swap_scan_cluster(si, ci, folio, offset);
- } else {
- swap_cluster_unlock(ci);
- }
+ found = alloc_swap_scan_cluster(si, ci, folio, offset);
if (found)
goto done;
}
@@ -1332,14 +1328,7 @@ static bool swap_alloc_fast(struct folio *folio)
return false;
ci = swap_cluster_lock(si, offset);
- if (cluster_is_usable(ci, order)) {
- if (cluster_is_empty(ci))
- offset = cluster_offset(si, ci);
- alloc_swap_scan_cluster(si, ci, folio, offset);
- } else {
- swap_cluster_unlock(ci);
- }
-
+ alloc_swap_scan_cluster(si, ci, folio, offset);
put_swap_device(si);
return folio_test_swapcache(folio);
}
@@ -1945,10 +1934,7 @@ swp_entry_t swap_alloc_hibernation_slot(int type)
pcp_offset = this_cpu_read(percpu_swap_cluster.offset[0]);
if (pcp_si == si && pcp_offset) {
ci = swap_cluster_lock(si, pcp_offset);
- if (cluster_is_usable(ci, 0))
- offset = alloc_swap_scan_cluster(si, ci, NULL, pcp_offset);
- else
- swap_cluster_unlock(ci);
+ offset = alloc_swap_scan_cluster(si, ci, NULL, pcp_offset);
}
if (!offset)
offset = cluster_alloc_swap_entry(si, NULL);
--
2.52.0On Mon, Feb 16, 2026 at 3:00 AM Kairui Song via B4 Relay
<devnull+kasong.tencent.com@kernel.org> wrote:
>
> From: Kairui Song <kasong@tencent.com>
>
> Almost all callers of the cluster scan helper require the: lock -> check
> usefulness/emptiness check -> allocate -> unlock routine. So merge them
> into the same helper to simplify the code.
Previously, when !cluster_is_usable(ci, order), we only called
swap_cluster_unlock(). Now we do more work in this path:
out:
relocate_cluster(si, ci);
swap_cluster_unlock(ci);
if (si->flags & SWP_SOLIDSTATE) {
this_cpu_write(percpu_swap_cluster.offset[order], next);
this_cpu_write(percpu_swap_cluster.si[order], si);
} else {
si->global_cluster->next[order] = next;
}
return found;
I assume this is what you want to do as well, but can we add
some explanation here?
Also, it would be better to add a comment that
alloc_swap_scan_cluster() expects ci->lock to be held on
entry and releases ci->lock before returning.
>
> Signed-off-by: Kairui Song <kasong@tencent.com>
> ---
> mm/swapfile.c | 30 ++++++++----------------------
> 1 file changed, 8 insertions(+), 22 deletions(-)
>
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 0d1b17c99221..68dbbbd0dd24 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -923,11 +923,14 @@ static unsigned int alloc_swap_scan_cluster(struct swap_info_struct *si,
> bool need_reclaim, ret, usable;
>
> lockdep_assert_held(&ci->lock);
> - VM_WARN_ON(!cluster_is_usable(ci, order));
>
> - if (end < nr_pages || ci->count + nr_pages > SWAPFILE_CLUSTER)
> + if (!cluster_is_usable(ci, order) || end < nr_pages ||
> + ci->count + nr_pages > SWAPFILE_CLUSTER)
> goto out;
>
> + if (cluster_is_empty(ci))
> + offset = cluster_offset(si, ci);
> +
> for (end -= nr_pages; offset <= end; offset += nr_pages) {
> need_reclaim = false;
> if (!cluster_scan_range(si, ci, offset, nr_pages, &need_reclaim))
> @@ -1060,14 +1063,7 @@ static unsigned long cluster_alloc_swap_entry(struct swap_info_struct *si,
> goto new_cluster;
>
> ci = swap_cluster_lock(si, offset);
> - /* Cluster could have been used by another order */
> - if (cluster_is_usable(ci, order)) {
> - if (cluster_is_empty(ci))
> - offset = cluster_offset(si, ci);
> - found = alloc_swap_scan_cluster(si, ci, folio, offset);
> - } else {
> - swap_cluster_unlock(ci);
> - }
> + found = alloc_swap_scan_cluster(si, ci, folio, offset);
> if (found)
> goto done;
> }
> @@ -1332,14 +1328,7 @@ static bool swap_alloc_fast(struct folio *folio)
> return false;
>
> ci = swap_cluster_lock(si, offset);
> - if (cluster_is_usable(ci, order)) {
> - if (cluster_is_empty(ci))
> - offset = cluster_offset(si, ci);
> - alloc_swap_scan_cluster(si, ci, folio, offset);
> - } else {
> - swap_cluster_unlock(ci);
> - }
> -
> + alloc_swap_scan_cluster(si, ci, folio, offset);
> put_swap_device(si);
> return folio_test_swapcache(folio);
> }
> @@ -1945,10 +1934,7 @@ swp_entry_t swap_alloc_hibernation_slot(int type)
> pcp_offset = this_cpu_read(percpu_swap_cluster.offset[0]);
> if (pcp_si == si && pcp_offset) {
> ci = swap_cluster_lock(si, pcp_offset);
> - if (cluster_is_usable(ci, 0))
> - offset = alloc_swap_scan_cluster(si, ci, NULL, pcp_offset);
> - else
> - swap_cluster_unlock(ci);
> + offset = alloc_swap_scan_cluster(si, ci, NULL, pcp_offset);
> }
> if (!offset)
> offset = cluster_alloc_swap_entry(si, NULL);
>
> --
> 2.52.0
>
>
Thanks
Barry
On Mon, Feb 16, 2026 at 03:34:54PM +0800, Barry Song wrote:
> On Mon, Feb 16, 2026 at 3:00 AM Kairui Song via B4 Relay
> <devnull+kasong.tencent.com@kernel.org> wrote:
> >
> > From: Kairui Song <kasong@tencent.com>
> >
> > Almost all callers of the cluster scan helper require the: lock -> check
> > usefulness/emptiness check -> allocate -> unlock routine. So merge them
> > into the same helper to simplify the code.
>
> Previously, when !cluster_is_usable(ci, order), we only called
> swap_cluster_unlock(). Now we do more work in this path:
>
>
> out:
> relocate_cluster(si, ci);
> swap_cluster_unlock(ci);
> if (si->flags & SWP_SOLIDSTATE) {
> this_cpu_write(percpu_swap_cluster.offset[order], next);
> this_cpu_write(percpu_swap_cluster.si[order], si);
> } else {
> si->global_cluster->next[order] = next;
> }
> return found;
>
> I assume this is what you want to do as well, but can we add
> some explanation here?
Yes, that's fine. alloc_swap_scan_cluster is suppose to update the
percpu offset cache so if the cluster is not usable, writing
SWAP_ENTRY_INVALID to invalidate the cache might even be helpful
for future scan. At lease not harmful, I'll add some explanation,
comments.
>
> Also, it would be better to add a comment that
> alloc_swap_scan_cluster() expects ci->lock to be held on
> entry and releases ci->lock before returning.
Thanks for the suggestion, I even thought about renaming the helper
to indicate it will try update the percpu offset and release the lock.
But didn't have a better idea to naming and we also have
alloc_swap_scan_list, leave the name untouched seems more consistent.
I'll just add some comment then.
© 2016 - 2026 Red Hat, Inc.