cgroup-aware OOM killer

Implement mem_cgroup_scan_tasks() functionality for the root memory cgroup to use this function for looking for a OOM victim task in the root memory cgroup by the cgroup-ware OOM killer.

The root memory cgroup is treated as a leaf cgroup, so only tasks which are directly belonging to the root cgroup are iterated over.

This patch doesn't introduce any functional change as mem_cgroup_scan_tasks() is never called for the root memcg. This is preparatory work for the cgroup-aware OOM killer, which will use this function to iterate over tasks belonging to the root memcg.

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

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.

Under OOM conditions, it 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 well, 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 leaf memory cgroups.
Due to memcg statistics implementation a special algorithm is used for estimating it's oom_score: we define it as maximum oom_score of the belonging tasks.

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 <penguin-kernel-JPay3/[email protected]>

Cc David Rientjes <[email protected]>

Cc Andrew Morton <akpm-de/[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            | 172 +++++++++++++++++++++++++++++++++++++++++++++
 mm/oom_kill.c              |  70 +++++++++++++-----
 4 files changed, 251 insertions(+), 20 deletions(-)

include/linux/memcontrol.h +17 -0

index 69966c461d1c..75b63b68846e 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 {

@@ -342,6 +343,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)
 

@@ -480,6 +486,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

@@ -744,6 +752,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,

@@ -936,6 +948,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 76aac4ce39bc..ca78e2d5956e 100644
@@ -9,6 +9,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;

@@ -39,7 +46,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;
 };
 

@@ -101,6 +109,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;

mm/memcontrol.c +172 -0

index 41d71f665550..191b70735f1f 100644
@@ -2670,6 +2670,178 @@ 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;
+
+	/*
+	 * We don't have necessary stats for the root memcg,
+	 * so we define it's oom_score as the maximum oom_score
+	 * of the belonging tasks.
+	 *
+	 * As tasks in the root memcg unlikely are parts of a
+	 * single workload, and we don't have to implement
+	 * group killing, this approximation is reasonable.
+	 *
+	 * But if we will have necessary stats for the root memcg,
+	 * we might switch to the approach which is used for all
+	 * other memcgs.
+	 */
+	if (memcg == root_mem_cgroup) {
+		struct css_task_iter it;
+		struct task_struct *task;
+		long score, max_score = 0;
+
+		css_task_iter_start(&memcg->css, 0, &it);
+		while ((task = css_task_iter_next(&it))) {
+			score = oom_badness(task, memcg, nodemask,
+					    totalpages);
+			if (score > max_score)
+				max_score = score;
+		}
+		css_task_iter_end(&it);
+
+		return max_score;
+	}
+
+	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;
+
+	/*
+	 * 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 +51 -19

index ccdb7d34cd13..20e62ec32ba8 100644
@@ -310,7 +310,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;

@@ -344,26 +344,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)
 {

@@ -926,7 +926,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;

@@ -987,6 +987,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.
  */

@@ -1039,6 +1060,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;

@@ -1083,27 +1105,37 @@ bool out_of_memory(struct oom_control *oc)
 	    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;
 		oom_kill_process(oc, "Out of memory (oom_kill_allocating_task)");
 		return true;
 	}
 
+	if (mem_cgroup_select_oom_victim(oc) && oom_kill_memcg_victim(oc)) {
+		delay = true;
+		goto out;
+	}
+
 	select_bad_process(oc);
 	/* 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.13.6

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.

Under OOM conditions, it 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 well, 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 leaf memory cgroups.
Due to memcg statistics implementation a special algorithm is used for estimating it's oom_score: we define it as maximum oom_score of the belonging tasks.

mm/memcontrol.c +53 -0

+/*
+ * 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;
+
+	/*
+	 * 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;
+

mm/memcontrol.c +13 -0

+
+		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.
+		 */

mm/memcontrol.c +28 -0

+	}
+
+	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 +22 -1

index ccdb7d34cd13..20e62ec32ba8 100644
@@ -987,6 +987,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.
  */

@@ -1083,27 +1105,37 @@ bool out_of_memory(struct oom_control *oc)
 	    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;

Quoted 11 lines · Roman Gushchin

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

This behavior doesn't suit well the system with many running containers:

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.

Quoted 6 lines

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.

I'd move the second point to the changelog for the next patch since this patch doesn't implement any support for memory.oom_group.

Quoted 8 lines

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

Under OOM conditions, it 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 well, and also provide an ability to kill all tasks belonging to the victim cgroup.

Quoted 6 lines

The root cgroup is treated as a leaf memory cgroup, so it's score is compared with leaf memory cgroups.
Due to memcg statistics implementation a special algorithm is used for estimating it's oom_score: we define it as maximum oom_score of the belonging tasks.

This seems to unfairly bias the root mem cgroup depending on process size. It isn't treated fairly as a leaf mem cgroup if they are being compared based on different criteria: the root mem cgroup as (mostly) the largest rss of a single process vs leaf mem cgroups as all anon, unevictable, and unreclaimable slab pages charged to it by all processes.

I imagine a configuration where the root mem cgroup has 100 processes attached each with rss of 80MB, compared to a leaf cgroup with 100 processes of 1MB rss each. How does this logic prevent repeatedly oom killing the processes of 1MB rss?

In this case, "the root cgroup is treated as a leaf memory cgroup" isn't quite fair, it can simply hide large processes from being selected. Users who configure cgroups in a unified hierarchy for other resource constraints are penalized for this choice even though the mem cgroup with 100 processes of 1MB rss each may not be limited itself.

I think for this comparison to be fair, it requires accounting for the root mem cgroup itself or for a different accounting methodology for leaf memory cgroups.

Quoted 53 lines

mm/memcontrol.c +53 -0

+/*
+ * 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;
+
+	/*
+	 * 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;
+

mm/memcontrol.c static inline bool memcg_has_children(struct mem_cgroup *memcg) · line 2793

Quoted 10 lines

mm/memcontrol.c +10 -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;

I'll reiterate what I did on the last version of the patchset: considering only leaf memory cgroups easily allows users to defeat this heuristic and bias against all of their memory usage up to the largest process size amongst the set of processes attached. If the user creates N child mem cgroups for their N processes and attaches one process to each child, the _only_ thing this achieved is to defeat your heuristic and prefer other leaf cgroups simply because those other leaf cgroups did not do this.

Effectively:

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

will radically shift the heuristic from a score of all anonymous + unevictable memory for all processes to a score of the largest anonymous + unevictable memory for a single process. There is no downside or ramifaction for the end user in doing this. When comparing cgroups based on usage, it only makes sense to compare the hierarchical usage of that cgroup so that attaching processes to descendants or splitting the implementation of a process into several smaller individual processes does not allow this heuristic to be defeated.

Quoted 21 lines

mm/memcontrol.c +21 -0

+
+		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;

Review

Quoted 2 lines

(quoted diff) +2 -0

+			oc->chosen_memcg = iter;
+		}

I'll reiterate what I did on previous versions of this patchset: this effectively removes all control the user has from influencing oom victim selection. Victim selection is very important, the user must be able to influence that decision to prevent the loss of important work when the system is out of memory.

Quoted 10 lines

mm/oom_kill.c +1 -1

@@ -1083,27 +1105,37 @@ bool out_of_memory(struct oom_control *oc)
 	    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;
 		oom_kill_process(oc, "Out of memory (oom_kill_allocating_task)");
 		return true;
 	}
 

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

Quoted 6 lines

mm/oom_kill.c +5 -0

+	if (mem_cgroup_select_oom_victim(oc) && oom_kill_memcg_victim(oc)) {
+		delay = true;
+		goto out;
+	}
+
 	select_bad_process(oc);

This is racy because mem_cgroup_select_oom_victim() found an eligible oc->chosen_memcg that is not INFLIGHT_VICTIM with at least one eligible process but mem_cgroup_scan_task(oc->chosen_memcg) did not. It means if a process cannot be killed because of oom_unkillable_task(), the only eligible processes moved or exited, or the /proc/pid/oom_score_adj of the eligible processes changed, we end up falling back to the complete tasklist scan.

It would be better for oom_evaluate_memcg() to consider oom_unkillable_task() and also retry in the case where oom_kill_memcg_victim() returns NULL.

Quoted 11 lines · Roman Gushchin

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

This behavior doesn't suit well the system with many running containers:

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.

Quoted 6 lines

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.

I'd move the second point to the changelog for the next patch since this patch doesn't implement any support for memory.oom_group.

Quoted 9 lines

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

Under OOM conditions, it 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 well, and also provide an ability to kill all tasks belonging to the victim cgroup.

Quoted 6 lines

The root cgroup is treated as a leaf memory cgroup, so it's score is compared with leaf memory cgroups.
Due to memcg statistics implementation a special algorithm is used for estimating it's oom_score: we define it as maximum oom_score of the belonging tasks.

This seems to unfairly bias the root mem cgroup depending on process size. It isn't treated fairly as a leaf mem cgroup if they are being compared based on different criteria: the root mem cgroup as (mostly) the largest rss of a single process vs leaf mem cgroups as all anon, unevictable, and unreclaimable slab pages charged to it by all processes.

I imagine a configuration where the root mem cgroup has 100 processes attached each with rss of 80MB, compared to a leaf cgroup with 100 processes of 1MB rss each. How does this logic prevent repeatedly oom killing the processes of 1MB rss?

In this case, "the root cgroup is treated as a leaf memory cgroup" isn't quite fair, it can simply hide large processes from being selected. Users who configure cgroups in a unified hierarchy for other resource constraints are penalized for this choice even though the mem cgroup with 100 processes of 1MB rss each may not be limited itself.

I think for this comparison to be fair, it requires accounting for the root mem cgroup itself or for a different accounting methodology for leaf memory cgroups.

I'll reiterate what I did on the last version of the patchset: considering only leaf memory cgroups easily allows users to defeat this heuristic and bias against all of their memory usage up to the largest process size amongst the set of processes attached. If the user creates N child mem cgroups for their N processes and attaches one process to each child, the _only_ thing this achieved is to defeat your heuristic and prefer other leaf cgroups simply because those other leaf cgroups did not do this.

Effectively:

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

will radically shift the heuristic from a score of all anonymous + unevictable memory for all processes to a score of the largest anonymous + unevictable memory for a single process. There is no downside or ramifaction for the end user in doing this. When comparing cgroups based on usage, it only makes sense to compare the hierarchical usage of that cgroup so that attaching processes to descendants or splitting the implementation of a process into several smaller individual processes does not allow this heuristic to be defeated.

This is racy because mem_cgroup_select_oom_victim() found an eligible oc->chosen_memcg that is not INFLIGHT_VICTIM with at least one eligible process but mem_cgroup_scan_task(oc->chosen_memcg) did not. It means if a process cannot be killed because of oom_unkillable_task(), the only eligible processes moved or exited, or the /proc/pid/oom_score_adj of the eligible processes changed, we end up falling back to the complete tasklist scan. It would be better for oom_evaluate_memcg() to consider oom_unkillable_task() and also retry in the case where oom_kill_memcg_victim() returns NULL.

Quoted 17 lines

This seems to unfairly bias the root mem cgroup depending on process size. It isn't treated fairly as a leaf mem cgroup if they are being compared based on different criteria: the root mem cgroup as (mostly) the largest rss of a single process vs leaf mem cgroups as all anon, unevictable, and unreclaimable slab pages charged to it by all processes.

I imagine a configuration where the root mem cgroup has 100 processes attached each with rss of 80MB, compared to a leaf cgroup with 100 processes of 1MB rss each. How does this logic prevent repeatedly oom killing the processes of 1MB rss?

In this case, "the root cgroup is treated as a leaf memory cgroup" isn't quite fair, it can simply hide large processes from being selected. Users who configure cgroups in a unified hierarchy for other resource constraints are penalized for this choice even though the mem cgroup with 100 processes of 1MB rss each may not be limited itself.

Quoted 3 lines

I think for this comparison to be fair, it requires accounting for the root mem cgroup itself or for a different accounting methodology for leaf memory cgroups.

This is basically a workaround, because we don't have necessary stats for root memory cgroup. If we'll start gathering them at some point, we can change this and treat root memcg exactly as other leaf cgroups.

Or, if someone will come with an idea of a better approximation, it can be implemented as a separate enhancement on top of the initial implementation. This is more than welcome.

Quoted 12 lines

I'll reiterate what I did on the last version of the patchset: considering only leaf memory cgroups easily allows users to defeat this heuristic and bias against all of their memory usage up to the largest process size amongst the set of processes attached. If the user creates N child mem cgroups for their N processes and attaches one process to each child, the _only_ thing this achieved is to defeat your heuristic and prefer other leaf cgroups simply because those other leaf cgroups did not do this.

Effectively:

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

Quoted 8 lines

will radically shift the heuristic from a score of all anonymous + unevictable memory for all processes to a score of the largest anonymous + unevictable memory for a single process. There is no downside or ramifaction for the end user in doing this. When comparing cgroups based on usage, it only makes sense to compare the hierarchical usage of that cgroup so that attaching processes to descendants or splitting the implementation of a process into several smaller individual processes does not allow this heuristic to be defeated.

To all previously said words I can only add that cgroup v2 allows to limit the amount of cgroups in the sub-tree:
1a926e0bbab8 ("cgroup: implement hierarchy limits").

Quoted 9 lines

This is racy because mem_cgroup_select_oom_victim() found an eligible oc->chosen_memcg that is not INFLIGHT_VICTIM with at least one eligible process but mem_cgroup_scan_task(oc->chosen_memcg) did not. It means if a process cannot be killed because of oom_unkillable_task(), the only eligible processes moved or exited, or the /proc/pid/oom_score_adj of the eligible processes changed, we end up falling back to the complete tasklist scan. It would be better for oom_evaluate_memcg() to consider oom_unkillable_task() and also retry in the case where oom_kill_memcg_victim() returns NULL.

I agree with you here. The fallback to the existing mechanism is implemented to be safe for sure, especially in a case of a global OOM. When we'll get more confidence in cgroup-aware OOM killer reliability, we can change this behavior. Personally, I would prefer to get rid of looking at all tasks just to find a pre-existing OOM victim, but it might be quite tricky to implement.

Quoted 17 lines · Roman Gushchin

Quoted 17 lines

This seems to unfairly bias the root mem cgroup depending on process size. It isn't treated fairly as a leaf mem cgroup if they are being compared based on different criteria: the root mem cgroup as (mostly) the largest rss of a single process vs leaf mem cgroups as all anon, unevictable, and unreclaimable slab pages charged to it by all processes.

I imagine a configuration where the root mem cgroup has 100 processes attached each with rss of 80MB, compared to a leaf cgroup with 100 processes of 1MB rss each. How does this logic prevent repeatedly oom killing the processes of 1MB rss?

In this case, "the root cgroup is treated as a leaf memory cgroup" isn't quite fair, it can simply hide large processes from being selected. Users who configure cgroups in a unified hierarchy for other resource constraints are penalized for this choice even though the mem cgroup with 100 processes of 1MB rss each may not be limited itself.

Quoted 8 lines

Quoted 3 lines

I think for this comparison to be fair, it requires accounting for the root mem cgroup itself or for a different accounting methodology for leaf memory cgroups.

This is basically a workaround, because we don't have necessary stats for root memory cgroup. If we'll start gathering them at some point, we can change this and treat root memcg exactly as other leaf cgroups.

I understand why it currently cannot be an apples vs apples comparison without, as I suggest in the last paragraph, that the same accounting is done for the root mem cgroup, which is intuitive if it is to be considered on the same basis as leaf mem cgroups.

I understand for the design to work that leaf mem cgroups and the root mem cgroup must be compared if processes can be attached to the root mem cgroup. My point is that it is currently completely unfair as I've
stated: you can have 10000 processes attached to the root mem cgroup with rss of 80MB each and a leaf mem cgroup with 100 processes of 1MB rss each and the oom killer is going to target the leaf mem cgroup as a result of this apples vs oranges comparison.

In case it's not clear, the 10000 processes of 80MB rss each is the most likely contributor to a system-wide oom kill. Unfortunately, the heuristic introduced by this patchset is broken wrt a fair comparison of the root mem cgroup usage.

Quoted 4 lines

Or, if someone will come with an idea of a better approximation, it can be implemented as a separate enhancement on top of the initial implementation. This is more than welcome.

We don't need a better approximation, we need a fair comparison. The heuristic that this patchset is implementing is based on the usage of individual mem cgroups. For the root mem cgroup to be considered eligible, we need to understand its usage. That usage is _not_ what is implemented by this patchset, which is the largest rss of a single attached process. This, in fact, is not an "approximation" at all. In the example of 10000 processes attached with 80MB rss each, the usage of the root mem cgroup is _not_ 80MB.

I'll restate that oom killing a process is a last resort for the kernel, but it also must be able to make a smart decision. Targeting dozens of 1MB processes instead of 80MB processes because of a shortcoming in this implementation is not the appropriate selection, it's the opposite of the correct selection.

Quoted 21 lines

Quoted 12 lines

I'll reiterate what I did on the last version of the patchset: considering only leaf memory cgroups easily allows users to defeat this heuristic and bias against all of their memory usage up to the largest process size amongst the set of processes attached. If the user creates N child mem cgroups for their N processes and attaches one process to each child, the _only_ thing this achieved is to defeat your heuristic and prefer other leaf cgroups simply because those other leaf cgroups did not do this.

Effectively:

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

Quoted 8 lines

will radically shift the heuristic from a score of all anonymous + unevictable memory for all processes to a score of the largest anonymous + unevictable memory for a single process. There is no downside or ramifaction for the end user in doing this. When comparing cgroups based on usage, it only makes sense to compare the hierarchical usage of that cgroup so that attaching processes to descendants or splitting the implementation of a process into several smaller individual processes does not allow this heuristic to be defeated.

Quoted 4 lines

To all previously said words I can only add that cgroup v2 allows to limit the amount of cgroups in the sub-tree:
1a926e0bbab8 ("cgroup: implement hierarchy limits").

So the solution to

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

evading all oom kills for your mem cgroup is to limit the number of cgroups that can be created by the user? With a unified cgroup hierarchy, that doesn't work well if I wanted to actually constrain these individual processes to different resource limits like cpu usage. In fact, the user may not know it is effectively evading the oom killer entirely because it has constrained the cpu of individual processes because its a side-effect of this heuristic.

You chose not to respond to my reiteration of userspace having absolutely no control over victim selection with the new heuristic without setting all processes to be oom disabled via /proc/pid/oom_score_adj. If I have a very important job that is running on a system that is really supposed to use 80% of memory, I need to be able to specify that it should not be oom killed based on user goals. Setting all processes to be oom disabled in the important mem cgroup to avoid being oom killed unless absolutely necessary in a system oom condition is not a robust solution: (1) the mem cgroup livelocks if it reaches its own mem cgroup limit and (2) the system panic()'s if these preferred mem cgroups are the only consumers left on the system. With overcommit, both of these possibilities exist in the wild and the problem is only a result of the implementation detail of this patchset.

For these reasons: unfair comparison of root mem cgroup usage to bias against that mem cgroup from oom kill in system oom conditions, the ability of users to completely evade the oom killer by attaching all processes to child cgroups either purposefully or unpurposefully, and the inability of userspace to effectively control oom victim selection:

Nacked-by David Rientjes <[email protected]>

Quoted 11 lines

Quoted 10 lines

This is racy because mem_cgroup_select_oom_victim() found an eligible oc->chosen_memcg that is not INFLIGHT_VICTIM with at least one eligible process but mem_cgroup_scan_task(oc->chosen_memcg) did not. It means if a process cannot be killed because of oom_unkillable_task(), the only eligible processes moved or exited, or the /proc/pid/oom_score_adj of the eligible processes changed, we end up falling back to the complete tasklist scan. It would be better for oom_evaluate_memcg() to consider oom_unkillable_task() and also retry in the case where oom_kill_memcg_victim() returns NULL.

Quoted 6 lines

I agree with you here. The fallback to the existing mechanism is implemented to be safe for sure, especially in a case of a global OOM. When we'll get more confidence in cgroup-aware OOM killer reliability, we can change this behavior. Personally, I would prefer to get rid of looking at all tasks just to find a pre-existing OOM victim, but it might be quite tricky to implement.

I'm not sure what this has to do with confidence in this patchset's reliability? The race obviously exists: mem_cgroup_select_oom_victim() found an eligible process in oc->chosen_memcg but it was either ineligible later because of oom_unkillable_task(), it moved, or it exited. It's a race. For users who opt-in to this new heuristic, they should not be concerned with a process exiting and thus killing a completely unexpected process from an unexpected memcg when it should be possible to retry and select the correct victim.

Quoted 8 lines

We don't need a better approximation, we need a fair comparison. The heuristic that this patchset is implementing is based on the usage of individual mem cgroups. For the root mem cgroup to be considered eligible, we need to understand its usage. That usage is _not_ what is implemented by this patchset, which is the largest rss of a single attached process. This, in fact, is not an "approximation" at all. In the example of 10000 processes attached with 80MB rss each, the usage of the root mem cgroup is _not_ 80MB.

It's hard to imagine a "healthy" setup with 10000 process in the root memory cgroup, and even if we kill 1 process we will still have 9999 remaining process. I agree with you at some point, but it's not a real world example.

Quoted 6 lines

For these reasons: unfair comparison of root mem cgroup usage to bias against that mem cgroup from oom kill in system oom conditions, the ability of users to completely evade the oom killer by attaching all processes to child cgroups either purposefully or unpurposefully, and the inability of userspace to effectively control oom victim selection:

Quoted 1 line

Nacked-by David Rientjes <[email protected]>

So, if we'll sum the oom_score of tasks belonging to the root memory cgroup, will it fix the problem?

It might have some drawbacks as well (especially around oom_score_adj), but it's doable, if we'll ignore tasks which are not owners of their's mm struct.

Quoted 9 lines · Roman Gushchin

Quoted 8 lines

We don't need a better approximation, we need a fair comparison. The heuristic that this patchset is implementing is based on the usage of individual mem cgroups. For the root mem cgroup to be considered eligible, we need to understand its usage. That usage is _not_ what is implemented by this patchset, which is the largest rss of a single attached process. This, in fact, is not an "approximation" at all. In the example of 10000 processes attached with 80MB rss each, the usage of the root mem cgroup is _not_ 80MB.

Quoted 5 lines

It's hard to imagine a "healthy" setup with 10000 process in the root memory cgroup, and even if we kill 1 process we will still have 9999 remaining process. I agree with you at some point, but it's not a real world example.

It's an example that illustrates the problem with the unfair comparison between the root mem cgroup and leaf mem cgroups. It's unfair to compare [largest rss of a single process attached to a cgroup] to [anon + unevictable + unreclaimable slab usage of a cgroup]. It's not an approximation, as previously stated: the usage of the root mem cgroup is not 100MB if there are 10 such processes attached to the root mem cgroup, it's off by orders of magnitude.

For the root mem cgroup to be treated equally as a leaf mem cgroup as this patchset proposes, it must have a fair comparison. That can be done by accounting memory to the root mem cgroup in the same way it is to leaf mem cgroups.

But let's move the discussion forward to fix it. To avoid necessarily accounting memory to the root mem cgroup, have we considered if it is even necessary to address the root mem cgroup? For the users who opt-in to this heuristic, would it be possible to discount the root mem cgroup from the heuristic entirely so that oom kills originate from leaf mem cgroups? Or, perhaps better, oom kill from non-memory.oom_group cgroups only if the victim rss is greater than an eligible victim rss attached to the root mem cgroup?

Quoted 7 lines

Quoted 7 lines

For these reasons: unfair comparison of root mem cgroup usage to bias against that mem cgroup from oom kill in system oom conditions, the ability of users to completely evade the oom killer by attaching all processes to child cgroups either purposefully or unpurposefully, and the inability of userspace to effectively control oom victim selection:

Quoted 8 lines

Quoted 1 line

Nacked-by David Rientjes <[email protected]>

So, if we'll sum the oom_score of tasks belonging to the root memory cgroup, will it fix the problem?

It might have some drawbacks as well (especially around oom_score_adj), but it's doable, if we'll ignore tasks which are not owners of their's mm struct.

You would be required to discount oom_score_adj because the heuristic doesn't account for oom_score_adj when comparing the anon + unevictable + unreclaimable slab of leaf mem cgroups. This wouldn't result in the correct victim selection in real-world scenarios where processes attached to the root mem cgroup are vital to the system and not part of any user job, i.e. they are important system daemons and the "activity manager" responsible for orchestrating the cgroup hierarchy.

It's also still unfair because it now compares
[sum of rss of processes attached to a cgroup] to
[anon + unevictable + unreclaimable slab usage of a cgroup]. RSS isn't going to be a solution, regardless if its one process or all processes, if it's being compared to more types of memory in leaf cgroups.

If we really don't want root mem cgroup accounting so this is a fair comparison, I think the heuristic needs to special case the root mem cgroup either by discounting root oom kills if there are eligible oom kills from leaf cgroups (the user would be opting-in to this behavior) or comparing the badness of a victim from a leaf cgroup to the badness of a victim from the root cgroup when deciding which to kill and allow the user to protect root mem cgroup processes with oom_score_adj.

That aside, all of this has only addressed one of the three concerns with the patchset.

I believe the solution to avoid allowing users to circumvent oom kill is to account usage up the hierarchy as you have done in the past. Cgroup hierarchies can be managed by the user so they can create their own subcontainers, this is nothing new, and I would hope that you wouldn't limit your feature to only a very specific set of usecases. That may be your solution for the root mem cgroup itself: if the hierarchical usage of all top-level mem cgroups is known, it's possible to find the root mem cgroup usage by subtraction, you are using stats that are global vmstats in your heuristic.

Accounting usage up the hierarchy avoids the first two concerns with the patchset. It allows you to implicitly understand the usage of the root mem cgroup itself, and does not allow users to circumvent oom kill by creating subcontainers, either purposefully or not. The third concern, userspace influence, can allow users to attack leaf mem cgroups deeper in the tree if it is using more memory than expected, but the hierarchical usage is lower at the top-level. That is the only objection that I have seen to using hierarchical usage: there may be a single cgroup deeper in the tree that avoids oom kill because another hierarchy has a higher usage. This can trivially be addressed either by oom priorities or an adjustment, just like oom_score_adj, on cgroup usage.

Quoted 8 lines

But let's move the discussion forward to fix it. To avoid necessarily accounting memory to the root mem cgroup, have we considered if it is even necessary to address the root mem cgroup? For the users who opt-in to this heuristic, would it be possible to discount the root mem cgroup from the heuristic entirely so that oom kills originate from leaf mem cgroups? Or, perhaps better, oom kill from non-memory.oom_group cgroups only if the victim rss is greater than an eligible victim rss attached to the root mem cgroup?

David, I'm not pretending for implementing the best possible accounting for the root memory cgroup, and I'm sure there is a place for further enhancement. But if it's not leading to some obviously stupid victim selection (like ignoring leaking task, which consumes most of the memory), I don't see why it should be treated as a blocker for the whole patchset. I also doubt that any of us has these examples, and the best way to get them is to get some real usage feedback.

Ignoring oom_score_adj, subtracting leaf usage sum from system usage etc, these all are perfect ideas which can be implemented on top of this patchset.

Quoted 35 lines

You would be required to discount oom_score_adj because the heuristic doesn't account for oom_score_adj when comparing the anon + unevictable + unreclaimable slab of leaf mem cgroups. This wouldn't result in the correct victim selection in real-world scenarios where processes attached to the root mem cgroup are vital to the system and not part of any user job, i.e. they are important system daemons and the "activity manager" responsible for orchestrating the cgroup hierarchy.

It's also still unfair because it now compares
[sum of rss of processes attached to a cgroup] to
[anon + unevictable + unreclaimable slab usage of a cgroup]. RSS isn't going to be a solution, regardless if its one process or all processes, if it's being compared to more types of memory in leaf cgroups.

If we really don't want root mem cgroup accounting so this is a fair comparison, I think the heuristic needs to special case the root mem cgroup either by discounting root oom kills if there are eligible oom kills from leaf cgroups (the user would be opting-in to this behavior) or comparing the badness of a victim from a leaf cgroup to the badness of a victim from the root cgroup when deciding which to kill and allow the user to protect root mem cgroup processes with oom_score_adj.

That aside, all of this has only addressed one of the three concerns with the patchset.

I believe the solution to avoid allowing users to circumvent oom kill is to account usage up the hierarchy as you have done in the past. Cgroup hierarchies can be managed by the user so they can create their own subcontainers, this is nothing new, and I would hope that you wouldn't limit your feature to only a very specific set of usecases. That may be your solution for the root mem cgroup itself: if the hierarchical usage of all top-level mem cgroups is known, it's possible to find the root mem cgroup usage by subtraction, you are using stats that are global vmstats in your heuristic.

Quoted 11 lines

Accounting usage up the hierarchy avoids the first two concerns with the patchset. It allows you to implicitly understand the usage of the root mem cgroup itself, and does not allow users to circumvent oom kill by creating subcontainers, either purposefully or not. The third concern, userspace influence, can allow users to attack leaf mem cgroups deeper in the tree if it is using more memory than expected, but the hierarchical usage is lower at the top-level. That is the only objection that I have seen to using hierarchical usage: there may be a single cgroup deeper in the tree that avoids oom kill because another hierarchy has a higher usage. This can trivially be addressed either by oom priorities or an adjustment, just like oom_score_adj, on cgroup usage.

As I've said, I barely understand how the exact implementation of root memory cgroup accounting is considered a blocker for the whole feature. The same is true for oom priorities: it's something that can and should be implemented on top of the basic semantics, introduced by this patchset.

So, the only real question is the way how we find a victim memcg in the
subtree: by performing independent election on each level or by searching tree-wide. We all had many discussion around, and as you remember, initially I was supporting the first option.
But then Michal provided a very strong argument:
if you have 3 similar workloads in A, B and C, but for non-memory-related

reasons (e.g. cpu time sharing) you have to join A and B into a group D:
/\
D C
/ \
A B
it's strange to penalize A and B for it. It looks to me that you're talking about the similar case, but you consider this hierarchy useful. So, overall, it seems to be depending on exact configuration.

The selected approach is easy extendable into hierarchical direction:
as I've said before, we can introduce a new value of memory.oom_group, which will enable cumulative accounting without mass killing.

And, tbh, I don't see how oom_priorities will resolve an opposite problem if we'd take the hierarchical approach.

On Wed, 11 Oct 2017, Roman Gushchin wrote:

Think about it in a different way: we currently compare per-process usage and userspace has /proc/pid/oom_score_adj to adjust that usage depending on priorities of that process and still oom kill if there's a memory leak. Your heuristic compares per-cgroup usage, it's the cgroup-aware oom killer after all. We don't need a strict memory.oom_priority that outranks all other sibling cgroups regardless of usage. We need a memory.oom_score_adj to adjust the per-cgroup usage. The decisionmaking in your earlier example would be under the control of C/memory.oom_score_adj and D/memory.oom_score_adj. Problem solved.

It also solves the problem of userspace being able to influence oom victim selection so now they can protect important cgroups just like we can protect important processes today.

And since this would be hierarchical usage, you can trivially infer root mem cgroup usage by subtraction of top-level mem cgroup usage.

This is a powerful solution to the problem and gives userspace the control they need so that it can work in all usecases, not a subset of usecases.

Quoted 17 lines

Think about it in a different way: we currently compare per-process usage and userspace has /proc/pid/oom_score_adj to adjust that usage depending on priorities of that process and still oom kill if there's a memory leak. Your heuristic compares per-cgroup usage, it's the cgroup-aware oom killer after all. We don't need a strict memory.oom_priority that outranks all other sibling cgroups regardless of usage. We need a memory.oom_score_adj to adjust the per-cgroup usage. The decisionmaking in your earlier example would be under the control of C/memory.oom_score_adj and D/memory.oom_score_adj. Problem solved.

It also solves the problem of userspace being able to influence oom victim selection so now they can protect important cgroups just like we can protect important processes today.

And since this would be hierarchical usage, you can trivially infer root mem cgroup usage by subtraction of top-level mem cgroup usage.

Quoted 2 lines

This is a powerful solution to the problem and gives userspace the control they need so that it can work in all usecases, not a subset of usecases.

You're right that per-cgroup oom_score_adj may resolve the issue with too strict semantics of oom_priorities. But I believe nobody likes the existing per-process oom_score_adj interface, and there are reasons behind.

Especially in case of memcg-OOM, getting the idea how exactly oom_score_adj will work is not trivial.

For these reasons: unfair comparison of root mem cgroup usage to bias against that mem cgroup from oom kill in system oom conditions, the ability of users to completely evade the oom killer by attaching all processes to child cgroups either purposefully or unpurposefully, and the inability of userspace to effectively control oom victim selection:

Nacked-by David Rientjes <[email protected]>

Quoted 7 lines

For these reasons: unfair comparison of root mem cgroup usage to bias against that mem cgroup from oom kill in system oom conditions, the ability of users to completely evade the oom killer by attaching all processes to child cgroups either purposefully or unpurposefully, and the inability of userspace to effectively control oom victim selection:

Nacked-by David Rientjes <[email protected]>

I consider this NACK rather dubious. Evading the heuristic as you describe requires root privileges in default configuration because normal users are not allowed to create subtrees. If you really want to delegate subtree to an untrusted entity then you do not have to opt-in for this oom strategy. We can work on an additional means which would allow to cover those as well (e.g. priority based one which is requested for other usecases).

A similar argument applies to the root memcg evaluation. While the proposed behavior is not optimal it would work for general usecase described here where the root memcg doesn't really run any large number of tasks. If somebody who explicitly opts-in for the new strategy and it doesn't work well for that usecase we can enhance the behavior. That alone is not a reason to nack the whole thing.

I find it really disturbing that you keep nacking this approach just because it doesn't suite your specific usecase while it doesn't break it. Moreover it has been stated several times already that future improvements are possible and cover what you have described already.

Quoted 8 lines · Michal Hocko

Quoted 7 lines

For these reasons: unfair comparison of root mem cgroup usage to bias against that mem cgroup from oom kill in system oom conditions, the ability of users to completely evade the oom killer by attaching all processes to child cgroups either purposefully or unpurposefully, and the inability of userspace to effectively control oom victim selection:

Nacked-by David Rientjes <[email protected]>

Quoted 8 lines

I consider this NACK rather dubious. Evading the heuristic as you describe requires root privileges in default configuration because normal users are not allowed to create subtrees. If you really want to delegate subtree to an untrusted entity then you do not have to opt-in for this oom strategy. We can work on an additional means which would allow to cover those as well (e.g. priority based one which is requested for other usecases).

You're missing the point that the user is trusted and it may be doing something to circumvent oom kill unknowingly.

With a single unified
hierarchy, the user is forced to attach its processes to subcontainers if it wants to constrain resources with other controllers. Doing so ends up completely avoiding oom kill because of this implementation detail. It has nothing to do with trust and the admin who is opting-in will not know a user has cirumvented oom kill purely because it constrains its processes with controllers other than the memory controller.

Quoted 7 lines

A similar argument applies to the root memcg evaluation. While the proposed behavior is not optimal it would work for general usecase described here where the root memcg doesn't really run any large number of tasks. If somebody who explicitly opts-in for the new strategy and it doesn't work well for that usecase we can enhance the behavior. That alone is not a reason to nack the whole thing.

Quoted 4 lines

I find it really disturbing that you keep nacking this approach just because it doesn't suite your specific usecase while it doesn't break it. Moreover it has been stated several times already that future improvements are possible and cover what you have described already.

This has nothing to do with my specific usecase.

Quoted 17 lines · Roman Gushchin

Quoted 17 lines

This seems to unfairly bias the root mem cgroup depending on process size. It isn't treated fairly as a leaf mem cgroup if they are being compared based on different criteria: the root mem cgroup as (mostly) the largest rss of a single process vs leaf mem cgroups as all anon, unevictable, and unreclaimable slab pages charged to it by all processes.

I imagine a configuration where the root mem cgroup has 100 processes attached each with rss of 80MB, compared to a leaf cgroup with 100 processes of 1MB rss each. How does this logic prevent repeatedly oom killing the processes of 1MB rss?

In this case, "the root cgroup is treated as a leaf memory cgroup" isn't quite fair, it can simply hide large processes from being selected. Users who configure cgroups in a unified hierarchy for other resource constraints are penalized for this choice even though the mem cgroup with 100 processes of 1MB rss each may not be limited itself.

Quoted 8 lines

Quoted 3 lines

I think for this comparison to be fair, it requires accounting for the root mem cgroup itself or for a different accounting methodology for leaf memory cgroups.

This is basically a workaround, because we don't have necessary stats for root memory cgroup. If we'll start gathering them at some point, we can change this and treat root memcg exactly as other leaf cgroups.

I understand why it currently cannot be an apples vs apples comparison without, as I suggest in the last paragraph, that the same accounting is done for the root mem cgroup, which is intuitive if it is to be considered on the same basis as leaf mem cgroups.

I understand for the design to work that leaf mem cgroups and the root mem cgroup must be compared if processes can be attached to the root mem cgroup. My point is that it is currently completely unfair as I've
stated: you can have 10000 processes attached to the root mem cgroup with rss of 80MB each and a leaf mem cgroup with 100 processes of 1MB rss each and the oom killer is going to target the leaf mem cgroup as a result of this apples vs oranges comparison.

In case it's not clear, the 10000 processes of 80MB rss each is the most likely contributor to a system-wide oom kill. Unfortunately, the heuristic introduced by this patchset is broken wrt a fair comparison of the root mem cgroup usage.

Quoted 4 lines

Or, if someone will come with an idea of a better approximation, it can be implemented as a separate enhancement on top of the initial implementation. This is more than welcome.

We don't need a better approximation, we need a fair comparison. The heuristic that this patchset is implementing is based on the usage of individual mem cgroups. For the root mem cgroup to be considered eligible, we need to understand its usage. That usage is _not_ what is implemented by this patchset, which is the largest rss of a single attached process. This, in fact, is not an "approximation" at all. In the example of 10000 processes attached with 80MB rss each, the usage of the root mem cgroup is _not_ 80MB.

I'll restate that oom killing a process is a last resort for the kernel, but it also must be able to make a smart decision. Targeting dozens of 1MB processes instead of 80MB processes because of a shortcoming in this implementation is not the appropriate selection, it's the opposite of the correct selection.

Quoted 21 lines

Quoted 12 lines

I'll reiterate what I did on the last version of the patchset: considering only leaf memory cgroups easily allows users to defeat this heuristic and bias against all of their memory usage up to the largest process size amongst the set of processes attached. If the user creates N child mem cgroups for their N processes and attaches one process to each child, the _only_ thing this achieved is to defeat your heuristic and prefer other leaf cgroups simply because those other leaf cgroups did not do this.

Effectively:

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

Quoted 8 lines

will radically shift the heuristic from a score of all anonymous + unevictable memory for all processes to a score of the largest anonymous + unevictable memory for a single process. There is no downside or ramifaction for the end user in doing this. When comparing cgroups based on usage, it only makes sense to compare the hierarchical usage of that cgroup so that attaching processes to descendants or splitting the implementation of a process into several smaller individual processes does not allow this heuristic to be defeated.

Quoted 4 lines

To all previously said words I can only add that cgroup v2 allows to limit the amount of cgroups in the sub-tree:
1a926e0bbab8 ("cgroup: implement hierarchy limits").

So the solution to

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

evading all oom kills for your mem cgroup is to limit the number of cgroups that can be created by the user? With a unified cgroup hierarchy, that doesn't work well if I wanted to actually constrain these individual processes to different resource limits like cpu usage. In fact, the user may not know it is effectively evading the oom killer entirely because it has constrained the cpu of individual processes because its a side-effect of this heuristic.

You chose not to respond to my reiteration of userspace having absolutely no control over victim selection with the new heuristic without setting all processes to be oom disabled via /proc/pid/oom_score_adj. If I have a very important job that is running on a system that is really supposed to use 80% of memory, I need to be able to specify that it should not be oom killed based on user goals. Setting all processes to be oom disabled in the important mem cgroup to avoid being oom killed unless absolutely necessary in a system oom condition is not a robust solution: (1) the mem cgroup livelocks if it reaches its own mem cgroup limit and (2) the system panic()'s if these preferred mem cgroups are the only consumers left on the system. With overcommit, both of these possibilities exist in the wild and the problem is only a result of the implementation detail of this patchset.

For these reasons: unfair comparison of root mem cgroup usage to bias against that mem cgroup from oom kill in system oom conditions, the ability of users to completely evade the oom killer by attaching all processes to child cgroups either purposefully or unpurposefully, and the inability of userspace to effectively control oom victim selection:

Nacked-by David Rientjes <[email protected]>

The cgroup-aware OOM killer treats leaf memory cgroups as memory consumption entities and performs the victim selection by comparing them based on their memory footprint. Then it kills the biggest task inside the selected memory cgroup.

But there are workloads, which are not tolerant to a such behavior. Killing a random task may leave the workload in a broken state.

To solve this problem, memory.oom_group knob is introduced. It will define, whether a memory group should be treated as an indivisible memory consumer, compared by total memory consumption with other memory consumers (leaf memory cgroups and other memory cgroups with memory.oom_group set), and whether all belonging tasks should be killed if the cgroup is selected.

If set on memcg A, it means that in case of system-wide OOM or memcg-wide OOM scoped to A or any ancestor cgroup, all tasks, belonging to the sub-tree of A will be killed. If OOM event is scoped to a descendant cgroup (A/B, for example), only tasks in that cgroup can be affected. OOM killer will never touch any tasks outside of the scope of the OOM event.

Also, tasks with oom_score_adj set to -1000 will not be killed because this has been a long established way to protect a particular process from seeing an unexpected SIGKILL from the OOM killer. Ignoring this user defined configuration might lead to data corruptions or other misbehavior.

The default value is 0.

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/memcontrol.h | 17 +++++++++++
 mm/memcontrol.c            | 75 +++++++++++++++++++++++++++++++++++++++++++---
 mm/oom_kill.c              | 49 +++++++++++++++++++++++-------
 3 files changed, 127 insertions(+), 14 deletions(-)

include/linux/memcontrol.h +17 -0

index 75b63b68846e..84ac10d7e67d 100644
@@ -200,6 +200,13 @@ struct mem_cgroup {
 	/* OOM-Killer disable */
 	int		oom_kill_disable;
 
+	/*
+	 * Treat the sub-tree as an indivisible memory consumer,
+	 * kill all belonging tasks if the memory cgroup selected
+	 * as OOM victim.
+	 */
+	bool oom_group;
+
 	/* handle for "memory.events" */
 	struct cgroup_file events_file;
 

@@ -488,6 +495,11 @@ bool mem_cgroup_oom_synchronize(bool wait);
 
 bool mem_cgroup_select_oom_victim(struct oom_control *oc);
 
+static inline bool mem_cgroup_oom_group(struct mem_cgroup *memcg)
+{
+	return memcg->oom_group;
+}
+
 #ifdef CONFIG_MEMCG_SWAP
 extern int do_swap_account;
 #endif

@@ -953,6 +965,11 @@ static inline bool mem_cgroup_select_oom_victim(struct oom_control *oc)
 {
 	return false;
 }
+
+static inline bool mem_cgroup_oom_group(struct mem_cgroup *memcg)
+{
+	return false;
+}
 #endif /* CONFIG_MEMCG */
 
 /* idx can be of type enum memcg_stat_item or node_stat_item */

mm/memcontrol.c +71 -4

index 191b70735f1f..d5acb278b11a 100644
@@ -2776,19 +2776,51 @@ static long oom_evaluate_memcg(struct mem_cgroup *memcg,
 
 static void select_victim_memcg(struct mem_cgroup *root, struct oom_control *oc)
 {
-	struct mem_cgroup *iter;
+	struct mem_cgroup *iter, *group = NULL;
+	long group_score = 0;
 
 	oc->chosen_memcg = NULL;
 	oc->chosen_points = 0;
 
 	/*
+	 * If OOM is memcg-wide, and the memcg has the oom_group flag set,
+	 * all tasks belonging to the memcg should be killed.
+	 * So, we mark the memcg as a victim.
+	 */
+	if (oc->memcg && mem_cgroup_oom_group(oc->memcg)) {
+		oc->chosen_memcg = oc->memcg;
+		css_get(&oc->chosen_memcg->css);
+		return;
+	}
+
+	/*
 	 * The oom_score is calculated for leaf memory cgroups (including
 	 * the root memcg).
+	 * Non-leaf oom_group cgroups accumulating score of descendant
+	 * leaf memory cgroups.
 	 */
 	rcu_read_lock();
 	for_each_mem_cgroup_tree(iter, root) {
 		long score;
 
+		/*
+		 * We don't consider non-leaf non-oom_group memory cgroups
+		 * as OOM victims.
+		 */
+		if (memcg_has_children(iter) && iter != root_mem_cgroup &&
+		    !mem_cgroup_oom_group(iter))
+			continue;
+
+		/*
+		 * If group is not set or we've ran out of the group's sub-tree,
+		 * we should set group and reset group_score.
+		 */
+		if (!group || group == root_mem_cgroup ||
+		    !mem_cgroup_is_descendant(iter, group)) {
+			group = iter;
+			group_score = 0;
+		}
+
 		if (memcg_has_children(iter) && iter != root_mem_cgroup)
 			continue;
 

@@ -2810,9 +2842,11 @@ static void select_victim_memcg(struct mem_cgroup *root, struct oom_control *oc)
 			break;
 		}
 
-		if (score > oc->chosen_points) {
-			oc->chosen_points = score;
-			oc->chosen_memcg = iter;
+		group_score += score;
+
+		if (group_score > oc->chosen_points) {
+			oc->chosen_points = group_score;
+			oc->chosen_memcg = group;
 		}
 	}
 

@@ -5437,6 +5471,33 @@ static ssize_t memory_max_write(struct kernfs_open_file *of,
 	return nbytes;
 }
 
+static int memory_oom_group_show(struct seq_file *m, void *v)
+{
+	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(m));
+	bool oom_group = memcg->oom_group;
+
+	seq_printf(m, "%d\n", oom_group);
+
+	return 0;
+}
+
+static ssize_t memory_oom_group_write(struct kernfs_open_file *of,
+				      char *buf, size_t nbytes,
+				      loff_t off)
+{
+	struct mem_cgroup *memcg = mem_cgroup_from_css(of_css(of));
+	int oom_group;
+	int err;
+
+	err = kstrtoint(strstrip(buf), 0, &oom_group);
+	if (err)
+		return err;
+
+	memcg->oom_group = oom_group;
+
+	return nbytes;
+}
+
 static int memory_events_show(struct seq_file *m, void *v)
 {
 	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(m));

@@ -5557,6 +5618,12 @@ static struct cftype memory_files[] = {
 		.write = memory_max_write,
 	},
 	{
+		.name = "oom_group",
+		.flags = CFTYPE_NOT_ON_ROOT | CFTYPE_NS_DELEGATABLE,
+		.seq_show = memory_oom_group_show,
+		.write = memory_oom_group_write,
+	},
+	{
 		.name = "events",
 		.flags = CFTYPE_NOT_ON_ROOT,
 		.file_offset = offsetof(struct mem_cgroup, events_file),

mm/oom_kill.c +39 -10

index 20e62ec32ba8..c8fbc73c4ed3 100644
@@ -851,6 +851,17 @@ static void __oom_kill_process(struct task_struct *victim)
 	struct mm_struct *mm;
 	bool can_oom_reap = true;
 
+	/*
+	 * __oom_kill_process() is used to kill all tasks belonging to
+	 * the selected memory cgroup, so we should check that we're not
+	 * trying to kill an unkillable task.
+	 */
+	if (is_global_init(victim) || (victim->flags & PF_KTHREAD) ||
+	    victim->signal->oom_score_adj == OOM_SCORE_ADJ_MIN) {
+		put_task_struct(victim);
+		return;
+	}
+
 	p = find_lock_task_mm(victim);
 	if (!p) {
 		put_task_struct(victim);

@@ -987,21 +998,39 @@ 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)
+static int oom_kill_memcg_member(struct task_struct *task, void *unused)
 {
+	get_task_struct(task);
+	__oom_kill_process(task);
+	return 0;
+}
 
+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);
+	/*
+	 * If memory.oom_group is set, kill all tasks belonging to the sub-tree
+	 * of the chosen memory cgroup, otherwise kill the task with the biggest
+	 * memory footprint.
+	 */
+	if (mem_cgroup_oom_group(oc->chosen_memcg)) {
+		mem_cgroup_scan_tasks(oc->chosen_memcg, oom_kill_memcg_member,
+				      NULL);
+		/* We have one or more terminating processes at this point. */
+		oc->chosen_task = INFLIGHT_VICTIM;
+	} else {
+		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);

Signature · 1 line

2.13.6

Btw. here is how I would do the recursive oom badness. The diff is not the nicest one because there is some code moving but the resulting code is smaller and imho easier to grasp. Only compile tested though

---

include/linux/cgroup.h +5 -0

index 085056e562b1..9cdba4682198 100644
@@ -122,6 +122,11 @@ void cgroup_free(struct task_struct *p);
 int cgroup_init_early(void);
 int cgroup_init(void);
 
+static bool cgroup_has_tasks(struct cgroup *cgrp)
+{
+	return cgrp->nr_populated_csets;
+}
+
 /*
  * Iteration helpers and macros.
  */

kernel/cgroup/cgroup.c +0 -5

index 8dacf73ad57e..a2dd7e3ffe23 100644
@@ -319,11 +319,6 @@ static void cgroup_idr_remove(struct idr *idr, int id)
 	spin_unlock_bh(&cgroup_idr_lock);
 }
 
-static bool cgroup_has_tasks(struct cgroup *cgrp)
-{
-	return cgrp->nr_populated_csets;
-}
-
 bool cgroup_is_threaded(struct cgroup *cgrp)
 {
 	return cgrp->dom_cgrp != cgrp;

Quoted 3 lines · Michal Hocko

Btw. here is how I would do the recursive oom badness. The diff is not the nicest one because there is some code moving but the resulting code is smaller and imho easier to grasp. Only compile tested though

Thanks!

I'm not against this approach, and maybe it can lead to a better code, but the version you sent is just not there yet.

There are some problems with it:

1) If there are nested cgroups with oom_group set, you will calculate a badness multiple times, and rely on the fact, that top memcg will become the largest score. It can be optimized, of course, but it's additional code.

2) cgroup_has_tasks() probably requires additional locking. Maybe it's ok to read nr_populated_csets without explicit locking, but it's not obvious for me.

3) Returning -1 from memcg_oom_badness() if eligible is equal to 0 is suspicious.

Right now your version has exactly the same amount of code (skipping comments). I assume, this approach just requires some additional thinking/rework.