diff mbox series

[1/2] mm: add vma_has_locality()

Message ID 20221222061341.381903-1-yuanchu@google.com
State New
Headers
Series [1/2] mm: add vma_has_locality() |

Commit Message

Yuanchu Xie Dec. 22, 2022, 6:13 a.m. UTC 
  From: Yu Zhao <yuzhao@google.com>

Currently in vm_flags in vm_area_struct, both VM_SEQ_READ and
VM_RAND_READ indicate a lack of locality in accesses to the vma. Some
places that check for locality are missing one of them. We add
vma_has_locality to replace the existing locality checks for clarity.

Signed-off-by: Yu Zhao <yuzhao@google.com>
Signed-off-by: Yuanchu Xie <yuanchu@google.com>
---
 include/linux/mm_inline.h |  8 ++++++++
 mm/memory.c               |  7 +++----
 mm/rmap.c                 | 42 +++++++++++++++++----------------------
 mm/vmscan.c               |  5 ++++-
 4 files changed, 33 insertions(+), 29 deletions(-)

Comments

Yuanchu Xie Dec. 22, 2022, 6:38 a.m. UTC | #1 
I forgot to add the Ack from Johannes earlier[1]

[1] https://lore.kernel.org/all/Y36PF972kOK3ADvx@cmpxchg.org/


Acked-by: Johannes Weiner <hannes@cmpxchg.org>
Andrew Morton Dec. 22, 2022, 6:49 p.m. UTC | #2 
On Wed, 21 Dec 2022 22:13:40 -0800 Yuanchu Xie <yuanchu@google.com> wrote:

> From: Yu Zhao <yuzhao@google.com>
> 
> Currently in vm_flags in vm_area_struct, both VM_SEQ_READ and
> VM_RAND_READ indicate a lack of locality in accesses to the vma. Some
> places that check for locality are missing one of them. We add
> vma_has_locality to replace the existing locality checks for clarity.

I'm all confused.  Surely VM_SEQ_READ implies locality and VM_RAND_READ
indicates no-locality?
Yu Zhao Dec. 22, 2022, 7:44 p.m. UTC | #3 
On Thu, Dec 22, 2022 at 11:49 AM Andrew Morton
<akpm@linux-foundation.org> wrote:
>
> On Wed, 21 Dec 2022 22:13:40 -0800 Yuanchu Xie <yuanchu@google.com> wrote:
>
> > From: Yu Zhao <yuzhao@google.com>

This works; suggested-by probably works even better, since I didn't do
the follow-up work.

> > Currently in vm_flags in vm_area_struct, both VM_SEQ_READ and
> > VM_RAND_READ indicate a lack of locality in accesses to the vma. Some
> > places that check for locality are missing one of them. We add
> > vma_has_locality to replace the existing locality checks for clarity.
>
> I'm all confused.  Surely VM_SEQ_READ implies locality and VM_RAND_READ
> indicates no-locality?

Spatially, yes. But we focus more on the temporal criteria here, i.e.,
the reuse of an area within a relatively small duration. Both the
active/inactive LRU and MGLRU rely on this.

VM_SEQ_READ, while being a special case of spatial locality, fails the
temporal criteria. VM_RAND_READ fails both criterias, obviously.

Once an area passes the temporal criteria, MGLRU additionally exploits
spatial locality by lru_gen_look_around(), which is also touched in
this patch. This part is good to know but not really relevant here.
Yu Zhao Dec. 22, 2022, 7:52 p.m. UTC | #4 
On Wed, Dec 21, 2022 at 11:13 PM Yuanchu Xie <yuanchu@google.com> wrote:
>
> From: Yu Zhao <yuzhao@google.com>
>
> Currently in vm_flags in vm_area_struct, both VM_SEQ_READ and
> VM_RAND_READ indicate a lack of locality in accesses to the vma. Some
> places that check for locality are missing one of them. We add
> vma_has_locality to replace the existing locality checks for clarity.

Need benchmark results. A simple fio test will do; doesn't need to be
the curl one.

> +       /*
> +        * If we are reclaiming on behalf of a cgroup, skip counting on behalf
> +        * of references from different cgroups

Nit: add a period at the end.

> @@ -906,6 +908,7 @@ int folio_referenced(struct folio *folio, int is_locked,
>                 .arg = (void *)&pra,
>                 .anon_lock = folio_lock_anon_vma_read,
>                 .try_lock = true,
> +               .invalid_vma = invalid_folio_referenced_vma,

Nice. (What I suggested isn't as clean:
https://lore.kernel.org/all/Y31s%2FK8T85jh05wH@google.com/)
Andrew Morton Dec. 22, 2022, 8:29 p.m. UTC | #5 
On Thu, 22 Dec 2022 12:44:35 -0700 Yu Zhao <yuzhao@google.com> wrote:

> On Thu, Dec 22, 2022 at 11:49 AM Andrew Morton
> <akpm@linux-foundation.org> wrote:
> >
> > On Wed, 21 Dec 2022 22:13:40 -0800 Yuanchu Xie <yuanchu@google.com> wrote:
> >
> > > From: Yu Zhao <yuzhao@google.com>
> 
> This works; suggested-by probably works even better, since I didn't do
> the follow-up work.
> 
> > > Currently in vm_flags in vm_area_struct, both VM_SEQ_READ and
> > > VM_RAND_READ indicate a lack of locality in accesses to the vma. Some
> > > places that check for locality are missing one of them. We add
> > > vma_has_locality to replace the existing locality checks for clarity.
> >
> > I'm all confused.  Surely VM_SEQ_READ implies locality and VM_RAND_READ
> > indicates no-locality?
> 
> Spatially, yes. But we focus more on the temporal criteria here, i.e.,
> the reuse of an area within a relatively small duration. Both the
> active/inactive LRU and MGLRU rely on this.

Oh.  Why didn't it say that ;)

How about s/locality/recency/g?
Yu Zhao Dec. 30, 2022, 9:59 p.m. UTC | #6 
On Thu, Dec 22, 2022 at 1:29 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Thu, 22 Dec 2022 12:44:35 -0700 Yu Zhao <yuzhao@google.com> wrote:
>
> > On Thu, Dec 22, 2022 at 11:49 AM Andrew Morton
> > <akpm@linux-foundation.org> wrote:
> > >
> > > On Wed, 21 Dec 2022 22:13:40 -0800 Yuanchu Xie <yuanchu@google.com> wrote:
> > >
> > > > From: Yu Zhao <yuzhao@google.com>
> >
> > This works; suggested-by probably works even better, since I didn't do
> > the follow-up work.
> >
> > > > Currently in vm_flags in vm_area_struct, both VM_SEQ_READ and
> > > > VM_RAND_READ indicate a lack of locality in accesses to the vma. Some
> > > > places that check for locality are missing one of them. We add
> > > > vma_has_locality to replace the existing locality checks for clarity.
> > >
> > > I'm all confused.  Surely VM_SEQ_READ implies locality and VM_RAND_READ
> > > indicates no-locality?
> >
> > Spatially, yes. But we focus more on the temporal criteria here, i.e.,
> > the reuse of an area within a relatively small duration. Both the
> > active/inactive LRU and MGLRU rely on this.
>
> Oh.  Why didn't it say that ;)
>
> How about s/locality/recency/g?

Thanks. I've done this, and posted the v2 which includes much better
commit messages.
diff mbox series

Patch

diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
index e8ed225d8f7c..80c0f6901ead 100644
--- a/include/linux/mm_inline.h
+++ b/include/linux/mm_inline.h
@@ -578,4 +578,12 @@  pte_install_uffd_wp_if_needed(struct vm_area_struct *vma, unsigned long addr,
 #endif
 }
 
+static inline bool vma_has_locality(struct vm_area_struct *vma)
+{
+	if (vma->vm_flags & (VM_SEQ_READ | VM_RAND_READ))
+		return false;
+
+	return true;
+}
+
 #endif
diff --git a/mm/memory.c b/mm/memory.c
index 4000e9f017e0..a3f60e53f348 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1402,8 +1402,7 @@  static unsigned long zap_pte_range(struct mmu_gather *tlb,
 						force_flush = 1;
 					}
 				}
-				if (pte_young(ptent) &&
-				    likely(!(vma->vm_flags & VM_SEQ_READ)))
+				if (pte_young(ptent) && likely(vma_has_locality(vma)))
 					mark_page_accessed(page);
 			}
 			rss[mm_counter(page)]--;
@@ -5148,8 +5147,8 @@  static inline void mm_account_fault(struct pt_regs *regs,
 #ifdef CONFIG_LRU_GEN
 static void lru_gen_enter_fault(struct vm_area_struct *vma)
 {
-	/* the LRU algorithm doesn't apply to sequential or random reads */
-	current->in_lru_fault = !(vma->vm_flags & (VM_SEQ_READ | VM_RAND_READ));
+	/* the LRU algorithm only applies to accesses with locality */
+	current->in_lru_fault = vma_has_locality(vma);
 }
 
 static void lru_gen_exit_fault(void)
diff --git a/mm/rmap.c b/mm/rmap.c
index 32e48b1c5847..a2e83fea6fed 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -823,25 +823,14 @@  static bool folio_referenced_one(struct folio *folio,
 		}
 
 		if (pvmw.pte) {
-			if (lru_gen_enabled() && pte_young(*pvmw.pte) &&
-			    !(vma->vm_flags & (VM_SEQ_READ | VM_RAND_READ))) {
+			if (lru_gen_enabled() && pte_young(*pvmw.pte)) {
 				lru_gen_look_around(&pvmw);
 				referenced++;
 			}
 
 			if (ptep_clear_flush_young_notify(vma, address,
-						pvmw.pte)) {
-				/*
-				 * Don't treat a reference through
-				 * a sequentially read mapping as such.
-				 * If the folio has been used in another mapping,
-				 * we will catch it; if this other mapping is
-				 * already gone, the unmap path will have set
-				 * the referenced flag or activated the folio.
-				 */
-				if (likely(!(vma->vm_flags & VM_SEQ_READ)))
-					referenced++;
-			}
+						pvmw.pte))
+				referenced++;
 		} else if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)) {
 			if (pmdp_clear_flush_young_notify(vma, address,
 						pvmw.pmd))
@@ -875,7 +864,20 @@  static bool invalid_folio_referenced_vma(struct vm_area_struct *vma, void *arg)
 	struct folio_referenced_arg *pra = arg;
 	struct mem_cgroup *memcg = pra->memcg;
 
-	if (!mm_match_cgroup(vma->vm_mm, memcg))
+	/*
+	 * Ignore references from this mapping if it has no locality. If the
+	 * folio has been used in another mapping, we will catch it; if this
+	 * other mapping is already gone, the unmap path will have set the
+	 * referenced flag or activated the folio in zap_pte_range().
+	 */
+	if (!vma_has_locality(vma))
+		return true;
+
+	/*
+	 * If we are reclaiming on behalf of a cgroup, skip counting on behalf
+	 * of references from different cgroups
+	 */
+	if (memcg && !mm_match_cgroup(vma->vm_mm, memcg))
 		return true;
 
 	return false;
@@ -906,6 +908,7 @@  int folio_referenced(struct folio *folio, int is_locked,
 		.arg = (void *)&pra,
 		.anon_lock = folio_lock_anon_vma_read,
 		.try_lock = true,
+		.invalid_vma = invalid_folio_referenced_vma,
 	};
 
 	*vm_flags = 0;
@@ -921,15 +924,6 @@  int folio_referenced(struct folio *folio, int is_locked,
 			return 1;
 	}
 
-	/*
-	 * If we are reclaiming on behalf of a cgroup, skip
-	 * counting on behalf of references from different
-	 * cgroups
-	 */
-	if (memcg) {
-		rwc.invalid_vma = invalid_folio_referenced_vma;
-	}
-
 	rmap_walk(folio, &rwc);
 	*vm_flags = pra.vm_flags;
 
diff --git a/mm/vmscan.c b/mm/vmscan.c
index e83d2a74e942..5cf39f314876 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -3782,7 +3782,10 @@  static int should_skip_vma(unsigned long start, unsigned long end, struct mm_wal
 	if (is_vm_hugetlb_page(vma))
 		return true;
 
-	if (vma->vm_flags & (VM_LOCKED | VM_SPECIAL | VM_SEQ_READ | VM_RAND_READ))
+	if (!vma_has_locality(vma))
+		return true;
+
+	if (vma->vm_flags & (VM_LOCKED | VM_SPECIAL))
 		return true;
 
 	if (vma == get_gate_vma(vma->vm_mm))