Messages in this thread
Patch in this message
 From Johannes Weiner <> Subject [PATCH 1/3] mm: memcontrol: fix memory.low proportional distribution Date Thu, 27 Feb 2020 14:56:04 -0500
`When memory.low is overcommitted - i.e. the children claim moreprotection than their shared ancestor grants them - the allowance isdistributed in proportion to how much each sibling uses their owndeclared protection:	low_usage = min(memory.low, memory.current)	elow = parent_elow * (low_usage / siblings_low_usage)However, siblings_low_usage is not the sum of all low_usages. It sumsup the usages of *only those cgroups that are within their memory.low*That means that low_usage can be *bigger* than siblings_low_usage, andconsequently the total protection afforded to the children can bebigger than what the ancestor grants the subtree.Consider three groups where two are in excess of their protection:  A/memory.low = 10G  A/A1/memory.low = 10G, memory.current = 20G  A/A2/memory.low = 10G, memory.current = 20G  A/A3/memory.low = 10G, memory.current =  8G  siblings_low_usage = 8G (only A3 contributes)  A1/elow = parent_elow(10G) * low_usage(10G) / siblings_low_usage(8G) = 12.5G -> 10G  A2/elow = parent_elow(10G) * low_usage(10G) / siblings_low_usage(8G) = 12.5G -> 10G  A3/elow = parent_elow(10G) * low_usage(8G) / siblings_low_usage(8G) = 10.0G  (the 12.5G are capped to the explicit memory.low setting of 10G)With that, the sum of all awarded protection below A is 30G, when Aonly grants 10G for the entire subtree.What does this mean in practice? A1 and A2 would still be in excess oftheir 10G allowance and would be reclaimed, whereas A3 would not. Asthey eventually drop below their protection setting, they would becounted in siblings_low_usage again and the error would right itself.When reclaim was applied in a binary fashion (cgroup is reclaimed whenit's above its protection, otherwise it's skipped) this would actuallywork out just fine. However, since 1bc63fb1272b ("mm, memcg: make scanaggression always exclude protection"), reclaim pressure is scaled tohow much a cgroup is above its protection. As a result thiscalculation error unduly skews pressure away from A1 and A2 toward therest of the system.But why did we do it like this in the first place?The reasoning behind exempting groups in excess fromsiblings_low_usage was to go after them first during reclaim in anovercommitted subtree:  A/memory.low = 2G, memory.current = 4G  A/A1/memory.low = 3G, memory.current = 2G  A/A2/memory.low = 1G, memory.current = 2G  siblings_low_usage = 2G (only A1 contributes)  A1/elow = parent_elow(2G) * low_usage(2G) / siblings_low_usage(2G) = 2G  A2/elow = parent_elow(2G) * low_usage(1G) / siblings_low_usage(2G) = 1GWhile the children combined are overcomitting A and are technicallyboth at fault, A2 is actively declaring unprotected memory and wewould like to reclaim that first.However, while this sounds like a noble goal on the face of it, itdoesn't make much difference in actual memory distribution: Because Ais overcommitted, reclaim will not stop once A2 gets pushed back towithin its allowance; we'll have to reclaim A1 either way. The endresult is still that protection is distributed proportionally, with A1getting 3/4 (1.5G) and A2 getting 1/4 (0.5G) of A's allowance.[ If A weren't overcommitted, it wouldn't make a difference since each  cgroup would just get the protection it declares:  A/memory.low = 2G, memory.current = 3G  A/A1/memory.low = 1G, memory.current = 1G  A/A2/memory.low = 1G, memory.current = 2G  With the current calculation:  siblings_low_usage = 1G (only A1 contributes)  A1/elow = parent_elow(2G) * low_usage(1G) / siblings_low_usage(1G) = 2G -> 1G  A2/elow = parent_elow(2G) * low_usage(1G) / siblings_low_usage(1G) = 2G -> 1G  Including excess groups in siblings_low_usage:  siblings_low_usage = 2G  A1/elow = parent_elow(2G) * low_usage(1G) / siblings_low_usage(2G) = 1G -> 1G  A2/elow = parent_elow(2G) * low_usage(1G) / siblings_low_usage(2G) = 1G -> 1G ]Simplify the calculation and fix the proportional reclaim bug byincluding excess cgroups in siblings_low_usage.After this patch, the effective memory.low distribution from theexample above would be as follows:  A/memory.low = 10G  A/A1/memory.low = 10G, memory.current = 20G  A/A2/memory.low = 10G, memory.current = 20G  A/A3/memory.low = 10G, memory.current =  8G  siblings_low_usage = 28G  A1/elow = parent_elow(10G) * low_usage(10G) / siblings_low_usage(28G) = 3.5G  A2/elow = parent_elow(10G) * low_usage(10G) / siblings_low_usage(28G) = 3.5G  A3/elow = parent_elow(10G) * low_usage(8G) / siblings_low_usage(28G) = 2.8GFixes: 1bc63fb1272b ("mm, memcg: make scan aggression always exclude protection")Fixes: 230671533d64 ("mm: memory.low hierarchical behavior")Acked-by: Tejun Heo <tj@kernel.org>Acked-by: Roman Gushchin <guro@fb.com>Acked-by: Chris Down <chris@chrisdown.name>Acked-by: Michal Hocko <mhocko@suse.com>Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>--- mm/memcontrol.c   |  4 +--- mm/page_counter.c | 12 ++---------- 2 files changed, 3 insertions(+), 13 deletions(-)diff --git a/mm/memcontrol.c b/mm/memcontrol.cindex c5b5f74cfd4d..874a0b00f89b 100644--- a/mm/memcontrol.c+++ b/mm/memcontrol.c@@ -6236,9 +6236,7 @@ struct cgroup_subsys memory_cgrp_subsys = {  * elow = min( memory.low, parent->elow * ------------------ ),  *                                        siblings_low_usage  *- *             | memory.current, if memory.current < memory.low- * low_usage = |- *	       | 0, otherwise.+ * low_usage = min(memory.low, memory.current)  *  *  * Such definition of the effective memory.low provides the expecteddiff --git a/mm/page_counter.c b/mm/page_counter.cindex de31470655f6..75d53f15f040 100644--- a/mm/page_counter.c+++ b/mm/page_counter.c@@ -23,11 +23,7 @@ static void propagate_protected_usage(struct page_counter *c, 		return;  	if (c->min || atomic_long_read(&c->min_usage)) {-		if (usage <= c->min)-			protected = usage;-		else-			protected = 0;-+		protected = min(usage, c->min); 		old_protected = atomic_long_xchg(&c->min_usage, protected); 		delta = protected - old_protected; 		if (delta)@@ -35,11 +31,7 @@ static void propagate_protected_usage(struct page_counter *c, 	}  	if (c->low || atomic_long_read(&c->low_usage)) {-		if (usage <= c->low)-			protected = usage;-		else-			protected = 0;-+		protected = min(usage, c->low); 		old_protected = atomic_long_xchg(&c->low_usage, protected); 		delta = protected - old_protected; 		if (delta)-- 2.24.1`

Last update: 2020-02-27 20:57    [W:0.047 / U:0.624 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site