[PATCH v1 0/2] *** SUBJECT HERE ***

[PATCH v1 0/2] *** SUBJECT HERE ***

[Zhiguo Jiang]

*** BLURB HERE ***

Zhiguo Jiang (2):
  mm:vmscan: fix workingset eviction memcg issue
  mm:vmscan: fix shrink sc parameters issue

 include/linux/swap.h |  2 +-
 mm/vmscan.c          | 46 ++++++++++++++++++++++----------------------
 mm/workingset.c      |  6 +++---
 3 files changed, 27 insertions(+), 27 deletions(-)
 mode change 100644 => 100755 include/linux/swap.h
 mode change 100644 => 100755 mm/vmscan.c
 mode change 100644 => 100755 mm/workingset.c

-- 
2.39.0

[PATCH v1 1/2] mm:vmscan: fix workingset eviction memcg issue

[Zhiguo Jiang]

The parameter of target_memcg is NULL in workingset_eviction(), and
the lruvec obtained by mem_cgroup_lruvec(target_memcg, pgdat) is always
root_mem_cgroup->lruvec, rather than the lruvec of mem_cgroup where
folio is actually located.

Fix target_memcg to the memcg obtained by folio_memcg(folio).

Signed-off-by: Zhiguo Jiang <justinjiang@vivo.com>
---
 include/linux/swap.h |  2 +-
 mm/vmscan.c          | 11 +++++------
 mm/workingset.c      |  6 +++---
 3 files changed, 9 insertions(+), 10 deletions(-)
 mode change 100644 => 100755 include/linux/swap.h
 mode change 100644 => 100755 mm/vmscan.c
 mode change 100644 => 100755 mm/workingset.c

diff --git a/include/linux/swap.h b/include/linux/swap.h
index 41e4b484bc34..4de61f158903
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -346,7 +346,7 @@ static inline swp_entry_t page_swap_entry(struct page *page)
 /* linux/mm/workingset.c */
 bool workingset_test_recent(void *shadow, bool file, bool *workingset);
 void workingset_age_nonresident(struct lruvec *lruvec, unsigned long nr_pages);
-void *workingset_eviction(struct folio *folio, struct mem_cgroup *target_memcg);
+void *workingset_eviction(struct folio *folio);
 void workingset_refault(struct folio *folio, void *shadow);
 void workingset_activation(struct folio *folio);
 
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 91e7d334a7ca..8a1fbdaca042
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -693,7 +693,7 @@ static pageout_t pageout(struct folio *folio, struct address_space *mapping,
  * gets returned with a refcount of 0.
  */
 static int __remove_mapping(struct address_space *mapping, struct folio *folio,
-			    bool reclaimed, struct mem_cgroup *target_memcg)
+			    bool reclaimed)
 {
 	int refcount;
 	void *shadow = NULL;
@@ -742,7 +742,7 @@ static int __remove_mapping(struct address_space *mapping, struct folio *folio,
 		swp_entry_t swap = folio->swap;
 
 		if (reclaimed && !mapping_exiting(mapping))
-			shadow = workingset_eviction(folio, target_memcg);
+			shadow = workingset_eviction(folio);
 		__delete_from_swap_cache(folio, swap, shadow);
 		mem_cgroup_swapout(folio, swap);
 		xa_unlock_irq(&mapping->i_pages);
@@ -769,7 +769,7 @@ static int __remove_mapping(struct address_space *mapping, struct folio *folio,
 		 */
 		if (reclaimed && folio_is_file_lru(folio) &&
 		    !mapping_exiting(mapping) && !dax_mapping(mapping))
-			shadow = workingset_eviction(folio, target_memcg);
+			shadow = workingset_eviction(folio);
 		__filemap_remove_folio(folio, shadow);
 		xa_unlock_irq(&mapping->i_pages);
 		if (mapping_shrinkable(mapping))
@@ -803,7 +803,7 @@ static int __remove_mapping(struct address_space *mapping, struct folio *folio,
  */
 long remove_mapping(struct address_space *mapping, struct folio *folio)
 {
-	if (__remove_mapping(mapping, folio, false, NULL)) {
+	if (__remove_mapping(mapping, folio, false)) {
 		/*
 		 * Unfreezing the refcount with 1 effectively
 		 * drops the pagecache ref for us without requiring another
@@ -1417,8 +1417,7 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
 			 */
 			count_vm_events(PGLAZYFREED, nr_pages);
 			count_memcg_folio_events(folio, PGLAZYFREED, nr_pages);
-		} else if (!mapping || !__remove_mapping(mapping, folio, true,
-							 sc->target_mem_cgroup))
+		} else if (!mapping || !__remove_mapping(mapping, folio, true))
 			goto keep_locked;
 
 		folio_unlock(folio);
diff --git a/mm/workingset.c b/mm/workingset.c
index 226012974328..f29396d4bf75
--- a/mm/workingset.c
+++ b/mm/workingset.c
@@ -372,15 +372,15 @@ void workingset_age_nonresident(struct lruvec *lruvec, unsigned long nr_pages)
 
 /**
  * workingset_eviction - note the eviction of a folio from memory
- * @target_memcg: the cgroup that is causing the reclaim
  * @folio: the folio being evicted
  *
  * Return: a shadow entry to be stored in @folio->mapping->i_pages in place
  * of the evicted @folio so that a later refault can be detected.
  */
-void *workingset_eviction(struct folio *folio, struct mem_cgroup *target_memcg)
+void *workingset_eviction(struct folio *folio)
 {
 	struct pglist_data *pgdat = folio_pgdat(folio);
+	struct mem_cgroup *memcg = folio_memcg(folio);
 	unsigned long eviction;
 	struct lruvec *lruvec;
 	int memcgid;
@@ -393,7 +393,7 @@ void *workingset_eviction(struct folio *folio, struct mem_cgroup *target_memcg)
 	if (lru_gen_enabled())
 		return lru_gen_eviction(folio);
 
-	lruvec = mem_cgroup_lruvec(target_memcg, pgdat);
+	lruvec = mem_cgroup_lruvec(memcg, pgdat);
 	/* XXX: target_memcg can be NULL, go through lruvec */
 	memcgid = mem_cgroup_id(lruvec_memcg(lruvec));
 	eviction = atomic_long_read(&lruvec->nonresident_age);
-- 
2.39.0

Re: [PATCH v1 1/2] mm:vmscan: fix workingset eviction memcg issue

[Johannes Weiner]

On Thu, Jan 11, 2024 at 08:24:50PM +0800, Zhiguo Jiang wrote:
> The parameter of target_memcg is NULL in workingset_eviction(), and
> the lruvec obtained by mem_cgroup_lruvec(target_memcg, pgdat) is always
> root_mem_cgroup->lruvec, rather than the lruvec of mem_cgroup where
> folio is actually located.

WTF? No!

	/*
	 * The memory cgroup that hit its limit and as a result is the
	 * primary target of this reclaim invocation.
	 */
	struct mem_cgroup *target_mem_cgroup;

The cgroup that is stored in the eviction cookie is the one whose
limit triggered the reclaim cycle. This is often several levels above
the cgroups that own the pages. Subsequent refaults need to be
evaluated at the eviction level:

	/*
	 * The activation decision for this folio is made at the level
	 * where the eviction occurred, as that is where the LRU order
	 * during folio reclaim is being determined.
	 *
	 * However, the cgroup that will own the folio is the one that
	 * is actually experiencing the refault event.
	 */

> Fix target_memcg to the memcg obtained by folio_memcg(folio).
> 
> Signed-off-by: Zhiguo Jiang <justinjiang@vivo.com>

Nacked-by: Johannes Weiner <hannes@cmpxchg.org>

Please take more time to read into the code you're proposing to
change. You made it sound like a trivial simplification, but this
totally screws up aging and pressure detection in containers.

Re: [PATCH v1 1/2] mm:vmscan: fix workingset eviction memcg issue

[zhiguojiang]

在 2024/1/11 21:41, Johannes Weiner 写道:
> On Thu, Jan 11, 2024 at 08:24:50PM +0800, Zhiguo Jiang wrote:
>> The parameter of target_memcg is NULL in workingset_eviction(), and
>> the lruvec obtained by mem_cgroup_lruvec(target_memcg, pgdat) is always
>> root_mem_cgroup->lruvec, rather than the lruvec of mem_cgroup where
>> folio is actually located.
> WTF? No!
>
> 	/*
> 	 * The memory cgroup that hit its limit and as a result is the
> 	 * primary target of this reclaim invocation.
> 	 */
> 	struct mem_cgroup *target_mem_cgroup;
>
> The cgroup that is stored in the eviction cookie is the one whose
> limit triggered the reclaim cycle. This is often several levels above
> the cgroups that own the pages. Subsequent refaults need to be
> evaluated at the eviction level:
>
> 	/*
> 	 * The activation decision for this folio is made at the level
> 	 * where the eviction occurred, as that is where the LRU order
> 	 * during folio reclaim is being determined.
> 	 *
> 	 * However, the cgroup that will own the folio is the one that
> 	 * is actually experiencing the refault event.
> 	 */
May I ask three questions: 1.I don't understand the meaning of this 
paragraph. Can you explain it in detail ? 2.What are the characteristics 
of folios managed by the root_mem_cgroup differring from other memcgs? 
3.If shrink flow uses target_lruvec->anon_cost/file_cost, what is the 
purposr of calculating the actual memcg's lruvec->anon_cost/file_cost in 
lru_note_cost(), and the actual memcg's lruvec->anon_cost/file_cost 
values are unused in shrink flow ? Thanks
>> Fix target_memcg to the memcg obtained by folio_memcg(folio).
>>
>> Signed-off-by: Zhiguo Jiang <justinjiang@vivo.com>
> Nacked-by: Johannes Weiner <hannes@cmpxchg.org>
>
> Please take more time to read into the code you're proposing to
> change. You made it sound like a trivial simplification, but this
> totally screws up aging and pressure detection in containers.

[PATCH v1 2/2] mm:vmscan: fix shrink sc parameters issue

[Zhiguo Jiang]

The values of struct scan_control's members obtained by
prepare_scan_control() are always from root_mem_cgroup, rather than
the memcg where the shrinked folio is actually located, such as
sc->anon_cost, sc->file_cost, sc->may_deactivate and so on. This is
an inaccurate sc values for the actual situation of the current
shrinking memcg. so we need fix the values of struct scan_control's
members are corresponding to the current shrinking memcg.

Signed-off-by: Zhiguo Jiang <justinjiang@vivo.com>
---
 mm/vmscan.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 8a1fbdaca042..31001123e8f1 100755
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2229,29 +2229,30 @@ enum scan_balance {
 	SCAN_FILE,
 };
 
-static void prepare_scan_control(pg_data_t *pgdat, struct scan_control *sc)
+static void prepare_scan_control(pg_data_t *pgdat, struct scan_control *sc,
+			   struct mem_cgroup *memcg)
 {
 	unsigned long file;
-	struct lruvec *target_lruvec;
+	struct lruvec *lruvec;
 
 	if (lru_gen_enabled())
 		return;
 
-	target_lruvec = mem_cgroup_lruvec(sc->target_mem_cgroup, pgdat);
+	lruvec = mem_cgroup_lruvec(memcg, pgdat);
 
 	/*
 	 * Flush the memory cgroup stats, so that we read accurate per-memcg
 	 * lruvec stats for heuristics.
 	 */
-	mem_cgroup_flush_stats(sc->target_mem_cgroup);
+	mem_cgroup_flush_stats(memcg);
 
 	/*
 	 * Determine the scan balance between anon and file LRUs.
 	 */
-	spin_lock_irq(&target_lruvec->lru_lock);
-	sc->anon_cost = target_lruvec->anon_cost;
-	sc->file_cost = target_lruvec->file_cost;
-	spin_unlock_irq(&target_lruvec->lru_lock);
+	spin_lock_irq(&lruvec->lru_lock);
+	sc->anon_cost = lruvec->anon_cost;
+	sc->file_cost = lruvec->file_cost;
+	spin_unlock_irq(&lruvec->lru_lock);
 
 	/*
 	 * Target desirable inactive:active list ratios for the anon
@@ -2265,18 +2266,18 @@ static void prepare_scan_control(pg_data_t *pgdat, struct scan_control *sc)
 		 * workingset is being established. Deactivate to get
 		 * rid of any stale active pages quickly.
 		 */
-		refaults = lruvec_page_state(target_lruvec,
+		refaults = lruvec_page_state(lruvec,
 				WORKINGSET_ACTIVATE_ANON);
-		if (refaults != target_lruvec->refaults[WORKINGSET_ANON] ||
-			inactive_is_low(target_lruvec, LRU_INACTIVE_ANON))
+		if (refaults != lruvec->refaults[WORKINGSET_ANON] ||
+			inactive_is_low(lruvec, LRU_INACTIVE_ANON))
 			sc->may_deactivate |= DEACTIVATE_ANON;
 		else
 			sc->may_deactivate &= ~DEACTIVATE_ANON;
 
-		refaults = lruvec_page_state(target_lruvec,
+		refaults = lruvec_page_state(lruvec,
 				WORKINGSET_ACTIVATE_FILE);
-		if (refaults != target_lruvec->refaults[WORKINGSET_FILE] ||
-		    inactive_is_low(target_lruvec, LRU_INACTIVE_FILE))
+		if (refaults != lruvec->refaults[WORKINGSET_FILE] ||
+		    inactive_is_low(lruvec, LRU_INACTIVE_FILE))
 			sc->may_deactivate |= DEACTIVATE_FILE;
 		else
 			sc->may_deactivate &= ~DEACTIVATE_FILE;
@@ -2288,7 +2289,7 @@ static void prepare_scan_control(pg_data_t *pgdat, struct scan_control *sc)
 	 * thrashing, try to reclaim those first before touching
 	 * anonymous pages.
 	 */
-	file = lruvec_page_state(target_lruvec, NR_INACTIVE_FILE);
+	file = lruvec_page_state(lruvec, NR_INACTIVE_FILE);
 	if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE))
 		sc->cache_trim_mode = 1;
 	else
@@ -5885,6 +5886,8 @@ static void shrink_node_memcgs(pg_data_t *pgdat, struct scan_control *sc)
 		reclaimed = sc->nr_reclaimed;
 		scanned = sc->nr_scanned;
 
+		prepare_scan_control(pgdat, sc, memcg);
+
 		shrink_lruvec(lruvec, sc);
 
 		shrink_slab(sc->gfp_mask, pgdat->node_id, memcg,
@@ -5918,8 +5921,6 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc)
 	nr_reclaimed = sc->nr_reclaimed;
 	nr_scanned = sc->nr_scanned;
 
-	prepare_scan_control(pgdat, sc);
-
 	shrink_node_memcgs(pgdat, sc);
 
 	flush_reclaim_state(sc);
-- 
2.39.0

Re: [PATCH v1 2/2] mm:vmscan: fix shrink sc parameters issue

[Johannes Weiner]

On Thu, Jan 11, 2024 at 08:24:51PM +0800, Zhiguo Jiang wrote:
> The values of struct scan_control's members obtained by
> prepare_scan_control() are always from root_mem_cgroup, rather than
> the memcg where the shrinked folio is actually located, such as
> sc->anon_cost, sc->file_cost, sc->may_deactivate and so on. This is
> an inaccurate sc values for the actual situation of the current
> shrinking memcg. so we need fix the values of struct scan_control's
> members are corresponding to the current shrinking memcg.

You don't seem to understand how cgroup reclaim works.

Nacked-by: Johannes Weiner <hannes@cmpxchg.org>

Re: [PATCH v1 2/2] mm:vmscan: fix shrink sc parameters issue

[zhiguojiang]

在 2024/1/11 21:43, Johannes Weiner 写道:
> On Thu, Jan 11, 2024 at 08:24:51PM +0800, Zhiguo Jiang wrote:
>> The values of struct scan_control's members obtained by
>> prepare_scan_control() are always from root_mem_cgroup, rather than
>> the memcg where the shrinked folio is actually located, such as
>> sc->anon_cost, sc->file_cost, sc->may_deactivate and so on. This is
>> an inaccurate sc values for the actual situation of the current
>> shrinking memcg. so we need fix the values of struct scan_control's
>> members are corresponding to the current shrinking memcg.
> You don't seem to understand how cgroup reclaim works.
It seems that the actual memcg's lruvec->anon_cost/file_cost values are 
not used in code exclude target_memcg ?
>
> Nacked-by: Johannes Weiner <hannes@cmpxchg.org>

Re: [PATCH v1 0/2] *** SUBJECT HERE ***

[Christophe Leroy]

Le 11/01/2024 à 13:24, Zhiguo Jiang a écrit :
> [Vous ne recevez pas souvent de courriers de justinjiang@vivo.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
> 
> *** BLURB HERE ***

You forgot to fill-in the cover letter it seems.

Christophe


> 
> Zhiguo Jiang (2):
>    mm:vmscan: fix workingset eviction memcg issue
>    mm:vmscan: fix shrink sc parameters issue
> 
>   include/linux/swap.h |  2 +-
>   mm/vmscan.c          | 46 ++++++++++++++++++++++----------------------
>   mm/workingset.c      |  6 +++---
>   3 files changed, 27 insertions(+), 27 deletions(-)
>   mode change 100644 => 100755 include/linux/swap.h
>   mode change 100644 => 100755 mm/vmscan.c
>   mode change 100644 => 100755 mm/workingset.c
> 
> --
> 2.39.0
> 
> 

Re: [PATCH v1 0/2] *** SUBJECT HERE ***

[zhiguojiang]

在 2024/1/11 20:51, Christophe Leroy 写道:
> [Certaines personnes qui ont reçu ce message ne reçoivent pas souvent de courriers de christophe.leroy@csgroup.eu. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
>
> Le 11/01/2024 à 13:24, Zhiguo Jiang a écrit :
>> [Vous ne recevez pas souvent de courriers de justinjiang@vivo.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
>>
>> *** BLURB HERE ***
> You forgot to fill-in the cover letter it seems.
>
> Christophe
Ok, I will supplement the cover letter as "mm:vmscan: fix shrink sc 
parameters issue" in the next version update.
Thanks
>
>
>> Zhiguo Jiang (2):
>>     mm:vmscan: fix workingset eviction memcg issue
>>     mm:vmscan: fix shrink sc parameters issue
>>
>>    include/linux/swap.h |  2 +-
>>    mm/vmscan.c          | 46 ++++++++++++++++++++++----------------------
>>    mm/workingset.c      |  6 +++---
>>    3 files changed, 27 insertions(+), 27 deletions(-)
>>    mode change 100644 => 100755 include/linux/swap.h
>>    mode change 100644 => 100755 mm/vmscan.c
>>    mode change 100644 => 100755 mm/workingset.c
>>
>> --
>> 2.39.0
>>
>>