[RFC PATCH v2 1/7] mm, oom: refactor select_bad_process() to take memcg as an argument

[RFC PATCH v2 1/7] mm, oom: refactor select_bad_process() to take memcg as an argument

[Roman Gushchin]

The select_bad_process() function will be used further
to select a process to kill in the victim cgroup.
This cgroup doesn't necessary match oc->memcg,
which is a cgroup, which limits were caused cgroup-wide OOM
(or NULL in case of global OOM).

So, refactor select_bad_process() to take a pointer to
a cgroup to iterate over as an argument.

Signed-off-by: Roman Gushchin <guro@fb.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Li Zefan <lizefan@huawei.com>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: kernel-team@fb.com
Cc: cgroups@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-mm@kvack.org
---
 mm/oom_kill.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 04c9143..f8b0fb1 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -343,10 +343,11 @@ static int oom_evaluate_task(struct task_struct *task, void *arg)
  * Simple selection loop. We choose the process with the highest number of
  * 'points'. In case scan was aborted, oc->chosen is set to -1.
  */
-static void select_bad_process(struct oom_control *oc)
+static void select_bad_process(struct oom_control *oc,
+			       struct mem_cgroup *memcg)
 {
-	if (is_memcg_oom(oc))
-		mem_cgroup_scan_tasks(oc->memcg, oom_evaluate_task, oc);
+	if (memcg)
+		mem_cgroup_scan_tasks(memcg, oom_evaluate_task, oc);
 	else {
 		struct task_struct *p;
 
@@ -1032,7 +1033,7 @@ bool out_of_memory(struct oom_control *oc)
 		return true;
 	}
 
-	select_bad_process(oc);
+	select_bad_process(oc, oc->memcg);
 	/* Found nothing?!?! Either we hang forever, or we panic. */
 	if (!oc->chosen && !is_sysrq_oom(oc) && !is_memcg_oom(oc)) {
 		dump_header(oc, NULL);
-- 
2.7.4

Re: [RFC PATCH v2 1/7] mm, oom: refactor select_bad_process() to take memcg as an argument

[Vladimir Davydov]

On Thu, Jun 01, 2017 at 07:35:09PM +0100, Roman Gushchin wrote:
> The select_bad_process() function will be used further
> to select a process to kill in the victim cgroup.
> This cgroup doesn't necessary match oc->memcg,
> which is a cgroup, which limits were caused cgroup-wide OOM
> (or NULL in case of global OOM).
> 
> So, refactor select_bad_process() to take a pointer to
> a cgroup to iterate over as an argument.

IMHO this patch, as well as patches 2-5, doesn't deserve to be submitted
separately: none of them make sense as a separate change; worse, patches
4 and 5 introduce user API that doesn't do anything without patch 6. All
of the changes are relatively small and singling them out doesn't really
facilitate review, so I'd merge them all in patch 6.

Re: [RFC PATCH v2 1/7] mm, oom: refactor select_bad_process() to take memcg as an argument

[David Rientjes]

We use a heavily modified system and memcg oom killer and I'm wondering
if there is some opportunity for collaboration because we may have some
shared goals.

I can summarize how we currently use the oom killer at a high level so
that it is not overwhelming with implementation details and give some
rationale for why we have converged onto this strategy over the period of
a few years.

For victim selection, we have strict priority based oom killing both at
the memcg level and the process level.

Each process has its own "badness" value that is independent of
oom_score_adj, although some conversion is done if a third-party thread
chooses to disable itself from oom killing for backwards compatibility.
Lower values are more preferable victims, but that detail should not
matter significantly.  If two processes share the same badness value,
tiebreaks are done by selecting the largest rss.

Each memcg in a hierarchy also has its own badness value which
semantically means the same as the per-process value, although it
considers the entire memcg as a unit, similar to your approach, when
iterating the hierarchy to choose a process.  The benefit of the
per-memcg and per-process approach is that you can kill the lowest
priority process from the lowest priority memcg.

The above scoring is enabled with a VM sysctl for the system and is used
for both system (global) and memcg oom kills.  For system overcommit,
this means we can kill the lowest priority job on the system; for memcg,
we can allow users to define their oom kill priorities at each level of
their own hierarchy.

When the system or root of an oom memcg hierarchy encounters its limit,
we iterate each level of the memcg hierarchy to find the lowest priority
job.  This is done by comparing the badness of the sibling memcgs at
each level, finding the lowest, and iterating that subtree.  If there are
lower priority processes per the per-process badness value compared to
all sibling memcgs, that process is killed.

We also have complete userspace oom handling support.  This complements
the existing memory.oom_control notification when a memcg is oom with a
separate notifier that notifies when the kernel has oom killed a process.
It is possible to delay the oom killer from killing a process for memcg
oom kills with a configurable, per-memcg, oom delay.  If set, the kernel
will wait for userspace to respond to its oom notification and effect its
own policy decisions until memory is uncharged to that memcg hierarchy,
or the oom delay expires.  If the oom delay expires, the kernel oom
killer kills a process based on badness.

Our oom kill notification file used to get an fd to register with
cgroup.event_control also provides oom kill statistics based on system,
memcg, local, hierarchical, and user-induced oom kills when read().

We also have a convenient "kill all" knob that userspace can write when
handling oom conditions to iterate all threads attached to a particular
memcg and kill them.  This is merely to prevent races where userspace
does the oom killing itself, which is not problematic in itself, but
additional tasks continue to be attached to an oom memcg.

A caveat here is that we also support fully inclusive kmem accounting to
memcg hierarchies, so we call the oom killer as part of the memcg charge
path rather than only upon returning from fault with VM_FAULT_OOM.  We
have our own oom killer livelock detection that isn't necessarily
important in this thread, but we haven't encountered a situation where we
livelock by calling the oom killer during charge, and this is a
requirement for memcg charging as part of slab allocation.

I could post many patches to implement all of this functionality that we
have used for a few years, but I first wanted to send this email to see
if there is any common ground or to better understand your methodology
for using the kernel oom killer for both system and memcg oom kills.

Otherwise, very interesting stuff!

Re: [RFC PATCH v2 1/7] mm, oom: refactor select_bad_process() to take memcg as an argument

[Roman Gushchin]

On Sun, Jun 04, 2017 at 03:50:37PM -0700, David Rientjes wrote:
> We use a heavily modified system and memcg oom killer and I'm wondering
> if there is some opportunity for collaboration because we may have some
> shared goals.
> 
> I can summarize how we currently use the oom killer at a high level so
> that it is not overwhelming with implementation details and give some
> rationale for why we have converged onto this strategy over the period of
> a few years.
> 
> For victim selection, we have strict priority based oom killing both at
> the memcg level and the process level.
> 
> Each process has its own "badness" value that is independent of
> oom_score_adj, although some conversion is done if a third-party thread
> chooses to disable itself from oom killing for backwards compatibility.
> Lower values are more preferable victims, but that detail should not
> matter significantly.  If two processes share the same badness value,
> tiebreaks are done by selecting the largest rss.
> 
> Each memcg in a hierarchy also has its own badness value which
> semantically means the same as the per-process value, although it
> considers the entire memcg as a unit, similar to your approach, when
> iterating the hierarchy to choose a process.  The benefit of the
> per-memcg and per-process approach is that you can kill the lowest
> priority process from the lowest priority memcg.
> 
> The above scoring is enabled with a VM sysctl for the system and is used
> for both system (global) and memcg oom kills.  For system overcommit,
> this means we can kill the lowest priority job on the system; for memcg,
> we can allow users to define their oom kill priorities at each level of
> their own hierarchy.
> 
> When the system or root of an oom memcg hierarchy encounters its limit,
> we iterate each level of the memcg hierarchy to find the lowest priority
> job.  This is done by comparing the badness of the sibling memcgs at
> each level, finding the lowest, and iterating that subtree.  If there are
> lower priority processes per the per-process badness value compared to
> all sibling memcgs, that process is killed.
> 
> We also have complete userspace oom handling support.  This complements
> the existing memory.oom_control notification when a memcg is oom with a
> separate notifier that notifies when the kernel has oom killed a process.
> It is possible to delay the oom killer from killing a process for memcg
> oom kills with a configurable, per-memcg, oom delay.  If set, the kernel
> will wait for userspace to respond to its oom notification and effect its
> own policy decisions until memory is uncharged to that memcg hierarchy,
> or the oom delay expires.  If the oom delay expires, the kernel oom
> killer kills a process based on badness.
> 
> Our oom kill notification file used to get an fd to register with
> cgroup.event_control also provides oom kill statistics based on system,
> memcg, local, hierarchical, and user-induced oom kills when read().
> 
> We also have a convenient "kill all" knob that userspace can write when
> handling oom conditions to iterate all threads attached to a particular
> memcg and kill them.  This is merely to prevent races where userspace
> does the oom killing itself, which is not problematic in itself, but
> additional tasks continue to be attached to an oom memcg.
> 
> A caveat here is that we also support fully inclusive kmem accounting to
> memcg hierarchies, so we call the oom killer as part of the memcg charge
> path rather than only upon returning from fault with VM_FAULT_OOM.  We
> have our own oom killer livelock detection that isn't necessarily
> important in this thread, but we haven't encountered a situation where we
> livelock by calling the oom killer during charge, and this is a
> requirement for memcg charging as part of slab allocation.
> 
> I could post many patches to implement all of this functionality that we
> have used for a few years, but I first wanted to send this email to see
> if there is any common ground or to better understand your methodology
> for using the kernel oom killer for both system and memcg oom kills.
> 
> Otherwise, very interesting stuff!

Hi David!

Thank you for sharing this!

It's very interesting, and it looks like,
it's not that far from what I've suggested.

So we definitily need to come up with some common solution.

Thanks!

Roman

Re: [RFC PATCH v2 1/7] mm, oom: refactor select_bad_process() to take memcg as an argument

[David Rientjes]

On Tue, 6 Jun 2017, Roman Gushchin wrote:

> Hi David!
> 
> Thank you for sharing this!
> 
> It's very interesting, and it looks like,
> it's not that far from what I've suggested.
> 
> So we definitily need to come up with some common solution.
> 

Hi Roman,

Yes, definitely.  I could post a series of patches to do everything that 
was listed in my email sans the fully inclusive kmem accounting, which may 
be pursued at a later date, if it would be helpful to see where there is 
common ground?

Another question is what you think about userspace oom handling?  We 
implement our own oom kill policies in userspace for both the system and 
for user-controlled memcg hierarchies because it often does not match the 
kernel implementation and there is some action that can be taken other 
than killing a process.  Have you tried to implement functionality to do 
userspace oom handling, or are you considering it?  This is the main 
motivation behind allowing an oom delay to be configured.

Re: [RFC PATCH v2 1/7] mm, oom: refactor select_bad_process() to take memcg as an argument

[Roman Gushchin]

On Tue, Jun 06, 2017 at 01:42:29PM -0700, David Rientjes wrote:
> On Tue, 6 Jun 2017, Roman Gushchin wrote:
> 
> > Hi David!
> > 
> > Thank you for sharing this!
> > 
> > It's very interesting, and it looks like,
> > it's not that far from what I've suggested.
> > 
> > So we definitily need to come up with some common solution.
> > 
> 

Hi David,

> Yes, definitely.  I could post a series of patches to do everything that 
> was listed in my email sans the fully inclusive kmem accounting, which may 
> be pursued at a later date, if it would be helpful to see where there is 
> common ground?
> 
> Another question is what you think about userspace oom handling?  We 
> implement our own oom kill policies in userspace for both the system and 
> for user-controlled memcg hierarchies because it often does not match the 
> kernel implementation and there is some action that can be taken other 
> than killing a process.  Have you tried to implement functionality to do 
> userspace oom handling, or are you considering it?  This is the main 
> motivation behind allowing an oom delay to be configured.

cgroup v2 memory controller is built on the idea of preventing OOMs
by using the memory.high limit. This allows an userspace app to get notified
before OOM happens (by looking at memory.events control), so there is (hopefully)
no need in things like oom delay.

Actually, I'm trying to implement some minimal functionality in the kernel,
which will simplify and make more consistent the userspace part of the job.
But, of course, the main goal of the patchset is to fix the unfairness
of the current victim selection.

Thanks!

Roman

[RFC PATCH v2 2/7] mm, oom: split oom_kill_process() and export __oom_kill_process()

[Roman Gushchin]

The oom_kill_process() function consists of two logical parts:
a first one is responsible for considering task's children as
a potential victim and some debug output.
The second half is responsible for sending SIGKILL to all
tasks sharing mm with the given victim.

This commit splits the oom_kill_process() function
and exports __oom_kill_process (second half) to reuse it
from the memory cgroup code.

Signed-off-by: Roman Gushchin <guro@fb.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Li Zefan <lizefan@huawei.com>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: kernel-team@fb.com
Cc: cgroups@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-mm@kvack.org
---
 include/linux/oom.h |   2 +
 mm/oom_kill.c       | 121 +++++++++++++++++++++++++++-------------------------
 2 files changed, 66 insertions(+), 57 deletions(-)

diff --git a/include/linux/oom.h b/include/linux/oom.h
index 8a266e2..47d9495 100644
--- a/include/linux/oom.h
+++ b/include/linux/oom.h
@@ -79,6 +79,8 @@ extern void oom_killer_enable(void);
 
 extern struct task_struct *find_lock_task_mm(struct task_struct *p);
 
+extern void __oom_kill_process(struct task_struct *victim);
+
 /* sysctls */
 extern int sysctl_oom_dump_tasks;
 extern int sysctl_oom_kill_allocating_task;
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index f8b0fb1..11b60a5 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -803,67 +803,12 @@ static bool task_will_free_mem(struct task_struct *task)
 	return ret;
 }
 
-static void oom_kill_process(struct oom_control *oc, const char *message)
+void __oom_kill_process(struct task_struct *victim)
 {
-	struct task_struct *p = oc->chosen;
-	unsigned int points = oc->chosen_points;
-	struct task_struct *victim = p;
-	struct task_struct *child;
-	struct task_struct *t;
+	struct task_struct *p;
 	struct mm_struct *mm;
-	unsigned int victim_points = 0;
-	static DEFINE_RATELIMIT_STATE(oom_rs, DEFAULT_RATELIMIT_INTERVAL,
-					      DEFAULT_RATELIMIT_BURST);
 	bool can_oom_reap = true;
 
-	/*
-	 * If the task is already exiting, don't alarm the sysadmin or kill
-	 * its children or threads, just set TIF_MEMDIE so it can die quickly
-	 */
-	task_lock(p);
-	if (task_will_free_mem(p)) {
-		mark_oom_victim(p);
-		wake_oom_reaper(p);
-		task_unlock(p);
-		put_task_struct(p);
-		return;
-	}
-	task_unlock(p);
-
-	if (__ratelimit(&oom_rs))
-		dump_header(oc, p);
-
-	pr_err("%s: Kill process %d (%s) score %u or sacrifice child\n",
-		message, task_pid_nr(p), p->comm, points);
-
-	/*
-	 * If any of p's children has a different mm and is eligible for kill,
-	 * the one with the highest oom_badness() score is sacrificed for its
-	 * parent.  This attempts to lose the minimal amount of work done while
-	 * still freeing memory.
-	 */
-	read_lock(&tasklist_lock);
-	for_each_thread(p, t) {
-		list_for_each_entry(child, &t->children, sibling) {
-			unsigned int child_points;
-
-			if (process_shares_mm(child, p->mm))
-				continue;
-			/*
-			 * oom_badness() returns 0 if the thread is unkillable
-			 */
-			child_points = oom_badness(child,
-				oc->memcg, oc->nodemask, oc->totalpages);
-			if (child_points > victim_points) {
-				put_task_struct(victim);
-				victim = child;
-				victim_points = child_points;
-				get_task_struct(victim);
-			}
-		}
-	}
-	read_unlock(&tasklist_lock);
-
 	p = find_lock_task_mm(victim);
 	if (!p) {
 		put_task_struct(victim);
@@ -930,6 +875,68 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
 	mmdrop(mm);
 	put_task_struct(victim);
 }
+
+static void oom_kill_process(struct oom_control *oc, const char *message)
+{
+	struct task_struct *p = oc->chosen;
+	unsigned int points = oc->chosen_points;
+	struct task_struct *victim = p;
+	struct task_struct *child;
+	struct task_struct *t;
+	unsigned int victim_points = 0;
+	static DEFINE_RATELIMIT_STATE(oom_rs, DEFAULT_RATELIMIT_INTERVAL,
+					      DEFAULT_RATELIMIT_BURST);
+
+	/*
+	 * If the task is already exiting, don't alarm the sysadmin or kill
+	 * its children or threads, just set TIF_MEMDIE so it can die quickly
+	 */
+	task_lock(p);
+	if (task_will_free_mem(p)) {
+		mark_oom_victim(p);
+		wake_oom_reaper(p);
+		task_unlock(p);
+		put_task_struct(p);
+		return;
+	}
+	task_unlock(p);
+
+	if (__ratelimit(&oom_rs))
+		dump_header(oc, p);
+
+	pr_err("%s: Kill process %d (%s) score %u or sacrifice child\n",
+		message, task_pid_nr(p), p->comm, points);
+
+	/*
+	 * If any of p's children has a different mm and is eligible for kill,
+	 * the one with the highest oom_badness() score is sacrificed for its
+	 * parent.  This attempts to lose the minimal amount of work done while
+	 * still freeing memory.
+	 */
+	read_lock(&tasklist_lock);
+	for_each_thread(p, t) {
+		list_for_each_entry(child, &t->children, sibling) {
+			unsigned int child_points;
+
+			if (process_shares_mm(child, p->mm))
+				continue;
+			/*
+			 * oom_badness() returns 0 if the thread is unkillable
+			 */
+			child_points = oom_badness(child,
+				oc->memcg, oc->nodemask, oc->totalpages);
+			if (child_points > victim_points) {
+				put_task_struct(victim);
+				victim = child;
+				victim_points = child_points;
+				get_task_struct(victim);
+			}
+		}
+	}
+	read_unlock(&tasklist_lock);
+
+	__oom_kill_process(victim);
+}
 #undef K
 
 /*
-- 
2.7.4

[RFC PATCH v2 3/7] mm, oom: export oom_evaluate_task() and select_bad_process()

[Roman Gushchin]

Export the oom_evaluate_task() and select_bad_process()
functions to reuse them for victim selection logic
in the cgroup-aware OOM killer.

Signed-off-by: Roman Gushchin <guro@fb.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Li Zefan <lizefan@huawei.com>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: kernel-team@fb.com
Cc: cgroups@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-mm@kvack.org
---
 include/linux/oom.h | 3 +++
 mm/oom_kill.c       | 5 ++---
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/linux/oom.h b/include/linux/oom.h
index 47d9495..edf7a77 100644
--- a/include/linux/oom.h
+++ b/include/linux/oom.h
@@ -80,6 +80,9 @@ extern void oom_killer_enable(void);
 extern struct task_struct *find_lock_task_mm(struct task_struct *p);
 
 extern void __oom_kill_process(struct task_struct *victim);
+extern int oom_evaluate_task(struct task_struct *task, void *arg);
+extern void select_bad_process(struct oom_control *oc,
+			       struct mem_cgroup *memcg);
 
 /* sysctls */
 extern int sysctl_oom_dump_tasks;
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 11b60a5..8cf77fb 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -288,7 +288,7 @@ static enum oom_constraint constrained_alloc(struct oom_control *oc)
 	return CONSTRAINT_NONE;
 }
 
-static int oom_evaluate_task(struct task_struct *task, void *arg)
+int oom_evaluate_task(struct task_struct *task, void *arg)
 {
 	struct oom_control *oc = arg;
 	unsigned long points;
@@ -343,8 +343,7 @@ static int oom_evaluate_task(struct task_struct *task, void *arg)
  * Simple selection loop. We choose the process with the highest number of
  * 'points'. In case scan was aborted, oc->chosen is set to -1.
  */
-static void select_bad_process(struct oom_control *oc,
-			       struct mem_cgroup *memcg)
+void select_bad_process(struct oom_control *oc, struct mem_cgroup *memcg)
 {
 	if (memcg)
 		mem_cgroup_scan_tasks(memcg, oom_evaluate_task, oc);
-- 
2.7.4

[RFC PATCH v2 4/7] mm, oom: introduce oom_kill_all_tasks option for memory cgroups

[Roman Gushchin]

This option defines whether a cgroup should be treated
as a single entity by the OOM killer.

If set, the OOM killer will compare the whole cgroup with other
memory consumers (other cgroups and tasks in the root cgroup),
and in case of an OOM will kill all belonging tasks.

Disabled by default.

Signed-off-by: Roman Gushchin <guro@fb.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Li Zefan <lizefan@huawei.com>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: kernel-team@fb.com
Cc: cgroups@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-mm@kvack.org
---
 include/linux/memcontrol.h |  3 +++
 mm/memcontrol.c            | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 899949b..8a308c9 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -195,6 +195,9 @@ struct mem_cgroup {
 	/* OOM-Killer disable */
 	int		oom_kill_disable;
 
+	/* kill all tasks below in case of OOM */
+	bool oom_kill_all_tasks;
+
 	/* handle for "memory.events" */
 	struct cgroup_file events_file;
 
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index c131f7e..d4ffa79 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -5156,6 +5156,33 @@ static ssize_t memory_max_write(struct kernfs_open_file *of,
 	return nbytes;
 }
 
+static int memory_oom_kill_all_tasks_show(struct seq_file *m, void *v)
+{
+	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(m));
+	bool oom_kill_all_tasks = memcg->oom_kill_all_tasks;
+
+	seq_printf(m, "%d\n", oom_kill_all_tasks);
+
+	return 0;
+}
+
+static ssize_t memory_oom_kill_all_tasks_write(struct kernfs_open_file *of,
+					       char *buf, size_t nbytes,
+					       loff_t off)
+{
+	struct mem_cgroup *memcg = mem_cgroup_from_css(of_css(of));
+	int oom_kill_all_tasks;
+	int err;
+
+	err = kstrtoint(strstrip(buf), 0, &oom_kill_all_tasks);
+	if (err)
+		return err;
+
+	memcg->oom_kill_all_tasks = !!oom_kill_all_tasks;
+
+	return nbytes;
+}
+
 static int memory_events_show(struct seq_file *m, void *v)
 {
 	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(m));
@@ -5265,6 +5292,12 @@ static struct cftype memory_files[] = {
 		.write = memory_max_write,
 	},
 	{
+		.name = "oom_kill_all_tasks",
+		.flags = CFTYPE_NOT_ON_ROOT,
+		.seq_show = memory_oom_kill_all_tasks_show,
+		.write = memory_oom_kill_all_tasks_write,
+	},
+	{
 		.name = "events",
 		.flags = CFTYPE_NOT_ON_ROOT,
 		.file_offset = offsetof(struct mem_cgroup, events_file),
-- 
2.7.4

Re: [RFC PATCH v2 4/7] mm, oom: introduce oom_kill_all_tasks option for memory cgroups

[Vladimir Davydov]

On Thu, Jun 01, 2017 at 07:35:12PM +0100, Roman Gushchin wrote:
> This option defines whether a cgroup should be treated
> as a single entity by the OOM killer.
> 
> If set, the OOM killer will compare the whole cgroup with other
> memory consumers (other cgroups and tasks in the root cgroup),
> and in case of an OOM will kill all belonging tasks.
> 
> Disabled by default.
> 
...
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> @@ -5265,6 +5292,12 @@ static struct cftype memory_files[] = {
>  		.write = memory_max_write,
>  	},
>  	{
> +		.name = "oom_kill_all_tasks",
> +		.flags = CFTYPE_NOT_ON_ROOT,
> +		.seq_show = memory_oom_kill_all_tasks_show,
> +		.write = memory_oom_kill_all_tasks_write,
> +	},
> +	{
>  		.name = "events",
>  		.flags = CFTYPE_NOT_ON_ROOT,
>  		.file_offset = offsetof(struct mem_cgroup, events_file),

I don't really like the name of the new knob, but can't come up with
anything better :-( May be, drop '_tasks' suffix and call it just
'oom_kill_all'? Or perhaps we should emphasize the fact that this
cgroup is treated as a single entity by the OOM killer by calling it
'oom_entity' or 'oom_unit'? Dunno...

[RFC PATCH v2 5/7] mm, oom: introduce oom_score_adj for memory cgroups

[Roman Gushchin]

Introduce a per-memory-cgroup oom_score_adj setting.
A read-write single value file which exits on non-root
cgroups. The default is "0".

It will have a similar meaning to a per-process value,
available via /proc/<pid>/oom_score_adj.
Should be in a range [-1000, 1000].

Signed-off-by: Roman Gushchin <guro@fb.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Li Zefan <lizefan@huawei.com>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: kernel-team@fb.com
Cc: cgroups@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-mm@kvack.org
---
 include/linux/memcontrol.h |  3 +++
 mm/memcontrol.c            | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 8a308c9..818a42e 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -198,6 +198,9 @@ struct mem_cgroup {
 	/* kill all tasks below in case of OOM */
 	bool oom_kill_all_tasks;
 
+	/* OOM kill score adjustment */
+	short oom_score_adj;
+
 	/* handle for "memory.events" */
 	struct cgroup_file events_file;
 
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index d4ffa79..f979ac7 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -5183,6 +5183,36 @@ static ssize_t memory_oom_kill_all_tasks_write(struct kernfs_open_file *of,
 	return nbytes;
 }
 
+static int memory_oom_score_adj_show(struct seq_file *m, void *v)
+{
+	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(m));
+	short oom_score_adj = memcg->oom_score_adj;
+
+	seq_printf(m, "%d\n", oom_score_adj);
+
+	return 0;
+}
+
+static ssize_t memory_oom_score_adj_write(struct kernfs_open_file *of,
+				char *buf, size_t nbytes, loff_t off)
+{
+	struct mem_cgroup *memcg = mem_cgroup_from_css(of_css(of));
+	int oom_score_adj;
+	int err;
+
+	err = kstrtoint(strstrip(buf), 0, &oom_score_adj);
+	if (err)
+		return err;
+
+	if (oom_score_adj < OOM_SCORE_ADJ_MIN ||
+			oom_score_adj > OOM_SCORE_ADJ_MAX)
+		return -EINVAL;
+
+	memcg->oom_score_adj = (short)oom_score_adj;
+
+	return nbytes;
+}
+
 static int memory_events_show(struct seq_file *m, void *v)
 {
 	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(m));
@@ -5298,6 +5328,12 @@ static struct cftype memory_files[] = {
 		.write = memory_oom_kill_all_tasks_write,
 	},
 	{
+		.name = "oom_score_adj",
+		.flags = CFTYPE_NOT_ON_ROOT,
+		.seq_show = memory_oom_score_adj_show,
+		.write = memory_oom_score_adj_write,
+	},
+	{
 		.name = "events",
 		.flags = CFTYPE_NOT_ON_ROOT,
 		.file_offset = offsetof(struct mem_cgroup, events_file),
-- 
2.7.4

Re: [RFC PATCH v2 5/7] mm, oom: introduce oom_score_adj for memory cgroups

[Vladimir Davydov]

On Thu, Jun 01, 2017 at 07:35:13PM +0100, Roman Gushchin wrote:
> Introduce a per-memory-cgroup oom_score_adj setting.
> A read-write single value file which exits on non-root
> cgroups. The default is "0".
> 
> It will have a similar meaning to a per-process value,
> available via /proc/<pid>/oom_score_adj.
> Should be in a range [-1000, 1000].

IMHO OOM scoring (not only the user API, but the logic as well) should
be introduced by a separate patch following the main one (#6) in the
series. Rationale: we might want to commit the main patch right away,
while postponing OOM scoring for later, because some people might find
the API controversial and needing a further, deeper discussion.

[RFC PATCH v2 6/7] mm, oom: cgroup-aware OOM killer

[Roman Gushchin]

Traditionally, the OOM killer is operating on a process level.
Under oom conditions, it finds a process with the highest oom score
and kills it.

This behavior doesn't suit well the system with many running
containers. There are two main issues:

1) There is no fairness between containers. A small container with
few large processes will be chosen over a large one with huge
number of small processes.

2) Containers often do not expect that some random process inside
will be killed. In many cases much more safer behavior is to kill
all tasks in the container. Traditionally, this was implemented
in userspace, but doing it in the kernel has some advantages,
especially in a case of a system-wide OOM.

3) Per-process oom_score_adj affects global OOM, so it's a breache
in the isolation.

To address these issues, cgroup-aware OOM killer is introduced.

Under OOM conditions, it tries to find the biggest memory consumer,
and free memory by killing corresponding task(s). The difference
the "traditional" OOM killer is that it can treat memory cgroups
as memory consumers as well as single processes.

By default, it will look for the biggest leaf cgroup, and kill
the largest task inside.

But a user can change this behavior by enabling the per-cgroup
oom_kill_all_tasks option. If set, it causes the OOM killer treat
the whole cgroup as an indivisible memory consumer. In case if it's
selected as on OOM victim, all belonging tasks will be killed.

Tasks in the root cgroup are treated as independent memory consumers,
and are compared with other memory consumers (e.g. leaf cgroups).
The root cgroup doesn't support the oom_kill_all_tasks feature.

Signed-off-by: Roman Gushchin <guro@fb.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Li Zefan <lizefan@huawei.com>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: kernel-team@fb.com
Cc: cgroups@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-mm@kvack.org
---
 include/linux/memcontrol.h |  13 ++++
 include/linux/oom.h        |   1 +
 mm/memcontrol.c            | 178 +++++++++++++++++++++++++++++++++++++++++++++
 mm/oom_kill.c              |   6 ++
 4 files changed, 198 insertions(+)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 818a42e..67709a4 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -34,6 +34,7 @@ struct mem_cgroup;
 struct page;
 struct mm_struct;
 struct kmem_cache;
+struct oom_control;
 
 /* Cgroup-specific page state, on top of universal node page state */
 enum memcg_stat_item {
@@ -471,6 +472,9 @@ static inline bool task_in_memcg_oom(struct task_struct *p)
 
 bool mem_cgroup_oom_synchronize(bool wait);
 
+bool mem_cgroup_select_oom_victim(struct oom_control *oc);
+bool mem_cgroup_kill_oom_victim(struct oom_control *oc);
+
 #ifdef CONFIG_MEMCG_SWAP
 extern int do_swap_account;
 #endif
@@ -931,6 +935,15 @@ static inline void memcg_kmem_update_page_stat(struct page *page,
 				enum memcg_stat_item idx, int val)
 {
 }
+
+static inline bool mem_cgroup_select_oom_victim(struct oom_control *oc)
+{
+	return false;
+}
+static inline bool mem_cgroup_kill_oom_victim(struct oom_control *oc)
+{
+	return false;
+}
 #endif /* CONFIG_MEMCG && !CONFIG_SLOB */
 
 #endif /* _LINUX_MEMCONTROL_H */
diff --git a/include/linux/oom.h b/include/linux/oom.h
index edf7a77..a6086a2 100644
--- a/include/linux/oom.h
+++ b/include/linux/oom.h
@@ -39,6 +39,7 @@ struct oom_control {
 	unsigned long totalpages;
 	struct task_struct *chosen;
 	unsigned long chosen_points;
+	struct mem_cgroup *chosen_memcg;
 };
 
 extern struct mutex oom_lock;
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index f979ac7..855d335 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2625,6 +2625,184 @@ static inline bool memcg_has_children(struct mem_cgroup *memcg)
 	return ret;
 }
 
+static long mem_cgroup_oom_badness(struct mem_cgroup *memcg,
+				   const nodemask_t *nodemask)
+{
+	long points = 0;
+	int nid;
+	struct mem_cgroup *iter;
+
+	for_each_mem_cgroup_tree(iter, memcg) {
+		for_each_node_state(nid, N_MEMORY) {
+			if (nodemask && !node_isset(nid, *nodemask))
+				continue;
+
+			points += mem_cgroup_node_nr_lru_pages(iter, nid,
+					LRU_ALL_ANON | BIT(LRU_UNEVICTABLE));
+		}
+
+		points += mem_cgroup_get_nr_swap_pages(iter);
+		points += memcg_page_state(iter, MEMCG_KERNEL_STACK_KB) /
+			(PAGE_SIZE / 1024);
+		points += memcg_page_state(iter, MEMCG_SLAB_UNRECLAIMABLE);
+		points += memcg_page_state(iter, MEMCG_SOCK);
+	}
+
+	return points;
+}
+
+bool mem_cgroup_select_oom_victim(struct oom_control *oc)
+{
+	struct cgroup_subsys_state *css = NULL;
+	struct mem_cgroup *iter = NULL;
+	struct mem_cgroup *chosen_memcg = NULL;
+	struct mem_cgroup *parent = root_mem_cgroup;
+	unsigned long totalpages = oc->totalpages;
+	long chosen_memcg_points = 0;
+	long points = 0;
+
+	oc->chosen = NULL;
+	oc->chosen_memcg = NULL;
+
+	if (mem_cgroup_disabled())
+		return false;
+
+	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys))
+		return false;
+
+	pr_info("Choosing a victim memcg because of the %s",
+		oc->memcg ?
+		"memory limit reached of cgroup " :
+		"system-wide OOM\n");
+	if (oc->memcg) {
+		pr_cont_cgroup_path(oc->memcg->css.cgroup);
+		pr_cont("\n");
+
+		chosen_memcg = oc->memcg;
+		parent = oc->memcg;
+	}
+
+	rcu_read_lock();
+
+	for (;;) {
+		css = css_next_child(css, &parent->css);
+		if (css) {
+			iter = mem_cgroup_from_css(css);
+
+			points = mem_cgroup_oom_badness(iter, oc->nodemask);
+			points += iter->oom_score_adj * (totalpages / 1000);
+
+			pr_info("Cgroup ");
+			pr_cont_cgroup_path(iter->css.cgroup);
+			pr_cont(": %ld\n", points);
+
+			if (points > chosen_memcg_points) {
+				chosen_memcg = iter;
+				chosen_memcg_points = points;
+				oc->chosen_points = points;
+			}
+
+			continue;
+		}
+
+		if (chosen_memcg && !chosen_memcg->oom_kill_all_tasks) {
+			/* Go deeper in the cgroup hierarchy */
+			totalpages = chosen_memcg_points;
+			chosen_memcg_points = 0;
+
+			parent = chosen_memcg;
+			chosen_memcg = NULL;
+
+			continue;
+		}
+
+		if (!chosen_memcg && parent != root_mem_cgroup)
+			chosen_memcg = parent;
+
+		break;
+	}
+
+	if (!oc->memcg) {
+		/*
+		 * We should also consider tasks in the root cgroup
+		 * with badness larger than oc->chosen_points
+		 */
+
+		struct css_task_iter it;
+		struct task_struct *task;
+		int ret = 0;
+
+		css_task_iter_start(&root_mem_cgroup->css, &it);
+		while (!ret && (task = css_task_iter_next(&it)))
+			ret = oom_evaluate_task(task, oc);
+		css_task_iter_end(&it);
+	}
+
+	if (!oc->chosen && chosen_memcg) {
+		pr_info("Chosen cgroup ");
+		pr_cont_cgroup_path(chosen_memcg->css.cgroup);
+		pr_cont(": %ld\n", oc->chosen_points);
+
+		if (chosen_memcg->oom_kill_all_tasks) {
+			css_get(&chosen_memcg->css);
+			oc->chosen_memcg = chosen_memcg;
+		} else {
+			/*
+			 * If we don't need to kill all tasks in the cgroup,
+			 * let's select the biggest task.
+			 */
+			oc->chosen_points = 0;
+			select_bad_process(oc, chosen_memcg);
+		}
+	} else if (oc->chosen)
+		pr_info("Chosen task %s (%d) in root cgroup: %ld\n",
+			oc->chosen->comm, oc->chosen->pid, oc->chosen_points);
+
+	rcu_read_unlock();
+
+	oc->chosen_points = 0;
+	return !!oc->chosen || !!oc->chosen_memcg;
+}
+
+static int __oom_kill_task(struct task_struct *tsk, void *arg)
+{
+	if (!is_global_init(tsk) && !(tsk->flags & PF_KTHREAD)) {
+		get_task_struct(tsk);
+		__oom_kill_process(tsk);
+	}
+	return 0;
+}
+
+bool mem_cgroup_kill_oom_victim(struct oom_control *oc)
+{
+	if (oc->chosen_memcg) {
+		/*
+		 * Kill all tasks in the cgroup hierarchy
+		 */
+		mem_cgroup_scan_tasks(oc->chosen_memcg,
+				      __oom_kill_task, NULL);
+
+		/*
+		 * Release oc->chosen_memcg
+		 */
+		css_put(&oc->chosen_memcg->css);
+		oc->chosen_memcg = NULL;
+	}
+
+	if (oc->chosen && oc->chosen != (void *)-1UL) {
+		__oom_kill_process(oc->chosen);
+		return true;
+	}
+
+	/*
+	 * Reset points before falling back to an old
+	 * per-process OOM victim selection logic
+	 */
+	oc->chosen_points = 0;
+
+	return !!oc->chosen;
+}
+
 /*
  * Reclaims as many pages from the given memcg as possible.
  *
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 8cf77fb..1346565 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -1039,6 +1039,12 @@ bool out_of_memory(struct oom_control *oc)
 		return true;
 	}
 
+	if (mem_cgroup_select_oom_victim(oc) &&
+	    mem_cgroup_kill_oom_victim(oc)) {
+		schedule_timeout_killable(1);
+		return true;
+	}
+
 	select_bad_process(oc, oc->memcg);
 	/* Found nothing?!?! Either we hang forever, or we panic. */
 	if (!oc->chosen && !is_sysrq_oom(oc) && !is_memcg_oom(oc)) {
-- 
2.7.4

Re: [RFC PATCH v2 6/7] mm, oom: cgroup-aware OOM killer

[Vladimir Davydov]

On Thu, Jun 01, 2017 at 07:35:14PM +0100, Roman Gushchin wrote:
> Traditionally, the OOM killer is operating on a process level.
> Under oom conditions, it finds a process with the highest oom score
> and kills it.
> 
> This behavior doesn't suit well the system with many running
> containers. There are two main issues:
> 
> 1) There is no fairness between containers. A small container with
> few large processes will be chosen over a large one with huge
> number of small processes.
> 
> 2) Containers often do not expect that some random process inside
> will be killed. In many cases much more safer behavior is to kill
> all tasks in the container. Traditionally, this was implemented
> in userspace, but doing it in the kernel has some advantages,
> especially in a case of a system-wide OOM.
> 
> 3) Per-process oom_score_adj affects global OOM, so it's a breache
> in the isolation.
> 
> To address these issues, cgroup-aware OOM killer is introduced.
> 
> Under OOM conditions, it tries to find the biggest memory consumer,
> and free memory by killing corresponding task(s). The difference
> the "traditional" OOM killer is that it can treat memory cgroups
> as memory consumers as well as single processes.
> 
> By default, it will look for the biggest leaf cgroup, and kill
> the largest task inside.
> 
> But a user can change this behavior by enabling the per-cgroup
> oom_kill_all_tasks option. If set, it causes the OOM killer treat
> the whole cgroup as an indivisible memory consumer. In case if it's
> selected as on OOM victim, all belonging tasks will be killed.
> 
> Tasks in the root cgroup are treated as independent memory consumers,
> and are compared with other memory consumers (e.g. leaf cgroups).
> The root cgroup doesn't support the oom_kill_all_tasks feature.
> 
...
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index f979ac7..855d335 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2625,6 +2625,184 @@ static inline bool memcg_has_children(struct mem_cgroup *memcg)
>  	return ret;
>  }
>  
> +static long mem_cgroup_oom_badness(struct mem_cgroup *memcg,
> +				   const nodemask_t *nodemask)
> +{
> +	long points = 0;
> +	int nid;
> +	struct mem_cgroup *iter;
> +
> +	for_each_mem_cgroup_tree(iter, memcg) {

AFAIU this function is called on every iteration over the cgroup tree,
which might be costly in case of a deep hierarchy, as it has quadratic
complexity at worst. We could eliminate the nested loop by computing
badness of all eligible cgroups before starting looking for a victim and
saving the values in struct mem_cgroup. Not sure if it's worth it, as
OOM is a pretty cold path.

> +		for_each_node_state(nid, N_MEMORY) {
> +			if (nodemask && !node_isset(nid, *nodemask))
> +				continue;
> +
> +			points += mem_cgroup_node_nr_lru_pages(iter, nid,
> +					LRU_ALL_ANON | BIT(LRU_UNEVICTABLE));

Hmm, is there a reason why we shouldn't take into account file pages?

> +		}
> +
> +		points += mem_cgroup_get_nr_swap_pages(iter);

AFAICS mem_cgroup_get_nr_swap_pages() returns the number of pages that
can still be charged to the cgroup. IIUC we want to account pages that
have already been charged to the cgroup, i.e. the value of the 'swap'
page counter or MEMCG_SWAP stat counter.

> +		points += memcg_page_state(iter, MEMCG_KERNEL_STACK_KB) /
> +			(PAGE_SIZE / 1024);
> +		points += memcg_page_state(iter, MEMCG_SLAB_UNRECLAIMABLE);
> +		points += memcg_page_state(iter, MEMCG_SOCK);
> +	}
> +
> +	return points;
> +}
> +
> +bool mem_cgroup_select_oom_victim(struct oom_control *oc)
> +{
> +	struct cgroup_subsys_state *css = NULL;
> +	struct mem_cgroup *iter = NULL;
> +	struct mem_cgroup *chosen_memcg = NULL;
> +	struct mem_cgroup *parent = root_mem_cgroup;
> +	unsigned long totalpages = oc->totalpages;
> +	long chosen_memcg_points = 0;
> +	long points = 0;
> +
> +	oc->chosen = NULL;
> +	oc->chosen_memcg = NULL;
> +
> +	if (mem_cgroup_disabled())
> +		return false;
> +
> +	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys))
> +		return false;
> +
> +	pr_info("Choosing a victim memcg because of the %s",
> +		oc->memcg ?
> +		"memory limit reached of cgroup " :
> +		"system-wide OOM\n");
> +	if (oc->memcg) {
> +		pr_cont_cgroup_path(oc->memcg->css.cgroup);
> +		pr_cont("\n");
> +
> +		chosen_memcg = oc->memcg;
> +		parent = oc->memcg;
> +	}
> +
> +	rcu_read_lock();
> +
> +	for (;;) {
> +		css = css_next_child(css, &parent->css);
> +		if (css) {
> +			iter = mem_cgroup_from_css(css);
> +
> +			points = mem_cgroup_oom_badness(iter, oc->nodemask);
> +			points += iter->oom_score_adj * (totalpages / 1000);
> +
> +			pr_info("Cgroup ");
> +			pr_cont_cgroup_path(iter->css.cgroup);
> +			pr_cont(": %ld\n", points);

Not sure if everyone wants to see these messages in the log.

> +
> +			if (points > chosen_memcg_points) {
> +				chosen_memcg = iter;
> +				chosen_memcg_points = points;
> +				oc->chosen_points = points;
> +			}
> +
> +			continue;
> +		}
> +
> +		if (chosen_memcg && !chosen_memcg->oom_kill_all_tasks) {
> +			/* Go deeper in the cgroup hierarchy */
> +			totalpages = chosen_memcg_points;

We set 'totalpages' to the target cgroup limit (or the total RAM
size) when computing a victim score. Why do you prefer to use
chosen_memcg_points here instead? Why not the limit of the chosen
cgroup?

> +			chosen_memcg_points = 0;
> +
> +			parent = chosen_memcg;
> +			chosen_memcg = NULL;
> +
> +			continue;
> +		}
> +
> +		if (!chosen_memcg && parent != root_mem_cgroup)
> +			chosen_memcg = parent;
> +
> +		break;
> +	}
> +

> +	if (!oc->memcg) {
> +		/*
> +		 * We should also consider tasks in the root cgroup
> +		 * with badness larger than oc->chosen_points
> +		 */
> +
> +		struct css_task_iter it;
> +		struct task_struct *task;
> +		int ret = 0;
> +
> +		css_task_iter_start(&root_mem_cgroup->css, &it);
> +		while (!ret && (task = css_task_iter_next(&it)))
> +			ret = oom_evaluate_task(task, oc);
> +		css_task_iter_end(&it);
> +	}

IMHO it isn't quite correct to compare tasks from the root cgroup with
leaf cgroups, because they are at different levels. Shouldn't we compare
their scores only with the top level cgroups?

As an alternative approach, may be, we could remove this branch
altogether and ignore root tasks here (i.e. have any root task a higher
priority a priori)? Perhaps, it could be acceptable, because normally
the root cgroup only hosts kernel processes and init (at least this is
the default systemd setup IIRC).

> +
> +	if (!oc->chosen && chosen_memcg) {
> +		pr_info("Chosen cgroup ");
> +		pr_cont_cgroup_path(chosen_memcg->css.cgroup);
> +		pr_cont(": %ld\n", oc->chosen_points);
> +
> +		if (chosen_memcg->oom_kill_all_tasks) {
> +			css_get(&chosen_memcg->css);
> +			oc->chosen_memcg = chosen_memcg;
> +		} else {
> +			/*
> +			 * If we don't need to kill all tasks in the cgroup,
> +			 * let's select the biggest task.
> +			 */
> +			oc->chosen_points = 0;

> +			select_bad_process(oc, chosen_memcg);

I think we'd better use mem_cgroup_scan_task() here directly, without
exporting select_bad_process() from oom_kill.c. IMHO it would be more
straightforward, because select_bad_process() has a branch handling the
global OOM, which isn't used in this case. Come to think of it, wouldn't
it be better to return the chosen cgroup in @oc and let out_of_memory()
select a process within it or kill it as a whole depending on the value
of the oom_kill_all_tasks flag?

Also, if the chosen cgroup has no tasks (which is perfectly possible if
all memory within the cgroup is consumed by shmem e.g.), shouldn't we
retry the cgroup selection?

> +		}
> +	} else if (oc->chosen)
> +		pr_info("Chosen task %s (%d) in root cgroup: %ld\n",
> +			oc->chosen->comm, oc->chosen->pid, oc->chosen_points);
> +
> +	rcu_read_unlock();
> +
> +	oc->chosen_points = 0;
> +	return !!oc->chosen || !!oc->chosen_memcg;
> +}
> +
> +static int __oom_kill_task(struct task_struct *tsk, void *arg)
> +{
> +	if (!is_global_init(tsk) && !(tsk->flags & PF_KTHREAD)) {
> +		get_task_struct(tsk);
> +		__oom_kill_process(tsk);
> +	}
> +	return 0;
> +}
> +
> +bool mem_cgroup_kill_oom_victim(struct oom_control *oc)

I think it'd be OK to define this function in oom_kill.c - we
have everything we need for that. We wouldn't have to export
__oom_kill_process without oom_kill_process then, which is kinda
ugly IMHO.

> +{
> +	if (oc->chosen_memcg) {
> +		/*
> +		 * Kill all tasks in the cgroup hierarchy
> +		 */
> +		mem_cgroup_scan_tasks(oc->chosen_memcg,
> +				      __oom_kill_task, NULL);
> +
> +		/*
> +		 * Release oc->chosen_memcg
> +		 */
> +		css_put(&oc->chosen_memcg->css);
> +		oc->chosen_memcg = NULL;
> +	}
> +
> +	if (oc->chosen && oc->chosen != (void *)-1UL) {

> +		__oom_kill_process(oc->chosen);

Why don't you use oom_kill_process (without leading underscores) here?

> +		return true;
> +	}
> +
> +	/*
> +	 * Reset points before falling back to an old
> +	 * per-process OOM victim selection logic
> +	 */
> +	oc->chosen_points = 0;
> +
> +	return !!oc->chosen;
> +}

Re: [RFC PATCH v2 6/7] mm, oom: cgroup-aware OOM killer

[Roman Gushchin]

On Sun, Jun 04, 2017 at 11:43:33PM +0300, Vladimir Davydov wrote:
> On Thu, Jun 01, 2017 at 07:35:14PM +0100, Roman Gushchin wrote:
> ...
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index f979ac7..855d335 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -2625,6 +2625,184 @@ static inline bool memcg_has_children(struct mem_cgroup *memcg)
> >  	return ret;
> >  }
> >  
> > +static long mem_cgroup_oom_badness(struct mem_cgroup *memcg,
> > +				   const nodemask_t *nodemask)
> > +{
> > +	long points = 0;
> > +	int nid;
> > +	struct mem_cgroup *iter;
> > +
> > +	for_each_mem_cgroup_tree(iter, memcg) {
> 
> AFAIU this function is called on every iteration over the cgroup tree,
> which might be costly in case of a deep hierarchy, as it has quadratic
> complexity at worst. We could eliminate the nested loop by computing
> badness of all eligible cgroups before starting looking for a victim and
> saving the values in struct mem_cgroup. Not sure if it's worth it, as
> OOM is a pretty cold path.

I've thought about it, but it really not obvious that we want to pay
with some additional memory usage (and code complexity) for optimization
of this path. So, I decided to leave it simple now, and postpone
all optimizations after we'll agree on everything else.

> 
> > +		for_each_node_state(nid, N_MEMORY) {
> > +			if (nodemask && !node_isset(nid, *nodemask))
> > +				continue;
> > +
> > +			points += mem_cgroup_node_nr_lru_pages(iter, nid,
> > +					LRU_ALL_ANON | BIT(LRU_UNEVICTABLE));
> 
> Hmm, is there a reason why we shouldn't take into account file pages?

Because under the OOM conditions we should not have too much pagecache,
and killing a process will unlikely help us to release any additional memory.
But maybe I'm missing something... Lazy free?

> 
> > +		}
> > +
> > +		points += mem_cgroup_get_nr_swap_pages(iter);
> 
> AFAICS mem_cgroup_get_nr_swap_pages() returns the number of pages that
> can still be charged to the cgroup. IIUC we want to account pages that
> have already been charged to the cgroup, i.e. the value of the 'swap'
> page counter or MEMCG_SWAP stat counter.

Ok, I'll check it. Thank you!

> 
> > +		points += memcg_page_state(iter, MEMCG_KERNEL_STACK_KB) /
> > +			(PAGE_SIZE / 1024);
> > +		points += memcg_page_state(iter, MEMCG_SLAB_UNRECLAIMABLE);
> > +		points += memcg_page_state(iter, MEMCG_SOCK);
> > +	}
> > +
> > +	return points;
> > +}
> > +
> > +bool mem_cgroup_select_oom_victim(struct oom_control *oc)
> > +{
> > +	struct cgroup_subsys_state *css = NULL;
> > +	struct mem_cgroup *iter = NULL;
> > +	struct mem_cgroup *chosen_memcg = NULL;
> > +	struct mem_cgroup *parent = root_mem_cgroup;
> > +	unsigned long totalpages = oc->totalpages;
> > +	long chosen_memcg_points = 0;
> > +	long points = 0;
> > +
> > +	oc->chosen = NULL;
> > +	oc->chosen_memcg = NULL;
> > +
> > +	if (mem_cgroup_disabled())
> > +		return false;
> > +
> > +	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys))
> > +		return false;
> > +
> > +	pr_info("Choosing a victim memcg because of the %s",
> > +		oc->memcg ?
> > +		"memory limit reached of cgroup " :
> > +		"system-wide OOM\n");
> > +	if (oc->memcg) {
> > +		pr_cont_cgroup_path(oc->memcg->css.cgroup);
> > +		pr_cont("\n");
> > +
> > +		chosen_memcg = oc->memcg;
> > +		parent = oc->memcg;
> > +	}
> > +
> > +	rcu_read_lock();
> > +
> > +	for (;;) {
> > +		css = css_next_child(css, &parent->css);
> > +		if (css) {
> > +			iter = mem_cgroup_from_css(css);
> > +
> > +			points = mem_cgroup_oom_badness(iter, oc->nodemask);
> > +			points += iter->oom_score_adj * (totalpages / 1000);
> > +
> > +			pr_info("Cgroup ");
> > +			pr_cont_cgroup_path(iter->css.cgroup);
> > +			pr_cont(": %ld\n", points);
> 
> Not sure if everyone wants to see these messages in the log.

What do you suggest? Remove debug output at all (probably, we still want some),
ratelimit it, make optional?

> 
> > +
> > +			if (points > chosen_memcg_points) {
> > +				chosen_memcg = iter;
> > +				chosen_memcg_points = points;
> > +				oc->chosen_points = points;
> > +			}
> > +
> > +			continue;
> > +		}
> > +
> > +		if (chosen_memcg && !chosen_memcg->oom_kill_all_tasks) {
> > +			/* Go deeper in the cgroup hierarchy */
> > +			totalpages = chosen_memcg_points;
> 
> We set 'totalpages' to the target cgroup limit (or the total RAM
> size) when computing a victim score. Why do you prefer to use
> chosen_memcg_points here instead? Why not the limit of the chosen
> cgroup?

Because I'm trying to implement hierarchical oom_score_adj, so that if
a parent cgroup has oom_score_adj set to -1000, it's successors will
(almost) never selected.

> > +			chosen_memcg_points = 0;
> > +
> > +			parent = chosen_memcg;
> > +			chosen_memcg = NULL;
> > +
> > +			continue;
> > +		}
> > +
> > +		if (!chosen_memcg && parent != root_mem_cgroup)
> > +			chosen_memcg = parent;
> > +
> > +		break;
> > +	}
> > +
> 
> > +	if (!oc->memcg) {
> > +		/*
> > +		 * We should also consider tasks in the root cgroup
> > +		 * with badness larger than oc->chosen_points
> > +		 */
> > +
> > +		struct css_task_iter it;
> > +		struct task_struct *task;
> > +		int ret = 0;
> > +
> > +		css_task_iter_start(&root_mem_cgroup->css, &it);
> > +		while (!ret && (task = css_task_iter_next(&it)))
> > +			ret = oom_evaluate_task(task, oc);
> > +		css_task_iter_end(&it);
> > +	}
> 
> IMHO it isn't quite correct to compare tasks from the root cgroup with
> leaf cgroups, because they are at different levels. Shouldn't we compare
> their scores only with the top level cgroups?

Not sure I follow your idea...
Of course, comparing tasks and cgroups is not really precise,
but hopefully should be good enough for the task.
 
> As an alternative approach, may be, we could remove this branch
> altogether and ignore root tasks here (i.e. have any root task a higher
> priority a priori)? Perhaps, it could be acceptable, because normally
> the root cgroup only hosts kernel processes and init (at least this is
> the default systemd setup IIRC).
> 
> > +
> > +	if (!oc->chosen && chosen_memcg) {
> > +		pr_info("Chosen cgroup ");
> > +		pr_cont_cgroup_path(chosen_memcg->css.cgroup);
> > +		pr_cont(": %ld\n", oc->chosen_points);
> > +
> > +		if (chosen_memcg->oom_kill_all_tasks) {
> > +			css_get(&chosen_memcg->css);
> > +			oc->chosen_memcg = chosen_memcg;
> > +		} else {
> > +			/*
> > +			 * If we don't need to kill all tasks in the cgroup,
> > +			 * let's select the biggest task.
> > +			 */
> > +			oc->chosen_points = 0;
> 
> > +			select_bad_process(oc, chosen_memcg);
> 
> I think we'd better use mem_cgroup_scan_task() here directly, without
> exporting select_bad_process() from oom_kill.c. IMHO it would be more
> straightforward, because select_bad_process() has a branch handling the
> global OOM, which isn't used in this case. Come to think of it, wouldn't
> it be better to return the chosen cgroup in @oc and let out_of_memory()
> select a process within it or kill it as a whole depending on the value
> of the oom_kill_all_tasks flag?
> 
> Also, if the chosen cgroup has no tasks (which is perfectly possible if
> all memory within the cgroup is consumed by shmem e.g.), shouldn't we
> retry the cgroup selection?

Good point. Whould we retry the cgroup selection or just ignore
non-populated cgroups during selection?

> 
> > +		}
> > +	} else if (oc->chosen)
> > +		pr_info("Chosen task %s (%d) in root cgroup: %ld\n",
> > +			oc->chosen->comm, oc->chosen->pid, oc->chosen_points);
> > +
> > +	rcu_read_unlock();
> > +
> > +	oc->chosen_points = 0;
> > +	return !!oc->chosen || !!oc->chosen_memcg;
> > +}
> > +
> > +static int __oom_kill_task(struct task_struct *tsk, void *arg)
> > +{
> > +	if (!is_global_init(tsk) && !(tsk->flags & PF_KTHREAD)) {
> > +		get_task_struct(tsk);
> > +		__oom_kill_process(tsk);
> > +	}
> > +	return 0;
> > +}
> > +
> > +bool mem_cgroup_kill_oom_victim(struct oom_control *oc)
> 
> I think it'd be OK to define this function in oom_kill.c - we
> have everything we need for that. We wouldn't have to export
> __oom_kill_process without oom_kill_process then, which is kinda
> ugly IMHO.
> 
> > +{
> > +	if (oc->chosen_memcg) {
> > +		/*
> > +		 * Kill all tasks in the cgroup hierarchy
> > +		 */
> > +		mem_cgroup_scan_tasks(oc->chosen_memcg,
> > +				      __oom_kill_task, NULL);
> > +
> > +		/*
> > +		 * Release oc->chosen_memcg
> > +		 */
> > +		css_put(&oc->chosen_memcg->css);
> > +		oc->chosen_memcg = NULL;
> > +	}
> > +
> > +	if (oc->chosen && oc->chosen != (void *)-1UL) {
> 
> > +		__oom_kill_process(oc->chosen);
> 
> Why don't you use oom_kill_process (without leading underscores) here?

Because oom_kill_process() has some unwanted side-effects:
1) it can kill other than specified process, we don't need this optimization here,
2) bulky debug output.

Thank you for review!

Roman

[RFC PATCH v2 7/7] mm,oom,docs: describe the cgroup-aware OOM killer

[Roman Gushchin]

Update cgroups v2 docs.

Signed-off-by: Roman Gushchin <guro@fb.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Li Zefan <lizefan@huawei.com>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: kernel-team@fb.com
Cc: cgroups@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-mm@kvack.org
---
 Documentation/cgroup-v2.txt | 47 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 45 insertions(+), 2 deletions(-)

diff --git a/Documentation/cgroup-v2.txt b/Documentation/cgroup-v2.txt
index dc5e2dc..bb1e45c 100644
--- a/Documentation/cgroup-v2.txt
+++ b/Documentation/cgroup-v2.txt
@@ -44,6 +44,7 @@ CONTENTS
     5-2-1. Memory Interface Files
     5-2-2. Usage Guidelines
     5-2-3. Memory Ownership
+    5-2-4. Cgroup-aware OOM Killer
   5-3. IO
     5-3-1. IO Interface Files
     5-3-2. Writeback
@@ -799,6 +800,26 @@ 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
+	cgroups.  The default is "0".
+
+	Defines whether the OOM killer should treat the cgroup
+	as a single entity during the victim selection.
+
+	If set, it will cause the OOM killer to kill all belonging
+	tasks, both in case of a system-wide or cgroup-wide OOM.
+
+  memory.oom_score_adj
+
+	A read-write single value file which exits on non-root
+	cgroups.  The default is "0".
+
+	OOM killer score adjustment, which has as similar meaning
+	to a per-process value, available via /proc/<pid>/oom_score_adj.
+	Should be in a range [-1000, 1000].
+
   memory.events
 
 	A read-only flat-keyed file which exists on non-root cgroups.
@@ -831,8 +852,7 @@ PAGE_SIZE multiple when read back.
 	  oom
 
 		The number of times the OOM killer has been invoked in
-		the cgroup.  This may not exactly match the number of
-		processes killed but should generally be close.
+		the cgroup.
 
   memory.stat
 
@@ -988,6 +1008,29 @@ POSIX_FADV_DONTNEED to relinquish the ownership of memory areas
 belonging to the affected files to ensure correct memory ownership.
 
 
+5-2-4. 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.
+
+But a user can change this behavior by enabling the per-cgroup
+oom_kill_all_tasks option. If set, it causes the OOM killer treat
+the whole cgroup as an indivisible memory consumer. In case if it's
+selected as on OOM victim, all belonging tasks will be killed.
+
+Tasks in the root cgroup are treated as independent memory consumers,
+and are compared with other memory consumers (e.g. leaf cgroups).
+The root cgroup doesn't support the oom_kill_all_tasks feature.
+
+This affects both system- and cgroup-wide OOMs. For a cgroup-wide OOM
+the memory controller considers only cgroups belonging to the sub-tree
+of the OOM'ing cgroup.
+
 5-3. IO
 
 The "io" controller regulates the distribution of IO resources.  This
-- 
2.7.4