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 should be treated as a leaf cgroup, so only tasks which are directly belonging to the root cgroup should be iterated over.

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]

---
 mm/memcontrol.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

mm/memcontrol.c +3 -4

index ae37b5624eb2..fa1a5120ce3f 100644
@@ -917,7 +917,8 @@ static void invalidate_reclaim_iterators(struct mem_cgroup *dead_memcg)
  * value, the function breaks the iteration loop and returns the value.
  * Otherwise, it will iterate over all tasks and return 0.
  *
- * This function must not be called for the root memory cgroup.
+ * If memcg is the root memory cgroup, this function will iterate only
+ * over tasks belonging directly to the root memory cgroup.
  */
 int mem_cgroup_scan_tasks(struct mem_cgroup *memcg,
 			  int (*fn)(struct task_struct *, void *), void *arg)

@@ -925,8 +926,6 @@ int mem_cgroup_scan_tasks(struct mem_cgroup *memcg,
 	struct mem_cgroup *iter;
 	int ret = 0;
 
-	BUG_ON(memcg == root_mem_cgroup);
-
 	for_each_mem_cgroup_tree(iter, memcg) {
 		struct css_task_iter it;
 		struct task_struct *task;

@@ -935,7 +934,7 @@ int mem_cgroup_scan_tasks(struct mem_cgroup *memcg,
 		while (!ret && (task = css_task_iter_next(&it)))
 			ret = fn(task, arg);
 		css_task_iter_end(&it);
-		if (ret) {
+		if (ret || memcg == root_mem_cgroup) {
 			mem_cgroup_iter_break(memcg, iter);
 			break;
 		}

Signature · 1 line

2.13.5

Quoted 4 lines · Roman Gushchin

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.

Quoted 3 lines

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

I would only add that this patch doesn't introduce any functionally visible change because we never trigger oom killer with the root memcg as the root of the hierarchy. So this is just a preparatory work for later changes.

Quoted 14 lines

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]

Acked-by Michal Hocko <[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 memory consumer:
a leaf memory cgroup or a memory cgroup with the memory.oom_group option set. Then it kills either a task with the biggest memory footprint, either all belonging tasks, if memory.oom_group is set. If a cgroup has memory.oom_group set, all descendant cgroups implicitly inherit the memory.oom_group setting.

Tasks with oom_score_adj set to -1000 are considered as unkillable.

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

mm/memcontrol.c +148 -0

+	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, *parent;
+
+	/*
+	 * If OOM is memcg-wide, and the memcg or it's ancestor has
+	 * the oom_group flag, simple select 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);
+		oc->chosen_points = oc->memcg->oom_score;
+		return;
+	}
+
+	oc->chosen_memcg = NULL;
+
+	/*
+	 * The oom_score is calculated for leaf memcgs and propagated upwards
+	 * by the tree.
+	 *
+	 * for_each_mem_cgroup_tree() walks the tree in pre-order,
+	 * so we simple reset oom_score for non-lead cgroups before
+	 * starting accumulating an actual value from underlying sub-tree.
+	 *
+	 * Root memcg is treated as a leaf memcg.
+	 */
+	rcu_read_lock();
+	for_each_mem_cgroup_tree(iter, root) {
+		if (memcg_has_children(iter) && iter != root_mem_cgroup) {
+			iter->oom_score = 0;
+			continue;
+		}
+
+		iter->oom_score = oom_evaluate_memcg(iter, oc->nodemask,
+						     oc->totalpages);
+
+		/*
+		 * Ignore empty and non-eligible memory cgroups.
+		 */
+		if (iter->oom_score == 0)
+			continue;
+
+		/*
+		 * If there are inflight OOM victims, we don't need to look
+		 * further for new victims.
+		 */
+		if (iter->oom_score == -1) {
+			oc->chosen_memcg = INFLIGHT_VICTIM;
+			mem_cgroup_iter_break(root, iter);
+			break;
+		}
+
+		if (iter->oom_score > oc->chosen_points) {
+			oc->chosen_memcg = iter;
+			oc->chosen_points = iter->oom_score;
+		}
+
+		for (parent = parent_mem_cgroup(iter); parent && parent != root;
+		     parent = parent_mem_cgroup(parent)) {
+			parent->oom_score += iter->oom_score;
+
+			if (mem_cgroup_oom_group(parent) &&
+			    parent->oom_score > oc->chosen_points) {
+				oc->chosen_memcg = parent;

mm/oom_kill.c +14 -0

+		get_task_struct(task);
+		__oom_kill_process(task);
+	}
+	return 0;
+}
+
+static bool oom_kill_memcg_victim(struct oom_control *oc)
+{
+	static DEFINE_RATELIMIT_STATE(oom_rs, DEFAULT_RATELIMIT_INTERVAL,
+				      DEFAULT_RATELIMIT_BURST);
+
+	if (oc->chosen_memcg == NULL || oc->chosen_memcg == INFLIGHT_VICTIM)
+		return oc->chosen_memcg;
+

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

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.

Quoted 8 lines

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

Under OOM conditions, it looks for the biggest memory consumer:
a leaf memory cgroup or a memory cgroup with the memory.oom_group option set. Then it kills either a task with the biggest memory footprint, either all belonging tasks, if memory.oom_group is set. If a cgroup has memory.oom_group set, all descendant cgroups implicitly inherit the memory.oom_group setting.

I think it would be better to separate oom_group into its own patch. So this patch would just add the cgroup awareness and oom_group will build on top of that.

Wrt. to the implicit inheritance you brought up in a separate email thread [1]. Let me quote
: after some additional thinking I don't think anymore that implicit : propagation of oom_group is a good idea. Let me explain: assume we : have memcg A with memory.max and memory.oom_group set, and nested : memcg A/B with memory.max set. Let's imagine we have an OOM event if : A/B. What is an expected system behavior?
: We have OOM scoped to A/B, and any action should be also scoped to A/B. : We really shouldn't touch processes which are not belonging to A/B. : That means we should either kill the biggest process in A/B, either all : processes in A/B. It's natural to make A/B/memory.oom_group responsible : for this decision. It's strange to make the depend on A/memory.oom_group, IMO. : It really makes no sense, and makes oom_group knob really hard to describe. :
: Also, after some off-list discussion, we've realized that memory.oom_knob : should be delegatable. The workload should have control over it to express : dependency between processes.

OK, I have asked about this already but I am not sure the answer was very explicit. So let me ask again. When exactly a subtree would disagree with the parent on oom_group? In other words when do we want a different cleanup based on the OOM root? I am not saying this is wrong I am just curious about a practical example.

Quoted 9 lines

Tasks with oom_score_adj set to -1000 are considered as unkillable.

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

[1] http://lkml.kernel.org/r/[email protected]

[...]

Quoted 11 lines

mm/memcontrol.c +11 -0

+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

Review

Quoted 2 lines

(quoted diff) +2 -0

+	 * of the belonging tasks.
+	 */

Why not a sum of all tasks which would more resemble what we do for other memcgs? Sure this would require ignoring oom_score_adj so oom_badness would have to be tweaked a bit (basically split it into __oom_badness which calculates the value without the bias and oom_badness on top adding the bias on top of the scaled value).

Quoted 149 lines

mm/memcontrol.c +148 -0

+	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, *parent;
+
+	/*
+	 * If OOM is memcg-wide, and the memcg or it's ancestor has
+	 * the oom_group flag, simple select 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);
+		oc->chosen_points = oc->memcg->oom_score;
+		return;
+	}
+
+	oc->chosen_memcg = NULL;
+
+	/*
+	 * The oom_score is calculated for leaf memcgs and propagated upwards
+	 * by the tree.
+	 *
+	 * for_each_mem_cgroup_tree() walks the tree in pre-order,
+	 * so we simple reset oom_score for non-lead cgroups before
+	 * starting accumulating an actual value from underlying sub-tree.
+	 *
+	 * Root memcg is treated as a leaf memcg.
+	 */
+	rcu_read_lock();
+	for_each_mem_cgroup_tree(iter, root) {
+		if (memcg_has_children(iter) && iter != root_mem_cgroup) {
+			iter->oom_score = 0;
+			continue;
+		}
+
+		iter->oom_score = oom_evaluate_memcg(iter, oc->nodemask,
+						     oc->totalpages);
+
+		/*
+		 * Ignore empty and non-eligible memory cgroups.
+		 */
+		if (iter->oom_score == 0)
+			continue;
+
+		/*
+		 * If there are inflight OOM victims, we don't need to look
+		 * further for new victims.
+		 */
+		if (iter->oom_score == -1) {
+			oc->chosen_memcg = INFLIGHT_VICTIM;
+			mem_cgroup_iter_break(root, iter);
+			break;
+		}
+
+		if (iter->oom_score > oc->chosen_points) {
+			oc->chosen_memcg = iter;
+			oc->chosen_points = iter->oom_score;
+		}
+
+		for (parent = parent_mem_cgroup(iter); parent && parent != root;
+		     parent = parent_mem_cgroup(parent)) {
+			parent->oom_score += iter->oom_score;
+
+			if (mem_cgroup_oom_group(parent) &&
+			    parent->oom_score > oc->chosen_points) {
+				oc->chosen_memcg = parent;

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

Quoted 10 lines

mm/memcontrol.c +10 -0

+				oc->chosen_points = parent->oom_score;
+			}
+		}
+	}
+
+	if (oc->chosen_memcg && oc->chosen_memcg != INFLIGHT_VICTIM)
+		css_get(&oc->chosen_memcg->css);
+
+	rcu_read_unlock();
+}

As I've written in a private email, things will get much easier if you get rid of memcg->oom_score and simply do the recursive oom_score evaluation of eligible inter nodes. You would basically do

	for_each_mem_cgroup_tree(root, iter) {
		if (!memcg_oom_eligible(iter))
			continue;

		oom_score = oom_evaluate_memcg(iter, mask);
		if (oom_score == -1) {
			oc->chosen_memcg = INFLIGHT_VICTIM;
			mem_cgroup_iter_break(root, iter);
			break;
		}
		if (oom_score > oc->chosen_points) {
			mark_new_oom_memcg(iter);
		}

		/* potential optimization to skip the whole subtree if
		 * iter is not leaf */
	}

where
bool memcg_oom_eligible(struct mem_cgroup *memcg)

{
	if (cgroup_has_tasks(memcg->css.cgroup))
		return true;
	if (mem_cgroup_oom_group(memcg))
		return true;
	return false;
}

unsigned long __oom_evaluate_memcg(struct mem_cgroup *memcg, mask)

{
	/* check eligible tasks - oom victims OOM_SCORE_ADJ_MIN */
	/* calculate badness */
}

unsigned long oom_evaluate_memcg(struct mem_cgroup *memcg, mask)

{
	unsigned long score = 0;

	if (memcg == root_mem_cgroup) {
		for_each_task()
			score += __oom_badness(task, mask);
		return score
	}

	for_each_mem_cgroup_tree(memcg, iter) {
		unsigned long memcg_score = __oom_evaluate_memcg(iter, mask);
		if (memcg_score == -1) {
			mem_cgroup_iter_break(memcg, iter);
			return -1;
		}
	}

	return score;
}

This should be also simple to split for oom_group in a separate patch while keeping the overall code structure.
Does this make any sense to you?

[...]

mm/oom_kill.c static void oom_kill_process(struct oom_control *oc, const char *message) · line 973

Quoted 7 lines

mm/oom_kill.c +3 -0

@@ -962,6 +968,48 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
 	__oom_kill_process(victim);
 }
 
+static int oom_kill_memcg_member(struct task_struct *task, void *unused)
+{
+	if (!tsk_is_oom_victim(task)) {

How can this happen?

Quoted 21 lines

mm/oom_kill.c +20 -0

+		get_task_struct(task);
+		__oom_kill_process(task);
+	}
+	return 0;
+}
+
+static bool oom_kill_memcg_victim(struct oom_control *oc)
+{
+	static DEFINE_RATELIMIT_STATE(oom_rs, DEFAULT_RATELIMIT_INTERVAL,
+				      DEFAULT_RATELIMIT_BURST);
+
+	if (oc->chosen_memcg == NULL || oc->chosen_memcg == INFLIGHT_VICTIM)
+		return oc->chosen_memcg;
+
+	/* Always begin with the task 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)

mm/oom_kill.c static void oom_kill_process(struct oom_control *oc, const char *message) · line 997

Quoted 4 lines

mm/oom_kill.c +4 -0

+		goto out;
+
+	if (__ratelimit(&oom_rs))
+		dump_header(oc, oc->chosen_task);

Hmm, does the full dump_header really apply for the new heuristic? E.g. does it make sense to dump_tasks()? Would it make sense to print stats of all eligible memcgs instead?

mm/oom_kill.c static void oom_kill_process(struct oom_control *oc, const char *message) · line 1006

Quoted 10 lines

mm/oom_kill.c +9 -0

+
+	__oom_kill_process(oc->chosen_task);
+
+	/* If oom_group flag is set, kill all belonging tasks */
+	if (mem_cgroup_oom_group(oc->chosen_memcg))
+		mem_cgroup_scan_tasks(oc->chosen_memcg, oom_kill_memcg_member,
+				      NULL);
+
+	schedule_timeout_killable(1);

I would prefer if we had this timeout at a single place in out_of_memory()

Other than that the semantic (sans oom_group which needs more clarification) makes sense to me.

On Tue, Oct 03, 2017 at 01:48:48PM +0200, Michal Hocko wrote:

Quoted 18 lines

Wrt. to the implicit inheritance you brought up in a separate email thread [1]. Let me quote
: after some additional thinking I don't think anymore that implicit : propagation of oom_group is a good idea. Let me explain: assume we : have memcg A with memory.max and memory.oom_group set, and nested : memcg A/B with memory.max set. Let's imagine we have an OOM event if : A/B. What is an expected system behavior?
: We have OOM scoped to A/B, and any action should be also scoped to A/B. : We really shouldn't touch processes which are not belonging to A/B. : That means we should either kill the biggest process in A/B, either all : processes in A/B. It's natural to make A/B/memory.oom_group responsible : for this decision. It's strange to make the depend on A/memory.oom_group, IMO. : It really makes no sense, and makes oom_group knob really hard to describe. :
: Also, after some off-list discussion, we've realized that memory.oom_knob : should be delegatable. The workload should have control over it to express : dependency between processes.

Quoted 5 lines

OK, I have asked about this already but I am not sure the answer was very explicit. So let me ask again. When exactly a subtree would disagree with the parent on oom_group? In other words when do we want a different cleanup based on the OOM root? I am not saying this is wrong I am just curious about a practical example.

Well, I do not have a practical example right now, but it's against the logic. Any OOM event has a scope, and group_oom knob is applied for OOM events scoped to the cgroup or any ancestors (including system as a whole). So, applying it implicitly to OOM scoped to descendant cgroups makes no sense. It's a strange configuration limitation, and I do not see any benefits:
it doesn't provide any new functionality or guarantees.

Even if we don't have practical examples, we should build something less surprising for a user, and I don't understand why oom_group should be inherited.

Quoted 23 lines

Quoted 8 lines

Tasks with oom_score_adj set to -1000 are considered as unkillable.

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

[1] http://lkml.kernel.org/r/[email protected]

[...]

Quoted 11 lines

mm/memcontrol.c +11 -0

+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

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	 * of the belonging tasks.
+	 */

Why not a sum of all tasks which would more resemble what we do for other memcgs? Sure this would require ignoring oom_score_adj so oom_badness would have to be tweaked a bit (basically split it into __oom_badness which calculates the value without the bias and oom_badness on top adding the bias on top of the scaled value).

We've discussed it already: calculating the sum is tricky, as tasks are sharing memory (and the mm struct(. As I remember, you suggested using maximum to solve exactly this problem, and I think it's a good approximation. Assuming that tasks in the root cgroup likely have nothing in common, and we don't support oom_group for it, looking at the biggest task makes perfect sense: we're exactly comparing killable entities.

Quoted 9 lines

mm/oom_kill.c static void oom_kill_process(struct oom_control *oc, const char *message) · line 973

Quoted 7 lines

mm/oom_kill.c +3 -0

@@ -962,6 +968,48 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
 	__oom_kill_process(victim);
 }
 
+static int oom_kill_memcg_member(struct task_struct *task, void *unused)
+{
+	if (!tsk_is_oom_victim(task)) {

How can this happen?

We do start with killing the largest process, and then iterate over all tasks in the cgroup. So, this check is required to avoid killing tasks which are already in the termination process.

Quoted 21 lines

Quoted 21 lines

mm/oom_kill.c +20 -0

+		get_task_struct(task);
+		__oom_kill_process(task);
+	}
+	return 0;
+}
+
+static bool oom_kill_memcg_victim(struct oom_control *oc)
+{
+	static DEFINE_RATELIMIT_STATE(oom_rs, DEFAULT_RATELIMIT_INTERVAL,
+				      DEFAULT_RATELIMIT_BURST);
+
+	if (oc->chosen_memcg == NULL || oc->chosen_memcg == INFLIGHT_VICTIM)
+		return oc->chosen_memcg;
+
+	/* Always begin with the task 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)

Quoted 8 lines

mm/oom_kill.c static void oom_kill_process(struct oom_control *oc, const char *message) · line 997

Quoted 4 lines

mm/oom_kill.c +4 -0

+		goto out;
+
+	if (__ratelimit(&oom_rs))
+		dump_header(oc, oc->chosen_task);

Hmm, does the full dump_header really apply for the new heuristic? E.g. does it make sense to dump_tasks()? Would it make sense to print stats of all eligible memcgs instead?

Hm, this is a tricky part: the dmesg output is at some point a part of ABI,

but is also closely connected with the implementation. So I would suggest to postpone this until we'll get more usage examples and will better understand what information we need.

Quoted 1 line · Roman Gushchin

On Tue, Oct 03, 2017 at 01:48:48PM +0200, Michal Hocko wrote:

[...]

Quoted 24 lines

Quoted 18 lines

Wrt. to the implicit inheritance you brought up in a separate email thread [1]. Let me quote
: after some additional thinking I don't think anymore that implicit : propagation of oom_group is a good idea. Let me explain: assume we : have memcg A with memory.max and memory.oom_group set, and nested : memcg A/B with memory.max set. Let's imagine we have an OOM event if : A/B. What is an expected system behavior?
: We have OOM scoped to A/B, and any action should be also scoped to A/B. : We really shouldn't touch processes which are not belonging to A/B. : That means we should either kill the biggest process in A/B, either all : processes in A/B. It's natural to make A/B/memory.oom_group responsible : for this decision. It's strange to make the depend on A/memory.oom_group, IMO. : It really makes no sense, and makes oom_group knob really hard to describe. :
: Also, after some off-list discussion, we've realized that memory.oom_knob : should be delegatable. The workload should have control over it to express : dependency between processes.

Quoted 5 lines

OK, I have asked about this already but I am not sure the answer was very explicit. So let me ask again. When exactly a subtree would disagree with the parent on oom_group? In other words when do we want a different cleanup based on the OOM root? I am not saying this is wrong I am just curious about a practical example.

Quoted 6 lines

Well, I do not have a practical example right now, but it's against the logic. Any OOM event has a scope, and group_oom knob is applied for OOM events scoped to the cgroup or any ancestors (including system as a whole). So, applying it implicitly to OOM scoped to descendant cgroups makes no sense. It's a strange configuration limitation, and I do not see any benefits:
it doesn't provide any new functionality or guarantees.

Well, I guess I agree. I was merely interested about consequences when the oom behavior is different depending on which layer it happens. Does it make sense to cleanup the whole hierarchy while any subtree would kill a single task if the oom happened there?

Quoted 3 lines

Even if we don't have practical examples, we should build something less surprising for a user, and I don't understand why oom_group should be inherited.

I guess we want to inherit the value on the memcg creation but I agree that enforcing parent setting is weird. I will think about it some more but I agree that it is saner to only enforce per memcg value.

Quoted 33 lines

Quoted 24 lines

Quoted 9 lines

Tasks with oom_score_adj set to -1000 are considered as unkillable.

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

[1] http://lkml.kernel.org/r/[email protected]

[...]

Quoted 11 lines

mm/memcontrol.c +11 -0

+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

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	 * of the belonging tasks.
+	 */

Why not a sum of all tasks which would more resemble what we do for other memcgs? Sure this would require ignoring oom_score_adj so oom_badness would have to be tweaked a bit (basically split it into __oom_badness which calculates the value without the bias and oom_badness on top adding the bias on top of the scaled value).

Quoted 7 lines

We've discussed it already: calculating the sum is tricky, as tasks are sharing memory (and the mm struct(. As I remember, you suggested using maximum to solve exactly this problem, and I think it's a good approximation. Assuming that tasks in the root cgroup likely have nothing in common, and we don't support oom_group for it, looking at the biggest task makes perfect sense: we're exactly comparing killable entities.

Please add a comment explaining that. I hope we can make root memcg less special eventually. It shouldn't be all that hard. We already have per LRU numbers and we only use few counters which could be accounted to the root memcg as well. Counters should be quite cheap.

[...]

Quoted 10 lines

Quoted 9 lines

mm/oom_kill.c static void oom_kill_process(struct oom_control *oc, const char *message) · line 973

Quoted 7 lines

mm/oom_kill.c +3 -0

@@ -962,6 +968,48 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
 	__oom_kill_process(victim);
 }
 
+static int oom_kill_memcg_member(struct task_struct *task, void *unused)
+{
+	if (!tsk_is_oom_victim(task)) {

How can this happen?

Quoted 3 lines

We do start with killing the largest process, and then iterate over all tasks in the cgroup. So, this check is required to avoid killing tasks which are already in the termination process.

Do you mean we have tsk_is_oom_victim && MMF_OOM_SKIP == T?

Quoted 31 lines

Quoted 22 lines

Quoted 22 lines

mm/oom_kill.c +20 -0

+		get_task_struct(task);
+		__oom_kill_process(task);
+	}
+	return 0;
+}
+
+static bool oom_kill_memcg_victim(struct oom_control *oc)
+{
+	static DEFINE_RATELIMIT_STATE(oom_rs, DEFAULT_RATELIMIT_INTERVAL,
+				      DEFAULT_RATELIMIT_BURST);
+
+	if (oc->chosen_memcg == NULL || oc->chosen_memcg == INFLIGHT_VICTIM)
+		return oc->chosen_memcg;
+
+	/* Always begin with the task 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)

Quoted 8 lines

mm/oom_kill.c static void oom_kill_process(struct oom_control *oc, const char *message) · line 997

Quoted 4 lines

mm/oom_kill.c +4 -0

+		goto out;
+
+	if (__ratelimit(&oom_rs))
+		dump_header(oc, oc->chosen_task);

Hmm, does the full dump_header really apply for the new heuristic? E.g. does it make sense to dump_tasks()? Would it make sense to print stats of all eligible memcgs instead?

Quoted 1 line

Hm, this is a tricky part: the dmesg output is at some point a part of ABI,

People are parsing oom reports but I disagree this is an ABI of any sort. The report is closely tight to the particular implementation and as such it has changed several times over the time.

Quoted 3 lines

but is also closely connected with the implementation. So I would suggest to postpone this until we'll get more usage examples and will better understand what information we need.

I would drop tasks list at least because that is clearly misleading in this context because we are not selecting from all tasks. We are selecting between memcgs. The memcg information can be added in a separate patch of course.

On Tue, Oct 03, 2017 at 03:36:23PM +0200, Michal Hocko wrote:

Quoted 3 lines

I guess we want to inherit the value on the memcg creation but I agree that enforcing parent setting is weird. I will think about it some more but I agree that it is saner to only enforce per memcg value.

I'm not against, but we should come up with a good explanation, why we're inheriting it; or not inherit.

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

mm/oom_kill.c static void oom_kill_process(struct oom_control *oc, const char *message) · line 973

Quoted 7 lines

mm/oom_kill.c +3 -0

@@ -962,6 +968,48 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
 	__oom_kill_process(victim);
 }
 
+static int oom_kill_memcg_member(struct task_struct *task, void *unused)
+{
+	if (!tsk_is_oom_victim(task)) {

How can this happen?

Quoted 5 lines

Quoted 3 lines

We do start with killing the largest process, and then iterate over all tasks in the cgroup. So, this check is required to avoid killing tasks which are already in the termination process.

Do you mean we have tsk_is_oom_victim && MMF_OOM_SKIP == T?

No, just tsk_is_oom_victim. We're are killing the biggest task, and then _all_ tasks. This is a way to skip the biggest task, and do not kill it again.

Quoted 14 lines

Quoted 5 lines

Quoted 3 lines

Hmm, does the full dump_header really apply for the new heuristic? E.g. does it make sense to dump_tasks()? Would it make sense to print stats of all eligible memcgs instead?

Hm, this is a tricky part: the dmesg output is at some point a part of ABI,

People are parsing oom reports but I disagree this is an ABI of any sort. The report is closely tight to the particular implementation and as such it has changed several times over the time.

Quoted 3 lines

but is also closely connected with the implementation. So I would suggest to postpone this until we'll get more usage examples and will better understand what information we need.

Quoted 4 lines

I would drop tasks list at least because that is clearly misleading in this context because we are not selecting from all tasks. We are selecting between memcgs. The memcg information can be added in a separate patch of course.

Let's postpone it until we'll land the rest of the patchset.

Quoted 1 line · Roman Gushchin

On Tue, Oct 03, 2017 at 03:36:23PM +0200, Michal Hocko wrote:

[...]

Quoted 4 lines

Quoted 3 lines

I guess we want to inherit the value on the memcg creation but I agree that enforcing parent setting is weird. I will think about it some more but I agree that it is saner to only enforce per memcg value.

Quoted 2 lines

I'm not against, but we should come up with a good explanation, why we're inheriting it; or not inherit.

Inheriting sounds like a less surprising behavior. Once you opt in for oom_group you can expect that descendants are going to assume the same unless they explicitly state otherwise.

Hello, Michal.

Quoted 7 lines · Michal Hocko

Quoted 1 line · Roman Gushchin

On Tue, Oct 03, 2017 at 03:36:23PM +0200, Michal Hocko wrote:

[...]

Quoted 4 lines

Quoted 3 lines

I guess we want to inherit the value on the memcg creation but I agree that enforcing parent setting is weird. I will think about it some more but I agree that it is saner to only enforce per memcg value.

Quoted 6 lines

Quoted 2 lines

I'm not against, but we should come up with a good explanation, why we're inheriting it; or not inherit.

Inheriting sounds like a less surprising behavior. Once you opt in for oom_group you can expect that descendants are going to assume the same unless they explicitly state otherwise.

Here's a counter example.

Let's say there's a container which hosts one main application, and the container shares its host with other containers.

* Let's say the container is a regular containerized OS instance and can't really guarantee system integrity if one its processes gets randomly killed.

* However, the application that it's running inside an isolated cgroup is more intelligent and composed of multiple interchangeable processes and can treat killing of a random process as partial capacity loss.

When the host is setting up the outer container, it doesn't necessarily know whether the containerized environment would be able to handle partial OOM kills or not. It's akin to panic_on_oom setting at system level - it's the containerized instance itself which knows whether it can handle partial OOM kills or not. This is why this knob should be delegatable.

Now, the container itself has group OOM set and the isolated main application is starting up. It obviously wants partial OOM kills rather than group killing. This is the same principle. The application which is being contained in the cgroup is the one which knows how it can handle OOM conditions, not the outer environment, so it obviously needs to be able to set the configuration it wants.

Quoted 1 line · Roman Gushchin

On Tue, Oct 03, 2017 at 03:36:23PM +0200, Michal Hocko wrote:

[...]

Quoted 6 lines

Quoted 3 lines

I guess we want to inherit the value on the memcg creation but I agree that enforcing parent setting is weird. I will think about it some more but I agree that it is saner to only enforce per memcg value.

I'm not against, but we should come up with a good explanation, why we're inheriting it; or not inherit.

Inheriting sounds like a less surprising behavior. Once you opt in for oom_group you can expect that descendants are going to assume the same unless they explicitly state otherwise.

[...]

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

mm/oom_kill.c static void oom_kill_process(struct oom_control *oc, const char *message) · line 973

Quoted 7 lines

mm/oom_kill.c +3 -0

@@ -962,6 +968,48 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
 	__oom_kill_process(victim);
 }
 
+static int oom_kill_memcg_member(struct task_struct *task, void *unused)
+{
+	if (!tsk_is_oom_victim(task)) {

How can this happen?

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

We do start with killing the largest process, and then iterate over all tasks in the cgroup. So, this check is required to avoid killing tasks which are already in the termination process.

Do you mean we have tsk_is_oom_victim && MMF_OOM_SKIP == T?

No, just tsk_is_oom_victim. We're are killing the biggest task, and then _all_ tasks. This is a way to skip the biggest task, and do not kill it again.

OK, I have missed that part. Why are we doing that actually? Why don't we simply do

	/* If oom_group flag is set, kill all belonging tasks */
	if (mem_cgroup_oom_group(oc->chosen_memcg))
		mem_cgroup_scan_tasks(oc->chosen_memcg, oom_kill_memcg_member,
				      NULL);

we are going to kill all the tasks anyway.

Quoted 41 lines · Michal Hocko

Quoted 1 line · Roman Gushchin

On Tue, Oct 03, 2017 at 03:36:23PM +0200, Michal Hocko wrote:

[...]

Quoted 6 lines

Quoted 3 lines

I guess we want to inherit the value on the memcg creation but I agree that enforcing parent setting is weird. I will think about it some more but I agree that it is saner to only enforce per memcg value.

I'm not against, but we should come up with a good explanation, why we're inheriting it; or not inherit.

Inheriting sounds like a less surprising behavior. Once you opt in for oom_group you can expect that descendants are going to assume the same unless they explicitly state otherwise.

[...]

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Review static void oom_kill_process(struct oom_control *oc, const char *message) · line 973

Quoted 7 lines

(quoted diff) +3 -0

@@ -962,6 +968,48 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
 	__oom_kill_process(victim);
 }
 
+static int oom_kill_memcg_member(struct task_struct *task, void *unused)
+{
+	if (!tsk_is_oom_victim(task)) {

How can this happen?

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

We do start with killing the largest process, and then iterate over all tasks in the cgroup. So, this check is required to avoid killing tasks which are already in the termination process.

Do you mean we have tsk_is_oom_victim && MMF_OOM_SKIP == T?

No, just tsk_is_oom_victim. We're are killing the biggest task, and then _all_ tasks. This is a way to skip the biggest task, and do not kill it again.

OK, I have missed that part. Why are we doing that actually? Why don't we simply do

	/* If oom_group flag is set, kill all belonging tasks */
	if (mem_cgroup_oom_group(oc->chosen_memcg))
		mem_cgroup_scan_tasks(oc->chosen_memcg, oom_kill_memcg_member,
				      NULL);

Quoted 1 line

we are going to kill all the tasks anyway.

Well, the idea behind was that killing the biggest process give us better chances to get out of global memory shortage and guarantee forward progress. I can drop it, if it considered to be excessive.

Quoted 1 line · Roman Gushchin

On Tue, Oct 03, 2017 at 03:36:23PM +0200, Michal Hocko wrote:

[...]

Quoted 4 lines

Quoted 3 lines

I guess we want to inherit the value on the memcg creation but I agree that enforcing parent setting is weird. I will think about it some more but I agree that it is saner to only enforce per memcg value.

Quoted 2 lines

I'm not against, but we should come up with a good explanation, why we're inheriting it; or not inherit.

Inheriting sounds like a less surprising behavior. Once you opt in for oom_group you can expect that descendants are going to assume the same unless they explicitly state otherwise.

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 3e55bbd31ad1..cae5343a8b21 100644
@@ -80,6 +80,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 c3421ee0d230..8d8aa46ff930 100644
@@ -1709,6 +1709,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);

@@ -1725,6 +1728,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;
 	}
 }
 

@@ -1732,6 +1740,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 353bb590713e..3f82b6f22d63 100644
@@ -2852,6 +2852,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.13.5

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.

I do not have a strong preference about this. I would just be worried that it is usually systemd which tries to own the whole hierarchy

Quoted 7 lines · Michal Hocko

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.

Quoted 2 lines

I do not have a strong preference about this. I would just be worried that it is usually systemd which tries to own the whole hierarchy

I actually like this fact.

It gives us the opportunity to change the default behavior for most users at the point when we'll be sure that new behavior is better; but at the same time we'll save full compatibility on the kernel level.