mm/mglru: fix ineffective memory protection for non-kswapd reclaim
Subsystems (6)
v3 cover mm/mglru: fix ineffective memory protection for non-kswapd reclaim
From: Ridong Chen <[email protected]>
For MGLRU, memory.min/low is not honored during non-kswapd global reclaim (global direct reclaim and root-level memory.reclaim), because these paths shrink memcgs using stale effective protection (emin/elow).
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.
---
v3:
- Per Barry Song's review comments
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 | 10 +++++++++ mm/memcontrol.c | 45 ++++++++++++++++++++++++++++++++++++++ mm/page_counter.c | 19 +++++++++++----- mm/vmscan.c | 8 ++++++- 4 files changed, 75 insertions(+), 7 deletions(-)
base-commit: 32b6ef9a5d0eca44f9cd91f52f4faa89f145a0de --
2.34.1
PATCH v3 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 v3 1/2] mm/page_counter: avoid integer overflow in effective_protection()
Quoted 62 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].
[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-8Reviewed-by Barry Song <[email protected]>Signed-off-by Ridong Chen <[email protected]>--- mm/page_counter.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)mm/page_counter.c +6
-2@@ -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;
mm/page_counter.c static unsigned long effective_protection(unsigned long usage, · line 411
Quoted 10 lines
mm/page_counter.c +6
-3- 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);
Looks correct to me. But a few nits on readability, since this code already is quite painfully complicated.
Please don't do math in the declaration block.
unclaimed made a bit more sense when it held *this group's* final share of the unclaimed protection. As an intermediate, it's *the parent's* unclaimed protection.
Put together, it should look something like this:
unsigned long parent_unclaimed, parent_unprotected, unprotected; parent_unclaimed = parent_effective - siblings_protected; parent_unprotected = parent_usage - siblings_protected; unprotected = usage - protected; /* overflow comment */ unprotected = min(usage - protected, parent_unprotected); ep += mul_u64_u64_div_u64(parent_unclaimed, unprotected, parent_unprotected);
With that,
Re: [PATCH v3 1/2] mm/page_counter: avoid integer overflow in effective_protection()
Quoted 95 lines · Johannes Weiner
Quoted 62 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].
[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-8Reviewed-by Barry Song <[email protected]>Signed-off-by Ridong Chen <[email protected]>--- mm/page_counter.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)mm/page_counter.c +6
-2@@ -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;mm/page_counter.c static unsigned long effective_protection(unsigned long usage, · line 411
Quoted 10 lines
mm/page_counter.c +6
-3- 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);Looks correct to me. But a few nits on readability, since this code already is quite painfully complicated.
Please don't do math in the declaration block.
unclaimedmade a bit more sense when it held *this group's* final share of the unclaimed protection. As an intermediate, it's *the parent's* unclaimed protection.Put together, it should look something like this:
unsigned long parent_unclaimed, parent_unprotected, unprotected; parent_unclaimed = parent_effective - siblings_protected; parent_unprotected = parent_usage - siblings_protected; unprotected = usage - protected; /* overflow comment */ unprotected = min(usage - protected, parent_unprotected); ep += mul_u64_u64_div_u64(parent_unclaimed, unprotected, parent_unprotected);
Quoted 3 lines
With that,
Reviewed-by Johannes Weiner <[email protected]>
Thank you for your suggestion. Will update.
Signature · 2 lines
Best regards Ridong
Re: [PATCH v3 1/2] mm/page_counter: avoid integer overflow in effective_protection()
In reply to Ridong ChenQuoted 45 lines · On Thu, 3 Sep 2026 11:19:51 +0800 Ridong Chen <[email protected]> wrote:
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].
[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-8Reviewed-by Barry Song <[email protected]>Signed-off-by Ridong Chen <[email protected]>--- mm/page_counter.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)mm/page_counter.c +1
-0@@ -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>
mm/page_counter.c static unsigned long effective_protection(unsigned long usage, · line 361
Quoted 6 lines
mm/page_counter.c +2
-1@@ -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);
On 32bit it is only necessary to use a 64bit intermediary. mul_u64_u64_div_u64() will drop back to the (probably faster) 64 by 64 divide (and then maybe to a 64 by 32 one).
But there is a lot of extra code before that happens.
Quoted 14 lines
mm/page_counter.c +3
-3/* * 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;
mm/page_counter.c static unsigned long effective_protection(unsigned long usage, · line 413
Quoted 10 lines
mm/page_counter.c +7
-2- 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);
If the ratio is forced to 1 there is no point doing the scaling. So maybe:
if (likely(parent_unprotected > unprotected)) unclaimed = mul_u64_u64_div_u64(unclaimed, unprotected, parent_unprotected); ep += unclaimed;
OTOH if the min() generates a cmov rather than a conditional branch then you don't get a statically mispredicted branch in the normal case (which is very likely with the empty 'else' branch).
David
Quoted 3 lines
mm/page_counter.c +0
-0}return ep;
Re: [PATCH v3 1/2] mm/page_counter: avoid integer overflow in effective_protection()
Quoted 51 lines · David Laight
Quoted 48 lines · On Thu, 3 Sep 2026 11:19:51 +0800 Ridong Chen <[email protected]> wrote:
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].
[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-8Reviewed-by Barry Song <[email protected]>Signed-off-by Ridong Chen <[email protected]>--- mm/page_counter.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)mm/page_counter.c +1
-0@@ -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)
Quoted 9 lines
mm/page_counter.c static unsigned long effective_protection(unsigned long usage, · line 361
Quoted 3 lines
mm/page_counter.c +2
-1-return protected * parent_effective / siblings_protected; +return mul_u64_u64_div_u64 (protected, parent_effective, + siblings_protected);On 32bit it is only necessary to use a 64bit intermediary.
mul_u64_u64_div_u64()will drop back to the (probably faster) 64 by 64 divide (and then maybe to a 64 by 32 one).
But there is a lot of extra code before that happens.
Hi David,
Thank you for your review.
Do you want me to split this for 32-bit, or is keeping the single mul_u64_u64_div_u64() call for readability fine with you?
Quoted 27 lines
Quoted 14 lines
mm/page_counter.c +3
-3/* * 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;mm/page_counter.c static unsigned long effective_protection(unsigned long usage, · line 413
Quoted 10 lines
mm/page_counter.c +7
-2- 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);If the ratio is forced to 1 there is no point doing the scaling. So maybe:
Quoted 8 lines
if (likely(parent_unprotected > unprotected)) unclaimed = mul_u64_u64_div_u64(unclaimed, unprotected, parent_unprotected); ep += unclaimed;OTOH if the
min()generates a cmov rather than a conditional branch then you don't get a statically mispredicted branch in the normal case (which is very likely with the empty 'else' branch).
I'd rather keep it as-is and avoid the extra branch. Does that work for you?
Quoted 5 lines
Quoted 4 lines
mm/page_counter.c +0
-0}return ep;
Signature · 2 lines
Best regards Ridong
Re: [PATCH v3 1/2] mm/page_counter: avoid integer overflow in effective_protection()
Quoted 62 lines · On Sun, 6 Sep 2026 09:11:24 +0800 Ridong Chen <[email protected]> wrote:
Quoted 51 lines · David Laight
Quoted 48 lines · On Thu, 3 Sep 2026 11:19:51 +0800 Ridong Chen <[email protected]> wrote:
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].
[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-8Reviewed-by Barry Song <[email protected]>Signed-off-by Ridong Chen <[email protected]>--- mm/page_counter.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)mm/page_counter.c +1
-0@@ -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)Quoted 9 lines
mm/page_counter.c static unsigned long effective_protection(unsigned long usage, · line 361
Quoted 3 lines
mm/page_counter.c +2
-1-return protected * parent_effective / siblings_protected; +return mul_u64_u64_div_u64 (protected, parent_effective, + siblings_protected);On 32bit it is only necessary to use a 64bit intermediary.
mul_u64_u64_div_u64()will drop back to the (probably faster) 64 by 64 divide (and then maybe to a 64 by 32 one).
But there is a lot of extra code before that happens.
Quoted 6 lines
Hi David,
Thank you for your review.
Do you want me to split this for 32-bit, or is keeping the single
mul_u64_u64_div_u64()call for readability fine with you?
Maybe I'll look at wrapping mul_u64_add_u64_div_u64() with something that checks statically_true(((a) | (b) | (c) | (d)) < 1ull << 32).
Probably neater than the conditional here.
Quoted 37 lines
Quoted 28 lines
Quoted 15 lines
mm/page_counter.c +3
-3/* * 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;mm/page_counter.c static unsigned long effective_protection(unsigned long usage, · line 413
Quoted 10 lines
mm/page_counter.c +7
-2- 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);If the ratio is forced to 1 there is no point doing the scaling. So maybe:
Quoted 8 lines
if (likely(parent_unprotected > unprotected)) unclaimed = mul_u64_u64_div_u64(unclaimed, unprotected, parent_unprotected); ep += unclaimed;OTOH if the
min()generates a cmov rather than a conditional branch then you don't get a statically mispredicted branch in the normal case (which is very likely with the empty 'else' branch).
Quoted 1 line
I'd rather keep it as-is and avoid the extra branch. Does that work for you?
I nearly came to that conclusion while writing the comment! Actually the same might go for the other three tests - generating a 0 or 1 from a conditional move ought to be better than the possibly mispredicted very unlikely branch.
(I am right in thinking that they should never fail?)
I also suspect the #UD is the least of the problems. You don't want the scaling to produce a bigger number at all.
David
Quoted 7 lines
Quoted 6 lines
Quoted 5 lines
mm/page_counter.c +0
-0}return ep;
Re: [PATCH v3 1/2] mm/page_counter: avoid integer overflow in effective_protection()
Quoted 65 lines · David Laight
Quoted 62 lines · On Sun, 6 Sep 2026 09:11:24 +0800 Ridong Chen<[email protected]> wrote:
Quoted 50 lines · David Laight
Quoted 47 lines · On Thu, 3 Sep 2026 11:19:51 +0800 Ridong Chen<[email protected]> wrote:
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].
[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-8Reviewed-by Barry Song<[email protected]>Signed-off-by Ridong Chen<[email protected]>--- mm/page_counter.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)mm/page_counter.c +1
-0@@ -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. */ Quoted 9 lines
mm/page_counter.c static unsigned long effective_protection(unsigned long usage, · line 361
Quoted 4 lines
mm/page_counter.c +2
-1if (siblings_protected > parent_effective) -return protected * parent_effective / siblings_protected; +return mul_u64_u64_div_u64 (protected, parent_effective, + siblings_protected);On 32bit it is only necessary to use a 64bit intermediary.
mul_u64_u64_div_u64()will drop back to the (probably faster) 64 by 64 divide (and then maybe to a 64 by 32 one).
But there is a lot of extra code before that happens.Hi David,
Quoted 9 lines
Quoted 4 lines
Thank you for your review.
Do you want me to split this for 32-bit, or is keeping the single
mul_u64_u64_div_u64()call for readability fine with you?Maybe I'll look at wrapping
mul_u64_add_u64_div_u64()with something that checksstatically_true(((a) | (b) | (c) | (d)) < 1ull << 32).Probably neater than the conditional here.
Agreed. Other callers face the same 32-bit concern, so a wrapper that does the statically_true(...) check once would be cleaner than open-coding a conditional in each site.
Quoted 34 lines
Quoted 34 lines
Quoted 34 lines
Quoted 14 lines
mm/page_counter.c +3
-3/* * 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;mm/page_counter.c static unsigned long effective_protection(unsigned long usage, · line 413
Quoted 10 lines
mm/page_counter.c +7
-2- 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);If the ratio is forced to 1 there is no point doing the scaling. So maybe:
if (likely(parent_unprotected > unprotected)) unclaimed = mul_u64_u64_div_u64(unclaimed, unprotected, parent_unprotected); ep += unclaimed;OTOH if the
min()generates a cmov rather than a conditional branch then you don't get a statically mispredicted branch in the normal case (which is very likely with the empty 'else' branch).
Quoted 7 lines
Quoted 1 line
I'd rather keep it as-is and avoid the extra branch. Does that work for you?
I nearly came to that conclusion while writing the comment! Actually the same might go for the other three tests - generating a 0 or 1 from a conditional move ought to be better than the possibly mispredicted very unlikely branch.
(I am right in thinking that they should never fail?)
No — these three checks fail all the time in normal operation, so I'd keep the branch:
if (parent_effective > siblings_protected &&
parent_usage > siblings_protected &&
usage > protected) {Checks 1 and 3 just ask "is there anything left to distribute?". Often there isn't, so they fail routinely.
Check 2 is a divide-by-zero guard. parent_usage - siblings_protected is the divisor. A conditional-move can't replace it: the division would run anyway and only get multiplied by zero afterwards, so we'd still divide by zero when the two are equal. The branch is what stops us reaching the division.
Quoted 3 lines
I also suspect the #UD is the least of the problems. You don't want the scaling to produce a bigger number at all.
Agreed, and the min() already ensures that. It caps the value so the result can never be larger than unclaimed. Avoiding the trap is just a bonus of that same cap.
So I'll keep the patch as-is. Thanks for the review!
Signature · 2 lines
Best regards Ridong
PATCH v3 2/2 mm/mglru: fix ineffective memory protection for non-kswapd reclaim
In reply to Ridong ChenFrom: Ridong Chen <[email protected]>
For MGLRU, memory.min/low is not honored during global proactive reclaim (writing to the root memory.reclaim) and global direct reclaim, because these paths shrink memcgs using stale or effective protection (emin/elow). 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 are whatever a previous kswapd run left behind
- or zero if kswapd never ran on this node. Relying on a prior kswapd pass is not correct either: a memcg's emin/elow are derived from its ancestors' memory.min/low settings and from children_min_usage, both of which change over time, so emin/elow go stale even after kswapd has run and must be recomputed at the point of reclaim.
Introduce mem_cgroup_calculate_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 | 10 +++++++++ mm/memcontrol.c | 45 ++++++++++++++++++++++++++++++++++++++ mm/vmscan.c | 8 ++++++- 3 files changed, 62 insertions(+), 1 deletion(-)
include/linux/memcontrol.h +10 -0
@@ -1885,6 +1885,16 @@ static inline bool memcg_is_dying(struct mem_cgroup *memcg) }#endif /* CONFIG_MEMCG */ +#if defined(CONFIG_MEMCG ) && defined(CONFIG_LRU_GEN ) +void mem_cgroup_calculate_protection_path (struct mem_cgroup *root, +struct mem_cgroup *memcg); +#else +static inline void mem_cgroup_calculate_protection_path (struct mem_cgroup *root, +struct mem_cgroup *memcg) +{ +} +#endif +#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);
mm/memcontrol.c +45 -0
51 lines not on this page · raw message
mm/vmscan.c +7 -1
@@ -5111,7 +5111,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_calculate_protection_path (NULL , memcg); +if (mem_cgroup_below_min (NULL , memcg))return MEMCG_LRU_YOUNG ;
Signature · 1 line
2.34.1
Quoted 6 lines · Ridong Chen
From: Ridong Chen <[email protected]>
For MGLRU, memory.min/low is not honored during global proactive reclaim (writing to the root memory.reclaim) and global direct reclaim, because these paths shrink memcgs using stale or effective protection (emin/elow).
Are you sure it should be “stale or effective”? My gut feeling is that we don't need the “or” here.
[...]
Quoted 6 lines
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]>---
With a few nits:
[...]
mm/vmscan.c static int shrink_one(struct lruvec *lruvec, struct scan_control *sc) · line 5117
Quoted 13 lines
mm/vmscan.c +4
-1@@ -5111,7 +5111,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() +*/
Could be clearer:
/*
* For kswapd, mem_cgroup_calculate_protection() has already
* been called during the top-down cgroup traversal. */
mm/vmscan.c static int shrink_one(struct lruvec *lruvec, struct scan_control *sc) · line 5119
Quoted 2 lines
mm/vmscan.c +2
-0+if (!current_is_kswapd ()) +mem_cgroup_calculate_protection_path (NULL , memcg);
Best Regards
Barry
Quoted 11 lines · Barry Song
Quoted 6 lines · Ridong Chen
From: Ridong Chen <[email protected]>
For MGLRU, memory.min/low is not honored during global proactive reclaim (writing to the root memory.reclaim) and global direct reclaim, because these paths shrink memcgs using stale or effective protection (emin/elow).
Are you sure it should be “stale or effective”? My gut feeling is that we don't need the “or” here.
My bad. It was a typo. Will remove "or".
Quoted 29 lines
[...]
Quoted 6 lines
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]>---
With a few nits:
Reviewed-by Barry Song <[email protected]>[...]
mm/vmscan.c static int shrink_one(struct lruvec *lruvec, struct scan_control *sc) · line 5117
Quoted 13 lines
mm/vmscan.c +4
-1@@ -5111,7 +5111,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() +*/ Could be clearer:
Quoted 5 lines
/*
* For kswapd,mem_cgroup_calculate_protection()has already
* been called during the top-down cgroup traversal. */
Thank you, will update.
Quoted 5 lines
mm/vmscan.c static int shrink_one(struct lruvec *lruvec, struct scan_control *sc) · line 5119
Quoted 2 lines
mm/vmscan.c +2
-0+if (!current_is_kswapd ()) +mem_cgroup_calculate_protection_path (NULL , memcg);Best Regards
Barry
Signature · 2 lines
Best regards Ridong
Quoted 44 lines · Ridong Chen
From: Ridong Chen <[email protected]>
For MGLRU, memory.min/low is not honored during global proactive reclaim (writing to the root memory.reclaim) and global direct reclaim, because these paths shrink memcgs using stale or effective protection (emin/elow). 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 are whatever a previous kswapd run left behind
- or zero if kswapd never ran on this node. Relying on a prior kswapd pass is not correct either: a memcg's emin/elow are derived from its ancestors' memory.min/low settings and fromchildren_min_usage, both of which change over time, so emin/elow go stale even after kswapd has run and must be recomputed at the point of reclaim.Introduce
mem_cgroup_calculate_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.
Quoted 7 lines
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]>
With Barry's two points of feedback (changelog and comment),