cgroup-aware OOM killer

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

Acked-by Michal Hocko <[email protected]>

Acked-by David Rientjes <[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]

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

Acked-by 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 +2 -3

index d5f3a62887cf..b4de17a78dc1 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;

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 <[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]

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

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.

Quoted 13 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]

This looks good to me.

Acked-by Johannes Weiner <[email protected]>

I just have one question:

Quoted 4 lines

mm/oom_kill.c +0 -0

@@ -828,6 +828,12 @@ static void __oom_kill_process(struct task_struct *victim)
 	struct mm_struct *mm;
 	bool can_oom_reap = true;
 

mm/oom_kill.c static void __oom_kill_process(struct task_struct *victim) · line 839

Quoted 9 lines

mm/oom_kill.c +6 -0

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

Is this necessary? The callers of this function use oom_badness() to find a victim, and that filters init, kthread, OOM_SCORE_ADJ_MIN.

Quoted 4 lines

Quoted 4 lines

mm/oom_kill.c +0 -0

@@ -828,6 +828,12 @@ static void __oom_kill_process(struct task_struct *victim)
 	struct mm_struct *mm;
 	bool can_oom_reap = true;
 

Quoted 12 lines

mm/oom_kill.c static void __oom_kill_process(struct task_struct *victim) · line 839

Quoted 9 lines

mm/oom_kill.c +6 -0

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

Is this necessary? The callers of this function use oom_badness() to find a victim, and that filters init, kthread, OOM_SCORE_ADJ_MIN.

It is. __oom_kill_process() is used to kill all processes belonging to the selected memory cgroup, so we should perform these checks to avoid killing unkillable processes.

Quoted 17 lines · Roman Gushchin

Quoted 4 lines

Quoted 4 lines

mm/oom_kill.c +0 -0

@@ -828,6 +828,12 @@ static void __oom_kill_process(struct task_struct *victim)
 	struct mm_struct *mm;
 	bool can_oom_reap = true;
 

Quoted 12 lines

mm/oom_kill.c static void __oom_kill_process(struct task_struct *victim) · line 839

Quoted 9 lines

mm/oom_kill.c +6 -0

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

Is this necessary? The callers of this function use oom_badness() to find a victim, and that filters init, kthread, OOM_SCORE_ADJ_MIN.

Quoted 4 lines

It is. __oom_kill_process() is used to kill all processes belonging to the selected memory cgroup, so we should perform these checks to avoid killing unkillable processes.

That's only true after the next patch in the series which uses the oom_kill_memcg_member() callback to kill processes for oom_group, correct? Would it be possible to move this check to that patch so it's more obvious?

Quoted 17 lines · Roman Gushchin

Quoted 4 lines

Quoted 4 lines

mm/oom_kill.c +0 -0

@@ -828,6 +828,12 @@ static void __oom_kill_process(struct task_struct *victim)
 	struct mm_struct *mm;
 	bool can_oom_reap = true;
 

Quoted 12 lines

mm/oom_kill.c static void __oom_kill_process(struct task_struct *victim) · line 839

Quoted 9 lines

mm/oom_kill.c +6 -0

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

Is this necessary? The callers of this function use oom_badness() to find a victim, and that filters init, kthread, OOM_SCORE_ADJ_MIN.

Quoted 4 lines

It is. __oom_kill_process() is used to kill all processes belonging to the selected memory cgroup, so we should perform these checks to avoid killing unkillable processes.

That's only true after the next patch in the series which uses the oom_kill_memcg_member() callback to kill processes for oom_group, correct? Would it be possible to move this check to that patch so it's more obvious?

Quoted 24 lines · David Rientjes

Quoted 17 lines · Roman Gushchin

Quoted 4 lines

Quoted 4 lines

mm/oom_kill.c +0 -0

@@ -828,6 +828,12 @@ static void __oom_kill_process(struct task_struct *victim)
 	struct mm_struct *mm;
 	bool can_oom_reap = true;
 

Quoted 12 lines

mm/oom_kill.c static void __oom_kill_process(struct task_struct *victim) · line 839

Quoted 9 lines

mm/oom_kill.c +6 -0

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

Is this necessary? The callers of this function use oom_badness() to find a victim, and that filters init, kthread, OOM_SCORE_ADJ_MIN.

Quoted 4 lines

It is. __oom_kill_process() is used to kill all processes belonging to the selected memory cgroup, so we should perform these checks to avoid killing unkillable processes.

Quoted 4 lines

That's only true after the next patch in the series which uses the oom_kill_memcg_member() callback to kill processes for oom_group, correct? Would it be possible to move this check to that patch so it's more obvious?

Yup, I realized it when reviewing the next patch. Moving this hunk to the next patch would probably make sense. Although, us reviewers have been made aware of this now, so I don't feel strongly about it. Won't make much of a difference once the patches are merged.

mm/memcontrol.c +8 -0

+
+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 +9 -0

+
+               score = oom_evaluate_memcg(iter, oc->nodemask, oc->totalpages);
+
+               /*
+                * Ignore empty and non-eligible memory cgroups.
+                */
+               if (score == 0)
+                       continue;
+

Quoted 12 lines

(quoted diff) +12 -0

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

Review

Quoted 6 lines

(quoted diff) +6 -0

+       rcu_read_lock();
+       for_each_mem_cgroup_tree(iter, root) {
+               long score;
+
+               if (memcg_has_children(iter))
+                       continue;

&& iter != root_mem_cgroup ?

Quoted 16 lines

(quoted diff) +15 -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;

Review

Quoted 4 lines

(quoted diff) +4 -0

+                       mem_cgroup_iter_break(root, iter);
+                       break;
+               }
+

Shouldn't there be a CSS_ONLINE check? Also instead of css_get at the end why not css_tryget_online() here and css_put for the previous selected one.

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+               if (memcg_has_children(iter))
+                       continue;

&& iter != root_mem_cgroup ?

Oh, sure. I had a stupid bug in my test script, which prevented me from catching this. Thanks!

This should fix the problem.
--

mm/memcontrol.c +0 -0

index 2e82625bd354..b3848bce4c86 100644

Quoted 4 lines

Shouldn't there be a CSS_ONLINE check? Also instead of css_get at the end why not css_tryget_online() here and css_put for the previous selected one.

Hm, why do we need to check this? I do not see, how we can choose an OFFLINE memcg as a victim, tbh. Please, explain the problem.

Quoted 17 lines

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+               if (memcg_has_children(iter))
+                       continue;

&& iter != root_mem_cgroup ?

Oh, sure. I had a stupid bug in my test script, which prevented me from catching this. Thanks!

This should fix the problem.
--

mm/memcontrol.c +0 -0

index 2e82625bd354..b3848bce4c86 100644
@@ -2807,7 +2807,8 @@ static void select_victim_memcg(struct mem_cgroup *root, struct oom_control *oc)
                 * We don't consider non-leaf non-oom_group memory cgroups
                 * as OOM victims.

mm/memcontrol.c static void select_victim_memcg(struct mem_cgroup *root, struct oom_control *oc) · line 2812

Quoted 5 lines

mm/memcontrol.c +2 -1

                 */
-               if (memcg_has_children(iter) && !mem_cgroup_oom_group(iter))
+               if (memcg_has_children(iter) && iter != root_mem_cgroup &&
+                   !mem_cgroup_oom_group(iter))
                        continue;

I think you are mixing the 3rd and 4th patch. The root_mem_cgroup check should be in 3rd while oom_group stuff should be in 4th.

Quoted 6 lines

Quoted 5 lines

Shouldn't there be a CSS_ONLINE check? Also instead of css_get at the end why not css_tryget_online() here and css_put for the previous selected one.

Quoted 3 lines

Hm, why do we need to check this? I do not see, how we can choose an OFFLINE memcg as a victim, tbh. Please, explain the problem.

Sorry about the confusion. There are two things. First, should we do a css_get on the newly selected memcg within the for loop when we still have a reference to it?

Second, for the OFFLINE memcg, you are right oom_evaluate_memcg() will return 0 for offlined memcgs. Maybe no need to call oom_evaluate_memcg() for offlined memcgs.

On Wed, Oct 04, 2017 at 02:24:26PM -0700, Shakeel Butt wrote:

Quoted 3 lines

Sorry about the confusion. There are two things. First, should we do a css_get on the newly selected memcg within the for loop when we still have a reference to it?

We're holding rcu_read_lock, it should be enough. We're bumping css counter just before releasing rcu lock.

Quoted 4 lines

Second, for the OFFLINE memcg, you are right oom_evaluate_memcg() will return 0 for offlined memcgs. Maybe no need to call oom_evaluate_memcg() for offlined memcgs.

Sounds like a good optimization, which can be done on top of the current patchset.

Quoted 1 line · Roman Gushchin

On Wed, Oct 04, 2017 at 02:24:26PM -0700, Shakeel Butt wrote:

[...]

Quoted 6 lines

Quoted 3 lines

Sorry about the confusion. There are two things. First, should we do a css_get on the newly selected memcg within the for loop when we still have a reference to it?

We're holding rcu_read_lock, it should be enough. We're bumping css counter just before releasing rcu lock.

yes

Quoted 5 lines

Quoted 4 lines

Second, for the OFFLINE memcg, you are right oom_evaluate_memcg() will return 0 for offlined memcgs. Maybe no need to call oom_evaluate_memcg() for offlined memcgs.

Quoted 2 lines

Sounds like a good optimization, which can be done on top of the current patchset.

You could achive this by checking whether a memcg has tasks rather than explicitly checking for children memcgs as I've suggested already.

mm/memcontrol.c +142 -0

index b4de17a78dc1..79f30c281185 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))
+			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;

By only considering leaf memcgs, does this penalize users if their memcg becomes oc->chosen_memcg purely because it has aggregated all of its processes to be members of that memcg, which would otherwise be the standard behavior?

What prevents me from spreading my memcg with N processes attached over N child memcgs instead so that memcg_oom_badness() becomes very small for each child memcg specifically to avoid being oom killed?

Quoted 5 lines

By only considering leaf memcgs, does this penalize users if their memcg becomes oc->chosen_memcg purely because it has aggregated all of its processes to be members of that memcg, which would otherwise be the standard behavior?

Quoted 3 lines

What prevents me from spreading my memcg with N processes attached over N child memcgs instead so that memcg_oom_badness() becomes very small for each child memcg specifically to avoid being oom killed?

It's no different from forking out multiple mm to avoid being the biggest process.

It's up to the parent to enforce limits on that group and prevent you from being able to cause global OOM in the first place, in particular if you delegate to untrusted and potentially malicious users.

Quoted 5 lines · Johannes Weiner

Quoted 5 lines

By only considering leaf memcgs, does this penalize users if their memcg becomes oc->chosen_memcg purely because it has aggregated all of its processes to be members of that memcg, which would otherwise be the standard behavior?

Quoted 7 lines

Quoted 3 lines

What prevents me from spreading my memcg with N processes attached over N child memcgs instead so that memcg_oom_badness() becomes very small for each child memcg specifically to avoid being oom killed?

It's no different from forking out multiple mm to avoid being the biggest process.

It is, because it can quite clearly be a DoSand was prevented with Roman's earlier design of iterating usage up the hierarchy and comparing siblings based on that criteria. I know exactly why he chose that implementation detail early on, and it was to prevent cases such as this and to not let userspace hide from the oom killer.

Quoted 4 lines

It's up to the parent to enforce limits on that group and prevent you from being able to cause global OOM in the first place, in particular if you delegate to untrusted and potentially malicious users.

Let's resolve that global oom is a real condition and getting into that situation is not a userspace problem. It's the result of overcommiting the system, and is used in the enterprise to address business goals. If the above is true, and its up to memcg to prevent global oom in the first place, then this entire patchset is absolutely pointless. Limit userspace to 95% of memory and when usage is approaching that limit, let userspace attached to the root memcg iterate the hierarchy itself and kill from the largest consumer.

This patchset exists because overcommit is real, exactly the same as overcommit within memcg hierarchies is real. 99% of the time we don't run into global oom because people aren't using their limits so it just works out. 1% of the time we run into global oom and we need a decision to made based for forward progress. Using Michal's earlier example of admins and students, a student can easily use all of his limit and also, with v10 of this patchset, 99% of the time avoid being oom killed just by forking N processes over N cgroups. It's going to oom kill an admin every single time.

Quoted 5 lines

It is, because it can quite clearly be a DoSand was prevented with Roman's earlier design of iterating usage up the hierarchy and comparing siblings based on that criteria. I know exactly why he chose that implementation detail early on, and it was to prevent cases such as this and to not let userspace hide from the oom killer.

This doesn't address how it's different from a single process following the same pattern right now.

Quoted 9 lines

Let's resolve that global oom is a real condition and getting into that situation is not a userspace problem. It's the result of overcommiting the system, and is used in the enterprise to address business goals. If the above is true, and its up to memcg to prevent global oom in the first place, then this entire patchset is absolutely pointless. Limit userspace to 95% of memory and when usage is approaching that limit, let userspace attached to the root memcg iterate the hierarchy itself and kill from the largest consumer.

Quoted 9 lines

This patchset exists because overcommit is real, exactly the same as overcommit within memcg hierarchies is real. 99% of the time we don't run into global oom because people aren't using their limits so it just works out. 1% of the time we run into global oom and we need a decision to made based for forward progress. Using Michal's earlier example of admins and students, a student can easily use all of his limit and also, with v10 of this patchset, 99% of the time avoid being oom killed just by forking N processes over N cgroups. It's going to oom kill an admin every single time.

We overcommit too, but our workloads organize themselves based on managing their resources, not based on evading the OOM killer. I'd wager that's true for many if not most users.

Quoted 5 lines · Johannes Weiner

Quoted 5 lines

By only considering leaf memcgs, does this penalize users if their memcg becomes oc->chosen_memcg purely because it has aggregated all of its processes to be members of that memcg, which would otherwise be the standard behavior?

Quoted 7 lines

Quoted 3 lines

What prevents me from spreading my memcg with N processes attached over N child memcgs instead so that memcg_oom_badness() becomes very small for each child memcg specifically to avoid being oom killed?

It's no different from forking out multiple mm to avoid being the biggest process.

It is, because it can quite clearly be a DoS, and was prevented with Roman's earlier design of iterating usage up the hierarchy and comparing siblings based on that criteria. I know exactly why he chose that implementation detail early on, and it was to prevent cases such as this and to not let userspace hide from the oom killer.

Quoted 4 lines

It's up to the parent to enforce limits on that group and prevent you from being able to cause global OOM in the first place, in particular if you delegate to untrusted and potentially malicious users.

Let's resolve that global oom is a real condition and getting into that situation is not a userspace problem. It's the result of overcommiting the system, and is used in the enterprise to address business goals. If the above is true, and its up to memcg to prevent global oom in the first place, then this entire patchset is absolutely pointless. Limit userspace to 95% of memory and when usage is approaching that limit, let userspace attached to the root memcg iterate the hierarchy itself and kill from the largest consumer.

This patchset exists because overcommit is real, exactly the same as overcommit within memcg hierarchies is real. 99% of the time we don't run into global oom because people aren't using their limits so it just works out. 1% of the time we run into global oom and we need a decision to made based for forward progress. Using Michal's earlier example of admins and students, a student can easily use all of his limit and also, with v10 of this patchset, 99% of the time avoid being oom killed just by forking N processes over N cgroups. It's going to oom kill an admin every single time.

I know exactly why earlier versions of this patchset iterated that usage up the tree so you would pick from students, pick from this troublemaking student, and then oom kill from his hierarchy. Roman has made that point himself. My suggestion was to add userspace influence to it so that enterprise users and users with business goals can actually define that we really do want 80% of memory to be used by this process or this hierarchy, it's in our best interest.

Earlier iterations of this patchset did this, and did it correctly. Userspace influence over the decisionmaking makes it a very powerful combination because you _can_ specify what your goals are or choose to leave the priorities as default so you can compare based solely on usage. It was a beautiful solution to the problem.

Quoted 9 lines

This patchset exists because overcommit is real, exactly the same as overcommit within memcg hierarchies is real. 99% of the time we don't run into global oom because people aren't using their limits so it just works out. 1% of the time we run into global oom and we need a decision to made based for forward progress. Using Michal's earlier example of admins and students, a student can easily use all of his limit and also, with v10 of this patchset, 99% of the time avoid being oom killed just by forking N processes over N cgroups. It's going to oom kill an admin every single time.

Overcommit is real, but configuring the system in a way that system-wide OOM happens often is a strange idea.

As we all know, the system can barely work
adequate under global memory shortage: network packets are dropped, latency is bad, weird kernel issues are revealed periodically, etc.
I do not see, why you can't overcommit on deeper layers of cgroup hierarchy, avoiding system-wide OOM to happen.

Quoted 7 lines

I know exactly why earlier versions of this patchset iterated that usage up the tree so you would pick from students, pick from this troublemaking student, and then oom kill from his hierarchy. Roman has made that point himself. My suggestion was to add userspace influence to it so that enterprise users and users with business goals can actually define that we really do want 80% of memory to be used by this process or this hierarchy, it's in our best interest.

I'll repeat myself: I believe that there is a range of possible policies:
from a complete flat (what Johannes did suggested few weeks ago), to a very hierarchical (as in v8). Each with their pros and cons.
(Michal did provide a clear example of bad behavior of the hierarchical approach).

I assume, that v10 is a good middle point, and it's good because it doesn't prevent further development. Just for example, you can introduce a third state of oom_group knob, which will mean "evaluate as a whole, but do not kill all". And this is what will solve your particular case, right?

I would need to add patches to add the "evaluate as a whole but do not kill all" knob and a knob for "oom priority" so that userspace has the same influence over a cgroup based comparison that it does with a process based comparison to meet business goals.

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 <[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]

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.

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]

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.

static void select_victim_memcg(struct mem_cgroup *root, struct oom_control *oc) {

mm/memcontrol.c +2 -1

-	struct mem_cgroup *iter;
+	struct mem_cgroup *iter, *group = NULL;
+	long group_score = 0;

mm/memcontrol.c +15 -0

+		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) && !mem_cgroup_oom_group(iter))
+			continue;
+

mm/oom_kill.c +2 -9

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

mm/oom_kill.c +3 -0

+
+		__oom_kill_process(oc->chosen_task);
+	}
 
 out:
 	mem_cgroup_put(oc->chosen_memcg);

Signature · 1 line

2.13.6

Quoted 15 lines · Roman Gushchin

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.

Quoted 8 lines

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.

I would extend the last sentence with an explanation. What about the following:
"
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.
"

few mostly nit picks below but this looks good other than that. Once the fix mentioned in patch 3 is folded I will ack this.

[...]

Quoted 9 lines

static void select_victim_memcg(struct mem_cgroup *root, struct oom_control *oc) {

mm/memcontrol.c +2 -1

-	struct mem_cgroup *iter;
+	struct mem_cgroup *iter, *group = NULL;
+	long group_score = 0;
 
 	oc->chosen_memcg = NULL;
 	oc->chosen_points = 0;

mm/memcontrol.c static long oom_evaluate_memcg(struct mem_cgroup *memcg, · line 2790

Quoted 6 lines

mm/memcontrol.c +5 -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)) {

we have is_memcg_oom() helper which is esier to read and understand than the explicit oc->memcg check

Quoted 23 lines

mm/memcontrol.c +15 -0

+		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) && !mem_cgroup_oom_group(iter))
+			continue;
+

mm/memcontrol.c static long oom_evaluate_memcg(struct mem_cgroup *memcg, · line 2822

Quoted 10 lines

mm/memcontrol.c +10 -0

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

hmm, I thought you would go with a recursive oom_evaluate_memcg implementation that would result in a more readable code IMHO. It is true that we would traverse oom_group more times. But I do not expect we would have very deep memcg hierarchies in the majority of workloads and even if we did then this is a cold path which should focus on readability more than a performance. Also implementing mem_cgroup_iter_skip_subtree shouldn't be all that hard if this ever turns out a real problem.

Anyway this is nothing really fundamental so I will leave the decision on you.

Quoted 14 lines

mm/oom_kill.c +2 -9

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

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

Quoted 10 lines

mm/oom_kill.c +10 -0

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

it took me a while to realize we need this because of return !!oc->chosen_task in out_of_memory. Subtle... Also a reason to hate oc->chosen_* thingy. As I've said in other reply, don't worry about this I will probably turn my hate into a patch ;)

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

Quoted 8 lines

mm/oom_kill.c +8 -0

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

How can this happen? There shouldn't be any INFLIGHT_VICTIM in our memcg because we have checked for that already. I can see how we do not find any task because those can terminate by the time we get here but no new oom victim should appear we are under the oom_lock.

Quoted 25 lines · Michal Hocko

Quoted 15 lines · Roman Gushchin

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.

Quoted 8 lines

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.

Quoted 9 lines

I would extend the last sentence with an explanation. What about the following:
"
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.
"

Added, thanks!

Quoted 15 lines

few mostly nit picks below but this looks good other than that. Once the fix mentioned in patch 3 is folded I will ack this.

[...]

Quoted 9 lines

static void select_victim_memcg(struct mem_cgroup *root, struct oom_control *oc) {

mm/memcontrol.c +2 -1

-	struct mem_cgroup *iter;
+	struct mem_cgroup *iter, *group = NULL;
+	long group_score = 0;
 
 	oc->chosen_memcg = NULL;
 	oc->chosen_points = 0;

Quoted 9 lines

mm/memcontrol.c static long oom_evaluate_memcg(struct mem_cgroup *memcg, · line 2790

Quoted 6 lines

mm/memcontrol.c +5 -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)) {

we have is_memcg_oom() helper which is esier to read and understand than the explicit oc->memcg check

It's defined in oom_kill.c and not exported, so I'm not sure.

Quoted 33 lines

Quoted 22 lines

mm/memcontrol.c +15 -0

+		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) && !mem_cgroup_oom_group(iter))
+			continue;
+

mm/memcontrol.c static long oom_evaluate_memcg(struct mem_cgroup *memcg, · line 2822

Quoted 10 lines

mm/memcontrol.c +10 -0

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

Quoted 8 lines

hmm, I thought you would go with a recursive oom_evaluate_memcg implementation that would result in a more readable code IMHO. It is true that we would traverse oom_group more times. But I do not expect we would have very deep memcg hierarchies in the majority of workloads and even if we did then this is a cold path which should focus on readability more than a performance. Also implementing mem_cgroup_iter_skip_subtree shouldn't be all that hard if this ever turns out a real problem.

I've tried to go this way, but I didn't like the result. These both loops will share a lot of code (e.g. breaking on finding a previous victim, skipping non-leaf non-oom-group memcgs, etc), so the result is more messy. And actually it's strange to start a new loop to iterate exactly over the same sub-tree, which you want to skip in the first loop.

Quoted 33 lines

Anyway this is nothing really fundamental so I will leave the decision on you.

Quoted 14 lines

mm/oom_kill.c +2 -9

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

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

Quoted 10 lines

mm/oom_kill.c +10 -0

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

it took me a while to realize we need this because of return !!oc->chosen_task in out_of_memory. Subtle... Also a reason to hate oc->chosen_* thingy. As I've said in other reply, don't worry about this I will probably turn my hate into a patch ;)

Quoted 13 lines

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

Quoted 8 lines

mm/oom_kill.c +8 -0

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

How can this happen? There shouldn't be any INFLIGHT_VICTIM in our memcg because we have checked for that already. I can see how we do not find any task because those can terminate by the time we get here but no new oom victim should appear we are under the oom_lock.

You're probably right, but I would prefer to have this check in place, rather then get a panic on attempt to kill an INFLIGHT_VICTIM task one day.

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 <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/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 +0 -0

index 1fcd6cc353d5..2e82625bd354 100644

That will silently ignore what the user writes to the memory.oom_group control files across the system's cgroup tree.

We'll have a knob that lets the workload declare itself an indivisible memory consumer, that it would like to get killed in one piece, and it's silently ignored because of a mount option they forgot to pass.

That's not good from an interface perspective.

On the other hand, the only benefit of this patch is to shield users from changes to the OOM killing heuristics. Yet, it's really hard to imagine that modifying the victim selection process slightly could be called a regression in any way. We have done that many times over, without a second thought on backwards compatibility:

5e9d834a0e0c oom: sacrifice child with highest badness score for parent a63d83f427fb oom: badness heuristic rewrite
778c14affaf9 mm, oom: base root bonus on current usage

Let's not make the userspace interface crap because of some misguided idea that the OOM heuristic is a hard promise to userspace. It's never been, and nobody has complained about changes in the past.

This case is doubly silly, as the behavior change only applies to cgroup2, which doesn't exactly have a large base of legacy users yet.

On Wed 04-10-17 16:04:53, Johannes Weiner wrote:
[...]

Quoted 7 lines

That will silently ignore what the user writes to the memory.oom_group control files across the system's cgroup tree.

We'll have a knob that lets the workload declare itself an indivisible memory consumer, that it would like to get killed in one piece, and it's silently ignored because of a mount option they forgot to pass.

Quoted 1 line

That's not good from an interface perspective.

Yes and that is why I think a boot time knob would be the most simple way. It will also open doors for more oom policies in future which I believe come sooner or later.

Quoted 10 lines

On the other hand, the only benefit of this patch is to shield users from changes to the OOM killing heuristics. Yet, it's really hard to imagine that modifying the victim selection process slightly could be called a regression in any way. We have done that many times over, without a second thought on backwards compatibility:

5e9d834a0e0c oom: sacrifice child with highest badness score for parent a63d83f427fb oom: badness heuristic rewrite
778c14affaf9 mm, oom: base root bonus on current usage

yes we have changed that without a deeper considerations. Some of those changes are arguable (e.g. child scarification). The oom badness heuristic rewrite has triggered quite some complains AFAIR (I remember Kosaki has made several attempts to revert it). I think that we are trying to be more careful about user visible changes than we used to be.

More importantly I do not think that the current (non-memcg aware) OOM policy is somehow obsolete and many people expect it to behave consistently. As I've said already, I have seen many complains that the OOM killer doesn't kill the right task. Most of them were just NUMA related issues where the oom report was not clear enough. I do not want to repeat that again now. Memcg awareness is certainly a useful heuristic but I do not see it universally applicable to all workloads.

Quoted 4 lines

Let's not make the userspace interface crap because of some misguided idea that the OOM heuristic is a hard promise to userspace. It's never been, and nobody has complained about changes in the past.

Quoted 2 lines

This case is doubly silly, as the behavior change only applies to cgroup2, which doesn't exactly have a large base of legacy users yet.

I agree on the interface part but I disagree with making it default just because v2 is not largerly adopted yet.

Quoted 11 lines · Michal Hocko

On Wed 04-10-17 16:04:53, Johannes Weiner wrote:
[...]

Quoted 8 lines

That will silently ignore what the user writes to the memory.oom_group control files across the system's cgroup tree.

We'll have a knob that lets the workload declare itself an indivisible memory consumer, that it would like to get killed in one piece, and it's silently ignored because of a mount option they forgot to pass.

That's not good from an interface perspective.

Quoted 3 lines

Yes and that is why I think a boot time knob would be the most simple way. It will also open doors for more oom policies in future which I believe come sooner or later.

So, we would rely on grub config to set up OOM policy? Sounds weird.

We use boot options, when it's hard to implement on the fly switching (like turning on/off socket memory accounting), but here is not this case.

Quoted 31 lines

Quoted 9 lines

On the other hand, the only benefit of this patch is to shield users from changes to the OOM killing heuristics. Yet, it's really hard to imagine that modifying the victim selection process slightly could be called a regression in any way. We have done that many times over, without a second thought on backwards compatibility:

5e9d834a0e0c oom: sacrifice child with highest badness score for parent a63d83f427fb oom: badness heuristic rewrite
778c14affaf9 mm, oom: base root bonus on current usage

yes we have changed that without a deeper considerations. Some of those changes are arguable (e.g. child scarification). The oom badness heuristic rewrite has triggered quite some complains AFAIR (I remember Kosaki has made several attempts to revert it). I think that we are trying to be more careful about user visible changes than we used to be.

More importantly I do not think that the current (non-memcg aware) OOM policy is somehow obsolete and many people expect it to behave consistently. As I've said already, I have seen many complains that the OOM killer doesn't kill the right task. Most of them were just NUMA related issues where the oom report was not clear enough. I do not want to repeat that again now. Memcg awareness is certainly a useful heuristic but I do not see it universally applicable to all workloads.

Quoted 6 lines

Let's not make the userspace interface crap because of some misguided idea that the OOM heuristic is a hard promise to userspace. It's never been, and nobody has complained about changes in the past.

This case is doubly silly, as the behavior change only applies to cgroup2, which doesn't exactly have a large base of legacy users yet.

Quoted 2 lines

I agree on the interface part but I disagree with making it default just because v2 is not largerly adopted yet.

I believe that the only real regression can be caused by active using of oom_score_adj. I really don't know how many cgroup v2 users are relying on it (hopefully, 0).

So, personally I would prefer to have an opt-out cgroup v2 mount option (sane new behavior for most users, 100% backward compatibility for rare strange setups), but I don't have a very strong opinion here.

On Wed 04-10-17 16:04:53, Johannes Weiner wrote:
[...]

Quoted 7 lines

That will silently ignore what the user writes to the memory.oom_group control files across the system's cgroup tree.

We'll have a knob that lets the workload declare itself an indivisible memory consumer, that it would like to get killed in one piece, and it's silently ignored because of a mount option they forgot to pass.

Quoted 1 line

That's not good from an interface perspective.

Yes and that is why I think a boot time knob would be the most simple way. It will also open doors for more oom policies in future which I believe come sooner or later.

Quoted 9 lines

On the other hand, the only benefit of this patch is to shield users from changes to the OOM killing heuristics. Yet, it's really hard to imagine that modifying the victim selection process slightly could be called a regression in any way. We have done that many times over, without a second thought on backwards compatibility:

5e9d834a0e0c oom: sacrifice child with highest badness score for parent a63d83f427fb oom: badness heuristic rewrite
778c14affaf9 mm, oom: base root bonus on current usage

yes we have changed that without a deeper considerations. Some of those changes are arguable (e.g. child scarification). The oom badness heuristic rewrite has triggered quite some complains AFAIR (I remember Kosaki has made several attempts to revert it). I think that we are trying to be more careful about user visible changes than we used to be.

Quoted 11 lines · Michal Hocko

On Wed 04-10-17 16:04:53, Johannes Weiner wrote:
[...]

Quoted 8 lines

That will silently ignore what the user writes to the memory.oom_group control files across the system's cgroup tree.

We'll have a knob that lets the workload declare itself an indivisible memory consumer, that it would like to get killed in one piece, and it's silently ignored because of a mount option they forgot to pass.

That's not good from an interface perspective.

Quoted 3 lines

Yes and that is why I think a boot time knob would be the most simple way. It will also open doors for more oom policies in future which I believe come sooner or later.

A boot time knob makes less sense to me than the mount option. It doesn't require a reboot to change this behavior, we shouldn't force the user to reboot when a runtime configuration is possible.

But I don't see how dropping this patch as part of this series would prevent adding modular oom policies in the future?

That said, selectable OOM policies sound like a total deadend to me. The kernel OOM happens way too late to be useful for any kind of resource policy already. Even now it won't prevent you from thrashing indefinitely, with only 5% of your workload's time spent productively.

What kind of service quality do you have at this point?

The *minority* of our OOM situations (in terms of "this isn't making real progress anymore due to a lack of memory") is even *seeing* OOM kills at this point. And it'll get worse as storage gets faster and memory bigger.

How is that useful as a resource arbitration point?

Then there is the question of reliability. I mean, we still don't have a global OOM killer that is actually free from deadlocks.

We don't
have reserves measured to the exact requirements of reclaim that would guarantee recovery, the OOM reaper requires a lock that we hope isn't taken, etc. I wouldn't want any of my fleet to rely on this for regular operation - I'm just glad that, when we do mess up and hit this event, we don't have to reboot.

It makes much more sense to monitor memory pressure from userspace and smartly intervene when things turn unproductive, which is a long way from the point where the kernel is about to *deadlock* due to memory.

Global OOM kills can still happen, but their goal should really be 1) to save the kernel, 2) respect the integrity of a memory consumer and
3) be comprehensible to userspace. (These patches are about 2 and 3.)

But abstracting such a rudimentary and fragile deadlock avoidance mechanism into higher-level resource management, or co-opting it as a policy enforcement tool, is crazy to me.

And it seems reckless to present it as those things to our users by encoding any such elaborate policy interfaces.

Quoted 10 lines

Quoted 9 lines

On the other hand, the only benefit of this patch is to shield users from changes to the OOM killing heuristics. Yet, it's really hard to imagine that modifying the victim selection process slightly could be called a regression in any way. We have done that many times over, without a second thought on backwards compatibility:

5e9d834a0e0c oom: sacrifice child with highest badness score for parent a63d83f427fb oom: badness heuristic rewrite
778c14affaf9 mm, oom: base root bonus on current usage

Quoted 5 lines

yes we have changed that without a deeper considerations. Some of those changes are arguable (e.g. child scarification). The oom badness heuristic rewrite has triggered quite some complains AFAIR (I remember Kosaki has made several attempts to revert it). I think that we are trying to be more careful about user visible changes than we used to be.

Whatever grumbling might have come up, it has not resulted in a revert or a way to switch back to the old behavior. So I don't think this can be considered an actual regression.

We change heuristics in the MM all the time. If you track for example allocator behavior over different kernel versions, you can see how much our caching policy, our huge page policy etc. fluctuates. The impact of that is way bigger to regular workloads than how we go about choosing an OOM victim.

We don't want to regress anybody, but let's also keep perspective here and especially consider the userspace interfaces we are willing to put in for at least the next few years, the promises we want to make, the further fragmentation of the config space, for such a negligible risk.

Yes and that is why I think a boot time knob would be the most simple way. It will also open doors for more oom policies in future which I believe come sooner or later.

Document the cgroup-aware OOM killer.

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 Andrew Morton <akpm-de/[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]