Date:   Mon, 29 Jan 2018 14:38:02 -0800 (PST)
From:   David Rientjes <rientjes@...gle.com>
To:     Michal Hocko <mhocko@...nel.org>
cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Roman Gushchin <guro@...com>,
        Vladimir Davydov <vdavydov.dev@...il.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        Tejun Heo <tj@...nel.org>, kernel-team@...com,
        cgroups@...r.kernel.org, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [patch -mm v2 1/3] mm, memcg: introduce per-memcg oom policy
 tunable

On Fri, 26 Jan 2018, Michal Hocko wrote:

> > The cgroup aware oom killer is needlessly declared for the entire system
> > by a mount option.  It's unnecessary to force the system into a single
> > oom policy: either cgroup aware, or the traditional process aware.
> > 
> > This patch introduces a memory.oom_policy tunable for all mem cgroups.
> > It is currently a no-op: it can only be set to "none", which is its
> > default policy.  It will be expanded in the next patch to define cgroup
> > aware oom killer behavior.
> > 
> > This is an extensible interface that can be used to define cgroup aware
> > assessment of mem cgroup subtrees or the traditional process aware
> > assessment.
> > 
> 
> So what is the actual semantic and scope of this policy. Does it apply
> only down the hierarchy. Also how do you compare cgroups with different
> policies? Let's say you have
>           root
>          / |  \
>         A  B   C
>        / \    / \
>       D   E  F   G
> 
> Assume A: cgroup, B: oom_group=1, C: tree, G: oom_group=1
> 

At each level of the hierarchy, memory.oom_policy compares immediate 
children, it's the only way that an admin can lock in a specific oom 
policy like "tree" and then delegate the subtree to the user.  If you've 
configured it as above, comparing A and C should be the same based on the 
cumulative usage of their child mem cgroups.

The policy for B hasn't been specified, but since it does not have any 
children "cgroup" and "tree" should be the same.

> Now we have the global OOM killer to choose a victim. From a quick
> glance over those patches, it seems that we will be comparing only
> tasks because root->oom_policy != MEMCG_OOM_POLICY_CGROUP. A, B and C
> policies are ignored.

Right, a policy of "none" reverts its subtree back to per-process 
comparison if you are either not using the cgroup aware oom killer or your 
subtree is not using the cgroup aware oom killer.

> Moreover If I select any of B's tasks then I will
> happily kill it breaking the expectation that the whole memcg will go
> away. Weird, don't you think? Or did I misunderstand?
> 

It's just as weird as the behavior of memory.oom_group today without using 
the mount option :)  In that case, mem_cgroup_select_oom_victim() always 
returns false and the value of memory.oom_group is ignored.  I agree that 
it's weird in -mm and there's nothing preventing us from separating 
memory.oom_group from the cgroup aware oom killer and allowing it to be 
set regardless of a selection change.  If memory.oom_group is set, and the 
kill originates from that mem cgroup, kill all processes attached to it 
and its subtree.

This is a criticism of the current implementation in -mm, however, my 
extension only respects its weirdness.

> So let's assume that root: cgroup. Then we are finally comparing
> cgroups. D, E, B, C. Of those D, E and F do not have any
> policy. Do they inherit their policy from the parent? If they don't then
> we should be comparing their tasks separately, no? The code disagrees
> because once we are in the cgroup mode, we do not care about separate
> tasks.
> 

No, perhaps I wasn't clear in the documentation: the policy at each level 
of the hierarchy is specified by memory.oom_policy and compares its 
immediate children with that policy.  So the per-cgroup usage of A, B, and 
C and compared regardless of A, B, and C's own oom policies.

> Let's say we choose C because it has the largest cumulative consumption.
> It is not oom_group so it will select a task from F, G. Again you are
> breaking oom_group policy of G if you kill a single task. So you would
> have to be recursive here. That sounds fixable though. Just be
> recursive.
> 

I fully agree, but that's (another) implementation detail of what is in 
-mm that isn't specified.  I think where you're going is the complete 
separation of mem cgroup selection from memory.oom_group.  I agree, and we 
can fix that.  memory.oom_group also shouldn't depend on any mount option, 
it can be set or unset depending on the properties of the workload.

> Then you say
> 
> > Another benefit of such an approach is that an admin can lock in a
> > certain policy for the system or for a mem cgroup subtree and can
> > delegate the policy decision to the user to determine if the kill should
> > originate from a subcontainer, as indivisible memory consumers
> > themselves, or selection should be done per process.
> 
> And the code indeed doesn't check oom_policy on each level of the
> hierarchy, unless I am missing something. So the subgroup is simply
> locked in to the oom_policy parent has chosen. That is not the case for
> the tree policy.
> 
> So look how we are comparing cumulative groups without policy with
> groups with policy with subtrees. Either I have grossly misunderstood
> something or this is massively inconsistent and it doesn't make much
> sense to me. Root memcg without cgroup policy will simply turn off the
> whole thing for the global OOM case. So you really need to enable it
> there but then it is not really clear how to configure lower levels.
> 
> From the above it seems that you are more interested in memcg OOMs and
> want to give different hierarchies different policies but you quickly
> hit the similar inconsistencies there as well.
> 
> I am not sure how extensible this is actually. How do we place
> priorities on top?
> 

If you're referring to strict priorities where one cgroup can be preferred 
or biased against regardless of usage, that would be an extension with yet 
another tunable.  Userspace influence over the selection is not addressed 
by this patchset, nor is the unfair comparison of the root mem cgroup with 
leaf mem cgroups.  My suggestion previously was a memory.oom_value 
tunable, which is configured depending on its parent's memory.oom_policy.  
If "cgroup" or "tree" it behaves as oom_score_adj-type behavior, i.e. it's 
an adjustment on the usage.  This way, a subtree's usage can have a 
certain amount of memory discounted, for example, because it is supposed 
to use more than 50% of memory.  If a new "priority" memory.oom_policy 
were implemented, which would be trivial, comparison between child cgroups 
would be as simple as comparing memory.oom_value integers.

Powered by blists - more mailing lists