mm: workingset: fix the shadow node budget under MGLRU
Subsystems (6)
v2 cover mm: workingset: fix the shadow node budget under MGLRU
From: Hui Zhu <[email protected]>
Commit 7404bd37cfbe ("mm: workingset: use lruvec_lru_size() to get the number of lru pages") broke the workingset shadow node budget under
MGLRU: lruvec_lru_size() reads mz->lru_zone_size, which MGLRU never maintains, so count_shadow_nodes() sees the evictable LRU lists as empty and the shadow shrinker reclaims eviction tokens almost as fast as they are created, losing thrashing protection.
Patch 1 switches count_shadow_nodes() back to lruvec_page_state_local(), which both classic LRU and MGLRU maintain.
Patch 2 addresses the reparenting race that motivated 7404bd37cfbe:
patch 1 makes cgroup v2 read state_local as well, so extend the dying-memcg stat redirection (previously cgroup v1 only) to all hierarchies.
Patch 3 recovers the performance. Patch 2 added an unconditional rcu_read_lock() to the stat update fast path; patch 3 moves the dying check out of the RCU read-side critical section so the lock is only taken on the rare dying path.
Performance testing
===================
The test script and the raw results are available at [1].
Environment: 10-vCPU QEMU guest, 8 GiB RAM, cgroup v2; 7 runs per configuration, medians reported. Workloads:
w1-anon-churn: single-threaded anon fault/charge loop in a memcg
(MADV_DONTNEED + re-fault, no reclaim). Every touch is a real fault with charge and memcg stat updates, so it stresses exactly the fast path patch 2 changes.
w2-file-churn: file read loop under memory.high pressure
(reclaim-bound, noisier).
w3-reparent: reparent accounting sanity check.
w1-anon-churn (pages/s):
classic LRU MGLRU base 4393028 4377122 patches 1-2 4385996 (-0.2%) 4352887 (-0.6%) patches 1-3 4381832 (-0.3%) 4377053 (+0.0%)
w2-file-churn (MB/s):
classic LRU MGLRU base 8277 8226 patches 1-2 8226 (-0.6%) 8123 (-1.3%) patches 1-3 8157 (-1.4%) 8294 (+0.8%)
w3-reparent passed on all kernels.
Patch 2 alone shows a small overhead, most visible under MGLRU (-0.6% on w1); patch 3 brings w1 back to the base level in both LRU configurations. The remaining differences are within run-to-run noise.
[1] https://gist.github.com/teawater/32f373ec41d185d840455eb167321a5a
Hui Zhu (3):
mm: workingset: use lruvec_page_state_local() to count lru pages
mm: memcg: redirect stats updates of dying memcgs for all hierarchies
mm: memcg: skip the RCU lock when the memcg is not dying
mm/memcontrol.c | 30 ++++++++++++------------------ mm/workingset.c | 5 ++--- 2 files changed, 14 insertions(+), 21 deletions(-)
Signature · 1 line
2.53.0
PATCH v2 1/3 mm: workingset: use lruvec_page_state_local() to count lru pages
From: Hui Zhu <[email protected]>
Commit 7404bd37cfbe ("mm: workingset: use lruvec_lru_size() to get the number of lru pages") switched count_shadow_nodes() to lruvec_lru_size(). With CONFIG_MEMCG enabled, lruvec_lru_size() reads mz->lru_zone_size, which only the classic LRU paths maintain. MGLRU accounts its pages through __update_lru_size(), which skips that array, so with MGLRU on the four evictable LRU lists are always seen as empty. The shadow node budget (pages >> 3) then collapses to slab plus unevictable pages, and the workingset shadow shrinker reclaims eviction tokens almost as fast as they are created, losing thrashing protection.
lruvec_page_state_local() reads lruvec_stats->state_local instead, which both classic LRU and MGLRU maintain. Switch back to it. The reparenting race this re-exposes on cgroup v2 is closed by the follow-up patch that redirects dying-memcg stat updates for all hierarchies.
--- mm/workingset.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
mm/workingset.c +2 -3
@@ -693,10 +693,9 @@ static unsigned long count_shadow_nodes(struct shrinker *shrinker,mem_cgroup_flush_stats_ratelimited (sc->memcg); lruvec =mem_cgroup_lruvec (sc->memcg,NODE_DATA (sc->nid)); -for (pages =0 , i =0 ; i <NR_LRU_LISTS ; i++) - pages +=lruvec_lru_size (lruvec, i,MAX_NR_ZONES -1 ); - + pages +=lruvec_page_state_local (lruvec, +NR_LRU_BASE + i); pages +=lruvec_page_state_local ( lruvec,NR_SLAB_RECLAIMABLE_B ) >>PAGE_SHIFT ; pages += lruvec_page_state_local(
Signature · 1 line
2.53.0
Shakeel Butt
maintainerRe: [PATCH v2 1/3] mm: workingset: use lruvec_page_state_local() to count lru pages
Quoted 12 lines · Hui Zhu
From: Hui Zhu <[email protected]>
Commit
7404bd37cfbe("mm: workingset: uselruvec_lru_size()to get the number of lru pages") switchedcount_shadow_nodes()tolruvec_lru_size(). WithCONFIG_MEMCGenabled,lruvec_lru_size()readsmz->lru_zone_size, which only the classic LRU paths maintain. MGLRU accounts its pages through__update_lru_size(), which skips that array, so with MGLRU on the four evictable LRU lists are always seen as empty. The shadow node budget (pages >> 3) then collapses to slab plus unevictable pages, and the workingset shadow shrinker reclaims eviction tokens almost as fast as they are created, losing thrashing protection.
Quoted 8 lines
lruvec_page_state_local()readslruvec_stats->state_localinstead, which both classic LRU and MGLRU maintain. Switch back to it. The reparenting race this re-exposes on cgroup v2 is closed by the follow-up patch that redirects dying-memcg stat updates for all hierarchies.Fixes 7404bd37cfbe ("mm: workingset: use lruvec_lru_size() to get the number of lru pages")Signed-off-by Hui Zhu <[email protected]>
Make this patch at the end (or after 2nd) of the series as it is only correct after the 2nd patch of the series.
Quoted 24 lines
--- mm/workingset.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)mm/workingset.c +2
-3@@ -693,10 +693,9 @@ static unsigned long count_shadow_nodes(struct shrinker *shrinker,mem_cgroup_flush_stats_ratelimited (sc->memcg); lruvec =mem_cgroup_lruvec (sc->memcg,NODE_DATA (sc->nid)); -for (pages =0 , i =0 ; i <NR_LRU_LISTS ; i++) - pages +=lruvec_lru_size (lruvec, i,MAX_NR_ZONES -1 ); - + pages +=lruvec_page_state_local (lruvec, +NR_LRU_BASE + i); pages +=lruvec_page_state_local ( lruvec,NR_SLAB_RECLAIMABLE_B ) >>PAGE_SHIFT ; pages += lruvec_page_state_local(Signature · 1 line
2.53.0
Re: [PATCH v2 1/3] mm: workingset: use lruvec_page_state_local() to count lru pages
In reply to Hui ZhuQuoted 12 lines · Hui Zhu
From: Hui Zhu <[email protected]>
Commit
7404bd37cfbe("mm: workingset: uselruvec_lru_size()to get the number of lru pages") switchedcount_shadow_nodes()tolruvec_lru_size(). WithCONFIG_MEMCGenabled,lruvec_lru_size()readsmz->lru_zone_size, which only the classic LRU paths maintain. MGLRU accounts its pages through__update_lru_size(), which skips that array, so with MGLRU on the four evictable LRU lists are always seen as empty. The shadow node budget (pages >> 3) then collapses to slab plus unevictable pages, and the workingset shadow shrinker reclaims eviction tokens almost as fast as they are created, losing thrashing protection.
Quoted 8 lines
lruvec_page_state_local()readslruvec_stats->state_localinstead, which both classic LRU and MGLRU maintain. Switch back to it. The reparenting race this re-exposes on cgroup v2 is closed by the follow-up patch that redirects dying-memcg stat updates for all hierarchies.Fixes 7404bd37cfbe ("mm: workingset: use lruvec_lru_size() to get the number of lru pages")Signed-off-by Hui Zhu <[email protected]>
Reorder this after 2nd and with that
PATCH v2 2/3 mm: memcg: redirect stats updates of dying memcgs for all hierarchies
In reply to Hui ZhuFrom: Hui Zhu <[email protected]>
get_non_dying_memcg_start() redirects the stat updates of a dying memcg to its closest non-dying ancestor, but only on cgroup v1; on cgroup v2 the stats keep being accounted to the dying memcg itself.
The previous patch restored lruvec_page_state_local() in count_shadow_nodes(), which reads those state_locals on cgroup v2 too, so apply the redirection to all hierarchies. Offlining is rare, so the added cost on the stat update fast path is limited to an rcu_read_lock() and a css_is_dying() check; the upward walk happens only while a memcg is dying.
--- mm/memcontrol.c | 30 +++++------------------------- 1 file changed, 5 insertions(+), 25 deletions(-)
mm/memcontrol.c +5 -25
@@ -805,20 +805,14 @@ static long memcg_state_val_in_pages(int idx, long val)return val <0 ? -res : res; } -#ifdef CONFIG_MEMCG_V1 /* -* Used in mod_memcg_state() and mod_memcg_lruvec_state() to avoid race with -* reparenting of non-hierarchical state_locals. +* Used in mod_memcg_state() and mod_memcg_lruvec_state() to avoid race +* with reparenting of non-hierarchical state_locals. Offlining a +* memcg is rare, so do the redirection for all cgroup hierarchies. */ -static inline struct mem_cgroup *get_non_dying_memcg_start (struct mem_cgroup *memcg, -bool *rcu_locked) +static inline struct mem_cgroup * +get_non_dying_memcg_start (struct mem_cgroup *memcg,bool *rcu_locked) { -/* Rebinding can cause this value to be changed at runtime */ -if (cgroup_subsys_on_dfl (memory_cgrp_subsys)) { - *rcu_locked = false; -return memcg; - } -rcu_read_lock (); *rcu_locked = true;
@@ -830,22 +824,8 @@ static inline struct mem_cgroup *get_non_dying_memcg_start(struct mem_cgroup *mestatic inline void get_non_dying_memcg_end (bool rcu_locked) { -if (!rcu_locked) -return ; -rcu_read_unlock (); } -#else -static inline struct mem_cgroup *get_non_dying_memcg_start (struct mem_cgroup *memcg, -bool *rcu_locked) -{ -return memcg; -} - -static inline void get_non_dying_memcg_end (bool rcu_locked) -{ -} -#endif static void __mod_memcg_state (struct mem_cgroup *memcg,enum memcg_stat_item idx,long val)
Signature · 1 line
2.53.0
Shakeel Butt
maintainerRe: [PATCH v2 2/3] mm: memcg: redirect stats updates of dying memcgs for all hierarchies
Quoted 6 lines · Hui Zhu
From: Hui Zhu <[email protected]>
get_non_dying_memcg_start()redirects the stat updates of a dying memcg to its closest non-dying ancestor, but only on cgroup v1; on cgroup v2 the stats keep being accounted to the dying memcg itself.
Quoted 7 lines
The previous patch restored
lruvec_page_state_local()incount_shadow_nodes(), which reads thosestate_localson cgroup v2 too, so apply the redirection to all hierarchies. Offlining is rare, so the added cost on the stat update fast path is limited to anrcu_read_lock()and acss_is_dying()check; the upward walk happens only while a memcg is dying.Signed-off-by Hui Zhu <[email protected]>
If we want to backport the first patch to stable trees then we need to backport this one too.
Quoted 60 lines
--- mm/memcontrol.c | 30 +++++------------------------- 1 file changed, 5 insertions(+), 25 deletions(-)mm/memcontrol.c +5
-25@@ -805,20 +805,14 @@ static long memcg_state_val_in_pages(int idx, long val)return val <0 ? -res : res; } -#ifdef CONFIG_MEMCG_V1 /* -* Used in mod_memcg_state() and mod_memcg_lruvec_state() to avoid race with -* reparenting of non-hierarchical state_locals. +* Used in mod_memcg_state() and mod_memcg_lruvec_state() to avoid race +* with reparenting of non-hierarchical state_locals. Offlining a +* memcg is rare, so do the redirection for all cgroup hierarchies. */ -static inline struct mem_cgroup *get_non_dying_memcg_start (struct mem_cgroup *memcg, -bool *rcu_locked) +static inline struct mem_cgroup * +get_non_dying_memcg_start (struct mem_cgroup *memcg,bool *rcu_locked) { -/* Rebinding can cause this value to be changed at runtime */ -if (cgroup_subsys_on_dfl (memory_cgrp_subsys)) { - *rcu_locked = false; -return memcg; - } -rcu_read_lock (); *rcu_locked = true;@@ -830,22 +824,8 @@ static inline struct mem_cgroup *get_non_dying_memcg_start(struct mem_cgroup *mestatic inline void get_non_dying_memcg_end (bool rcu_locked) { -if (!rcu_locked) -return ; -rcu_read_unlock (); } -#else -static inline struct mem_cgroup *get_non_dying_memcg_start (struct mem_cgroup *memcg, -bool *rcu_locked) -{ -return memcg; -} - -static inline void get_non_dying_memcg_end (bool rcu_locked) -{ -} -#endif static void __mod_memcg_state (struct mem_cgroup *memcg,enum memcg_stat_item idx,long val)Signature · 1 line
2.53.0
Re: [PATCH v2 2/3] mm: memcg: redirect stats updates of dying memcgs for all hierarchies
In reply to Hui ZhuQuoted 6 lines · Hui Zhu
From: Hui Zhu <[email protected]>
get_non_dying_memcg_start()redirects the stat updates of a dying memcg to its closest non-dying ancestor, but only on cgroup v1; on cgroup v2 the stats keep being accounted to the dying memcg itself.
Quoted 7 lines
The previous patch restored
lruvec_page_state_local()incount_shadow_nodes(), which reads thosestate_localson cgroup v2 too, so apply the redirection to all hierarchies. Offlining is rare, so the added cost on the stat update fast path is limited to anrcu_read_lock()and acss_is_dying()check; the upward walk happens only while a memcg is dying.Signed-off-by Hui Zhu <[email protected]>
Please CC to stable and add a sentence on user visible impact of the bug in the commit message. With that:
PATCH v2 3/3 mm: memcg: skip the RCU lock when the memcg is not dying
In reply to Hui ZhuFrom: Hui Zhu <[email protected]>
get_non_dying_memcg_start() takes rcu_read_lock() on every stat update, but the lock only protects the upward walk to a non-dying ancestor, which happens solely while a memcg is being offlined. The dying check itself reads the CSS_DYING flag of a memcg the caller already holds a reference to, so it is safe without the lock.
Check memcg_is_dying() first and return immediately when the memcg is alive, taking the RCU lock only on the rare dying path. On an anon fault/charge churn workload in a memcg this recovers the ~0.6% overhead added by the previous patch (4368077 vs 4343159 pages/s before, back to ~4377000 pages/s after).
--- mm/memcontrol.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
mm/memcontrol.c +14 -0
@@ -813,6 +813,17 @@ static long memcg_state_val_in_pages(int idx, long val)static inline struct mem_cgroup *get_non_dying_memcg_start (struct mem_cgroup *memcg,bool *rcu_locked) { +/* +* Fast path: the caller holds a reference to @memcg, so reading +* its CSS_DYING flag without the RCU lock is safe. The RCU lock +* is only needed to walk up to a non-dying ancestor, which +* happens only while a memcg is actually being offlined. +*/ +if (!memcg_is_dying (memcg)) { + *rcu_locked = false; +return memcg; + } +rcu_read_lock (); *rcu_locked = true;
@@ -824,6 +835,9 @@ get_non_dying_memcg_start(struct mem_cgroup *memcg, bool *rcu_locked)static inline void get_non_dying_memcg_end (bool rcu_locked) { +if (!rcu_locked) +return ; +rcu_read_unlock (); }
Signature · 1 line
2.53.0
Re: [PATCH v2 3/3] mm: memcg: skip the RCU lock when the memcg is not dying
Quoted 8 lines · Hui Zhu
From: Hui Zhu <[email protected]>
get_non_dying_memcg_start()takesrcu_read_lock()on every stat update, but the lock only protects the upward walk to a non-dying ancestor, which happens solely while a memcg is being offlined. The dying check itself reads theCSS_DYINGflag of a memcg the caller already holds a reference to, so it is safe without the lock.
Quoted 7 lines
Check
memcg_is_dying()first and return immediately when the memcg is alive, taking the RCU lock only on the rare dying path. On an anon fault/charge churn workload in a memcg this recovers the ~0.6% overhead added by the previous patch (4368077 vs 4343159 pages/s before, back to ~4377000 pages/s after).Signed-off-by Hui Zhu <[email protected]>
Shakeel Butt
maintainerQuoted 52 lines · Hui Zhu
From: Hui Zhu <[email protected]>
Commit
7404bd37cfbe("mm: workingset: uselruvec_lru_size()to get the number of lru pages") broke the workingset shadow node budget under
MGLRU:lruvec_lru_size()readsmz->lru_zone_size, which MGLRU never maintains, socount_shadow_nodes()sees the evictable LRU lists as empty and the shadow shrinker reclaims eviction tokens almost as fast as they are created, losing thrashing protection.Patch 1 switches
count_shadow_nodes()back tolruvec_page_state_local(), which both classic LRU and MGLRU maintain.Patch 2 addresses the reparenting race that motivated
7404bd37cfbe:
patch 1 makes cgroup v2 readstate_localas well, so extend the dying-memcg stat redirection (previously cgroup v1 only) to all hierarchies.Patch 3 recovers the performance. Patch 2 added an unconditional
rcu_read_lock()to the stat update fast path; patch 3 moves the dying check out of the RCU read-side critical section so the lock is only taken on the rare dying path.Performance testing
===================The test script and the raw results are available at [1].
Environment: 10-vCPU QEMU guest, 8 GiB RAM, cgroup v2; 7 runs per configuration, medians reported. Workloads:
w1-anon-churn: single-threaded anon fault/charge loop in a memcg
(MADV_DONTNEED+ re-fault, no reclaim). Every touch is a real fault with charge and memcg stat updates, so it stresses exactly the fast path patch 2 changes.
w2-file-churn: file read loop under memory.high pressure
(reclaim-bound, noisier).
w3-reparent: reparent accounting sanity check.w1-anon-churn (pages/s):
classic LRU MGLRU base 4393028 4377122 patches 1-2 4385996 (-0.2%) 4352887 (-0.6%) patches 1-3 4381832 (-0.3%) 4377053 (+0.0%)w2-file-churn (MB/s):
classic LRU MGLRU base 8277 8226 patches 1-2 8226 (-0.6%) 8123 (-1.3%) patches 1-3 8157 (-1.4%) 8294 (+0.8%)
Quoted 8 lines
w3-reparent passed on all kernels.
Patch 2 alone shows a small overhead, most visible under MGLRU (-0.6% on w1); patch 3 brings w1 back to the base level in both LRU configurations. The remaining differences are within run-to-run noise.
[1] https://gist.github.com/teawater/32f373ec41d185d840455eb167321a5a
Thanks for running the benchmarks as well. I think we just need to order the first patch after the second and add CC:stable to the second. We can ask Andrew but it might be simpler for Andrew to just resend with the correct ordering.