cgroup-aware OOM killer

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:

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 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.

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

This patch introduces the core functionality: an ability to select a memory cgroup as an OOM victim. Under OOM conditions the OOM killer looks for the biggest leaf memory cgroup and kills the biggest task belonging to it.

The following patches will extend this functionality to consider non-leaf memory cgroups as OOM victims, and also provide an ability to kill all tasks belonging to the victim cgroup.

The root cgroup is treated as a leaf memory cgroup, so it's score is compared with other leaf memory cgroups.
Due to memcg statistics implementation a special approximation is used for estimating oom_score of root memory cgroup: we sum oom_score of the belonging processes (or, to be more precise, tasks owning their mm structures).

Signed-off-by Roman Gushchin <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Vladimir Davydov <[email protected]>

Cc Tetsuo Handa <[email protected]>

Cc David Rientjes <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Tejun Heo <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

---
 include/linux/memcontrol.h |  17 +++++
 include/linux/oom.h        |  12 ++-
 mm/memcontrol.c            | 181 +++++++++++++++++++++++++++++++++++++++++++++
 mm/oom_kill.c              |  84 +++++++++++++++------
 4 files changed, 272 insertions(+), 22 deletions(-)

include/linux/memcontrol.h +17 -0

index 882046863581..cb4db659a8b5 100644
@@ -35,6 +35,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 {

@@ -344,6 +345,11 @@ struct mem_cgroup *mem_cgroup_from_css(struct cgroup_subsys_state *css){
 	return css ? container_of(css, struct mem_cgroup, css) : NULL;
 }
 
+static inline void mem_cgroup_put(struct mem_cgroup *memcg)
+{
+	css_put(&memcg->css);
+}
+
 #define mem_cgroup_from_counter(counter, member)	\
 	container_of(counter, struct mem_cgroup, member)
 

@@ -482,6 +488,8 @@ 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);
+
 #ifdef CONFIG_MEMCG_SWAP
 extern int do_swap_account;
 #endif

@@ -781,6 +789,10 @@ static inline bool task_in_mem_cgroup(struct task_struct *task,
 	return true;
 }
 
+static inline void mem_cgroup_put(struct mem_cgroup *memcg)
+{
+}
+
 static inline struct mem_cgroup *
 mem_cgroup_iter(struct mem_cgroup *root,
 		struct mem_cgroup *prev,

@@ -973,6 +985,11 @@ static inline
 void count_memcg_event_mm(struct mm_struct *mm, enum vm_event_item idx)
 {
 }
+
+static inline bool mem_cgroup_select_oom_victim(struct oom_control *oc)
+{
+	return false;
+}
 #endif /* CONFIG_MEMCG */
 
 /* idx can be of type enum memcg_stat_item or node_stat_item */

include/linux/oom.h +11 -1

index 27cd36b762b5..10f495c8454d 100644
@@ -10,6 +10,13 @@
 #include <linux/sched/coredump.h> /* MMF_* */
 #include <linux/mm.h> /* VM_FAULT* */
 
+
+/*
+ * Special value returned by victim selection functions to indicate
+ * that are inflight OOM victims.
+ */
+#define INFLIGHT_VICTIM ((void *)-1UL)
+
 struct zonelist;
 struct notifier_block;
 struct mem_cgroup;

@@ -51,7 +58,8 @@ struct oom_control {
 
 	/* Used by oom implementation, do not set */
 	unsigned long totalpages;
-	struct task_struct *chosen;
+	struct task_struct *chosen_task;
+	struct mem_cgroup *chosen_memcg;
 	unsigned long chosen_points;
 };
 

@@ -115,6 +123,8 @@ extern struct task_struct *find_lock_task_mm(struct task_struct *p);
 
 extern struct page *alloc_pages_before_oomkill(const struct oom_control *oc);
 
+extern int oom_evaluate_task(struct task_struct *task, void *arg);
+
 /* sysctls */
 extern int sysctl_oom_dump_tasks;
 extern int sysctl_oom_kill_allocating_task;

mm/memcontrol.c +181 -0

index 55fbda60cef6..592ffb1c98a7 100644
@@ -2664,6 +2664,187 @@ static inline bool memcg_has_children(struct mem_cgroup *memcg)
 	return ret;
 }
 
+static long memcg_oom_badness(struct mem_cgroup *memcg,
+			      const nodemask_t *nodemask,
+			      unsigned long totalpages)
+{
+	long points = 0;
+	int nid;
+	pg_data_t *pgdat;
+
+	for_each_node_state(nid, N_MEMORY) {
+		if (nodemask && !node_isset(nid, *nodemask))
+			continue;
+
+		points += mem_cgroup_node_nr_lru_pages(memcg, nid,
+				LRU_ALL_ANON | BIT(LRU_UNEVICTABLE));
+
+		pgdat = NODE_DATA(nid);
+		points += lruvec_page_state(mem_cgroup_lruvec(pgdat, memcg),
+					    NR_SLAB_UNRECLAIMABLE);
+	}
+
+	points += memcg_page_state(memcg, MEMCG_KERNEL_STACK_KB) /
+		(PAGE_SIZE / 1024);
+	points += memcg_page_state(memcg, MEMCG_SOCK);
+	points += memcg_page_state(memcg, MEMCG_SWAP);
+
+	return points;
+}
+
+/*
+ * Checks if the given memcg is a valid OOM victim and returns a number,
+ * which means the folowing:
+ *   -1: there are inflight OOM victim tasks, belonging to the memcg
+ *    0: memcg is not eligible, e.g. all belonging tasks are protected
+ *       by oom_score_adj set to OOM_SCORE_ADJ_MIN
+ *   >0: memcg is eligible, and the returned value is an estimation
+ *       of the memory footprint
+ */
+static long oom_evaluate_memcg(struct mem_cgroup *memcg,
+			       const nodemask_t *nodemask,
+			       unsigned long totalpages)
+{
+	struct css_task_iter it;
+	struct task_struct *task;
+	int eligible = 0;
+
+	/*
+	 * Root memory cgroup is a special case:
+	 * we don't have necessary stats to evaluate it exactly as
+	 * leaf memory cgroups, so we approximate it's oom_score
+	 * by summing oom_score of all belonging tasks, which are
+	 * owners of their mm structs.
+	 *
+	 * If there are inflight OOM victim tasks inside
+	 * the root memcg, we return -1.
+	 */
+	if (memcg == root_mem_cgroup) {
+		struct css_task_iter it;
+		struct task_struct *task;
+		long score = 0;
+
+		css_task_iter_start(&memcg->css, 0, &it);
+		while ((task = css_task_iter_next(&it))) {
+			if (tsk_is_oom_victim(task) &&
+			    !test_bit(MMF_OOM_SKIP,
+				      &task->signal->oom_mm->flags)) {
+				score = -1;
+				break;
+			}
+
+			task_lock(task);
+			if (!task->mm || task->mm->owner != task) {
+				task_unlock(task);
+				continue;
+			}
+			task_unlock(task);
+
+			score += oom_badness(task, memcg, nodemask,
+					     totalpages);
+		}
+		css_task_iter_end(&it);
+
+		return score;
+	}
+
+	/*
+	 * Memcg is OOM eligible if there are OOM killable tasks inside.
+	 *
+	 * We treat tasks with oom_score_adj set to OOM_SCORE_ADJ_MIN
+	 * as unkillable.
+	 *
+	 * If there are inflight OOM victim tasks inside the memcg,
+	 * we return -1.
+	 */
+	css_task_iter_start(&memcg->css, 0, &it);
+	while ((task = css_task_iter_next(&it))) {
+		if (!eligible &&
+		    task->signal->oom_score_adj != OOM_SCORE_ADJ_MIN)
+			eligible = 1;
+
+		if (tsk_is_oom_victim(task) &&
+		    !test_bit(MMF_OOM_SKIP, &task->signal->oom_mm->flags)) {
+			eligible = -1;
+			break;
+		}
+	}
+	css_task_iter_end(&it);
+
+	if (eligible <= 0)
+		return eligible;
+
+	return memcg_oom_badness(memcg, nodemask, totalpages);
+}
+
+static void select_victim_memcg(struct mem_cgroup *root, struct oom_control *oc)
+{
+	struct mem_cgroup *iter;
+
+	oc->chosen_memcg = NULL;
+	oc->chosen_points = 0;
+
+	/*
+	 * The oom_score is calculated for leaf memory cgroups (including
+	 * the root memcg).
+	 */
+	rcu_read_lock();
+	for_each_mem_cgroup_tree(iter, root) {
+		long score;
+
+		if (memcg_has_children(iter) && iter != root_mem_cgroup)
+			continue;
+
+		score = oom_evaluate_memcg(iter, oc->nodemask, oc->totalpages);
+
+		/*
+		 * Ignore empty and non-eligible memory cgroups.
+		 */
+		if (score == 0)
+			continue;
+
+		/*
+		 * If there are inflight OOM victims, we don't need
+		 * to look further for new victims.
+		 */
+		if (score == -1) {
+			oc->chosen_memcg = INFLIGHT_VICTIM;
+			mem_cgroup_iter_break(root, iter);
+			break;
+		}
+
+		if (score > oc->chosen_points) {
+			oc->chosen_points = score;
+			oc->chosen_memcg = iter;
+		}
+	}
+
+	if (oc->chosen_memcg && oc->chosen_memcg != INFLIGHT_VICTIM)
+		css_get(&oc->chosen_memcg->css);
+
+	rcu_read_unlock();
+}
+
+bool mem_cgroup_select_oom_victim(struct oom_control *oc)
+{
+	struct mem_cgroup *root;
+
+	if (mem_cgroup_disabled())
+		return false;
+
+	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys))
+		return false;
+
+	if (oc->memcg)
+		root = oc->memcg;
+	else
+		root = root_mem_cgroup;
+
+	select_victim_memcg(root, oc);
+
+	return oc->chosen_memcg;
+}
+
 /*
  * Reclaims as many pages from the given memcg as possible.
  *

mm/oom_kill.c +63 -21

index f041534d77d3..bcfa92f29407 100644
@@ -309,7 +309,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,26 +343,26 @@ static int oom_evaluate_task(struct task_struct *task, void *arg)
 		goto next;
 
 	/* Prefer thread group leaders for display purposes */
-	if (points == oc->chosen_points && thread_group_leader(oc->chosen))
+	if (points == oc->chosen_points && thread_group_leader(oc->chosen_task))
 		goto next;
 select:
-	if (oc->chosen)
-		put_task_struct(oc->chosen);
+	if (oc->chosen_task)
+		put_task_struct(oc->chosen_task);
 	get_task_struct(task);
-	oc->chosen = task;
+	oc->chosen_task = task;
 	oc->chosen_points = points;
 next:
 	return 0;
 abort:
-	if (oc->chosen)
-		put_task_struct(oc->chosen);
-	oc->chosen = (void *)-1UL;
+	if (oc->chosen_task)
+		put_task_struct(oc->chosen_task);
+	oc->chosen_task = INFLIGHT_VICTIM;
 	return 1;
 }
 
 /*
  * Simple selection loop. We choose the process with the highest number of
- * 'points'. In case scan was aborted, oc->chosen is set to -1.
+ * 'points'. In case scan was aborted, oc->chosen_task is set to -1.
  */
 static void select_bad_process(struct oom_control *oc)
 {

@@ -895,7 +895,7 @@ static void __oom_kill_process(struct task_struct *victim)
 
 static void oom_kill_process(struct oom_control *oc, const char *message)
 {
-	struct task_struct *p = oc->chosen;
+	struct task_struct *p = oc->chosen_task;
 	unsigned int points = oc->chosen_points;
 	struct task_struct *victim = p;
 	struct task_struct *child;

@@ -956,6 +956,27 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
 	__oom_kill_process(victim);
 }
 
+static bool oom_kill_memcg_victim(struct oom_control *oc)
+{
+
+	if (oc->chosen_memcg == NULL || oc->chosen_memcg == INFLIGHT_VICTIM)
+		return oc->chosen_memcg;
+
+	/* Kill a task in the chosen memcg with the biggest memory footprint */
+	oc->chosen_points = 0;
+	oc->chosen_task = NULL;
+	mem_cgroup_scan_tasks(oc->chosen_memcg, oom_evaluate_task, oc);
+
+	if (oc->chosen_task == NULL || oc->chosen_task == INFLIGHT_VICTIM)
+		goto out;
+
+	__oom_kill_process(oc->chosen_task);
+
+out:
+	mem_cgroup_put(oc->chosen_memcg);
+	return oc->chosen_task;
+}
+
 /*
  * Determines whether the kernel must panic because of the panic_on_oom sysctl.
  */

@@ -1008,6 +1029,7 @@ bool out_of_memory(struct oom_control *oc)
 {
 	unsigned long freed = 0;
 	enum oom_constraint constraint = CONSTRAINT_NONE;
+	bool delay = false; /* if set, delay next allocation attempt */
 
 	if (oom_killer_disabled)
 		return false;

@@ -1055,11 +1077,26 @@ bool out_of_memory(struct oom_control *oc)
 		if (oc->page)
 			return true;
 		get_task_struct(current);
-		oc->chosen = current;
+		oc->chosen_task = current;
 		oom_kill_process(oc, "Out of memory (oom_kill_allocating_task)");
 		return true;
 	}
 
+	if (mem_cgroup_select_oom_victim(oc)) {
+		oc->page = alloc_pages_before_oomkill(oc);
+		if (oc->page) {
+			if (oc->chosen_memcg &&
+			    oc->chosen_memcg != INFLIGHT_VICTIM)
+				mem_cgroup_put(oc->chosen_memcg);
+			return true;
+		}
+
+		if (oom_kill_memcg_victim(oc)) {
+			delay = true;
+			goto out;
+		}
+	}
+
 	select_bad_process(oc);
 	/*
 	 * Try really last second allocation attempt after we selected an OOM

@@ -1068,25 +1105,30 @@ bool out_of_memory(struct oom_control *oc)
 	 */
 	oc->page = alloc_pages_before_oomkill(oc);
 	if (oc->page) {
-		if (oc->chosen && oc->chosen != (void *)-1UL)
-			put_task_struct(oc->chosen);
+		if (oc->chosen_task && oc->chosen_task != INFLIGHT_VICTIM)
+			put_task_struct(oc->chosen_task);
 		return true;
 	}
 	/* Found nothing?!?! Either we hang forever, or we panic. */
-	if (!oc->chosen && !is_sysrq_oom(oc) && !is_memcg_oom(oc)) {
+	if (!oc->chosen_task && !is_sysrq_oom(oc) && !is_memcg_oom(oc)) {
 		dump_header(oc, NULL);
 		panic("Out of memory and no killable processes...\n");
 	}
-	if (oc->chosen && oc->chosen != (void *)-1UL) {
+	if (oc->chosen_task && oc->chosen_task != INFLIGHT_VICTIM) {
 		oom_kill_process(oc, !is_memcg_oom(oc) ? "Out of memory" :
 				 "Memory cgroup out of memory");
-		/*
-		 * Give the killed process a good chance to exit before trying
-		 * to allocate memory again.
-		 */
-		schedule_timeout_killable(1);
+		delay = true;
 	}
-	return !!oc->chosen;
+
+out:
+	/*
+	 * Give the killed process a good chance to exit before trying
+	 * to allocate memory again.
+	 */
+	if (delay)
+		schedule_timeout_killable(1);
+
+	return !!oc->chosen_task;
 }
 
 /*

Signature · 1 line

2.14.3

As a result of the "stalled MM patches" discussion I've dropped these three patches:

mm,oom: move last second allocation to inside the OOM killer mm,oom: use ALLOC_OOM for OOM victim's last second allocation mm,oom: remove oom_lock serialization from the OOM reaper

and I had to rework this patch as a result. Please carefully check (and preferable test) my handiwork in out_of_memory()?

From: Roman Gushchin <[email protected]>
Subject: mm, oom: cgroup-aware OOM killer

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:

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 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.

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

This patch introduces the core functionality: an ability to select a memory cgroup as an OOM victim. Under OOM conditions the OOM killer looks for the biggest leaf memory cgroup and kills the biggest task belonging to it.

The following patches will extend this functionality to consider non-leaf memory cgroups as OOM victims, and also provide an ability to kill all tasks belonging to the victim cgroup.

The root cgroup is treated as a leaf memory cgroup, so it's score is compared with other leaf memory cgroups. Due to memcg statistics implementation a special approximation is used for estimating oom_score of root memory cgroup: we sum oom_score of the belonging processes (or, to be more precise, tasks owning their mm structures).

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Roman Gushchin <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Vladimir Davydov <[email protected]>

Cc Tetsuo Handa <[email protected]>

Cc David Rientjes <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Michal Hocko <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---

 include/linux/memcontrol.h |   17 +++
 include/linux/oom.h        |   12 ++
 mm/memcontrol.c            |  181 +++++++++++++++++++++++++++++++++++
 mm/oom_kill.c              |   72 ++++++++++---
 4 files changed, 262 insertions(+), 20 deletions(-)

diff -puN include/linux/memcontrol.h~mm-oom-cgroup-aware-oom-killer include/linux/memcontrol.h

include/linux/memcontrol.h renamed from include/linux/memcontrol.h~mm-oom-cgroup-aware-oom-killer +17 -0

@@ -35,6 +35,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 {

@@ -344,6 +345,11 @@ struct mem_cgroup *mem_cgroup_from_css(s
 	return css ? container_of(css, struct mem_cgroup, css) : NULL;
 }
 
+static inline void mem_cgroup_put(struct mem_cgroup *memcg)
+{
+	css_put(&memcg->css);
+}
+
 #define mem_cgroup_from_counter(counter, member)	\
 	container_of(counter, struct mem_cgroup, member)
 

@@ -482,6 +488,8 @@ static inline bool task_in_memcg_oom(str
 
 bool mem_cgroup_oom_synchronize(bool wait);
 
+bool mem_cgroup_select_oom_victim(struct oom_control *oc);
+
 #ifdef CONFIG_MEMCG_SWAP
 extern int do_swap_account;
 #endif

@@ -781,6 +789,10 @@ static inline bool task_in_mem_cgroup(st
 	return true;
 }
 
+static inline void mem_cgroup_put(struct mem_cgroup *memcg)
+{
+}
+
 static inline struct mem_cgroup *
 mem_cgroup_iter(struct mem_cgroup *root,
 		struct mem_cgroup *prev,

@@ -973,6 +985,11 @@ static inline
 void count_memcg_event_mm(struct mm_struct *mm, enum vm_event_item idx)
 {
 }
+
+static inline bool mem_cgroup_select_oom_victim(struct oom_control *oc)
+{
+	return false;
+}
 #endif /* CONFIG_MEMCG */
 
 /* idx can be of type enum memcg_stat_item or node_stat_item */

diff -puN include/linux/oom.h~mm-oom-cgroup-aware-oom-killer include/linux/oom.h

include/linux/oom.h renamed from include/linux/oom.h~mm-oom-cgroup-aware-oom-killer +11 -1

@@ -10,6 +10,13 @@
 #include <linux/sched/coredump.h> /* MMF_* */
 #include <linux/mm.h> /* VM_FAULT* */
 
+
+/*
+ * Special value returned by victim selection functions to indicate
+ * that are inflight OOM victims.
+ */
+#define INFLIGHT_VICTIM ((void *)-1UL)
+
 struct zonelist;
 struct notifier_block;
 struct mem_cgroup;

@@ -40,7 +47,8 @@ struct oom_control {
 
 	/* Used by oom implementation, do not set */
 	unsigned long totalpages;
-	struct task_struct *chosen;
+	struct task_struct *chosen_task;
+	struct mem_cgroup *chosen_memcg;
 	unsigned long chosen_points;
 };
 

@@ -102,6 +110,8 @@ extern void oom_killer_enable(void);
 
 extern struct task_struct *find_lock_task_mm(struct task_struct *p);
 
+extern int oom_evaluate_task(struct task_struct *task, void *arg);
+
 /* sysctls */
 extern int sysctl_oom_dump_tasks;
 extern int sysctl_oom_kill_allocating_task;

diff -puN mm/memcontrol.c~mm-oom-cgroup-aware-oom-killer mm/memcontrol.c

mm/memcontrol.c renamed from mm/memcontrol.c~mm-oom-cgroup-aware-oom-killer +181 -0

@@ -2664,6 +2664,187 @@ static inline bool memcg_has_children(st
 	return ret;
 }
 
+static long memcg_oom_badness(struct mem_cgroup *memcg,
+			      const nodemask_t *nodemask,
+			      unsigned long totalpages)
+{
+	long points = 0;
+	int nid;
+	pg_data_t *pgdat;
+
+	for_each_node_state(nid, N_MEMORY) {
+		if (nodemask && !node_isset(nid, *nodemask))
+			continue;
+
+		points += mem_cgroup_node_nr_lru_pages(memcg, nid,
+				LRU_ALL_ANON | BIT(LRU_UNEVICTABLE));
+
+		pgdat = NODE_DATA(nid);
+		points += lruvec_page_state(mem_cgroup_lruvec(pgdat, memcg),
+					    NR_SLAB_UNRECLAIMABLE);
+	}
+
+	points += memcg_page_state(memcg, MEMCG_KERNEL_STACK_KB) /
+		(PAGE_SIZE / 1024);
+	points += memcg_page_state(memcg, MEMCG_SOCK);
+	points += memcg_page_state(memcg, MEMCG_SWAP);
+
+	return points;
+}
+
+/*
+ * Checks if the given memcg is a valid OOM victim and returns a number,
+ * which means the folowing:
+ *   -1: there are inflight OOM victim tasks, belonging to the memcg
+ *    0: memcg is not eligible, e.g. all belonging tasks are protected
+ *       by oom_score_adj set to OOM_SCORE_ADJ_MIN
+ *   >0: memcg is eligible, and the returned value is an estimation
+ *       of the memory footprint
+ */
+static long oom_evaluate_memcg(struct mem_cgroup *memcg,
+			       const nodemask_t *nodemask,
+			       unsigned long totalpages)
+{
+	struct css_task_iter it;
+	struct task_struct *task;
+	int eligible = 0;
+
+	/*
+	 * Root memory cgroup is a special case:
+	 * we don't have necessary stats to evaluate it exactly as
+	 * leaf memory cgroups, so we approximate it's oom_score
+	 * by summing oom_score of all belonging tasks, which are
+	 * owners of their mm structs.
+	 *
+	 * If there are inflight OOM victim tasks inside
+	 * the root memcg, we return -1.
+	 */
+	if (memcg == root_mem_cgroup) {
+		struct css_task_iter it;
+		struct task_struct *task;
+		long score = 0;
+
+		css_task_iter_start(&memcg->css, 0, &it);
+		while ((task = css_task_iter_next(&it))) {
+			if (tsk_is_oom_victim(task) &&
+			    !test_bit(MMF_OOM_SKIP,
+				      &task->signal->oom_mm->flags)) {
+				score = -1;
+				break;
+			}
+
+			task_lock(task);
+			if (!task->mm || task->mm->owner != task) {
+				task_unlock(task);
+				continue;
+			}
+			task_unlock(task);
+
+			score += oom_badness(task, memcg, nodemask,
+					     totalpages);
+		}
+		css_task_iter_end(&it);
+
+		return score;
+	}
+
+	/*
+	 * Memcg is OOM eligible if there are OOM killable tasks inside.
+	 *
+	 * We treat tasks with oom_score_adj set to OOM_SCORE_ADJ_MIN
+	 * as unkillable.
+	 *
+	 * If there are inflight OOM victim tasks inside the memcg,
+	 * we return -1.
+	 */
+	css_task_iter_start(&memcg->css, 0, &it);
+	while ((task = css_task_iter_next(&it))) {
+		if (!eligible &&
+		    task->signal->oom_score_adj != OOM_SCORE_ADJ_MIN)
+			eligible = 1;
+
+		if (tsk_is_oom_victim(task) &&
+		    !test_bit(MMF_OOM_SKIP, &task->signal->oom_mm->flags)) {
+			eligible = -1;
+			break;
+		}
+	}
+	css_task_iter_end(&it);
+
+	if (eligible <= 0)
+		return eligible;
+
+	return memcg_oom_badness(memcg, nodemask, totalpages);
+}
+
+static void select_victim_memcg(struct mem_cgroup *root, struct oom_control *oc)
+{
+	struct mem_cgroup *iter;
+
+	oc->chosen_memcg = NULL;
+	oc->chosen_points = 0;
+
+	/*
+	 * The oom_score is calculated for leaf memory cgroups (including
+	 * the root memcg).
+	 */
+	rcu_read_lock();
+	for_each_mem_cgroup_tree(iter, root) {
+		long score;
+
+		if (memcg_has_children(iter) && iter != root_mem_cgroup)
+			continue;
+
+		score = oom_evaluate_memcg(iter, oc->nodemask, oc->totalpages);
+
+		/*
+		 * Ignore empty and non-eligible memory cgroups.
+		 */
+		if (score == 0)
+			continue;
+
+		/*
+		 * If there are inflight OOM victims, we don't need
+		 * to look further for new victims.
+		 */
+		if (score == -1) {
+			oc->chosen_memcg = INFLIGHT_VICTIM;
+			mem_cgroup_iter_break(root, iter);
+			break;
+		}
+
+		if (score > oc->chosen_points) {
+			oc->chosen_points = score;
+			oc->chosen_memcg = iter;
+		}
+	}
+
+	if (oc->chosen_memcg && oc->chosen_memcg != INFLIGHT_VICTIM)
+		css_get(&oc->chosen_memcg->css);
+
+	rcu_read_unlock();
+}
+
+bool mem_cgroup_select_oom_victim(struct oom_control *oc)
+{
+	struct mem_cgroup *root;
+
+	if (mem_cgroup_disabled())
+		return false;
+
+	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys))
+		return false;
+
+	if (oc->memcg)
+		root = oc->memcg;
+	else
+		root = root_mem_cgroup;
+
+	select_victim_memcg(root, oc);
+
+	return oc->chosen_memcg;
+}
+
 /*
  * Reclaims as many pages from the given memcg as possible.
  *

diff -puN mm/oom_kill.c~mm-oom-cgroup-aware-oom-killer mm/oom_kill.c

mm/oom_kill.c renamed from mm/oom_kill.c~mm-oom-cgroup-aware-oom-killer +33 -11

@@ -309,7 +309,7 @@ static enum oom_constraint constrained_a
 	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,26 +343,26 @@ static int oom_evaluate_task(struct task
 		goto next;
 
 	/* Prefer thread group leaders for display purposes */
-	if (points == oc->chosen_points && thread_group_leader(oc->chosen))
+	if (points == oc->chosen_points && thread_group_leader(oc->chosen_task))
 		goto next;
 select:
-	if (oc->chosen)
-		put_task_struct(oc->chosen);
+	if (oc->chosen_task)
+		put_task_struct(oc->chosen_task);
 	get_task_struct(task);
-	oc->chosen = task;
+	oc->chosen_task = task;
 	oc->chosen_points = points;
 next:
 	return 0;
 abort:
-	if (oc->chosen)
-		put_task_struct(oc->chosen);
-	oc->chosen = (void *)-1UL;
+	if (oc->chosen_task)
+		put_task_struct(oc->chosen_task);
+	oc->chosen_task = INFLIGHT_VICTIM;
 	return 1;
 }
 
 /*
  * Simple selection loop. We choose the process with the highest number of
- * 'points'. In case scan was aborted, oc->chosen is set to -1.
+ * 'points'. In case scan was aborted, oc->chosen_task is set to -1.
  */
 static void select_bad_process(struct oom_control *oc)
 {

@@ -912,7 +912,7 @@ static void __oom_kill_process(struct ta
 
 static void oom_kill_process(struct oom_control *oc, const char *message)
 {
-	struct task_struct *p = oc->chosen;
+	struct task_struct *p = oc->chosen_task;
 	unsigned int points = oc->chosen_points;
 	struct task_struct *victim = p;
 	struct task_struct *child;

@@ -973,6 +973,27 @@ static void oom_kill_process(struct oom_
 	__oom_kill_process(victim);
 }
 
+static bool oom_kill_memcg_victim(struct oom_control *oc)
+{
+
+	if (oc->chosen_memcg == NULL || oc->chosen_memcg == INFLIGHT_VICTIM)
+		return oc->chosen_memcg;
+
+	/* Kill a task in the chosen memcg with the biggest memory footprint */
+	oc->chosen_points = 0;
+	oc->chosen_task = NULL;
+	mem_cgroup_scan_tasks(oc->chosen_memcg, oom_evaluate_task, oc);
+
+	if (oc->chosen_task == NULL || oc->chosen_task == INFLIGHT_VICTIM)
+		goto out;
+
+	__oom_kill_process(oc->chosen_task);
+
+out:
+	mem_cgroup_put(oc->chosen_memcg);
+	return oc->chosen_task;
+}
+
 /*
  * Determines whether the kernel must panic because of the panic_on_oom sysctl.
  */

@@ -1025,6 +1046,7 @@ bool out_of_memory(struct oom_control *o
 {
 	unsigned long freed = 0;
 	enum oom_constraint constraint = CONSTRAINT_NONE;
+	bool delay = false; /* if set, delay next allocation attempt */
 
 	if (oom_killer_disabled)
 		return false;

@@ -1069,27 +1091,39 @@ bool out_of_memory(struct oom_control *o
 	    current->mm && !oom_unkillable_task(current, NULL, oc->nodemask) &&
 	    current->signal->oom_score_adj != OOM_SCORE_ADJ_MIN) {
 		get_task_struct(current);
-		oc->chosen = current;
+		oc->chosen_task = current;

Add a "groupoom" cgroup v2 mount option to enable the cgroup-aware OOM killer. If not set, the OOM selection is performed in a "traditional" per-process way.

The behavior can be changed dynamically by remounting the cgroupfs.

Signed-off-by Roman Gushchin <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Vladimir Davydov <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Tetsuo Handa <[email protected]>

Cc David Rientjes <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Tejun Heo <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

---
 include/linux/cgroup-defs.h |  5 +++++
 kernel/cgroup/cgroup.c      | 10 ++++++++++
 mm/memcontrol.c             |  3 +++
 3 files changed, 18 insertions(+)

include/linux/cgroup-defs.h +5 -0

index 8b7fd8eeccee..9fb99e25d654 100644
@@ -81,6 +81,11 @@ enum {
 	 * Enable cpuset controller in v1 cgroup to use v2 behavior.
 	 */
 	CGRP_ROOT_CPUSET_V2_MODE = (1 << 4),
+
+	/*
+	 * Enable cgroup-aware OOM killer.
+	 */
+	CGRP_GROUP_OOM = (1 << 5),
 };
 
 /* cftype->flags */

kernel/cgroup/cgroup.c +10 -0

index 0b1ffe147f24..7338e12979e1 100644
@@ -1731,6 +1731,9 @@ static int parse_cgroup_root_flags(char *data, unsigned int *root_flags)
 		if (!strcmp(token, "nsdelegate")) {
 			*root_flags |= CGRP_ROOT_NS_DELEGATE;
 			continue;
+		} else if (!strcmp(token, "groupoom")) {
+			*root_flags |= CGRP_GROUP_OOM;
+			continue;
 		}
 
 		pr_err("cgroup2: unknown option \"%s\"\n", token);

@@ -1747,6 +1750,11 @@ static void apply_cgroup_root_flags(unsigned int root_flags)
 			cgrp_dfl_root.flags |= CGRP_ROOT_NS_DELEGATE;
 		else
 			cgrp_dfl_root.flags &= ~CGRP_ROOT_NS_DELEGATE;
+
+		if (root_flags & CGRP_GROUP_OOM)
+			cgrp_dfl_root.flags |= CGRP_GROUP_OOM;
+		else
+			cgrp_dfl_root.flags &= ~CGRP_GROUP_OOM;
 	}
 }
 

@@ -1754,6 +1762,8 @@ static int cgroup_show_options(struct seq_file *seq, struct kernfs_root *kf_root
 {
 	if (cgrp_dfl_root.flags & CGRP_ROOT_NS_DELEGATE)
 		seq_puts(seq, ",nsdelegate");
+	if (cgrp_dfl_root.flags & CGRP_GROUP_OOM)
+		seq_puts(seq, ",groupoom");
 	return 0;
 }
 

mm/memcontrol.c +3 -0

index 5d27a4bbd478..c76d5fb55c5c 100644
@@ -2869,6 +2869,9 @@ bool mem_cgroup_select_oom_victim(struct oom_control *oc)
 	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys))
 		return false;
 
+	if (!(cgrp_dfl_root.flags & CGRP_GROUP_OOM))
+		return false;
+
 	if (oc->memcg)
 		root = oc->memcg;
 	else

Signature · 1 line

2.14.3

Quoted 5 lines · Roman Gushchin

Add a "groupoom" cgroup v2 mount option to enable the cgroup-aware OOM killer. If not set, the OOM selection is performed in a "traditional" per-process way.

The behavior can be changed dynamically by remounting the cgroupfs.

Is it ok to create oom_group if the option is not enabled? This looks confusing. I forgot all the details about how cgroup core creates file so I do not have a good idea how to fix this.

So, maybe we just need to return -EAGAIN (or may be -ENOTSUP) on any read/write attempt if option is not enabled?

On Fri 01-12-17 13:15:38, Roman Gushchin wrote:
[...]

Quoted 2 lines

So, maybe we just need to return -EAGAIN (or may be -ENOTSUP) on any read/write attempt if option is not enabled?

Yes, that would work as well. ENOTSUP sounds better to me.

Signature · 2 lines

Michal Hocko
SUSE Labs

Review to be accessed repeatedly by other cgroups, it may make sense to use · line 1259

Quoted 9 lines · Roman Gushchin

(quoted diff) +5 -0

@@ -1229,6 +1252,41 @@ to be accessed repeatedly by other cgroups, it may make sense to use
 POSIX_FADV_DONTNEED to relinquish the ownership of memory areas
 belonging to the affected files to ensure correct memory ownership.
 
+OOM Killer
+~~~~~~~~~~
+
+Cgroup v2 memory controller implements a cgroup-aware OOM killer.
+It means that it treats cgroups as first class OOM entities.

This should mention groupoom mount option to enable this functionality.

Other than that looks ok to me

Acked-by Michal Hocko <[email protected]>

Signature · 2 lines

Michal Hocko
SUSE Labs

Documentation/cgroup-v2.txt +3 -0

index c80a147f94b7..ff8e92db636d 100644
@@ -1049,6 +1049,9 @@ PAGE_SIZE multiple when read back.
 	and will never kill the unkillable task, even if memory.oom_group
 	is set.
 
+	If cgroup-aware OOM killer is not enabled, ENOTSUPP error
+	is returned on attempt to access the file.
+
   memory.events
 	A read-only flat-keyed file which exists on non-root cgroups.
 	The following entries are defined.  Unless specified

@@ -1258,6 +1261,12 @@ OOM Killer
 Cgroup v2 memory controller implements a cgroup-aware OOM killer.
 It means that it treats cgroups as first class OOM entities.

This patchset makes the OOM killer cgroup-aware.

Quoted 1 line

This patchset makes the OOM killer cgroup-aware.

Thanks, I'll grab these.

There has been controversy over this patchset, to say the least. I can't say that I followed it closely! Could those who still have reservations please summarise their concerns and hopefully suggest a way forward?

Quoted 9 lines · Andrew Morton

Quoted 1 line

This patchset makes the OOM killer cgroup-aware.

Thanks, I'll grab these.

There has been controversy over this patchset, to say the least. I can't say that I followed it closely! Could those who still have reservations please summarise their concerns and hopefully suggest a way forward?

Yes, I'll summarize what my concerns have been in the past and what they are wrt the patchset as it stands in -mm. None of them originate from my current usecase or anticipated future usecase of the oom killer for system-wide or memcg-constrained oom conditions. They are based purely on the patchset's use of an incomplete and unfair heuristic for deciding which cgroup to target.

I'll also suggest simple changes to the patchset, which I have in the past, that can be made to address all of these concerns.

1. The unfair comparison of the root mem cgroup vs leaf mem cgroups

The patchset uses two different heuristics to compare root and leaf mem cgroups and scores them based on number of pages. For the root mem cgroup, it totals the /proc/pid/oom_score of all processes attached:
that's based on rss, swap, pgtables, and, most importantly, oom_score_adj. For leaf mem cgroups, it's based on that memcg's anonymous, unevictable, unreclaimable slab, kernel stack, and swap counters. These can be wildly different independent of /proc/pid/oom_score_adj, but the most obvious unfairness comes from users who tune oom_score_adj.

An example: start a process that faults 1GB of anonymous memory and leave it attached to the root mem cgroup. Start six more processes that each fault 1GB of anonymous memory and attached them to a leaf mem cgroup. Set all processes to have /proc/pid/oom_score_adj of 1000. System oom kill will always kill the 1GB process attached to the root mem cgroup. It's because oom_badness() relies on /proc/pid/oom_score_adj, which is used to evaluate the root mem cgroup, and leaf mem cgroups completely disregard it.

In this example, the leaf mem cgroup's score is 1,573,044, the number of pages for the 6GB of faulted memory. The root mem cgroup's score is 12,652,907, eight times larger even though its usage is six times smaller.

This is caused by the patchset disregarding oom_score_adj entirely for leaf mem cgroups and relying on it heavily for the root mem cgroup. It's the complete opposite result of what the cgroup aware oom killer advertises.

It also works the other way, if a large memory hog is attached to the root mem cgroup but has a negative oom_score_adj it is never killed and random processes are nuked solely because they happened to be attached to a leaf mem cgroup. This behavior wrt oom_score_adj is completely undocumented, so I can't presume that it is either known nor tested.

Solution: compare the root mem cgroup and leaf mem cgroups equally with the same criteria by doing hierarchical accounting of usage and subtracting from total system usage to find root usage.

2. Evading the oom killer by attaching processes to child cgroups

Any cgroup on the system can attach all their processes to individual child cgroups. This is functionally the same as doing

	for i in $(cat cgroup.procs); do mkdir $i; echo $i > $i/cgroup.procs; done

without the no internal process constraint introduced with cgroup v2. All child cgroups are evaluated based on their own usage: all anon, unevictable, and unreclaimable slab as described previously. It requires an individual cgroup to be the single largest consumer to be targeted by the oom killer.

An example: allow users to manage two different mem cgroup hierarchies limited to 100GB each. User A uses 10GB of memory and user B uses 90GB of memory in their respective hierarchies. On a system oom condition, we'd expect at least one process from user B's hierarchy would always be oom killed with the cgroup aware oom killer. In fact, the changelog explicitly states it solves an issue where "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."

The opposite becomes true, however, if user B creates child cgroups and distributes its processes such that each child cgroup's usage never exceeds 10GB of memory. This can either be done intentionally to purposefully have a low cgroup memory footprint to evade the oom killer or unintentionally with cgroup v2 to allow those individual processes to be constrained by other cgroups in a single hierarchy model. User A, using 10% of his memory limit, is always oom killed instead of user B, using 90% of his memory limit.

Others have commented its still possible to do this with a per-process model if users split their processes into many subprocesses with small memory footprints.

Solution: comparing cgroups must be done hierarchically. Neither user A nor user B can evade the oom killer because targeting is done based on the total hierarchical usage rather than individual cgroups in their hierarchies.

3. Userspace has zero control over oom kill selection in leaf mem cgroups

Unlike using /proc/pid/oom_score_adj to bias or prefer certain processes from the oom killer, the cgroup aware oom killer does not provide any solution for the user to protect leaf mem cgroups. This is a result of leaf mem cgroups being evaluated based on their anon, unevictable, and unreclaimable slab usage and disregarding any user tunable.

Absent the cgroup aware oom killer, users have the ability to strongly prefer a process is oom killed (/proc/pid/oom_score_adj = 1000) or strongly bias against a process (/proc/pid/oom_score_adj = -999).

An example: a process knows its going to use a lot of memory, so it sets /proc/self/oom_score_adj to 1000. It wants to be killed first to avoid distrupting any other process. If it's attached to the root mem cgroup, it will be oom killed. If it's attached to a leaf mem cgroup by an admin outside its control, it will never be oom killed unless that cgroup's usage is the largest single cgroup usage on the system. The reverse also is true for processes that the admin does not want to be oom killed: set /proc/pid/oom_score_adj to -999, but it will *always* be oom killed if its cgroup has the highest usage on the system.

The result is that both admins and users have lost all control over which processes are oom killed. They are left with only one alternative: set /proc/pid/oom_score_adj to -1000 to completely disable a process from oom kill. It doesn't address the issue at all for memcg-constrained oom conditions since no processes are killable anymore, and risks panicking the system if it is the only process left on the system. A process preferring that it is first in line for oom kill simply cannot volunteer anymore.

Solution: allow users and admins to control oom kill selection by introducing a memory.oom_score_adj to affect the oom score of that mem cgroup, exactly the same as /proc/pid/oom_score_adj affects the oom score of a process.

I proposed a solution in
https://marc.info/?l=linux-kernel&m=150956897302725, which was never responded to, for all of these issues. The idea is to do hierarchical accounting of mem cgroup hierarchies so that the hierarchy is traversed comparing total usage at each level to select target cgroups. Admins and users can use memory.oom_score_adj to influence that decisionmaking at each level.

This solves #1 because mem cgroups can be compared based on the same classes of memory and the root mem cgroup's usage can be fairly compared by subtracting top-level mem cgroup usage from system usage. All of the criteria used to evaluate a leaf mem cgroup has a reasonable system-wide counterpart that can be used to do the simple subtraction.

This solves #2 because evaluation is done hierarchically so that distributing processes over a set of child cgroups either intentionally or unintentionally no longer evades the oom killer. Total usage is always accounted to the parent and there is no escaping this criteria for users.

This solves #3 because it allows admins to protect important processes in cgroups that are supposed to use, for example, 75% of system memory without it unconditionally being selected for oom kill but still oom kill if it exceeds a certain threshold. In this sense, the cgroup aware oom killer, as currently implemented, is selling mem cgroups short by requiring the user to accept that the important process will be oom killed iff it uses mem cgroups and isn't attached to root. It also allows users to actually volunteer to be oom killed first without majority usage.

It has come up time and time again that this support can be introduced on top of the cgroup oom killer as implemented. It simply cannot. For admins and users to have control over decisionmaking, it needs a oom_score_adj type tunable that cannot change semantics from kernel version to kernel version and without polluting the mem cgroup filesystem. That, in my suggestion, is an adjustment on the amount of total hierarchical usage of each mem cgroup at each level of the hierarchy. That requires that the heuristic uses hierarchical usage rather than considering each cgroup as independent consumers as it does today. We need to implement that heuristic and introduce userspace influence over oom kill selection now rather than later because its implementation changes how this patchset is implemented.

I can implement these changes, if preferred, on top of the current patchset, but I do not believe we want inconsistencies between kernel versions that introduce user visible changes for the sole reason that this current implementation is incomplete and unfair. We can implement and introduce it once without behavior changing later because the core heuristic has necessarily changed.

Hello, David!

Quoted 57 lines · David Rientjes

Quoted 9 lines · Andrew Morton

Quoted 1 line

This patchset makes the OOM killer cgroup-aware.

Thanks, I'll grab these.

There has been controversy over this patchset, to say the least. I can't say that I followed it closely! Could those who still have reservations please summarise their concerns and hopefully suggest a way forward?

Yes, I'll summarize what my concerns have been in the past and what they are wrt the patchset as it stands in -mm. None of them originate from my current usecase or anticipated future usecase of the oom killer for system-wide or memcg-constrained oom conditions. They are based purely on the patchset's use of an incomplete and unfair heuristic for deciding which cgroup to target.

I'll also suggest simple changes to the patchset, which I have in the past, that can be made to address all of these concerns.

1. The unfair comparison of the root mem cgroup vs leaf mem cgroups

The patchset uses two different heuristics to compare root and leaf mem cgroups and scores them based on number of pages. For the root mem cgroup, it totals the /proc/pid/oom_score of all processes attached:
that's based on rss, swap, pgtables, and, most importantly, oom_score_adj. For leaf mem cgroups, it's based on that memcg's anonymous, unevictable, unreclaimable slab, kernel stack, and swap counters. These can be wildly different independent of /proc/pid/oom_score_adj, but the most obvious unfairness comes from users who tune oom_score_adj.

An example: start a process that faults 1GB of anonymous memory and leave it attached to the root mem cgroup. Start six more processes that each fault 1GB of anonymous memory and attached them to a leaf mem cgroup. Set all processes to have /proc/pid/oom_score_adj of 1000. System oom kill will always kill the 1GB process attached to the root mem cgroup. It's because oom_badness() relies on /proc/pid/oom_score_adj, which is used to evaluate the root mem cgroup, and leaf mem cgroups completely disregard it.

In this example, the leaf mem cgroup's score is 1,573,044, the number of pages for the 6GB of faulted memory. The root mem cgroup's score is 12,652,907, eight times larger even though its usage is six times smaller.

This is caused by the patchset disregarding oom_score_adj entirely for leaf mem cgroups and relying on it heavily for the root mem cgroup. It's the complete opposite result of what the cgroup aware oom killer advertises.

It also works the other way, if a large memory hog is attached to the root mem cgroup but has a negative oom_score_adj it is never killed and random processes are nuked solely because they happened to be attached to a leaf mem cgroup. This behavior wrt oom_score_adj is completely undocumented, so I can't presume that it is either known nor tested.

Quoted 3 lines

Solution: compare the root mem cgroup and leaf mem cgroups equally with the same criteria by doing hierarchical accounting of usage and subtracting from total system usage to find root usage.

I find this problem quite minor, because I haven't seen any practical problems caused by accounting of the root cgroup memory.
If it's a serious problem for you, it can be solved without switching to the hierarchical accounting: it's possible to sum up all leaf cgroup stats and substract them from global values. So, it can be a relatively small enhancement on top of the current mm tree. This has nothing to do with global victim selection approach.

Quoted 32 lines

2. Evading the oom killer by attaching processes to child cgroups

Any cgroup on the system can attach all their processes to individual child cgroups. This is functionally the same as doing

	for i in $(cat cgroup.procs); do mkdir $i; echo $i > $i/cgroup.procs; done

without the no internal process constraint introduced with cgroup v2. All child cgroups are evaluated based on their own usage: all anon, unevictable, and unreclaimable slab as described previously. It requires an individual cgroup to be the single largest consumer to be targeted by the oom killer.

An example: allow users to manage two different mem cgroup hierarchies limited to 100GB each. User A uses 10GB of memory and user B uses 90GB of memory in their respective hierarchies. On a system oom condition, we'd expect at least one process from user B's hierarchy would always be oom killed with the cgroup aware oom killer. In fact, the changelog explicitly states it solves an issue where "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."

The opposite becomes true, however, if user B creates child cgroups and distributes its processes such that each child cgroup's usage never exceeds 10GB of memory. This can either be done intentionally to purposefully have a low cgroup memory footprint to evade the oom killer or unintentionally with cgroup v2 to allow those individual processes to be constrained by other cgroups in a single hierarchy model. User A, using 10% of his memory limit, is always oom killed instead of user B, using 90% of his memory limit.

Quoted 8 lines

Others have commented its still possible to do this with a per-process model if users split their processes into many subprocesses with small memory footprints.

Solution: comparing cgroups must be done hierarchically. Neither user A nor user B can evade the oom killer because targeting is done based on the total hierarchical usage rather than individual cgroups in their hierarchies.

We've discussed this a lot.
Hierarchical approach has their own issues, which we've discussed during previous iterations of the patchset. If you know how to address them (I've no idea), please, go on and suggest your version.

Quoted 32 lines

3. Userspace has zero control over oom kill selection in leaf mem cgroups

Unlike using /proc/pid/oom_score_adj to bias or prefer certain processes from the oom killer, the cgroup aware oom killer does not provide any solution for the user to protect leaf mem cgroups. This is a result of leaf mem cgroups being evaluated based on their anon, unevictable, and unreclaimable slab usage and disregarding any user tunable.

Absent the cgroup aware oom killer, users have the ability to strongly prefer a process is oom killed (/proc/pid/oom_score_adj = 1000) or strongly bias against a process (/proc/pid/oom_score_adj = -999).

An example: a process knows its going to use a lot of memory, so it sets /proc/self/oom_score_adj to 1000. It wants to be killed first to avoid distrupting any other process. If it's attached to the root mem cgroup, it will be oom killed. If it's attached to a leaf mem cgroup by an admin outside its control, it will never be oom killed unless that cgroup's usage is the largest single cgroup usage on the system. The reverse also is true for processes that the admin does not want to be oom killed: set /proc/pid/oom_score_adj to -999, but it will *always* be oom killed if its cgroup has the highest usage on the system.

The result is that both admins and users have lost all control over which processes are oom killed. They are left with only one alternative: set /proc/pid/oom_score_adj to -1000 to completely disable a process from oom kill. It doesn't address the issue at all for memcg-constrained oom conditions since no processes are killable anymore, and risks panicking the system if it is the only process left on the system. A process preferring that it is first in line for oom kill simply cannot volunteer anymore.

Quoted 4 lines

Solution: allow users and admins to control oom kill selection by introducing a memory.oom_score_adj to affect the oom score of that mem cgroup, exactly the same as /proc/pid/oom_score_adj affects the oom score of a process.

The per-process oom_score_adj interface is not the nicest one, and I'm not sure we want to replicate it on cgroup level as is. If you have an idea of how it should look like, please, propose a patch; otherwise it's hard to discuss it without the code.

Quoted 43 lines

I proposed a solution in
https://marc.info/?l=linux-kernel&m=150956897302725, which was never responded to, for all of these issues. The idea is to do hierarchical accounting of mem cgroup hierarchies so that the hierarchy is traversed comparing total usage at each level to select target cgroups. Admins and users can use memory.oom_score_adj to influence that decisionmaking at each level.

This solves #1 because mem cgroups can be compared based on the same classes of memory and the root mem cgroup's usage can be fairly compared by subtracting top-level mem cgroup usage from system usage. All of the criteria used to evaluate a leaf mem cgroup has a reasonable system-wide counterpart that can be used to do the simple subtraction.

This solves #2 because evaluation is done hierarchically so that distributing processes over a set of child cgroups either intentionally or unintentionally no longer evades the oom killer. Total usage is always accounted to the parent and there is no escaping this criteria for users.

This solves #3 because it allows admins to protect important processes in cgroups that are supposed to use, for example, 75% of system memory without it unconditionally being selected for oom kill but still oom kill if it exceeds a certain threshold. In this sense, the cgroup aware oom killer, as currently implemented, is selling mem cgroups short by requiring the user to accept that the important process will be oom killed iff it uses mem cgroups and isn't attached to root. It also allows users to actually volunteer to be oom killed first without majority usage.

It has come up time and time again that this support can be introduced on top of the cgroup oom killer as implemented. It simply cannot. For admins and users to have control over decisionmaking, it needs a oom_score_adj type tunable that cannot change semantics from kernel version to kernel version and without polluting the mem cgroup filesystem. That, in my suggestion, is an adjustment on the amount of total hierarchical usage of each mem cgroup at each level of the hierarchy. That requires that the heuristic uses hierarchical usage rather than considering each cgroup as independent consumers as it does today. We need to implement that heuristic and introduce userspace influence over oom kill selection now rather than later because its implementation changes how this patchset is implemented.

Quoted 6 lines

I can implement these changes, if preferred, on top of the current patchset, but I do not believe we want inconsistencies between kernel versions that introduce user visible changes for the sole reason that this current implementation is incomplete and unfair. We can implement and introduce it once without behavior changing later because the core heuristic has necessarily changed.

David, I _had_ hierarchical accounting implemented in one of the previous versions of this patchset. And there were _reasons_, why we went away from it.

You can't just ignore them and say that "there is a simple solution, which Roman is not responding". If you know how to address these issues and convince everybody that hierarchical approach is a way to go, please, go on and send your version of the patchset.

Thanks!

Roman

Quoted 64 lines · On Wed, 10 Jan 2018 05:11:44 -0800 Roman Gushchin <[email protected]> wrote:

Hello, David!

Quoted 57 lines · David Rientjes

Quoted 9 lines · Andrew Morton

Quoted 1 line

This patchset makes the OOM killer cgroup-aware.

Thanks, I'll grab these.

There has been controversy over this patchset, to say the least. I can't say that I followed it closely! Could those who still have reservations please summarise their concerns and hopefully suggest a way forward?

Yes, I'll summarize what my concerns have been in the past and what they are wrt the patchset as it stands in -mm. None of them originate from my current usecase or anticipated future usecase of the oom killer for system-wide or memcg-constrained oom conditions. They are based purely on the patchset's use of an incomplete and unfair heuristic for deciding which cgroup to target.

I'll also suggest simple changes to the patchset, which I have in the past, that can be made to address all of these concerns.

1. The unfair comparison of the root mem cgroup vs leaf mem cgroups

The patchset uses two different heuristics to compare root and leaf mem cgroups and scores them based on number of pages. For the root mem cgroup, it totals the /proc/pid/oom_score of all processes attached:
that's based on rss, swap, pgtables, and, most importantly, oom_score_adj. For leaf mem cgroups, it's based on that memcg's anonymous, unevictable, unreclaimable slab, kernel stack, and swap counters. These can be wildly different independent of /proc/pid/oom_score_adj, but the most obvious unfairness comes from users who tune oom_score_adj.

An example: start a process that faults 1GB of anonymous memory and leave it attached to the root mem cgroup. Start six more processes that each fault 1GB of anonymous memory and attached them to a leaf mem cgroup. Set all processes to have /proc/pid/oom_score_adj of 1000. System oom kill will always kill the 1GB process attached to the root mem cgroup. It's because oom_badness() relies on /proc/pid/oom_score_adj, which is used to evaluate the root mem cgroup, and leaf mem cgroups completely disregard it.

In this example, the leaf mem cgroup's score is 1,573,044, the number of pages for the 6GB of faulted memory. The root mem cgroup's score is 12,652,907, eight times larger even though its usage is six times smaller.

This is caused by the patchset disregarding oom_score_adj entirely for leaf mem cgroups and relying on it heavily for the root mem cgroup. It's the complete opposite result of what the cgroup aware oom killer advertises.

It also works the other way, if a large memory hog is attached to the root mem cgroup but has a negative oom_score_adj it is never killed and random processes are nuked solely because they happened to be attached to a leaf mem cgroup. This behavior wrt oom_score_adj is completely undocumented, so I can't presume that it is either known nor tested.

Quoted 3 lines

Solution: compare the root mem cgroup and leaf mem cgroups equally with the same criteria by doing hierarchical accounting of usage and subtracting from total system usage to find root usage.

Quoted 7 lines

I find this problem quite minor, because I haven't seen any practical problems caused by accounting of the root cgroup memory.
If it's a serious problem for you, it can be solved without switching to the hierarchical accounting: it's possible to sum up all leaf cgroup stats and substract them from global values. So, it can be a relatively small enhancement on top of the current mm tree. This has nothing to do with global victim selection approach.

It sounds like a significant shortcoming to me - the oom-killing decisions which David describes are clearly incorrect?

If this can be fixed against the -mm patchset with a "relatively small enhancement" then please let's get that done so it can be reviewed and tested.

Quoted 40 lines

Quoted 39 lines

2. Evading the oom killer by attaching processes to child cgroups

Any cgroup on the system can attach all their processes to individual child cgroups. This is functionally the same as doing

	for i in $(cat cgroup.procs); do mkdir $i; echo $i > $i/cgroup.procs; done

without the no internal process constraint introduced with cgroup v2. All child cgroups are evaluated based on their own usage: all anon, unevictable, and unreclaimable slab as described previously. It requires an individual cgroup to be the single largest consumer to be targeted by the oom killer.

An example: allow users to manage two different mem cgroup hierarchies limited to 100GB each. User A uses 10GB of memory and user B uses 90GB of memory in their respective hierarchies. On a system oom condition, we'd expect at least one process from user B's hierarchy would always be oom killed with the cgroup aware oom killer. In fact, the changelog explicitly states it solves an issue where "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."

The opposite becomes true, however, if user B creates child cgroups and distributes its processes such that each child cgroup's usage never exceeds 10GB of memory. This can either be done intentionally to purposefully have a low cgroup memory footprint to evade the oom killer or unintentionally with cgroup v2 to allow those individual processes to be constrained by other cgroups in a single hierarchy model. User A, using 10% of his memory limit, is always oom killed instead of user B, using 90% of his memory limit.

Others have commented its still possible to do this with a per-process model if users split their processes into many subprocesses with small memory footprints.

Solution: comparing cgroups must be done hierarchically. Neither user A nor user B can evade the oom killer because targeting is done based on the total hierarchical usage rather than individual cgroups in their hierarchies.

Quoted 4 lines

We've discussed this a lot.
Hierarchical approach has their own issues, which we've discussed during previous iterations of the patchset. If you know how to address them (I've no idea), please, go on and suggest your version.

Well, if a hierarchical approach isn't a workable fix for the problem which David has identified then what *is* the fix?

Quoted 36 lines

Quoted 35 lines

3. Userspace has zero control over oom kill selection in leaf mem cgroups

Unlike using /proc/pid/oom_score_adj to bias or prefer certain processes from the oom killer, the cgroup aware oom killer does not provide any solution for the user to protect leaf mem cgroups. This is a result of leaf mem cgroups being evaluated based on their anon, unevictable, and unreclaimable slab usage and disregarding any user tunable.

Absent the cgroup aware oom killer, users have the ability to strongly prefer a process is oom killed (/proc/pid/oom_score_adj = 1000) or strongly bias against a process (/proc/pid/oom_score_adj = -999).

An example: a process knows its going to use a lot of memory, so it sets /proc/self/oom_score_adj to 1000. It wants to be killed first to avoid distrupting any other process. If it's attached to the root mem cgroup, it will be oom killed. If it's attached to a leaf mem cgroup by an admin outside its control, it will never be oom killed unless that cgroup's usage is the largest single cgroup usage on the system. The reverse also is true for processes that the admin does not want to be oom killed: set /proc/pid/oom_score_adj to -999, but it will *always* be oom killed if its cgroup has the highest usage on the system.

The result is that both admins and users have lost all control over which processes are oom killed. They are left with only one alternative: set /proc/pid/oom_score_adj to -1000 to completely disable a process from oom kill. It doesn't address the issue at all for memcg-constrained oom conditions since no processes are killable anymore, and risks panicking the system if it is the only process left on the system. A process preferring that it is first in line for oom kill simply cannot volunteer anymore.

Solution: allow users and admins to control oom kill selection by introducing a memory.oom_score_adj to affect the oom score of that mem cgroup, exactly the same as /proc/pid/oom_score_adj affects the oom score of a process.

Quoted 4 lines

The per-process oom_score_adj interface is not the nicest one, and I'm not sure we want to replicate it on cgroup level as is. If you have an idea of how it should look like, please, propose a patch; otherwise it's hard to discuss it without the code.

It does make sense to have some form of per-cgroup tunability. Why is the oom_score_adj approach inappropriate and what would be better?

How hard is it to graft such a thing onto the -mm patchset?

Quoted 48 lines

Quoted 47 lines

I proposed a solution in
https://marc.info/?l=linux-kernel&m=150956897302725, which was never responded to, for all of these issues. The idea is to do hierarchical accounting of mem cgroup hierarchies so that the hierarchy is traversed comparing total usage at each level to select target cgroups. Admins and users can use memory.oom_score_adj to influence that decisionmaking at each level.

This solves #1 because mem cgroups can be compared based on the same classes of memory and the root mem cgroup's usage can be fairly compared by subtracting top-level mem cgroup usage from system usage. All of the criteria used to evaluate a leaf mem cgroup has a reasonable system-wide counterpart that can be used to do the simple subtraction.

This solves #2 because evaluation is done hierarchically so that distributing processes over a set of child cgroups either intentionally or unintentionally no longer evades the oom killer. Total usage is always accounted to the parent and there is no escaping this criteria for users.

This solves #3 because it allows admins to protect important processes in cgroups that are supposed to use, for example, 75% of system memory without it unconditionally being selected for oom kill but still oom kill if it exceeds a certain threshold. In this sense, the cgroup aware oom killer, as currently implemented, is selling mem cgroups short by requiring the user to accept that the important process will be oom killed iff it uses mem cgroups and isn't attached to root. It also allows users to actually volunteer to be oom killed first without majority usage.

It has come up time and time again that this support can be introduced on top of the cgroup oom killer as implemented. It simply cannot. For admins and users to have control over decisionmaking, it needs a oom_score_adj type tunable that cannot change semantics from kernel version to kernel version and without polluting the mem cgroup filesystem. That, in my suggestion, is an adjustment on the amount of total hierarchical usage of each mem cgroup at each level of the hierarchy. That requires that the heuristic uses hierarchical usage rather than considering each cgroup as independent consumers as it does today. We need to implement that heuristic and introduce userspace influence over oom kill selection now rather than later because its implementation changes how this patchset is implemented.

I can implement these changes, if preferred, on top of the current patchset, but I do not believe we want inconsistencies between kernel versions that introduce user visible changes for the sole reason that this current implementation is incomplete and unfair. We can implement and introduce it once without behavior changing later because the core heuristic has necessarily changed.

Quoted 2 lines

David, I _had_ hierarchical accounting implemented in one of the previous versions of this patchset. And there were _reasons_, why we went away from it.

Can you please summarize those issues for my understanding?

Quoted 7 lines · Andrew Morton

Quoted 4 lines · On Wed, 10 Jan 2018 05:11:44 -0800 Roman Gushchin…

The per-process oom_score_adj interface is not the nicest one, and I'm not sure we want to replicate it on cgroup level as is. If you have an idea of how it should look like, please, propose a patch; otherwise it's hard to discuss it without the code.

Quoted 2 lines

It does make sense to have some form of per-cgroup tunability. Why is the oom_score_adj approach inappropriate and what would be better?

oom_score_adj is basically unusable for any fine tuning on the process level for most setups except for very specialized ones. The only reasonable usage I've seen so far was to disable OOM killer for a process or make it a prime candidate. Using the same limited concept for cgroups sounds like repeating the same error to me.

Quoted 2 lines

How hard is it to graft such a thing onto the -mm patchset?

I think this should be thought through very well before we add another tuning. Moreover the current usecase doesn't seem to require it so I am not really sure why we should implement something right away and later suffer from API mistakes.

Quoted 48 lines

Quoted 48 lines

Quoted 47 lines

I proposed a solution in
https://marc.info/?l=linux-kernel&m=150956897302725, which was never responded to, for all of these issues. The idea is to do hierarchical accounting of mem cgroup hierarchies so that the hierarchy is traversed comparing total usage at each level to select target cgroups. Admins and users can use memory.oom_score_adj to influence that decisionmaking at each level.

This solves #1 because mem cgroups can be compared based on the same classes of memory and the root mem cgroup's usage can be fairly compared by subtracting top-level mem cgroup usage from system usage. All of the criteria used to evaluate a leaf mem cgroup has a reasonable system-wide counterpart that can be used to do the simple subtraction.

This solves #2 because evaluation is done hierarchically so that distributing processes over a set of child cgroups either intentionally or unintentionally no longer evades the oom killer. Total usage is always accounted to the parent and there is no escaping this criteria for users.

This solves #3 because it allows admins to protect important processes in cgroups that are supposed to use, for example, 75% of system memory without it unconditionally being selected for oom kill but still oom kill if it exceeds a certain threshold. In this sense, the cgroup aware oom killer, as currently implemented, is selling mem cgroups short by requiring the user to accept that the important process will be oom killed iff it uses mem cgroups and isn't attached to root. It also allows users to actually volunteer to be oom killed first without majority usage.

It has come up time and time again that this support can be introduced on top of the cgroup oom killer as implemented. It simply cannot. For admins and users to have control over decisionmaking, it needs a oom_score_adj type tunable that cannot change semantics from kernel version to kernel version and without polluting the mem cgroup filesystem. That, in my suggestion, is an adjustment on the amount of total hierarchical usage of each mem cgroup at each level of the hierarchy. That requires that the heuristic uses hierarchical usage rather than considering each cgroup as independent consumers as it does today. We need to implement that heuristic and introduce userspace influence over oom kill selection now rather than later because its implementation changes how this patchset is implemented.

I can implement these changes, if preferred, on top of the current patchset, but I do not believe we want inconsistencies between kernel versions that introduce user visible changes for the sole reason that this current implementation is incomplete and unfair. We can implement and introduce it once without behavior changing later because the core heuristic has necessarily changed.

Quoted 4 lines

Quoted 2 lines

David, I _had_ hierarchical accounting implemented in one of the previous versions of this patchset. And there were _reasons_, why we went away from it.

Can you please summarize those issues for my understanding?

Because it makes the oom decision directly hardwired to the hierarchy structure. Just take a simple example of the cgroup structure which reflects a higher level organization

         root
	/  |  \ 

admins | teachers
students

Now your students group will be most like the largest one. Why should we kill tasks/cgroups from that cgroup just because it is cumulatively the largest one. It might have been some of the teacher blowing up the memory usage.

Another example is when you need a mid layer cgroups for other controllers to better control resources.

	root
	/  \
   cpuset1  cpuset2
   /    \   /  |  \
  A     B  C   D   E

You really do not want to select cpuset2 just because it has more subgroups and potentially larger cumulative usage. The hierarchical accounting works _only_ if higher level cgroups are semantically _comparable_ which might be true for some workloads but by no means this is true in general.

That all being said, I can see further improvements to happen on top of the current work but I also think that the current implementation works for the usecase which many users can use without those improvements.

Summarizing all this, following the hierarchy is good when it reflects the "importance" of cgroup's memory for a user, and bad otherwise. In generic case with unified hierarchy it's not true, so following the hierarchy unconditionally is bad.

Current version of the patchset allows common evaluation of cgroups by setting memory.groupoom to true. The only limitation is that it also changes the OOM action: all belonging processes will be killed. If we really want to preserve an option to evaluate cgroups together without forcing "kill all processes" action, we can convert memory.groupoom from being boolean to the multi-value knob. For instance, "disabled" and "killall". This will allow to add a third state ("evaluate", for example) later without breaking the API.

Quoted 5 lines · Roman Gushchin

Summarizing all this, following the hierarchy is good when it reflects the "importance" of cgroup's memory for a user, and bad otherwise. In generic case with unified hierarchy it's not true, so following the hierarchy unconditionally is bad.

Quoted 9 lines

Current version of the patchset allows common evaluation of cgroups by setting memory.groupoom to true. The only limitation is that it also changes the OOM action: all belonging processes will be killed. If we really want to preserve an option to evaluate cgroups together without forcing "kill all processes" action, we can convert memory.groupoom from being boolean to the multi-value knob. For instance, "disabled" and "killall". This will allow to add a third state ("evaluate", for example) later without breaking the API.

No, this isn't how kernel features get introduced. We don't design a new kernel feature with its own API for a highly specialized usecase and then claim we'll fix the problems later. Users will work around the constraints of the new feature, if possible, and then we can end up breaking them later. Or, we can pollute the mem cgroup v2 filesystem with even more tunables to cover up for mistakes in earlier designs.

The key point to all three of my objections: extensibility.

Both you and Michal have acknowledged blantently obvious shortcomings in the design.

Quoted 6 lines

No, this isn't how kernel features get introduced. We don't design a new kernel feature with its own API for a highly specialized usecase and then claim we'll fix the problems later. Users will work around the constraints of the new feature, if possible, and then we can end up breaking them later. Or, we can pollute the mem cgroup v2 filesystem with even more tunables to cover up for mistakes in earlier designs.

This is a blatant misinterpretation of the proposed changes. I haven't heard _any_ single argument against the proposed user interface except for complaints for missing tunables. This is not how the kernel development works and should work. The usecase was clearly described and far from limited to a single workload or company.

Quoted 1 line

The key point to all three of my objections: extensibility.

And it has been argued that further _features_ can be added on top. I am absolutely fed up discussing those things again and again without any progress. You simply keep _ignoring_ counter arguments and that is a major PITA to be honest with you. You are basically blocking a useful feature because it doesn't solve your particular workload. This is simply not acceptable in the kernel development.

Quoted 2 lines

Both you and Michal have acknowledged blantently obvious shortcomings in the design.

What you call blatant shortcomings we do not see affecting any _existing_ workloads. If they turn out to be real issues then we can fix them without deprecating any user APIs added by this patchset.

Quoted 7 lines · Michal Hocko

Quoted 6 lines

No, this isn't how kernel features get introduced. We don't design a new kernel feature with its own API for a highly specialized usecase and then claim we'll fix the problems later. Users will work around the constraints of the new feature, if possible, and then we can end up breaking them later. Or, we can pollute the mem cgroup v2 filesystem with even more tunables to cover up for mistakes in earlier designs.

Quoted 6 lines

This is a blatant misinterpretation of the proposed changes. I haven't heard _any_ single argument against the proposed user interface except for complaints for missing tunables. This is not how the kernel development works and should work. The usecase was clearly described and far from limited to a single workload or company.

The complaint about the user interface is that it is not extensible, as my next line states.

This doesn't need to be opted into with a mount option locking the entire system into a single oom policy. That, itself, is the result of a poor design. What is needed is a way for users to define an oom policy that is generally useful, not something that is locked in for the whole system.

We don't need several different cgroup mount options only for mem cgroup oom policies.

We also don't need random
memory.groupoom files being added to the mem cgroup v2 filesystem only for one or two particular policies and being no-ops otherwise.

It can easily
be specified as part of the policy itself.

Quoted 10 lines

Quoted 7 lines

I find this problem quite minor, because I haven't seen any practical problems caused by accounting of the root cgroup memory.
If it's a serious problem for you, it can be solved without switching to the hierarchical accounting: it's possible to sum up all leaf cgroup stats and substract them from global values. So, it can be a relatively small enhancement on top of the current mm tree. This has nothing to do with global victim selection approach.

It sounds like a significant shortcoming to me - the oom-killing decisions which David describes are clearly incorrect?

Well, I would rather look at that from the use case POV. The primary user of the new OOM killer functionality are containers. I might be wrong but I _assume_ that root cgroup will only contain the basic system infrastructure there and all the workload will run containers (aka cgroups). The only oom tuning inside the root cgroup would be to disable oom for some of those processes. The current implementation should work reasonably well for that configuration.

Quoted 7 lines

Quoted 4 lines

We've discussed this a lot.
Hierarchical approach has their own issues, which we've discussed during previous iterations of the patchset. If you know how to address them (I've no idea), please, go on and suggest your version.

Well, if a hierarchical approach isn't a workable fix for the problem which David has identified then what *is* the fix?

Hierarchical approach basically hardcodes the oom decision into the hierarchy structure and that is simply a no go because that would turn into a massive configuration PITA (more on that below). I consider the above example rather artificial to be honest. Anyway, if we _really_ have to address it in the future we can do that by providing a mechanism to prioritize cgroups. It seems that this is required for some usecases anyway.

Quoted 5 lines

Quoted 4 lines

The per-process oom_score_adj interface is not the nicest one, and I'm not sure we want to replicate it on cgroup level as is. If you have an idea of how it should look like, please, propose a patch; otherwise it's hard to discuss it without the code.

Quoted 2 lines

It does make sense to have some form of per-cgroup tunability. Why is the oom_score_adj approach inappropriate and what would be better?

oom_score_adj is basically unusable for any fine tuning on the process level for most setups except for very specialized ones. The only reasonable usage I've seen so far was to disable OOM killer for a process or make it a prime candidate. Using the same limited concept for cgroups sounds like repeating the same error to me.

Quoted 1 line

How hard is it to graft such a thing onto the -mm patchset?

I think this should be thought through very well before we add another tuning. Moreover the current usecase doesn't seem to require it so I am not really sure why we should implement something right away and later suffer from API mistakes.

Quoted 4 lines

Quoted 2 lines

David, I _had_ hierarchical accounting implemented in one of the previous versions of this patchset. And there were _reasons_, why we went away from it.

Can you please summarize those issues for my understanding?

Because it makes the oom decision directly hardwired to the hierarchy structure. Just take a simple example of the cgroup structure which reflects a higher level organization

         root
	/  |  \ 

admins | teachers
students

Now your students group will be most like the largest one. Why should we kill tasks/cgroups from that cgroup just because it is cumulatively the largest one. It might have been some of the teacher blowing up the memory usage.

Quoted 61 lines · On Wed, 10 Jan 2018 05:11:44 -0800 Roman Gushchin <[email protected]> wrote:

Quoted 56 lines · David Rientjes

Quoted 9 lines · Andrew Morton

Quoted 1 line

This patchset makes the OOM killer cgroup-aware.

Thanks, I'll grab these.

There has been controversy over this patchset, to say the least. I can't say that I followed it closely! Could those who still have reservations please summarise their concerns and hopefully suggest a way forward?

Yes, I'll summarize what my concerns have been in the past and what they are wrt the patchset as it stands in -mm. None of them originate from my current usecase or anticipated future usecase of the oom killer for system-wide or memcg-constrained oom conditions. They are based purely on the patchset's use of an incomplete and unfair heuristic for deciding which cgroup to target.

I'll also suggest simple changes to the patchset, which I have in the past, that can be made to address all of these concerns.

1. The unfair comparison of the root mem cgroup vs leaf mem cgroups

The patchset uses two different heuristics to compare root and leaf mem cgroups and scores them based on number of pages. For the root mem cgroup, it totals the /proc/pid/oom_score of all processes attached:
that's based on rss, swap, pgtables, and, most importantly, oom_score_adj. For leaf mem cgroups, it's based on that memcg's anonymous, unevictable, unreclaimable slab, kernel stack, and swap counters. These can be wildly different independent of /proc/pid/oom_score_adj, but the most obvious unfairness comes from users who tune oom_score_adj.

An example: start a process that faults 1GB of anonymous memory and leave it attached to the root mem cgroup. Start six more processes that each fault 1GB of anonymous memory and attached them to a leaf mem cgroup. Set all processes to have /proc/pid/oom_score_adj of 1000. System oom kill will always kill the 1GB process attached to the root mem cgroup. It's because oom_badness() relies on /proc/pid/oom_score_adj, which is used to evaluate the root mem cgroup, and leaf mem cgroups completely disregard it.

In this example, the leaf mem cgroup's score is 1,573,044, the number of pages for the 6GB of faulted memory. The root mem cgroup's score is 12,652,907, eight times larger even though its usage is six times smaller.

This is caused by the patchset disregarding oom_score_adj entirely for leaf mem cgroups and relying on it heavily for the root mem cgroup. It's the complete opposite result of what the cgroup aware oom killer advertises.

It also works the other way, if a large memory hog is attached to the root mem cgroup but has a negative oom_score_adj it is never killed and random processes are nuked solely because they happened to be attached to a leaf mem cgroup. This behavior wrt oom_score_adj is completely undocumented, so I can't presume that it is either known nor tested.

Quoted 3 lines

Solution: compare the root mem cgroup and leaf mem cgroups equally with the same criteria by doing hierarchical accounting of usage and subtracting from total system usage to find root usage.

Quoted 7 lines

I find this problem quite minor, because I haven't seen any practical problems caused by accounting of the root cgroup memory.
If it's a serious problem for you, it can be solved without switching to the hierarchical accounting: it's possible to sum up all leaf cgroup stats and substract them from global values. So, it can be a relatively small enhancement on top of the current mm tree. This has nothing to do with global victim selection approach.

It sounds like a significant shortcoming to me - the oom-killing decisions which David describes are clearly incorrect?

If this can be fixed against the -mm patchset with a "relatively small enhancement" then please let's get that done so it can be reviewed and tested.

Quoted 40 lines

Quoted 39 lines

2. Evading the oom killer by attaching processes to child cgroups

Any cgroup on the system can attach all their processes to individual child cgroups. This is functionally the same as doing

	for i in $(cat cgroup.procs); do mkdir $i; echo $i > $i/cgroup.procs; done

without the no internal process constraint introduced with cgroup v2. All child cgroups are evaluated based on their own usage: all anon, unevictable, and unreclaimable slab as described previously. It requires an individual cgroup to be the single largest consumer to be targeted by the oom killer.

An example: allow users to manage two different mem cgroup hierarchies limited to 100GB each. User A uses 10GB of memory and user B uses 90GB of memory in their respective hierarchies. On a system oom condition, we'd expect at least one process from user B's hierarchy would always be oom killed with the cgroup aware oom killer. In fact, the changelog explicitly states it solves an issue where "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."

The opposite becomes true, however, if user B creates child cgroups and distributes its processes such that each child cgroup's usage never exceeds 10GB of memory. This can either be done intentionally to purposefully have a low cgroup memory footprint to evade the oom killer or unintentionally with cgroup v2 to allow those individual processes to be constrained by other cgroups in a single hierarchy model. User A, using 10% of his memory limit, is always oom killed instead of user B, using 90% of his memory limit.

Others have commented its still possible to do this with a per-process model if users split their processes into many subprocesses with small memory footprints.

Solution: comparing cgroups must be done hierarchically. Neither user A nor user B can evade the oom killer because targeting is done based on the total hierarchical usage rather than individual cgroups in their hierarchies.

Quoted 4 lines

We've discussed this a lot.
Hierarchical approach has their own issues, which we've discussed during previous iterations of the patchset. If you know how to address them (I've no idea), please, go on and suggest your version.

Well, if a hierarchical approach isn't a workable fix for the problem which David has identified then what *is* the fix?

Quoted 36 lines

Quoted 35 lines

3. Userspace has zero control over oom kill selection in leaf mem cgroups

Unlike using /proc/pid/oom_score_adj to bias or prefer certain processes from the oom killer, the cgroup aware oom killer does not provide any solution for the user to protect leaf mem cgroups. This is a result of leaf mem cgroups being evaluated based on their anon, unevictable, and unreclaimable slab usage and disregarding any user tunable.

Absent the cgroup aware oom killer, users have the ability to strongly prefer a process is oom killed (/proc/pid/oom_score_adj = 1000) or strongly bias against a process (/proc/pid/oom_score_adj = -999).

An example: a process knows its going to use a lot of memory, so it sets /proc/self/oom_score_adj to 1000. It wants to be killed first to avoid distrupting any other process. If it's attached to the root mem cgroup, it will be oom killed. If it's attached to a leaf mem cgroup by an admin outside its control, it will never be oom killed unless that cgroup's usage is the largest single cgroup usage on the system. The reverse also is true for processes that the admin does not want to be oom killed: set /proc/pid/oom_score_adj to -999, but it will *always* be oom killed if its cgroup has the highest usage on the system.

The result is that both admins and users have lost all control over which processes are oom killed. They are left with only one alternative: set /proc/pid/oom_score_adj to -1000 to completely disable a process from oom kill. It doesn't address the issue at all for memcg-constrained oom conditions since no processes are killable anymore, and risks panicking the system if it is the only process left on the system. A process preferring that it is first in line for oom kill simply cannot volunteer anymore.

Solution: allow users and admins to control oom kill selection by introducing a memory.oom_score_adj to affect the oom score of that mem cgroup, exactly the same as /proc/pid/oom_score_adj affects the oom score of a process.

Quoted 4 lines

The per-process oom_score_adj interface is not the nicest one, and I'm not sure we want to replicate it on cgroup level as is. If you have an idea of how it should look like, please, propose a patch; otherwise it's hard to discuss it without the code.

It does make sense to have some form of per-cgroup tunability. Why is the oom_score_adj approach inappropriate and what would be better? How hard is it to graft such a thing onto the -mm patchset?

Quoted 7 lines

Quoted 7 lines

I proposed a solution in
https://marc.info/?l=linux-kernel&m=150956897302725, which was never responded to, for all of these issues. The idea is to do hierarchical accounting of mem cgroup hierarchies so that the hierarchy is traversed comparing total usage at each level to select target cgroups. Admins and users can use memory.oom_score_adj to influence that decisionmaking at each level.

Quoted 40 lines

Quoted 39 lines

This solves #1 because mem cgroups can be compared based on the same classes of memory and the root mem cgroup's usage can be fairly compared by subtracting top-level mem cgroup usage from system usage. All of the criteria used to evaluate a leaf mem cgroup has a reasonable system-wide counterpart that can be used to do the simple subtraction.

This solves #2 because evaluation is done hierarchically so that distributing processes over a set of child cgroups either intentionally or unintentionally no longer evades the oom killer. Total usage is always accounted to the parent and there is no escaping this criteria for users.

This solves #3 because it allows admins to protect important processes in cgroups that are supposed to use, for example, 75% of system memory without it unconditionally being selected for oom kill but still oom kill if it exceeds a certain threshold. In this sense, the cgroup aware oom killer, as currently implemented, is selling mem cgroups short by requiring the user to accept that the important process will be oom killed iff it uses mem cgroups and isn't attached to root. It also allows users to actually volunteer to be oom killed first without majority usage.

It has come up time and time again that this support can be introduced on top of the cgroup oom killer as implemented. It simply cannot. For admins and users to have control over decisionmaking, it needs a oom_score_adj type tunable that cannot change semantics from kernel version to kernel version and without polluting the mem cgroup filesystem. That, in my suggestion, is an adjustment on the amount of total hierarchical usage of each mem cgroup at each level of the hierarchy. That requires that the heuristic uses hierarchical usage rather than considering each cgroup as independent consumers as it does today. We need to implement that heuristic and introduce userspace influence over oom kill selection now rather than later because its implementation changes how this patchset is implemented.

I can implement these changes, if preferred, on top of the current patchset, but I do not believe we want inconsistencies between kernel versions that introduce user visible changes for the sole reason that this current implementation is incomplete and unfair. We can implement and introduce it once without behavior changing later because the core heuristic has necessarily changed.

Quoted 2 lines

David, I _had_ hierarchical accounting implemented in one of the previous versions of this patchset. And there were _reasons_, why we went away from it.

Can you please summarize those issues for my understanding?

You don't have any control and no accounting of the stuff situated inside the root cgroup, so it doesn't make sense to leave anything in there while also using sophisticated containerization mechanisms like this group oom setting.

In fact, the laptop I'm writing this email on runs an unmodified mainstream Linux distribution. The only thing in the root cgroup are kernel threads.

The decisions are good enough for the rare cases you forget something in there and it explodes.

This assumes you even need one. Right now, the OOM killer picks the biggest MM, so you can evade selection by forking your MM. This patch allows picking the biggest cgroup, so you can evade by forking groups.

It's not a new vector, and clearly nobody cares. This has never been brought up against the current design that I know of.

Note, however, that there actually *is* a way to guard against it: in cgroup2 there is a hierarchical limit you can configure for the number of cgroups that are allowed to be created in the subtree. See 1a926e0bbab8 ("cgroup: implement hierarchy limits").

It could be useful, but we have no concensus on the desired semantics. And it's not clear why we couldn't add it later as long as the default settings of a new knob maintain the default behavior (which would have to be preserved anyway, since we rely on it).

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

I proposed a solution in
https://marc.info/?l=linux-kernel&m=150956897302725, which was never responded to, for all of these issues. The idea is to do hierarchical accounting of mem cgroup hierarchies so that the hierarchy is traversed comparing total usage at each level to select target cgroups. Admins and users can use memory.oom_score_adj to influence that decisionmaking at each level.

We did respond repeatedly: this doesn't work for a lot of setups.

Quoted 9 lines · Johannes Weiner

You don't have any control and no accounting of the stuff situated inside the root cgroup, so it doesn't make sense to leave anything in there while also using sophisticated containerization mechanisms like this group oom setting.

In fact, the laptop I'm writing this email on runs an unmodified mainstream Linux distribution. The only thing in the root cgroup are kernel threads.

Quoted 2 lines

The decisions are good enough for the rare cases you forget something in there and it explodes.

It's quite trivial to allow the root mem cgroup to be compared exactly the same as another cgroup. Please see
https://marc.info/?l=linux-kernel&m=151579459920305.

Quoted 3 lines

This assumes you even need one. Right now, the OOM killer picks the biggest MM, so you can evade selection by forking your MM. This patch allows picking the biggest cgroup, so you can evade by forking groups.

It's quite trivial to prevent any cgroup from evading the oom killer by either forking their mm or attaching all their processes to subcontainers. Please see https://marc.info/?l=linux-kernel&m=151579459920305.

Quoted 2 lines

It's not a new vector, and clearly nobody cares. This has never been brought up against the current design that I know of.

As cgroup v2 becomes more popular, people will organize their cgroup hierarchies for all controllers they need to use. We do this today, for example, by attaching some individual consumers to child mem cgroups purely for the rich statistics and vmscan stats that mem cgroup provides without any limitation on those cgroups.

Quoted 4 lines

Note, however, that there actually *is* a way to guard against it: in cgroup2 there is a hierarchical limit you can configure for the number of cgroups that are allowed to be created in the subtree. See 1a926e0bbab8 ("cgroup: implement hierarchy limits").

Not allowing the user to create subcontainers to track statistics to paper over an obvious and acknowledged shortcoming in the design of the cgroup aware oom killer seems like a pretty nasty shortcoming itself.

Quoted 8 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

I proposed a solution in
https://marc.info/?l=linux-kernel&m=150956897302725, which was never responded to, for all of these issues. The idea is to do hierarchical accounting of mem cgroup hierarchies so that the hierarchy is traversed comparing total usage at each level to select target cgroups. Admins and users can use memory.oom_score_adj to influence that decisionmaking at each level.

Quoted 1 line

We did respond repeatedly: this doesn't work for a lot of setups.

We need to move this discussion to the active proposal at
https://marc.info/?l=linux-kernel&m=151579459920305, because it does address your setup, so it's not good use of anyones time to further discuss simply memory.oom_score_adj.

Quoted 3 lines

It's quite trivial to allow the root mem cgroup to be compared exactly the same as another cgroup. Please see
https://marc.info/?l=linux-kernel&m=151579459920305.

This only says "that will be fixed" and doesn't address why I care.

Quoted 7 lines

Quoted 3 lines

This assumes you even need one. Right now, the OOM killer picks the biggest MM, so you can evade selection by forking your MM. This patch allows picking the biggest cgroup, so you can evade by forking groups.

It's quite trivial to prevent any cgroup from evading the oom killer by either forking their mm or attaching all their processes to subcontainers. Please see https://marc.info/?l=linux-kernel&m=151579459920305.

This doesn't address anything I wrote.

Quoted 8 lines

Quoted 2 lines

It's not a new vector, and clearly nobody cares. This has never been brought up against the current design that I know of.

As cgroup v2 becomes more popular, people will organize their cgroup hierarchies for all controllers they need to use. We do this today, for example, by attaching some individual consumers to child mem cgroups purely for the rich statistics and vmscan stats that mem cgroup provides without any limitation on those cgroups.

There is no connecting tissue between what I wrote and what you wrote.

We have a patch set that was developed, iterated and improved over 10+ revisions, based on evaluating and weighing trade-offs. It's reached a state where the memcg maintainers are happy with it and don't have any concern about future extendabilty to cover more specialized setups.

You've had nine months to contribute and shape this patch series productively, and instead resorted to a cavalcade of polemics, evasion, and repetition of truisms and refuted points. A ten paragraph proposal of vague ideas at this point is simply not a valid argument.

You can send patches to replace or improve on Roman's code and make the case for them.

Quoted 35 lines

1. The unfair comparison of the root mem cgroup vs leaf mem cgroups

The patchset uses two different heuristics to compare root and leaf mem cgroups and scores them based on number of pages. For the root mem cgroup, it totals the /proc/pid/oom_score of all processes attached:
that's based on rss, swap, pgtables, and, most importantly, oom_score_adj. For leaf mem cgroups, it's based on that memcg's anonymous, unevictable, unreclaimable slab, kernel stack, and swap counters. These can be wildly different independent of /proc/pid/oom_score_adj, but the most obvious unfairness comes from users who tune oom_score_adj.

An example: start a process that faults 1GB of anonymous memory and leave it attached to the root mem cgroup. Start six more processes that each fault 1GB of anonymous memory and attached them to a leaf mem cgroup. Set all processes to have /proc/pid/oom_score_adj of 1000. System oom kill will always kill the 1GB process attached to the root mem cgroup. It's because oom_badness() relies on /proc/pid/oom_score_adj, which is used to evaluate the root mem cgroup, and leaf mem cgroups completely disregard it.

In this example, the leaf mem cgroup's score is 1,573,044, the number of pages for the 6GB of faulted memory. The root mem cgroup's score is 12,652,907, eight times larger even though its usage is six times smaller.

This is caused by the patchset disregarding oom_score_adj entirely for leaf mem cgroups and relying on it heavily for the root mem cgroup. It's the complete opposite result of what the cgroup aware oom killer advertises.

It also works the other way, if a large memory hog is attached to the root mem cgroup but has a negative oom_score_adj it is never killed and random processes are nuked solely because they happened to be attached to a leaf mem cgroup. This behavior wrt oom_score_adj is completely undocumented, so I can't presume that it is either known nor tested.

Quoted 3 lines

Solution: compare the root mem cgroup and leaf mem cgroups equally with the same criteria by doing hierarchical accounting of usage and subtracting from total system usage to find root usage.

I find this problem quite minor, because I haven't seen any practical problems caused by accounting of the root cgroup memory.
If it's a serious problem for you, it can be solved without switching to the hierarchical accounting: it's possible to sum up all leaf cgroup stats and substract them from global values. So, it can be a relatively small enhancement on top of the current mm tree. This has nothing to do with global victim selection approach.

Quoted 31 lines

2. Evading the oom killer by attaching processes to child cgroups

Any cgroup on the system can attach all their processes to individual child cgroups. This is functionally the same as doing

	for i in $(cat cgroup.procs); do mkdir $i; echo $i > $i/cgroup.procs; done

without the no internal process constraint introduced with cgroup v2. All child cgroups are evaluated based on their own usage: all anon, unevictable, and unreclaimable slab as described previously. It requires an individual cgroup to be the single largest consumer to be targeted by the oom killer.

An example: allow users to manage two different mem cgroup hierarchies limited to 100GB each. User A uses 10GB of memory and user B uses 90GB of memory in their respective hierarchies. On a system oom condition, we'd expect at least one process from user B's hierarchy would always be oom killed with the cgroup aware oom killer. In fact, the changelog explicitly states it solves an issue where "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."

The opposite becomes true, however, if user B creates child cgroups and distributes its processes such that each child cgroup's usage never exceeds 10GB of memory. This can either be done intentionally to purposefully have a low cgroup memory footprint to evade the oom killer or unintentionally with cgroup v2 to allow those individual processes to be constrained by other cgroups in a single hierarchy model. User A, using 10% of his memory limit, is always oom killed instead of user B, using 90% of his memory limit.

Quoted 8 lines

Others have commented its still possible to do this with a per-process model if users split their processes into many subprocesses with small memory footprints.

Solution: comparing cgroups must be done hierarchically. Neither user A nor user B can evade the oom killer because targeting is done based on the total hierarchical usage rather than individual cgroups in their hierarchies.

We've discussed this a lot.
Hierarchical approach has their own issues, which we've discussed during previous iterations of the patchset. If you know how to address them (I've no idea), please, go on and suggest your version.

Quoted 31 lines

3. Userspace has zero control over oom kill selection in leaf mem cgroups

Unlike using /proc/pid/oom_score_adj to bias or prefer certain processes from the oom killer, the cgroup aware oom killer does not provide any solution for the user to protect leaf mem cgroups. This is a result of leaf mem cgroups being evaluated based on their anon, unevictable, and unreclaimable slab usage and disregarding any user tunable.

Absent the cgroup aware oom killer, users have the ability to strongly prefer a process is oom killed (/proc/pid/oom_score_adj = 1000) or strongly bias against a process (/proc/pid/oom_score_adj = -999).

An example: a process knows its going to use a lot of memory, so it sets /proc/self/oom_score_adj to 1000. It wants to be killed first to avoid distrupting any other process. If it's attached to the root mem cgroup, it will be oom killed. If it's attached to a leaf mem cgroup by an admin outside its control, it will never be oom killed unless that cgroup's usage is the largest single cgroup usage on the system. The reverse also is true for processes that the admin does not want to be oom killed: set /proc/pid/oom_score_adj to -999, but it will *always* be oom killed if its cgroup has the highest usage on the system.

The result is that both admins and users have lost all control over which processes are oom killed. They are left with only one alternative: set /proc/pid/oom_score_adj to -1000 to completely disable a process from oom kill. It doesn't address the issue at all for memcg-constrained oom conditions since no processes are killable anymore, and risks panicking the system if it is the only process left on the system. A process preferring that it is first in line for oom kill simply cannot volunteer anymore.

Quoted 4 lines

Solution: allow users and admins to control oom kill selection by introducing a memory.oom_score_adj to affect the oom score of that mem cgroup, exactly the same as /proc/pid/oom_score_adj affects the oom score of a process.

The per-process oom_score_adj interface is not the nicest one, and I'm not sure we want to replicate it on cgroup level as is. If you have an idea of how it should look like, please, propose a patch; otherwise it's hard to discuss it without the code.

Quoted 13 lines

It has come up time and time again that this support can be introduced on top of the cgroup oom killer as implemented. It simply cannot. For admins and users to have control over decisionmaking, it needs a oom_score_adj type tunable that cannot change semantics from kernel version to kernel version and without polluting the mem cgroup filesystem. That, in my suggestion, is an adjustment on the amount of total hierarchical usage of each mem cgroup at each level of the hierarchy. That requires that the heuristic uses hierarchical usage rather than considering each cgroup as independent consumers as it does today. We need to implement that heuristic and introduce userspace influence over oom kill selection now rather than later because its implementation changes how this patchset is implemented.

Quoted 6 lines

I can implement these changes, if preferred, on top of the current patchset, but I do not believe we want inconsistencies between kernel versions that introduce user visible changes for the sole reason that this current implementation is incomplete and unfair. We can implement and introduce it once without behavior changing later because the core heuristic has necessarily changed.

David, I _had_ hierarchical accounting implemented in one of the previous versions of this patchset. And there were _reasons_, why we went away from it. You can't just ignore them and say that "there is a simple solution, which Roman is not responding". If you know how to address these issues and convince everybody that hierarchical approach is a way to go, please, go on and send your version of the patchset.

Recently added alloc_pages_before_oomkill gained new caller with this patchset and I think it just grown to deserve a simpler code flow. What do you think about this on top of the series?

Recently added alloc_pages_before_oomkill gained new caller with this patchset and I think it just grown to deserve a simpler code flow. What do you think about this on top of the series?

---
From f1f6035ea0df65e7619860b013f2fabdda65233e Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko-IBi9RG/[email protected]>
Date: Fri, 1 Dec 2017 10:05:25 +0100
Subject: [PATCH] mm, oom: simplify alloc_pages_before_oomkill handling

alloc_pages_before_oomkill is the last attempt to allocate memory before we go and try to kill a process or a memcg. It's success path always has to properly clean up the oc state (namely victim reference count). Let's pull this into alloc_pages_before_oomkill directly rather than risk somebody will forget to do it in future. Also document that we _know_ alloc_pages_before_oomkill violates proper layering and that is a pragmatic decision.

Signed-off-by Michal Hocko <mhocko-IBi9RG/[email protected]>

---
 include/linux/oom.h |  2 +-
 mm/oom_kill.c       | 21 +++------------------
 mm/page_alloc.c     | 24 ++++++++++++++++++++++--
 3 files changed, 26 insertions(+), 21 deletions(-)

include/linux/oom.h +1 -1

index 10f495c8454d..7052e0a20e13 100644
@@ -121,7 +121,7 @@ extern void oom_killer_enable(void);
 
 extern struct task_struct *find_lock_task_mm(struct task_struct *p);
 
-extern struct page *alloc_pages_before_oomkill(const struct oom_control *oc);
+extern bool alloc_pages_before_oomkill(struct oom_control *oc);
 
 extern int oom_evaluate_task(struct task_struct *task, void *arg);
 

mm/oom_kill.c +1 -2

index 4678468bae17..5c2cd299757b 100644
@@ -1102,8 +1102,7 @@ bool out_of_memory(struct oom_control *oc)
 	if (!is_memcg_oom(oc) && sysctl_oom_kill_allocating_task &&
 	    current->mm && !oom_unkillable_task(current, NULL, oc->nodemask) &&
 	    current->signal->oom_score_adj != OOM_SCORE_ADJ_MIN) {
-		oc->page = alloc_pages_before_oomkill(oc);
-		if (oc->page)
+		if (alloc_pages_before_oomkill(oc))
 			return true;
 		get_task_struct(current);
 		oc->chosen_task = current;

Hi, Michal!

I totally agree that out_of_memory() function deserves some refactoring.

But I think there is an issue with your patch (see below):

Quoted 5 lines · Michal Hocko

Recently added alloc_pages_before_oomkill gained new caller with this patchset and I think it just grown to deserve a simpler code flow. What do you think about this on top of the series?

---

mm/oom_kill.c bool out_of_memory(struct oom_control *oc) · line 1119

Quoted 9 lines

mm/oom_kill.c +0 -5

@@ -1112,13 +1111,8 @@ bool out_of_memory(struct oom_control *oc)
 	}
 
 	if (mem_cgroup_select_oom_victim(oc)) {
-		oc->page = alloc_pages_before_oomkill(oc);
-		if (oc->page) {
-			if (oc->chosen_memcg &&
-			    oc->chosen_memcg != INFLIGHT_VICTIM)
-				mem_cgroup_put(oc->chosen_memcg);

You're removing chosen_memcg releasing here, but I don't see where you do this instead. And I'm not sure that putting mem_cgroup_put() into alloc_pages_before_oomkill() is a way towards simpler code.

I was thinking about a bit larger refactoring: splitting out_of_memory() into the following parts (defined as separate functions): victim selection (per-process, memcg-aware or just allocating task), last allocation attempt, OOM action (kill process, kill memcg, panic). Hopefully it can simplify the things, but I don't have code yet.

It seems that this is still in limbo mostly because of David's concerns. So let me reiterate them and provide my POV once more (and the last time) just to help Andrew make a decision:

1) comparision root with tail memcgs during the OOM killer is not fair because we are comparing tasks with memcgs.

This is true, but I do not think this matters much for workloads which are going to use the feature. Why? Because the main consumers of the new feature seem to be containers which really need some fairness when comparing _workloads_ rather than processes. Those are unlikely to contain any significant memory consumers in the root memcg. That would be mostly common infrastructure.

Is this is fixable? Yes, we would need to account in the root memcgs. Why are we not doing that now? Because it has some negligible performance overhead. Are there other ways? Yes we can approximate root memcg memory consumption but I would rather wait for somebody seeing that as a real problem rather than add hacks now without a strong reason.

2) Evading the oom killer by attaching processes to child cgroups which basically means that a task can split up the workload into smaller memcgs to hide their real memory consumption.

Again true but not really anything new. Processes can already fork and split up the memory consumption. Moreover it doesn't even require any special privileges to do so unlike creating a sub memcg. Is this fixable? Yes, untrusted workloads can setup group oom evaluation at the delegation layer so all subgroups would be considered together.

3) Userspace has zero control over oom kill selection in leaf mem cgroups.

Again true but this is something that needs a good evaluation to not end up in the fiasko we have seen with oom_score*. Current users demanding this feature can live without any prioritization so blocking the whole feature seems unreasonable.

4) Future extensibility to be backward compatible.

David is wrong here IMHO. Any prioritization or oom selection policy controls added in future are orthogonal to the oom_group concept added by this patchset. Allowing memcg to be an oom entity is something that we really want longterm. Global CGRP_GROUP_OOM is the most restrictive semantic and softening it will be possible by a adding a new knob to tell whether a memcg/hierarchy is a workload or a set of tasks.

Signature · 2 lines

Michal Hocko
SUSE Labs

1) comparision root with tail memcgs during the OOM killer is not fair because we are comparing tasks with memcgs.

This is true, but I do not think this matters much for workloads which are going to use the feature. Why? Because the main consumers of the new feature seem to be containers which really need some fairness when comparing _workloads_ rather than processes. Those are unlikely to contain any significant memory consumers in the root memcg. That would be mostly common infrastructure.

Is this is fixable? Yes, we would need to account in the root memcgs. Why are we not doing that now? Because it has some negligible performance overhead. Are there other ways? Yes we can approximate root memcg memory consumption but I would rather wait for somebody seeing that as a real problem rather than add hacks now without a strong reason.

2) Evading the oom killer by attaching processes to child cgroups which basically means that a task can split up the workload into smaller memcgs to hide their real memory consumption.

Again true but not really anything new. Processes can already fork and split up the memory consumption. Moreover it doesn't even require any special privileges to do so unlike creating a sub memcg. Is this fixable? Yes, untrusted workloads can setup group oom evaluation at the delegation layer so all subgroups would be considered together.

3) Userspace has zero control over oom kill selection in leaf mem cgroups.

Again true but this is something that needs a good evaluation to not end up in the fiasko we have seen with oom_score*. Current users demanding this feature can live without any prioritization so blocking the whole feature seems unreasonable.

4) Future extensibility to be backward compatible.

David is wrong here IMHO. Any prioritization or oom selection policy controls added in future are orthogonal to the oom_group concept added by this patchset. Allowing memcg to be an oom entity is something that we really want longterm. Global CGRP_GROUP_OOM is the most restrictive semantic and softening it will be possible by a adding a new knob to tell whether a memcg/hierarchy is a workload or a set of tasks.

I'm not trying to preclude the cgroup-aware oom killer from being merged, I'm the only person actively trying to get it merged.

Quoted 2 lines · David Rientjes

I'm not trying to preclude the cgroup-aware oom killer from being merged, I'm the only person actively trying to get it merged.

Before merging the cgroup-aware oom killer, can we merge OOM lockup fixes and my cleanup? The gap between linux.git and linux-next.git keeps us unable to use agreed baseline.

No response from Roman and David...

Andrew, will you once drop Roman's cgroup-aware OOM killer and David's patches? Roman's series has a bug which I mentioned and which can be avoided by my patch. David's patch is using MMF_UNSTABLE incorrectly such that it might start selecting next OOM victim without trying to reclaim any memory.

Since they are not responding to my mail, I suggest once dropping from linux-next.

Quoted 8 lines · Tetsuo Handa

No response from Roman and David...

Andrew, will you once drop Roman's cgroup-aware OOM killer and David's patches? Roman's series has a bug which I mentioned and which can be avoided by my patch. David's patch is using MMF_UNSTABLE incorrectly such that it might start selecting next OOM victim without trying to reclaim any memory.

Since they are not responding to my mail, I suggest once dropping from linux-next.

I was in cc, and didn't thought that you're expecting something from me.

I don't get, why it's necessary to drop the cgroup oom killer to merge your fix? I'm happy to help with rebasing and everything else.

Quoted 2 lines

I don't get, why it's necessary to drop the cgroup oom killer to merge your fix? I'm happy to help with rebasing and everything else.

Yes, I wish you rebase your series on top of OOM lockup (CVE-2016-10723) mitigation patch ( https://marc.info/?l=linux-mm&m=153112243424285&w=4 ). It is a trivial change and easy to cleanly backport (if applied before your series).

Also, I expect you to check whether my cleanup patch which removes "abort" path ( [PATCH 1/2] at https://marc.info/?l=linux-mm&m=153119509215026&w=4 ) helps simplifying your series. I don't know detailed behavior of your series, but I assume that your series do not kill threads which current thread should not wait for MMF_OOM_SKIP.

Quoted 7 lines · Tetsuo Handa

Quoted 2 lines

I don't get, why it's necessary to drop the cgroup oom killer to merge your fix? I'm happy to help with rebasing and everything else.

Yes, I wish you rebase your series on top of OOM lockup (CVE-2016-10723) mitigation patch ( https://marc.info/?l=linux-mm&m=153112243424285&w=4 ). It is a trivial change and easy to cleanly backport (if applied before your series).

Quoted 5 lines

Also, I expect you to check whether my cleanup patch which removes "abort" path ( [PATCH 1/2] at https://marc.info/?l=linux-mm&m=153119509215026&w=4 ) helps simplifying your series. I don't know detailed behavior of your series, but I assume that your series do not kill threads which current thread should not wait for MMF_OOM_SKIP.

syzbot is hitting WARN(1) due to mem_cgroup_out_of_memory() == false.
https://urldefense.proofpoint.com/v2/url?u=https-3A__syzkaller.appspot.com_bug-3Fid-3Dea8c7912757d253537375e981b61749b2da69258&d=DwICJg&c=5VD0RTtNlTh3ycd41b3MUw&r=i6WobKxbeG3slzHSIOxTVtYIJw7qjCE6S0spDTKL-J4&m=h9FJRAWtCmDLT-cVwvXKCYIUVRSrD--0XFJE-OnNY64&s=If6eFu6MlYjnfLXeg5_S-3tuhCZhSMv8_qfSrMfwOQ0&e=

I can't tell what change is triggering this race. Maybe removal of oom_lock from the oom reaper made more likely to hit. But anyway I suspect that

static bool oom_kill_memcg_victim(struct oom_control *oc)

{
        if (oc->chosen_memcg == NULL || oc->chosen_memcg == INFLIGHT_VICTIM)
                return oc->chosen_memcg; // <= This line is still broken

because

                /* We have one or more terminating processes at this point. */
                oc->chosen_task = INFLIGHT_VICTIM;

is not called.

Also, that patch is causing confusion by reviving schedule_timeout_killable(1) with oom_lock held.

Can we temporarily drop cgroup-aware OOM killer from linux-next.git and apply my cleanup patch? Since the merge window is approaching, I really want to see how next -rc1 would look like...

Quoted 3 lines · Tetsuo Handa

Can we temporarily drop cgroup-aware OOM killer from linux-next.git and apply my cleanup patch? Since the merge window is approaching, I really want to see how next -rc1 would look like...

Hi Tetsuo!

Has this cleanup patch been acked by somebody?

Which problem does it solve?

Dropping patches for making a cleanup (if it's a cleanup) sounds a bit strange.

Anyway, there is a good chance that current cgroup-aware OOM killer implementation will be replaced by a lightweight version (memory.oom.group). Please, take a look at it, probably your cleanup will not conflict with it at all.

Thanks!

Quoted 8 lines · Roman Gushchin

Quoted 3 lines · Tetsuo Handa

Can we temporarily drop cgroup-aware OOM killer from linux-next.git and apply my cleanup patch? Since the merge window is approaching, I really want to see how next -rc1 would look like...

Hi Tetsuo!

Has this cleanup patch been acked by somebody?

Not yet. But since Michal considers this cleanup as "a nice shortcut" ( https://marc.info/[email protected] ), I assume that I will get an ACK regarding this cleanup.

Quoted 1 line

Which problem does it solve?

It simplifies tricky out_of_memory() return value decision, and it also fixes a bug in your series which syzbot is pointing out.

Quoted 1 line

Dropping patches for making a cleanup (if it's a cleanup) sounds a bit strange.

What I need is a git tree which I can use as a baseline for making this cleanup. linux.git is not suitable because it does not include Michal's fix, but linux-next.git is not suitable because Michal's fix is overwritten by your series. I want a git tree which includes Michal's fix and does not include your series.

Quoted 1 line

Quoted 4 lines

Anyway, there is a good chance that current cgroup-aware OOM killer implementation will be replaced by a lightweight version (memory.oom.group). Please, take a look at it, probably your cleanup will not conflict with it at all.

Then, please drop current cgroup-aware OOM killer implementation from linux-next.git . I want to see how next -rc1 would look like (for testing purpose) and want to use linux-next.git as a baseline (for making this cleanup).

Quoted 10 lines · Michal Hocko

1) comparision root with tail memcgs during the OOM killer is not fair because we are comparing tasks with memcgs.

This is true, but I do not think this matters much for workloads which are going to use the feature. Why? Because the main consumers of the new feature seem to be containers which really need some fairness when comparing _workloads_ rather than processes. Those are unlikely to contain any significant memory consumers in the root memcg. That would be mostly common infrastructure.

There are users (us) who want to use the feature and not all processes are attached to leaf mem cgroups. The functionality can be provided in a generally useful way that doesn't require any specific hierarchy, and I implemented this in my patch series at
https://marc.info/?l=linux-mm&m=152175563004458&w=2. That proposal to fix *all* of my concerns with the cgroup-aware oom killer as it sits in -mm, in it's entirety, only extends it so it is generally useful and does not remove any functionality. I'm not sure why we are discussing ways of moving forward when that patchset has been waiting for review for almost four months and, to date, I haven't seen an objection to.

Well, I didn't really get to your patches yet. The last time I've checked I had some pretty serious concerns about the consistency of your proposal. Those might have been fixed in the lastest version of your patchset I haven't seen. But I still strongly suspect that you are largerly underestimating the complexity of more generic oom policies which you are heading to.

Considering user API failures from the past (oom_*adj fiasco for example) suggests that we should start with smaller steps and only provide a clear and simple API. oom_group is such a simple and semantically consistent thing which is the reason I am OK with it much more than your "we can be more generic" approach. I simply do not trust we can agree on sane and consistent api in a reasonable time.

And it is quite mind boggling that a simpler approach has been basically blocked for months because there are some concerns for workloads which are not really asking for the feature. Sure your usecase might need to handle root memcg differently. That is a fair point but that shouldn't really block containers users who can use the proposed solution without any further changes. If we ever decide to handle root memcg differently we are free to do so because the oom selection policy is not carved in stone by any api.