Re: cgroup-aware OOM killer, how to move forward

From: David Rientjes <rientjes@google.com>
To: Roman Gushchin <guro@fb.com>
Cc: linux-mm@kvack.org, akpm@linux-foundation.org, mhocko@kernel.org,
	hannes@cmpxchg.org, tj@kernel.org, gthelen@google.com
Subject: Re: cgroup-aware OOM killer, how to move forward
Date: Fri, 13 Jul 2018 15:39:15 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.21.1807131535420.202408@chino.kir.corp.google.com> (raw)
In-Reply-To: <20180713221602.GA15005@castle.DHCP.thefacebook.com>

On Fri, 13 Jul 2018, Roman Gushchin wrote:

> > No objection, of course, this was always the mechanism vs policy 
> > separation that I was referring to.  Having the ability to kill all 
> > processes attached to the cgroup when one of its processes is selected is 
> > useful, and we have our own patches that do just that, with the exception 
> > that it's triggerable by the user.
> 
> Perfect! I'll prepare the patchset.
> 

I mean, I separated it out completely in my own 
https://marc.info/?l=linux-kernel&m=152175564704473 as part of a patch 
series that completely fixes all of the issues with the cgroup aware oom 
killer, so of course there's no objection to separating it out.

> > 
> > One of the things that I really like about cgroup v2, though, is what 
> > appears to be an implicit, but rather apparent, goal to minimize the 
> > number of files for each controller.  It's very clean.  So I'd suggest 
> > that we consider memory.group_oom, or however it is named, to allow for 
> > future development.
> > 
> > For example, rather than simply being binary, we'd probably want the 
> > ability to kill all eligible processes attached directly to the victim's 
> > mem cgroup *or* all processes attached to its subtree as well.
> > 
> > I'd suggest it be implemented to accept a string, "default"/"process", 
> > "local" or "tree"/"hierarchy", or better names, to define the group oom 
> > mechanism for the mem cgroup that is oom when one of its processes is 
> > selected as a victim.
> 
> I would prefer to keep it boolean to match the simplicity of cgroup v2 API.
> In v2 hierarchy processes can't be attached to non-leaf cgroups,
> so I don't see the place for the 3rd meaning.
> 

All cgroup v2 files do not need to be boolean and the only way you can add 
a subtree oom kill is to introduce yet another file later.  Please make it 
tristate so that you can define a mechanism of default (process only), 
local cgroup, or subtree, and so we can avoid adding another option later 
that conflicts with the proposed one.  This should be easy.