mm, oom: refactor the oom_kill_process() function

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.

3) Per-process oom_score_adj affects global OOM, so it's a breache in the isolation.

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

Under OOM conditions, it tries to find the biggest memory consumer, and free memory by killing corresponding task(s). The difference the "traditional" OOM killer is that it can treat memory cgroups as memory consumers as well as single processes.

By default, it will look for the biggest leaf cgroup, and kill the largest task inside.

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

Tasks in the root cgroup are treated as independent memory consumers, and are compared with other memory consumers (e.g. leaf cgroups). The root cgroup doesn't support the oom_kill_all_tasks feature.

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

3) Per-process oom_score_adj affects global OOM, so it's a breache in the isolation.

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

Under OOM conditions, it tries to find the biggest memory consumer, and free memory by killing corresponding task(s). The difference the "traditional" OOM killer is that it can treat memory cgroups as memory consumers as well as single processes.

Quoted 8 lines

By default, it will look for the biggest leaf cgroup, and kill the largest task inside.

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.

I'm very happy with the rest of the patchset, but I feel that I must renew my objection to memory.oom_kill_all_tasks being able to override the setting of the admin of setting a process to be oom disabled. From my perspective, setting memory.oom_kill_all_tasks with an oom disabled process attached that now becomes killable either (1) overrides the CAP_SYS_RESOURCE oom disabled setting or (2) is lazy and doesn't modify /proc/pid/oom_score_adj itself.

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.

3) Per-process oom_score_adj affects global OOM, so it's a breache in the isolation.

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

Under OOM conditions, it tries to find the biggest memory consumer, and free memory by killing corresponding task(s). The difference the "traditional" OOM killer is that it can treat memory cgroups as memory consumers as well as single processes.

By default, it will look for the biggest leaf cgroup, and kill the largest task inside.

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

Tasks in the root cgroup are treated as independent memory consumers, and are compared with other memory consumers (e.g. leaf cgroups). The root cgroup doesn't support the oom_kill_all_tasks feature.

mm/memcontrol.c +15 -0

+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))
+			continue;
+
+		points += mem_cgroup_node_nr_lru_pages(memcg, nid,
+				LRU_ALL_ANON | BIT(LRU_UNEVICTABLE));
+
+		pgdat = NODE_DATA(nid);

This doesn't apply on top of mmotm cleanly. You are missing
http://lkml.kernel.org/r/[email protected]

Quoted 11 lines

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

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

1) There is no fairness between containers. A small container with few large processes will be chosen over a large one with huge number of small processes.

Quoted 8 lines

2) Containers often do not expect that some random process inside will be killed. In many cases much safer behavior is to kill all tasks in the container. Traditionally, this was implemented in userspace, but doing it in the kernel has some advantages, especially in a case of a system-wide OOM.

3) Per-process oom_score_adj affects global OOM, so it's a breache in the isolation.

Please explain more. I guess you mean that an untrusted memcg could hide itself from the global OOM killer by reducing the oom scores? Well you need CAP_SYS_RESOURCE do reduce the current oom_score{_adj} as David has already pointed out. I also agree that we absolutely must not kill an oom disabled task. I am pretty sure somebody is using OOM_SCORE_ADJ_MIN as a protection from an untrusted SIGKILL and inconsistent state as a result. Those applications simply shouldn't behave differently in the global and container contexts.

If nothing else we have to skip OOM_SCORE_ADJ_MIN tasks during the kill.

Quoted 7 lines

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

Under OOM conditions, it tries to find the biggest memory consumer, and free memory by killing corresponding task(s). The difference the "traditional" OOM killer is that it can treat memory cgroups as memory consumers as well as single processes.

Quoted 2 lines

By default, it will look for the biggest leaf cgroup, and kill the largest task inside.

Why? I believe that the semantic should be as simple as kill the largest oom killable entity. And the entity is either a process or a memcg which is marked that way.

Why should we mix things and select a memcg to kill a process inside it? More on that below.

Quoted 9 lines

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

Tasks in the root cgroup are treated as independent memory consumers, and are compared with other memory consumers (e.g. leaf cgroups). The root cgroup doesn't support the oom_kill_all_tasks feature.

If anything you wouldn't have to treat the root memcg any special. It will be like any other memcg which doesn't have oom_kill_all_tasks...

[...]

Quoted 21 lines

mm/memcontrol.c +21 -0

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

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

Quoted 4 lines

mm/memcontrol.c +4 -0

+	points += memcg_page_state(memcg, MEMCG_SOCK);
+	points += memcg_page_state(memcg, MEMCG_SWAP);
+
+	return points;

I guess I have asked already and we haven't reached any consensus. I do not like how you treat memcgs and tasks differently. Why cannot we have a memcg score a sum of all its tasks?

How do you want to compare memcg score with tasks score?

This just smells like the outcome of a weird
semantic that you try to select the largest group I have mentioned above.

This is a rather fundamental concern and I believe we should find a consensus on it before going any further. I believe that users shouldn't see any difference in the OOM behavior when memcg v2 is used and there is no kill-all memcg. If there is such a memcg then we should treat only those specially. But you might have really strong usecases which haven't been presented or I've missed their importance.

Hi Michal!

Quoted 2 lines · Michal Hocko

This doesn't apply on top of mmotm cleanly. You are missing
http://lkml.kernel.org/r/[email protected]

I'll rebase. Thanks!

Quoted 22 lines · Roman Gushchin

Quoted 11 lines

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

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

1) There is no fairness between containers. A small container with few large processes will be chosen over a large one with huge number of small processes.

Quoted 8 lines

2) Containers often do not expect that some random process inside will be killed. In many cases much safer behavior is to kill all tasks in the container. Traditionally, this was implemented in userspace, but doing it in the kernel has some advantages, especially in a case of a system-wide OOM.

3) Per-process oom_score_adj affects global OOM, so it's a breache in the isolation.

Quoted 8 lines

Please explain more. I guess you mean that an untrusted memcg could hide itself from the global OOM killer by reducing the oom scores? Well you need CAP_SYS_RESOURCE do reduce the current oom_score{_adj} as David has already pointed out. I also agree that we absolutely must not kill an oom disabled task. I am pretty sure somebody is using OOM_SCORE_ADJ_MIN as a protection from an untrusted SIGKILL and inconsistent state as a result. Those applications simply shouldn't behave differently in the global and container contexts.

The main point of the kill_all option is to clean up the victim cgroup _completely_. If some tasks can survive, that means userspace should take care of them, look at the cgroup after oom, and kill the survivors manually.

If you want to rely on OOM_SCORE_ADJ_MIN, don't set kill_all. I really don't get the usecase for this "kill all, except this and that".

Also, it's really confusing to respect -1000 value, and completely ignore -999.

I believe that any complex userspace OOM handling should use memory.high and handle memory shortage before an actual OOM.

Quoted 10 lines

If nothing else we have to skip OOM_SCORE_ADJ_MIN tasks during the kill.

Quoted 7 lines

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

Under OOM conditions, it tries to find the biggest memory consumer, and free memory by killing corresponding task(s). The difference the "traditional" OOM killer is that it can treat memory cgroups as memory consumers as well as single processes.

Quoted 6 lines

Quoted 2 lines

By default, it will look for the biggest leaf cgroup, and kill the largest task inside.

Why? I believe that the semantic should be as simple as kill the largest oom killable entity. And the entity is either a process or a memcg which is marked that way.

So, you still need to compare memcgroups and processes.

In my case, it's more like an exception (only processes from root memcg, and only if there are no eligible cgroups with lower oom_priority). You suggest to rely on this comparison.

Quoted 2 lines

Why should we mix things and select a memcg to kill a process inside it? More on that below.

To have some sort of "fairness" in a containerized environemnt. Say, 1 cgroup with 1 big task, another cgroup with many smaller tasks. It's not necessary true, that first one is a better victim.

Quoted 35 lines

Quoted 8 lines

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

Tasks in the root cgroup are treated as independent memory consumers, and are compared with other memory consumers (e.g. leaf cgroups). The root cgroup doesn't support the oom_kill_all_tasks feature.

If anything you wouldn't have to treat the root memcg any special. It will be like any other memcg which doesn't have oom_kill_all_tasks...

[...]

Quoted 21 lines

mm/memcontrol.c +21 -0

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

Quoted 8 lines

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

Quoted 4 lines

mm/memcontrol.c +4 -0

+	points += memcg_page_state(memcg, MEMCG_SOCK);
+	points += memcg_page_state(memcg, MEMCG_SWAP);
+
+	return points;

I guess I have asked already and we haven't reached any consensus. I do not like how you treat memcgs and tasks differently. Why cannot we have a memcg score a sum of all its tasks?

It sounds like a more expensive way to get almost the same with less accuracy. Why it's better?

Quoted 1 line

How do you want to compare memcg score with tasks score?

I have to do it for tasks in root cgroups, but it shouldn't be a common case.

Quoted 2 lines · Roman Gushchin

Hi Michal!

There is nothing like a "better victim". We are pretty much in a catastrophic situation when we try to survive by killing a userspace.

We try to kill the largest because that assumes that we return the most memory from it. Now I do understand that you want to treat the memcg as a single killable entity but I find it really questionable to do a per-memcg metric and then do not treat it like that and kill only a single task. Just imagine a single memcg with zillions of taks each very small and you select it as the largest while a small taks itself doesn't help to help to get us out of the OOM.

Quoted 6 lines

Quoted 3 lines

I guess I have asked already and we haven't reached any consensus. I do not like how you treat memcgs and tasks differently. Why cannot we have a memcg score a sum of all its tasks?

It sounds like a more expensive way to get almost the same with less accuracy. Why it's better?

because then you are comparing apples to apples?

Besides that you have
to check each task for over-killing anyway. So I do not see any performance merits here.

Quoted 4 lines

Quoted 2 lines

How do you want to compare memcg score with tasks score?

I have to do it for tasks in root cgroups, but it shouldn't be a common case.

How come? I can easily imagine a setup where only some memcgs which really do need a kill-all semantic while all others can live with single task killed perfectly fine.

Quoted 5 lines · Michal Hocko

Quoted 2 lines · Roman Gushchin

Hi Michal!

There is nothing like a "better victim". We are pretty much in a catastrophic situation when we try to survive by killing a userspace.

Not necessary, it can be a cgroup OOM.

Quoted 7 lines

We try to kill the largest because that assumes that we return the most memory from it. Now I do understand that you want to treat the memcg as a single killable entity but I find it really questionable to do a per-memcg metric and then do not treat it like that and kill only a single task. Just imagine a single memcg with zillions of taks each very small and you select it as the largest while a small taks itself doesn't help to help to get us out of the OOM.

I don't think it's different from a non-containerized state: if you have a zillion of small tasks in the system, you'll meet the same issues.

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

I guess I have asked already and we haven't reached any consensus. I do not like how you treat memcgs and tasks differently. Why cannot we have a memcg score a sum of all its tasks?

Quoted 4 lines

Quoted 2 lines

It sounds like a more expensive way to get almost the same with less accuracy. Why it's better?

because then you are comparing apples to apples?

Well, I can say that I compare some number of pages against some other number of pages. And the relation between a page and memcg is more obvious, than a relation between a page and a process.

Both ways are not ideal, and sum of the processes is not ideal too. Especially, if you take oom_score_adj into account. Will you respect it?

I've started actually with such approach, but then found it weird.

Quoted 3 lines

Besides that you have
to check each task for over-killing anyway. So I do not see any performance merits here.

It's an implementation detail, and we can hopefully get rid of it at some point.

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

How do you want to compare memcg score with tasks score?

I have to do it for tasks in root cgroups, but it shouldn't be a common case.

How come? I can easily imagine a setup where only some memcgs which really do need a kill-all semantic while all others can live with single task killed perfectly fine.

I mean taking a unified cgroup hierarchy into an account, there should not be lot of tasks in the root cgroup, if any.

Quoted 8 lines · Roman Gushchin

Quoted 5 lines · Michal Hocko

Quoted 2 lines · Roman Gushchin

Hi Michal!

There is nothing like a "better victim". We are pretty much in a catastrophic situation when we try to survive by killing a userspace.

Not necessary, it can be a cgroup OOM.

memcg OOM is no different. The catastrophic is scoped to the specific hierarchy but tasks in that hierarchy still fail to make a further progress.

Quoted 8 lines

Quoted 7 lines

We try to kill the largest because that assumes that we return the most memory from it. Now I do understand that you want to treat the memcg as a single killable entity but I find it really questionable to do a per-memcg metric and then do not treat it like that and kill only a single task. Just imagine a single memcg with zillions of taks each very small and you select it as the largest while a small taks itself doesn't help to help to get us out of the OOM.

Quoted 2 lines

I don't think it's different from a non-containerized state: if you have a zillion of small tasks in the system, you'll meet the same issues.

Yes this is possible but usually you are comparing apples to apples so you will kill the largest offender and then go on. To be honest I really do hate how we try to kill a children rather than the selected victim for the same reason.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

I guess I have asked already and we haven't reached any consensus. I do not like how you treat memcgs and tasks differently. Why cannot we have a memcg score a sum of all its tasks?

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

It sounds like a more expensive way to get almost the same with less accuracy. Why it's better?

because then you are comparing apples to apples?

Well, I can say that I compare some number of pages against some other number of pages. And the relation between a page and memcg is more obvious, than a relation between a page and a process.

But you are comparing different accounting systems.

Quoted 2 lines

Both ways are not ideal, and sum of the processes is not ideal too. Especially, if you take oom_score_adj into account. Will you respect it?

Yes, and I do not see any reason why we shouldn't.

Quoted 7 lines

I've started actually with such approach, but then found it weird.

Quoted 3 lines

Besides that you have
to check each task for over-killing anyway. So I do not see any performance merits here.

Quoted 1 line

It's an implementation detail, and we can hopefully get rid of it at some point.

Well, we might do some estimations and ignore oom scopes but I that sounds really complicated and error prone. Unless we have anything like that then I would start from tasks and build up the necessary to make a decision at the higher level.

Quoted 9 lines

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

How do you want to compare memcg score with tasks score?

I have to do it for tasks in root cgroups, but it shouldn't be a common case.

How come? I can easily imagine a setup where only some memcgs which really do need a kill-all semantic while all others can live with single task killed perfectly fine.

Quoted 2 lines

I mean taking a unified cgroup hierarchy into an account, there should not be lot of tasks in the root cgroup, if any.

Is that really the case? I would assume that memory controller would be enabled only in those subtrees which really use the functionality and the rest will be sitting in the root memcg. It might be the case if you are running only containers but I am not really sure this is true in general.

Quoted 1 line · Michal Hocko

On Thu 24-08-17 14:58:42, Roman Gushchin wrote:

Quoted 4 lines

Quoted 2 lines

Both ways are not ideal, and sum of the processes is not ideal too. Especially, if you take oom_score_adj into account. Will you respect it?

Yes, and I do not see any reason why we shouldn't.

It makes things even more complicated.
Right now task's oom_score can be in (~ -total_memory, ~ +2*total_memory) range, and it you're starting summing it, it can be multiplied by number of tasks... Weird.

It also will be different in case of system and memcg-wide OOM.

Quoted 8 lines

Quoted 7 lines

I've started actually with such approach, but then found it weird.

Quoted 3 lines

Besides that you have
to check each task for over-killing anyway. So I do not see any performance merits here.

It's an implementation detail, and we can hopefully get rid of it at some point.

Quoted 4 lines

Well, we might do some estimations and ignore oom scopes but I that sounds really complicated and error prone. Unless we have anything like that then I would start from tasks and build up the necessary to make a decision at the higher level.

Seriously speaking, do you have an example, when summing per-process oom_score will work better?

Especially, if we're talking about customizing oom_score calculation, it makes no sence to me. How you will sum process timestamps?

Quoted 2 lines · Roman Gushchin

Quoted 1 line · Michal Hocko

On Thu 24-08-17 14:58:42, Roman Gushchin wrote:

[...]

Quoted 9 lines

Quoted 4 lines

Quoted 2 lines

Both ways are not ideal, and sum of the processes is not ideal too. Especially, if you take oom_score_adj into account. Will you respect it?

Yes, and I do not see any reason why we shouldn't.

It makes things even more complicated.
Right now task's oom_score can be in (~ -total_memory, ~ +2*total_memory) range, and it you're starting summing it, it can be multiplied by number of tasks... Weird.

oom_score_adj is just a normalized bias so if tasks inside oom will use it the whole memcg will get accumulated bias from all such tasks so it is not completely off. I agree that the more tasks use the bias the more biased the whole memcg will be. This might or might not be a problem. As you are trying to reimplement the existing oom killer implementation I do not think we cannot simply ignore API which people are used to.

If this was a configurable oom policy then I could see how ignoring oom_score_adj is acceptable because it would be an explicit opt-in.

Quoted 1 line

It also will be different in case of system and memcg-wide OOM.

Why, we do honor oom_score_adj for the memcg OOM now and in fact the kernel memcg OOM killer shouldn't be very much different from the global one except for the tasks scope.

Quoted 14 lines

Quoted 13 lines

Quoted 8 lines

I've started actually with such approach, but then found it weird.

Quoted 3 lines

Besides that you have
to check each task for over-killing anyway. So I do not see any performance merits here.

It's an implementation detail, and we can hopefully get rid of it at some point.

Well, we might do some estimations and ignore oom scopes but I that sounds really complicated and error prone. Unless we have anything like that then I would start from tasks and build up the necessary to make a decision at the higher level.

Quoted 2 lines

Seriously speaking, do you have an example, when summing per-process oom_score will work better?

The primary reason I am pushing for this is to have the common iterator code path (which we have since Vladimir has unified memcg and global oom paths) and only parametrize the value calculation and victim selection.

Quoted 21 lines · Michal Hocko

Quoted 2 lines · Roman Gushchin

Quoted 1 line · Michal Hocko

On Thu 24-08-17 14:58:42, Roman Gushchin wrote:

[...]

Quoted 9 lines

Quoted 4 lines

Quoted 2 lines

Both ways are not ideal, and sum of the processes is not ideal too. Especially, if you take oom_score_adj into account. Will you respect it?

Yes, and I do not see any reason why we shouldn't.

It makes things even more complicated.
Right now task's oom_score can be in (~ -total_memory, ~ +2*total_memory) range, and it you're starting summing it, it can be multiplied by number of tasks... Weird.

oom_score_adj is just a normalized bias so if tasks inside oom will use it the whole memcg will get accumulated bias from all such tasks so it is not completely off. I agree that the more tasks use the bias the more biased the whole memcg will be. This might or might not be a problem. As you are trying to reimplement the existing oom killer implementation I do not think we cannot simply ignore API which people are used to.

Quoted 8 lines

If this was a configurable oom policy then I could see how ignoring oom_score_adj is acceptable because it would be an explicit opt-in.

Quoted 1 line

It also will be different in case of system and memcg-wide OOM.

Why, we do honor oom_score_adj for the memcg OOM now and in fact the kernel memcg OOM killer shouldn't be very much different from the global one except for the tasks scope.

Assume, you have two tasks (2Gb and 1Gb) in a cgroup with limit 3Gb. The second task has oom_score_adj +100. Total memory is 64Gb, for example.

I case of memcg-wide oom first task will be selected; in case of system-wide OOM - the second.

Personally I don't like this, but it looks like we have to respect oom_score_adj set to -1000, I'll alter my patch.

Quoted 13 lines

Quoted 13 lines

Quoted 12 lines

Quoted 7 lines

I've started actually with such approach, but then found it weird.

Quoted 3 lines

Besides that you have
to check each task for over-killing anyway. So I do not see any performance merits here.

It's an implementation detail, and we can hopefully get rid of it at some point.

Well, we might do some estimations and ignore oom scopes but I that sounds really complicated and error prone. Unless we have anything like that then I would start from tasks and build up the necessary to make a decision at the higher level.

Quoted 6 lines

Quoted 2 lines

Seriously speaking, do you have an example, when summing per-process oom_score will work better?

The primary reason I am pushing for this is to have the common iterator code path (which we have since Vladimir has unified memcg and global oom paths) and only parametrize the value calculation and victim selection.

I agree, but I'm not sure that we can (and have to) totally unify the way, how oom_score is calculated for processes and cgroups.

But I'd like to see an unified oom_priority approach. This will allow to define an OOM killing order in a clear way, and use size-based tiebreaking for items of the same priority. Root-cgroup processes will be compared with other memory consumers by oom_priority first and oom_score afterwards.

Quoted 2 lines · Roman Gushchin

Quoted 1 line · Michal Hocko

On Thu 24-08-17 14:58:42, Roman Gushchin wrote:

[...]

Quoted 9 lines

Quoted 4 lines

Quoted 2 lines

Both ways are not ideal, and sum of the processes is not ideal too. Especially, if you take oom_score_adj into account. Will you respect it?

Yes, and I do not see any reason why we shouldn't.

It makes things even more complicated.
Right now task's oom_score can be in (~ -total_memory, ~ +2*total_memory) range, and it you're starting summing it, it can be multiplied by number of tasks... Weird.

oom_score_adj is just a normalized bias so if tasks inside oom will use it the whole memcg will get accumulated bias from all such tasks so it is not completely off. I agree that the more tasks use the bias the more biased the whole memcg will be. This might or might not be a problem. As you are trying to reimplement the existing oom killer implementation I do not think we cannot simply ignore API which people are used to.

If this was a configurable oom policy then I could see how ignoring oom_score_adj is acceptable because it would be an explicit opt-in.

Quoted 1 line

It also will be different in case of system and memcg-wide OOM.

Why, we do honor oom_score_adj for the memcg OOM now and in fact the kernel memcg OOM killer shouldn't be very much different from the global one except for the tasks scope.

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

I've started actually with such approach, but then found it weird.

Quoted 3 lines

Besides that you have
to check each task for over-killing anyway. So I do not see any performance merits here.

It's an implementation detail, and we can hopefully get rid of it at some point.

Quoted 7 lines

Quoted 4 lines

Well, we might do some estimations and ignore oom scopes but I that sounds really complicated and error prone. Unless we have anything like that then I would start from tasks and build up the necessary to make a decision at the higher level.

Seriously speaking, do you have an example, when summing per-process oom_score will work better?

The primary reason I am pushing for this is to have the common iterator code path (which we have since Vladimir has unified memcg and global oom paths) and only parametrize the value calculation and victim selection.

Quoted 2 lines

Especially, if we're talking about customizing oom_score calculation, it makes no sence to me. How you will sum process timestamps?

Well, I meant you could sum oom_badness for your particular implementation. If we need some other policy then this wouldn't work and that's why I've said that I would like to preserve the current common code and only parametrize value calculation and victim selection...

I've spent some time to implement such a version.

It really became shorter and more existing code were reused, howewer I've met a couple of serious issues:

1) Simple summing of per-task oom_score doesn't make sense.
First, we calculate oom_score per-task, while should sum per-process values, or, better, per-mm struct. We can take only threa-group leader's score into account, but it's also not 100% accurate.
And, again, we have a question what to do with per-task oom_score_adj, if we don't task the task's oom_score into account.

Using memcg stats still looks to me as a more accurate and consistent way of estimating memcg memory footprint.

Quoted 12 lines · Roman Gushchin

I've spent some time to implement such a version.

It really became shorter and more existing code were reused, howewer I've met a couple of serious issues:

1) Simple summing of per-task oom_score doesn't make sense.
First, we calculate oom_score per-task, while should sum per-process values, or, better, per-mm struct. We can take only threa-group leader's score into account, but it's also not 100% accurate.
And, again, we have a question what to do with per-task oom_score_adj, if we don't task the task's oom_score into account.

Quoted 3 lines

Using memcg stats still looks to me as a more accurate and consistent way of estimating memcg memory footprint.

The patchset is introducing a new methodology for selecting oom victims so you can define how cgroups are compared vs other cgroups with your own "badness" calculation. I think your implementation based heavily on anon and unevictable lrus and unreclaimable slab is fine and you can describe that detail in the documentation (along with the caveat that it is only calculated for nodes in the allocation's mempolicy). With memory.oom_priority, the user has full ability to change that selection. Process selection heuristics have changed over time themselves, it's not something that must be backwards compatibile and trying to sum the usage from each of the cgroup's mm_struct's and respect oom_score_adj is unnecessarily complex.

So, it looks to me that we're close to an acceptable version, and the only remaining question is the default behavior (when oom_group is not set).

Michal suggests to ignore non-oom_group memcgs, and compare tasks with memcgs with oom_group set. This makes the whole thing completely opt-in, but then we probably need another knob (or value) to select between "select memcg, kill biggest task" and "select memcg, kill all tasks".

Also, as the whole thing is based on comparison between processes and memcgs, we probably need oom_priority for processes.

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

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

mm/memcontrol.c +0 -0

@@ -2760,7 +2761,12 @@ static void select_victim_memcg(struct mem_cgroup *root, struct oom_control *oc)
 			if (iter->oom_score == 0)
 				continue;
 

Quoted 3 lines · Roman Gushchin

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

Why do we need a range here?

Quoted 3 lines

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

So this is basically orthogonal to the score based selection and the real size is only the tiebreaker for same priorities? Could you describe the usecase? Becasuse to me this sounds like a separate oom killer strategy. I can imagine somebody might be interested (e.g. always kill the oldest memcgs...) but an explicit range wouldn't fly with such a usecase very well.

That brings me back to my original suggestion. Wouldn't a "register an oom strategy" approach much better than blending things together and then have to wrap heads around different combinations of tunables?

[...]

Quoted 4 lines

mm/memcontrol.c +0 -0

@@ -2760,7 +2761,12 @@ static void select_victim_memcg(struct mem_cgroup *root, struct oom_control *oc)
 			if (iter->oom_score == 0)
 				continue;
 

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

Quoted 10 lines

mm/memcontrol.c +6 -1

-			if (iter->oom_score > score) {
+			if (iter->oom_priority > prio) {
+				memcg = iter;
+				prio = iter->oom_priority;
+				score = iter->oom_score;
+			} else if (iter->oom_priority == prio &&
+				   iter->oom_score > score) {
 				memcg = iter;
 				score = iter->oom_score;
 			}

Just a minor thing. Why do we even have to calculate oom_score when we use it only as a tiebreaker?

Quoted 6 lines · Michal Hocko

Quoted 3 lines · Roman Gushchin

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

Why do we need a range here?

No specific reason, both [INT_MIN, INT_MAX] and [-10000, 10000] will work equally.

We should be able to predefine an OOM killing order for any reasonable amount of cgroups.

Quoted 10 lines

Quoted 3 lines

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

So this is basically orthogonal to the score based selection and the real size is only the tiebreaker for same priorities? Could you describe the usecase? Becasuse to me this sounds like a separate oom killer strategy. I can imagine somebody might be interested (e.g. always kill the oldest memcgs...) but an explicit range wouldn't fly with such a usecase very well.

The usecase: you have a machine with several containerized workloads of different importance, and some system-level stuff, also in (memory) cgroups.
In case of global memory shortage, some workloads should be killed in a first order, others should be killed only if there is no other option. Several workloads can have equal importance. Size-based tiebreaking is very useful to catch memory leakers amongst them.

Quoted 3 lines

That brings me back to my original suggestion. Wouldn't a "register an oom strategy" approach much better than blending things together and then have to wrap heads around different combinations of tunables?

Well, I believe that 90% of this patchset is still relevant;

the only
thing you might want to customize/replace size-based tiebreaking with something else (like timestamp-based tiebreaking, mentioned by David earlier).

What about tunables, there are two, and they are completely orthogonal:
1) oom_priority allows to define an order, in which cgroups will be OOMed
2) oom_kill_all defines if all or just one task should be killed

So, I don't think it's a too complex interface.

Again, I'm not against oom strategy approach, it just looks as a much bigger project, and I do not see a big need.

Do you have an example, which can't be effectively handled by an approach I'm suggesting?

Quoted 6 lines · Roman Gushchin

Quoted 5 lines · Michal Hocko

Quoted 3 lines · Roman Gushchin

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

Quoted 4 lines

Quoted 1 line

Why do we need a range here?

No specific reason, both [INT_MIN, INT_MAX] and [-10000, 10000] will work equally.

Then do not enforce a range because this just reduces possible usecases (e.g. timestamp one...).

Quoted 15 lines

We should be able to predefine an OOM killing order for any reasonable amount of cgroups.

Quoted 10 lines

Quoted 3 lines

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

So this is basically orthogonal to the score based selection and the real size is only the tiebreaker for same priorities? Could you describe the usecase? Becasuse to me this sounds like a separate oom killer strategy. I can imagine somebody might be interested (e.g. always kill the oldest memcgs...) but an explicit range wouldn't fly with such a usecase very well.

Quoted 7 lines

The usecase: you have a machine with several containerized workloads of different importance, and some system-level stuff, also in (memory) cgroups.
In case of global memory shortage, some workloads should be killed in a first order, others should be killed only if there is no other option. Several workloads can have equal importance. Size-based tiebreaking is very useful to catch memory leakers amongst them.

OK, please document that in the changelog.

Quoted 6 lines

Quoted 4 lines

That brings me back to my original suggestion. Wouldn't a "register an oom strategy" approach much better than blending things together and then have to wrap heads around different combinations of tunables?

Well, I believe that 90% of this patchset is still relevant;

agreed and didn't say otherwise.

Quoted 4 lines

the only
thing you might want to customize/replace size-based tiebreaking with something else (like timestamp-based tiebreaking, mentioned by David earlier).

Quoted 8 lines

What about tunables, there are two, and they are completely orthogonal:
1) oom_priority allows to define an order, in which cgroups will be OOMed
2) oom_kill_all defines if all or just one task should be killed

So, I don't think it's a too complex interface.

Again, I'm not against oom strategy approach, it just looks as a much bigger project, and I do not see a big need.

Well, I was thinking that our current oom victim selection code is quite extensible already. Your patches will teach it kill the whole group semantic which is already very useful. Now we can talk about the selection criteria and this is something to be replaceable. Because even the current discussion has shown that different people might and will have different requirements. Can we structure the code in such a way that new comparison algorithm would be simple to add without reworking the whole selection logic?

Quoted 3 lines

Do you have an example, which can't be effectively handled by an approach I'm suggesting?

No, I do not have any which would be _explicitly_ requested but I do envision new requirements will emerge. The most probable one would be kill the youngest container because that would imply the least amount of work wasted.

Quoted 7 lines

Quoted 2 lines

Do you have an example, which can't be effectively handled by an approach I'm suggesting?

No, I do not have any which would be _explicitly_ requested but I do envision new requirements will emerge. The most probable one would be kill the youngest container because that would imply the least amount of work wasted.

I agree, this a nice feature. It can be implemented in userspace by setting oom_priority.

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

index 53b44425ef35..5c29a3dd591b 100644
@@ -817,67 +817,12 @@ static bool task_will_free_mem(struct task_struct *task)
 	return ret;
 }
 
-static void oom_kill_process(struct oom_control *oc, const char *message)
+static void __oom_kill_process(struct task_struct *victim)
 {

mm/oom_kill.c +2 -0

+static void oom_kill_process(struct oom_control *oc, const char *message)
+{