Re: [PATCH v2 2/5] cgroup: Account for memory_recursiveprot in test_memcg_low()

[David Vernet]

Sorry for the delayed reply, Michal. I've been at LSFMM this week.

On Fri, Apr 29, 2022 at 11:26:20AM +0200, Michal Koutný wrote:
> I still think that the behavior when there's no protection left for the
> memory.low == 0 child, there should be no memory.low events (not just
> uncounted but not happening) and test should not accept this (even
> though it's the current behavior).

That's fair. I think part of the problem here is that in general, the
memcontroller itself is quite heuristic, so it's tough to write tests that
provide useful coverage while also being sufficiently flexible to avoid
flakiness and over-prescribing expected behavior. In this case I think it's
probably correct that the memory.low == 0 child shouldn't inherit
protection from its parent under any circumstances due to its siblings
overcommitting the parent's protection, but I also wonder if it's really
necessary to enforce that. If you look at how much memory A/B/E gets at the
end of the reclaim, it's still far less than 1MB (though should it be 0?).
I'd be curious to hear what Johannes thinks.

> What might improve the test space would be to have two configs like
> 
> Original one (simplified here)
> 	parent		memory.low=50M	memory.current=100M
> 	` child1	memory.low=50M	memory.current=50M
> 	` child2	memory.low=0M	memory.current=50M
> 
> New one (checks events due to recursive protection)
> 	parent		memory.low=50M	memory.current=100M
> 	` child1	memory.low=40M	memory.current=50M
> 	` child2	memory.low=0M	memory.current=50M
> 
> The second config assigns recursive protection to child2 and should
> therefore cause memory.low events in child2 (with memory_recursiveprot
> enabled of course).

Something like this would work, though I think it's useful to specifically
validate the behavior of the memcontroller when the children overcommit the
parent's memory.low protection, which the current test does. So I'm
inclined to keep this testcase, and add your next suggestion:

> Or alternative new one (checks events due to recursive protection)
> 	parent		memory.low=50M	memory.current=100M
> 	` child1	memory.low=0M	memory.current=50M
> 	` child2	memory.low=0M	memory.current=50M

This definitely sounds to me like a useful testcase to add, and I'm happy
to do so in a follow-on patch. If we added this, do you think we need to
keep the check for memory.low events for the memory.low == 0 child in the
overcommit testcase? It arguably helped to catch the SWAP_CLUSTER_MAX
rounding issue you pointed out. Again, curious to hear what Johannes thinks
as well.

Thanks,
David