[PATCH 00/12] Tiny cleanup and improvements about SWAP code

[PATCH 00/12] Tiny cleanup and improvements about SWAP code

[Baoquan He]

These are all made during reviewing and learning below patchset from
Kairui.

[PATCH v3 00/13] mm, swap: rework of swap allocator locks

Baoquan He (12):
  mm/swap_state.c: fix the obsolete code comment
  mm/swap_state.c: optimize the code in clear_shadow_from_swap_cache()
  mm/swap: remove SWAP_FLAG_PRIO_SHIFT
  mm/swap: skip scanning cluster range if it's empty cluster
  mm/swap: rename swap_is_has_cache() to swap_only_has_cache()
  mm/swapfile.c: update the code comment above swap_count_continued()
  mm/swapfile.c: optimize code in setup_clusters()
  mm/swap_state.c: remove the meaningless code comment
  mm/swapfile.c: remove the unneeded checking
  mm/swap: rename swap_swapcount() to swap_entry_swapped()
  mm/swapfile.c: remove the incorrect code comment
  mm/swapfile.c: open code cluster_alloc_swap()

 include/linux/swap.h |  7 ++--
 mm/swap_state.c      | 12 ++-----
 mm/swapfile.c        | 83 +++++++++++++++++---------------------------
 3 files changed, 37 insertions(+), 65 deletions(-)

-- 
2.41.0

[PATCH 01/12] mm/swap_state.c: fix the obsolete code comment

[Baoquan He]

Since commit 85a1333417a7 ("mm/swap: use dedicated entry for swap in
folio"), there's a dedicated field in folio for swap entry. Let's
update the code comment above add_to_swap_cache() accordingly.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 mm/swap_state.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/swap_state.c b/mm/swap_state.c
index ca42b2be64d9..59acb55174c8 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -85,7 +85,7 @@ void *get_shadow_from_swap_cache(swp_entry_t entry)
 
 /*
  * add_to_swap_cache resembles filemap_add_folio on swapper_space,
- * but sets SwapCache flag and private instead of mapping and index.
+ * but sets SwapCache flag and 'swap' instead of mapping and index.
  */
 int add_to_swap_cache(struct folio *folio, swp_entry_t entry,
 			gfp_t gfp, void **shadowp)
-- 
2.41.0

[PATCH 02/12] mm/swap_state.c: optimize the code in clear_shadow_from_swap_cache()

[Baoquan He]

Use ALIGN to achieve the same effect and simplify the code.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 mm/swap_state.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/mm/swap_state.c b/mm/swap_state.c
index 59acb55174c8..d7cbf5bb2450 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -270,9 +270,7 @@ void clear_shadow_from_swap_cache(int type, unsigned long begin,
 		xa_unlock_irq(&address_space->i_pages);
 
 		/* search the next swapcache until we meet end */
-		curr >>= SWAP_ADDRESS_SPACE_SHIFT;
-		curr++;
-		curr <<= SWAP_ADDRESS_SPACE_SHIFT;
+		curr = ALIGN((curr + 1), SWAP_ADDRESS_SPACE_PAGES);
 		if (curr > end)
 			break;
 	}
-- 
2.41.0

[PATCH 03/12] mm/swap: remove SWAP_FLAG_PRIO_SHIFT

[Baoquan He]

It doesn't make sense to have a zero value of shift. Remove it to
avoid confusion.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 include/linux/swap.h | 1 -
 mm/swapfile.c        | 3 +--
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/linux/swap.h b/include/linux/swap.h
index b13b72645db3..20bfedc8c4f2 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -24,7 +24,6 @@ struct pagevec;
 
 #define SWAP_FLAG_PREFER	0x8000	/* set if swap priority specified */
 #define SWAP_FLAG_PRIO_MASK	0x7fff
-#define SWAP_FLAG_PRIO_SHIFT	0
 #define SWAP_FLAG_DISCARD	0x10000 /* enable discard for swap */
 #define SWAP_FLAG_DISCARD_ONCE	0x20000 /* discard swap area at swapon-time */
 #define SWAP_FLAG_DISCARD_PAGES 0x40000 /* discard page-clusters after use */
diff --git a/mm/swapfile.c b/mm/swapfile.c
index ba19430dd4ea..9c9a4ec6d4c6 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -3449,8 +3449,7 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
 	mutex_lock(&swapon_mutex);
 	prio = -1;
 	if (swap_flags & SWAP_FLAG_PREFER)
-		prio =
-		  (swap_flags & SWAP_FLAG_PRIO_MASK) >> SWAP_FLAG_PRIO_SHIFT;
+		prio = swap_flags & SWAP_FLAG_PRIO_MASK;
 	enable_swap_info(si, prio, swap_map, cluster_info, zeromap);
 
 	pr_info("Adding %uk swap on %s.  Priority:%d extents:%d across:%lluk %s%s%s%s\n",
-- 
2.41.0

[PATCH 04/12] mm/swap: skip scanning cluster range if it's empty cluster

[Baoquan He]

Since ci->lock has been taken when isolating cluster from
si->free_clusters or taking si->percpu_cluster->next[order],
it's unnecessary to scan and check the cluster range availability
if i'ts empty cluster, and this can accelerate the huge page
swapping.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 mm/swapfile.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 9c9a4ec6d4c6..61efde853eea 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -729,6 +729,9 @@ static bool cluster_scan_range(struct swap_info_struct *si,
 	unsigned long offset, end = start + nr_pages;
 	unsigned char *map = si->swap_map;
 
+	if (cluster_is_empty(ci))
+		return true;
+
 	for (offset = start; offset < end; offset++) {
 		switch (READ_ONCE(map[offset])) {
 		case 0:
-- 
2.41.0

[PATCH 05/12] mm/swap: rename swap_is_has_cache() to swap_only_has_cache()

[Baoquan He]

There are two predicates in the name of swap_is_has_cache() which
is confusing. Renaming it to remove the confusion and can better
reflect its functionality.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 mm/swapfile.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 61efde853eea..bf284ba16198 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -161,7 +161,7 @@ static long swap_usage_in_pages(struct swap_info_struct *si)
 /* Reclaim directly, bypass the slot cache and don't touch device lock */
 #define TTRS_DIRECT		0x8
 
-static bool swap_is_has_cache(struct swap_info_struct *si,
+static bool swap_only_has_cache(struct swap_info_struct *si,
 			      unsigned long offset, int nr_pages)
 {
 	unsigned char *map = si->swap_map + offset;
@@ -243,7 +243,7 @@ static int __try_to_reclaim_swap(struct swap_info_struct *si,
 	 * reference or pending writeback, and can't be allocated to others.
 	 */
 	ci = lock_cluster(si, offset);
-	need_reclaim = swap_is_has_cache(si, offset, nr_pages);
+	need_reclaim = swap_only_has_cache(si, offset, nr_pages);
 	unlock_cluster(ci);
 	if (!need_reclaim)
 		goto out_unlock;
@@ -1572,7 +1572,7 @@ void put_swap_folio(struct folio *folio, swp_entry_t entry)
 		return;
 
 	ci = lock_cluster(si, offset);
-	if (swap_is_has_cache(si, offset, size))
+	if (swap_only_has_cache(si, offset, size))
 		swap_entry_range_free(si, ci, entry, size);
 	else {
 		for (int i = 0; i < size; i++, entry.val++) {
-- 
2.41.0

[PATCH 06/12] mm/swapfile.c: update the code comment above swap_count_continued()

[Baoquan He]

Now, swap_count_continued() has two callers, __swap_duplicate() and
__swap_entry_free_locked(), the relevant code comment is stale.
Update it to reflect the current situation.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 mm/swapfile.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index bf284ba16198..9ee2238042a5 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -3789,8 +3789,8 @@ int add_swap_count_continuation(swp_entry_t entry, gfp_t gfp_mask)
  * into, carry if so, or else fail until a new continuation page is allocated;
  * when the original swap_map count is decremented from 0 with continuation,
  * borrow from the continuation and report whether it still holds more.
- * Called while __swap_duplicate() or swap_entry_free() holds swap or cluster
- * lock.
+ * Called while __swap_duplicate() or caller of __swap_entry_free_locked()
+ * holds swap or cluster lock.
  */
 static bool swap_count_continued(struct swap_info_struct *si,
 				 pgoff_t offset, unsigned char count)
-- 
2.41.0

[PATCH 07/12] mm/swapfile.c: optimize code in setup_clusters()

[Baoquan He]

In the last 'for' loop inside setup_clusters(), using two local variable
'k' and 'j' are obvisouly redundant. Using 'j' is enough and simpler.

And also move macro SWAP_CLUSTER_COLS close to its only user
setup_clusters().

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 mm/swapfile.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 9ee2238042a5..78b7329ad2c7 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -3123,13 +3123,6 @@ static unsigned long read_swap_header(struct swap_info_struct *si,
 	return maxpages;
 }
 
-#define SWAP_CLUSTER_INFO_COLS						\
-	DIV_ROUND_UP(L1_CACHE_BYTES, sizeof(struct swap_cluster_info))
-#define SWAP_CLUSTER_SPACE_COLS						\
-	DIV_ROUND_UP(SWAP_ADDRESS_SPACE_PAGES, SWAPFILE_CLUSTER)
-#define SWAP_CLUSTER_COLS						\
-	max_t(unsigned int, SWAP_CLUSTER_INFO_COLS, SWAP_CLUSTER_SPACE_COLS)
-
 static int setup_swap_map_and_extents(struct swap_info_struct *si,
 					union swap_header *swap_header,
 					unsigned char *swap_map,
@@ -3169,13 +3162,20 @@ static int setup_swap_map_and_extents(struct swap_info_struct *si,
 	return nr_extents;
 }
 
+#define SWAP_CLUSTER_INFO_COLS						\
+	DIV_ROUND_UP(L1_CACHE_BYTES, sizeof(struct swap_cluster_info))
+#define SWAP_CLUSTER_SPACE_COLS						\
+	DIV_ROUND_UP(SWAP_ADDRESS_SPACE_PAGES, SWAPFILE_CLUSTER)
+#define SWAP_CLUSTER_COLS						\
+	max_t(unsigned int, SWAP_CLUSTER_INFO_COLS, SWAP_CLUSTER_SPACE_COLS)
+
 static struct swap_cluster_info *setup_clusters(struct swap_info_struct *si,
 						union swap_header *swap_header,
 						unsigned long maxpages)
 {
 	unsigned long nr_clusters = DIV_ROUND_UP(maxpages, SWAPFILE_CLUSTER);
 	struct swap_cluster_info *cluster_info;
-	unsigned long i, j, k, idx;
+	unsigned long i, j, idx;
 	int cpu, err = -ENOMEM;
 
 	cluster_info = kvcalloc(nr_clusters, sizeof(*cluster_info), GFP_KERNEL);
@@ -3236,8 +3236,7 @@ static struct swap_cluster_info *setup_clusters(struct swap_info_struct *si,
 	 * Reduce false cache line sharing between cluster_info and
 	 * sharing same address space.
 	 */
-	for (k = 0; k < SWAP_CLUSTER_COLS; k++) {
-		j = k % SWAP_CLUSTER_COLS;
+	for (j = 0; j < SWAP_CLUSTER_COLS; j++) {
 		for (i = 0; i < DIV_ROUND_UP(nr_clusters, SWAP_CLUSTER_COLS); i++) {
 			struct swap_cluster_info *ci;
 			idx = i * SWAP_CLUSTER_COLS + j;
-- 
2.41.0

[PATCH 08/12] mm/swap_state.c: remove the meaningless code comment

[Baoquan He]

Since commit 8d93b41c09d1 ("mm: Convert add_to_swap_cache to XArray"),
there's no returned _EEXIT, so the code comment doesn't make sense any
more.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 mm/swap_state.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/mm/swap_state.c b/mm/swap_state.c
index d7cbf5bb2450..b393cefb3be0 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -203,10 +203,6 @@ bool add_to_swap(struct folio *folio)
 	err = add_to_swap_cache(folio, entry,
 			__GFP_HIGH|__GFP_NOMEMALLOC|__GFP_NOWARN, NULL);
 	if (err)
-		/*
-		 * add_to_swap_cache() doesn't return -EEXIST, so we can safely
-		 * clear SWAP_HAS_CACHE flag.
-		 */
 		goto fail;
 	/*
 	 * Normally the folio will be dirtied in unmap because its
-- 
2.41.0

[PATCH 09/12] mm/swapfile.c: remove the unneeded checking

[Baoquan He]

In free_swap_and_cache_nr(), invocation of get_swap_device() has done
the checking if it's a swap entry. So remove the redundant checking
here.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 mm/swapfile.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 78b7329ad2c7..2a25ff5f31c8 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1784,9 +1784,6 @@ void free_swap_and_cache_nr(swp_entry_t entry, int nr)
 	bool any_only_cache = false;
 	unsigned long offset;
 
-	if (non_swap_entry(entry))
-		return;
-
 	si = get_swap_device(entry);
 	if (!si)
 		return;
-- 
2.41.0

[PATCH 10/12] mm/swap: rename swap_swapcount() to swap_entry_swapped()

[Baoquan He]

The new function name can reflect the real behaviour of the function
more clearly and more accurately. And the renaming avoids the confusion
between swap_swapcount() and swp_swapcount().

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 include/linux/swap.h | 6 +++---
 mm/swap_state.c      | 2 +-
 mm/swapfile.c        | 6 +++---
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/linux/swap.h b/include/linux/swap.h
index 20bfedc8c4f2..395d41cf9676 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -500,7 +500,7 @@ int find_first_swap(dev_t *device);
 extern unsigned int count_swap_pages(int, int);
 extern sector_t swapdev_block(int, pgoff_t);
 extern int __swap_count(swp_entry_t entry);
-extern int swap_swapcount(struct swap_info_struct *si, swp_entry_t entry);
+extern bool swap_entry_swapped(struct swap_info_struct *si, swp_entry_t entry);
 extern int swp_swapcount(swp_entry_t entry);
 struct swap_info_struct *swp_swap_info(swp_entry_t entry);
 struct backing_dev_info;
@@ -583,9 +583,9 @@ static inline int __swap_count(swp_entry_t entry)
 	return 0;
 }
 
-static inline int swap_swapcount(struct swap_info_struct *si, swp_entry_t entry)
+static inline bool swap_entry_swapped(struct swap_info_struct *si, swp_entry_t entry)
 {
-	return 0;
+	return false;
 }
 
 static inline int swp_swapcount(swp_entry_t entry)
diff --git a/mm/swap_state.c b/mm/swap_state.c
index b393cefb3be0..3dba6eb164db 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -457,7 +457,7 @@ struct folio *__read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
 		 * as SWAP_HAS_CACHE.  That's done in later part of code or
 		 * else swap_off will be aborted if we return NULL.
 		 */
-		if (!swap_swapcount(si, entry) && swap_slot_cache_enabled)
+		if (!swap_entry_swapped(si, entry) && swap_slot_cache_enabled)
 			goto put_and_return;
 
 		/*
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 2a25ff5f31c8..93fd12c12bba 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1615,7 +1615,7 @@ int __swap_count(swp_entry_t entry)
  * This does not give an exact answer when swap count is continued,
  * but does include the high COUNT_CONTINUED flag to allow for that.
  */
-int swap_swapcount(struct swap_info_struct *si, swp_entry_t entry)
+bool swap_entry_swapped(struct swap_info_struct *si, swp_entry_t entry)
 {
 	pgoff_t offset = swp_offset(entry);
 	struct swap_cluster_info *ci;
@@ -1624,7 +1624,7 @@ int swap_swapcount(struct swap_info_struct *si, swp_entry_t entry)
 	ci = lock_cluster(si, offset);
 	count = swap_count(si->swap_map[offset]);
 	unlock_cluster(ci);
-	return count;
+	return !!count;
 }
 
 /*
@@ -1710,7 +1710,7 @@ static bool folio_swapped(struct folio *folio)
 		return false;
 
 	if (!IS_ENABLED(CONFIG_THP_SWAP) || likely(!folio_test_large(folio)))
-		return swap_swapcount(si, entry) != 0;
+		return swap_entry_swapped(si, entry);
 
 	return swap_page_trans_huge_swapped(si, entry, folio_order(folio));
 }
-- 
2.41.0

[PATCH 11/12] mm/swapfile.c: remove the incorrect code comment

[Baoquan He]

Since commit eb085574a752 ("mm, swap: fix race between swapoff and
some swap operations"), the non_swap_entry() checking has been taken
off from function __swap_duplicate(). Hence, in the kernel-doc comment,
the line 'swp_entry is migration entry -> EINVAL' is obsolete. Remove
that line to avoid misleading people.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 mm/swapfile.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 93fd12c12bba..12bb7556c7f1 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -3525,7 +3525,6 @@ void si_swapinfo(struct sysinfo *val)
  * Returns error code in following case.
  * - success -> 0
  * - swp_entry is invalid -> EINVAL
- * - swp_entry is migration entry -> EINVAL
  * - swap-cache reference is requested but there is already one. -> EEXIST
  * - swap-cache reference is requested but the entry is not used. -> ENOENT
  * - swap-mapped reference requested but needs continued swap count. -> ENOMEM
-- 
2.41.0

[PATCH 12/12] mm/swapfile.c: open code cluster_alloc_swap()

[Baoquan He]

It's only called in scan_swap_map_slots().

And also remove the stale code comment in scan_swap_map_slots()
because it's not fit for the current cluster allocation mechanism.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 mm/swapfile.c | 38 ++++++++++----------------------------
 1 file changed, 10 insertions(+), 28 deletions(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 12bb7556c7f1..7448a387621d 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1158,39 +1158,13 @@ static void swap_range_free(struct swap_info_struct *si, unsigned long offset,
 	swap_usage_sub(si, nr_entries);
 }
 
-static int cluster_alloc_swap(struct swap_info_struct *si,
-			     unsigned char usage, int nr,
-			     swp_entry_t slots[], int order)
-{
-	int n_ret = 0;
-
-	while (n_ret < nr) {
-		unsigned long offset = cluster_alloc_swap_entry(si, order, usage);
-
-		if (!offset)
-			break;
-		slots[n_ret++] = swp_entry(si->type, offset);
-	}
-
-	return n_ret;
-}
-
 static int scan_swap_map_slots(struct swap_info_struct *si,
 			       unsigned char usage, int nr,
 			       swp_entry_t slots[], int order)
 {
 	unsigned int nr_pages = 1 << order;
+	int n_ret = 0;
 
-	/*
-	 * We try to cluster swap pages by allocating them sequentially
-	 * in swap.  Once we've allocated SWAPFILE_CLUSTER pages this
-	 * way, however, we resort to first-free allocation, starting
-	 * a new cluster.  This prevents us from scattering swap pages
-	 * all over the entire swap partition, so that we reduce
-	 * overall disk seek times between swap pages.  -- sct
-	 * But we do now try to find an empty cluster.  -Andrea
-	 * And we let swap pages go all over an SSD partition.  Hugh
-	 */
 	if (order > 0) {
 		/*
 		 * Should not even be attempting large allocations when huge
@@ -1210,7 +1184,15 @@ static int scan_swap_map_slots(struct swap_info_struct *si,
 			return 0;
 	}
 
-	return cluster_alloc_swap(si, usage, nr, slots, order);
+	while (n_ret < nr) {
+		unsigned long offset = cluster_alloc_swap_entry(si, order, usage);
+
+		if (!offset)
+			break;
+		slots[n_ret++] = swp_entry(si->type, offset);
+	}
+
+	return n_ret;
 }
 
 static bool get_swap_device_info(struct swap_info_struct *si)
-- 
2.41.0

Re: [PATCH 04/12] mm/swap: skip scanning cluster range if it's empty cluster

[Kairui Song]

On Wed, Feb 5, 2025 at 5:27 PM Baoquan He <bhe@redhat.com> wrote:
>
> Since ci->lock has been taken when isolating cluster from
> si->free_clusters or taking si->percpu_cluster->next[order],
> it's unnecessary to scan and check the cluster range availability
> if i'ts empty cluster, and this can accelerate the huge page
> swapping.
>
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  mm/swapfile.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 9c9a4ec6d4c6..61efde853eea 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -729,6 +729,9 @@ static bool cluster_scan_range(struct swap_info_struct *si,
>         unsigned long offset, end = start + nr_pages;
>         unsigned char *map = si->swap_map;
>
> +       if (cluster_is_empty(ci))
> +               return true;
> +

Hi Baoquan,

Thanks for the series.

Most commits are looking great, but this one is a bit questionable.
cluster_scan_range is only called by alloc_swap_scan_cluster, and it
already checks if the cluster has enough empty slots to use, so this
might be redundant.

It is possible that cluster_scan_range sees an empty cluster if the
cluster lock was dropped for reclaiming HAS_CACHE, but the chance
should be extremely low, that this might be a negative optimization.

Re: [PATCH 04/12] mm/swap: skip scanning cluster range if it's empty cluster

[Baoquan He]

On 02/06/25 at 01:07am, Kairui Song wrote:
> On Wed, Feb 5, 2025 at 5:27 PM Baoquan He <bhe@redhat.com> wrote:
> >
> > Since ci->lock has been taken when isolating cluster from
> > si->free_clusters or taking si->percpu_cluster->next[order],
> > it's unnecessary to scan and check the cluster range availability
> > if i'ts empty cluster, and this can accelerate the huge page
> > swapping.
> >
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > ---
> >  mm/swapfile.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/mm/swapfile.c b/mm/swapfile.c
> > index 9c9a4ec6d4c6..61efde853eea 100644
> > --- a/mm/swapfile.c
> > +++ b/mm/swapfile.c
> > @@ -729,6 +729,9 @@ static bool cluster_scan_range(struct swap_info_struct *si,
> >         unsigned long offset, end = start + nr_pages;
> >         unsigned char *map = si->swap_map;
> >
> > +       if (cluster_is_empty(ci))
> > +               return true;
> > +
> 
> Hi Baoquan,
> 
> Thanks for the series.

Thanks for your reviewing.

> 
> Most commits are looking great, but this one is a bit questionable.
> cluster_scan_range is only called by alloc_swap_scan_cluster, and it
> already checks if the cluster has enough empty slots to use, so this
> might be redundant.

Hmm, maybe no. Assume we want to allocate 2M space on system with 4K
page size. Even if a empty cluster is taken into consideration,
cluster_scan_range() will loop 512 times to check if each slot is
available. That for sure is not necessary in the case, while the added
empty cluster checking is very cheap.

> 
> It is possible that cluster_scan_range sees an empty cluster if the
> cluster lock was dropped for reclaiming HAS_CACHE, but the chance
> should be extremely low, that this might be a negative optimization.

It may be not like that. If it's empty cluster, the added checking will
return directly. Then 'need_reclaim' is kept false, there's no chance to
drop cluster lock to do reclaiming for HAS_CACHE. Means for empty
cluster scanning, the ci->lock is kept held. Not sure if I missed
anything.

Thanks
Baoquan

Re: [PATCH 04/12] mm/swap: skip scanning cluster range if it's empty cluster

[Kairui Song]

On Thu, Feb 6, 2025 at 9:41 AM Baoquan He <bhe@redhat.com> wrote:
>
> On 02/06/25 at 01:07am, Kairui Song wrote:
> > On Wed, Feb 5, 2025 at 5:27 PM Baoquan He <bhe@redhat.com> wrote:
> > >
> > > Since ci->lock has been taken when isolating cluster from
> > > si->free_clusters or taking si->percpu_cluster->next[order],
> > > it's unnecessary to scan and check the cluster range availability
> > > if i'ts empty cluster, and this can accelerate the huge page
> > > swapping.
> > >
> > > Signed-off-by: Baoquan He <bhe@redhat.com>
> > > ---
> > >  mm/swapfile.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/mm/swapfile.c b/mm/swapfile.c
> > > index 9c9a4ec6d4c6..61efde853eea 100644
> > > --- a/mm/swapfile.c
> > > +++ b/mm/swapfile.c
> > > @@ -729,6 +729,9 @@ static bool cluster_scan_range(struct swap_info_struct *si,
> > >         unsigned long offset, end = start + nr_pages;
> > >         unsigned char *map = si->swap_map;
> > >
> > > +       if (cluster_is_empty(ci))
> > > +               return true;
> > > +
> >
> > Hi Baoquan,
> >
> > Thanks for the series.
>
> Thanks for your reviewing.
>
> >
> > Most commits are looking great, but this one is a bit questionable.
> > cluster_scan_range is only called by alloc_swap_scan_cluster, and it
> > already checks if the cluster has enough empty slots to use, so this
> > might be redundant.
>
> Hmm, maybe no. Assume we want to allocate 2M space on system with 4K
> page size. Even if a empty cluster is taken into consideration,
> cluster_scan_range() will loop 512 times to check if each slot is
> available. That for sure is not necessary in the case, while the added
> empty cluster checking is very cheap.
>
> >
> > It is possible that cluster_scan_range sees an empty cluster if the
> > cluster lock was dropped for reclaiming HAS_CACHE, but the chance
> > should be extremely low, that this might be a negative optimization.
>
> It may be not like that. If it's empty cluster, the added checking will
> return directly. Then 'need_reclaim' is kept false, there's no chance to
> drop cluster lock to do reclaiming for HAS_CACHE. Means for empty
> cluster scanning, the ci->lock is kept held. Not sure if I missed
> anything.

Ah, right, sorry, I just understood your code wrongly. This makes
sense to me now.

>
> Thanks
> Baoquan
>

Re: [PATCH 06/12] mm/swapfile.c: update the code comment above swap_count_continued()

[Kairui Song]

On Wed, Feb 5, 2025 at 5:28 PM Baoquan He <bhe@redhat.com> wrote:
>
> Now, swap_count_continued() has two callers, __swap_duplicate() and
> __swap_entry_free_locked(), the relevant code comment is stale.
> Update it to reflect the current situation.
>
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  mm/swapfile.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index bf284ba16198..9ee2238042a5 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -3789,8 +3789,8 @@ int add_swap_count_continuation(swp_entry_t entry, gfp_t gfp_mask)
>   * into, carry if so, or else fail until a new continuation page is allocated;
>   * when the original swap_map count is decremented from 0 with continuation,
>   * borrow from the continuation and report whether it still holds more.
> - * Called while __swap_duplicate() or swap_entry_free() holds swap or cluster
> - * lock.
> + * Called while __swap_duplicate() or caller of __swap_entry_free_locked()
> + * holds swap or cluster lock.

It should be only "cluster lock" not "swap or cluster lock" now?

>   */
>  static bool swap_count_continued(struct swap_info_struct *si,
>                                  pgoff_t offset, unsigned char count)
> --
> 2.41.0
>

Re: [PATCH 06/12] mm/swapfile.c: update the code comment above swap_count_continued()

[Baoquan He]

On 02/06/25 at 03:50pm, Kairui Song wrote:
> On Wed, Feb 5, 2025 at 5:28 PM Baoquan He <bhe@redhat.com> wrote:
> >
> > Now, swap_count_continued() has two callers, __swap_duplicate() and
> > __swap_entry_free_locked(), the relevant code comment is stale.
> > Update it to reflect the current situation.
> >
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > ---
> >  mm/swapfile.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/swapfile.c b/mm/swapfile.c
> > index bf284ba16198..9ee2238042a5 100644
> > --- a/mm/swapfile.c
> > +++ b/mm/swapfile.c
> > @@ -3789,8 +3789,8 @@ int add_swap_count_continuation(swp_entry_t entry, gfp_t gfp_mask)
> >   * into, carry if so, or else fail until a new continuation page is allocated;
> >   * when the original swap_map count is decremented from 0 with continuation,
> >   * borrow from the continuation and report whether it still holds more.
> > - * Called while __swap_duplicate() or swap_entry_free() holds swap or cluster
> > - * lock.
> > + * Called while __swap_duplicate() or caller of __swap_entry_free_locked()
> > + * holds swap or cluster lock.
> 
> It should be only "cluster lock" not "swap or cluster lock" now?

You are right, I will update.

> 
> >   */
> >  static bool swap_count_continued(struct swap_info_struct *si,
> >                                  pgoff_t offset, unsigned char count)
> > --
> > 2.41.0
> >
> 

[PATCH v2 06/12] mm/swapfile.c: update the code comment above swap_count_continued()

[Baoquan He]

Now, swap_count_continued() has two callers, __swap_duplicate() and
__swap_entry_free_locked(), the relevant code comment is stale.
Update it to reflect the current situation.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
v1->v2:
- Update the code comment according to Kairui's comment.

 mm/swapfile.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index bf284ba16198..f6cc169f59d9 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -3789,8 +3789,8 @@ int add_swap_count_continuation(swp_entry_t entry, gfp_t gfp_mask)
  * into, carry if so, or else fail until a new continuation page is allocated;
  * when the original swap_map count is decremented from 0 with continuation,
  * borrow from the continuation and report whether it still holds more.
- * Called while __swap_duplicate() or swap_entry_free() holds swap or cluster
- * lock.
+ * Called while __swap_duplicate() or caller of __swap_entry_free_locked()
+ * holds cluster lock.
  */
 static bool swap_count_continued(struct swap_info_struct *si,
 				 pgoff_t offset, unsigned char count)
-- 
2.41.0

Re: [PATCH 00/12] Tiny cleanup and improvements about SWAP code

[Kairui Song]

On Wed, Feb 5, 2025 at 5:27 PM Baoquan He <bhe@redhat.com> wrote:
>
> These are all made during reviewing and learning below patchset from
> Kairui.

Thanks, I've noticed some obsolete comments and code, currently
refactoring many parts so some of the functions would be just gone
someday.
But this surely cleans things up and is good to have, refactoring will
take much longer time to happen.

With the V2 update of "mm/swapfile.c: update the code comment above
swap_count_continued()":

Reviewed-by: Kairui Song <kasong@tencent.com>

> [PATCH v3 00/13] mm, swap: rework of swap allocator locks

BTW, I noticed most patches here are patching legacy code, and not
directly related to that series.
Just to clarify, so people won't need to worry about missing "Fixes:"
or things like that :)

Re: [PATCH 00/12] Tiny cleanup and improvements about SWAP code

[Baoquan He]

On 02/07/25 at 05:36pm, Kairui Song wrote:
> On Wed, Feb 5, 2025 at 5:27 PM Baoquan He <bhe@redhat.com> wrote:
> >
> > These are all made during reviewing and learning below patchset from
> > Kairui.
> 
> Thanks, I've noticed some obsolete comments and code, currently
> refactoring many parts so some of the functions would be just gone
> someday.

Thanks a lot for reviewing, Kairui.

I have a queued patchset to refactor the old hdd/cluster code, it could
be made about 2 years ago. Then I heard you and Chris's presentation in
LPC in last fall, and your later rework patchset, just began to notice
the big change in swap code which is not what I knew. Sorry, didn't know
you are still refactoring the code, so will stop touching swap code, look
forward to seeing your new refactoring patches. I will keep an eye so
that I can follow the step to update my knowledge about swap code.

> But this surely cleans things up and is good to have, refactoring will
> take much longer time to happen.

Thanks again. Please help add me to CC when you post, I am interested in
the new change.

> 
> With the V2 update of "mm/swapfile.c: update the code comment above
> swap_count_continued()":
> 
> Reviewed-by: Kairui Song <kasong@tencent.com>
> 
> > [PATCH v3 00/13] mm, swap: rework of swap allocator locks
> 
> BTW, I noticed most patches here are patching legacy code, and not
> directly related to that series.
> Just to clarify, so people won't need to worry about missing "Fixes:"
> or things like that :)

LOL, right, at least people following the recent change of swap won't
misunderstand.