mm, oom: refactor the oom_kill_process() function

include/linux/oom.h +3 -0

index 8a266e2be5a6..b7ec3bd441be 100644
@@ -39,6 +39,7 @@ struct oom_control {
 	unsigned long totalpages;
 	struct task_struct *chosen;
 	unsigned long chosen_points;
+	struct mem_cgroup *chosen_memcg;
 };
 
 extern struct mutex oom_lock;

@@ -79,6 +80,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 +12 -0

index df6f63ee95d6..0b81dc55c6ac 100644
@@ -2639,6 +2639,181 @@ 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)
+{
+	long points = 0;
+	int nid;
+
+	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));

mm/memcontrol.c +9 -0

+
+static long oom_evaluate_memcg(struct mem_cgroup *memcg,
+			       const nodemask_t *nodemask)
+{
+	struct css_task_iter it;
+	struct task_struct *task;
+	int elegible = 0;
+
+	css_task_iter_start(&memcg->css, 0, &it);

Quoted 9 lines · Roman Gushchin

mm/memcontrol.c +9 -0

+
+static long oom_evaluate_memcg(struct mem_cgroup *memcg,
+			       const nodemask_t *nodemask)
+{
+	struct css_task_iter it;
+	struct task_struct *task;
+	int elegible = 0;
+
+	css_task_iter_start(&memcg->css, 0, &it);

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

Quoted 10 lines

mm/memcontrol.c +10 -0

+	while ((task = css_task_iter_next(&it))) {
+		/*
+		 * If there are no tasks, or all tasks have oom_score_adj set
+		 * to OOM_SCORE_ADJ_MIN and oom_kill_all_tasks is not set,
+		 * don't select this memory cgroup.
+		 */
+		if (!elegible &&
+		    (memcg->oom_kill_all_tasks ||
+		     task->signal->oom_score_adj != OOM_SCORE_ADJ_MIN))
+			elegible = 1;

I'm curious about the decision made in this conditional and how oom_kill_memcg_member() ignores task->signal->oom_score_adj. It means that memory.oom_kill_all_tasks overrides /proc/pid/oom_score_adj if it should otherwise be disabled.

It's undocumented in the changelog, but I'm questioning whether it's the right decision. Doesn't it make sense to kill all tasks that are not oom disabled, and allow the user to still protect certain processes by their /proc/pid/oom_score_adj setting? Otherwise, there's no way to do that protection without a sibling memcg and its own reservation of memory. I'm thinking about a process that governs jobs inside the memcg and if there is an oom kill, it wants to do logging and any cleanup necessary before exiting itself. It seems like a powerful combination if coupled with oom notification.

Also, s/elegible/eligible/

Otherwise, looks good!

Generally, oom_score_adj should have a meaning only on a cgroup level, so extending it to the system level doesn't sound as a good idea.

Quoted 2 lines · Roman Gushchin

Generally, oom_score_adj should have a meaning only on a cgroup level, so extending it to the system level doesn't sound as a good idea.

But wasn't the original purpose of oom_score (and oom_score_adj) to work on a system level, aka "normal" OOM? Is there some peculiarity about memcg OOM that I'm missing?

Quoted 5 lines

I'm curious about the decision made in this conditional and how oom_kill_memcg_member() ignores task->signal->oom_score_adj. It means that memory.oom_kill_all_tasks overrides /proc/pid/oom_score_adj if it should otherwise be disabled.

Quoted 9 lines

It's undocumented in the changelog, but I'm questioning whether it's the right decision. Doesn't it make sense to kill all tasks that are not oom disabled, and allow the user to still protect certain processes by their /proc/pid/oom_score_adj setting? Otherwise, there's no way to do that protection without a sibling memcg and its own reservation of memory. I'm thinking about a process that governs jobs inside the memcg and if there is an oom kill, it wants to do logging and any cleanup necessary before exiting itself. It seems like a powerful combination if coupled with oom notification.

Good question!
I think, that an ability to override any oom_score_adj value and get all tasks killed is more important, than an ability to kill all processes with some exceptions.

In your example someone still needs to look after the remaining process, and kill it after some timeout, if it will not quit by itself, right?

The special treatment of the -1000 value (without oom_kill_all_tasks) is required only to not to break the existing setups.

Quoted 15 lines · Roman Gushchin

Quoted 5 lines

I'm curious about the decision made in this conditional and how oom_kill_memcg_member() ignores task->signal->oom_score_adj. It means that memory.oom_kill_all_tasks overrides /proc/pid/oom_score_adj if it should otherwise be disabled.

Quoted 9 lines

It's undocumented in the changelog, but I'm questioning whether it's the right decision. Doesn't it make sense to kill all tasks that are not oom disabled, and allow the user to still protect certain processes by their /proc/pid/oom_score_adj setting? Otherwise, there's no way to do that protection without a sibling memcg and its own reservation of memory. I'm thinking about a process that governs jobs inside the memcg and if there is an oom kill, it wants to do logging and any cleanup necessary before exiting itself. It seems like a powerful combination if coupled with oom notification.

Quoted 5 lines

Good question!
I think, that an ability to override any oom_score_adj value and get all tasks killed is more important, than an ability to kill all processes with some exceptions.

I'm disagreeing because getting all tasks killed is not necessarily something that only the kernel can do. If all processes are oom disabled, that's a configuration issue done by sysadmin and the kernel should decide to kill the next largest memory cgroup or lower priority memory cgroup. It's not killing things like sshd that intentionally oom disable themselves.

You could argue that having an oom disabled process attached to these memcgs in the first place is also a configuration issue, but the problem is that in cgroup v2 with a restriction on processes only being attached at the leaf cgroups that there is no competition for memory in this case. I must assign memory resources to that sshd, or "Activity Manager" described by the cgroup v1 documentation, just to prevent it from being killed.

I think the latter of what you describe, killing all processes with some exceptions, is actually quite powerful. I can guarantee that processes that set themselves to oom disabled are really oom disabled and I don't need to work around that in the cgroup hierarchy only because of this caveat. I can also oom disable my Activity Manger that wants to wait on oom notification and collect the oom kill logs, raise notifications, and perhaps restart the process that it manage.

Quoted 3 lines

In your example someone still needs to look after the remaining process, and kill it after some timeout, if it will not quit by itself, right?

No, it can restart the process that was oom killed; or it can be sshd and I can still ssh into my machine.

Quoted 3 lines

The special treatment of the -1000 value (without oom_kill_all_tasks) is required only to not to break the existing setups.

I think as a general principle that allowing an oom disabled process to be oom killed is incorrect and if you really do want these to be killed, then
(1) either your oom_score_adj is already wrong or (2) you can wait on oom notification and exit.

It's natural to expect that inside a container there are their own sshd, "activity manager" or some other stuff, which can play with oom_score_adj. If it can override the upper cgroup-level settings, the whole delegation model is broken.

You can think about the oom_kill_all_tasks like the panic_on_oom, but on a cgroup level. It should _guarantee_, that in case of oom the whole cgroup will be destroyed completely, and will not remain in a non-consistent state.

The model you're describing is based on a trust given to these oom-unkillable processes on system level. But we can't really trust some unknown processes inside a cgroup that they will be able to do some useful work and finish in a reasonable time; especially in case of a global memory shortage.

Quoted 5 lines · Roman Gushchin

It's natural to expect that inside a container there are their own sshd, "activity manager" or some other stuff, which can play with oom_score_adj. If it can override the upper cgroup-level settings, the whole delegation model is broken.

I don't think any delegation model related to core cgroups or memory cgroup is broken, I think it's based on how memory.oom_kill_all_tasks is defined. It could very well behave as memory.oom_kill_all_eligible_tasks when enacted upon.

Quoted 5 lines

You can think about the oom_kill_all_tasks like the panic_on_oom, but on a cgroup level. It should _guarantee_, that in case of oom the whole cgroup will be destroyed completely, and will not remain in a non-consistent state.

Only CAP_SYS_ADMIN has this ability to set /proc/pid/oom_score_adj to

OOM_SCORE_ADJ_MIN, so it preserves the ability to change that setting, if needed, when it sets memory.oom_kill_all_tasks. If a user gains permissions to change memory.oom_kill_all_tasks, I disagree it should override the CAP_SYS_ADMIN setting of /proc/pid/oom_score_adj.

I would prefer not to exclude oom disabled processes to their own sibling cgroups because they would require their own reservation with cgroup v2 and it makes the single hierarchy model much more difficult to arrange alongside cpusets, for example.

Quoted 4 lines

The model you're describing is based on a trust given to these oom-unkillable processes on system level. But we can't really trust some unknown processes inside a cgroup that they will be able to do some useful work and finish in a reasonable time; especially in case of a global memory shortage.

Yes, we prefer to panic instead of sshd, for example, being oom killed. We trust that sshd, as well as our own activity manager and security daemons are trusted to do useful work and that we never want the kernel to do this. I'm not sure why you are describing processes that CAP_SYS_ADMIN has set to be oom disabled as unknown processes.

I'd be interested in hearing the opinions of others related to a per-memcg knob being allowed to override the setting of the sysadmin.

mm/memcontrol.c +7 -0

@@ -2639,6 +2639,181 @@ 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)
+{
+	long points = 0;
+	int nid;
+
+	for_each_node_state(nid, N_MEMORY) {

mm/memcontrol.c +65 -0

+		for (parent = parent_mem_cgroup(iter); parent && parent != root;
+		     parent = parent_mem_cgroup(parent))
+			parent->oom_score += iter->oom_score;
+	}
+
+	for (;;) {
+		struct cgroup_subsys_state *css;
+		struct mem_cgroup *memcg = NULL;
+		long score = LONG_MIN;
+
+		css_for_each_child(css, &root->css) {
+			struct mem_cgroup *iter = mem_cgroup_from_css(css);
+
+			if (iter->oom_score > score) {
+				memcg = iter;
+				score = iter->oom_score;
+			}
+		}
+
+		if (!memcg) {
+			if (oc->memcg && root == oc->memcg) {
+				oc->chosen_memcg = oc->memcg;
+				css_get(&oc->chosen_memcg->css);
+				oc->chosen_points = oc->memcg->oom_score;
+			}
+			break;
+		}
+
+		if (memcg->oom_kill_all_tasks || !memcg_has_children(memcg)) {
+			oc->chosen_memcg = memcg;
+			css_get(&oc->chosen_memcg->css);
+			oc->chosen_points = score;
+			break;
+		}
+
+		root = memcg;
+	}
+}
+
+static void select_victim_root_cgroup_task(struct oom_control *oc)
+{
+	struct css_task_iter it;
+	struct task_struct *task;
+	int ret = 0;
+
+	css_task_iter_start(&root_mem_cgroup->css, 0, &it);
+	while (!ret && (task = css_task_iter_next(&it)))
+		ret = oom_evaluate_task(task, oc);
+	css_task_iter_end(&it);
+}
+
+bool mem_cgroup_select_oom_victim(struct oom_control *oc)
+{
+	struct mem_cgroup *root = root_mem_cgroup;
+
+	oc->chosen = NULL;
+	oc->chosen_memcg = NULL;
+
+	if (mem_cgroup_disabled())
+		return false;
+
+	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys))
+		return false;
+
+	if (oc->memcg)

mm/memcontrol.c +6 -0

+	/*
+	 * For system-wide OOMs we should consider tasks in the root cgroup
+	 * with oom_score larger than oc->chosen_points.
+	 */
+	if (!oc->memcg) {
+		select_victim_root_cgroup_task(oc);

mm/memcontrol.c +27 -0

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

mm/memcontrol.c +1 -0

+	{
 		.name = "events",
 		.flags = CFTYPE_NOT_ON_ROOT,
 		.file_offset = offsetof(struct mem_cgroup, events_file),

mm/oom_kill.c +20 -1

index 5c29a3dd591b..28e42a0d5eee 100644
@@ -288,7 +288,7 @@ static enum oom_constraint constrained_alloc(struct oom_control *oc)
 	return CONSTRAINT_NONE;
 }
 
-static int oom_evaluate_task(struct task_struct *task, void *arg)
+int oom_evaluate_task(struct task_struct *task, void *arg)
 {
 	struct oom_control *oc = arg;
 	unsigned long points;

@@ -823,6 +823,9 @@ static void __oom_kill_process(struct task_struct *victim)
 	struct mm_struct *mm;
 	bool can_oom_reap = true;
 
+	if (is_global_init(victim) || (victim->flags & PF_KTHREAD))
+		return;
+
 	p = find_lock_task_mm(victim);
 	if (!p) {
 		put_task_struct(victim);

@@ -958,6 +961,60 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
 	put_task_struct(victim);
 }
 
+static int oom_kill_memcg_member(struct task_struct *task, void *unused)
+{
+	if (!tsk_is_oom_victim(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) {
+		if (oc->chosen != (void *)-1UL) {
+			if (__ratelimit(&oom_rs))
+				dump_header(oc, oc->chosen);

Hi Roman,

great work! This looks mostly good to me now. Below are some nitpicks concerning naming and code layout, but nothing major.

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

Quoted 12 lines

mm/memcontrol.c +12 -0

+
+	css_task_iter_start(&memcg->css, 0, &it);
+	while ((task = css_task_iter_next(&it))) {
+		/*
+		 * If there are no tasks, or all tasks have oom_score_adj set
+		 * to OOM_SCORE_ADJ_MIN and oom_kill_all_tasks is not set,
+		 * don't select this memory cgroup.
+		 */
+		if (!elegible &&
+		    (memcg->oom_kill_all_tasks ||
+		     task->signal->oom_score_adj != OOM_SCORE_ADJ_MIN))
+			elegible = 1;

This is a little awkward to read. How about something like this:

	/*
	 * When killing individual tasks, we respect OOM score adjustments:
	 * at least one task in the group needs to be killable for the group
	 * to be oomable.
	 *
	 * Also check that previous OOM kills have finished, and abort if
	 * there are any pending OOM victims.
	 */
	oomable = memcg->oom_kill_all_tasks;
	while ((task = css_task_iter_next(&it))) {
		if (!oomable && task->signal_oom_score_adj != OOM_SCORE_ADJ_MIN)
			oomable = 1;

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

Quoted 7 lines

mm/memcontrol.c +7 -0

+		if (tsk_is_oom_victim(task) &&
+		    !test_bit(MMF_OOM_SKIP, &task->signal->oom_mm->flags)) {
+			elegible = -1;
+			break;
+		}
+	}
+	css_task_iter_end(&it);

etc.

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	return elegible > 0 ? memcg_oom_badness(memcg, nodemask) : elegible;

I find these much easier to read if broken up, even if it's more LOC:

	if (eligible <= 0)
		return eligible;

	return memcg_oom_badness(memcg, nodemask);

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

Quoted 12 lines

mm/memcontrol.c +12 -0

+static void select_victim_memcg(struct mem_cgroup *root, struct oom_control *oc)
+{
+	struct mem_cgroup *iter, *parent;
+
+	for_each_mem_cgroup_tree(iter, root) {
+		if (memcg_has_children(iter)) {
+			iter->oom_score = 0;
+			continue;
+		}
+
+		iter->oom_score = oom_evaluate_memcg(iter, oc->nodemask);
+		if (iter->oom_score == -1) {

Please add comments to document the special returns. Maybe #defines would be clearer, too.

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

Quoted 7 lines

mm/memcontrol.c +7 -0

+			oc->chosen_memcg = (void *)-1UL;
+			mem_cgroup_iter_break(root, iter);
+			return;
+		}
+
+		if (!iter->oom_score)
+			continue;

Same here.

Maybe a switch would be suitable to handle the abort/no-score cases.

mm/memcontrol.c static struct cftype memory_files[] = { · line 5519

Quoted 12 lines

static int memory_events_show(struct seq_file *m, void *v) {

mm/memcontrol.c +5 -0

 	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(m));

@@ -5310,6 +5512,12 @@ static struct cftype memory_files[] = {
 		.write = memory_max_write,
 	},
 	{
+		.name = "oom_kill_all_tasks",
+		.flags = CFTYPE_NOT_ON_ROOT,
+		.seq_show = memory_oom_kill_all_tasks_show,
+		.write = memory_oom_kill_all_tasks_write,
+	},

This name is quite a mouthful and reminiscent of the awkward v1 interface names. It doesn't really go well with the v2 names.

How about memory.oom_group?

Quoted 27 lines · Johannes Weiner

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

Quoted 11 lines

mm/memcontrol.c +11 -0

+	css_task_iter_start(&memcg->css, 0, &it);
+	while ((task = css_task_iter_next(&it))) {
+		/*
+		 * If there are no tasks, or all tasks have oom_score_adj set
+		 * to OOM_SCORE_ADJ_MIN and oom_kill_all_tasks is not set,
+		 * don't select this memory cgroup.
+		 */
+		if (!elegible &&
+		    (memcg->oom_kill_all_tasks ||
+		     task->signal->oom_score_adj != OOM_SCORE_ADJ_MIN))
+			elegible = 1;

This is a little awkward to read. How about something like this:

	/*
	 * When killing individual tasks, we respect OOM score adjustments:
	 * at least one task in the group needs to be killable for the group
	 * to be oomable.
	 *
	 * Also check that previous OOM kills have finished, and abort if
	 * there are any pending OOM victims.
	 */
	oomable = memcg->oom_kill_all_tasks;
	while ((task = css_task_iter_next(&it))) {
		if (!oomable && task->signal_oom_score_adj != OOM_SCORE_ADJ_MIN)
			oomable = 1;

Quoted 7 lines

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

Quoted 7 lines

mm/memcontrol.c +7 -0

+		if (tsk_is_oom_victim(task) &&
+		    !test_bit(MMF_OOM_SKIP, &task->signal->oom_mm->flags)) {
+			elegible = -1;
+			break;
+		}
+	}
+	css_task_iter_end(&it);

We ignore oom_score_adj if oom_kill_all_tasks is set, it's not reflected in your version. Anyway, I've moved the comments block outside and rephrased it to make more clear.

Quoted 26 lines

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

Quoted 12 lines

mm/memcontrol.c +12 -0

+static void select_victim_memcg(struct mem_cgroup *root, struct oom_control *oc)
+{
+	struct mem_cgroup *iter, *parent;
+
+	for_each_mem_cgroup_tree(iter, root) {
+		if (memcg_has_children(iter)) {
+			iter->oom_score = 0;
+			continue;
+		}
+
+		iter->oom_score = oom_evaluate_memcg(iter, oc->nodemask);
+		if (iter->oom_score == -1) {

Please add comments to document the special returns. Maybe #defines would be clearer, too.

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

Quoted 7 lines

mm/memcontrol.c +7 -0

+			oc->chosen_memcg = (void *)-1UL;
+			mem_cgroup_iter_break(root, iter);
+			return;
+		}
+
+		if (!iter->oom_score)
+			continue;

Same here.

Quoted 1 line

Maybe a switch would be suitable to handle the abort/no-score cases.

Not sure about switch/defines, but I've added several comment blocks to describe possible return values, as well as their handling. Hope, it will be enough.

Quoted 16 lines

mm/memcontrol.c static struct cftype memory_files[] = { · line 5519

Quoted 12 lines

static int memory_events_show(struct seq_file *m, void *v) {

mm/memcontrol.c +5 -0

 	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(m));

@@ -5310,6 +5512,12 @@ static struct cftype memory_files[] = {
 		.write = memory_max_write,
 	},
 	{
+		.name = "oom_kill_all_tasks",
+		.flags = CFTYPE_NOT_ON_ROOT,
+		.seq_show = memory_oom_kill_all_tasks_show,
+		.write = memory_oom_kill_all_tasks_write,
+	},

This name is quite a mouthful and reminiscent of the awkward v1 interface names. It doesn't really go well with the v2 names.

Quoted 1 line

How about memory.oom_group?

I'd prefer to have something more obvious. I've renamed memory.oom_kill_all_tasks to memory.oom_kill_all, which was earlier suggested by Vladimir. Are you ok with it?

Hi,

Quoted 28 lines · Roman Gushchin

Quoted 27 lines · Johannes Weiner

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

Quoted 11 lines

mm/memcontrol.c +11 -0

+	css_task_iter_start(&memcg->css, 0, &it);
+	while ((task = css_task_iter_next(&it))) {
+		/*
+		 * If there are no tasks, or all tasks have oom_score_adj set
+		 * to OOM_SCORE_ADJ_MIN and oom_kill_all_tasks is not set,
+		 * don't select this memory cgroup.
+		 */
+		if (!elegible &&
+		    (memcg->oom_kill_all_tasks ||
+		     task->signal->oom_score_adj != OOM_SCORE_ADJ_MIN))
+			elegible = 1;

This is a little awkward to read. How about something like this:

	/*
	 * When killing individual tasks, we respect OOM score adjustments:
	 * at least one task in the group needs to be killable for the group
	 * to be oomable.
	 *
	 * Also check that previous OOM kills have finished, and abort if
	 * there are any pending OOM victims.
	 */
	oomable = memcg->oom_kill_all_tasks;
	while ((task = css_task_iter_next(&it))) {
		if (!oomable && task->signal_oom_score_adj != OOM_SCORE_ADJ_MIN)
			oomable = 1;

Quoted 11 lines

Quoted 7 lines

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

Quoted 7 lines

mm/memcontrol.c +7 -0

+		if (tsk_is_oom_victim(task) &&
+		    !test_bit(MMF_OOM_SKIP, &task->signal->oom_mm->flags)) {
+			elegible = -1;
+			break;
+		}
+	}
+	css_task_iter_end(&it);

We ignore oom_score_adj if oom_kill_all_tasks is set, it's not reflected in your version. Anyway, I've moved the comments block outside and rephrased it to make more clear.

Yes it is...? We only respect the score if !oomable, which is set to oom_kill_all_tasks.

Quoted 18 lines

Quoted 17 lines

mm/memcontrol.c static struct cftype memory_files[] = { · line 5519

Quoted 12 lines

static int memory_events_show(struct seq_file *m, void *v) {

mm/memcontrol.c +5 -0

 	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(m));

@@ -5310,6 +5512,12 @@ static struct cftype memory_files[] = {
 		.write = memory_max_write,
 	},
 	{
+		.name = "oom_kill_all_tasks",
+		.flags = CFTYPE_NOT_ON_ROOT,
+		.seq_show = memory_oom_kill_all_tasks_show,
+		.write = memory_oom_kill_all_tasks_write,
+	},

This name is quite a mouthful and reminiscent of the awkward v1 interface names. It doesn't really go well with the v2 names.

How about memory.oom_group?

Quoted 3 lines

I'd prefer to have something more obvious. I've renamed memory.oom_kill_all_tasks to memory.oom_kill_all, which was earlier suggested by Vladimir. Are you ok with it?

No, we should be striving for short and sweet mnemonics that express a concept (oom applies to group, not member tasks) instead of underscore sentences that describe an implementation (upon oom, kill all tasks in the group).

It's better to have newbies consult the documentation once than making everybody deal with long and cumbersome names for the rest of time.

Like 'ls' being better than 'read_and_print_directory_contents'.

Quoted 4 lines

It's better to have newbies consult the documentation once than making everybody deal with long and cumbersome names for the rest of time.

Like 'ls' being better than 'read_and_print_directory_contents'.

I don't think it's a good argument here: realistically, nobody will type the knob's name often. Your option is shorter only by 3 characters :)

Anyway, I'm ok with memory.oom_group too, if everybody else prefer it. Michal, David?
What's your opinion?

Introduce a per-memory-cgroup oom_priority setting: an integer number within the [-10000, 10000] range, which defines the order in which the OOM killer selects victim memory cgroups.

OOM killer prefers memory cgroups with larger priority if they are populated with elegible tasks.

The oom_priority value is compared within sibling cgroups.

The root cgroup has the oom_priority 0, which cannot be changed.

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

Cc Michal Hocko <[email protected]>

Cc Vladimir Davydov <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc David Rientjes <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Tetsuo Handa <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Documentation/cgroup-v2.txt +11 -0

+
+	The oom_priority value is compared within sibling cgroups.
+
+	The root cgroup has the oom_priority 0, which cannot be changed.
+
   memory.events
 	A read-only flat-keyed file which exists on non-root cgroups.
 	The following entries are defined.  Unless specified

@@ -1206,6 +1238,36 @@ POSIX_FADV_DONTNEED to relinquish the ownership of memory areas
 belonging to the affected files to ensure correct memory ownership.
 
 
+Cgroup-aware OOM Killer
+~~~~~~~~~~~~~~~~~~~~~~~
+
+Cgroup v2 memory controller implements a cgroup-aware OOM killer.
+It means that it treats memory cgroups as first class OOM entities.
+

Documentation/cgroup-v2.txt +7 -0

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

Documentation/cgroup-v2.txt v1 is available under Documentation/cgroup-v1/. · line 51

Quoted 9 lines · Roman Gushchin

Documentation/cgroup-v2.txt +1 -0

index dec5afdaa36d..22108f31e09d 100644
@@ -48,6 +48,7 @@ v1 is available under Documentation/cgroup-v1/.
        5-2-1. Memory Interface Files
        5-2-2. Usage Guidelines
        5-2-3. Memory Ownership
+       5-2-4. Cgroup-aware OOM Killer

Random curiousness, why cgroup-aware oom killer and not memcg-aware oom killer?

Quoted 8 lines

Documentation/cgroup-v2.txt +0 -0

      5-3. IO
        5-3-1. IO Interface Files
        5-3-2. Writeback

@@ -1002,6 +1003,37 @@ PAGE_SIZE multiple when read back.
 	high limit is used and monitored properly, this limit's
 	utility is limited to providing the final safety net.
 

Documentation/cgroup-v2.txt PAGE_SIZE multiple when read back. · line 1008

Quoted 3 lines

Documentation/cgroup-v2.txt +3 -0

+  memory.oom_kill_all_tasks
+
+	A read-write single value file which exits on non-root

s/exits/exists/

Documentation/cgroup-v2.txt PAGE_SIZE multiple when read back. · line 1012

Quoted 5 lines

Documentation/cgroup-v2.txt +4 -0

+	cgroups.  The default is "0".
+
+	Defines whether the OOM killer should treat the cgroup
+	as a single entity during the victim selection.

Isn't this true independent of the memory.oom_kill_all_tasks setting? The cgroup aware oom killer will consider memcg's as logical units when deciding what to kill with or without memory.oom_kill_all_tasks, right?

I think you cover this fact in the cgroup aware oom killer section below so this might result in confusion if described alongside a setting of memory.oom_kill_all_tasks.

Documentation/cgroup-v2.txt PAGE_SIZE multiple when read back. · line 1015

Quoted 3 lines

Documentation/cgroup-v2.txt +3 -0

+
+	If set, OOM killer will kill all belonging tasks in
+	corresponding cgroup is selected as an OOM victim.

Maybe

"If set, the OOM killer will kill all threads attached to the memcg if selected as an OOM victim."

is better?

Documentation/cgroup-v2.txt PAGE_SIZE multiple when read back. · line 1023

Quoted 9 lines

Documentation/cgroup-v2.txt +8 -0

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

s/exits/exists/

Documentation/cgroup-v2.txt PAGE_SIZE multiple when read back. · line 1031

Quoted 9 lines

Documentation/cgroup-v2.txt +8 -0

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

s/elegible/eligible/

Quoted 24 lines

Documentation/cgroup-v2.txt +16 -0

+
+	The oom_priority value is compared within sibling cgroups.
+
+	The root cgroup has the oom_priority 0, which cannot be changed.
+
   memory.events
 	A read-only flat-keyed file which exists on non-root cgroups.
 	The following entries are defined.  Unless specified

@@ -1206,6 +1238,36 @@ POSIX_FADV_DONTNEED to relinquish the ownership of memory areas
 belonging to the affected files to ensure correct memory ownership.
 
 
+Cgroup-aware OOM Killer
+~~~~~~~~~~~~~~~~~~~~~~~
+
+Cgroup v2 memory controller implements a cgroup-aware OOM killer.
+It means that it treats memory cgroups as first class OOM entities.
+
+Under OOM conditions the memory controller tries to make the best
+choise of a victim, hierarchically looking for the largest memory
+consumer. By default, it will look for the biggest task in the
+biggest leaf cgroup.
+

Documentation/cgroup-v2.txt POSIX_FADV_DONTNEED to relinquish the ownership of memory areas · line 1256

Quoted 5 lines

Documentation/cgroup-v2.txt +5 -0

+Be default, all cgroups have oom_priority 0, and OOM killer will
+chose the largest cgroup recursively on each level. For non-root
+cgroups it's possible to change the oom_priority, and it will cause
+the OOM killer to look athe the priority value first, and compare
+sizes only of cgroups with equal priority.

Maybe some description of "largest" would be helpful here? I think you could briefly describe what is accounted for in the decisionmaking.

s/athe/at the/

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

Quoted 4 lines

Quoted 4 lines

Documentation/cgroup-v2.txt +0 -0

index dec5afdaa36d..22108f31e09d 100644

Quoted 8 lines

Documentation/cgroup-v2.txt v1 is available under Documentation/cgroup-v1/. · line 51

Quoted 5 lines

Documentation/cgroup-v2.txt +1 -0

@@ -48,6 +48,7 @@ v1 is available under Documentation/cgroup-v1/.
        5-2-1. Memory Interface Files
        5-2-2. Usage Guidelines
        5-2-3. Memory Ownership
+       5-2-4. Cgroup-aware OOM Killer

Random curiousness, why cgroup-aware oom killer and not memcg-aware oom killer?

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

Quoted 9 lines

Documentation/cgroup-v2.txt PAGE_SIZE multiple when read back. · line 1012

Quoted 4 lines

Documentation/cgroup-v2.txt +4 -0

+	cgroups.  The default is "0".
+
+	Defines whether the OOM killer should treat the cgroup
+	as a single entity during the victim selection.

Isn't this true independent of the memory.oom_kill_all_tasks setting? The cgroup aware oom killer will consider memcg's as logical units when deciding what to kill with or without memory.oom_kill_all_tasks, right?

Quoted 4 lines

I think you cover this fact in the cgroup aware oom killer section below so this might result in confusion if described alongside a setting of memory.oom_kill_all_tasks.

Quoted 5 lines

Documentation/cgroup-v2.txt PAGE_SIZE multiple when read back. · line 1015

Quoted 2 lines

Documentation/cgroup-v2.txt +2 -0

+	If set, OOM killer will kill all belonging tasks in
+	corresponding cgroup is selected as an OOM victim.

Maybe

Quoted 4 lines

"If set, the OOM killer will kill all threads attached to the memcg if selected as an OOM victim."

is better?

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

Quoted 11 lines

Quoted 11 lines

Documentation/cgroup-v2.txt +11 -0

+Cgroup-aware OOM Killer
+~~~~~~~~~~~~~~~~~~~~~~~
+
+Cgroup v2 memory controller implements a cgroup-aware OOM killer.
+It means that it treats memory cgroups as first class OOM entities.
+
+Under OOM conditions the memory controller tries to make the best
+choise of a victim, hierarchically looking for the largest memory
+consumer. By default, it will look for the biggest task in the
+biggest leaf cgroup.
+

Quoted 8 lines

Documentation/cgroup-v2.txt POSIX_FADV_DONTNEED to relinquish the ownership of memory areas · line 1256

Quoted 5 lines

Documentation/cgroup-v2.txt +5 -0

+Be default, all cgroups have oom_priority 0, and OOM killer will
+chose the largest cgroup recursively on each level. For non-root
+cgroups it's possible to change the oom_priority, and it will cause
+the OOM killer to look athe the priority value first, and compare
+sizes only of cgroups with equal priority.

Maybe some description of "largest" would be helpful here? I think you could briefly describe what is accounted for in the decisionmaking.

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

Quoted 13 lines · Roman Gushchin

Quoted 4 lines

Quoted 4 lines

Documentation/cgroup-v2.txt +0 -0

index dec5afdaa36d..22108f31e09d 100644

Quoted 8 lines

Documentation/cgroup-v2.txt v1 is available under Documentation/cgroup-v1/. · line 51

Quoted 5 lines

Documentation/cgroup-v2.txt +1 -0

@@ -48,6 +48,7 @@ v1 is available under Documentation/cgroup-v1/.
        5-2-1. Memory Interface Files
        5-2-2. Usage Guidelines
        5-2-3. Memory Ownership
+       5-2-4. Cgroup-aware OOM Killer

Random curiousness, why cgroup-aware oom killer and not memcg-aware oom killer?

Quoted 3 lines

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

I think it would be better to not describe it as its own entity, but rather a part of how the memory cgroup works, so simply describing it in section 5-2, perhaps as its own subsection, as how the oom killer works when using the memory cgroup is sufficient. I wouldn't separate it out as a distinct cgroup feature in the documentation.

Quoted 10 lines

Quoted 10 lines

Documentation/cgroup-v2.txt PAGE_SIZE multiple when read back. · line 1012

Quoted 5 lines

Documentation/cgroup-v2.txt +4 -0

+	cgroups.  The default is "0".
+
+	Defines whether the OOM killer should treat the cgroup
+	as a single entity during the victim selection.

Isn't this true independent of the memory.oom_kill_all_tasks setting? The cgroup aware oom killer will consider memcg's as logical units when deciding what to kill with or without memory.oom_kill_all_tasks, right?

Quoted 4 lines

Quoted 4 lines

I think you cover this fact in the cgroup aware oom killer section below so this might result in confusion if described alongside a setting of memory.oom_kill_all_tasks.

I assume this is fixed so that it's documented that memory cgroups are considered logical units by the oom killer and that memory.oom_kill_all_tasks is separate? The former defines the policy on how a memory cgroup is targeted and the latter defines the mechanism it uses to free memory.

Quoted 20 lines

Quoted 11 lines

Quoted 11 lines

Documentation/cgroup-v2.txt +11 -0

+Cgroup-aware OOM Killer
+~~~~~~~~~~~~~~~~~~~~~~~
+
+Cgroup v2 memory controller implements a cgroup-aware OOM killer.
+It means that it treats memory cgroups as first class OOM entities.
+
+Under OOM conditions the memory controller tries to make the best
+choise of a victim, hierarchically looking for the largest memory
+consumer. By default, it will look for the biggest task in the
+biggest leaf cgroup.
+

Quoted 8 lines

Documentation/cgroup-v2.txt POSIX_FADV_DONTNEED to relinquish the ownership of memory areas · line 1256

Quoted 5 lines

Documentation/cgroup-v2.txt +5 -0

+Be default, all cgroups have oom_priority 0, and OOM killer will
+chose the largest cgroup recursively on each level. For non-root
+cgroups it's possible to change the oom_priority, and it will cause
+the OOM killer to look athe the priority value first, and compare
+sizes only of cgroups with equal priority.

Maybe some description of "largest" would be helpful here? I think you could briefly describe what is accounted for in the decisionmaking.

Quoted 3 lines

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

The point is that "largest cgroup" is ambiguous here: largest in what sense? The cgroup with the largest number of processes attached? Using the largest amount of memory?

I think the documentation should clearly define that the oom killer selects the memory cgroup that has the most memory managed at each level.

Hi David!

Please, find an updated version of docs patch below.

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

This commit splits the oom_kill_process() function with an intention to re-use the the second half: __oom_kill_process().

The cgroup-aware OOM killer will kill multiple tasks belonging to the victim cgroup. We don't need to print the debug information for the each task, as well as play with task selection (considering task's children), so we can't use the existing oom_kill_process().

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 Tejun Heo <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

mm/oom_kill.c static bool task_will_free_mem(struct task_struct *task) · line 820

Quoted 6 lines · Roman Gushchin

mm/oom_kill.c +1 -1

@@ -817,67 +817,12 @@ static bool task_will_free_mem(struct task_struct *task)
 	return ret;
 }
 
-static void oom_kill_process(struct oom_control *oc, const char *message)
+static void __oom_kill_process(struct task_struct *victim)

oom_kill_task()?