swapin refactor for optimization and unified readahead

From: Kairui Song <[email protected]>

No feature change, simply move the routine to a standalone function to be re-used later. The error path handling is copied from the "out_page" label, to make the code change minimized for easier reviewing.

Signed-off-by Kairui Song <[email protected]>

---
 mm/memory.c     | 32 ++++----------------------------
 mm/swap.h       |  8 ++++++++
 mm/swap_state.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 28 deletions(-)

mm/memory.c +4 -28

index 7e1f4849463a..81dc9d467f4e 100644
@@ -3803,7 +3803,6 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
 	swp_entry_t entry;
 	pte_t pte;
 	vm_fault_t ret = 0;
-	void *shadow = NULL;
 
 	if (!pte_unmap_same(vmf))
 		goto out;

@@ -3867,33 +3866,10 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
 	if (!folio) {
 		if (data_race(si->flags & SWP_SYNCHRONOUS_IO) &&
 		    __swap_count(entry) == 1) {
-			/* skip swapcache */
-			folio = vma_alloc_folio(GFP_HIGHUSER_MOVABLE, 0,
-						vma, vmf->address, false);
-			page = &folio->page;
-			if (folio) {
-				__folio_set_locked(folio);
-				__folio_set_swapbacked(folio);
-
-				if (mem_cgroup_swapin_charge_folio(folio,
-							vma->vm_mm, GFP_KERNEL,
-							entry)) {
-					ret = VM_FAULT_OOM;
-					goto out_page;
-				}
-				mem_cgroup_swapin_uncharge_swap(entry);
-
-				shadow = get_shadow_from_swap_cache(entry);
-				if (shadow)
-					workingset_refault(folio, shadow);
-
-				folio_add_lru(folio);
-
-				/* To provide entry to swap_read_folio() */
-				folio->swap = entry;
-				swap_read_folio(folio, true, NULL);
-				folio->private = NULL;
-			}
+			/* skip swapcache and readahead */
+			folio = swapin_direct(entry, GFP_HIGHUSER_MOVABLE, vmf);
+			if (folio)
+				page = &folio->page;
 		} else {
 			page = swapin_readahead(entry, GFP_HIGHUSER_MOVABLE,
 						vmf);

mm/swap.h +8 -0

index 758c46ca671e..83eab7b67e77 100644
@@ -56,6 +56,8 @@ struct folio *swap_cluster_readahead(swp_entry_t entry, gfp_t flag,
 		struct mempolicy *mpol, pgoff_t ilx);
 struct page *swapin_readahead(swp_entry_t entry, gfp_t flag,
 			      struct vm_fault *vmf);
+struct folio *swapin_direct(swp_entry_t entry, gfp_t flag,
+			    struct vm_fault *vmf);
 
 static inline unsigned int folio_swap_flags(struct folio *folio)
 {

@@ -86,6 +88,12 @@ static inline struct folio *swap_cluster_readahead(swp_entry_t entry,
 	return NULL;
 }
 
+struct folio *swapin_direct(swp_entry_t entry, gfp_t flag,
+			struct vm_fault *vmf)
+{
+	return NULL;
+}
+
 static inline struct page *swapin_readahead(swp_entry_t swp, gfp_t gfp_mask,
 			struct vm_fault *vmf)
 {

mm/swap_state.c +47 -0

index e671266ad772..645f5bcad123 100644
@@ -861,6 +861,53 @@ static struct folio *swap_vma_readahead(swp_entry_t targ_entry, gfp_t gfp_mask,
 	return folio;
 }
 
+/**
+ * swapin_direct - swap in a folio skipping swap cache and readahead
+ * @entry: swap entry of this memory
+ * @gfp_mask: memory allocation flags
+ * @vmf: fault information
+ *
+ * Returns the struct folio for entry and addr after the swap entry is read
+ * in.
+ */
+struct folio *swapin_direct(swp_entry_t entry, gfp_t gfp_mask,
+			    struct vm_fault *vmf)
+{
+	struct vm_area_struct *vma = vmf->vma;
+	struct folio *folio;
+	void *shadow = NULL;
+
+	/* skip swapcache */
+	folio = vma_alloc_folio(gfp_mask, 0,
+				vma, vmf->address, false);
+	if (folio) {
+		__folio_set_locked(folio);
+		__folio_set_swapbacked(folio);
+
+		if (mem_cgroup_swapin_charge_folio(folio,
+					vma->vm_mm, GFP_KERNEL,
+					entry)) {
+			folio_unlock(folio);
+			folio_put(folio);
+			return NULL;
+		}
+		mem_cgroup_swapin_uncharge_swap(entry);
+
+		shadow = get_shadow_from_swap_cache(entry);
+		if (shadow)
+			workingset_refault(folio, shadow);
+
+		folio_add_lru(folio);
+
+		/* To provide entry to swap_read_folio() */
+		folio->swap = entry;
+		swap_read_folio(folio, true, NULL);
+		folio->private = NULL;
+	}
+
+	return folio;
+}
+
 /**
  * swapin_readahead - swap in pages in hope we need them soon
  * @entry: swap entry of this memory

Quoted 2 lines · Kairui Song

From: Kairui Song <[email protected]>

Quoted 3 lines

No feature change, simply move the routine to a standalone function to be re-used later. The error path handling is copied from the "out_page" label, to make the code change minimized for easier reviewing.

The error processing for mem_cgroup_swapin_charge_folio() failure is changed a little. That looks OK for me. But you need to make it explicit in change log. Especially, it's not "no feature change" strictly.

--
Best Regards,
Huang, Ying

From: Kairui Song <[email protected]>

Currently, mem_cgroup_swapin_charge_folio is always called with mm == NULL, except in swapin_direct.

swapin_direct is only used when swapin should skip readahead and swapcache (SWP_SYNCHRONOUS_IO). All other callers of mem_cgroup_swapin_charge_folio are for swapin that should not skip readahead and cache.

This could cause swapin charging to behave differently depending on swap device, which is unexpected.

This is currently not happening because the only caller of swapin_direct is the direct anon page fault path, where mm always equals to current->mm, but will no longer be true if swapin_direct is shared and have other callers (eg, swapoff) to share the readahead skipping logic.

So make swapin_direct also pass NULL for mm, so swpain charge will behave consistently and not effected by type of swapin device or readahead policy.

After this, the second param of mem_cgroup_swapin_charge_folio is never used now, so it can be safely dropped.

Signed-off-by Kairui Song <[email protected]>

---
 include/linux/memcontrol.h | 4 ++--
 mm/memcontrol.c            | 5 ++---
 mm/swap_state.c            | 7 +++----
 3 files changed, 7 insertions(+), 9 deletions(-)

include/linux/memcontrol.h +2 -2

index 20ff87f8e001..540590d80958 100644
@@ -693,7 +693,7 @@ static inline int mem_cgroup_charge(struct folio *folio, struct mm_struct *mm,
 int mem_cgroup_hugetlb_try_charge(struct mem_cgroup *memcg, gfp_t gfp,
 		long nr_pages);
 
-int mem_cgroup_swapin_charge_folio(struct folio *folio, struct mm_struct *mm,
+int mem_cgroup_swapin_charge_folio(struct folio *folio,
 				  gfp_t gfp, swp_entry_t entry);
 void mem_cgroup_swapin_uncharge_swap(swp_entry_t entry);
 

@@ -1281,7 +1281,7 @@ static inline int mem_cgroup_hugetlb_try_charge(struct mem_cgroup *memcg,
 }
 
 static inline int mem_cgroup_swapin_charge_folio(struct folio *folio,
-			struct mm_struct *mm, gfp_t gfp, swp_entry_t entry)
+		gfp_t gfp, swp_entry_t entry)
 {
 	return 0;
 }

mm/memcontrol.c +1 -2

index e4c8735e7c85..5852742df958 100644
@@ -7306,8 +7306,7 @@ int mem_cgroup_hugetlb_try_charge(struct mem_cgroup *memcg, gfp_t gfp,
  *
  * Returns 0 on success. Otherwise, an error code is returned.
  */
-int mem_cgroup_swapin_charge_folio(struct folio *folio, struct mm_struct *mm,
-				  gfp_t gfp, swp_entry_t entry)
+int mem_cgroup_swapin_charge_folio(struct folio *folio, gfp_t gfp, swp_entry_t entry)
 {
 	struct mem_cgroup *memcg;
 	unsigned short id;

mm/memcontrol.c +0 -0

 	rcu_read_unlock();
 
 	ret = charge_memcg(folio, memcg, gfp);

mm/swap_state.c +3 -4

index 645f5bcad123..a450d09fc0db 100644
@@ -495,7 +495,7 @@ struct folio *__read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
 	__folio_set_locked(folio);
 	__folio_set_swapbacked(folio);
 
-	if (mem_cgroup_swapin_charge_folio(folio, NULL, gfp_mask, entry))
+	if (mem_cgroup_swapin_charge_folio(folio, gfp_mask, entry))
 		goto fail_unlock;
 
 	/* May fail (-ENOMEM) if XArray node allocation failed. */

@@ -884,9 +884,8 @@ struct folio *swapin_direct(swp_entry_t entry, gfp_t gfp_mask,
 		__folio_set_locked(folio);
 		__folio_set_swapbacked(folio);
 
-		if (mem_cgroup_swapin_charge_folio(folio,
-					vma->vm_mm, GFP_KERNEL,
-					entry)) {
+		if (mem_cgroup_swapin_charge_folio(folio, GFP_KERNEL,
+						   entry)) {
 			folio_unlock(folio);
 			folio_put(folio);
 			return NULL;

Quoted 71 lines · Kairui Song

From: Kairui Song <[email protected]>

Currently, mem_cgroup_swapin_charge_folio is always called with mm == NULL, except in swapin_direct.

swapin_direct is only used when swapin should skip readahead and swapcache (SWP_SYNCHRONOUS_IO). All other callers of mem_cgroup_swapin_charge_folio are for swapin that should not skip readahead and cache.

This could cause swapin charging to behave differently depending on swap device, which is unexpected.

This is currently not happening because the only caller of swapin_direct is the direct anon page fault path, where mm always equals to current->mm, but will no longer be true if swapin_direct is shared and have other callers (eg, swapoff) to share the readahead skipping logic.

So make swapin_direct also pass NULL for mm, so swpain charge will behave consistently and not effected by type of swapin device or readahead policy.

After this, the second param of mem_cgroup_swapin_charge_folio is never used now, so it can be safely dropped.

Signed-off-by Kairui Song <[email protected]>

---
 include/linux/memcontrol.h | 4 ++--
 mm/memcontrol.c            | 5 ++---
 mm/swap_state.c            | 7 +++----
 3 files changed, 7 insertions(+), 9 deletions(-)

include/linux/memcontrol.h +2 -2

index 20ff87f8e001..540590d80958 100644
@@ -693,7 +693,7 @@ static inline int mem_cgroup_charge(struct folio *folio, struct mm_struct *mm,
 int mem_cgroup_hugetlb_try_charge(struct mem_cgroup *memcg, gfp_t gfp,
              long nr_pages);
 
-int mem_cgroup_swapin_charge_folio(struct folio *folio, struct mm_struct *mm,
+int mem_cgroup_swapin_charge_folio(struct folio *folio,
                                gfp_t gfp, swp_entry_t entry);
 void mem_cgroup_swapin_uncharge_swap(swp_entry_t entry);
 

@@ -1281,7 +1281,7 @@ static inline int mem_cgroup_hugetlb_try_charge(struct mem_cgroup *memcg,
 }
 
 static inline int mem_cgroup_swapin_charge_folio(struct folio *folio,
-                     struct mm_struct *mm, gfp_t gfp, swp_entry_t entry)
+             gfp_t gfp, swp_entry_t entry)
 {
      return 0;
 }

mm/memcontrol.c +1 -2

index e4c8735e7c85..5852742df958 100644
@@ -7306,8 +7306,7 @@ int mem_cgroup_hugetlb_try_charge(struct mem_cgroup *memcg, gfp_t gfp,
  *
  * Returns 0 on success. Otherwise, an error code is returned.
  */
-int mem_cgroup_swapin_charge_folio(struct folio *folio, struct mm_struct *mm,
-                               gfp_t gfp, swp_entry_t entry)
+int mem_cgroup_swapin_charge_folio(struct folio *folio, gfp_t gfp, swp_entry_t entry)
 {
      struct mem_cgroup *memcg;
      unsigned short id;

@@ -7320,7 +7319,7 @@ int mem_cgroup_swapin_charge_folio(struct folio *folio, struct mm_struct *mm,
      rcu_read_lock();

mm/memcontrol.c int mem_cgroup_swapin_charge_folio(struct folio *folio, struct mm_struct *mm, · line 7322

Quoted 4 lines

mm/memcontrol.c +1 -1

      memcg = mem_cgroup_from_id(id);
      if (!memcg || !css_tryget_online(&memcg->css))
-             memcg = get_mem_cgroup_from_mm(mm);
+             memcg = get_mem_cgroup_from_current();

The behavior of get_mem_cgroup_from_mm(NULL) and get_mem_cgroup_from_current() isn't same exactly. Are you sure that this is OK?

--
Best Regards,
Huang, Ying

Quoted 74 lines · Ying

Quoted 71 lines · Kairui Song

From: Kairui Song <[email protected]>

Currently, mem_cgroup_swapin_charge_folio is always called with mm == NULL, except in swapin_direct.

swapin_direct is only used when swapin should skip readahead and swapcache (SWP_SYNCHRONOUS_IO). All other callers of mem_cgroup_swapin_charge_folio are for swapin that should not skip readahead and cache.

This could cause swapin charging to behave differently depending on swap device, which is unexpected.

This is currently not happening because the only caller of swapin_direct is the direct anon page fault path, where mm always equals to current->mm, but will no longer be true if swapin_direct is shared and have other callers (eg, swapoff) to share the readahead skipping logic.

So make swapin_direct also pass NULL for mm, so swpain charge will behave consistently and not effected by type of swapin device or readahead policy.

After this, the second param of mem_cgroup_swapin_charge_folio is never used now, so it can be safely dropped.

Signed-off-by Kairui Song <[email protected]>

---
 include/linux/memcontrol.h | 4 ++--
 mm/memcontrol.c            | 5 ++---
 mm/swap_state.c            | 7 +++----
 3 files changed, 7 insertions(+), 9 deletions(-)

include/linux/memcontrol.h +2 -2

index 20ff87f8e001..540590d80958 100644
@@ -693,7 +693,7 @@ static inline int mem_cgroup_charge(struct folio *folio, struct mm_struct *mm,
 int mem_cgroup_hugetlb_try_charge(struct mem_cgroup *memcg, gfp_t gfp,
              long nr_pages);
 
-int mem_cgroup_swapin_charge_folio(struct folio *folio, struct mm_struct *mm,
+int mem_cgroup_swapin_charge_folio(struct folio *folio,
                                gfp_t gfp, swp_entry_t entry);
 void mem_cgroup_swapin_uncharge_swap(swp_entry_t entry);
 

@@ -1281,7 +1281,7 @@ static inline int mem_cgroup_hugetlb_try_charge(struct mem_cgroup *memcg,
 }
 
 static inline int mem_cgroup_swapin_charge_folio(struct folio *folio,
-                     struct mm_struct *mm, gfp_t gfp, swp_entry_t entry)
+             gfp_t gfp, swp_entry_t entry)
 {
      return 0;
 }

mm/memcontrol.c +1 -2

index e4c8735e7c85..5852742df958 100644
@@ -7306,8 +7306,7 @@ int mem_cgroup_hugetlb_try_charge(struct mem_cgroup *memcg, gfp_t gfp,
  *
  * Returns 0 on success. Otherwise, an error code is returned.
  */
-int mem_cgroup_swapin_charge_folio(struct folio *folio, struct mm_struct *mm,
-                               gfp_t gfp, swp_entry_t entry)
+int mem_cgroup_swapin_charge_folio(struct folio *folio, gfp_t gfp, swp_entry_t entry)
 {
      struct mem_cgroup *memcg;
      unsigned short id;

@@ -7320,7 +7319,7 @@ int mem_cgroup_swapin_charge_folio(struct folio *folio, struct mm_struct *mm,
      rcu_read_lock();

Quoted 8 lines

mm/memcontrol.c int mem_cgroup_swapin_charge_folio(struct folio *folio, struct mm_struct *mm, · line 7322

Quoted 4 lines

mm/memcontrol.c +1 -1

      memcg = mem_cgroup_from_id(id);
      if (!memcg || !css_tryget_online(&memcg->css))
-             memcg = get_mem_cgroup_from_mm(mm);
+             memcg = get_mem_cgroup_from_current();

The behavior of get_mem_cgroup_from_mm(NULL) and get_mem_cgroup_from_current() isn't same exactly. Are you sure that this is OK?

Hi Ying, thank you very much for the careful review.

IIUC, usually get_mem_cgroup_from_mm(NULL) is for allocations without mm context (after set_active_memcg), so remote charging cgroup is used first.

But for swap cases it's a bit special, all swapin are issued from userspace, so remote charging isn't useful. Not sure if it even may potentially lead to charging into the wrong cgroup.

And for this callsite, it's called only when if (!memcg || !css_tryget_online(&memcg->css)) is true, only case I know is swapoff (and the memcg is dead) case, or there are some leaks. The behaviour

of swapoff case has been discussed previously, so currently we just charge it into the current task's memcg.

This is indeed a potential behaviour change though, I can change it back to get_mem_cgroup_from_mm(NULL), and post another patch later for this and discuss in more details.

Quoted 4 lines

--
Best Regards,
Huang, Ying

From: Kairui Song <[email protected]>

Introduce swapin_entry which merges swapin_readahead and swapin_direct making it the main entry for swapin pages, and use a unified swapin readahead policy.

This commit makes swapoff make use of this new helper and skip readahead for SYNCHRONOUS_IO device since it's not helpful here. Now swapping off a 10G ZRAM (lzo-rle) after same workload is faster since readahead is skipped and overhead is reduced.

Before:
time swapoff /dev/zram0

real    0m12.337s
user    0m0.001s
sys     0m12.329s

After:
time swapoff /dev/zram0

real    0m9.728s
user    0m0.001s
sys     0m9.719s

Signed-off-by Kairui Song <[email protected]>

---
 mm/memory.c     | 18 +++---------------
 mm/swap.h       | 16 ++++------------
 mm/swap_state.c | 40 ++++++++++++++++++++++++----------------
 mm/swapfile.c   |  7 ++-----
 4 files changed, 33 insertions(+), 48 deletions(-)

mm/memory.c +2 -14

index 81dc9d467f4e..8711f8a07039 100644
@@ -3864,20 +3864,8 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
 	swapcache = folio;
 
 	if (!folio) {
-		if (data_race(si->flags & SWP_SYNCHRONOUS_IO) &&
-		    __swap_count(entry) == 1) {
-			/* skip swapcache and readahead */
-			folio = swapin_direct(entry, GFP_HIGHUSER_MOVABLE, vmf);
-			if (folio)
-				page = &folio->page;
-		} else {
-			page = swapin_readahead(entry, GFP_HIGHUSER_MOVABLE,
-						vmf);
-			if (page)
-				folio = page_folio(page);
-			swapcache = folio;
-		}
-
+		folio = swapin_entry(entry, GFP_HIGHUSER_MOVABLE,
+				     vmf, &swapcache);
 		if (!folio) {
 			/*
 			 * Back out if somebody else faulted in this pte

mm/memory.c +0 -0

 	} else if (PageHWPoison(page)) {
 		/*
 		 * hwpoisoned dirty swapcache pages are kept for killing

mm/swap.h +4 -12

index 83eab7b67e77..8f8185d3865c 100644
@@ -54,10 +54,8 @@ struct folio *__read_swap_cache_async(swp_entry_t entry, gfp_t gfp_flags,
 		bool skip_if_exists);
 struct folio *swap_cluster_readahead(swp_entry_t entry, gfp_t flag,
 		struct mempolicy *mpol, pgoff_t ilx);
-struct page *swapin_readahead(swp_entry_t entry, gfp_t flag,
-			      struct vm_fault *vmf);
-struct folio *swapin_direct(swp_entry_t entry, gfp_t flag,
-			    struct vm_fault *vmf);
+struct folio *swapin_entry(swp_entry_t entry, gfp_t flag,
+			   struct vm_fault *vmf, struct folio **swapcached);
 
 static inline unsigned int folio_swap_flags(struct folio *folio)
 {

@@ -88,14 +86,8 @@ static inline struct folio *swap_cluster_readahead(swp_entry_t entry,
 	return NULL;
 }
 
-struct folio *swapin_direct(swp_entry_t entry, gfp_t flag,
-			struct vm_fault *vmf)
-{
-	return NULL;
-}
-
-static inline struct page *swapin_readahead(swp_entry_t swp, gfp_t gfp_mask,
-			struct vm_fault *vmf)
+static inline struct folio *swapin_entry(swp_entry_t swp, gfp_t gfp_mask,
+			struct vm_fault *vmf, struct folio **swapcached)
 {
 	return NULL;
 }

mm/swap_state.c +7 -5

index a450d09fc0db..5e06b2e140d4 100644
@@ -870,8 +870,8 @@ static struct folio *swap_vma_readahead(swp_entry_t targ_entry, gfp_t gfp_mask,
  * Returns the struct folio for entry and addr after the swap entry is read
  * in.
  */
-struct folio *swapin_direct(swp_entry_t entry, gfp_t gfp_mask,
-			    struct vm_fault *vmf)
+static struct folio *swapin_direct(swp_entry_t entry, gfp_t gfp_mask,
+				  struct vm_fault *vmf)
 {
 	struct vm_area_struct *vma = vmf->vma;
 	struct folio *folio;

@@ -908,33 +908,41 @@ struct folio *swapin_direct(swp_entry_t entry, gfp_t gfp_mask,
 }
 
 /**
- * swapin_readahead - swap in pages in hope we need them soon
+ * swapin_entry - swap in a folio from swap entry
  * @entry: swap entry of this memory
  * @gfp_mask: memory allocation flags
  * @vmf: fault information
+ * @swapcache: set to the swapcache folio if swapcache is used
  *
  * Returns the struct page for entry and addr, after queueing swapin.
  *
- * It's a main entry function for swap readahead. By the configuration,
+ * It's the main entry function for swap in. By the configuration,
  * it will read ahead blocks by cluster-based(ie, physical disk based)
- * or vma-based(ie, virtual address based on faulty address) readahead.
+ * or vma-based(ie, virtual address based on faulty address) readahead,
+ * or skip the readahead(ie, ramdisk based swap device).

mm/swap_state.c +14 -8

-	mpol = get_vma_policy(vmf->vma, vmf->address, 0, &ilx);
-	folio = swap_use_vma_readahead() ?
-		swap_vma_readahead(entry, gfp_mask, mpol, ilx, vmf) :
-		swap_cluster_readahead(entry, gfp_mask, mpol, ilx);
-	mpol_cond_put(mpol);
+	if (data_race(swp_swap_info(entry)->flags & SWP_SYNCHRONOUS_IO) &&
+	    __swap_count(entry) == 1) {
+		folio = swapin_direct(entry, gfp_mask, vmf);
+	} else {
+		mpol = get_vma_policy(vmf->vma, vmf->address, 0, &ilx);
+		if (swap_use_vma_readahead())
+			folio = swap_vma_readahead(entry, gfp_mask, mpol, ilx, vmf);
+		else
+			folio = swap_cluster_readahead(entry, gfp_mask, mpol, ilx);
+		mpol_cond_put(mpol);
+		if (swapcache)
+			*swapcache = folio;
+	}
 
-	if (!folio)
-		return NULL;
-	return folio_file_page(folio, swp_offset(entry));
+	return folio;
 }
 
 #ifdef CONFIG_SYSFS

mm/swapfile.c +2 -5

index 606d95b56304..1cf7e72e19e3 100644
@@ -1867,7 +1867,6 @@ static int unuse_pte_range(struct vm_area_struct *vma, pmd_t *pmd,
 
 		folio = swap_cache_get_folio(entry, vma, addr);
 		if (!folio) {
-			struct page *page;
 			struct vm_fault vmf = {
 				.vma = vma,
 				.address = addr,

@@ -1875,10 +1874,8 @@ static int unuse_pte_range(struct vm_area_struct *vma, pmd_t *pmd,
 				.pmd = pmd,
 			};
 
-			page = swapin_readahead(entry, GFP_HIGHUSER_MOVABLE,
-						&vmf);
-			if (page)
-				folio = page_folio(page);
+			folio = swapin_entry(entry, GFP_HIGHUSER_MOVABLE,
+					    &vmf, NULL);
 		}
 		if (!folio) {
 			/*

From: Kairui Song <[email protected]>

When a xa_value is returned by the cache lookup, keep it to be used later for workingset refault check instead of doing the looking up again in swapin_no_readahead.

Shadow look up and workingset check is skipped for swapoff to reduce overhead, workingset checking for anon pages upon swapoff is not helpful, simply consider all pages as inactive make more sense since swapoff doesn't mean pages is being accessed.

After this commit, swappin is about 4% faster for ZRAM, micro benchmark result which use madvise to swap out 10G zero-filled data to ZRAM then read them in:

Before: 11143285 us
After:  10692644 us (+4.1%)

Signed-off-by Kairui Song <[email protected]>

---
 mm/memory.c     |  5 +++--
 mm/shmem.c      |  2 +-
 mm/swap.h       | 11 ++++++-----
 mm/swap_state.c | 23 +++++++++++++----------
 mm/swapfile.c   |  4 ++--
 5 files changed, 25 insertions(+), 20 deletions(-)

mm/memory.c +3 -2

index 8711f8a07039..349946899f8d 100644
@@ -3800,6 +3800,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
 	struct swap_info_struct *si = NULL;
 	rmap_t rmap_flags = RMAP_NONE;
 	bool exclusive = false;
+	void *shadow = NULL;
 	swp_entry_t entry;
 	pte_t pte;
 	vm_fault_t ret = 0;

@@ -3858,14 +3859,14 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
 	if (unlikely(!si))
 		goto out;
 
-	folio = swap_cache_get_folio(entry, vma, vmf->address);
+	folio = swap_cache_get_folio(entry, vma, vmf->address, &shadow);
 	if (folio)
 		page = folio_file_page(folio, swp_offset(entry));
 	swapcache = folio;
 
 	if (!folio) {
 		folio = swapin_entry(entry, GFP_HIGHUSER_MOVABLE,
-				     vmf, &swapcache);
+				     vmf, &swapcache, shadow);
 		if (!folio) {
 			/*
 			 * Back out if somebody else faulted in this pte

mm/shmem.c +1 -1

index d7c84ff62186..698a31bf7baa 100644
@@ -1873,7 +1873,7 @@ static int shmem_swapin_folio(struct inode *inode, pgoff_t index,
 	}
 
 	/* Look it up and read it in.. */
-	folio = swap_cache_get_folio(swap, NULL, 0);
+	folio = swap_cache_get_folio(swap, NULL, 0, NULL);
 	if (!folio) {
 		/* Or update major stats only when swapin succeeds?? */
 		if (fault_type) {

mm/swap.h +6 -5

index 8f8185d3865c..ca9cb472a263 100644
@@ -42,7 +42,8 @@ void delete_from_swap_cache(struct folio *folio);
 void clear_shadow_from_swap_cache(int type, unsigned long begin,
 				  unsigned long end);
 struct folio *swap_cache_get_folio(swp_entry_t entry,
-		struct vm_area_struct *vma, unsigned long addr);
+		struct vm_area_struct *vma, unsigned long addr,
+		void **shadowp);
 struct folio *filemap_get_incore_folio(struct address_space *mapping,
 		pgoff_t index);
 

@@ -54,8 +55,8 @@ struct folio *__read_swap_cache_async(swp_entry_t entry, gfp_t gfp_flags,
 		bool skip_if_exists);
 struct folio *swap_cluster_readahead(swp_entry_t entry, gfp_t flag,
 		struct mempolicy *mpol, pgoff_t ilx);
-struct folio *swapin_entry(swp_entry_t entry, gfp_t flag,
-			   struct vm_fault *vmf, struct folio **swapcached);
+struct folio *swapin_entry(swp_entry_t entry, gfp_t flag, struct vm_fault *vmf,
+		struct folio **swapcached, void *shadow);
 
 static inline unsigned int folio_swap_flags(struct folio *folio)
 {

@@ -87,7 +88,7 @@ static inline struct folio *swap_cluster_readahead(swp_entry_t entry,
 }
 
 static inline struct folio *swapin_entry(swp_entry_t swp, gfp_t gfp_mask,
-			struct vm_fault *vmf, struct folio **swapcached)
+			struct vm_fault *vmf, struct folio **swapcached, void *shadow)
 {
 	return NULL;
 }

@@ -98,7 +99,7 @@ static inline int swap_writepage(struct page *p, struct writeback_control *wbc)
 }
 
 static inline struct folio *swap_cache_get_folio(swp_entry_t entry,
-		struct vm_area_struct *vma, unsigned long addr)
+		struct vm_area_struct *vma, unsigned long addr, void **shadowp)
 {
 	return NULL;
 }

mm/swap_state.c +13 -10

index 5e06b2e140d4..e41a137a6123 100644
@@ -330,12 +330,18 @@ static inline bool swap_use_vma_readahead(void)
  * Caller must lock the swap device or hold a reference to keep it valid.
  */
 struct folio *swap_cache_get_folio(swp_entry_t entry,
-		struct vm_area_struct *vma, unsigned long addr)
+		struct vm_area_struct *vma, unsigned long addr, void **shadowp)
 {
 	struct folio *folio;
 
-	folio = filemap_get_folio(swap_address_space(entry), swp_offset(entry));
-	if (!IS_ERR(folio)) {
+	folio = filemap_get_entry(swap_address_space(entry), swp_offset(entry));
+	if (xa_is_value(folio)) {
+		if (shadowp)
+			*shadowp = folio;
+		return NULL;
+	}
+
+	if (folio) {
 		bool vma_ra = swap_use_vma_readahead();
 		bool readahead;
 

@@ -365,8 +371,6 @@ struct folio *swap_cache_get_folio(swp_entry_t entry,
 			if (!vma || !vma_ra)
 				atomic_inc(&swapin_readahead_hits);
 		}
-	} else {
-		folio = NULL;
 	}
 
 	return folio;

@@ -866,16 +870,16 @@ static struct folio *swap_vma_readahead(swp_entry_t targ_entry, gfp_t gfp_mask,
  * @entry: swap entry of this memory
  * @gfp_mask: memory allocation flags
  * @vmf: fault information
+ * @shadow: workingset shadow corresponding to entry
  *
  * Returns the struct folio for entry and addr after the swap entry is read
  * in.
  */
 static struct folio *swapin_direct(swp_entry_t entry, gfp_t gfp_mask,
-				  struct vm_fault *vmf)
+				  struct vm_fault *vmf, void *shadow)
 {
 	struct vm_area_struct *vma = vmf->vma;
 	struct folio *folio;
-	void *shadow = NULL;
 
 	/* skip swapcache */
 	folio = vma_alloc_folio(gfp_mask, 0,

@@ -892,7 +896,6 @@ static struct folio *swapin_direct(swp_entry_t entry, gfp_t gfp_mask,
 		}
 		mem_cgroup_swapin_uncharge_swap(entry);
 
-		shadow = get_shadow_from_swap_cache(entry);
 		if (shadow)
 			workingset_refault(folio, shadow);
 

@@ -922,7 +925,7 @@ static struct folio *swapin_direct(swp_entry_t entry, gfp_t gfp_mask,
  * or skip the readahead(ie, ramdisk based swap device).
  */
 struct folio *swapin_entry(swp_entry_t entry, gfp_t gfp_mask,
-			   struct vm_fault *vmf, struct folio **swapcache)
+			   struct vm_fault *vmf, struct folio **swapcache, void *shadow)
 {
 	struct mempolicy *mpol;
 	struct folio *folio;

@@ -930,7 +933,7 @@ struct folio *swapin_entry(swp_entry_t entry, gfp_t gfp_mask,
 
 	if (data_race(swp_swap_info(entry)->flags & SWP_SYNCHRONOUS_IO) &&
 	    __swap_count(entry) == 1) {
-		folio = swapin_direct(entry, gfp_mask, vmf);
+		folio = swapin_direct(entry, gfp_mask, vmf, shadow);
 	} else {
 		mpol = get_vma_policy(vmf->vma, vmf->address, 0, &ilx);
 		if (swap_use_vma_readahead())

mm/swapfile.c +2 -2

index 1cf7e72e19e3..aac26f5a6cec 100644
@@ -1865,7 +1865,7 @@ static int unuse_pte_range(struct vm_area_struct *vma, pmd_t *pmd,
 		pte_unmap(pte);
 		pte = NULL;
 
-		folio = swap_cache_get_folio(entry, vma, addr);
+		folio = swap_cache_get_folio(entry, vma, addr, NULL);
 		if (!folio) {
 			struct vm_fault vmf = {
 				.vma = vma,

@@ -1875,7 +1875,7 @@ static int unuse_pte_range(struct vm_area_struct *vma, pmd_t *pmd,
 			};
 
 			folio = swapin_entry(entry, GFP_HIGHUSER_MOVABLE,
-					    &vmf, NULL);
+					    &vmf, NULL, NULL);
 		}
 		if (!folio) {
 			/*

Hi, Minchan,

When I review the patchset from Kairui, I checked the code to skip swap cache in do_swap_page() for swap device with SWP_SYNCHRONOUS_IO. Is the following race possible? Where a page is swapped out to a swap device with SWP_SYNCHRONOUS_IO and the swap count is 1. Then 2 threads of the process runs on CPU0 and CPU1 as below. CPU0 is running do_swap_page().

CPU0 CPU1
---- ----
swap_cache_get_folio()
check sync io and swap count
alloc folio
swap_readpage()
folio_lock_or_retry()
swap in the swap entry write page
swap out to same swap entry
pte_offset_map_lock()
check pte_same()
swap_free() <-- new content lost!
set_pte_at() <-- stale page!
folio_unlock()
pte_unmap_unlock()

Hi Ying,

Quoted 8 lines · Ying

Hi, Minchan,

When I review the patchset from Kairui, I checked the code to skip swap cache in do_swap_page() for swap device with SWP_SYNCHRONOUS_IO. Is the following race possible? Where a page is swapped out to a swap device with SWP_SYNCHRONOUS_IO and the swap count is 1. Then 2 threads of the process runs on CPU0 and CPU1 as below. CPU0 is running do_swap_page().

Chris raised a similar issue about the shmem path, and I was worrying about the same issue in previous discussions about do_swap_page:
https://lore.kernel.org/linux-mm/CAMgjq7AwFiDb7cAMkWMWb3vkccie1-tocmZfT7m4WRb_UKPghg@mail.gmail.com/

"""
In do_swap_page path, multiple process could swapin the page at the same time (a mapped once page can still be shared by sub threads), they could get different folios. The later pte lock and pte_same check is not enough, because while one process is not holding the pte lock, another process could read-in, swap_free the entry, then swap-out the page again, using same entry, an ABA problem. The race is not likely to happen in reality but in theory possible.
"""

Quoted 17 lines

CPU0 CPU1
---- ----
swap_cache_get_folio()
check sync io and swap count
alloc folio
swap_readpage()
folio_lock_or_retry()
swap in the swap entry write page
swap out to same swap entry
pte_offset_map_lock()
check pte_same()
swap_free() <-- new content lost!
set_pte_at() <-- stale page!
folio_unlock()
pte_unmap_unlock()

Thank you very much for highlighting this!

My concern previously is the same as yours (swapping out using the same entry is like an ABA issue, where pte_same failed to detect the page table change), later when working on V3, I mistakenly thought that's impossible as entry should be pinned until swap_free on CPU0, and I'm wrong. CPU1 can also just call swap_free, then swap count is dropped to 0 and it can just swap out using the same entry. Now I think my patch 6/7 is also affected by this potential race. Seems nothing can stop it from doing this.

Actually I was trying to make a reproducer locally, due to swap slot cache, swap allocation algorithm, and the short race window, this is very unlikely to happen though.

How about we just increase the swap count temporarily in the direct swap in path (after alloc folio), then drop the count after pte_same (or shmem_add_to_page_cache in shmem path)? That seems enough to prevent the entry reuse issue.

Quoted 13 lines · Kairui Song

Hi Ying,

Quoted 8 lines · Ying

Hi, Minchan,

When I review the patchset from Kairui, I checked the code to skip swap cache in do_swap_page() for swap device with SWP_SYNCHRONOUS_IO. Is the following race possible? Where a page is swapped out to a swap device with SWP_SYNCHRONOUS_IO and the swap count is 1. Then 2 threads of the process runs on CPU0 and CPU1 as below. CPU0 is running do_swap_page().

Quoted 3 lines

Chris raised a similar issue about the shmem path, and I was worrying about the same issue in previous discussions about do_swap_page:
https://lore.kernel.org/linux-mm/CAMgjq7AwFiDb7cAMkWMWb3vkccie1-tocmZfT7m4WRb_UKPghg@mail.gmail.com/

Ha thanks for remembering that.

Quoted 40 lines

"""
In do_swap_page path, multiple process could swapin the page at the same time (a mapped once page can still be shared by sub threads), they could get different folios. The later pte lock and pte_same check is not enough, because while one process is not holding the pte lock, another process could read-in, swap_free the entry, then swap-out the page again, using same entry, an ABA problem. The race is not likely to happen in reality but in theory possible.
"""

Quoted 17 lines

CPU0 CPU1
---- ----
swap_cache_get_folio()
check sync io and swap count
alloc folio
swap_readpage()
folio_lock_or_retry()
swap in the swap entry write page
swap out to same swap entry
pte_offset_map_lock()
check pte_same()
swap_free() <-- new content lost!
set_pte_at() <-- stale page!
folio_unlock()
pte_unmap_unlock()

Thank you very much for highlighting this!

My concern previously is the same as yours (swapping out using the same entry is like an ABA issue, where pte_same failed to detect the page table change), later when working on V3, I mistakenly thought that's impossible as entry should be pinned until swap_free on CPU0, and I'm wrong. CPU1 can also just call swap_free, then swap count is dropped to 0 and it can just swap out using the same entry. Now I think my patch 6/7 is also affected by this potential race. Seems nothing can stop it from doing this.

Quoted 3 lines

Actually I was trying to make a reproducer locally, due to swap slot cache, swap allocation algorithm, and the short race window, this is very unlikely to happen though.

You can put some sleep in some of the CPU0 where expect the other race to happen to manual help triggering it. Yes, it sounds hard to trigger in real life due to reclaim swap out.

Quoted 1 line

Quoted 4 lines

How about we just increase the swap count temporarily in the direct swap in path (after alloc folio), then drop the count after pte_same (or shmem_add_to_page_cache in shmem path)? That seems enough to prevent the entry reuse issue.

Sounds like a good solution.

Hi, Minchan,

When I review the patchset from Kairui, I checked the code to skip swap cache in do_swap_page() for swap device with SWP_SYNCHRONOUS_IO. Is the following race possible? Where a page is swapped out to a swap device with SWP_SYNCHRONOUS_IO and the swap count is 1. Then 2 threads of the process runs on CPU0 and CPU1 as below. CPU0 is running do_swap_page().

CPU0 CPU1
---- ----
swap_cache_get_folio()
check sync io and swap count
alloc folio
swap_readpage()
folio_lock_or_retry()
swap in the swap entry write page
swap out to same swap entry
pte_offset_map_lock()
check pte_same()
swap_free() <-- new content lost!
set_pte_at() <-- stale page!
folio_unlock()
pte_unmap_unlock()