Re: [PATCH v2 3/3] mm: memcontrol: recursive memory.low protection

[Michal Koutný]

Hello.

On Fri, Feb 21, 2020 at 01:58:39PM -0500, Johannes Weiner <hannes@xxxxxxxxxxx> wrote:
> When you set task's and logger's memory.low to "max" or 10G or any
> bogus number like this, a limit reclaim in job treats this as origin
> protection and tries hard to avoid reclaiming anything in either of
> the two cgroups.
What do you mean by origin protection? (I'm starting to see some
misunderstanding here, c.f. my remark regarding the parent==root
condition in the other patch [1]).

> memory.events::low skyrockets even though no intended
> protection was violated, we'll have reclaim latencies (especially when
> there are a few dying cgroups accumluated in subtree).
Hopefully, I see where are you coming from. There would be no (false)
low notifications if the elow was calculated all they way top-down from
the real root. Would such calculation be the way to go?

> that job can't possibly *know* about the top-level host
> protection that lies beyond the delegation point and outside its own
> namespace,
Yes, I agree.

> and that it needs to propagate protection against rpm upgrades into
> its own leaf groups for each tasklet and component.
If a job wants to use concrete protection than it sets it, if it wants
to use protection from above, then it can express it with the infinity
(after changing the effective calculation I described above).

Now, you may argue that the infinity would be nonsensical if it's not a
subordinate job. Simplest approach would be likely to introduce the
special "inherit" value (such a literal name may be misleading as it
would be also "dont-care").

> Again, in practice we have found this to be totally unmanageable and
> routinely first forgot and then had trouble hacking the propagation
> into random jobs that create their own groups.
I've been bitten by this as well. However, the protection defaults to
off and I find it this matches the general rule that kernel provides the
mechanism and user(space) the policy.

> And when you add new hardware configurations, you cannot just make a
> top-level change in the host config, you have to update all the job
> specs of workloads running in the fleet.
(I acknowledge the current mechanism lacks an explicit way to express
the inherit/dont-care value.)


> My patch brings memory configuration in line with other cgroup2
> controllers.
Other controllers mostly provide the limit or weight controls, I'd say
protection semantics is specific only to the memory controller so
far [2]. I don't think (at least by now) it can be aligned as the weight
or limit semantics.

> I've made the case why it's not a supported usecase, and why it is a
> meaningless configuration in practice due to the way other controllers
> already behave.
I see how your reasoning works for limits (you set memory limit and you
need to control io/cpu too to maintain intended isolation). I'm confused
why having a scapegoat (or donor) sibling for protection should not be
supported or how it breaks protection for others if not combined with
io/cpu controllers. Feel free to point me to the message if I overlooked
it.

> I think at this point in the discussion, the only thing I can do is
> remind you 
I think there is different perception on both sides because of unclear
definitions, so I seek to clarify that.

> that the behavior I'm introducing is gated behind a mount
> option that nobody is forced to enable if they insist on disagreeing
> against all evidence to the contrary.
Sure, but I think it's better to (try reaching|reach) a consensus than
to introduce split behavior (maintenance, debugging).

Thanks,
Michal

[1] https://lore.kernel.org/lkml/20200221171024.GA23476@xxxxxxxxxxxxxxxxx/
[2] I admit I didn't look into io.latency that much and IIRC
    cpu.uclamp.{min,max} don't have the overcommit issue.

Attachment: signature.asc
Description: Digital signature