[PATCH mm-unstable v1 0/5] mm/swap: remove boilerplate

[PATCH mm-unstable v1 0/5] mm/swap: remove boilerplate

[Yu Zhao]

Use folio_activate() as an example:

Before this series
------------------
    if (!folio_test_active(folio) && !folio_test_unevictable(folio)) {
      struct folio_batch *fbatch;

      folio_get(folio);
      if (!folio_test_clear_lru(folio)) {
        folio_put(folio);
        return;
      }

      local_lock(&cpu_fbatches.lock);
      fbatch = this_cpu_ptr(&cpu_fbatches.activate);
      folio_batch_add_and_move(fbatch, folio, folio_activate_fn);
      local_unlock(&cpu_fbatches.lock);
    }
  }

After this series
-----------------
  void folio_activate(struct folio *folio)
  {
    if (folio_test_active(folio) || folio_test_unevictable(folio))
      return;
  
    folio_batch_add_and_move(folio, lru_activate, true);
  }

And this is applied to all 6 folio_batch handlers in mm/swap.c.

bloat-o-meter
-------------
  add/remove: 12/13 grow/shrink: 3/2 up/down: 4653/-4721 (-68)
  ...
  Total: Before=28083019, After=28082951, chg -0.00%

Yu Zhao (5):
  mm/swap: reduce indentation level
  mm/swap: rename cpu_fbatches->activate
  mm/swap: fold lru_rotate into cpu_fbatches
  mm/swap: remove remaining _fn suffix
  mm/swap: remove boilerplate

 mm/swap.c | 296 ++++++++++++++++++++++++------------------------------
 1 file changed, 134 insertions(+), 162 deletions(-)

-- 
2.45.2.803.g4e1b14247a-goog

[PATCH mm-unstable v1 1/5] mm/swap: reduce indentation level

[Yu Zhao]

Reduce indentation level by returning directly when there is no
cleanup needed, i.e.,

  if (condition) {    |    if (condition) {
    do_this();        |      do_this();
    return;           |      return;
  } else {            |    }
    do_that();        |
  }                   |    do_that();

and

  if (condition) {    |    if (!condition)
    do_this();        |      return;
    do_that();        |
  }                   |    do_this();
  return;             |    do_that();

Presumably the old style became repetitive as the result of copy and
paste.

Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 mm/swap.c | 209 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 109 insertions(+), 100 deletions(-)

diff --git a/mm/swap.c b/mm/swap.c
index 9caf6b017cf0..952e4aac6eb1 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -117,7 +117,9 @@ void __folio_put(struct folio *folio)
 	if (unlikely(folio_is_zone_device(folio))) {
 		free_zone_device_folio(folio);
 		return;
-	} else if (folio_test_hugetlb(folio)) {
+	}
+
+	if (folio_test_hugetlb(folio)) {
 		free_huge_folio(folio);
 		return;
 	}
@@ -228,17 +230,19 @@ static void folio_batch_add_and_move(struct folio_batch *fbatch,
 	if (folio_batch_add(fbatch, folio) && !folio_test_large(folio) &&
 	    !lru_cache_disabled())
 		return;
+
 	folio_batch_move_lru(fbatch, move_fn);
 }
 
 static void lru_move_tail_fn(struct lruvec *lruvec, struct folio *folio)
 {
-	if (!folio_test_unevictable(folio)) {
-		lruvec_del_folio(lruvec, folio);
-		folio_clear_active(folio);
-		lruvec_add_folio_tail(lruvec, folio);
-		__count_vm_events(PGROTATED, folio_nr_pages(folio));
-	}
+	if (folio_test_unevictable(folio))
+		return;
+
+	lruvec_del_folio(lruvec, folio);
+	folio_clear_active(folio);
+	lruvec_add_folio_tail(lruvec, folio);
+	__count_vm_events(PGROTATED, folio_nr_pages(folio));
 }
 
 /*
@@ -250,22 +254,23 @@ static void lru_move_tail_fn(struct lruvec *lruvec, struct folio *folio)
  */
 void folio_rotate_reclaimable(struct folio *folio)
 {
-	if (!folio_test_locked(folio) && !folio_test_dirty(folio) &&
-	    !folio_test_unevictable(folio)) {
-		struct folio_batch *fbatch;
-		unsigned long flags;
+	struct folio_batch *fbatch;
+	unsigned long flags;
 
-		folio_get(folio);
-		if (!folio_test_clear_lru(folio)) {
-			folio_put(folio);
-			return;
-		}
+	if (folio_test_locked(folio) || folio_test_dirty(folio) ||
+	    folio_test_unevictable(folio))
+		return;
 
-		local_lock_irqsave(&lru_rotate.lock, flags);
-		fbatch = this_cpu_ptr(&lru_rotate.fbatch);
-		folio_batch_add_and_move(fbatch, folio, lru_move_tail_fn);
-		local_unlock_irqrestore(&lru_rotate.lock, flags);
+	folio_get(folio);
+	if (!folio_test_clear_lru(folio)) {
+		folio_put(folio);
+		return;
 	}
+
+	local_lock_irqsave(&lru_rotate.lock, flags);
+	fbatch = this_cpu_ptr(&lru_rotate.fbatch);
+	folio_batch_add_and_move(fbatch, folio, lru_move_tail_fn);
+	local_unlock_irqrestore(&lru_rotate.lock, flags);
 }
 
 void lru_note_cost(struct lruvec *lruvec, bool file,
@@ -328,18 +333,19 @@ void lru_note_cost_refault(struct folio *folio)
 
 static void folio_activate_fn(struct lruvec *lruvec, struct folio *folio)
 {
-	if (!folio_test_active(folio) && !folio_test_unevictable(folio)) {
-		long nr_pages = folio_nr_pages(folio);
-
-		lruvec_del_folio(lruvec, folio);
-		folio_set_active(folio);
-		lruvec_add_folio(lruvec, folio);
-		trace_mm_lru_activate(folio);
-
-		__count_vm_events(PGACTIVATE, nr_pages);
-		__count_memcg_events(lruvec_memcg(lruvec), PGACTIVATE,
-				     nr_pages);
-	}
+	long nr_pages = folio_nr_pages(folio);
+
+	if (folio_test_active(folio) || folio_test_unevictable(folio))
+		return;
+
+
+	lruvec_del_folio(lruvec, folio);
+	folio_set_active(folio);
+	lruvec_add_folio(lruvec, folio);
+	trace_mm_lru_activate(folio);
+
+	__count_vm_events(PGACTIVATE, nr_pages);
+	__count_memcg_events(lruvec_memcg(lruvec), PGACTIVATE, nr_pages);
 }
 
 #ifdef CONFIG_SMP
@@ -353,20 +359,21 @@ static void folio_activate_drain(int cpu)
 
 void folio_activate(struct folio *folio)
 {
-	if (!folio_test_active(folio) && !folio_test_unevictable(folio)) {
-		struct folio_batch *fbatch;
+	struct folio_batch *fbatch;
 
-		folio_get(folio);
-		if (!folio_test_clear_lru(folio)) {
-			folio_put(folio);
-			return;
-		}
+	if (folio_test_active(folio) || folio_test_unevictable(folio))
+		return;
 
-		local_lock(&cpu_fbatches.lock);
-		fbatch = this_cpu_ptr(&cpu_fbatches.activate);
-		folio_batch_add_and_move(fbatch, folio, folio_activate_fn);
-		local_unlock(&cpu_fbatches.lock);
+	folio_get(folio);
+	if (!folio_test_clear_lru(folio)) {
+		folio_put(folio);
+		return;
 	}
+
+	local_lock(&cpu_fbatches.lock);
+	fbatch = this_cpu_ptr(&cpu_fbatches.activate);
+	folio_batch_add_and_move(fbatch, folio, folio_activate_fn);
+	local_unlock(&cpu_fbatches.lock);
 }
 
 #else
@@ -378,12 +385,13 @@ void folio_activate(struct folio *folio)
 {
 	struct lruvec *lruvec;
 
-	if (folio_test_clear_lru(folio)) {
-		lruvec = folio_lruvec_lock_irq(folio);
-		folio_activate_fn(lruvec, folio);
-		unlock_page_lruvec_irq(lruvec);
-		folio_set_lru(folio);
-	}
+	if (!folio_test_clear_lru(folio))
+		return;
+
+	lruvec = folio_lruvec_lock_irq(folio);
+	folio_activate_fn(lruvec, folio);
+	unlock_page_lruvec_irq(lruvec);
+	folio_set_lru(folio);
 }
 #endif
 
@@ -610,41 +618,41 @@ static void lru_deactivate_file_fn(struct lruvec *lruvec, struct folio *folio)
 
 static void lru_deactivate_fn(struct lruvec *lruvec, struct folio *folio)
 {
-	if (!folio_test_unevictable(folio) && (folio_test_active(folio) || lru_gen_enabled())) {
-		long nr_pages = folio_nr_pages(folio);
+	long nr_pages = folio_nr_pages(folio);
 
-		lruvec_del_folio(lruvec, folio);
-		folio_clear_active(folio);
-		folio_clear_referenced(folio);
-		lruvec_add_folio(lruvec, folio);
+	if (folio_test_unevictable(folio) || !(folio_test_active(folio) || lru_gen_enabled()))
+		return;
 
-		__count_vm_events(PGDEACTIVATE, nr_pages);
-		__count_memcg_events(lruvec_memcg(lruvec), PGDEACTIVATE,
-				     nr_pages);
-	}
+	lruvec_del_folio(lruvec, folio);
+	folio_clear_active(folio);
+	folio_clear_referenced(folio);
+	lruvec_add_folio(lruvec, folio);
+
+	__count_vm_events(PGDEACTIVATE, nr_pages);
+	__count_memcg_events(lruvec_memcg(lruvec), PGDEACTIVATE, nr_pages);
 }
 
 static void lru_lazyfree_fn(struct lruvec *lruvec, struct folio *folio)
 {
-	if (folio_test_anon(folio) && folio_test_swapbacked(folio) &&
-	    !folio_test_swapcache(folio) && !folio_test_unevictable(folio)) {
-		long nr_pages = folio_nr_pages(folio);
+	long nr_pages = folio_nr_pages(folio);
 
-		lruvec_del_folio(lruvec, folio);
-		folio_clear_active(folio);
-		folio_clear_referenced(folio);
-		/*
-		 * Lazyfree folios are clean anonymous folios.  They have
-		 * the swapbacked flag cleared, to distinguish them from normal
-		 * anonymous folios
-		 */
-		folio_clear_swapbacked(folio);
-		lruvec_add_folio(lruvec, folio);
+	if (!folio_test_anon(folio) || !folio_test_swapbacked(folio) ||
+	    folio_test_swapcache(folio) || folio_test_unevictable(folio))
+		return;
 
-		__count_vm_events(PGLAZYFREE, nr_pages);
-		__count_memcg_events(lruvec_memcg(lruvec), PGLAZYFREE,
-				     nr_pages);
-	}
+	lruvec_del_folio(lruvec, folio);
+	folio_clear_active(folio);
+	folio_clear_referenced(folio);
+	/*
+	 * Lazyfree folios are clean anonymous folios.  They have
+	 * the swapbacked flag cleared, to distinguish them from normal
+	 * anonymous folios
+	 */
+	folio_clear_swapbacked(folio);
+	lruvec_add_folio(lruvec, folio);
+
+	__count_vm_events(PGLAZYFREE, nr_pages);
+	__count_memcg_events(lruvec_memcg(lruvec), PGLAZYFREE, nr_pages);
 }
 
 /*
@@ -726,21 +734,21 @@ void deactivate_file_folio(struct folio *folio)
  */
 void folio_deactivate(struct folio *folio)
 {
-	if (!folio_test_unevictable(folio) && (folio_test_active(folio) ||
-	    lru_gen_enabled())) {
-		struct folio_batch *fbatch;
+	struct folio_batch *fbatch;
 
-		folio_get(folio);
-		if (!folio_test_clear_lru(folio)) {
-			folio_put(folio);
-			return;
-		}
+	if (folio_test_unevictable(folio) || !(folio_test_active(folio) || lru_gen_enabled()))
+		return;
 
-		local_lock(&cpu_fbatches.lock);
-		fbatch = this_cpu_ptr(&cpu_fbatches.lru_deactivate);
-		folio_batch_add_and_move(fbatch, folio, lru_deactivate_fn);
-		local_unlock(&cpu_fbatches.lock);
+	folio_get(folio);
+	if (!folio_test_clear_lru(folio)) {
+		folio_put(folio);
+		return;
 	}
+
+	local_lock(&cpu_fbatches.lock);
+	fbatch = this_cpu_ptr(&cpu_fbatches.lru_deactivate);
+	folio_batch_add_and_move(fbatch, folio, lru_deactivate_fn);
+	local_unlock(&cpu_fbatches.lock);
 }
 
 /**
@@ -752,21 +760,22 @@ void folio_deactivate(struct folio *folio)
  */
 void folio_mark_lazyfree(struct folio *folio)
 {
-	if (folio_test_anon(folio) && folio_test_swapbacked(folio) &&
-	    !folio_test_swapcache(folio) && !folio_test_unevictable(folio)) {
-		struct folio_batch *fbatch;
+	struct folio_batch *fbatch;
 
-		folio_get(folio);
-		if (!folio_test_clear_lru(folio)) {
-			folio_put(folio);
-			return;
-		}
+	if (!folio_test_anon(folio) || !folio_test_swapbacked(folio) ||
+	    folio_test_swapcache(folio) || folio_test_unevictable(folio))
+		return;
 
-		local_lock(&cpu_fbatches.lock);
-		fbatch = this_cpu_ptr(&cpu_fbatches.lru_lazyfree);
-		folio_batch_add_and_move(fbatch, folio, lru_lazyfree_fn);
-		local_unlock(&cpu_fbatches.lock);
+	folio_get(folio);
+	if (!folio_test_clear_lru(folio)) {
+		folio_put(folio);
+		return;
 	}
+
+	local_lock(&cpu_fbatches.lock);
+	fbatch = this_cpu_ptr(&cpu_fbatches.lru_lazyfree);
+	folio_batch_add_and_move(fbatch, folio, lru_lazyfree_fn);
+	local_unlock(&cpu_fbatches.lock);
 }
 
 void lru_add_drain(void)
-- 
2.45.2.803.g4e1b14247a-goog

[PATCH mm-unstable v1 2/5] mm/swap: rename cpu_fbatches->activate

[Yu Zhao]

Rename cpu_fbatches->activate to cpu_fbatches->lru_activate, and its
handler folio_activate_fn() to lru_activate() so that all the
boilerplate can be removed at the end of this series.

Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 mm/swap.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/mm/swap.c b/mm/swap.c
index 952e4aac6eb1..e4745b88a964 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -67,7 +67,7 @@ struct cpu_fbatches {
 	struct folio_batch lru_deactivate;
 	struct folio_batch lru_lazyfree;
 #ifdef CONFIG_SMP
-	struct folio_batch activate;
+	struct folio_batch lru_activate;
 #endif
 };
 static DEFINE_PER_CPU(struct cpu_fbatches, cpu_fbatches) = {
@@ -331,7 +331,7 @@ void lru_note_cost_refault(struct folio *folio)
 		      folio_nr_pages(folio), 0);
 }
 
-static void folio_activate_fn(struct lruvec *lruvec, struct folio *folio)
+static void lru_activate(struct lruvec *lruvec, struct folio *folio)
 {
 	long nr_pages = folio_nr_pages(folio);
 
@@ -351,10 +351,10 @@ static void folio_activate_fn(struct lruvec *lruvec, struct folio *folio)
 #ifdef CONFIG_SMP
 static void folio_activate_drain(int cpu)
 {
-	struct folio_batch *fbatch = &per_cpu(cpu_fbatches.activate, cpu);
+	struct folio_batch *fbatch = &per_cpu(cpu_fbatches.lru_activate, cpu);
 
 	if (folio_batch_count(fbatch))
-		folio_batch_move_lru(fbatch, folio_activate_fn);
+		folio_batch_move_lru(fbatch, lru_activate);
 }
 
 void folio_activate(struct folio *folio)
@@ -371,8 +371,8 @@ void folio_activate(struct folio *folio)
 	}
 
 	local_lock(&cpu_fbatches.lock);
-	fbatch = this_cpu_ptr(&cpu_fbatches.activate);
-	folio_batch_add_and_move(fbatch, folio, folio_activate_fn);
+	fbatch = this_cpu_ptr(&cpu_fbatches.lru_activate);
+	folio_batch_add_and_move(fbatch, folio, lru_activate);
 	local_unlock(&cpu_fbatches.lock);
 }
 
@@ -389,7 +389,7 @@ void folio_activate(struct folio *folio)
 		return;
 
 	lruvec = folio_lruvec_lock_irq(folio);
-	folio_activate_fn(lruvec, folio);
+	lru_activate(lruvec, folio);
 	unlock_page_lruvec_irq(lruvec);
 	folio_set_lru(folio);
 }
@@ -490,7 +490,7 @@ void folio_mark_accessed(struct folio *folio)
 	} else if (!folio_test_active(folio)) {
 		/*
 		 * If the folio is on the LRU, queue it for activation via
-		 * cpu_fbatches.activate. Otherwise, assume the folio is in a
+		 * cpu_fbatches.lru_activate. Otherwise, assume the folio is in a
 		 * folio_batch, mark it active and it'll be moved to the active
 		 * LRU on the next drain.
 		 */
@@ -829,7 +829,7 @@ static bool cpu_needs_drain(unsigned int cpu)
 		folio_batch_count(&fbatches->lru_deactivate_file) ||
 		folio_batch_count(&fbatches->lru_deactivate) ||
 		folio_batch_count(&fbatches->lru_lazyfree) ||
-		folio_batch_count(&fbatches->activate) ||
+		folio_batch_count(&fbatches->lru_activate) ||
 		need_mlock_drain(cpu) ||
 		has_bh_in_lru(cpu, NULL);
 }
-- 
2.45.2.803.g4e1b14247a-goog

[PATCH mm-unstable v1 3/5] mm/swap: fold lru_rotate into cpu_fbatches

[Yu Zhao]

Fold lru_rotate into cpu_fbatches, and rename the folio_batch and the
lock protecting it to lru_move_tail and lock_irq respectively so that
all the boilerplate can be removed at the end of this series.

Also remove data_race() around folio_batch_count(), which is out of
place: all folio_batch_count() calls on remote cpu_fbatches are
subject to data_race(), and therefore data_race() should be inside
folio_batch_count().

Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 mm/swap.c | 36 ++++++++++++++++--------------------
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/mm/swap.c b/mm/swap.c
index e4745b88a964..774ae9eab1e6 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -47,20 +47,11 @@
 int page_cluster;
 const int page_cluster_max = 31;
 
-/* Protecting only lru_rotate.fbatch which requires disabling interrupts */
-struct lru_rotate {
-	local_lock_t lock;
-	struct folio_batch fbatch;
-};
-static DEFINE_PER_CPU(struct lru_rotate, lru_rotate) = {
-	.lock = INIT_LOCAL_LOCK(lock),
-};
-
-/*
- * The following folio batches are grouped together because they are protected
- * by disabling preemption (and interrupts remain enabled).
- */
 struct cpu_fbatches {
+	/*
+	 * The following folio batches are grouped together because they are protected
+	 * by disabling preemption (and interrupts remain enabled).
+	 */
 	local_lock_t lock;
 	struct folio_batch lru_add;
 	struct folio_batch lru_deactivate_file;
@@ -69,9 +60,14 @@ struct cpu_fbatches {
 #ifdef CONFIG_SMP
 	struct folio_batch lru_activate;
 #endif
+	/* Protecting the following batches which require disabling interrupts */
+	local_lock_t lock_irq;
+	struct folio_batch lru_move_tail;
 };
+
 static DEFINE_PER_CPU(struct cpu_fbatches, cpu_fbatches) = {
 	.lock = INIT_LOCAL_LOCK(lock),
+	.lock_irq = INIT_LOCAL_LOCK(lock_irq),
 };
 
 static void __page_cache_release(struct folio *folio, struct lruvec **lruvecp,
@@ -267,10 +263,10 @@ void folio_rotate_reclaimable(struct folio *folio)
 		return;
 	}
 
-	local_lock_irqsave(&lru_rotate.lock, flags);
-	fbatch = this_cpu_ptr(&lru_rotate.fbatch);
+	local_lock_irqsave(&cpu_fbatches.lock_irq, flags);
+	fbatch = this_cpu_ptr(&cpu_fbatches.lru_move_tail);
 	folio_batch_add_and_move(fbatch, folio, lru_move_tail_fn);
-	local_unlock_irqrestore(&lru_rotate.lock, flags);
+	local_unlock_irqrestore(&cpu_fbatches.lock_irq, flags);
 }
 
 void lru_note_cost(struct lruvec *lruvec, bool file,
@@ -668,15 +664,15 @@ void lru_add_drain_cpu(int cpu)
 	if (folio_batch_count(fbatch))
 		folio_batch_move_lru(fbatch, lru_add_fn);
 
-	fbatch = &per_cpu(lru_rotate.fbatch, cpu);
+	fbatch = &fbatches->lru_move_tail;
 	/* Disabling interrupts below acts as a compiler barrier. */
 	if (data_race(folio_batch_count(fbatch))) {
 		unsigned long flags;
 
 		/* No harm done if a racing interrupt already did this */
-		local_lock_irqsave(&lru_rotate.lock, flags);
+		local_lock_irqsave(&cpu_fbatches.lock_irq, flags);
 		folio_batch_move_lru(fbatch, lru_move_tail_fn);
-		local_unlock_irqrestore(&lru_rotate.lock, flags);
+		local_unlock_irqrestore(&cpu_fbatches.lock_irq, flags);
 	}
 
 	fbatch = &fbatches->lru_deactivate_file;
@@ -825,7 +821,7 @@ static bool cpu_needs_drain(unsigned int cpu)
 
 	/* Check these in order of likelihood that they're not zero */
 	return folio_batch_count(&fbatches->lru_add) ||
-		data_race(folio_batch_count(&per_cpu(lru_rotate.fbatch, cpu))) ||
+		folio_batch_count(&fbatches->lru_move_tail) ||
 		folio_batch_count(&fbatches->lru_deactivate_file) ||
 		folio_batch_count(&fbatches->lru_deactivate) ||
 		folio_batch_count(&fbatches->lru_lazyfree) ||
-- 
2.45.2.803.g4e1b14247a-goog

[PATCH mm-unstable v1 4/5] mm/swap: remove remaining _fn suffix

[Yu Zhao]

Remove remaining _fn suffix from cpu_fbatches handlers, which are
already self-explanatory.

Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 mm/swap.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/mm/swap.c b/mm/swap.c
index 774ae9eab1e6..4a66d2f87f26 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -160,7 +160,7 @@ EXPORT_SYMBOL(put_pages_list);
 
 typedef void (*move_fn_t)(struct lruvec *lruvec, struct folio *folio);
 
-static void lru_add_fn(struct lruvec *lruvec, struct folio *folio)
+static void lru_add(struct lruvec *lruvec, struct folio *folio)
 {
 	int was_unevictable = folio_test_clear_unevictable(folio);
 	long nr_pages = folio_nr_pages(folio);
@@ -230,7 +230,7 @@ static void folio_batch_add_and_move(struct folio_batch *fbatch,
 	folio_batch_move_lru(fbatch, move_fn);
 }
 
-static void lru_move_tail_fn(struct lruvec *lruvec, struct folio *folio)
+static void lru_move_tail(struct lruvec *lruvec, struct folio *folio)
 {
 	if (folio_test_unevictable(folio))
 		return;
@@ -265,7 +265,7 @@ void folio_rotate_reclaimable(struct folio *folio)
 
 	local_lock_irqsave(&cpu_fbatches.lock_irq, flags);
 	fbatch = this_cpu_ptr(&cpu_fbatches.lru_move_tail);
-	folio_batch_add_and_move(fbatch, folio, lru_move_tail_fn);
+	folio_batch_add_and_move(fbatch, folio, lru_move_tail);
 	local_unlock_irqrestore(&cpu_fbatches.lock_irq, flags);
 }
 
@@ -527,7 +527,7 @@ void folio_add_lru(struct folio *folio)
 	folio_get(folio);
 	local_lock(&cpu_fbatches.lock);
 	fbatch = this_cpu_ptr(&cpu_fbatches.lru_add);
-	folio_batch_add_and_move(fbatch, folio, lru_add_fn);
+	folio_batch_add_and_move(fbatch, folio, lru_add);
 	local_unlock(&cpu_fbatches.lock);
 }
 EXPORT_SYMBOL(folio_add_lru);
@@ -571,7 +571,7 @@ void folio_add_lru_vma(struct folio *folio, struct vm_area_struct *vma)
  * written out by flusher threads as this is much more efficient
  * than the single-page writeout from reclaim.
  */
-static void lru_deactivate_file_fn(struct lruvec *lruvec, struct folio *folio)
+static void lru_deactivate_file(struct lruvec *lruvec, struct folio *folio)
 {
 	bool active = folio_test_active(folio);
 	long nr_pages = folio_nr_pages(folio);
@@ -612,7 +612,7 @@ static void lru_deactivate_file_fn(struct lruvec *lruvec, struct folio *folio)
 	}
 }
 
-static void lru_deactivate_fn(struct lruvec *lruvec, struct folio *folio)
+static void lru_deactivate(struct lruvec *lruvec, struct folio *folio)
 {
 	long nr_pages = folio_nr_pages(folio);
 
@@ -628,7 +628,7 @@ static void lru_deactivate_fn(struct lruvec *lruvec, struct folio *folio)
 	__count_memcg_events(lruvec_memcg(lruvec), PGDEACTIVATE, nr_pages);
 }
 
-static void lru_lazyfree_fn(struct lruvec *lruvec, struct folio *folio)
+static void lru_lazyfree(struct lruvec *lruvec, struct folio *folio)
 {
 	long nr_pages = folio_nr_pages(folio);
 
@@ -662,7 +662,7 @@ void lru_add_drain_cpu(int cpu)
 	struct folio_batch *fbatch = &fbatches->lru_add;
 
 	if (folio_batch_count(fbatch))
-		folio_batch_move_lru(fbatch, lru_add_fn);
+		folio_batch_move_lru(fbatch, lru_add);
 
 	fbatch = &fbatches->lru_move_tail;
 	/* Disabling interrupts below acts as a compiler barrier. */
@@ -671,21 +671,21 @@ void lru_add_drain_cpu(int cpu)
 
 		/* No harm done if a racing interrupt already did this */
 		local_lock_irqsave(&cpu_fbatches.lock_irq, flags);
-		folio_batch_move_lru(fbatch, lru_move_tail_fn);
+		folio_batch_move_lru(fbatch, lru_move_tail);
 		local_unlock_irqrestore(&cpu_fbatches.lock_irq, flags);
 	}
 
 	fbatch = &fbatches->lru_deactivate_file;
 	if (folio_batch_count(fbatch))
-		folio_batch_move_lru(fbatch, lru_deactivate_file_fn);
+		folio_batch_move_lru(fbatch, lru_deactivate_file);
 
 	fbatch = &fbatches->lru_deactivate;
 	if (folio_batch_count(fbatch))
-		folio_batch_move_lru(fbatch, lru_deactivate_fn);
+		folio_batch_move_lru(fbatch, lru_deactivate);
 
 	fbatch = &fbatches->lru_lazyfree;
 	if (folio_batch_count(fbatch))
-		folio_batch_move_lru(fbatch, lru_lazyfree_fn);
+		folio_batch_move_lru(fbatch, lru_lazyfree);
 
 	folio_activate_drain(cpu);
 }
@@ -716,7 +716,7 @@ void deactivate_file_folio(struct folio *folio)
 
 	local_lock(&cpu_fbatches.lock);
 	fbatch = this_cpu_ptr(&cpu_fbatches.lru_deactivate_file);
-	folio_batch_add_and_move(fbatch, folio, lru_deactivate_file_fn);
+	folio_batch_add_and_move(fbatch, folio, lru_deactivate_file);
 	local_unlock(&cpu_fbatches.lock);
 }
 
@@ -743,7 +743,7 @@ void folio_deactivate(struct folio *folio)
 
 	local_lock(&cpu_fbatches.lock);
 	fbatch = this_cpu_ptr(&cpu_fbatches.lru_deactivate);
-	folio_batch_add_and_move(fbatch, folio, lru_deactivate_fn);
+	folio_batch_add_and_move(fbatch, folio, lru_deactivate);
 	local_unlock(&cpu_fbatches.lock);
 }
 
@@ -770,7 +770,7 @@ void folio_mark_lazyfree(struct folio *folio)
 
 	local_lock(&cpu_fbatches.lock);
 	fbatch = this_cpu_ptr(&cpu_fbatches.lru_lazyfree);
-	folio_batch_add_and_move(fbatch, folio, lru_lazyfree_fn);
+	folio_batch_add_and_move(fbatch, folio, lru_lazyfree);
 	local_unlock(&cpu_fbatches.lock);
 }
 
-- 
2.45.2.803.g4e1b14247a-goog

[PATCH mm-unstable v1 5/5] mm/swap: remove boilerplate

[Yu Zhao]

Remove boilerplate by using a macro to choose the corresponding lock
and handler for each folio_batch in cpu_fbatches.

Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 mm/swap.c | 107 +++++++++++++++++++-----------------------------------
 1 file changed, 37 insertions(+), 70 deletions(-)

diff --git a/mm/swap.c b/mm/swap.c
index 4a66d2f87f26..342ff4e39ba4 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -220,16 +220,45 @@ static void folio_batch_move_lru(struct folio_batch *fbatch, move_fn_t move_fn)
 	folios_put(fbatch);
 }
 
-static void folio_batch_add_and_move(struct folio_batch *fbatch,
-		struct folio *folio, move_fn_t move_fn)
+static void __folio_batch_add_and_move(struct folio_batch *fbatch,
+		struct folio *folio, move_fn_t move_fn,
+		bool on_lru, bool disable_irq)
 {
+	unsigned long flags;
+
+	folio_get(folio);
+
+	if (on_lru && !folio_test_clear_lru(folio)) {
+		folio_put(folio);
+		return;
+	}
+
 	if (folio_batch_add(fbatch, folio) && !folio_test_large(folio) &&
 	    !lru_cache_disabled())
 		return;
 
+	if (disable_irq)
+		local_lock_irqsave(&cpu_fbatches.lock_irq, flags);
+	else
+		local_lock(&cpu_fbatches.lock);
+
 	folio_batch_move_lru(fbatch, move_fn);
+
+	if (disable_irq)
+		local_unlock_irqrestore(&cpu_fbatches.lock_irq, flags);
+	else
+		local_unlock(&cpu_fbatches.lock);
 }
 
+#define folio_batch_add_and_move(folio, op, on_lru)						\
+	__folio_batch_add_and_move(								\
+		this_cpu_ptr(&cpu_fbatches.op),							\
+		folio,										\
+		op,										\
+		on_lru,										\
+		offsetof(struct cpu_fbatches, op) > offsetof(struct cpu_fbatches, lock_irq)	\
+	)
+
 static void lru_move_tail(struct lruvec *lruvec, struct folio *folio)
 {
 	if (folio_test_unevictable(folio))
@@ -250,23 +279,11 @@ static void lru_move_tail(struct lruvec *lruvec, struct folio *folio)
  */
 void folio_rotate_reclaimable(struct folio *folio)
 {
-	struct folio_batch *fbatch;
-	unsigned long flags;
-
 	if (folio_test_locked(folio) || folio_test_dirty(folio) ||
 	    folio_test_unevictable(folio))
 		return;
 
-	folio_get(folio);
-	if (!folio_test_clear_lru(folio)) {
-		folio_put(folio);
-		return;
-	}
-
-	local_lock_irqsave(&cpu_fbatches.lock_irq, flags);
-	fbatch = this_cpu_ptr(&cpu_fbatches.lru_move_tail);
-	folio_batch_add_and_move(fbatch, folio, lru_move_tail);
-	local_unlock_irqrestore(&cpu_fbatches.lock_irq, flags);
+	folio_batch_add_and_move(folio, lru_move_tail, true);
 }
 
 void lru_note_cost(struct lruvec *lruvec, bool file,
@@ -355,21 +372,10 @@ static void folio_activate_drain(int cpu)
 
 void folio_activate(struct folio *folio)
 {
-	struct folio_batch *fbatch;
-
 	if (folio_test_active(folio) || folio_test_unevictable(folio))
 		return;
 
-	folio_get(folio);
-	if (!folio_test_clear_lru(folio)) {
-		folio_put(folio);
-		return;
-	}
-
-	local_lock(&cpu_fbatches.lock);
-	fbatch = this_cpu_ptr(&cpu_fbatches.lru_activate);
-	folio_batch_add_and_move(fbatch, folio, lru_activate);
-	local_unlock(&cpu_fbatches.lock);
+	folio_batch_add_and_move(folio, lru_activate, true);
 }
 
 #else
@@ -513,8 +519,6 @@ EXPORT_SYMBOL(folio_mark_accessed);
  */
 void folio_add_lru(struct folio *folio)
 {
-	struct folio_batch *fbatch;
-
 	VM_BUG_ON_FOLIO(folio_test_active(folio) &&
 			folio_test_unevictable(folio), folio);
 	VM_BUG_ON_FOLIO(folio_test_lru(folio), folio);
@@ -524,11 +528,7 @@ void folio_add_lru(struct folio *folio)
 	    lru_gen_in_fault() && !(current->flags & PF_MEMALLOC))
 		folio_set_active(folio);
 
-	folio_get(folio);
-	local_lock(&cpu_fbatches.lock);
-	fbatch = this_cpu_ptr(&cpu_fbatches.lru_add);
-	folio_batch_add_and_move(fbatch, folio, lru_add);
-	local_unlock(&cpu_fbatches.lock);
+	folio_batch_add_and_move(folio, lru_add, false);
 }
 EXPORT_SYMBOL(folio_add_lru);
 
@@ -702,22 +702,11 @@ void lru_add_drain_cpu(int cpu)
  */
 void deactivate_file_folio(struct folio *folio)
 {
-	struct folio_batch *fbatch;
-
 	/* Deactivating an unevictable folio will not accelerate reclaim */
 	if (folio_test_unevictable(folio))
 		return;
 
-	folio_get(folio);
-	if (!folio_test_clear_lru(folio)) {
-		folio_put(folio);
-		return;
-	}
-
-	local_lock(&cpu_fbatches.lock);
-	fbatch = this_cpu_ptr(&cpu_fbatches.lru_deactivate_file);
-	folio_batch_add_and_move(fbatch, folio, lru_deactivate_file);
-	local_unlock(&cpu_fbatches.lock);
+	folio_batch_add_and_move(folio, lru_deactivate_file, true);
 }
 
 /*
@@ -730,21 +719,10 @@ void deactivate_file_folio(struct folio *folio)
  */
 void folio_deactivate(struct folio *folio)
 {
-	struct folio_batch *fbatch;
-
 	if (folio_test_unevictable(folio) || !(folio_test_active(folio) || lru_gen_enabled()))
 		return;
 
-	folio_get(folio);
-	if (!folio_test_clear_lru(folio)) {
-		folio_put(folio);
-		return;
-	}
-
-	local_lock(&cpu_fbatches.lock);
-	fbatch = this_cpu_ptr(&cpu_fbatches.lru_deactivate);
-	folio_batch_add_and_move(fbatch, folio, lru_deactivate);
-	local_unlock(&cpu_fbatches.lock);
+	folio_batch_add_and_move(folio, lru_deactivate, true);
 }
 
 /**
@@ -756,22 +734,11 @@ void folio_deactivate(struct folio *folio)
  */
 void folio_mark_lazyfree(struct folio *folio)
 {
-	struct folio_batch *fbatch;
-
 	if (!folio_test_anon(folio) || !folio_test_swapbacked(folio) ||
 	    folio_test_swapcache(folio) || folio_test_unevictable(folio))
 		return;
 
-	folio_get(folio);
-	if (!folio_test_clear_lru(folio)) {
-		folio_put(folio);
-		return;
-	}
-
-	local_lock(&cpu_fbatches.lock);
-	fbatch = this_cpu_ptr(&cpu_fbatches.lru_lazyfree);
-	folio_batch_add_and_move(fbatch, folio, lru_lazyfree);
-	local_unlock(&cpu_fbatches.lock);
+	folio_batch_add_and_move(folio, lru_lazyfree, true);
 }
 
 void lru_add_drain(void)
-- 
2.45.2.803.g4e1b14247a-goog

Re: [PATCH mm-unstable v1 5/5] mm/swap: remove boilerplate

[Barry Song]

On Thu, Jul 11, 2024 at 2:15 PM Yu Zhao <yuzhao@google.com> wrote:
>
> Remove boilerplate by using a macro to choose the corresponding lock
> and handler for each folio_batch in cpu_fbatches.
>
> Signed-off-by: Yu Zhao <yuzhao@google.com>
> ---
>  mm/swap.c | 107 +++++++++++++++++++-----------------------------------
>  1 file changed, 37 insertions(+), 70 deletions(-)
>
> diff --git a/mm/swap.c b/mm/swap.c
> index 4a66d2f87f26..342ff4e39ba4 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -220,16 +220,45 @@ static void folio_batch_move_lru(struct folio_batch *fbatch, move_fn_t move_fn)
>         folios_put(fbatch);
>  }
>
> -static void folio_batch_add_and_move(struct folio_batch *fbatch,
> -               struct folio *folio, move_fn_t move_fn)
> +static void __folio_batch_add_and_move(struct folio_batch *fbatch,
> +               struct folio *folio, move_fn_t move_fn,
> +               bool on_lru, bool disable_irq)
>  {
> +       unsigned long flags;
> +
> +       folio_get(folio);
> +
> +       if (on_lru && !folio_test_clear_lru(folio)) {
> +               folio_put(folio);
> +               return;
> +       }
> +
>         if (folio_batch_add(fbatch, folio) && !folio_test_large(folio) &&
>             !lru_cache_disabled())
>                 return;
>
> +       if (disable_irq)
> +               local_lock_irqsave(&cpu_fbatches.lock_irq, flags);
> +       else
> +               local_lock(&cpu_fbatches.lock);
> +
>         folio_batch_move_lru(fbatch, move_fn);
> +
> +       if (disable_irq)
> +               local_unlock_irqrestore(&cpu_fbatches.lock_irq, flags);
> +       else
> +               local_unlock(&cpu_fbatches.lock);
>  }
>
> +#define folio_batch_add_and_move(folio, op, on_lru)                                            \
> +       __folio_batch_add_and_move(                                                             \
> +               this_cpu_ptr(&cpu_fbatches.op),                                                 \
> +               folio,                                                                          \
> +               op,                                                                             \
> +               on_lru,                                                                         \
> +               offsetof(struct cpu_fbatches, op) > offsetof(struct cpu_fbatches, lock_irq)     \
> +       )

I am running into this BUG, is it relevant?

/ # [   64.908801] check_preemption_disabled: 1804 callbacks suppressed
[   64.908915] BUG: using smp_processor_id() in preemptible [00000000]
code: jbd2/vda-8/96
[   64.909912] caller is debug_smp_processor_id+0x20/0x30
[   64.911743] CPU: 0 UID: 0 PID: 96 Comm: jbd2/vda-8 Not tainted
6.10.0-gef32eccacce2 #59
[   64.912373] Hardware name: linux,dummy-virt (DT)
[   64.912741] Call trace:
[   64.913048]  dump_backtrace+0x9c/0x100
[   64.913414]  show_stack+0x20/0x38
[   64.913761]  dump_stack_lvl+0xc4/0x150
[   64.914197]  dump_stack+0x18/0x28
[   64.914557]  check_preemption_disabled+0xd8/0x120
[   64.914944]  debug_smp_processor_id+0x20/0x30
[   64.915321]  folio_add_lru+0x30/0xa8
[   64.915680]  filemap_add_folio+0xe4/0x118
[   64.916082]  __filemap_get_folio+0x178/0x450
[   64.916455]  __getblk_slow+0xb0/0x310
[   64.916816]  bdev_getblk+0x94/0xc0
[   64.917169]  jbd2_journal_get_descriptor_buffer+0x6c/0x1b0
[   64.917590]  jbd2_journal_commit_transaction+0x7f0/0x1c88
[   64.917994]  kjournald2+0xd4/0x278
[   64.918344]  kthread+0x11c/0x128
[   64.918693]  ret_from_fork+0x10/0x20
[   64.928277] BUG: using smp_processor_id() in preemptible [00000000]
code: jbd2/vda-8/96
[   64.928878] caller is debug_smp_processor_id+0x20/0x30
[   64.929381] CPU: 0 UID: 0 PID: 96 Comm: jbd2/vda-8 Not tainted
6.10.0-gef32eccacce2 #59
[   64.929886] Hardware name: linux,dummy-virt (DT)
[   64.930252] Call trace:
[   64.930544]  dump_backtrace+0x9c/0x100
[   64.930907]  show_stack+0x20/0x38
[   64.931255]  dump_stack_lvl+0xc4/0x150
[   64.931616]  dump_stack+0x18/0x28
[   64.932022]  check_preemption_disabled+0xd8/0x120
[   64.932486]  debug_smp_processor_id+0x20/0x30
[   64.933023]  folio_add_lru+0x30/0xa8
[   64.933523]  filemap_add_folio+0xe4/0x118
[   64.933892]  __filemap_get_folio+0x178/0x450
[   64.934265]  __getblk_slow+0xb0/0x310
[   64.934626]  bdev_getblk+0x94/0xc0
[   64.934977]  jbd2_journal_get_descriptor_buffer+0x6c/0x1b0
[   64.935418]  journal_submit_commit_record.part.0.constprop.0+0x48/0x288
[   64.935919]  jbd2_journal_commit_transaction+0x1590/0x1c88
[   64.936519]  kjournald2+0xd4/0x278
[   64.936908]  kthread+0x11c/0x128
[   64.937323]  ret_from_fork+0x10/0x20

> +
>  static void lru_move_tail(struct lruvec *lruvec, struct folio *folio)
>  {
>         if (folio_test_unevictable(folio))
> @@ -250,23 +279,11 @@ static void lru_move_tail(struct lruvec *lruvec, struct folio *folio)
>   */
>  void folio_rotate_reclaimable(struct folio *folio)
>  {
> -       struct folio_batch *fbatch;
> -       unsigned long flags;
> -
>         if (folio_test_locked(folio) || folio_test_dirty(folio) ||
>             folio_test_unevictable(folio))
>                 return;
>
> -       folio_get(folio);
> -       if (!folio_test_clear_lru(folio)) {
> -               folio_put(folio);
> -               return;
> -       }
> -
> -       local_lock_irqsave(&cpu_fbatches.lock_irq, flags);
> -       fbatch = this_cpu_ptr(&cpu_fbatches.lru_move_tail);
> -       folio_batch_add_and_move(fbatch, folio, lru_move_tail);
> -       local_unlock_irqrestore(&cpu_fbatches.lock_irq, flags);
> +       folio_batch_add_and_move(folio, lru_move_tail, true);
>  }
>
>  void lru_note_cost(struct lruvec *lruvec, bool file,
> @@ -355,21 +372,10 @@ static void folio_activate_drain(int cpu)
>
>  void folio_activate(struct folio *folio)
>  {
> -       struct folio_batch *fbatch;
> -
>         if (folio_test_active(folio) || folio_test_unevictable(folio))
>                 return;
>
> -       folio_get(folio);
> -       if (!folio_test_clear_lru(folio)) {
> -               folio_put(folio);
> -               return;
> -       }
> -
> -       local_lock(&cpu_fbatches.lock);
> -       fbatch = this_cpu_ptr(&cpu_fbatches.lru_activate);
> -       folio_batch_add_and_move(fbatch, folio, lru_activate);
> -       local_unlock(&cpu_fbatches.lock);
> +       folio_batch_add_and_move(folio, lru_activate, true);
>  }
>
>  #else
> @@ -513,8 +519,6 @@ EXPORT_SYMBOL(folio_mark_accessed);
>   */
>  void folio_add_lru(struct folio *folio)
>  {
> -       struct folio_batch *fbatch;
> -
>         VM_BUG_ON_FOLIO(folio_test_active(folio) &&
>                         folio_test_unevictable(folio), folio);
>         VM_BUG_ON_FOLIO(folio_test_lru(folio), folio);
> @@ -524,11 +528,7 @@ void folio_add_lru(struct folio *folio)
>             lru_gen_in_fault() && !(current->flags & PF_MEMALLOC))
>                 folio_set_active(folio);
>
> -       folio_get(folio);
> -       local_lock(&cpu_fbatches.lock);
> -       fbatch = this_cpu_ptr(&cpu_fbatches.lru_add);
> -       folio_batch_add_and_move(fbatch, folio, lru_add);
> -       local_unlock(&cpu_fbatches.lock);
> +       folio_batch_add_and_move(folio, lru_add, false);
>  }
>  EXPORT_SYMBOL(folio_add_lru);
>
> @@ -702,22 +702,11 @@ void lru_add_drain_cpu(int cpu)
>   */
>  void deactivate_file_folio(struct folio *folio)
>  {
> -       struct folio_batch *fbatch;
> -
>         /* Deactivating an unevictable folio will not accelerate reclaim */
>         if (folio_test_unevictable(folio))
>                 return;
>
> -       folio_get(folio);
> -       if (!folio_test_clear_lru(folio)) {
> -               folio_put(folio);
> -               return;
> -       }
> -
> -       local_lock(&cpu_fbatches.lock);
> -       fbatch = this_cpu_ptr(&cpu_fbatches.lru_deactivate_file);
> -       folio_batch_add_and_move(fbatch, folio, lru_deactivate_file);
> -       local_unlock(&cpu_fbatches.lock);
> +       folio_batch_add_and_move(folio, lru_deactivate_file, true);
>  }
>
>  /*
> @@ -730,21 +719,10 @@ void deactivate_file_folio(struct folio *folio)
>   */
>  void folio_deactivate(struct folio *folio)
>  {
> -       struct folio_batch *fbatch;
> -
>         if (folio_test_unevictable(folio) || !(folio_test_active(folio) || lru_gen_enabled()))
>                 return;
>
> -       folio_get(folio);
> -       if (!folio_test_clear_lru(folio)) {
> -               folio_put(folio);
> -               return;
> -       }
> -
> -       local_lock(&cpu_fbatches.lock);
> -       fbatch = this_cpu_ptr(&cpu_fbatches.lru_deactivate);
> -       folio_batch_add_and_move(fbatch, folio, lru_deactivate);
> -       local_unlock(&cpu_fbatches.lock);
> +       folio_batch_add_and_move(folio, lru_deactivate, true);
>  }
>
>  /**
> @@ -756,22 +734,11 @@ void folio_deactivate(struct folio *folio)
>   */
>  void folio_mark_lazyfree(struct folio *folio)
>  {
> -       struct folio_batch *fbatch;
> -
>         if (!folio_test_anon(folio) || !folio_test_swapbacked(folio) ||
>             folio_test_swapcache(folio) || folio_test_unevictable(folio))
>                 return;
>
> -       folio_get(folio);
> -       if (!folio_test_clear_lru(folio)) {
> -               folio_put(folio);
> -               return;
> -       }
> -
> -       local_lock(&cpu_fbatches.lock);
> -       fbatch = this_cpu_ptr(&cpu_fbatches.lru_lazyfree);
> -       folio_batch_add_and_move(fbatch, folio, lru_lazyfree);
> -       local_unlock(&cpu_fbatches.lock);
> +       folio_batch_add_and_move(folio, lru_lazyfree, true);
>  }
>
>  void lru_add_drain(void)
> --
> 2.45.2.803.g4e1b14247a-goog
>
>

Thanks
Barry

Re: [PATCH mm-unstable v1 5/5] mm/swap: remove boilerplate

[Barry Song]

On Fri, Jul 26, 2024 at 5:48 PM Barry Song <21cnbao@gmail.com> wrote:
>
> On Thu, Jul 11, 2024 at 2:15 PM Yu Zhao <yuzhao@google.com> wrote:
> >
> > Remove boilerplate by using a macro to choose the corresponding lock
> > and handler for each folio_batch in cpu_fbatches.
> >
> > Signed-off-by: Yu Zhao <yuzhao@google.com>
> > ---
> >  mm/swap.c | 107 +++++++++++++++++++-----------------------------------
> >  1 file changed, 37 insertions(+), 70 deletions(-)
> >
> > diff --git a/mm/swap.c b/mm/swap.c
> > index 4a66d2f87f26..342ff4e39ba4 100644
> > --- a/mm/swap.c
> > +++ b/mm/swap.c
> > @@ -220,16 +220,45 @@ static void folio_batch_move_lru(struct folio_batch *fbatch, move_fn_t move_fn)
> >         folios_put(fbatch);
> >  }
> >
> > -static void folio_batch_add_and_move(struct folio_batch *fbatch,
> > -               struct folio *folio, move_fn_t move_fn)
> > +static void __folio_batch_add_and_move(struct folio_batch *fbatch,
> > +               struct folio *folio, move_fn_t move_fn,
> > +               bool on_lru, bool disable_irq)
> >  {
> > +       unsigned long flags;
> > +
> > +       folio_get(folio);
> > +
> > +       if (on_lru && !folio_test_clear_lru(folio)) {
> > +               folio_put(folio);
> > +               return;
> > +       }
> > +
> >         if (folio_batch_add(fbatch, folio) && !folio_test_large(folio) &&
> >             !lru_cache_disabled())
> >                 return;
> >
> > +       if (disable_irq)
> > +               local_lock_irqsave(&cpu_fbatches.lock_irq, flags);
> > +       else
> > +               local_lock(&cpu_fbatches.lock);
> > +
> >         folio_batch_move_lru(fbatch, move_fn);
> > +
> > +       if (disable_irq)
> > +               local_unlock_irqrestore(&cpu_fbatches.lock_irq, flags);
> > +       else
> > +               local_unlock(&cpu_fbatches.lock);
> >  }
> >
> > +#define folio_batch_add_and_move(folio, op, on_lru)                                            \
> > +       __folio_batch_add_and_move(                                                             \
> > +               this_cpu_ptr(&cpu_fbatches.op),                                                 \
> > +               folio,                                                                          \
> > +               op,                                                                             \
> > +               on_lru,                                                                         \
> > +               offsetof(struct cpu_fbatches, op) > offsetof(struct cpu_fbatches, lock_irq)     \
> > +       )
>
> I am running into this BUG, is it relevant?
>
> / # [   64.908801] check_preemption_disabled: 1804 callbacks suppressed
> [   64.908915] BUG: using smp_processor_id() in preemptible [00000000]
> code: jbd2/vda-8/96
> [   64.909912] caller is debug_smp_processor_id+0x20/0x30
> [   64.911743] CPU: 0 UID: 0 PID: 96 Comm: jbd2/vda-8 Not tainted
> 6.10.0-gef32eccacce2 #59
> [   64.912373] Hardware name: linux,dummy-virt (DT)
> [   64.912741] Call trace:
> [   64.913048]  dump_backtrace+0x9c/0x100
> [   64.913414]  show_stack+0x20/0x38
> [   64.913761]  dump_stack_lvl+0xc4/0x150
> [   64.914197]  dump_stack+0x18/0x28
> [   64.914557]  check_preemption_disabled+0xd8/0x120
> [   64.914944]  debug_smp_processor_id+0x20/0x30
> [   64.915321]  folio_add_lru+0x30/0xa8
> [   64.915680]  filemap_add_folio+0xe4/0x118
> [   64.916082]  __filemap_get_folio+0x178/0x450
> [   64.916455]  __getblk_slow+0xb0/0x310
> [   64.916816]  bdev_getblk+0x94/0xc0
> [   64.917169]  jbd2_journal_get_descriptor_buffer+0x6c/0x1b0
> [   64.917590]  jbd2_journal_commit_transaction+0x7f0/0x1c88
> [   64.917994]  kjournald2+0xd4/0x278
> [   64.918344]  kthread+0x11c/0x128
> [   64.918693]  ret_from_fork+0x10/0x20
> [   64.928277] BUG: using smp_processor_id() in preemptible [00000000]
> code: jbd2/vda-8/96
> [   64.928878] caller is debug_smp_processor_id+0x20/0x30
> [   64.929381] CPU: 0 UID: 0 PID: 96 Comm: jbd2/vda-8 Not tainted
> 6.10.0-gef32eccacce2 #59
> [   64.929886] Hardware name: linux,dummy-virt (DT)
> [   64.930252] Call trace:
> [   64.930544]  dump_backtrace+0x9c/0x100
> [   64.930907]  show_stack+0x20/0x38
> [   64.931255]  dump_stack_lvl+0xc4/0x150
> [   64.931616]  dump_stack+0x18/0x28
> [   64.932022]  check_preemption_disabled+0xd8/0x120
> [   64.932486]  debug_smp_processor_id+0x20/0x30
> [   64.933023]  folio_add_lru+0x30/0xa8
> [   64.933523]  filemap_add_folio+0xe4/0x118
> [   64.933892]  __filemap_get_folio+0x178/0x450
> [   64.934265]  __getblk_slow+0xb0/0x310
> [   64.934626]  bdev_getblk+0x94/0xc0
> [   64.934977]  jbd2_journal_get_descriptor_buffer+0x6c/0x1b0
> [   64.935418]  journal_submit_commit_record.part.0.constprop.0+0x48/0x288
> [   64.935919]  jbd2_journal_commit_transaction+0x1590/0x1c88
> [   64.936519]  kjournald2+0xd4/0x278
> [   64.936908]  kthread+0x11c/0x128
> [   64.937323]  ret_from_fork+0x10/0x20

This removes the BUG complaint, but I'm unsure if it's the correct fix:

diff --git a/mm/swap.c b/mm/swap.c
index 342ff4e39ba4..a2781edeceef 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -252,7 +252,7 @@ static void __folio_batch_add_and_move(struct
folio_batch *fbatch,

 #define folio_batch_add_and_move(folio, op, on_lru)
                         \
        __folio_batch_add_and_move(
                         \
-               this_cpu_ptr(&cpu_fbatches.op),
                         \
+               raw_cpu_ptr(&cpu_fbatches.op),
                         \
                folio,
                         \
                op,
                         \
                on_lru,
                         \

>
> > +
> >  static void lru_move_tail(struct lruvec *lruvec, struct folio *folio)
> >  {
> >         if (folio_test_unevictable(folio))
> > @@ -250,23 +279,11 @@ static void lru_move_tail(struct lruvec *lruvec, struct folio *folio)
> >   */
> >  void folio_rotate_reclaimable(struct folio *folio)
> >  {
> > -       struct folio_batch *fbatch;
> > -       unsigned long flags;
> > -
> >         if (folio_test_locked(folio) || folio_test_dirty(folio) ||
> >             folio_test_unevictable(folio))
> >                 return;
> >
> > -       folio_get(folio);
> > -       if (!folio_test_clear_lru(folio)) {
> > -               folio_put(folio);
> > -               return;
> > -       }
> > -
> > -       local_lock_irqsave(&cpu_fbatches.lock_irq, flags);
> > -       fbatch = this_cpu_ptr(&cpu_fbatches.lru_move_tail);
> > -       folio_batch_add_and_move(fbatch, folio, lru_move_tail);
> > -       local_unlock_irqrestore(&cpu_fbatches.lock_irq, flags);
> > +       folio_batch_add_and_move(folio, lru_move_tail, true);
> >  }
> >
> >  void lru_note_cost(struct lruvec *lruvec, bool file,
> > @@ -355,21 +372,10 @@ static void folio_activate_drain(int cpu)
> >
> >  void folio_activate(struct folio *folio)
> >  {
> > -       struct folio_batch *fbatch;
> > -
> >         if (folio_test_active(folio) || folio_test_unevictable(folio))
> >                 return;
> >
> > -       folio_get(folio);
> > -       if (!folio_test_clear_lru(folio)) {
> > -               folio_put(folio);
> > -               return;
> > -       }
> > -
> > -       local_lock(&cpu_fbatches.lock);
> > -       fbatch = this_cpu_ptr(&cpu_fbatches.lru_activate);
> > -       folio_batch_add_and_move(fbatch, folio, lru_activate);
> > -       local_unlock(&cpu_fbatches.lock);
> > +       folio_batch_add_and_move(folio, lru_activate, true);
> >  }
> >
> >  #else
> > @@ -513,8 +519,6 @@ EXPORT_SYMBOL(folio_mark_accessed);
> >   */
> >  void folio_add_lru(struct folio *folio)
> >  {
> > -       struct folio_batch *fbatch;
> > -
> >         VM_BUG_ON_FOLIO(folio_test_active(folio) &&
> >                         folio_test_unevictable(folio), folio);
> >         VM_BUG_ON_FOLIO(folio_test_lru(folio), folio);
> > @@ -524,11 +528,7 @@ void folio_add_lru(struct folio *folio)
> >             lru_gen_in_fault() && !(current->flags & PF_MEMALLOC))
> >                 folio_set_active(folio);
> >
> > -       folio_get(folio);
> > -       local_lock(&cpu_fbatches.lock);
> > -       fbatch = this_cpu_ptr(&cpu_fbatches.lru_add);
> > -       folio_batch_add_and_move(fbatch, folio, lru_add);
> > -       local_unlock(&cpu_fbatches.lock);
> > +       folio_batch_add_and_move(folio, lru_add, false);
> >  }
> >  EXPORT_SYMBOL(folio_add_lru);
> >
> > @@ -702,22 +702,11 @@ void lru_add_drain_cpu(int cpu)
> >   */
> >  void deactivate_file_folio(struct folio *folio)
> >  {
> > -       struct folio_batch *fbatch;
> > -
> >         /* Deactivating an unevictable folio will not accelerate reclaim */
> >         if (folio_test_unevictable(folio))
> >                 return;
> >
> > -       folio_get(folio);
> > -       if (!folio_test_clear_lru(folio)) {
> > -               folio_put(folio);
> > -               return;
> > -       }
> > -
> > -       local_lock(&cpu_fbatches.lock);
> > -       fbatch = this_cpu_ptr(&cpu_fbatches.lru_deactivate_file);
> > -       folio_batch_add_and_move(fbatch, folio, lru_deactivate_file);
> > -       local_unlock(&cpu_fbatches.lock);
> > +       folio_batch_add_and_move(folio, lru_deactivate_file, true);
> >  }
> >
> >  /*
> > @@ -730,21 +719,10 @@ void deactivate_file_folio(struct folio *folio)
> >   */
> >  void folio_deactivate(struct folio *folio)
> >  {
> > -       struct folio_batch *fbatch;
> > -
> >         if (folio_test_unevictable(folio) || !(folio_test_active(folio) || lru_gen_enabled()))
> >                 return;
> >
> > -       folio_get(folio);
> > -       if (!folio_test_clear_lru(folio)) {
> > -               folio_put(folio);
> > -               return;
> > -       }
> > -
> > -       local_lock(&cpu_fbatches.lock);
> > -       fbatch = this_cpu_ptr(&cpu_fbatches.lru_deactivate);
> > -       folio_batch_add_and_move(fbatch, folio, lru_deactivate);
> > -       local_unlock(&cpu_fbatches.lock);
> > +       folio_batch_add_and_move(folio, lru_deactivate, true);
> >  }
> >
> >  /**
> > @@ -756,22 +734,11 @@ void folio_deactivate(struct folio *folio)
> >   */
> >  void folio_mark_lazyfree(struct folio *folio)
> >  {
> > -       struct folio_batch *fbatch;
> > -
> >         if (!folio_test_anon(folio) || !folio_test_swapbacked(folio) ||
> >             folio_test_swapcache(folio) || folio_test_unevictable(folio))
> >                 return;
> >
> > -       folio_get(folio);
> > -       if (!folio_test_clear_lru(folio)) {
> > -               folio_put(folio);
> > -               return;
> > -       }
> > -
> > -       local_lock(&cpu_fbatches.lock);
> > -       fbatch = this_cpu_ptr(&cpu_fbatches.lru_lazyfree);
> > -       folio_batch_add_and_move(fbatch, folio, lru_lazyfree);
> > -       local_unlock(&cpu_fbatches.lock);
> > +       folio_batch_add_and_move(folio, lru_lazyfree, true);
> >  }
> >
> >  void lru_add_drain(void)
> > --
> > 2.45.2.803.g4e1b14247a-goog
> >
> >
>
> Thanks
> Barry

Re: [PATCH mm-unstable v1 5/5] mm/swap: remove boilerplate

[Yu Zhao]

On Fri, Jul 26, 2024 at 05:56:10PM +1200, Barry Song wrote:
> On Fri, Jul 26, 2024 at 5:48 PM Barry Song <21cnbao@gmail.com> wrote:
> >
> > On Thu, Jul 11, 2024 at 2:15 PM Yu Zhao <yuzhao@google.com> wrote:
> > >
> > > Remove boilerplate by using a macro to choose the corresponding lock
> > > and handler for each folio_batch in cpu_fbatches.
> > >
> > > Signed-off-by: Yu Zhao <yuzhao@google.com>
> > > ---
> > >  mm/swap.c | 107 +++++++++++++++++++-----------------------------------
> > >  1 file changed, 37 insertions(+), 70 deletions(-)
> > >
> > > diff --git a/mm/swap.c b/mm/swap.c
> > > index 4a66d2f87f26..342ff4e39ba4 100644
> > > --- a/mm/swap.c
> > > +++ b/mm/swap.c
> > > @@ -220,16 +220,45 @@ static void folio_batch_move_lru(struct folio_batch *fbatch, move_fn_t move_fn)
> > >         folios_put(fbatch);
> > >  }
> > >
> > > -static void folio_batch_add_and_move(struct folio_batch *fbatch,
> > > -               struct folio *folio, move_fn_t move_fn)
> > > +static void __folio_batch_add_and_move(struct folio_batch *fbatch,
> > > +               struct folio *folio, move_fn_t move_fn,
> > > +               bool on_lru, bool disable_irq)
> > >  {
> > > +       unsigned long flags;
> > > +
> > > +       folio_get(folio);
> > > +
> > > +       if (on_lru && !folio_test_clear_lru(folio)) {
> > > +               folio_put(folio);
> > > +               return;
> > > +       }
> > > +
> > >         if (folio_batch_add(fbatch, folio) && !folio_test_large(folio) &&
> > >             !lru_cache_disabled())
> > >                 return;
> > >
> > > +       if (disable_irq)
> > > +               local_lock_irqsave(&cpu_fbatches.lock_irq, flags);
> > > +       else
> > > +               local_lock(&cpu_fbatches.lock);
> > > +
> > >         folio_batch_move_lru(fbatch, move_fn);
> > > +
> > > +       if (disable_irq)
> > > +               local_unlock_irqrestore(&cpu_fbatches.lock_irq, flags);
> > > +       else
> > > +               local_unlock(&cpu_fbatches.lock);
> > >  }
> > >
> > > +#define folio_batch_add_and_move(folio, op, on_lru)                                            \
> > > +       __folio_batch_add_and_move(                                                             \
> > > +               this_cpu_ptr(&cpu_fbatches.op),                                                 \
> > > +               folio,                                                                          \
> > > +               op,                                                                             \
> > > +               on_lru,                                                                         \
> > > +               offsetof(struct cpu_fbatches, op) > offsetof(struct cpu_fbatches, lock_irq)     \
> > > +       )
> >
> > I am running into this BUG, is it relevant?

Sorry for the trouble.

> > / # [   64.908801] check_preemption_disabled: 1804 callbacks suppressed
> > [   64.908915] BUG: using smp_processor_id() in preemptible [00000000]
> > code: jbd2/vda-8/96
> > [   64.909912] caller is debug_smp_processor_id+0x20/0x30
> > [   64.911743] CPU: 0 UID: 0 PID: 96 Comm: jbd2/vda-8 Not tainted
> > 6.10.0-gef32eccacce2 #59
> > [   64.912373] Hardware name: linux,dummy-virt (DT)
> > [   64.912741] Call trace:
> > [   64.913048]  dump_backtrace+0x9c/0x100
> > [   64.913414]  show_stack+0x20/0x38
> > [   64.913761]  dump_stack_lvl+0xc4/0x150
> > [   64.914197]  dump_stack+0x18/0x28
> > [   64.914557]  check_preemption_disabled+0xd8/0x120
> > [   64.914944]  debug_smp_processor_id+0x20/0x30
> > [   64.915321]  folio_add_lru+0x30/0xa8
> > [   64.915680]  filemap_add_folio+0xe4/0x118
> > [   64.916082]  __filemap_get_folio+0x178/0x450
> > [   64.916455]  __getblk_slow+0xb0/0x310
> > [   64.916816]  bdev_getblk+0x94/0xc0
> > [   64.917169]  jbd2_journal_get_descriptor_buffer+0x6c/0x1b0
> > [   64.917590]  jbd2_journal_commit_transaction+0x7f0/0x1c88
> > [   64.917994]  kjournald2+0xd4/0x278
> > [   64.918344]  kthread+0x11c/0x128
> > [   64.918693]  ret_from_fork+0x10/0x20
> 
> This removes the BUG complaint, but I'm unsure if it's the correct fix:

Below is the proper fix. Will post v2.

--- a/mm/swap.c
+++ b/mm/swap.c
@@ -221,7 +221,7 @@ static void folio_batch_move_lru(struct folio_batch *fbatch, move_fn_t move_fn)
 	folios_put(fbatch);
 }
 
-static void __folio_batch_add_and_move(struct folio_batch *fbatch,
+static void __folio_batch_add_and_move(struct folio_batch __percpu *fbatch,
 		struct folio *folio, move_fn_t move_fn,
 		bool on_lru, bool disable_irq)
 {
@@ -234,16 +234,14 @@ static void __folio_batch_add_and_move(struct folio_batch *fbatch,
 		return;
 	}
 
-	if (folio_batch_add(fbatch, folio) && !folio_test_large(folio) &&
-	    !lru_cache_disabled())
-		return;
-
 	if (disable_irq)
 		local_lock_irqsave(&cpu_fbatches.lock_irq, flags);
 	else
 		local_lock(&cpu_fbatches.lock);
 
-	folio_batch_move_lru(fbatch, move_fn);
+	if (!folio_batch_add(this_cpu_ptr(fbatch), folio) || folio_test_large(folio) ||
+	    lru_cache_disabled())
+		folio_batch_move_lru(this_cpu_ptr(fbatch), move_fn);
 
 	if (disable_irq)
 		local_unlock_irqrestore(&cpu_fbatches.lock_irq, flags);
@@ -253,7 +251,7 @@ static void __folio_batch_add_and_move(struct folio_batch *fbatch,
 
 #define folio_batch_add_and_move(folio, op, on_lru)						\
 	__folio_batch_add_and_move(								\
-		this_cpu_ptr(&cpu_fbatches.op),							\
+		&cpu_fbatches.op,								\
 		folio,										\
 		op,										\
 		on_lru,										\

Re: [PATCH mm-unstable v1 5/5] mm/swap: remove boilerplate

[Hugh Dickins]

On Wed, 10 Jul 2024, Yu Zhao wrote:

> Remove boilerplate by using a macro to choose the corresponding lock
> and handler for each folio_batch in cpu_fbatches.
> 
> Signed-off-by: Yu Zhao <yuzhao@google.com>

Andrew, please revert this "remove boilerplate" patch (and of course its
followup fix) from mm-unstable. From the title I presume it was intended
to make no functional change, but that's far from so.

Under tmpfs swapping load, on different runs I see various badnesses:
"Bad page" in __free_one_page(), Oops in __run_timer_base(),
WARNING at kernel/workqueue.c:790 in set_work_data(), PageBuddy BUG
at page-flags.h:1009 from __del_page_from_freelist(), something (I'd
given up taking better notes by this time) in __queue_work(), others.

All those were including the fix to Barry's report: without that fix,
the boot is drowned in warnings scrolling past too fast to be read.

(All the above were on the HP workstation, swapping to SSD; whereas on
this ThinkPad, swapping to NVMe, no problem seen at all - I mention the
swapping medium, but have no idea whether that's a relevant difference.
In each case, MGLRU compiled in but not enabled. THPs and 64kTHPs active.)

Sorry, but I've put no effort whatsoever into debugging this: "remove
boilerplate" didn't seem worth the effort, and my personal preference
is for readable boilerplate over hiding in a macro.  If you prefer the
macro, I expect Yu can soon come up with a fix (which I could test here):
but for now please revert "remove boilerplate", since its issues get in
the way of further mm testing.

Thanks,
Hugh

> ---
>  mm/swap.c | 107 +++++++++++++++++++-----------------------------------
>  1 file changed, 37 insertions(+), 70 deletions(-)
> 
> diff --git a/mm/swap.c b/mm/swap.c
> index 4a66d2f87f26..342ff4e39ba4 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -220,16 +220,45 @@ static void folio_batch_move_lru(struct folio_batch *fbatch, move_fn_t move_fn)
>  	folios_put(fbatch);
>  }
>  
> -static void folio_batch_add_and_move(struct folio_batch *fbatch,
> -		struct folio *folio, move_fn_t move_fn)
> +static void __folio_batch_add_and_move(struct folio_batch *fbatch,
> +		struct folio *folio, move_fn_t move_fn,
> +		bool on_lru, bool disable_irq)
>  {
> +	unsigned long flags;
> +
> +	folio_get(folio);
> +
> +	if (on_lru && !folio_test_clear_lru(folio)) {
> +		folio_put(folio);
> +		return;
> +	}
> +
>  	if (folio_batch_add(fbatch, folio) && !folio_test_large(folio) &&
>  	    !lru_cache_disabled())
>  		return;
>  
> +	if (disable_irq)
> +		local_lock_irqsave(&cpu_fbatches.lock_irq, flags);
> +	else
> +		local_lock(&cpu_fbatches.lock);
> +
>  	folio_batch_move_lru(fbatch, move_fn);
> +
> +	if (disable_irq)
> +		local_unlock_irqrestore(&cpu_fbatches.lock_irq, flags);
> +	else
> +		local_unlock(&cpu_fbatches.lock);
>  }
>  
> +#define folio_batch_add_and_move(folio, op, on_lru)						\
> +	__folio_batch_add_and_move(								\
> +		this_cpu_ptr(&cpu_fbatches.op),							\
> +		folio,										\
> +		op,										\
> +		on_lru,										\
> +		offsetof(struct cpu_fbatches, op) > offsetof(struct cpu_fbatches, lock_irq)	\
> +	)
> +
>  static void lru_move_tail(struct lruvec *lruvec, struct folio *folio)
>  {
>  	if (folio_test_unevictable(folio))
> @@ -250,23 +279,11 @@ static void lru_move_tail(struct lruvec *lruvec, struct folio *folio)
>   */
>  void folio_rotate_reclaimable(struct folio *folio)
>  {
> -	struct folio_batch *fbatch;
> -	unsigned long flags;
> -
>  	if (folio_test_locked(folio) || folio_test_dirty(folio) ||
>  	    folio_test_unevictable(folio))
>  		return;
>  
> -	folio_get(folio);
> -	if (!folio_test_clear_lru(folio)) {
> -		folio_put(folio);
> -		return;
> -	}
> -
> -	local_lock_irqsave(&cpu_fbatches.lock_irq, flags);
> -	fbatch = this_cpu_ptr(&cpu_fbatches.lru_move_tail);
> -	folio_batch_add_and_move(fbatch, folio, lru_move_tail);
> -	local_unlock_irqrestore(&cpu_fbatches.lock_irq, flags);
> +	folio_batch_add_and_move(folio, lru_move_tail, true);
>  }
>  
>  void lru_note_cost(struct lruvec *lruvec, bool file,
> @@ -355,21 +372,10 @@ static void folio_activate_drain(int cpu)
>  
>  void folio_activate(struct folio *folio)
>  {
> -	struct folio_batch *fbatch;
> -
>  	if (folio_test_active(folio) || folio_test_unevictable(folio))
>  		return;
>  
> -	folio_get(folio);
> -	if (!folio_test_clear_lru(folio)) {
> -		folio_put(folio);
> -		return;
> -	}
> -
> -	local_lock(&cpu_fbatches.lock);
> -	fbatch = this_cpu_ptr(&cpu_fbatches.lru_activate);
> -	folio_batch_add_and_move(fbatch, folio, lru_activate);
> -	local_unlock(&cpu_fbatches.lock);
> +	folio_batch_add_and_move(folio, lru_activate, true);
>  }
>  
>  #else
> @@ -513,8 +519,6 @@ EXPORT_SYMBOL(folio_mark_accessed);
>   */
>  void folio_add_lru(struct folio *folio)
>  {
> -	struct folio_batch *fbatch;
> -
>  	VM_BUG_ON_FOLIO(folio_test_active(folio) &&
>  			folio_test_unevictable(folio), folio);
>  	VM_BUG_ON_FOLIO(folio_test_lru(folio), folio);
> @@ -524,11 +528,7 @@ void folio_add_lru(struct folio *folio)
>  	    lru_gen_in_fault() && !(current->flags & PF_MEMALLOC))
>  		folio_set_active(folio);
>  
> -	folio_get(folio);
> -	local_lock(&cpu_fbatches.lock);
> -	fbatch = this_cpu_ptr(&cpu_fbatches.lru_add);
> -	folio_batch_add_and_move(fbatch, folio, lru_add);
> -	local_unlock(&cpu_fbatches.lock);
> +	folio_batch_add_and_move(folio, lru_add, false);
>  }
>  EXPORT_SYMBOL(folio_add_lru);
>  
> @@ -702,22 +702,11 @@ void lru_add_drain_cpu(int cpu)
>   */
>  void deactivate_file_folio(struct folio *folio)
>  {
> -	struct folio_batch *fbatch;
> -
>  	/* Deactivating an unevictable folio will not accelerate reclaim */
>  	if (folio_test_unevictable(folio))
>  		return;
>  
> -	folio_get(folio);
> -	if (!folio_test_clear_lru(folio)) {
> -		folio_put(folio);
> -		return;
> -	}
> -
> -	local_lock(&cpu_fbatches.lock);
> -	fbatch = this_cpu_ptr(&cpu_fbatches.lru_deactivate_file);
> -	folio_batch_add_and_move(fbatch, folio, lru_deactivate_file);
> -	local_unlock(&cpu_fbatches.lock);
> +	folio_batch_add_and_move(folio, lru_deactivate_file, true);
>  }
>  
>  /*
> @@ -730,21 +719,10 @@ void deactivate_file_folio(struct folio *folio)
>   */
>  void folio_deactivate(struct folio *folio)
>  {
> -	struct folio_batch *fbatch;
> -
>  	if (folio_test_unevictable(folio) || !(folio_test_active(folio) || lru_gen_enabled()))
>  		return;
>  
> -	folio_get(folio);
> -	if (!folio_test_clear_lru(folio)) {
> -		folio_put(folio);
> -		return;
> -	}
> -
> -	local_lock(&cpu_fbatches.lock);
> -	fbatch = this_cpu_ptr(&cpu_fbatches.lru_deactivate);
> -	folio_batch_add_and_move(fbatch, folio, lru_deactivate);
> -	local_unlock(&cpu_fbatches.lock);
> +	folio_batch_add_and_move(folio, lru_deactivate, true);
>  }
>  
>  /**
> @@ -756,22 +734,11 @@ void folio_deactivate(struct folio *folio)
>   */
>  void folio_mark_lazyfree(struct folio *folio)
>  {
> -	struct folio_batch *fbatch;
> -
>  	if (!folio_test_anon(folio) || !folio_test_swapbacked(folio) ||
>  	    folio_test_swapcache(folio) || folio_test_unevictable(folio))
>  		return;
>  
> -	folio_get(folio);
> -	if (!folio_test_clear_lru(folio)) {
> -		folio_put(folio);
> -		return;
> -	}
> -
> -	local_lock(&cpu_fbatches.lock);
> -	fbatch = this_cpu_ptr(&cpu_fbatches.lru_lazyfree);
> -	folio_batch_add_and_move(fbatch, folio, lru_lazyfree);
> -	local_unlock(&cpu_fbatches.lock);
> +	folio_batch_add_and_move(folio, lru_lazyfree, true);
>  }
>  
>  void lru_add_drain(void)
> -- 
> 2.45.2.803.g4e1b14247a-goog

Re: [PATCH mm-unstable v1 5/5] mm/swap: remove boilerplate

[Yu Zhao]

On Sat, Aug 03, 2024 at 11:55:51PM -0700, Hugh Dickins wrote:
> On Wed, 10 Jul 2024, Yu Zhao wrote:
> 
> > Remove boilerplate by using a macro to choose the corresponding lock
> > and handler for each folio_batch in cpu_fbatches.
> > 
> > Signed-off-by: Yu Zhao <yuzhao@google.com>
> 
> Andrew, please revert this "remove boilerplate" patch (and of course its
> followup fix) from mm-unstable. From the title I presume it was intended
> to make no functional change, but that's far from so.
> 
> Under tmpfs swapping load, on different runs I see various badnesses:
> "Bad page" in __free_one_page(), Oops in __run_timer_base(),
> WARNING at kernel/workqueue.c:790 in set_work_data(), PageBuddy BUG
> at page-flags.h:1009 from __del_page_from_freelist(), something (I'd
> given up taking better notes by this time) in __queue_work(), others.
> 
> All those were including the fix to Barry's report: without that fix,
> the boot is drowned in warnings scrolling past too fast to be read.
> 
> (All the above were on the HP workstation, swapping to SSD; whereas on
> this ThinkPad, swapping to NVMe, no problem seen at all - I mention the
> swapping medium, but have no idea whether that's a relevant difference.
> In each case, MGLRU compiled in but not enabled. THPs and 64kTHPs active.)
> 
> Sorry, but I've put no effort whatsoever into debugging this: "remove
> boilerplate" didn't seem worth the effort, and my personal preference
> is for readable boilerplate over hiding in a macro.  If you prefer the
> macro, I expect Yu can soon come up with a fix (which I could test here):
> but for now please revert "remove boilerplate", since its issues get in
> the way of further mm testing.

Sorry for getting in your way, Hugh.

Apparently I didn't expect local_lock_t to be zero length, i.e., when
CONFIG_DEBUG_LOCK_ALLOC is not set. So that might explain why you only
had problems with one of the two machines, where it failed to disable
IRQ when rotating clean pages after writeback.

The following should fix it, in case you want to verify the above:

diff --git a/mm/swap.c b/mm/swap.c
index 4bc08352ad87..67a246772811 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -254,7 +254,7 @@ static void __folio_batch_add_and_move(struct folio_batch __percpu *fbatch,
 		folio,										\
 		op,										\
 		on_lru,										\
-		offsetof(struct cpu_fbatches, op) > offsetof(struct cpu_fbatches, lock_irq)	\
+		offsetof(struct cpu_fbatches, op) >= offsetof(struct cpu_fbatches, lock_irq)	\
 	)
 
 static void lru_move_tail(struct lruvec *lruvec, struct folio *folio)

Re: [PATCH mm-unstable v1 5/5] mm/swap: remove boilerplate

[Hugh Dickins]

On Sun, 4 Aug 2024, Yu Zhao wrote:
> On Sat, Aug 03, 2024 at 11:55:51PM -0700, Hugh Dickins wrote:
> > On Wed, 10 Jul 2024, Yu Zhao wrote:
> > 
> > > Remove boilerplate by using a macro to choose the corresponding lock
> > > and handler for each folio_batch in cpu_fbatches.
> > > 
> > > Signed-off-by: Yu Zhao <yuzhao@google.com>
> > 
> > Andrew, please revert this "remove boilerplate" patch (and of course its
> > followup fix) from mm-unstable. From the title I presume it was intended
> > to make no functional change, but that's far from so.
> > 
> > Under tmpfs swapping load, on different runs I see various badnesses:
> > "Bad page" in __free_one_page(), Oops in __run_timer_base(),
> > WARNING at kernel/workqueue.c:790 in set_work_data(), PageBuddy BUG
> > at page-flags.h:1009 from __del_page_from_freelist(), something (I'd
> > given up taking better notes by this time) in __queue_work(), others.
> > 
> > All those were including the fix to Barry's report: without that fix,
> > the boot is drowned in warnings scrolling past too fast to be read.
> > 
> > (All the above were on the HP workstation, swapping to SSD; whereas on
> > this ThinkPad, swapping to NVMe, no problem seen at all - I mention the
> > swapping medium, but have no idea whether that's a relevant difference.
> > In each case, MGLRU compiled in but not enabled. THPs and 64kTHPs active.)
> > 
> > Sorry, but I've put no effort whatsoever into debugging this: "remove
> > boilerplate" didn't seem worth the effort, and my personal preference
> > is for readable boilerplate over hiding in a macro.  If you prefer the
> > macro, I expect Yu can soon come up with a fix (which I could test here):
> > but for now please revert "remove boilerplate", since its issues get in
> > the way of further mm testing.
> 
> Sorry for getting in your way, Hugh.
> 
> Apparently I didn't expect local_lock_t to be zero length, i.e., when
> CONFIG_DEBUG_LOCK_ALLOC is not set. So that might explain why you only
> had problems with one of the two machines, where it failed to disable
> IRQ when rotating clean pages after writeback.
> 
> The following should fix it, in case you want to verify the above:
> 
> diff --git a/mm/swap.c b/mm/swap.c
> index 4bc08352ad87..67a246772811 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -254,7 +254,7 @@ static void __folio_batch_add_and_move(struct folio_batch __percpu *fbatch,
>  		folio,										\
>  		op,										\
>  		on_lru,										\
> -		offsetof(struct cpu_fbatches, op) > offsetof(struct cpu_fbatches, lock_irq)	\
> +		offsetof(struct cpu_fbatches, op) >= offsetof(struct cpu_fbatches, lock_irq)	\
>  	)
>  
>  static void lru_move_tail(struct lruvec *lruvec, struct folio *folio)

Well caught! Yes, I confirm that fixes all the bad behaviour I was seeing
(and fits with my having DEBUG_LOCK_ALLOC and lockdep enabled on the
untroubled machine, but not on the one showing problems) - thanks.

But it does reinforce my opinion that mm/swap.c is more understandable
without that macro than with it.

Hugh