oom && coredump
Subsystems (3)
cover oom && coredump
Quoted 3 lines · Oleg Nesterov
So I think the patch below makes sense anyway. Although I should probably split it and remove
PT_TRACE_EXITin 2/2.
So let me send the patches.
David, Michal, could you review and ack/nack these changes explicitly?
Let me repeat once again that this patch doesn't pretend to solve all problems, even with the coredumping. And I have to admit that my main motivation is 2/2, this PT_TRACE_EXIT check annoys me ;)
Oleg.
mm/oom_kill.c | 21 +++++++++++---------- 1 files changed, 11 insertions(+), 10 deletions(-)
PATCH 1/2 oom: don't assume that a coredumping thread will exit soon
oom_kill.c assumes that PF_EXITING task should exit and free the memory soon. This is wrong in many ways and one important case is the coredump. A task can sleep in exit_mm() "forever" while the coredumping sub-thread can need more memory.
Change the PF_EXITING checks to take SIGNAL_GROUP_COREDUMP into account, we add the new trivial helper for that.
Note: this is only the first step, this patch doesn't try to solve other problems. For example it doesn't try to clear the wrongly set TIF_MEMDIE (SIGNAL_GROUP_COREDUMP check is obviously racy), fatal_signal_pending() can be false positive, etc.
--- mm/oom_kill.c | 12 +++++++++--- 1 files changed, 9 insertions(+), 3 deletions(-)
mm/oom_kill.c +9 -3
@@ -254,6 +254,12 @@ static enum oom_constraint constrained_alloc(struct zonelist *zonelist, }#endif +static inline bool task_will_free_mem (struct task_struct *task) +{ +return (task->flags &PF_EXITING ) && + !(task->signal->flags &SIGNAL_GROUP_COREDUMP ); +} +enum oom_scan_t oom_scan_process_thread (struct task_struct *task,unsigned long totalpages,const nodemask_t *nodemask,bool force_kill)
@@ -281,7 +287,7 @@ enum oom_scan_t oom_scan_process_thread(struct task_struct *task,if (oom_task_origin (task))return OOM_SCAN_SELECT ; -if (task->flags &PF_EXITING && !force_kill) { +if (task_will_free_mem (task) && !force_kill) {/* * If this task is not being ptraced on exit, then wait for it * to finish before killing some other task unnecessarily.
@@ -443,7 +449,7 @@ void oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,* If the task is already exiting, don't alarm the sysadmin or kill * its children or threads, just set TIF_MEMDIE so it can die quickly */ -if (p->flags &PF_EXITING ) { +if (task_will_free_mem (p)) {set_tsk_thread_flag (p,TIF_MEMDIE );put_task_struct (p);return ;
@@ -649,7 +655,7 @@ void out_of_memory(struct zonelist *zonelist, gfp_t gfp_mask,* select it. The goal is to allow it to allocate so that it may * quickly exit and free its memory. */ -if (fatal_signal_pending (current) || current->flags &PF_EXITING ) { +if (fatal_signal_pending (current) ||task_will_free_mem (current)) {set_thread_flag (TIF_MEMDIE );return ; }
Signature · 1 line
1.5.5.1
Re: [PATCH 1/2] oom: don't assume that a coredumping thread will exit soon
Quoted 11 lines · Oleg Nesterov
oom_kill.cassumes thatPF_EXITINGtask should exit and free the memory soon. This is wrong in many ways and one important case is the coredump. A task can sleep inexit_mm()"forever" while the coredumping sub-thread can need more memory.Change the
PF_EXITINGchecks to takeSIGNAL_GROUP_COREDUMPinto account, we add the new trivial helper for that.Note: this is only the first step, this patch doesn't try to solve other problems. For example it doesn't try to clear the wrongly set
TIF_MEMDIE(SIGNAL_GROUP_COREDUMPcheck is obviously racy),
I am not sure I understand this. What do you mean by wrongly set TIF_MEMDIE? That we give a process access to reserves even though it is already done with the coredumping?
Quoted 1 line
fatal_signal_pending()can be false positive, etc.
When can this happen?
Quoted 1 line
Signed-off-by Oleg Nesterov <[email protected]>
I guess the patch as is makes sense and it is an improvement. We need to call the helper in mem_cgroup_out_of_memory as well, though.
With that feel free to add
Also the original fix for the coredumping (edd45544c6f0 "oom: avoid deferring oom killer if exiting task is being traced") doesn't work really as per http://marc.info/?l=linux-kernel&m=141711049013620 then this and the follow up patch should be marked for stable I guess.
Quoted 51 lines
--- mm/oom_kill.c | 12 +++++++++--- 1 files changed, 9 insertions(+), 3 deletions(-)mm/oom_kill.c +9
-3@@ -254,6 +254,12 @@ static enum oom_constraint constrained_alloc(struct zonelist *zonelist, }#endif +static inline bool task_will_free_mem (struct task_struct *task) +{ +return (task->flags &PF_EXITING ) && + !(task->signal->flags &SIGNAL_GROUP_COREDUMP ); +} +enum oom_scan_t oom_scan_process_thread (struct task_struct *task,unsigned long totalpages,const nodemask_t *nodemask,bool force_kill)@@ -281,7 +287,7 @@ enum oom_scan_t oom_scan_process_thread(struct task_struct *task,if (oom_task_origin (task))return OOM_SCAN_SELECT ; -if (task->flags &PF_EXITING && !force_kill) { +if (task_will_free_mem (task) && !force_kill) {/* * If this task is not being ptraced on exit, then wait for it * to finish before killing some other task unnecessarily. @@ -443,7 +449,7 @@ void oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,* If the task is already exiting, don't alarm the sysadmin or kill * its children or threads, just set TIF_MEMDIE so it can die quickly */ -if (p->flags &PF_EXITING ) { +if (task_will_free_mem (p)) {set_tsk_thread_flag (p,TIF_MEMDIE );put_task_struct (p);return ;@@ -649,7 +655,7 @@ void out_of_memory(struct zonelist *zonelist, gfp_t gfp_mask,* select it. The goal is to allow it to allocate so that it may * quickly exit and free its memory. */ -if (fatal_signal_pending (current) || current->flags &PF_EXITING ) { +if (fatal_signal_pending (current) ||task_will_free_mem (current)) {set_thread_flag (TIF_MEMDIE );return ; }Signature · 1 line
1.5.5.1
Signature · 2 lines
Michal Hocko SUSE Labs
Re: [PATCH 1/2] oom: don't assume that a coredumping thread will exit soon
Quoted 10 lines · Michal Hocko
Quoted 3 lines · Oleg Nesterov
Note: this is only the first step, this patch doesn't try to solve other problems. For example it doesn't try to clear the wrongly set
TIF_MEMDIE(SIGNAL_GROUP_COREDUMPcheck is obviously racy),I am not sure I understand this. What do you mean by wrongly set
TIF_MEMDIE? That we give a process access to reserves even though it is already done with the coredumping?
I meant that (say) oom_kill_process() can set TIF_MEMDIE because PF_EXITING && !SIGNAL_GROUP_COREDUMP, and after that this task can participate the coredumping. For example, this thread can exit on its own, but before it calls exit_mm() another thread can start the coredump.
In this case TIF_MEMDIE can fool oom-killer the same way, oom_scan_process_thread() returns OOM_SCAN_ABORT if TIF_MEMDIE is set.
Quoted 3 lines
Quoted 1 line
fatal_signal_pending()can be false positive, etc.When can this happen?
I meant "if (fatal_signal_pending(current) || task_will_free_mem(current))" in out_of_memory(). Yes, sorry, "false positive" looks confusing. I meant that fatal_signal_pending() can be true because of SIGNAL_GROUP_COREDUMP.
Quoted 4 lines
Quoted 1 line
Signed-off-by Oleg Nesterov <[email protected]>I guess the patch as is makes sense and it is an improvement. We need to call the helper in
mem_cgroup_out_of_memoryas well, though.
Yes, but can't we do this in a separate patch? try_charge() plays with TIF_MEMDIE/PF_EXITING too, but probably this is fine.
Quoted 2 lines
With that feel free to add
Acked-by Michal Hocko <[email protected]>
Thanks.
Quoted 4 lines
Also the original fix for the coredumping (
edd45544c6f0"oom: avoid deferring oom killer if exiting task is being traced") doesn't work really as per http://marc.info/?l=linux-kernel&m=141711049013620 then this and the follow up patch should be marked for stable I guess.
Perhaps this makes sense. It looks simple enough.
Oleg.
Re: [PATCH 1/2] oom: don't assume that a coredumping thread will exit soon
Quoted 20 lines · Oleg Nesterov
Quoted 10 lines · Michal Hocko
Quoted 3 lines · Oleg Nesterov
Note: this is only the first step, this patch doesn't try to solve other problems. For example it doesn't try to clear the wrongly set
TIF_MEMDIE(SIGNAL_GROUP_COREDUMPcheck is obviously racy),I am not sure I understand this. What do you mean by wrongly set
TIF_MEMDIE? That we give a process access to reserves even though it is already done with the coredumping?I meant that (say)
oom_kill_process()can setTIF_MEMDIEbecausePF_EXITING&& !SIGNAL_GROUP_COREDUMP, and after that this task can participate the coredumping. For example, this thread can exit on its own, but before it callsexit_mm()another thread can start the coredump.In this case
TIF_MEMDIEcan fool oom-killer the same way,oom_scan_process_thread()returnsOOM_SCAN_ABORTifTIF_MEMDIEis set.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
fatal_signal_pending()can be false positive, etc.When can this happen?
I meant "if (
fatal_signal_pending(current)||task_will_free_mem(current))" inout_of_memory(). Yes, sorry, "false positive" looks confusing. I meant thatfatal_signal_pending()can be true because ofSIGNAL_GROUP_COREDUMP.
Ahh, OK I guess I see what you meant.
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Oleg Nesterov <[email protected]>I guess the patch as is makes sense and it is an improvement. We need to call the helper in
mem_cgroup_out_of_memoryas well, though.Yes, but can't we do this in a separate patch?
I would prefer if it was in the same patch because we might be facing the same problem in memcg as with the global case. And worse, smaller limit tend to trigger corner cases more often than the global case.
Quoted 2 lines
try_charge()plays with TIF_MEMDIE/PF_EXITING too, but probably this is fine.
try_charge is OK because this is from the time when the allocation has been already done and we just decide to bypass the charge.
Quoted 14 lines
Quoted 2 lines
With that feel free to add
Acked-by Michal Hocko <[email protected]>Thanks.
Quoted 4 lines
Also the original fix for the coredumping (
edd45544c6f0"oom: avoid deferring oom killer if exiting task is being traced") doesn't work really as per http://marc.info/?l=linux-kernel&m=141711049013620 then this and the follow up patch should be marked for stable I guess.Perhaps this makes sense. It looks simple enough.
Oleg.
Signature · 2 lines
Michal Hocko SUSE Labs
Re: [PATCH 1/2] oom: don't assume that a coredumping thread will exit soon
Quoted 4 lines · Michal Hocko
Quoted 2 lines · Oleg Nesterov
Quoted 1 line · Michal Hocko
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
I guess the patch as is makes sense and it is an improvement. We need to call the helper in
mem_cgroup_out_of_memoryas well, though.Yes, but can't we do this in a separate patch?
I would prefer if it was in the same patch because we might be facing the same problem in memcg as with the global case. And worse, smaller limit tend to trigger corner cases more often than the global case.
OK, I'll do V2...
But let me explain why I thought about another patch. I do not want to export task_will_free_mem(). If nothing else, its name matches the current "quickly exit and free its memory" comments but not the reality. An exiting thread won't free the memory (ignoring task_struct/etc) if the process is multithreaded.
I'd rather add another helper for oom_kill.c and memcontrol.c which does
if (fatal_signal_pending(current) || task_will_free_mem(current)) {
set_thread_flag(TIF_MEMDIE);
return true;
}
return false;This way the patch could document that fatal_signal_pending() is not exactly right as we discussed, and then we can improve this helper.
But OK, probably this helper doesn't really make sense, and I can not invent the good name for it ;)
Quoted 5 lines
Quoted 2 lines
try_charge()plays with TIF_MEMDIE/PF_EXITING too, but probably this is fine.
try_chargeis OK because this is from the time when the allocation has been already done and we just decide to bypass the charge.
Yes, thanks, this was my vague understanding but I wasn't sure. However, I am not sure that PF_EXITING check is 100% right (again, this can only mean that a single thread from a thread group exits), but I do not understand this code and I agree this is another story in any case.
Oleg.
Re: [PATCH 1/2] oom: don't assume that a coredumping thread will exit soon
Quoted 14 lines · Oleg Nesterov
Quoted 4 lines · Michal Hocko
Quoted 2 lines · Oleg Nesterov
Quoted 1 line · Michal Hocko
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
I guess the patch as is makes sense and it is an improvement. We need to call the helper in
mem_cgroup_out_of_memoryas well, though.Yes, but can't we do this in a separate patch?
I would prefer if it was in the same patch because we might be facing the same problem in memcg as with the global case. And worse, smaller limit tend to trigger corner cases more often than the global case.
Quoted 5 lines
OK, I'll do V2...
But let me explain why I thought about another patch. I do not want to export
task_will_free_mem(). If nothing else, its name matches the current "quickly exit and free its memory" comments but not the reality.
Yes, the name suggests much more than what it does.
Quoted 12 lines
An exiting thread won't free the memory (ignoring task_struct/etc) if the process is multithreaded.
I'd rather add another helper for
oom_kill.candmemcontrol.cwhich doesif (fatal_signal_pending(current) || task_will_free_mem(current)) { set_thread_flag(TIF_MEMDIE); return true; } return false;
Quoted 5 lines
This way the patch could document that
fatal_signal_pending()is not exactly right as we discussed, and then we can improve this helper.But OK, probably this helper doesn't really make sense, and I can not invent the good name for it ;)
I've failed to come up with a better name as well. The code duplication is not nice either so I guess it would be better to keep the helper localt to mm/oom_kill.c and have it open coded in mm/memcontro.c. Git blame will still tell us all the motivation if they are in the single patch.
Quoted 10 lines
Quoted 5 lines
Quoted 2 lines
try_charge()plays with TIF_MEMDIE/PF_EXITING too, but probably this is fine.
try_chargeis OK because this is from the time when the allocation has been already done and we just decide to bypass the charge.Yes, thanks, this was my vague understanding but I wasn't sure. However, I am not sure that
PF_EXITINGcheck is 100% right (again, this can only mean that a single thread from a thread group exits), but I do not understand this code and I agree this is another story in any case.
See d8dc595ce390 "memcg: do not hang on OOM when killed by userspace OOM access to memory reserves" for more details.
Signature · 2 lines
Michal Hocko SUSE Labs
PATCH 2/2 oom: kill the insufficient and no longer needed PT_TRACE_EXIT check
In reply to Oleg NesterovAfter the previous patch we can remove the PT_TRACE_EXIT check in oom_scan_process_thread(), it was added to handle the case when the coredumping was "frozen" by ptrace, but it doesn't really work. If nothing else, we would need to check all threads which could share the same ->mm to make it more or less correct.
--- mm/oom_kill.c | 11 +++-------- 1 files changed, 3 insertions(+), 8 deletions(-)
mm/oom_kill.c +3 -8
@@ -287,14 +287,9 @@ enum oom_scan_t oom_scan_process_thread(struct task_struct *task,if (oom_task_origin (task))return OOM_SCAN_SELECT ; -if (task_will_free_mem (task) && !force_kill) { -/* -* If this task is not being ptraced on exit, then wait for it -* to finish before killing some other task unnecessarily. -*/ -if (!(task->group_leader->ptrace &PT_TRACE_EXIT )) -return OOM_SCAN_ABORT ; - } +if (task_will_free_mem (task) && !force_kill) +return OOM_SCAN_ABORT ; +return OOM_SCAN_OK ; }
Signature · 1 line
1.5.5.1
Re: [PATCH 2/2] oom: kill the insufficient and no longer needed PT_TRACE_EXIT check
Quoted 7 lines · Oleg Nesterov
After the previous patch we can remove the
PT_TRACE_EXITcheck inoom_scan_process_thread(), it was added to handle the case when the coredumping was "frozen" by ptrace, but it doesn't really work. If nothing else, we would need to check all threads which could share the same ->mm to make it more or less correct.Signed-off-by Oleg Nesterov <[email protected]>
I still do not see why we do not need task->ptrace & PT_TRACE_EXIT check here. I do understand that the check on group_leader doesn't make much sense. ptrace_event would block until the tracer let the task run again which may be never AFAICS.
It is really sad how subtle and racy are all these checks :/
Quoted 29 lines
--- mm/oom_kill.c | 11 +++-------- 1 files changed, 3 insertions(+), 8 deletions(-)mm/oom_kill.c +3
-8@@ -287,14 +287,9 @@ enum oom_scan_t oom_scan_process_thread(struct task_struct *task,if (oom_task_origin (task))return OOM_SCAN_SELECT ; -if (task_will_free_mem (task) && !force_kill) { -/* -* If this task is not being ptraced on exit, then wait for it -* to finish before killing some other task unnecessarily. -*/ -if (!(task->group_leader->ptrace &PT_TRACE_EXIT )) -return OOM_SCAN_ABORT ; - } +if (task_will_free_mem (task) && !force_kill) +return OOM_SCAN_ABORT ; +return OOM_SCAN_OK ; }Signature · 1 line
1.5.5.1
Signature · 2 lines
Michal Hocko SUSE Labs
Re: [PATCH 2/2] oom: kill the insufficient and no longer needed PT_TRACE_EXIT check
Quoted 8 lines · Michal Hocko
Quoted 6 lines · Oleg Nesterov
After the previous patch we can remove the
PT_TRACE_EXITcheck inoom_scan_process_thread(), it was added to handle the case when the coredumping was "frozen" by ptrace, but it doesn't really work. If nothing else, we would need to check all threads which could share the same ->mm to make it more or less correct.
Quoted 6 lines
Quoted 1 line
Signed-off-by Oleg Nesterov <[email protected]>I still do not see why we do not need
task->ptrace&PT_TRACE_EXITcheck here. I do understand that the check ongroup_leaderdoesn't make much sense.ptrace_eventwould block until the tracer let the task run again which may be never AFAICS.
No, note that PT_TRACE_EXIT (the last ptrace event) is reported before PF_EXITING is set.
(just in case... we do have some problems with SIGKILL && ptrace, but
this is completely off-topic and has nothing to do with oom-kill.c)
Oleg.
Michal Hocko
Acked-byRe: [PATCH 2/2] oom: kill the insufficient and no longer needed PT_TRACE_EXIT check
Quoted 11 lines · Oleg Nesterov
Quoted 10 lines · Michal Hocko
Quoted 7 lines · Oleg Nesterov
After the previous patch we can remove the
PT_TRACE_EXITcheck inoom_scan_process_thread(), it was added to handle the case when the coredumping was "frozen" by ptrace, but it doesn't really work. If nothing else, we would need to check all threads which could share the same ->mm to make it more or less correct.Signed-off-by Oleg Nesterov <[email protected]>
Quoted 7 lines
Quoted 4 lines
I still do not see why we do not need
task->ptrace&PT_TRACE_EXITcheck here. I do understand that the check ongroup_leaderdoesn't make much sense.ptrace_eventwould block until the tracer let the task run again which may be never AFAICS.No, note that
PT_TRACE_EXIT(the last ptrace event) is reported beforePF_EXITINGis set.
OK, I managed to confuse myself by the PF_EXITING test before exit_signals in do_exit but now that I am looking closer it seems that this is not interesting from the OOM POV.
Thanks for the clarification. This code is so subtle that I rather ask than miss something.
Quoted 5 lines
(just in case... we do have some problems with SIGKILL && ptrace, but
this is completely off-topic and has nothing to do withoom-kill.c)Oleg.
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 6 lines · Oleg Nesterov
Quoted 3 lines · Oleg Nesterov
So I think the patch below makes sense anyway. Although I should probably split it and remove
PT_TRACE_EXITin 2/2.
Quoted 7 lines
So let me send the patches.
David, Michal, could you review and ack/nack these changes explicitly?
Let me repeat once again that this patch doesn't pretend to solve all problems, even with the coredumping. And I have to admit that my main motivation is 2/2, this
PT_TRACE_EXITcheck annoys me ;)
Another simple test. cat mtsleep.c:
#include <unistd.h>
#include <pthread.h>
void *tfunc(void *arg)
{
pause();
return NULL;
}
int main(void)
{
pthread_t th;
pthread_create(&th, NULL, tfunc, NULL);
pause();
return 0;
}Now,
# echo '|/bin/sleep 1000' >> /proc/sys/kernel/core_pattern # echo 10 >> /proc/sys/kernel/core_pipe_limit
# ./mtsleep & # kill -QUIT %1 # perl -e 'push @_,"x" x 1000_1000 while 1'
Before this series the system hangs and doesn't respond. With these patches it correctly kills perl.
Oleg.
v2 cover oom: don't assume that a coredumping thread will exit soon
In reply to Oleg NesterovAndrew,
This replaces
oom-dont-assume-that-a-coredumping-thread-will-exit-soon.patch
in -mm tree.
The patch is basically the same, but as Michal pointed out mem_cgroup_out_of_memory() can use the new helper too, it can face the same problems.
Also, I tried to update the changelog.
Michal, can you ack this version?
Oleg.
PATCH v2 1/1 oom: don't assume that a coredumping thread will exit soon
oom_kill.c assumes that PF_EXITING task should exit and free the memory soon. This is wrong in many ways and one important case is the coredump. A task can sleep in exit_mm() "forever" while the coredumping sub-thread can need more memory.
Change the PF_EXITING checks to take SIGNAL_GROUP_COREDUMP into account, we add the new trivial helper for that.
Note: this is only the first step, this patch doesn't try to solve other problems. The SIGNAL_GROUP_COREDUMP check is obviously racy, a task can participate in coredump after it was already observed in PF_EXITING state, so TIF_MEMDIE (which also blocks oom-killer) still can be wrongly set. fatal_signal_pending() can be true because of SIGNAL_GROUP_COREDUMP so out_of_memory() and mem_cgroup_out_of_memory() shouldn't blindly trust it. And even the name/usage of the new helper is confusing, an exiting thread can only free its ->mm if it is the only/last task in thread group.
--- include/linux/oom.h | 6 ++++++ mm/memcontrol.c | 2 +- mm/oom_kill.c | 6 +++--- 3 files changed, 10 insertions(+), 4 deletions(-)
include/linux/oom.h +6 -0
@@ -92,6 +92,12 @@ static inline bool oom_gfp_allowed(gfp_t gfp_mask)extern struct task_struct *find_lock_task_mm (struct task_struct *p); +static inline bool task_will_free_mem (struct task_struct *task) +{ +return (task->flags &PF_EXITING ) && + !(task->signal->flags &SIGNAL_GROUP_COREDUMP ); +} +/* sysctls */ extern int sysctl_oom_dump_tasks;extern int sysctl_oom_kill_allocating_task;
mm/memcontrol.c +1 -1
@@ -1734,7 +1734,7 @@ static void mem_cgroup_out_of_memory(struct mem_cgroup *memcg, gfp_t gfp_mask,* select it. The goal is to allow it to allocate so that it may * quickly exit and free its memory. */ -if (fatal_signal_pending (current) || current->flags &PF_EXITING ) { +if (fatal_signal_pending (current) ||task_will_free_mem (current)) {set_thread_flag (TIF_MEMDIE );return ; }
mm/oom_kill.c +3 -3
@@ -281,7 +281,7 @@ enum oom_scan_t oom_scan_process_thread(struct task_struct *task,if (oom_task_origin (task))return OOM_SCAN_SELECT ; -if (task->flags &PF_EXITING && !force_kill) { +if (task_will_free_mem (task) && !force_kill) {/* * If this task is not being ptraced on exit, then wait for it * to finish before killing some other task unnecessarily.
@@ -443,7 +443,7 @@ void oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,* If the task is already exiting, don't alarm the sysadmin or kill * its children or threads, just set TIF_MEMDIE so it can die quickly */ -if (p->flags &PF_EXITING ) { +if (task_will_free_mem (p)) {set_tsk_thread_flag (p,TIF_MEMDIE );put_task_struct (p);return ;
@@ -649,7 +649,7 @@ void out_of_memory(struct zonelist *zonelist, gfp_t gfp_mask,* select it. The goal is to allow it to allocate so that it may * quickly exit and free its memory. */ -if (fatal_signal_pending (current) || current->flags &PF_EXITING ) { +if (fatal_signal_pending (current) ||task_will_free_mem (current)) {set_thread_flag (TIF_MEMDIE );return ; }
Signature · 1 line
1.5.5.1
Andrew Morton
maintainerRe: [PATCH v2 1/1] oom: don't assume that a coredumping thread will exit soon
Quoted 22 lines · On Wed, 3 Dec 2014 20:50:20 +0100 Oleg Nesterov <[email protected]> wrote:
oom_kill.cassumes thatPF_EXITINGtask should exit and free the memory soon. This is wrong in many ways and one important case is the coredump. A task can sleep inexit_mm()"forever" while the coredumping sub-thread can need more memory.Change the
PF_EXITINGchecks to takeSIGNAL_GROUP_COREDUMPinto account, we add the new trivial helper for that.Note: this is only the first step, this patch doesn't try to solve other problems. The
SIGNAL_GROUP_COREDUMPcheck is obviously racy, a task can participate in coredump after it was already observed inPF_EXITINGstate, soTIF_MEMDIE(which also blocks oom-killer) still can be wrongly set.fatal_signal_pending()can be true because ofSIGNAL_GROUP_COREDUMPsoout_of_memory()andmem_cgroup_out_of_memory()shouldn't blindly trust it. And even the name/usage of the new helper is confusing, an exiting thread can only free its ->mm if it is the only/last task in thread group....
include/linux/oom.h static inline bool oom_gfp_allowed(gfp_t gfp_mask) · line 99
Quoted 8 lines
include/linux/oom.h +5
-0@@ -92,6 +92,12 @@ static inline bool oom_gfp_allowed(gfp_t gfp_mask)extern struct task_struct *find_lock_task_mm (struct task_struct *p); +static inline bool task_will_free_mem (struct task_struct *task) +{ +return (task->flags &PF_EXITING ) && + !(task->signal->flags &SIGNAL_GROUP_COREDUMP ); +}
The SIGNAL_GROUP_COREDUMP test is utterly obscure. Let's help our poor readers?
include/linux/oom.h renamed from include/linux/oom.h~oom-dont-assume-that-a-coredumping-thread-will-exit-soon-v2-fix +5 -0
@@ -89,6 +89,11 @@ extern struct task_struct *find_lock_tasstatic inline bool task_will_free_mem (struct task_struct *task) { +/* +* A coredumping process may sleep for extended periods in exit_mm(), so +* the oom killer cannot assume that the process will promptly exit and +* release memory. +*/ return (task->flags &PF_EXITING ) && !(task->signal->flags &SIGNAL_GROUP_COREDUMP ); }
Re: [PATCH v2 1/1] oom: don't assume that a coredumping thread will exit soon
In reply to Oleg NesterovQuoted 17 lines · Oleg Nesterov
oom_kill.cassumes thatPF_EXITINGtask should exit and free the memory soon. This is wrong in many ways and one important case is the coredump. A task can sleep inexit_mm()"forever" while the coredumping sub-thread can need more memory.Change the
PF_EXITINGchecks to takeSIGNAL_GROUP_COREDUMPinto account, we add the new trivial helper for that.Note: this is only the first step, this patch doesn't try to solve other problems. The
SIGNAL_GROUP_COREDUMPcheck is obviously racy, a task can participate in coredump after it was already observed inPF_EXITINGstate, soTIF_MEMDIE(which also blocks oom-killer) still can be wrongly set.fatal_signal_pending()can be true because ofSIGNAL_GROUP_COREDUMPsoout_of_memory()andmem_cgroup_out_of_memory()shouldn't blindly trust it. And even the name/usage of the new helper is confusing, an exiting thread can only free its ->mm if it is the only/last task in thread group.
Quoted 1 line
Signed-off-by Oleg Nesterov <[email protected]>
OK, I do not have a better name for the helper and code duplication looks even worse so
Andrew's additional comment makes sense as well.
Quoted 71 lines
--- include/linux/oom.h | 6 ++++++ mm/memcontrol.c | 2 +- mm/oom_kill.c | 6 +++--- 3 files changed, 10 insertions(+), 4 deletions(-)include/linux/oom.h +6
-0@@ -92,6 +92,12 @@ static inline bool oom_gfp_allowed(gfp_t gfp_mask)extern struct task_struct *find_lock_task_mm (struct task_struct *p); +static inline bool task_will_free_mem (struct task_struct *task) +{ +return (task->flags &PF_EXITING ) && + !(task->signal->flags &SIGNAL_GROUP_COREDUMP ); +} +/* sysctls */ extern int sysctl_oom_dump_tasks;extern int sysctl_oom_kill_allocating_task;mm/memcontrol.c +1
-1@@ -1734,7 +1734,7 @@ static void mem_cgroup_out_of_memory(struct mem_cgroup *memcg, gfp_t gfp_mask,* select it. The goal is to allow it to allocate so that it may * quickly exit and free its memory. */ -if (fatal_signal_pending (current) || current->flags &PF_EXITING ) { +if (fatal_signal_pending (current) ||task_will_free_mem (current)) {set_thread_flag (TIF_MEMDIE );return ; }mm/oom_kill.c +3
-3@@ -281,7 +281,7 @@ enum oom_scan_t oom_scan_process_thread(struct task_struct *task,if (oom_task_origin (task))return OOM_SCAN_SELECT ; -if (task->flags &PF_EXITING && !force_kill) { +if (task_will_free_mem (task) && !force_kill) {/* * If this task is not being ptraced on exit, then wait for it * to finish before killing some other task unnecessarily. @@ -443,7 +443,7 @@ void oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,* If the task is already exiting, don't alarm the sysadmin or kill * its children or threads, just set TIF_MEMDIE so it can die quickly */ -if (p->flags &PF_EXITING ) { +if (task_will_free_mem (p)) {set_tsk_thread_flag (p,TIF_MEMDIE );put_task_struct (p);return ;@@ -649,7 +649,7 @@ void out_of_memory(struct zonelist *zonelist, gfp_t gfp_mask,* select it. The goal is to allow it to allocate so that it may * quickly exit and free its memory. */ -if (fatal_signal_pending (current) || current->flags &PF_EXITING ) { +if (fatal_signal_pending (current) ||task_will_free_mem (current)) {set_thread_flag (TIF_MEMDIE );return ; }Signature · 1 line
1.5.5.1
Signature · 2 lines
Michal Hocko SUSE Labs
Re: [PATCH v2 1/1] oom: don't assume that a coredumping thread will exit soon
In reply to Oleg NesterovQuoted 17 lines · Oleg Nesterov
oom_kill.cassumes thatPF_EXITINGtask should exit and free the memory soon. This is wrong in many ways and one important case is the coredump. A task can sleep inexit_mm()"forever" while the coredumping sub-thread can need more memory.Change the
PF_EXITINGchecks to takeSIGNAL_GROUP_COREDUMPinto account, we add the new trivial helper for that.Note: this is only the first step, this patch doesn't try to solve other problems. The
SIGNAL_GROUP_COREDUMPcheck is obviously racy, a task can participate in coredump after it was already observed inPF_EXITINGstate, soTIF_MEMDIE(which also blocks oom-killer) still can be wrongly set.fatal_signal_pending()can be true because ofSIGNAL_GROUP_COREDUMPsoout_of_memory()andmem_cgroup_out_of_memory()shouldn't blindly trust it. And even the name/usage of the new helper is confusing, an exiting thread can only free its ->mm if it is the only/last task in thread group.
Quoted 1 line
Signed-off-by Oleg Nesterov <[email protected]>