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

Cc Michal Hocko <[email protected]>

Cc Vladimir Davydov <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Tetsuo Handa <[email protected]>

Cc David Rientjes <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Tejun Heo <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

---
 mm/oom_kill.c | 123 +++++++++++++++++++++++++++++++---------------------------
 1 file changed, 65 insertions(+), 58 deletions(-)

mm/oom_kill.c +65 -58

index 99736e026712..f061b627092c 100644
@@ -804,68 +804,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)
 {
-	struct task_struct *p = oc->chosen;
-	unsigned int points = oc->chosen_points;
-	struct task_struct *victim = p;
-	struct task_struct *child;
-	struct task_struct *t;
+	struct task_struct *p;
 	struct mm_struct *mm;
-	unsigned int victim_points = 0;
-	static DEFINE_RATELIMIT_STATE(oom_rs, DEFAULT_RATELIMIT_INTERVAL,
-					      DEFAULT_RATELIMIT_BURST);
 	bool can_oom_reap = true;
 
-	/*
-	 * If the task is already exiting, don't alarm the sysadmin or kill
-	 * its children or threads, just give it access to memory reserves
-	 * so it can die quickly
-	 */
-	task_lock(p);
-	if (task_will_free_mem(p)) {
-		mark_oom_victim(p);
-		wake_oom_reaper(p);
-		task_unlock(p);
-		put_task_struct(p);
-		return;
-	}
-	task_unlock(p);
-
-	if (__ratelimit(&oom_rs))
-		dump_header(oc, p);
-
-	pr_err("%s: Kill process %d (%s) score %u or sacrifice child\n",
-		message, task_pid_nr(p), p->comm, points);
-
-	/*
-	 * If any of p's children has a different mm and is eligible for kill,
-	 * the one with the highest oom_badness() score is sacrificed for its
-	 * parent.  This attempts to lose the minimal amount of work done while
-	 * still freeing memory.
-	 */
-	read_lock(&tasklist_lock);
-	for_each_thread(p, t) {
-		list_for_each_entry(child, &t->children, sibling) {
-			unsigned int child_points;
-
-			if (process_shares_mm(child, p->mm))
-				continue;
-			/*
-			 * oom_badness() returns 0 if the thread is unkillable
-			 */
-			child_points = oom_badness(child,
-				oc->memcg, oc->nodemask, oc->totalpages);
-			if (child_points > victim_points) {
-				put_task_struct(victim);
-				victim = child;
-				victim_points = child_points;
-				get_task_struct(victim);
-			}
-		}
-	}
-	read_unlock(&tasklist_lock);
-
 	p = find_lock_task_mm(victim);
 	if (!p) {
 		put_task_struct(victim);

@@ -939,6 +883,69 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
 }
 #undef K
 
+static void oom_kill_process(struct oom_control *oc, const char *message)
+{
+	struct task_struct *p = oc->chosen;
+	unsigned int points = oc->chosen_points;
+	struct task_struct *victim = p;
+	struct task_struct *child;
+	struct task_struct *t;
+	unsigned int victim_points = 0;
+	static DEFINE_RATELIMIT_STATE(oom_rs, DEFAULT_RATELIMIT_INTERVAL,
+					      DEFAULT_RATELIMIT_BURST);
+
+	/*
+	 * If the task is already exiting, don't alarm the sysadmin or kill
+	 * its children or threads, just give it access to memory reserves
+	 * so it can die quickly
+	 */
+	task_lock(p);
+	if (task_will_free_mem(p)) {
+		mark_oom_victim(p);
+		wake_oom_reaper(p);
+		task_unlock(p);
+		put_task_struct(p);
+		return;
+	}
+	task_unlock(p);
+
+	if (__ratelimit(&oom_rs))
+		dump_header(oc, p);
+
+	pr_err("%s: Kill process %d (%s) score %u or sacrifice child\n",
+		message, task_pid_nr(p), p->comm, points);
+
+	/*
+	 * If any of p's children has a different mm and is eligible for kill,
+	 * the one with the highest oom_badness() score is sacrificed for its
+	 * parent.  This attempts to lose the minimal amount of work done while
+	 * still freeing memory.
+	 */
+	read_lock(&tasklist_lock);
+	for_each_thread(p, t) {
+		list_for_each_entry(child, &t->children, sibling) {
+			unsigned int child_points;
+
+			if (process_shares_mm(child, p->mm))
+				continue;
+			/*
+			 * oom_badness() returns 0 if the thread is unkillable
+			 */
+			child_points = oom_badness(child,
+				oc->memcg, oc->nodemask, oc->totalpages);
+			if (child_points > victim_points) {
+				put_task_struct(victim);
+				victim = child;
+				victim_points = child_points;
+				get_task_struct(victim);
+			}
+		}
+	}
+	read_unlock(&tasklist_lock);
+
+	__oom_kill_process(victim);
+}
+
 /*
  * Determines whether the kernel must panic because of the panic_on_oom sysctl.
  */

Signature · 1 line

2.13.5

mm/memcontrol.c +3 -0

index a69d23082abf..97813c56163b 100644
@@ -2649,6 +2649,213 @@ 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)
+{

mm/memcontrol.c +12 -0

+
+		iter->oom_score = oom_evaluate_memcg(iter, oc->nodemask);
+
+		/*
+		 * Ignore empty and non-eligible memory cgroups.
+		 */
+		if (iter->oom_score == 0)
+			continue;
+
+		/*
+		 * If there are inflight OOM victims, we don't need to look
+		 * further for new victims.

mm/memcontrol.c +42 -0

+	}
+
+	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);
+
+			/*
+			 * Ignore empty and non-eligible memory cgroups.
+			 */
+			if (iter->oom_score == 0)
+				continue;
+
+			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_group || !memcg_has_children(memcg)) {
+			oc->chosen_memcg = memcg;
+			css_get(&oc->chosen_memcg->css);
+			oc->chosen_points = score;
+			break;
+		}
+
+		root = memcg;
+	}
+}
+

mm/memcontrol.c +0 -0

 	p = find_lock_task_mm(victim);
 	if (!p) {
 		put_task_struct(victim);

On Mon 04-09-17 15:21:05, Roman Gushchin wrote:
[...]

Quoted 17 lines

mm/memcontrol.c +9 -0

index a69d23082abf..97813c56163b 100644
@@ -2649,6 +2649,213 @@ 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;
+	pg_data_t *pgdat;
+
+	for_each_node_state(nid, N_MEMORY) {
+		if (nodemask && !node_isset(nid, *nodemask))

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

Quoted 4 lines

mm/memcontrol.c +4 -0

+			continue;
+
+		points += mem_cgroup_node_nr_lru_pages(memcg, nid,
+				LRU_ALL_ANON | BIT(LRU_UNEVICTABLE));

Why don't you consider file LRUs here? What if there is a lot of page cache which is not reclaimed because it is protected by memcg->low. Should we hide that from the OOM killer?

[...]

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

Quoted 9 lines

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

Do we really need this check? If it is a mere optimization then we should probably check for tasks in the memcg rather than descendant. More on that below.

Quoted 13 lines

mm/memcontrol.c +12 -0

+
+		iter->oom_score = oom_evaluate_memcg(iter, oc->nodemask);
+
+		/*
+		 * Ignore empty and non-eligible memory cgroups.
+		 */
+		if (iter->oom_score == 0)
+			continue;
+
+		/*
+		 * If there are inflight OOM victims, we don't need to look
+		 * further for new victims.

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

Quoted 10 lines

mm/memcontrol.c +10 -0

+		 */
+		if (iter->oom_score == -1) {
+			oc->chosen_memcg = INFLIGHT_VICTIM;
+			mem_cgroup_iter_break(root, iter);
+			return;
+		}
+
+		for (parent = parent_mem_cgroup(iter); parent && parent != root;
+		     parent = parent_mem_cgroup(parent))
+			parent->oom_score += iter->oom_score;

Hmm. The changelog says "By default, it will look for the biggest leaf cgroup, and kill the largest task inside." But you are accumulating oom_score up the hierarchy and so parents will have higher score than the layer of their children and the larger the sub-hierarchy the more biased it will become. Say you have

	root
         /\
        /  \
       A    D
      / \
     B   C

B (5), C(15) thus A(20) and D(20). Unless I am missing something we are going to go down A path and then chose C even though D is the largest leaf group, right?

Quoted 43 lines

mm/memcontrol.c +42 -0

+	}
+
+	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);
+
+			/*
+			 * Ignore empty and non-eligible memory cgroups.
+			 */
+			if (iter->oom_score == 0)
+				continue;
+
+			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_group || !memcg_has_children(memcg)) {
+			oc->chosen_memcg = memcg;
+			css_get(&oc->chosen_memcg->css);
+			oc->chosen_points = score;
+			break;
+		}
+
+		root = memcg;
+	}
+}
+

[...]

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

Quoted 6 lines

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

I do not understand why do we have to handle root cgroup specially here. select_victim_memcg already iterates all memcgs in the oom hierarchy (including root) so if the root memcg is the largest one then we should simply consider it no?

You are skipping root there because of
memcg_has_children but I suspect this and the whole accumulate up the hierarchy approach just makes the whole thing more complex than necessary. With "tasks only in leafs" cgroup policy we should only see any pages on LRUs on the global root memcg and leaf cgroups. The same applies to memcg stats. So why cannot we simply do the tree walk, calculate badness/check the priority and select the largest memcg in one go?

mm/memcontrol.c static void __oom_kill_process(struct task_struct *victim) · line 815

Quoted 8 lines

mm/memcontrol.c +3 -0

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

This will leak a reference to the victim AFACS

On Tue, Sep 05, 2017 at 04:57:00PM +0200, Michal Hocko wrote:

Quoted 12 lines

Hmm. The changelog says "By default, it will look for the biggest leaf cgroup, and kill the largest task inside." But you are accumulating oom_score up the hierarchy and so parents will have higher score than the layer of their children and the larger the sub-hierarchy the more biased it will become. Say you have

	root
         /\
        /  \
       A    D
      / \
     B   C

Quoted 3 lines

B (5), C(15) thus A(20) and D(20). Unless I am missing something we are going to go down A path and then chose C even though D is the largest leaf group, right?

You're right, changelog is not accurate, I'll fix it. The behavior is correct, IMO.

Quoted 4 lines

I do not understand why do we have to handle root cgroup specially here. select_victim_memcg already iterates all memcgs in the oom hierarchy (including root) so if the root memcg is the largest one then we should simply consider it no?

We don't have necessary stats for the root cgroup, so we can't calculate it's oom_score.

Quoted 7 lines

You are skipping root there because of
memcg_has_children but I suspect this and the whole accumulate up the hierarchy approach just makes the whole thing more complex than necessary. With "tasks only in leafs" cgroup policy we should only see any pages on LRUs on the global root memcg and leaf cgroups. The same applies to memcg stats. So why cannot we simply do the tree walk, calculate badness/check the priority and select the largest memcg in one go?

We have to traverse from top to bottom to make priority-based decision, but size-based oom_score is calculated as sum of descending leaf cgroup scores.

For example:
root

          /\
         /  \
        A    D
       / \
      B   C

A and D have same priorities, B has larger priority than C.

In this case we need to calculate size-based score for A, which requires summing oom_score of the sub-tree (B an C), despite we don't need it for choosing between B and C.

Maybe I don't see it, but I don't know how to implement it more optimal.

Quoted 1 line · Roman Gushchin

On Tue, Sep 05, 2017 at 04:57:00PM +0200, Michal Hocko wrote:

[...]

Quoted 16 lines

Quoted 15 lines

Hmm. The changelog says "By default, it will look for the biggest leaf cgroup, and kill the largest task inside." But you are accumulating oom_score up the hierarchy and so parents will have higher score than the layer of their children and the larger the sub-hierarchy the more biased it will become. Say you have

	root
         /\
        /  \
       A    D
      / \
     B   C

B (5), C(15) thus A(20) and D(20). Unless I am missing something we are going to go down A path and then chose C even though D is the largest leaf group, right?

Quoted 2 lines

You're right, changelog is not accurate, I'll fix it. The behavior is correct, IMO.

Please explain why. This is really a non-intuitive semantic. Why should larger hierarchies be punished more than shallow ones? I would completely agree if the whole hierarchy would be a killable entity (aka A would be kill-all).

[...]

Quoted 5 lines

Quoted 4 lines

I do not understand why do we have to handle root cgroup specially here. select_victim_memcg already iterates all memcgs in the oom hierarchy (including root) so if the root memcg is the largest one then we should simply consider it no?

Quoted 2 lines

We don't have necessary stats for the root cgroup, so we can't calculate it's oom_score.

We used to charge pages to the root memcg as well so we might resurrect that idea. In any case this is something that could be hidden in memcg_oom_badness rather then special cased somewhere else.

Quoted 25 lines

Quoted 8 lines

You are skipping root there because of
memcg_has_children but I suspect this and the whole accumulate up the hierarchy approach just makes the whole thing more complex than necessary. With "tasks only in leafs" cgroup policy we should only see any pages on LRUs on the global root memcg and leaf cgroups. The same applies to memcg stats. So why cannot we simply do the tree walk, calculate badness/check the priority and select the largest memcg in one go?

We have to traverse from top to bottom to make priority-based decision, but size-based oom_score is calculated as sum of descending leaf cgroup scores.

For example:
root

          /\
         /  \
        A    D
       / \
      B   C

A and D have same priorities, B has larger priority than C.

In this case we need to calculate size-based score for A, which requires summing oom_score of the sub-tree (B an C), despite we don't need it for choosing between B and C.

Quoted 1 line

Maybe I don't see it, but I don't know how to implement it more optimal.

I have to think about the priority based oom killing some more to be honest. Do we really want to allow setting a priority to non-leaf memcgs? How are you going to manage the whole tree consistency? Say your above example have prio(A) < prio(D) && prio(C) > prio(D). Your current implementation would kill D, right?

Isn't that counter intuitive
behavior again. If anything we should prio(A) = max(tree_prio(A)). Again I could understand comparing priorities only on killable entities.

Quoted 22 lines · Michal Hocko

Quoted 1 line · Roman Gushchin

On Tue, Sep 05, 2017 at 04:57:00PM +0200, Michal Hocko wrote:

[...]

Quoted 18 lines

Quoted 12 lines

Hmm. The changelog says "By default, it will look for the biggest leaf cgroup, and kill the largest task inside." But you are accumulating oom_score up the hierarchy and so parents will have higher score than the layer of their children and the larger the sub-hierarchy the more biased it will become. Say you have

	root
         /\
        /  \
       A    D
      / \
     B   C

Quoted 3 lines

B (5), C(15) thus A(20) and D(20). Unless I am missing something we are going to go down A path and then chose C even though D is the largest leaf group, right?

You're right, changelog is not accurate, I'll fix it. The behavior is correct, IMO.

Quoted 4 lines

Please explain why. This is really a non-intuitive semantic. Why should larger hierarchies be punished more than shallow ones? I would completely agree if the whole hierarchy would be a killable entity (aka A would be kill-all).

I think it's a reasonable and clear policy: we're looking for a memcg with the smallest oom_priority and largest memory footprint recursively.

Then we reclaim some memory from it (by killing the biggest process or all processes, depending on memcg preferences).

In general, if there are two memcgs of equal importance (which is defined by setting the oom_priority), we're choosing the largest, because there are more chances that it contain a leaking process. The same is true right now for processes.

I agree, that for size-based comparison we could use a different policy:
comparing leaf cgroups despite their level. But I don't see a clever way to apply oom_priorities in this case. Comparing oom_priority on each level is a simple and powerful policy, and it works well for delegation.

Quoted 9 lines

[...]

Quoted 7 lines

Quoted 4 lines

I do not understand why do we have to handle root cgroup specially here. select_victim_memcg already iterates all memcgs in the oom hierarchy (including root) so if the root memcg is the largest one then we should simply consider it no?

We don't have necessary stats for the root cgroup, so we can't calculate it's oom_score.

Quoted 3 lines

We used to charge pages to the root memcg as well so we might resurrect that idea. In any case this is something that could be hidden in memcg_oom_badness rather then special cased somewhere else.

In theory I agree, but I do not see a good way to calculate root memcg oom_score.

Quoted 23 lines · Roman Gushchin

Quoted 22 lines · Michal Hocko

Quoted 1 line · Roman Gushchin

On Tue, Sep 05, 2017 at 04:57:00PM +0200, Michal Hocko wrote:

[...]

Quoted 18 lines

Quoted 12 lines

Hmm. The changelog says "By default, it will look for the biggest leaf cgroup, and kill the largest task inside." But you are accumulating oom_score up the hierarchy and so parents will have higher score than the layer of their children and the larger the sub-hierarchy the more biased it will become. Say you have

	root
         /\
        /  \
       A    D
      / \
     B   C

Quoted 3 lines

B (5), C(15) thus A(20) and D(20). Unless I am missing something we are going to go down A path and then chose C even though D is the largest leaf group, right?

You're right, changelog is not accurate, I'll fix it. The behavior is correct, IMO.

Quoted 7 lines

Quoted 4 lines

Please explain why. This is really a non-intuitive semantic. Why should larger hierarchies be punished more than shallow ones? I would completely agree if the whole hierarchy would be a killable entity (aka A would be kill-all).

I think it's a reasonable and clear policy: we're looking for a memcg with the smallest oom_priority and largest memory footprint recursively.

But this can get really complex for non-trivial setups. Anything with deeper and larger hierarchies will get quite complex IMHO.

Btw. do you have any specific usecase for the priority based oom killer? I remember David was asking for this because it _would_ be useful but you didn't have it initially. And I agree with that I am just not sure the semantic is thought through wery well. I am thinking whether it would be easier to push this further without priority thing for now and add it later with a clear example of the configuration and how it should work and a more thought through semantic. Would that sound acceptable? I believe the rest is quite useful to get merged on its own.

Quoted 8 lines

Then we reclaim some memory from it (by killing the biggest process or all processes, depending on memcg preferences).

In general, if there are two memcgs of equal importance (which is defined by setting the oom_priority), we're choosing the largest, because there are more chances that it contain a leaking process. The same is true right now for processes.

Yes except this is not the case as shown above. We can easily select a smaller leaf memcg just because it is in a larger hierarchy and that sounds very dubious to me. Even when all the priorities are the same.

Quoted 5 lines

I agree, that for size-based comparison we could use a different policy:
comparing leaf cgroups despite their level. But I don't see a clever way to apply oom_priorities in this case. Comparing oom_priority on each level is a simple and powerful policy, and it works well for delegation.

You are already shaping semantic around the implementation and that is a clear sign of problem.

Quoted 13 lines

Quoted 12 lines

[...]

Quoted 7 lines

Quoted 4 lines

I do not understand why do we have to handle root cgroup specially here. select_victim_memcg already iterates all memcgs in the oom hierarchy (including root) so if the root memcg is the largest one then we should simply consider it no?

We don't have necessary stats for the root cgroup, so we can't calculate it's oom_score.

We used to charge pages to the root memcg as well so we might resurrect that idea. In any case this is something that could be hidden in memcg_oom_badness rather then special cased somewhere else.

Quoted 2 lines

In theory I agree, but I do not see a good way to calculate root memcg oom_score.

Why cannot you emulate that by the largest task in the root? The same way you actually do in select_victim_root_cgroup_task now?

Although, I don't think the whole thing is useful without any way to adjust the memcg selection, so we can't postpone if for too long. Anyway, if you think it's a way to go forward, let's do it.

On Tue, Sep 05, 2017 at 04:57:00PM +0200, Michal Hocko wrote:

Quoted 9 lines

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

Quoted 7 lines

mm/oom_kill.c +3 -0

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

This will leak a reference to the victim AFACS

Good catch!
I didn't fix this after moving reference dropping into __oom_kill_process(). Fixed.

Quoted 1 line · Roman Gushchin

On Tue, Sep 05, 2017 at 04:57:00PM +0200, Michal Hocko wrote:

[...]

Quoted 10 lines

Quoted 9 lines

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

Quoted 7 lines

mm/oom_kill.c +3 -0

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

This will leak a reference to the victim AFACS

Quoted 3 lines

Good catch!
I didn't fix this after moving reference dropping into __oom_kill_process(). Fixed.

Btw. didn't you want to check victim->signal->oom_score_adj == OOM_SCORE_ADJ_MIN

here as well? Maybe I've missed something but you still can kill a task which is oom disabled which I thought we agreed is the wrong thing to do.

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

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.

On Mon, 4 Sep 2017, Roman Gushchin wrote

Quoted 1 line

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

You are missing a major issue here. Processes may have allocation constraints to memory nodes, special DMA zones etc etc. OOM conditions on such resource constricted allocations need to be dealt with. Killing processes that do not allocate with the same restrictions may not do anything to improve conditions.

Introducing of cgroup-aware OOM killer changes the victim selection algorithm used by default: instead of picking the largest process, it will pick the largest memcg and then the largest process inside.

This affects only cgroup v2 users.

To provide a way to use cgroups v2 if the old OOM victim selection algorithm is preferred for some reason, the nogroupoom mount option is added.

Quoted 4 lines · Roman Gushchin

Introducing of cgroup-aware OOM killer changes the victim selection algorithm used by default: instead of picking the largest process, it will pick the largest memcg and then the largest process inside.

Quoted 5 lines

This affects only cgroup v2 users.

To provide a way to use cgroups v2 if the old OOM victim selection algorithm is preferred for some reason, the nogroupoom mount option is added.

Is this mount option or boot parameter? From the code, it seems like a boot parameter.

Introducing of cgroup-aware OOM killer changes the victim selection algorithm used by default: instead of picking the largest process, it will pick the largest memcg and then the largest process inside.

This affects only cgroup v2 users.

To provide a way to use cgroups v2 if the old OOM victim selection algorithm is preferred for some reason, the nogroupoom mount option is added.

If set, the OOM selection is performed in a "traditional" per-process way. Both oom_priority and oom_group memcg knobs are ignored.

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]

---
 Documentation/admin-guide/kernel-parameters.txt | 1 +
 mm/memcontrol.c                                 | 8 ++++++++
 2 files changed, 9 insertions(+)

Documentation/admin-guide/kernel-parameters.txt +1 -0

index 28f1a0f84456..07891f1030aa 100644
@@ -489,6 +489,7 @@
 			Format: <string>
 			nosocket -- Disable socket memory accounting.
 			nokmem -- Disable kernel memory accounting.
+			nogroupoom -- Disable cgroup-aware OOM killer.
 
 	checkreqprot	[SELINUX] Set initial checkreqprot flag value.
 			Format: { "0" | "1" }

mm/memcontrol.c +8 -0

index d7dd293897ca..6a8235dc41f6 100644
@@ -87,6 +87,9 @@ static bool cgroup_memory_nosocket;
 /* Kernel memory accounting disabled? */
 static bool cgroup_memory_nokmem;
 
+/* Cgroup-aware OOM  disabled? */
+static bool cgroup_memory_nogroupoom;
+
 /* Whether the swap controller is active */
 #ifdef CONFIG_MEMCG_SWAP
 int do_swap_account __read_mostly;

@@ -2822,6 +2825,9 @@ bool mem_cgroup_select_oom_victim(struct oom_control *oc)
 	if (mem_cgroup_disabled())
 		return false;
 
+	if (cgroup_memory_nogroupoom)
+		return false;
+
 	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys))
 		return false;
 

@@ -6188,6 +6194,8 @@ static int __init cgroup_memory(char *s)
 			cgroup_memory_nosocket = true;
 		if (!strcmp(token, "nokmem"))
 			cgroup_memory_nokmem = true;
+		if (!strcmp(token, "nogroupoom"))
+			cgroup_memory_nogroupoom = true;
 	}
 	return 0;
 }

Signature · 1 line

2.13.5

I will go and check patch 2 more deeply but this is something that I wanted to sort out first.

Quoted 4 lines · Roman Gushchin

Introducing of cgroup-aware OOM killer changes the victim selection algorithm used by default: instead of picking the largest process, it will pick the largest memcg and then the largest process inside.

Quoted 8 lines

This affects only cgroup v2 users.

To provide a way to use cgroups v2 if the old OOM victim selection algorithm is preferred for some reason, the nogroupoom mount option is added.

If set, the OOM selection is performed in a "traditional" per-process way. Both oom_priority and oom_group memcg knobs are ignored.

Why is this an opt out rather than opt-in? IMHO the original oom logic should be preserved by default and specific workloads should opt in for the cgroup aware logic. Changing the global behavior depending on whether cgroup v2 interface is in use is more than unexpected and IMHO wrong approach to take. I think we should instead go with oom_strategy=[alloc_task,biggest_task,cgroup]

we currently have alloc_task (via sysctl_oom_kill_allocating_task) and biggest_task which is the default. You are adding cgroup and the more I think about the more I agree that it doesn't really make sense to try to fit thew new semantic into the existing one (compare tasks to kill-all memcgs). Just introduce a new strategy and define a new semantic from scratch. Memcg priority and kill-all are a natural extension of this new strategy. This will make the life easier and easier to understand by users.

Does that make sense to you?

I think we should instead go with
oom_strategy=[alloc_task,biggest_task,cgroup]

On Tue, Sep 05, 2017 at 03:44:12PM +0200, Michal Hocko wrote:

Quoted 16 lines

Why is this an opt out rather than opt-in? IMHO the original oom logic should be preserved by default and specific workloads should opt in for the cgroup aware logic. Changing the global behavior depending on whether cgroup v2 interface is in use is more than unexpected and IMHO wrong approach to take. I think we should instead go with oom_strategy=[alloc_task,biggest_task,cgroup]

we currently have alloc_task (via sysctl_oom_kill_allocating_task) and biggest_task which is the default. You are adding cgroup and the more I think about the more I agree that it doesn't really make sense to try to fit thew new semantic into the existing one (compare tasks to kill-all memcgs). Just introduce a new strategy and define a new semantic from scratch. Memcg priority and kill-all are a natural extension of this new strategy. This will make the life easier and easier to understand by users.

Quoted 1 line

Does that make sense to you?

Absolutely.

The only thing: I'm not sure that we have to preserve the existing logic as default option. For most users (except few very specific usecases), it should be at least as good, as the existing one.

Making it opt-in means that corresponding code will be executed only by few users, who cares.

Then we should probably hide corresponding
cgroup interface (oom_group and oom_priority knobs) by default, and it feels as unnecessary complication and is overall against cgroup v2 interface design.

Quoted 2 lines

I think we should instead go with
oom_strategy=[alloc_task,biggest_task,cgroup]

It would be a really nice interface; although I've no idea how to implement it:
"alloc_task" is an existing sysctl, which we have to preserve;

while "cgroup" depends on cgroup v2.

Quoted 1 line · Roman Gushchin

On Tue, Sep 05, 2017 at 03:44:12PM +0200, Michal Hocko wrote:

[...]

Quoted 16 lines

Quoted 16 lines

Why is this an opt out rather than opt-in? IMHO the original oom logic should be preserved by default and specific workloads should opt in for the cgroup aware logic. Changing the global behavior depending on whether cgroup v2 interface is in use is more than unexpected and IMHO wrong approach to take. I think we should instead go with oom_strategy=[alloc_task,biggest_task,cgroup]

we currently have alloc_task (via sysctl_oom_kill_allocating_task) and biggest_task which is the default. You are adding cgroup and the more I think about the more I agree that it doesn't really make sense to try to fit thew new semantic into the existing one (compare tasks to kill-all memcgs). Just introduce a new strategy and define a new semantic from scratch. Memcg priority and kill-all are a natural extension of this new strategy. This will make the life easier and easier to understand by users.

Quoted 7 lines

Quoted 1 line

Does that make sense to you?

Absolutely.

The only thing: I'm not sure that we have to preserve the existing logic as default option. For most users (except few very specific usecases), it should be at least as good, as the existing one.

But this is really an unexpected change. Users even might not know that they are using cgroup v2 and memcg is in use.

Quoted 2 lines

Making it opt-in means that corresponding code will be executed only by few users, who cares.

Yeah, which is the way we should introduce new features no?

Quoted 4 lines

Then we should probably hide corresponding
cgroup interface (oom_group and oom_priority knobs) by default, and it feels as unnecessary complication and is overall against cgroup v2 interface design.

Why. If we care enough, we could simply return EINVAL when those knobs are written while the corresponding strategy is not used.

Quoted 6 lines

Quoted 3 lines

I think we should instead go with
oom_strategy=[alloc_task,biggest_task,cgroup]

It would be a really nice interface; although I've no idea how to implement it:
"alloc_task" is an existing sysctl, which we have to preserve;

I would argue that we should simply deprecate and later drop the sysctl. I _strongly_ suspect anybody is using this. If yes it is not that hard to change the kernel command like rather than select the sysctl.

Quoted 2 lines

while "cgroup" depends on cgroup v2.

Which is not a big deal either. Simply fall back to default if there are no cgroup v2. The implementation would have essentially the same effect because there won't be any kill-all cgroups and so we will select the largest task.

On Tue, Sep 05, 2017 at 05:12:51PM +0200, Michal Hocko wrote:

Quoted 7 lines

Quoted 4 lines

Then we should probably hide corresponding
cgroup interface (oom_group and oom_priority knobs) by default, and it feels as unnecessary complication and is overall against cgroup v2 interface design.

Why. If we care enough, we could simply return EINVAL when those knobs are written while the corresponding strategy is not used.

It doesn't look as a nice default interface.

Quoted 9 lines

Quoted 5 lines

Quoted 2 lines

I think we should instead go with
oom_strategy=[alloc_task,biggest_task,cgroup]

It would be a really nice interface; although I've no idea how to implement it:
"alloc_task" is an existing sysctl, which we have to preserve;

I would argue that we should simply deprecate and later drop the sysctl. I _strongly_ suspect anybody is using this. If yes it is not that hard to change the kernel command like rather than select the sysctl.

I agree. And if so, why do we need a new interface for an useless feature?

Quoted 1 line · Roman Gushchin

On Tue, Sep 05, 2017 at 05:12:51PM +0200, Michal Hocko wrote:

[...]

Quoted 9 lines

Quoted 7 lines

Quoted 4 lines

Then we should probably hide corresponding
cgroup interface (oom_group and oom_priority knobs) by default, and it feels as unnecessary complication and is overall against cgroup v2 interface design.

Why. If we care enough, we could simply return EINVAL when those knobs are written while the corresponding strategy is not used.

It doesn't look as a nice default interface.

I do not have a strong opinion on this. A printk_once could explain why the knob is ignored and instruct the admin how to enable the feature completely.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

I think we should instead go with
oom_strategy=[alloc_task,biggest_task,cgroup]

It would be a really nice interface; although I've no idea how to implement it:
"alloc_task" is an existing sysctl, which we have to preserve;

Quoted 5 lines

Quoted 3 lines

I would argue that we should simply deprecate and later drop the sysctl. I _strongly_ suspect anybody is using this. If yes it is not that hard to change the kernel command like rather than select the sysctl.

I agree. And if so, why do we need a new interface for an useless feature?

Well, I won't be opposed just deprecating the sysfs and only add a "real" kill-allocate strategy if somebody explicitly asks for it.

Quoted 1 line

From f6e2339926a07500834d86548f3f116af7335d71 Mon Sep 17 00:00:00 2001

From: Roman Gushchin <[email protected]>
Date: Wed, 6 Sep 2017 17:43:44 +0100
Subject: [PATCH] mm, oom: first step towards oom_kill_allocating_task
deprecation

The oom_kill_allocating_task sysctl which causes the OOM killer to simple kill the allocating task is useless. Killing the random

task is not the best idea.

Nobody likes it, and hopefully nobody uses it.
We want to completely deprecate it at some point.

To make a first step towards deprecation, let's warn potential users about deprecation plans.

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

Cc Andrew Morton <akpm-de/[email protected]>

Cc Michal Hocko <mhocko-IBi9RG/[email protected]>

Cc Johannes Weiner <[email protected]>

Cc David Rientjes <[email protected]>

Cc Vladimir Davydov <[email protected]>

Cc [email protected]

Cc [email protected]

---
 kernel/sysctl.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

kernel/sysctl.c +12 -1

index 655686d546cb..9158f1980584 100644
@@ -220,6 +220,17 @@ static int sysrq_sysctl_handler(struct ctl_table *table, int write,
 
 #endif
 
+static int proc_oom_kill_allocating_tasks(struct ctl_table *table, int write,
+				   void __user *buffer, size_t *lenp,
+				   loff_t *ppos)
+{
+	pr_warn_once("The oom_kill_allocating_task sysctl will be deprecated.\n"
+		     "If you're using it, please, report to "
+		     "linux-mm-Ja57EE8LTNBzeIdxy0IIJ+G/[email protected]\n");
+
+	return proc_dointvec(table, write, buffer, lenp, ppos);
+}
+
 static struct ctl_table kern_table[];
 static struct ctl_table vm_table[];
 static struct ctl_table fs_table[];

@@ -1235,7 +1246,7 @@ static struct ctl_table vm_table[] = {
 		.data		= &sysctl_oom_kill_allocating_task,
 		.maxlen		= sizeof(sysctl_oom_kill_allocating_task),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec,
+		.proc_handler	= proc_oom_kill_allocating_tasks,
 	},
 	{
 		.procname	= "oom_dump_tasks",

Signature · 1 line

2.13.5

From f6e2339926a07500834d86548f3f116af7335d71 Mon Sep 17 00:00:00 2001
From: Roman Gushchin <[email protected]>
Date: Wed, 6 Sep 2017 17:43:44 +0100
Subject: [PATCH] mm, oom: first step towards oom_kill_allocating_task
deprecation

The oom_kill_allocating_task sysctl which causes the OOM killer to simple kill the allocating task is useless. Killing the random task is not the best idea.

Nobody likes it, and hopefully nobody uses it.
We want to completely deprecate it at some point.

To make a first step towards deprecation, let's warn potential users about deprecation plans.

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

Cc Andrew Morton <akpm-de/[email protected]>

Cc Michal Hocko <mhocko-IBi9RG/[email protected]>

Cc Johannes Weiner <[email protected]>

Cc David Rientjes <[email protected]>

Cc Vladimir Davydov <[email protected]>

Cc [email protected]

Cc [email protected]

---
 kernel/sysctl.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

kernel/sysctl.c +12 -1

index 655686d546cb..9158f1980584 100644
@@ -220,6 +220,17 @@ static int sysrq_sysctl_handler(struct ctl_table *table, int write,
 
 #endif
 
+static int proc_oom_kill_allocating_tasks(struct ctl_table *table, int write,
+				   void __user *buffer, size_t *lenp,
+				   loff_t *ppos)
+{
+	pr_warn_once("The oom_kill_allocating_task sysctl will be deprecated.\n"
+		     "If you're using it, please, report to "
+		     "linux-mm-Ja57EE8LTNBzeIdxy0IIJ+G/[email protected]\n");
+
+	return proc_dointvec(table, write, buffer, lenp, ppos);
+}
+
 static struct ctl_table kern_table[];
 static struct ctl_table vm_table[];
 static struct ctl_table fs_table[];

@@ -1235,7 +1246,7 @@ static struct ctl_table vm_table[] = {
 		.data		= &sysctl_oom_kill_allocating_task,
 		.maxlen		= sizeof(sysctl_oom_kill_allocating_task),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec,
+		.proc_handler	= proc_oom_kill_allocating_tasks,
 	},
 	{
 		.procname	= "oom_dump_tasks",

Quoted 7 lines

The oom_kill_allocating_task sysctl which causes the OOM killer to simple kill the allocating task is useless. Killing the random task is not the best idea.

Nobody likes it, and hopefully nobody uses it.
We want to completely deprecate it at some point.

SGI required it when it was introduced simply to avoid the very expensive tasklist scan. Adding Christoph Lameter to the cc since he was involved back then.

Quoted 11 lines · David Rientjes

Quoted 7 lines

The oom_kill_allocating_task sysctl which causes the OOM killer to simple kill the allocating task is useless. Killing the random task is not the best idea.

Nobody likes it, and hopefully nobody uses it.
We want to completely deprecate it at some point.

SGI required it when it was introduced simply to avoid the very expensive tasklist scan. Adding Christoph Lameter to the cc since he was involved back then.

Really? From what I know and worked on way back when: The reason was to be able to contain the affected application in a cpuset. Multiple apps may have been running in multiple cpusets on a large NUMA machine and the OOM condition in one cpuset should not affect the other. It also helped to isolate the application behavior causing the oom in numerous cases.

Doesnt this requirement transfer to cgroups in the same way?

Left SGI in 2008 so adding Dimitri who may know about the current situation. Robin Holt also left SGI as far as I know.

Quoted 7 lines

Really? From what I know and worked on way back when: The reason was to be able to contain the affected application in a cpuset. Multiple apps may have been running in multiple cpusets on a large NUMA machine and the OOM condition in one cpuset should not affect the other. It also helped to isolate the application behavior causing the oom in numerous cases.

Doesnt this requirement transfer to cgroups in the same way?

We have per-node memory stats and plan to use them during the OOM victim selection. Hopefully it can help.

Quoted 8 lines · Roman Gushchin

Quoted 7 lines

Really? From what I know and worked on way back when: The reason was to be able to contain the affected application in a cpuset. Multiple apps may have been running in multiple cpusets on a large NUMA machine and the OOM condition in one cpuset should not affect the other. It also helped to isolate the application behavior causing the oom in numerous cases.

Doesnt this requirement transfer to cgroups in the same way?

Quoted 2 lines

We have per-node memory stats and plan to use them during the OOM victim selection. Hopefully it can help.

One of the OOM causes could be that memory was restricted to a certain node set. Killing the allocating task is (was?) default behavior in that case so that the task that has the restrictions is killed. Not any task that may not have the restrictions and woiuld not experience OOM.

Quoted 10 lines · Christopher Lameter

Quoted 8 lines · Roman Gushchin

Quoted 7 lines

Really? From what I know and worked on way back when: The reason was to be able to contain the affected application in a cpuset. Multiple apps may have been running in multiple cpusets on a large NUMA machine and the OOM condition in one cpuset should not affect the other. It also helped to isolate the application behavior causing the oom in numerous cases.

Doesnt this requirement transfer to cgroups in the same way?

Quoted 7 lines

Quoted 2 lines

We have per-node memory stats and plan to use them during the OOM victim selection. Hopefully it can help.

One of the OOM causes could be that memory was restricted to a certain node set. Killing the allocating task is (was?) default behavior in that case so that the task that has the restrictions is killed. Not any task that may not have the restrictions and woiuld not experience OOM.

As I can see, it's not the default behavior these days. If we have a way to select a victim between memcgs/tasks which are actually using the corresponding type of memory, it's much better than to kill an allocating task.

Quoted 3 lines

SGI required it when it was introduced simply to avoid the very expensive tasklist scan. Adding Christoph Lameter to the cc since he was involved back then.

Really? From what I know and worked on way back when: The reason was to be able to contain the affected application in a cpuset. Multiple apps may have been running in multiple cpusets on a large NUMA machine and the OOM condition in one cpuset should not affect the other. It also helped to isolate the application behavior causing the oom in numerous cases.

Doesnt this requirement transfer to cgroups in the same way?

Left SGI in 2008 so adding Dimitri who may know about the current situation. Robin Holt also left SGI as far as I know.

I would argue that we should simply deprecate and later drop the sysctl. I _strongly_ suspect anybody is using this. If yes it is not that hard to change the kernel command like rather than select the sysctl. The deprecation process would be
- warn when somebody writes to the sysctl and check both boot and sysctl values
[ wait some time ]
- keep the sysctl but return EINVAL
[ wait some time ]
- remove the sysctl

Why is this an opt out rather than opt-in? IMHO the original oom logic should be preserved by default and specific workloads should opt in for the cgroup aware logic. Changing the global behavior depending on whether cgroup v2 interface is in use is more than unexpected and IMHO wrong approach to take. I think we should instead go with oom_strategy=[alloc_task,biggest_task,cgroup]

we currently have alloc_task (via sysctl_oom_kill_allocating_task) and biggest_task which is the default. You are adding cgroup and the more I think about the more I agree that it doesn't really make sense to try to fit thew new semantic into the existing one (compare tasks to kill-all memcgs). Just introduce a new strategy and define a new semantic from scratch. Memcg priority and kill-all are a natural extension of this new strategy. This will make the life easier and easier to understand by users.

Quoted 7 lines · Michal Hocko

Why is this an opt out rather than opt-in? IMHO the original oom logic should be preserved by default and specific workloads should opt in for the cgroup aware logic. Changing the global behavior depending on whether cgroup v2 interface is in use is more than unexpected and IMHO wrong approach to take. I think we should instead go with oom_strategy=[alloc_task,biggest_task,cgroup]

Quoted 8 lines

we currently have alloc_task (via sysctl_oom_kill_allocating_task) and biggest_task which is the default. You are adding cgroup and the more I think about the more I agree that it doesn't really make sense to try to fit thew new semantic into the existing one (compare tasks to kill-all memcgs). Just introduce a new strategy and define a new semantic from scratch. Memcg priority and kill-all are a natural extension of this new strategy. This will make the life easier and easier to understand by users.

oom_kill_allocating_task is actually a really good example of why cgroup-awareness *should* be the new default.

Before we had the oom killer victim selection, we simply killed the faulting/allocating task. While a valid answer to the problem, it's not very fair or representative of what the user wants or intends.

Then we added code to kill the biggest offender instead, which should have been the case from the start and was hence made the new default. The oom_kill_allocating_task was added on the off-chance that there might be setups who, for historical reasons, rely on the old behavior. But our default was chosen based on what behavior is fair, expected, and most reflective of the user's intentions.

The cgroup-awareness in the OOM killer is exactly the same thing. It should have been the default from the beginning, because the user configures a group of tasks to be an interdependent, terminal unit of memory consumption, and it's undesirable for the OOM killer to ignore this intention and compare members across these boundaries.

Quoted 5 lines · Johannes Weiner

The cgroup-awareness in the OOM killer is exactly the same thing. It should have been the default from the beginning, because the user configures a group of tasks to be an interdependent, terminal unit of memory consumption, and it's undesirable for the OOM killer to ignore this intention and compare members across these boundaries.

I would agree if that was true in general. I can completely see how the cgroup awareness is useful in e.g. containerized environments (especially with kill-all enabled) but memcgs are used in a large variety of usecases and I cannot really say all of them really demand the new semantic. Say I have a workload which doesn't want to see reclaim interference from others on the same machine. Why should I kill a process from that particular memcg just because it is the largest one when there is a memory hog/leak outside of this memcg?

From my point of view the safest (in a sense of the least surprise) way to go with opt-in for the new heuristic. I am pretty sure all who would benefit from the new behavior will enable it while others will not regress in unexpected way.

We can talk about the way _how_ to control these oom strategies, of course. But I would be really reluctant to change the default which is used for years and people got used to it.

Quoted 7 lines · Michal Hocko

Quoted 5 lines · Johannes Weiner

The cgroup-awareness in the OOM killer is exactly the same thing. It should have been the default from the beginning, because the user configures a group of tasks to be an interdependent, terminal unit of memory consumption, and it's undesirable for the OOM killer to ignore this intention and compare members across these boundaries.

Quoted 8 lines

I would agree if that was true in general. I can completely see how the cgroup awareness is useful in e.g. containerized environments (especially with kill-all enabled) but memcgs are used in a large variety of usecases and I cannot really say all of them really demand the new semantic. Say I have a workload which doesn't want to see reclaim interference from others on the same machine. Why should I kill a process from that particular memcg just because it is the largest one when there is a memory hog/leak outside of this memcg?

Sure, it's always possible to come up with a config for which this isn't the optimal behavior. But this is about picking a default that makes sense to most users, and that type of cgroup usage just isn't the common case.

Quoted 4 lines

From my point of view the safest (in a sense of the least surprise) way to go with opt-in for the new heuristic. I am pretty sure all who would benefit from the new behavior will enable it while others will not regress in unexpected way.

This thinking simply needs to be balanced against the need to make an unsurprising and consistent final interface.

The current behavior breaks isolation by letting tasks in different cgroups compete with each other during an OOM kill. While you can rightfully argue that it's possible for usecases to rely on this, you cannot tell me that this is the least-surprising thing we can offer users; certainly not new users, but also not many/most existing ones.

Quoted 3 lines

We can talk about the way _how_ to control these oom strategies, of course. But I would be really reluctant to change the default which is used for years and people got used to it.

I really doubt there are many cgroup users that rely on that particular global OOM behavior.

We have to agree to disagree, I guess.

Quoted 17 lines · Johannes Weiner

Quoted 7 lines · Michal Hocko

Quoted 5 lines · Johannes Weiner

The cgroup-awareness in the OOM killer is exactly the same thing. It should have been the default from the beginning, because the user configures a group of tasks to be an interdependent, terminal unit of memory consumption, and it's undesirable for the OOM killer to ignore this intention and compare members across these boundaries.

Quoted 8 lines

I would agree if that was true in general. I can completely see how the cgroup awareness is useful in e.g. containerized environments (especially with kill-all enabled) but memcgs are used in a large variety of usecases and I cannot really say all of them really demand the new semantic. Say I have a workload which doesn't want to see reclaim interference from others on the same machine. Why should I kill a process from that particular memcg just because it is the largest one when there is a memory hog/leak outside of this memcg?

Quoted 4 lines

Sure, it's always possible to come up with a config for which this isn't the optimal behavior. But this is about picking a default that makes sense to most users, and that type of cgroup usage just isn't the common case.

How can you tell, really? Even if cgroup2 is a new interface we still want as many legacy (v1) users to be migrated to the new hierarchy. I have seen quite different usecases over time and I have hard time to tell which of them to call common enough.

Quoted 5 lines

Quoted 4 lines

From my point of view the safest (in a sense of the least surprise) way to go with opt-in for the new heuristic. I am pretty sure all who would benefit from the new behavior will enable it while others will not regress in unexpected way.

Quoted 2 lines

This thinking simply needs to be balanced against the need to make an unsurprising and consistent final interface.

Sure. And I _think_ we can come up with a clear interface to configure the oom behavior - e.g. a kernel command line parameter with a default based on a config option.

I am not sure this is how things evolved actually. This is way before my time so my git log interpretation might be imprecise. We do have oom_badness heuristic since out_of_memory has been introduced and oom_kill_allocating_task has been introduced much later because of large boxes with zillions of tasks (SGI I suspect) which took too long to select a victim so David has added this heuristic.

Quoted 6 lines

I am not sure this is how things evolved actually. This is way before my time so my git log interpretation might be imprecise. We do have oom_badness heuristic since out_of_memory has been introduced and oom_kill_allocating_task has been introduced much later because of large boxes with zillions of tasks (SGI I suspect) which took too long to select a victim so David has added this heuristic.

Nope. The logic was required for tasks that run out of memory when the restriction on the allocation did not allow the use of all of memory. cpuset restrictions and memory policy restrictions where the prime considerations at the time.

It has *nothing* to do with zillions of tasks. Its amusing that the SGI ghost is still haunting the discussion here. The company died a couple of years ago finally (ok somehow HP has an "SGI" brand now I believe). But there are multiple companies that have large NUMA configurations and they all have configurations where they want to restrict allocations of a process to subset of system memory. This is even more important now that we get new forms of memory (NVDIMM, PCI-E device memory etc). You need to figure out what to do with allocations that fail because the *allowed* memory pools are empty.

Quoted 10 lines

It has *nothing* to do with zillions of tasks. Its amusing that the SGI ghost is still haunting the discussion here. The company died a couple of years ago finally (ok somehow HP has an "SGI" brand now I believe). But there are multiple companies that have large NUMA configurations and they all have configurations where they want to restrict allocations of a process to subset of system memory. This is even more important now that we get new forms of memory (NVDIMM, PCI-E device memory etc). You need to figure out what to do with allocations that fail because the *allowed* memory pools are empty.

We already had CONSTRAINT_CPUSET at the time, this was requested by Paul and acked by him in https://marc.info/?l=linux-mm&m=118306851418425.

Ok. Certainly there were scalability issues (lots of them) and the sysctl may have helped there if set globally. But the ability to kill the allocating tasks was primarily used in cpusets for constrained allocation.

The issue of scaling is irrelevant in the context of deciding what to do about the sysctl. You can address the issue differently if it still exists. The systems with super high NUMA nodes (hundreds to a thousand) have somehow fallen out of fashion a bit. So I doubt that this is still an issue. And no one of the old stakeholders is speaking up.

What is the current approach for an OOM occuring in a cpuset or cgroup with a restricted numa node set?