On Fri 24-04-20 11:10:13, Johannes Weiner wrote:
On Fri, Apr 24, 2020 at 04:29:58PM +0200, Michal Hocko wrote:
On Fri 24-04-20 09:14:50, Johannes Weiner wrote:
On Thu, Apr 23, 2020 at 02:16:29AM -0400, Yafang Shao wrote:
This patch is an improvement of a previous version[1], as the previous version is not easy to understand. This issue persists in the newest kernel, I have to resend the fix. As the implementation is changed, I drop Roman's ack from the previous version.
Now that I understand the problem, I much prefer the previous version.
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 745697906ce3..2bf91ae1e640 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -6332,8 +6332,19 @@ enum mem_cgroup_protection mem_cgroup_protected(struct mem_cgroup *root, if (!root) root = root_mem_cgroup;
- if (memcg == root)
- if (memcg == root) {
/** The cgroup is the reclaim root in this reclaim* cycle, and therefore not protected. But it may have* stale effective protection values from previous* cycles in which it was not the reclaim root - for* example, global reclaim followed by limit reclaim.* Reset these values for mem_cgroup_protection().*/memcg->memory.emin = 0;memcg->memory.elow = 0;- }
Could you be more specific why you prefer this over the mem_cgroup_protection which doesn't change the effective value? Isn't it easier to simply ignore effective value for the reclaim roots?
Because now both mem_cgroup_protection() and mem_cgroup_protected() have to know about the reclaim root semantics, instead of just the one central place.
Yes this is true but it is also potentially overwriting the state with a parallel reclaim which can lead to surprising results beacause parent's effective protection is used to define protection distribution for children. Let's have global and A's reclaim in parallel: | A (low=2G, usage = 3G, max = 3G, children_low_usage = 1.5G) |\ | C (low = 1G, usage = 2.5G) B (low = 1G, usage = 0.5G)
for A reclaim we have B.elow = B.low C.elow = C.low
For the global reclaim A.elow = A.low B.elow = min(B.usage, B.low) because children_low_usage <= A.elow C.elow = min(C.usage, C.low)
With the effective values reseting we have A reclaim A.elow = 0 B.elow = B.low C.elow = C.low [...]
and global reclaim could see the above and then B.elow = C.elow = 0 because children_low_usage > A.elow
And the query function has to know additional rules about when the emin/elow values are uptodate or it could silently be looking at stale data, which isn't very robust.
"The effective protection values are uptodate after calling mem_cgroup_protected() inside the reclaim cycle - UNLESS the group you're looking at happens to be..."
It's much easier to make the rule: The values are uptodate after you called mem_cgroup_protected().
Or mem_cgroup_calculate_protection(), if we go with that later.
As others have noted, it's fairly hard to understand the problem from the above changelog. How about the following:
A cgroup can have both memory protection and a memory limit to isolate it from its siblings in both directions - for example, to prevent it from being shrunk below 2G under high pressure from outside, but also from growing beyond 4G under low pressure.
9783aa9917f8 ("mm, memcg: proportional memory.{low,min} reclaim") implemented proportional scan pressure so that multiple siblings in excess of their protection settings don't get reclaimed equally but instead in accordance to their unprotected portion.
During limit reclaim, this proportionality shouldn't apply of course: there is no competition, all pressure is from within the cgroup and should be applied as such. Reclaim should operate at full efficiency.
However, mem_cgroup_protected() never expected anybody to look at the effective protection values when it indicated that the cgroup is above its protection. As a result, a query during limit reclaim may return stale protection values that were calculated by a previous reclaim cycle in which the cgroup did have siblings.
This is better. Thanks!
When this happens, reclaim is unnecessarily hesitant and potentially slow to meet the desired limit. In theory this could lead to premature OOM kills, although it's not obvious this has occurred in practice.
I do not see how this would lead all the way to OOM killer but it certainly can lead to unnecessary increase of the reclaim priority. The smaller the difference between the reclaim target and protection the more visible the effect would be. But if there are reclaimable pages then the reclaim should see them sooner or later
It would be a pretty extreme case, but not impossible AFAICS, because OOM is just a sampled state, not deterministic.
If memory.max is 64G and memory.low is 64G minus one page, this bug could cause limit reclaim to look at no more than SWAP_CLUSTER_MAX pages at priority 0. It's possible it wouldn't get through the full 64G worth of memory before giving up and declaring OOM.
Yes, my bad I didn't really realize that there won't be a full scan even under priority 0.