mm/mglru: fix ineffective memory protection for non-kswapd reclaim
Subsystems (6)
v2 cover mm/mglru: fix ineffective memory protection for non-kswapd reclaim
From: Ridong Chen <[email protected]>
This series fixes memory.min/low being silently bypassed for MGLRU during non-kswapd global reclaim (global direct reclaim and root-level memory.reclaim).
Patch 2 is the actual fix. Patch 1 is a prerequisite: an integer overflow in effective_protection(), spotted by the sashiko review tool, which patch 2's new caller would also be exposed to.
---
v2:
- Fix an issue caused by non-atomic read races in patch 1 [1]
[1] https://sashiko.dev/#/patchset/[email protected]?part=1
Ridong Chen (2):
mm/page_counter: avoid integer overflow in effective_protection() mm/mglru: fix ineffective memory protection for non-kswapd reclaim
include/linux/memcontrol.h | 11 ++++++++++ mm/memcontrol.c | 45 ++++++++++++++++++++++++++++++++++++++ mm/page_counter.c | 19 +++++++++++----- mm/vmscan.c | 8 ++++++- 4 files changed, 76 insertions(+), 7 deletions(-)
Signature · 1 line
2.34.1
PATCH v2 1/2 mm/page_counter: avoid integer overflow in effective_protection()
From: Ridong Chen <[email protected]>
effective_protection() scales a parent's protection by a ratio of page counts, e.g. for recursive protection:
(parent_effective - siblings_protected) * (usage - protected)
/ (parent_usage - siblings_protected)
The multiply is done at unsigned long width before dividing. On systems with >= 16TB RAM the product can exceed 2^64 and wrap, giving a bogus protection value and silently breaking memory.min/low enforcement.
Use mul_u64_u64_div_u64() to multiply in a 128-bit intermediate. Because usage and parent_usage are not read atomically (a child is charged before its parent), usage - protected can briefly exceed the divisor, making the quotient overflow 64 bits and trap (#DE on x86). Cap it so the ratio stays <= 1.
Reported by the sashiko review tool [1].
[1] https://sashiko.dev/#/patchset/[email protected]?part=1
--- mm/page_counter.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)
mm/page_counter.c +13 -6
@@ -8,6 +8,7 @@#include <linux/page_counter.h> #include <linux/atomic.h> #include <linux/kernel.h> +#include <linux/math64.h> #include <linux/string.h> #include <linux/sched.h> #include <linux/bug.h>
@@ -356,7 +357,8 @@ static unsigned long effective_protection(unsigned long usage,* otherwise get a smaller chunk than what they claimed. */ if (siblings_protected > parent_effective) -return protected * parent_effective / siblings_protected; +return mul_u64_u64_div_u64 (protected, parent_effective, + siblings_protected);/* * Ok, utilized protection of all children is within what the
@@ -397,13 +399,18 @@ static unsigned long effective_protection(unsigned long usage,if (parent_effective > siblings_protected && parent_usage > siblings_protected && usage > protected) { -unsigned long unclaimed; +unsigned long unclaimed = parent_effective - siblings_protected; +unsigned long unprotected = usage - protected; +unsigned long parent_unprotected = parent_usage - siblings_protected; - unclaimed = parent_effective - siblings_protected; - unclaimed *= usage - protected; - unclaimed /= parent_usage - siblings_protected; +/* +* The usages aren't read atomically, so a child can transiently +* appear to use more than its parent, making the ratio exceed 1 +* and the quotient overflow 64 bits (#DE on x86). Cap it. +*/ + unprotected =min (unprotected, parent_unprotected); - ep += unclaimed; + ep +=mul_u64_u64_div_u64 (unclaimed, unprotected, parent_unprotected); }return ep;
Signature · 1 line
2.34.1
Re: [PATCH v2 1/2] mm/page_counter: avoid integer overflow in effective_protection()
Quoted 21 lines · Ridong Chen
From: Ridong Chen <[email protected]>
effective_protection()scales a parent's protection by a ratio of page counts, e.g. for recursive protection:(
parent_effective-siblings_protected) * (usage - protected)
/ (parent_usage-siblings_protected)The multiply is done at unsigned long width before dividing. On systems with >= 16TB RAM the product can exceed 2^64 and wrap, giving a bogus protection value and silently breaking memory.min/low enforcement.
Use
mul_u64_u64_div_u64()to multiply in a 128-bit intermediate. Because usage andparent_usageare not read atomically (a child is charged before its parent), usage - protected can briefly exceed the divisor, making the quotient overflow 64 bits and trap (#DE on x86). Cap it so the ratio stays <= 1.Reported by the sashiko review tool [1].
Quoted 8 lines
[1] https://sashiko.dev/#/patchset/[email protected]?part=1
Fixes bc50bcc6e00b ("mm: memcontrol: clean up and document effective low/min calculations")Fixes 8a931f801340 ("mm: memcontrol: recursive memory.low protection")Assisted-by Claude:claude-opus-4-8Signed-off-by Ridong Chen <[email protected]>---
LGTM,
PATCH v2 2/2 mm/mglru: fix ineffective memory protection for non-kswapd reclaim
In reply to Ridong ChenFrom: Ridong Chen <[email protected]>
memory.min/low is silently bypassed for MGLRU during global proactive reclaim (writing to the root memory.reclaim) and global direct reclaim. It can be reproduced as follows:
# echo 7 > /sys/kernel/mm/lru_gen/enabled # cd /sys/fs/cgroup # mkdir -p a/b # echo 100M > a/memory.min # echo +memory > a/cgroup.subtree_control # echo 100M > a/b/memory.min # echo $$ > a/b/cgroup.procs # dd if=/dev/zero of=/tmp/testfile bs=1M count=200 # cat a/b/memory.current 222650368 # echo 500M > memory.reclaim -bash: echo: write error: Resource temporarily unavailable # cat a/b/memory.current 6070272
memory.min is 100M, yet reclaim drops a/b down to 6M, breaking the protection. The traditional LRU path is not affected because shrink_node() calls mem_cgroup_calculate_protection() for each memcg it visits during a top-down tree walk.
Commit 30d77b7eef01 ("mm/mglru: fix ineffective protection calculation") moved the protection computation into lru_gen_age_node(), which only runs for kswapd. Non-kswapd global reclaim reaches shrink_one() through lru_gen_shrink_node() -> shrink_many() without any protection computation, so emin/elow remain stale or zero.
Introduce mem_cgroup_protection_path() which computes emin/elow along the root-to-target path only by iterating through the cgroup ancestors array top-down. This avoids the full tree traversal that would be needed with mem_cgroup_calculate_protection(), limiting the cost to O(depth) per memcg - typically 3-5 levels.
Call it from shrink_one() for the non-kswapd path so that each memcg about to be shrunk has correct protection values.
--- include/linux/memcontrol.h | 11 ++++++++++ mm/memcontrol.c | 45 ++++++++++++++++++++++++++++++++++++++ mm/vmscan.c | 8 ++++++- 3 files changed, 63 insertions(+), 1 deletion(-)
include/linux/memcontrol.h +11 -0
@@ -605,6 +605,10 @@ static inline void mem_cgroup_protection(struct mem_cgroup *root,void mem_cgroup_calculate_protection (struct mem_cgroup *root,struct mem_cgroup *memcg); +#ifdef CONFIG_LRU_GEN +void mem_cgroup_protection_path (struct mem_cgroup *root, +struct mem_cgroup *memcg); +#endif static inline bool mem_cgroup_unprotected (struct mem_cgroup *target,struct mem_cgroup *memcg)
@@ -1133,6 +1137,13 @@ static inline void mem_cgroup_calculate_protection(struct mem_cgroup *root, { } +#ifdef CONFIG_LRU_GEN +static inline void mem_cgroup_protection_path (struct mem_cgroup *root, +struct mem_cgroup *memcg) +{ +} +#endif +static inline bool mem_cgroup_unprotected (struct mem_cgroup *target,struct mem_cgroup *memcg) {
mm/memcontrol.c +45 -0
51 lines not on this page · raw message
mm/vmscan.c +7 -1
@@ -5102,7 +5102,13 @@ static int shrink_one(struct lruvec *lruvec, struct scan_control *sc)struct mem_cgroup *memcg =lruvec_memcg (lruvec);struct pglist_data *pgdat =lruvec_pgdat (lruvec); -/* lru_gen_age_node() called mem_cgroup_calculate_protection() */ +/* +* For kswapd, lru_gen_age_node() has already called +* mem_cgroup_calculate_protection() +*/ +if (!current_is_kswapd ()) +mem_cgroup_protection_path (NULL , memcg); +if (mem_cgroup_below_min (NULL , memcg))return MEMCG_LRU_YOUNG ;
Signature · 1 line
2.34.1
Barry Song
reviewerQuoted 5 lines · Ridong Chen
From: Ridong Chen <[email protected]>
memory.min/low is silently bypassed for MGLRU during global proactive reclaim (writing to the root memory.reclaim) and global direct reclaim.
I guess nobody is silently bypassing anything. It's just that the effective min is stale data. If kswapd has run at least once, should the protection have been updated already?
I guess we need to update the changelog a bit?
Quoted 63 lines
It can be reproduced as follows:
# echo 7 > /sys/kernel/mm/lru_gen/enabled # cd /sys/fs/cgroup # mkdir -p a/b # echo 100M > a/memory.min # echo +memory > a/cgroup.subtree_control # echo 100M > a/b/memory.min # echo $$ > a/b/cgroup.procs # dd if=/dev/zero of=/tmp/testfile bs=1M count=200 # cat a/b/memory.current 222650368 # echo 500M > memory.reclaim -bash: echo: write error: Resource temporarily unavailable # cat a/b/memory.current 6070272memory.min is 100M, yet reclaim drops a/b down to 6M, breaking the protection. The traditional LRU path is not affected because
shrink_node()callsmem_cgroup_calculate_protection()for each memcg it visits during a top-down tree walk.Commit
30d77b7eef01("mm/mglru: fix ineffective protection calculation") moved the protection computation intolru_gen_age_node(), which only runs for kswapd. Non-kswapd global reclaim reachesshrink_one()throughlru_gen_shrink_node()->shrink_many()without any protection computation, so emin/elow remain stale or zero.Introduce
mem_cgroup_protection_path()which computes emin/elow along the root-to-target path only by iterating through the cgroup ancestors array top-down. This avoids the full tree traversal that would be needed withmem_cgroup_calculate_protection(), limiting the cost to O(depth) per memcg - typically 3-5 levels.Call it from
shrink_one()for the non-kswapd path so that each memcg about to be shrunk has correct protection values.Fixes e4dde56cd208 ("mm: multi-gen LRU: per-node lru_gen_folio lists")Assisted-by Claude:claude-opus-4-8Signed-off-by Ridong Chen <[email protected]>--- include/linux/memcontrol.h | 11 ++++++++++ mm/memcontrol.c | 45 ++++++++++++++++++++++++++++++++++++++ mm/vmscan.c | 8 ++++++- 3 files changed, 63 insertions(+), 1 deletion(-)include/linux/memcontrol.h +4
-0@@ -605,6 +605,10 @@ static inline void mem_cgroup_protection(struct mem_cgroup *root,void mem_cgroup_calculate_protection (struct mem_cgroup *root,struct mem_cgroup *memcg); +#ifdef CONFIG_LRU_GEN +void mem_cgroup_protection_path (struct mem_cgroup *root, +struct mem_cgroup *memcg); +#endif static inline bool mem_cgroup_unprotected (struct mem_cgroup *target,struct mem_cgroup *memcg)
include/linux/memcontrol.h static inline void mem_cgroup_calculate_protection(struct mem_cgroup *root, · line 1146
Quoted 10 lines
include/linux/memcontrol.h +7
-0@@ -1133,6 +1137,13 @@ static inline void mem_cgroup_calculate_protection(struct mem_cgroup *root, { } +#ifdef CONFIG_LRU_GEN +static inline void mem_cgroup_protection_path (struct mem_cgroup *root, +struct mem_cgroup *memcg) +{ +} +#endif +
I wonder if we could follow the zswap pattern?
#if defined(CONFIG_MEMCG) && defined(CONFIG_ZSWAP)
bool obj_cgroup_may_zswap(struct obj_cgroup *objcg);
void obj_cgroup_charge_zswap(struct obj_cgroup *objcg, size_t size); void obj_cgroup_uncharge_zswap(struct obj_cgroup *objcg, size_t size); bool mem_cgroup_zswap_writeback_enabled(struct mem_cgroup *memcg); #else
static inline bool obj_cgroup_may_zswap(struct obj_cgroup *objcg)
{
return true;
}...
#endif
Quoted 13 lines
include/linux/memcontrol.h +0
-0static inline bool mem_cgroup_unprotected (struct mem_cgroup *target,struct mem_cgroup *memcg) {mm/memcontrol.c +2
-0@@ -5198,6 +5198,51 @@ void mem_cgroup_calculate_protection(struct mem_cgroup *root,page_counter_calculate_protection (&root->memory, &memcg->memory, recursive_protection); } +#ifdef CONFIG_LRU_GEN +/**
mm/memcontrol.c void mem_cgroup_calculate_protection(struct mem_cgroup *root, · line 5212
Quoted 10 lines
mm/memcontrol.c +10
-0+* mem_cgroup_protection_path - compute protection along root->memcg path +* @root: the top ancestor of the sub-tree being checked (NULL for root_mem_cgroup) +* @memcg: the target memory cgroup +* +* Walk the ancestor path from @root down to @memcg and compute the effective +* protection at each level. This is safe for isolated queries because it +* ensures parents are computed before children. +*/ +void mem_cgroup_protection_path (struct mem_cgroup *root, +struct mem_cgroup *memcg)
Can we rename it to mem_cgroup_calculate_protection_path()?
BTW, I see that the only caller is in vmscan and it passes NULL as root. Do we need to keep the root argument if the new helper is only used for global reclaim?
Best Regards
Barry
Quoted 4 lines · Barry Song
Quoted 3 lines · Ridong Chen
From: Ridong Chen <[email protected]>
Quoted 8 lines
Quoted 2 lines
memory.min/low is silently bypassed for MGLRU during global proactive reclaim (writing to the root memory.reclaim) and global direct reclaim.
I guess nobody is silently bypassing anything. It's just that the effective min is stale data. If kswapd has run at least once, should the protection have been updated already?
I guess we need to update the changelog a bit?
The children's emin/elow are derived from the parent's min/low settings and children_min_usage, both of which can change over time. As a result, emin/elow may become stale, even if kswapd has already run once.
Quoted 83 lines
Quoted 63 lines
It can be reproduced as follows:
# echo 7 > /sys/kernel/mm/lru_gen/enabled # cd /sys/fs/cgroup # mkdir -p a/b # echo 100M > a/memory.min # echo +memory > a/cgroup.subtree_control # echo 100M > a/b/memory.min # echo $$ > a/b/cgroup.procs # dd if=/dev/zero of=/tmp/testfile bs=1M count=200 # cat a/b/memory.current 222650368 # echo 500M > memory.reclaim -bash: echo: write error: Resource temporarily unavailable # cat a/b/memory.current 6070272memory.min is 100M, yet reclaim drops a/b down to 6M, breaking the protection. The traditional LRU path is not affected because
shrink_node()callsmem_cgroup_calculate_protection()for each memcg it visits during a top-down tree walk.Commit
30d77b7eef01("mm/mglru: fix ineffective protection calculation") moved the protection computation intolru_gen_age_node(), which only runs for kswapd. Non-kswapd global reclaim reachesshrink_one()throughlru_gen_shrink_node()->shrink_many()without any protection computation, so emin/elow remain stale or zero.Introduce
mem_cgroup_protection_path()which computes emin/elow along the root-to-target path only by iterating through the cgroup ancestors array top-down. This avoids the full tree traversal that would be needed withmem_cgroup_calculate_protection(), limiting the cost to O(depth) per memcg - typically 3-5 levels.Call it from
shrink_one()for the non-kswapd path so that each memcg about to be shrunk has correct protection values.Fixes e4dde56cd208 ("mm: multi-gen LRU: per-node lru_gen_folio lists")Assisted-by Claude:claude-opus-4-8Signed-off-by Ridong Chen <[email protected]>--- include/linux/memcontrol.h | 11 ++++++++++ mm/memcontrol.c | 45 ++++++++++++++++++++++++++++++++++++++ mm/vmscan.c | 8 ++++++- 3 files changed, 63 insertions(+), 1 deletion(-)include/linux/memcontrol.h +4
-0@@ -605,6 +605,10 @@ static inline void mem_cgroup_protection(struct mem_cgroup *root,void mem_cgroup_calculate_protection (struct mem_cgroup *root,struct mem_cgroup *memcg); +#ifdef CONFIG_LRU_GEN +void mem_cgroup_protection_path (struct mem_cgroup *root, +struct mem_cgroup *memcg); +#endif static inline bool mem_cgroup_unprotected (struct mem_cgroup *target,struct mem_cgroup *memcg)include/linux/memcontrol.h static inline void mem_cgroup_calculate_protection(struct mem_cgroup *root, · line 1146
Quoted 10 lines
include/linux/memcontrol.h +7
-0@@ -1133,6 +1137,13 @@ static inline void mem_cgroup_calculate_protection(struct mem_cgroup *root, { } +#ifdef CONFIG_LRU_GEN +static inline void mem_cgroup_protection_path (struct mem_cgroup *root, +struct mem_cgroup *memcg) +{ +} +#endif +I wonder if we could follow the zswap pattern?
#if
defined(CONFIG_MEMCG)&&defined(CONFIG_ZSWAP)
boolobj_cgroup_may_zswap(struct obj_cgroup *objcg);
voidobj_cgroup_charge_zswap(struct obj_cgroup *objcg, size_t size); voidobj_cgroup_uncharge_zswap(struct obj_cgroup *objcg, size_t size); boolmem_cgroup_zswap_writeback_enabled(struct mem_cgroup *memcg); #else
static inline boolobj_cgroup_may_zswap(struct obj_cgroup *objcg)
Quoted 3 lines
{
return true;
}That is fine for me, I will update it.
Quoted 29 lines
...
#endifQuoted 13 lines
include/linux/memcontrol.h +0
-0static inline bool mem_cgroup_unprotected (struct mem_cgroup *target,struct mem_cgroup *memcg) {mm/memcontrol.c +2
-0@@ -5198,6 +5198,51 @@ void mem_cgroup_calculate_protection(struct mem_cgroup *root,page_counter_calculate_protection (&root->memory, &memcg->memory, recursive_protection); } +#ifdef CONFIG_LRU_GEN +/** mm/memcontrol.c void mem_cgroup_calculate_protection(struct mem_cgroup *root, · line 5212
Quoted 10 lines
mm/memcontrol.c +10
-0+* mem_cgroup_protection_path - compute protection along root->memcg path +* @root: the top ancestor of the sub-tree being checked (NULL for root_mem_cgroup) +* @memcg: the target memory cgroup +* +* Walk the ancestor path from @root down to @memcg and compute the effective +* protection at each level. This is safe for isolated queries because it +* ensures parents are computed before children. +*/ +void mem_cgroup_protection_path (struct mem_cgroup *root, +struct mem_cgroup *memcg)Can we rename it to
mem_cgroup_calculate_protection_path()?
Quoted 4 lines
BTW, I see that the only caller is in vmscan and it passes NULL as
root. Do we need to keep therootargument if the new helper is only used for global reclaim?
I'd suggest keeping it as is. This function updates protection along the path from root to memcg, and could be reused later. Note that mem_cgroup_calculate_protection() assumes the caller has already performed the top-down walk, each level's calculation depends on its parent being updated first.
For mem_cgroup_calculate_protection_path(), it can be called in any context without such a precondition.
Signature · 2 lines
Best regards Ridong
Barry Song
reviewerQuoted 17 lines · Ridong Chen
Quoted 4 lines · Barry Song
Quoted 3 lines · Ridong Chen
From: Ridong Chen <[email protected]>
Quoted 8 lines
Quoted 2 lines
memory.min/low is silently bypassed for MGLRU during global proactive reclaim (writing to the root memory.reclaim) and global direct reclaim.
I guess nobody is silently bypassing anything. It's just that the effective min is stale data. If kswapd has run at least once, should the protection have been updated already?
I guess we need to update the changelog a bit?
Quoted 3 lines
The children's emin/elow are derived from the parent's min/low settings and
children_min_usage, both of which can change over time. As a result, emin/elow may become stale, even if kswapd has already run once.
right, let's just say this in changelog, we are *not* bypassing we are just checking
against stable data. The current changelog seems to be misleading.
[...]
Quoted 9 lines
Quoted 9 lines
mm/memcontrol.c void mem_cgroup_calculate_protection(struct mem_cgroup *root, · line 5212
Quoted 2 lines
mm/memcontrol.c +2
-0+void mem_cgroup_protection_path (struct mem_cgroup *root, +struct mem_cgroup *memcg)Can we rename it to
mem_cgroup_calculate_protection_path()?BTW, I see that the only caller is in vmscan and it passes NULL as
root. Do we need to keep therootargument if the new helper is only used for global reclaim?
Quoted 7 lines
I'd suggest keeping it as is. This function updates protection along the path from root to memcg, and could be reused later. Note that
mem_cgroup_calculate_protection()assumes the caller has already performed the top-down walk, each level's calculation depends on its parent being updated first.For
mem_cgroup_calculate_protection_path(), it can be called in any context without such a precondition.
I am fine with this - keeping the root there. but I guess rename is worth it.
Best Regards
Barry
Quoted 18 lines · Barry Song
Quoted 17 lines · Ridong Chen
Quoted 12 lines · Barry Song
Quoted 5 lines · Ridong Chen
From: Ridong Chen <[email protected]>
memory.min/low is silently bypassed for MGLRU during global proactive reclaim (writing to the root memory.reclaim) and global direct reclaim.
I guess nobody is silently bypassing anything. It's just that the effective min is stale data. If kswapd has run at least once, should the protection have been updated already?
I guess we need to update the changelog a bit?
Quoted 8 lines
Quoted 3 lines
The children's emin/elow are derived from the parent's min/low settings and
children_min_usage, both of which can change over time. As a result, emin/elow may become stale, even if kswapd has already run once.right, let's just say this in changelog, we are *not* bypassing we are just checking
against stable data. The current changelog seems to be misleading.
Thanks, Will update.
Quoted 15 lines
[...]
Quoted 14 lines
Quoted 9 lines
mm/memcontrol.c void mem_cgroup_calculate_protection(struct mem_cgroup *root, · line 5212
Quoted 2 lines
mm/memcontrol.c +2
-0+void mem_cgroup_protection_path (struct mem_cgroup *root, +struct mem_cgroup *memcg)Can we rename it to
mem_cgroup_calculate_protection_path()?BTW, I see that the only caller is in vmscan and it passes NULL as
root. Do we need to keep therootargument if the new helper is only used for global reclaim?I'd suggest keeping it as is. This function updates protection along the path from root to memcg, and could be reused later. Note that
mem_cgroup_calculate_protection()assumes the caller has already performed the top-down walk, each level's calculation depends on its parent being updated first.
Quoted 5 lines
Quoted 2 lines
For
mem_cgroup_calculate_protection_path(), it can be called in any context without such a precondition.I am fine with this - keeping the root there. but I guess rename is worth it.
Yeah, I will rename in the next version.
Signature · 2 lines
Best regards Ridong