introduce memory.oom.group
Subsystems (5)
v2 cover introduce memory.oom.group
This is a tiny implementation of cgroup-aware OOM killer, which adds an ability to kill a cgroup as a single unit and so guarantee the integrity of the workload.
Although it has only a limited functionality in comparison to what now resides in the mm tree (it doesn't change the victim task selection algorithm, doesn't look at memory stas on cgroup level, etc), it's also much simpler and more straightforward. So, hopefully, we can avoid having long debates here, as we had with the full implementation.
As it doesn't prevent any futher development,
and implements an useful and complete feature,
it looks as a sane way forward.
v2->v1:
- added dmesg message about killing all tasks in cgroup
- removed an unnecessary check for memcg being NULL pointer
- adjusted docs and commit message
- rebased to linus/master
--
This patchset is against Linus's tree to avoid conflicts with the cgroup-aware OOM killer patchset in the mm tree. It's intended to replace it.
Two first patches are already in the mm tree.
The first one ("mm: introduce mem_cgroup_put() helper") is totally fine.
Commit message of the second one has to be changed to reflect that it's not a part of the old patchset anymore.
Roman Gushchin (3):
mm: introduce mem_cgroup_put() helper
mm, oom: refactor oom_kill_process()
mm, oom: introduce memory.oom.group
Documentation/admin-guide/cgroup-v2.rst | 18 ++++ include/linux/memcontrol.h | 27 ++++++ mm/memcontrol.c | 93 +++++++++++++++++++ mm/oom_kill.c | 153 ++++++++++++++++++++------------ 4 files changed, 233 insertions(+), 58 deletions(-)
Signature · 1 line
2.14.4
PATCH v2 1/3 mm: introduce mem_cgroup_put() helper
Introduce the mem_cgroup_put() helper, which helps to eliminate guarding memcg css release with "#ifdef CONFIG_MEMCG" in multiple places.
--- include/linux/memcontrol.h | 9 +++++++++ 1 file changed, 9 insertions(+)
include/linux/memcontrol.h +9 -0
@@ -375,6 +375,11 @@ struct mem_cgroup *mem_cgroup_from_css(struct cgroup_subsys_state *css){ return css ? container_of(css, struct mem_cgroup, css) : NULL; } +static inline void mem_cgroup_put (struct mem_cgroup *memcg) +{ +css_put (&memcg->css); +} +#define mem_cgroup_from_counter (counter, member) \ container_of(counter, struct mem_cgroup, member)
@@ -837,6 +842,10 @@ static inline bool task_in_mem_cgroup(struct task_struct *task,return true; } +static inline void mem_cgroup_put (struct mem_cgroup *memcg) +{ +} +static inline struct mem_cgroup * mem_cgroup_iter(struct mem_cgroup *root, struct mem_cgroup *prev,
Signature · 1 line
2.14.4
Re: [PATCH v2 1/3] mm: introduce mem_cgroup_put() helper
Hi Roman,
Quoted 4 lines · On Wed, 1 Aug 2018 17:31:59 -0700 Roman Gushchin <[email protected]> wrote:
Introduce the
mem_cgroup_put()helper, which helps to eliminate guarding memcg css release with "#ifdefCONFIG_MEMCG" in multiple places.
Quoted 8 lines
Signed-off-by Roman Gushchin <[email protected]>Reviewed-by Shakeel Butt <[email protected]>Reviewed-by Andrew Morton <[email protected]>Acked-by Johannes Weiner <[email protected]>Acked-by Michal Hocko <[email protected]>Signed-off-by Andrew Morton <[email protected]>Signed-off-by Stephen Rothwell <[email protected]>
I have no idea why my Signed-off-by is attached to this patch (or Andrew's for that matter) ...
Signature · 2 lines
Cheers, Stephen Rothwell
Re: [PATCH v2 1/3] mm: introduce mem_cgroup_put() helper
Quoted 17 lines · Stephen Rothwell
Hi Roman,
Quoted 4 lines · On Wed, 1 Aug 2018 17:31:59 -0700 Roman Gushchin <[email protected]> wrote:
Introduce the
mem_cgroup_put()helper, which helps to eliminate guarding memcg css release with "#ifdefCONFIG_MEMCG" in multiple places.Quoted 8 lines
Signed-off-by Roman Gushchin <[email protected]>Reviewed-by Shakeel Butt <[email protected]>Reviewed-by Andrew Morton <[email protected]>Acked-by Johannes Weiner <[email protected]>Acked-by Michal Hocko <[email protected]>Signed-off-by Andrew Morton <[email protected]>Signed-off-by Stephen Rothwell <[email protected]>
Quoted 3 lines
I have no idea why my Signed-off-by is attached to this patch (or Andrew's for that matter) ...
Roman might have picked this patch from linux-next and sent as it is.
Shakeel
Re: [PATCH v2 1/3] mm: introduce mem_cgroup_put() helper
In reply to Stephen RothwellQuoted 16 lines · Stephen Rothwell
Hi Roman,
Quoted 4 lines · On Wed, 1 Aug 2018 17:31:59 -0700 Roman Gushchin <[email protected]> wrote:
Introduce the
mem_cgroup_put()helper, which helps to eliminate guarding memcg css release with "#ifdefCONFIG_MEMCG" in multiple places.Quoted 8 lines
Signed-off-by Roman Gushchin <[email protected]>Reviewed-by Shakeel Butt <[email protected]>Reviewed-by Andrew Morton <[email protected]>Acked-by Johannes Weiner <[email protected]>Acked-by Michal Hocko <[email protected]>Signed-off-by Andrew Morton <[email protected]>Signed-off-by Stephen Rothwell <[email protected]>
Quoted 2 lines
I have no idea why my Signed-off-by is attached to this patch (or Andrew's for that matter) ...
Hi Stephen!
I've cherry-picked this patch from the next tree,
so it got your signed-off-by.
Sorry for that!
Thanks!
Roman
Re: [PATCH v2 1/3] mm: introduce mem_cgroup_put() helper
In reply to Roman GushchinQuoted 11 lines · Roman Gushchin
Introduce the
mem_cgroup_put()helper, which helps to eliminate guarding memcg css release with "#ifdefCONFIG_MEMCG" in multiple places.Signed-off-by Roman Gushchin <[email protected]>Reviewed-by Shakeel Butt <[email protected]>Reviewed-by Andrew Morton <[email protected]>Acked-by Johannes Weiner <[email protected]>Acked-by Michal Hocko <[email protected]>Signed-off-by Andrew Morton <[email protected]>Signed-off-by Stephen Rothwell <[email protected]>
PATCH v2 2/3 mm, oom: refactor oom_kill_process()
In reply to Roman Gushchinoom_kill_process() 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 oom_kill_process() 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().
--- mm/oom_kill.c | 123 +++++++++++++++++++++++++++++++--------------------------- 1 file changed, 65 insertions(+), 58 deletions(-)
mm/oom_kill.c +65 -58
@@ -835,68 +835,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);
@@ -970,6 +914,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.14.4
PATCH v2 3/3 mm, oom: introduce memory.oom.group
In reply to Roman GushchinFor 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 an indivisible workload by the OOM killer. If set, all tasks belonging to the cgroup or to its descendants (if the memory cgroup is not a leaf cgroup) 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.
--- Documentation/admin-guide/cgroup-v2.rst | 18 +++++++ include/linux/memcontrol.h | 18 +++++++ mm/memcontrol.c | 93 +++++++++++++++++++++++++++++++++ mm/oom_kill.c | 30 +++++++++++ 4 files changed, 159 insertions(+)
Documentation/admin-guide/cgroup-v2.rst +18 -0
@@ -1069,6 +1069,24 @@ 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 + an indivisible workload by the OOM killer. If set, + all tasks belonging to the cgroup or to its descendants + (if the memory cgroup is not a leaf cgroup) 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
include/linux/memcontrol.h +18 -0
@@ -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,9 @@ 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); +void mem_cgroup_print_oom_group (struct mem_cgroup *memcg);#ifdef CONFIG_MEMCG_SWAP extern int do_swap_account;
@@ -951,6 +959,16 @@ 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 void mem_cgroup_print_oom_group (struct mem_cgroup *memcg) +{ +} +static inline unsigned long memcg_page_state (struct mem_cgroup *memcg,int idx) {
mm/memcontrol.c +93 -0
111 lines not on this page · raw message
mm/oom_kill.c +30 -0
@@ -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,23 @@ 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_print_oom_group (oom_group); +mem_cgroup_scan_tasks (oom_group, oom_kill_memcg_member,NULL ); +mem_cgroup_put (oom_group); + } }/*
Signature · 1 line
2.14.4
Re: [PATCH v2 3/3] mm, oom: introduce memory.oom.group
Quoted 10 lines · Roman Gushchin
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) enablingpanic_on_oom,
2) using a userspace daemon to monitor OOMs and kill all outstanding processes.
Quoted 5 lines
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.
We could start a one-time userspace agent which handles an cgroup OOM event and then terminates...
Quoted 15 lines
mm/memcontrol.c +15
-0+/** +* 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;
mm/memcontrol.c bool mem_cgroup_oom_synchronize(bool handle) · line 1604
Quoted 10 lines
mm/memcontrol.c +10
-0+ +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);
Isn't this racy? I guess that memcg of this "victim" can change to somewhere else from the one as of determining the final candidate. This "victim" might have already passed exit_mm()/cgroup_exit() from do_exit(). This "victim" might be moving to a memcg which is different from the one determining the final candidate.
Quoted 33 lines
mm/memcontrol.c +26
-0+if (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; +}@@ -974,7 +988,23 @@ 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?
mm/memcontrol.c static void oom_kill_process(struct oom_control *oc, const char *message) · line 1003
Quoted 10 lines
mm/memcontrol.c +9
-0+* 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) {
Isn't "killing a child process of the biggest memory hog" and "killing all processes which belongs to a memcg which the child process of the biggest memory hog belongs to" strange? The intent of selecting a child is to try to minimize lost work while the intent of oom_cgroup is to try to discard all work. If oom_cgroup is enabled, I feel that we should
pr_err("%s: Kill all processes in ", message);
pr_cont_cgroup_path(memcg->css.cgroup);
pr_cont(" due to memory.oom.group set\n");without
pr_err("%s: Kill process %d (%s) score %u or sacrifice child\n", message, task_pid_nr(p), p->comm, points);(I mean, don't try to select a child).
Quoted 7 lines
mm/oom_kill.c +4
-0+mem_cgroup_print_oom_group (oom_group); +mem_cgroup_scan_tasks (oom_group, oom_kill_memcg_member,NULL ); +mem_cgroup_put (oom_group); + } }/*
Michal Hocko
maintainerRe: [PATCH v2 3/3] mm, oom: introduce memory.oom.group
Quoted 1 line · Tetsuo Handa
On 2018/08/02 9:32, Roman Gushchin wrote:
[...]
Quoted 10 lines
Quoted 10 lines
mm/memcontrol.c +10
-0+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)
Quoted 8 lines
mm/memcontrol.c bool mem_cgroup_oom_synchronize(bool handle) · line 1604
Quoted 5 lines
mm/memcontrol.c +5
-0+ oom_domain = root_mem_cgroup; + +rcu_read_lock (); + + memcg =mem_cgroup_from_task (victim);Isn't this racy? I guess that memcg of this "victim" can change to somewhere else from the one as of determining the final candidate.
How is this any different from the existing code? We select a victim and then kill it. The victim might move away and won't be part of the oom memcg anymore but we will still kill it. I do not remember this ever being a problem. Migration is a privileged operation. If you loose this restriction you shouldn't allow to move outside of the oom domain.
Quoted 1 line
This "victim" might have already passed exit_mm()/cgroup_exit() from
do_exit().
Why does this matter? The victim hasn't been killed yet so if it exists by its own I do not think we really have to tear the whole cgroup down.
Quoted 57 lines
This "victim" might be moving to a memcg which is different from the one determining the final candidate.
Quoted 33 lines
mm/memcontrol.c +26
-0+if (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; +}@@ -974,7 +988,23 @@ 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? mm/memcontrol.c static void oom_kill_process(struct oom_control *oc, const char *message) · line 1003
Quoted 10 lines
mm/memcontrol.c +9
-0+* 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) {Isn't "killing a child process of the biggest memory hog" and "killing all processes which belongs to a memcg which the child process of the biggest memory hog belongs to" strange? The intent of selecting a child is to try to minimize lost work while the intent of
oom_cgroupis to try to discard all work. Ifoom_cgroupis enabled, I feel that we shouldpr_err("%s: Kill all processes in ", message); pr_cont_cgroup_path(memcg->css.cgroup); pr_cont(" due to memory.oom.group set\n");
Quoted 5 lines
without
pr_err("%s: Kill process %d (%s) score %u or sacrifice child\n", message, task_pid_nr(p), p->comm, points);(I mean, don't try to select a child).
Well, the child can belong into a different memcg. Whether the heuristic to pick up the child is sensible is another question and I do not think it is related to this patchset. The code works as intended, albeit being questionable.
Signature · 2 lines
Michal Hocko SUSE Labs
Re: [PATCH v2 3/3] mm, oom: introduce memory.oom.group
Quoted 22 lines · Michal Hocko
Quoted 1 line · Tetsuo Handa
On 2018/08/02 9:32, Roman Gushchin wrote:
[...]
Quoted 10 lines
Quoted 10 lines
mm/memcontrol.c +10
-0+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)Quoted 8 lines
mm/memcontrol.c bool mem_cgroup_oom_synchronize(bool handle) · line 1604
Quoted 5 lines
mm/memcontrol.c +5
-0+ oom_domain = root_mem_cgroup; + +rcu_read_lock (); + + memcg =mem_cgroup_from_task (victim);Isn't this racy? I guess that memcg of this "victim" can change to somewhere else from the one as of determining the final candidate.
Quoted 5 lines
How is this any different from the existing code? We select a victim and then kill it. The victim might move away and won't be part of the oom memcg anymore but we will still kill it. I do not remember this ever being a problem. Migration is a privileged operation. If you loose this restriction you shouldn't allow to move outside of the oom domain.
The existing code kills one process (plus other processes sharing mm if any). But oom_cgroup kills multiple processes. Thus, whether we made decision based on correct memcg becomes important.
Quoted 5 lines
Quoted 2 lines
This "victim" might have already passed exit_mm()/cgroup_exit() from
do_exit().Why does this matter? The victim hasn't been killed yet so if it exists by its own I do not think we really have to tear the whole cgroup down.
The existing code does not send SIGKILL if find_lock_task_mm() failed. Who can guarantee that the victim is not inside do_exit() yet when this code is executed?
Michal Hocko
maintainerRe: [PATCH v2 3/3] mm, oom: introduce memory.oom.group
Quoted 29 lines · Tetsuo Handa
Quoted 22 lines · Michal Hocko
Quoted 1 line · Tetsuo Handa
On 2018/08/02 9:32, Roman Gushchin wrote:
[...]
Quoted 10 lines
Quoted 10 lines
mm/memcontrol.c +10
-0+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)Quoted 8 lines
mm/memcontrol.c bool mem_cgroup_oom_synchronize(bool handle) · line 1604
Quoted 5 lines
mm/memcontrol.c +5
-0+ oom_domain = root_mem_cgroup; + +rcu_read_lock (); + + memcg =mem_cgroup_from_task (victim);Isn't this racy? I guess that memcg of this "victim" can change to somewhere else from the one as of determining the final candidate.
Quoted 5 lines
How is this any different from the existing code? We select a victim and then kill it. The victim might move away and won't be part of the oom memcg anymore but we will still kill it. I do not remember this ever being a problem. Migration is a privileged operation. If you loose this restriction you shouldn't allow to move outside of the oom domain.
Quoted 3 lines
The existing code kills one process (plus other processes sharing mm if any). But
oom_cgroupkills multiple processes. Thus, whether we made decision based on correct memcg becomes important.
Yes but a proper configuration should already mitigate the harm because you shouldn't be able to migrate the task outside of the oom domain.
A (oom.group = 1)
/ \
B Cmoving task between B and C should be harmless while moving it out of A subtree completely is a dubious configuration.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
This "victim" might have already passed exit_mm()/cgroup_exit() from
do_exit().Why does this matter? The victim hasn't been killed yet so if it exists by its own I do not think we really have to tear the whole cgroup down.
The existing code does not send SIGKILL if
find_lock_task_mm()failed. Who can guarantee that the victim is not insidedo_exit()yet when this code is executed?
I do not follow. Why does this matter at all?
Signature · 2 lines
Michal Hocko SUSE Labs
Re: [PATCH v2 3/3] mm, oom: introduce memory.oom.group
In reply to Tetsuo HandaQuoted 11 lines · Tetsuo Handa
Quoted 10 lines · Roman Gushchin
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) enablingpanic_on_oom,
2) using a userspace daemon to monitor OOMs and kill all outstanding processes.
Quoted 8 lines
Quoted 5 lines
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.We could start a one-time userspace agent which handles an cgroup OOM event and then terminates...
That might be not so trivial if there is a shortage of memory.
Quoted 26 lines
Quoted 26 lines
mm/memcontrol.c +23
-0+/** +* 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 ();
Quoted 8 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ + memcg = mem_cgroup_from_task(victim);Isn't this racy? I guess that memcg of this "victim" can change to somewhere else from the one as of determining the final candidate. This "victim" might have already passed exit_mm()/cgroup_exit() from
do_exit(). This "victim" might be moving to a memcg which is different from the one determining the final candidate.
It is, as well as _all_ OOM handling code.
E.g. what if a user will set oom_score_adj to -1000 in the last moment?
It really doesn't matter, OOM killer should guarantee forward progress without making too stupid decisions. It doesn't provide any strict guarantees and really shouldn't.
Quoted 55 lines
Quoted 34 lines
mm/memcontrol.c +26
-0+if (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; +}@@ -974,7 +988,23 @@ 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? mm/memcontrol.c static void oom_kill_process(struct oom_control *oc, const char *message) · line 1003
Quoted 10 lines
mm/memcontrol.c +9
-0+* 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) {Isn't "killing a child process of the biggest memory hog" and "killing all processes which belongs to a memcg which the child process of the biggest memory hog belongs to" strange? The intent of selecting a child is to try to minimize lost work while the intent of
oom_cgroupis to try to discard all work. Ifoom_cgroupis enabled, I feel that we shouldpr_err("%s: Kill all processes in ", message); pr_cont_cgroup_path(memcg->css.cgroup); pr_cont(" due to memory.oom.group set\n");
Quoted 5 lines
without
pr_err("%s: Kill process %d (%s) score %u or sacrifice child\n", message, task_pid_nr(p), p->comm, points);(I mean, don't try to select a child).
We can do this optimization, but I would be accurate with changing dmesg output format. Although it never was a part of ABI, I wonder, how many users are using something like "kill process [0-9]+ or sacrifice child" regexps?
Thanks!
Re: [PATCH v2 3/3] mm, oom: introduce memory.oom.group
In reply to Roman GushchinQuoted 3 lines · Roman Gushchin
For some workloads an intervention from the OOM killer can be painful. Killing a random task can bring the workload into an inconsistent state.
For patches 1-3,
Thanks.
Signature · 1 line
tejun