Date: Tue, 15 Aug 2017 15:13:50 +0100
From: Roman Gushchin <guro@fb.com>
To: David Rientjes <rientjes@google.com>
CC: <linux-mm@kvack.org>, Michal Hocko <mhocko@kernel.org>,
        Vladimir Davydov <vdavydov.dev@gmail.com>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
        Tejun Heo <tj@kernel.org>, <kernel-team@fb.com>,
        <cgroups@vger.kernel.org>, <linux-doc@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [v5 4/4] mm, oom, docs: describe the cgroup-aware OOM killer
Message-ID: <20170815141350.GA4510@castle.DHCP.thefacebook.com>
References: <20170814183213.12319-1-guro@fb.com>
 <20170814183213.12319-5-guro@fb.com>
 <alpine.DEB.2.10.1708141544280.63207@chino.kir.corp.google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <alpine.DEB.2.10.1708141544280.63207@chino.kir.corp.google.com>
User-Agent: Mutt/1.8.3 (2017-05-23)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Aug 2017 14:14:01.7468 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL2PR15MB1074
X-OriginatorOrg: fb.com
X-Proofpoint-Spam-Reason: safe
X-FB-Internal: Safe
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-08-15_10:,,
 signatures=0
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 4687
Lines: 144

On Mon, Aug 14, 2017 at 03:52:26PM -0700, David Rientjes wrote:
> On Mon, 14 Aug 2017, Roman Gushchin wrote:
> 
> > diff --git a/Documentation/cgroup-v2.txt b/Documentation/cgroup-v2.txt
> > index dec5afdaa36d..22108f31e09d 100644
> > --- a/Documentation/cgroup-v2.txt
> > +++ b/Documentation/cgroup-v2.txt
> > @@ -48,6 +48,7 @@ v1 is available under Documentation/cgroup-v1/.
> >         5-2-1. Memory Interface Files
> >         5-2-2. Usage Guidelines
> >         5-2-3. Memory Ownership
> > +       5-2-4. Cgroup-aware OOM Killer
> 
> Random curiousness, why cgroup-aware oom killer and not memcg-aware oom 
> killer?

I don't think we use the term "memcg" somewhere in v2 docs.
Do you think that "Memory cgroup-aware OOM killer" is better?

> 
> >       5-3. IO
> >         5-3-1. IO Interface Files
> >         5-3-2. Writeback
> > @@ -1002,6 +1003,37 @@ PAGE_SIZE multiple when read back.
> >  	high limit is used and monitored properly, this limit's
> >  	utility is limited to providing the final safety net.
> >  
> > +  memory.oom_kill_all_tasks
> > +
> > +	A read-write single value file which exits on non-root
> 
> s/exits/exists/

Fixed. Thanks!

> 
> > +	cgroups.  The default is "0".
> > +
> > +	Defines whether the OOM killer should treat the cgroup
> > +	as a single entity during the victim selection.
> 
> Isn't this true independent of the memory.oom_kill_all_tasks setting?  
> The cgroup aware oom killer will consider memcg's as logical units when 
> deciding what to kill with or without memory.oom_kill_all_tasks, right?
> 
> I think you cover this fact in the cgroup aware oom killer section below 
> so this might result in confusion if described alongside a setting of
> memory.oom_kill_all_tasks.
> 
> > +
> > +	If set, OOM killer will kill all belonging tasks in
> > +	corresponding cgroup is selected as an OOM victim.
> 
> Maybe
> 
> "If set, the OOM killer will kill all threads attached to the memcg if 
> selected as an OOM victim."
> 
> is better?

Fixed to the following (to conform with core v2 concepts):
  If set, OOM killer will kill all processes attached to the cgroup
  if selected as an OOM victim.

> 
> > +
> > +	Be default, OOM killer respect /proc/pid/oom_score_adj value
> > +	-1000, and will never kill the task, unless oom_kill_all_tasks
> > +	is set.
> > +
> > +  memory.oom_priority
> > +
> > +	A read-write single value file which exits on non-root
> 
> s/exits/exists/

Fixed.

> 
> > +	cgroups.  The default is "0".
> > +
> > +	An integer number within the [-10000, 10000] range,
> > +	which defines the order in which the OOM killer selects victim
> > +	memory cgroups.
> > +
> > +	OOM killer prefers memory cgroups with larger priority if they
> > +	are populated with elegible tasks.
> 
> s/elegible/eligible/

Fixed.

> 
> > +
> > +	The oom_priority value is compared within sibling cgroups.
> > +
> > +	The root cgroup has the oom_priority 0, which cannot be changed.
> > +
> >    memory.events
> >  	A read-only flat-keyed file which exists on non-root cgroups.
> >  	The following entries are defined.  Unless specified
> > @@ -1206,6 +1238,36 @@ POSIX_FADV_DONTNEED to relinquish the ownership of memory areas
> >  belonging to the affected files to ensure correct memory ownership.
> >  
> >  
> > +Cgroup-aware OOM Killer
> > +~~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +Cgroup v2 memory controller implements a cgroup-aware OOM killer.
> > +It means that it treats memory cgroups as first class OOM entities.
> > +
> > +Under OOM conditions the memory controller tries to make the best
> > +choise of a victim, hierarchically looking for the largest memory
> > +consumer. By default, it will look for the biggest task in the
> > +biggest leaf cgroup.
> > +
> > +Be default, all cgroups have oom_priority 0, and OOM killer will
> > +chose the largest cgroup recursively on each level. For non-root
> > +cgroups it's possible to change the oom_priority, and it will cause
> > +the OOM killer to look athe the priority value first, and compare
> > +sizes only of cgroups with equal priority.
> 
> Maybe some description of "largest" would be helpful here?  I think you 
> could briefly describe what is accounted for in the decisionmaking.

I'm afraid that it's too implementation-defined to be described.
Do you have an idea, how to describe it without going too much into details?

> s/athe/at the/

Fixed.

> 
> Reading through this, it makes me wonder if doing s/cgroup/memcg/ over 
> most of it would be better.

I don't think memcg is a good user term, but I agree, that it's necessary
to highlight the fact that a user should enable memory controller to get
this functionality.
Added a corresponding note.

Thanks!

Roman