| Message ID | 20180730180100.25079-4-guro@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | introduce memory.oom.group | expand |
On Mon 30-07-18 11:01:00, Roman Gushchin wrote: > For some workloads an intervention from the OOM killer > can be painful. Killing a random task can bring > the workload into an inconsistent state. > > Historically, there are two common solutions for this > problem: > 1) enabling panic_on_oom, > 2) using a userspace daemon to monitor OOMs and kill > all outstanding processes. > > Both approaches have their downsides: > rebooting on each OOM is an obvious waste of capacity, > and handling all in userspace is tricky and requires > a userspace agent, which will monitor all cgroups > for OOMs. > > In most cases an in-kernel after-OOM cleaning-up > mechanism can eliminate the necessity of enabling > panic_on_oom. Also, it can simplify the cgroup > management for userspace applications. > > This commit introduces a new knob for cgroup v2 memory > controller: memory.oom.group. The knob determines > whether the cgroup should be treated as a single > unit by the OOM killer. If set, the cgroup and its > descendants are killed together or not at all. I do not want to nit pick on wording but unit is not really a good description. I would expect that to mean that the oom killer will consider the unit also when selecting the task and that is not the case. I would be more explicit about this being a single killable entity because it forms an indivisible workload. You can reuse http://lkml.kernel.org/r/20180730080357.GA24267@dhcp22.suse.cz if you want. [...] > +/** > + * mem_cgroup_get_oom_group - get a memory cgroup to clean up after OOM > + * @victim: task to be killed by the OOM killer > + * @oom_domain: memcg in case of memcg OOM, NULL in case of system-wide OOM > + * > + * Returns a pointer to a memory cgroup, which has to be cleaned up > + * by killing all belonging OOM-killable tasks. Caller has to call mem_cgroup_put on the returned non-null memcg. > + */ > +struct mem_cgroup *mem_cgroup_get_oom_group(struct task_struct *victim, > + struct mem_cgroup *oom_domain) > +{ > + struct mem_cgroup *oom_group = NULL; > + struct mem_cgroup *memcg; > + > + if (!cgroup_subsys_on_dfl(memory_cgrp_subsys)) > + return NULL; > + > + if (!oom_domain) > + oom_domain = root_mem_cgroup; > + > + rcu_read_lock(); > + > + memcg = mem_cgroup_from_task(victim); > + if (!memcg || memcg == root_mem_cgroup) > + goto out; When can we have memcg == NULL? victim should be always non-NULL. Also why do you need to special case the root_mem_cgroup here. The loop below should handle that just fine no? > + > + /* > + * Traverse the memory cgroup hierarchy from the victim task's > + * cgroup up to the OOMing cgroup (or root) to find the > + * highest-level memory cgroup with oom.group set. > + */ > + for (; memcg; memcg = parent_mem_cgroup(memcg)) { > + if (memcg->oom_group) > + oom_group = memcg; > + > + if (memcg == oom_domain) > + break; > + } > + > + if (oom_group) > + css_get(&oom_group->css); > +out: > + rcu_read_unlock(); > + > + return oom_group; > +} > + [...] > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > index 8bded6b3205b..08f30ed5abed 100644 > --- a/mm/oom_kill.c > +++ b/mm/oom_kill.c > @@ -914,6 +914,19 @@ static void __oom_kill_process(struct task_struct *victim) > } > #undef K > > +/* > + * Kill provided task unless it's secured by setting > + * oom_score_adj to OOM_SCORE_ADJ_MIN. > + */ > +static int oom_kill_memcg_member(struct task_struct *task, void *unused) > +{ > + if (task->signal->oom_score_adj != OOM_SCORE_ADJ_MIN) { > + get_task_struct(task); > + __oom_kill_process(task); > + } > + return 0; > +} > + > static void oom_kill_process(struct oom_control *oc, const char *message) > { > struct task_struct *p = oc->chosen; > @@ -921,6 +934,7 @@ static void oom_kill_process(struct oom_control *oc, const char *message) > struct task_struct *victim = p; > struct task_struct *child; > struct task_struct *t; > + struct mem_cgroup *oom_group; > unsigned int victim_points = 0; > static DEFINE_RATELIMIT_STATE(oom_rs, DEFAULT_RATELIMIT_INTERVAL, > DEFAULT_RATELIMIT_BURST); > @@ -974,7 +988,22 @@ static void oom_kill_process(struct oom_control *oc, const char *message) > } > read_unlock(&tasklist_lock); > > + /* > + * Do we need to kill the entire memory cgroup? > + * Or even one of the ancestor memory cgroups? > + * Check this out before killing the victim task. > + */ > + oom_group = mem_cgroup_get_oom_group(victim, oc->memcg); > + > __oom_kill_process(victim); > + > + /* > + * If necessary, kill all tasks in the selected memory cgroup. > + */ > + if (oom_group) { we want a printk explaining that we are going to tear down the whole oom_group here. > + mem_cgroup_scan_tasks(oom_group, oom_kill_memcg_member, NULL); > + mem_cgroup_put(oom_group); > + } > } Other than that looks good to me. My concern that the previous implementation was more consistent because we were comparing memcgs still holds but if there is no way forward that direction this should be acceptable as well. After above small things are addressed you can add Acked-by: Michal Hocko <mhocko@suse.com> Thanks!
On Tue, Jul 31, 2018 at 11:07:00AM +0200, Michal Hocko wrote: > On Mon 30-07-18 11:01:00, Roman Gushchin wrote: > > For some workloads an intervention from the OOM killer > > can be painful. Killing a random task can bring > > the workload into an inconsistent state. > > > > Historically, there are two common solutions for this > > problem: > > 1) enabling panic_on_oom, > > 2) using a userspace daemon to monitor OOMs and kill > > all outstanding processes. > > > > Both approaches have their downsides: > > rebooting on each OOM is an obvious waste of capacity, > > and handling all in userspace is tricky and requires > > a userspace agent, which will monitor all cgroups > > for OOMs. > > > > In most cases an in-kernel after-OOM cleaning-up > > mechanism can eliminate the necessity of enabling > > panic_on_oom. Also, it can simplify the cgroup > > management for userspace applications. > > > > This commit introduces a new knob for cgroup v2 memory > > controller: memory.oom.group. The knob determines > > whether the cgroup should be treated as a single > > unit by the OOM killer. If set, the cgroup and its > > descendants are killed together or not at all. > > I do not want to nit pick on wording but unit is not really a good > description. I would expect that to mean that the oom killer will > consider the unit also when selecting the task and that is not the case. > I would be more explicit about this being a single killable entity > because it forms an indivisible workload. > > You can reuse http://lkml.kernel.org/r/20180730080357.GA24267@dhcp22.suse.cz > if you want. Ok, I'll do my best to make it clearer. > > [...] > > +/** > > + * mem_cgroup_get_oom_group - get a memory cgroup to clean up after OOM > > + * @victim: task to be killed by the OOM killer > > + * @oom_domain: memcg in case of memcg OOM, NULL in case of system-wide OOM > > + * > > + * Returns a pointer to a memory cgroup, which has to be cleaned up > > + * by killing all belonging OOM-killable tasks. > > Caller has to call mem_cgroup_put on the returned non-null memcg. Added. > > > + */ > > +struct mem_cgroup *mem_cgroup_get_oom_group(struct task_struct *victim, > > + struct mem_cgroup *oom_domain) > > +{ > > + struct mem_cgroup *oom_group = NULL; > > + struct mem_cgroup *memcg; > > + > > + if (!cgroup_subsys_on_dfl(memory_cgrp_subsys)) > > + return NULL; > > + > > + if (!oom_domain) > > + oom_domain = root_mem_cgroup; > > + > > + rcu_read_lock(); > > + > > + memcg = mem_cgroup_from_task(victim); > > + if (!memcg || memcg == root_mem_cgroup) > > + goto out; > > When can we have memcg == NULL? victim should be always non-NULL. > Also why do you need to special case the root_mem_cgroup here. The loop > below should handle that just fine no? Idk, I prefer to keep an explicit root_mem_cgroup check, rather than traversing the tree and relying on an inability to set oom_group on the root. !memcg check removed, you're right. > > > + > > + /* > > + * Traverse the memory cgroup hierarchy from the victim task's > > + * cgroup up to the OOMing cgroup (or root) to find the > > + * highest-level memory cgroup with oom.group set. > > + */ > > + for (; memcg; memcg = parent_mem_cgroup(memcg)) { > > + if (memcg->oom_group) > > + oom_group = memcg; > > + > > + if (memcg == oom_domain) > > + break; > > + } > > + > > + if (oom_group) > > + css_get(&oom_group->css); > > +out: > > + rcu_read_unlock(); > > + > > + return oom_group; > > +} > > + > [...] > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > > index 8bded6b3205b..08f30ed5abed 100644 > > --- a/mm/oom_kill.c > > +++ b/mm/oom_kill.c > > @@ -914,6 +914,19 @@ static void __oom_kill_process(struct task_struct *victim) > > } > > #undef K > > > > +/* > > + * Kill provided task unless it's secured by setting > > + * oom_score_adj to OOM_SCORE_ADJ_MIN. > > + */ > > +static int oom_kill_memcg_member(struct task_struct *task, void *unused) > > +{ > > + if (task->signal->oom_score_adj != OOM_SCORE_ADJ_MIN) { > > + get_task_struct(task); > > + __oom_kill_process(task); > > + } > > + return 0; > > +} > > + > > static void oom_kill_process(struct oom_control *oc, const char *message) > > { > > struct task_struct *p = oc->chosen; > > @@ -921,6 +934,7 @@ static void oom_kill_process(struct oom_control *oc, const char *message) > > struct task_struct *victim = p; > > struct task_struct *child; > > struct task_struct *t; > > + struct mem_cgroup *oom_group; > > unsigned int victim_points = 0; > > static DEFINE_RATELIMIT_STATE(oom_rs, DEFAULT_RATELIMIT_INTERVAL, > > DEFAULT_RATELIMIT_BURST); > > @@ -974,7 +988,22 @@ static void oom_kill_process(struct oom_control *oc, const char *message) > > } > > read_unlock(&tasklist_lock); > > > > + /* > > + * Do we need to kill the entire memory cgroup? > > + * Or even one of the ancestor memory cgroups? > > + * Check this out before killing the victim task. > > + */ > > + oom_group = mem_cgroup_get_oom_group(victim, oc->memcg); > > + > > __oom_kill_process(victim); > > + > > + /* > > + * If necessary, kill all tasks in the selected memory cgroup. > > + */ > > + if (oom_group) { > > we want a printk explaining that we are going to tear down the whole > oom_group here. Does this looks good? Or it's better to remove "memory." prefix? [ 52.835327] Out of memory: Kill process 1221 (allocate) score 241 or sacrifice child [ 52.836625] Killed process 1221 (allocate) total-vm:2257144kB, anon-rss:2009128kB, file-rss:4kB, shmem-rss:0kB [ 52.841431] Tasks in /A1 are going to be killed due to memory.oom.group set [ 52.869439] Killed process 1217 (allocate) total-vm:2052344kB, anon-rss:1704036kB, file-rss:0kB, shmem-rss:0kB [ 52.875601] Killed process 1218 (allocate) total-vm:106668kB, anon-rss:24668kB, file-rss:0kB, shmem-rss:0kB [ 52.882914] Killed process 1219 (allocate) total-vm:106668kB, anon-rss:21528kB, file-rss:0kB, shmem-rss:0kB [ 52.891806] Killed process 1220 (allocate) total-vm:2257144kB, anon-rss:1984120kB, file-rss:4kB, shmem-rss:0kB [ 52.903770] Killed process 1221 (allocate) total-vm:2257144kB, anon-rss:2009128kB, file-rss:4kB, shmem-rss:0kB [ 52.905574] Killed process 1222 (allocate) total-vm:2257144kB, anon-rss:2063640kB, file-rss:0kB, shmem-rss:0kB [ 53.202153] oom_reaper: reaped process 1222 (allocate), now anon-rss:0kB, file-rss:0kB, shmem-rss:0kB > > > + mem_cgroup_scan_tasks(oom_group, oom_kill_memcg_member, NULL); > > + mem_cgroup_put(oom_group); > > + } > > } > > Other than that looks good to me. My concern that the previous > implementation was more consistent because we were comparing memcgs > still holds but if there is no way forward that direction this should be > acceptable as well. > > After above small things are addressed you can add > Acked-by: Michal Hocko <mhocko@suse.com> Thank you!
On Tue 31-07-18 18:14:48, Roman Gushchin wrote: > On Tue, Jul 31, 2018 at 11:07:00AM +0200, Michal Hocko wrote: > > On Mon 30-07-18 11:01:00, Roman Gushchin wrote: > > > For some workloads an intervention from the OOM killer > > > can be painful. Killing a random task can bring > > > the workload into an inconsistent state. > > > > > > Historically, there are two common solutions for this > > > problem: > > > 1) enabling panic_on_oom, > > > 2) using a userspace daemon to monitor OOMs and kill > > > all outstanding processes. > > > > > > Both approaches have their downsides: > > > rebooting on each OOM is an obvious waste of capacity, > > > and handling all in userspace is tricky and requires > > > a userspace agent, which will monitor all cgroups > > > for OOMs. > > > > > > In most cases an in-kernel after-OOM cleaning-up > > > mechanism can eliminate the necessity of enabling > > > panic_on_oom. Also, it can simplify the cgroup > > > management for userspace applications. > > > > > > This commit introduces a new knob for cgroup v2 memory > > > controller: memory.oom.group. The knob determines > > > whether the cgroup should be treated as a single > > > unit by the OOM killer. If set, the cgroup and its > > > descendants are killed together or not at all. > > > > I do not want to nit pick on wording but unit is not really a good > > description. I would expect that to mean that the oom killer will > > consider the unit also when selecting the task and that is not the case. > > I would be more explicit about this being a single killable entity > > because it forms an indivisible workload. > > > > You can reuse http://lkml.kernel.org/r/20180730080357.GA24267@dhcp22.suse.cz > > if you want. > > Ok, I'll do my best to make it clearer. > > > > > [...] > > > +/** > > > + * mem_cgroup_get_oom_group - get a memory cgroup to clean up after OOM > > > + * @victim: task to be killed by the OOM killer > > > + * @oom_domain: memcg in case of memcg OOM, NULL in case of system-wide OOM > > > + * > > > + * Returns a pointer to a memory cgroup, which has to be cleaned up > > > + * by killing all belonging OOM-killable tasks. > > > > Caller has to call mem_cgroup_put on the returned non-null memcg. > > Added. > > > > > > + */ > > > +struct mem_cgroup *mem_cgroup_get_oom_group(struct task_struct *victim, > > > + struct mem_cgroup *oom_domain) > > > +{ > > > + struct mem_cgroup *oom_group = NULL; > > > + struct mem_cgroup *memcg; > > > + > > > + if (!cgroup_subsys_on_dfl(memory_cgrp_subsys)) > > > + return NULL; > > > + > > > + if (!oom_domain) > > > + oom_domain = root_mem_cgroup; > > > + > > > + rcu_read_lock(); > > > + > > > + memcg = mem_cgroup_from_task(victim); > > > + if (!memcg || memcg == root_mem_cgroup) > > > + goto out; > > > > When can we have memcg == NULL? victim should be always non-NULL. > > Also why do you need to special case the root_mem_cgroup here. The loop > > below should handle that just fine no? > > Idk, I prefer to keep an explicit root_mem_cgroup check, > rather than traversing the tree and relying on an inability > to set oom_group on the root. I will not insist but this just makes the code harder to read. [...] > > > + if (oom_group) { > > > > we want a printk explaining that we are going to tear down the whole > > oom_group here. > > Does this looks good? > Or it's better to remove "memory." prefix? > > [ 52.835327] Out of memory: Kill process 1221 (allocate) score 241 or sacrifice child > [ 52.836625] Killed process 1221 (allocate) total-vm:2257144kB, anon-rss:2009128kB, file-rss:4kB, shmem-rss:0kB > [ 52.841431] Tasks in /A1 are going to be killed due to memory.oom.group set Yes, looks good to me. > [ 52.869439] Killed process 1217 (allocate) total-vm:2052344kB, anon-rss:1704036kB, file-rss:0kB, shmem-rss:0kB > [ 52.875601] Killed process 1218 (allocate) total-vm:106668kB, anon-rss:24668kB, file-rss:0kB, shmem-rss:0kB > [ 52.882914] Killed process 1219 (allocate) total-vm:106668kB, anon-rss:21528kB, file-rss:0kB, shmem-rss:0kB > [ 52.891806] Killed process 1220 (allocate) total-vm:2257144kB, anon-rss:1984120kB, file-rss:4kB, shmem-rss:0kB > [ 52.903770] Killed process 1221 (allocate) total-vm:2257144kB, anon-rss:2009128kB, file-rss:4kB, shmem-rss:0kB > [ 52.905574] Killed process 1222 (allocate) total-vm:2257144kB, anon-rss:2063640kB, file-rss:0kB, shmem-rss:0kB > [ 53.202153] oom_reaper: reaped process 1222 (allocate), now anon-rss:0kB, file-rss:0kB, shmem-rss:0kB > > > > > > + mem_cgroup_scan_tasks(oom_group, oom_kill_memcg_member, NULL); > > > + mem_cgroup_put(oom_group); > > > + } > > > } > > > > Other than that looks good to me. My concern that the previous > > implementation was more consistent because we were comparing memcgs > > still holds but if there is no way forward that direction this should be > > acceptable as well. > > > > After above small things are addressed you can add > > Acked-by: Michal Hocko <mhocko@suse.com> > > Thank you!
On Wed, Aug 01, 2018 at 07:55:03AM +0200, Michal Hocko wrote: > On Tue 31-07-18 18:14:48, Roman Gushchin wrote: > > On Tue, Jul 31, 2018 at 11:07:00AM +0200, Michal Hocko wrote: > > > On Mon 30-07-18 11:01:00, Roman Gushchin wrote: > > > > +struct mem_cgroup *mem_cgroup_get_oom_group(struct task_struct *victim, > > > > + struct mem_cgroup *oom_domain) > > > > +{ > > > > + struct mem_cgroup *oom_group = NULL; > > > > + struct mem_cgroup *memcg; > > > > + > > > > + if (!cgroup_subsys_on_dfl(memory_cgrp_subsys)) > > > > + return NULL; > > > > + > > > > + if (!oom_domain) > > > > + oom_domain = root_mem_cgroup; > > > > + > > > > + rcu_read_lock(); > > > > + > > > > + memcg = mem_cgroup_from_task(victim); > > > > + if (!memcg || memcg == root_mem_cgroup) > > > > + goto out; > > > > > > When can we have memcg == NULL? victim should be always non-NULL. > > > Also why do you need to special case the root_mem_cgroup here. The loop > > > below should handle that just fine no? > > > > Idk, I prefer to keep an explicit root_mem_cgroup check, > > rather than traversing the tree and relying on an inability > > to set oom_group on the root. > > I will not insist but this just makes the code harder to read. Just FYI, I'd prefer the explicit check. The loop would do the right thing, but it's a little too implicit and subtle for my taste...
On Mon, Jul 30, 2018 at 11:01:00AM -0700, Roman Gushchin wrote: > For some workloads an intervention from the OOM killer > can be painful. Killing a random task can bring > the workload into an inconsistent state. > > Historically, there are two common solutions for this > problem: > 1) enabling panic_on_oom, > 2) using a userspace daemon to monitor OOMs and kill > all outstanding processes. > > Both approaches have their downsides: > rebooting on each OOM is an obvious waste of capacity, > and handling all in userspace is tricky and requires > a userspace agent, which will monitor all cgroups > for OOMs. > > In most cases an in-kernel after-OOM cleaning-up > mechanism can eliminate the necessity of enabling > panic_on_oom. Also, it can simplify the cgroup > management for userspace applications. > > This commit introduces a new knob for cgroup v2 memory > controller: memory.oom.group. The knob determines > whether the cgroup should be treated as a single > unit by the OOM killer. If set, the cgroup and its > descendants are killed together or not at all. > > To determine which cgroup has to be killed, we do > traverse the cgroup hierarchy from the victim task's > cgroup up to the OOMing cgroup (or root) and looking > for the highest-level cgroup with memory.oom.group set. > > Tasks with the OOM protection (oom_score_adj set to -1000) > are treated as an exception and are never killed. > > This patch doesn't change the OOM victim selection algorithm. > > Signed-off-by: Roman Gushchin <guro@fb.com> > Cc: Michal Hocko <mhocko@suse.com> > Cc: Johannes Weiner <hannes@cmpxchg.org> > Cc: David Rientjes <rientjes@google.com> > Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Cc: Tejun Heo <tj@kernel.org> The semantics make sense to me and the code is straight-forward. With Michal's other feedback incorporated, please feel free to add: Acked-by: Johannes Weiner <hannes@cmpxchg.org>
diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst index 8a2c52d5c53b..f11de1b78cfc 100644 --- a/Documentation/admin-guide/cgroup-v2.rst +++ b/Documentation/admin-guide/cgroup-v2.rst @@ -1069,6 +1069,22 @@ PAGE_SIZE multiple when read back. high limit is used and monitored properly, this limit's utility is limited to providing the final safety net. + memory.oom.group + A read-write single value file which exists on non-root + cgroups. The default value is "0". + + Determines whether the cgroup should be treated as a single + unit by the OOM killer. If set, the cgroup and its descendants + are killed together or not at all. This can be used to avoid + partial kills to guarantee workload integrity. + + Tasks with the OOM protection (oom_score_adj set to -1000) + are treated as an exception and are never killed. + + If the OOM killer is invoked in a cgroup, it's not going + to kill any tasks outside of this cgroup, regardless + memory.oom.group values of ancestor cgroups. + memory.events A read-only flat-keyed file which exists on non-root cgroups. The following entries are defined. Unless specified diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index e53e00cdbe3f..05c9c867a3ab 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -213,6 +213,11 @@ struct mem_cgroup { */ bool use_hierarchy; + /* + * Should the OOM killer kill all belonging tasks, had it kill one? + */ + bool oom_group; + /* protected by memcg_oom_lock */ bool oom_lock; int under_oom; @@ -517,6 +522,8 @@ static inline bool task_in_memcg_oom(struct task_struct *p) } bool mem_cgroup_oom_synchronize(bool wait); +struct mem_cgroup *mem_cgroup_get_oom_group(struct task_struct *victim, + struct mem_cgroup *oom_domain); #ifdef CONFIG_MEMCG_SWAP extern int do_swap_account; @@ -951,6 +958,12 @@ static inline bool mem_cgroup_oom_synchronize(bool wait) return false; } +static inline struct mem_cgroup *mem_cgroup_get_oom_group( + struct task_struct *victim, struct mem_cgroup *oom_domain) +{ + return NULL; +} + static inline unsigned long memcg_page_state(struct mem_cgroup *memcg, int idx) { diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 8c0280b3143e..ade832571091 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -1577,6 +1577,53 @@ bool mem_cgroup_oom_synchronize(bool handle) return true; } +/** + * mem_cgroup_get_oom_group - get a memory cgroup to clean up after OOM + * @victim: task to be killed by the OOM killer + * @oom_domain: memcg in case of memcg OOM, NULL in case of system-wide OOM + * + * Returns a pointer to a memory cgroup, which has to be cleaned up + * by killing all belonging OOM-killable tasks. + */ +struct mem_cgroup *mem_cgroup_get_oom_group(struct task_struct *victim, + struct mem_cgroup *oom_domain) +{ + struct mem_cgroup *oom_group = NULL; + struct mem_cgroup *memcg; + + if (!cgroup_subsys_on_dfl(memory_cgrp_subsys)) + return NULL; + + if (!oom_domain) + oom_domain = root_mem_cgroup; + + rcu_read_lock(); + + memcg = mem_cgroup_from_task(victim); + if (!memcg || memcg == root_mem_cgroup) + goto out; + + /* + * Traverse the memory cgroup hierarchy from the victim task's + * cgroup up to the OOMing cgroup (or root) to find the + * highest-level memory cgroup with oom.group set. + */ + for (; memcg; memcg = parent_mem_cgroup(memcg)) { + if (memcg->oom_group) + oom_group = memcg; + + if (memcg == oom_domain) + break; + } + + if (oom_group) + css_get(&oom_group->css); +out: + rcu_read_unlock(); + + return oom_group; +} + /** * lock_page_memcg - lock a page->mem_cgroup binding * @page: the page @@ -5328,6 +5375,37 @@ static int memory_stat_show(struct seq_file *m, void *v) return 0; } +static int memory_oom_group_show(struct seq_file *m, void *v) +{ + struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(m)); + + seq_printf(m, "%d\n", memcg->oom_group); + + return 0; +} + +static ssize_t memory_oom_group_write(struct kernfs_open_file *of, + char *buf, size_t nbytes, loff_t off) +{ + struct mem_cgroup *memcg = mem_cgroup_from_css(of_css(of)); + int ret, oom_group; + + buf = strstrip(buf); + if (!buf) + return -EINVAL; + + ret = kstrtoint(buf, 0, &oom_group); + if (ret) + return ret; + + if (oom_group != 0 && oom_group != 1) + return -EINVAL; + + memcg->oom_group = oom_group; + + return nbytes; +} + static struct cftype memory_files[] = { { .name = "current", @@ -5369,6 +5447,12 @@ static struct cftype memory_files[] = { .flags = CFTYPE_NOT_ON_ROOT, .seq_show = memory_stat_show, }, + { + .name = "oom.group", + .flags = CFTYPE_NOT_ON_ROOT | CFTYPE_NS_DELEGATABLE, + .seq_show = memory_oom_group_show, + .write = memory_oom_group_write, + }, { } /* terminate */ }; diff --git a/mm/oom_kill.c b/mm/oom_kill.c index 8bded6b3205b..08f30ed5abed 100644 --- a/mm/oom_kill.c +++ b/mm/oom_kill.c @@ -914,6 +914,19 @@ static void __oom_kill_process(struct task_struct *victim) } #undef K +/* + * Kill provided task unless it's secured by setting + * oom_score_adj to OOM_SCORE_ADJ_MIN. + */ +static int oom_kill_memcg_member(struct task_struct *task, void *unused) +{ + if (task->signal->oom_score_adj != OOM_SCORE_ADJ_MIN) { + get_task_struct(task); + __oom_kill_process(task); + } + return 0; +} + static void oom_kill_process(struct oom_control *oc, const char *message) { struct task_struct *p = oc->chosen; @@ -921,6 +934,7 @@ static void oom_kill_process(struct oom_control *oc, const char *message) struct task_struct *victim = p; struct task_struct *child; struct task_struct *t; + struct mem_cgroup *oom_group; unsigned int victim_points = 0; static DEFINE_RATELIMIT_STATE(oom_rs, DEFAULT_RATELIMIT_INTERVAL, DEFAULT_RATELIMIT_BURST); @@ -974,7 +988,22 @@ static void oom_kill_process(struct oom_control *oc, const char *message) } read_unlock(&tasklist_lock); + /* + * Do we need to kill the entire memory cgroup? + * Or even one of the ancestor memory cgroups? + * Check this out before killing the victim task. + */ + oom_group = mem_cgroup_get_oom_group(victim, oc->memcg); + __oom_kill_process(victim); + + /* + * If necessary, kill all tasks in the selected memory cgroup. + */ + if (oom_group) { + mem_cgroup_scan_tasks(oom_group, oom_kill_memcg_member, NULL); + mem_cgroup_put(oom_group); + } } /*
For some workloads an intervention from the OOM killer can be painful. Killing a random task can bring the workload into an inconsistent state. Historically, there are two common solutions for this problem: 1) enabling panic_on_oom, 2) using a userspace daemon to monitor OOMs and kill all outstanding processes. Both approaches have their downsides: rebooting on each OOM is an obvious waste of capacity, and handling all in userspace is tricky and requires a userspace agent, which will monitor all cgroups for OOMs. In most cases an in-kernel after-OOM cleaning-up mechanism can eliminate the necessity of enabling panic_on_oom. Also, it can simplify the cgroup management for userspace applications. This commit introduces a new knob for cgroup v2 memory controller: memory.oom.group. The knob determines whether the cgroup should be treated as a single unit by the OOM killer. If set, the cgroup and its descendants are killed together or not at all. To determine which cgroup has to be killed, we do traverse the cgroup hierarchy from the victim task's cgroup up to the OOMing cgroup (or root) and looking for the highest-level cgroup with memory.oom.group set. Tasks with the OOM protection (oom_score_adj set to -1000) are treated as an exception and are never killed. This patch doesn't change the OOM victim selection algorithm. Signed-off-by: Roman Gushchin <guro@fb.com> Cc: Michal Hocko <mhocko@suse.com> Cc: Johannes Weiner <hannes@cmpxchg.org> Cc: David Rientjes <rientjes@google.com> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Cc: Tejun Heo <tj@kernel.org> --- Documentation/admin-guide/cgroup-v2.rst | 16 +++++++ include/linux/memcontrol.h | 13 +++++ mm/memcontrol.c | 84 +++++++++++++++++++++++++++++++++ mm/oom_kill.c | 29 ++++++++++++ 4 files changed, 142 insertions(+)