oom killer: break from infinite loop
in [Kernel]
|
From: KOSAKI Motohiro on 24 Mar 2010 23:00 > In multi-threading environment, if the current task(A) have got > the mm->mmap_sem semaphore, and the thread(B) in the same process > is selected to be oom killed, because they shares the same semaphore, > thread B can not really be killed. So __alloc_pages_slowpath turns > to be a infinite loop. Here set all the threads in the group to > TIF_MEMDIE, it gets a chance to break and exit. > > Signed-off-by: Anfei Zhou <anfei.zhou(a)gmail.com> I like this patch very much. Thanks, Anfei! Reviewed-by: KOSAKI Motohiro <kosaki.motohiro(a)jp.fujitsu.com> > --- > mm/oom_kill.c | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > index 9b223af..aab9892 100644 > --- a/mm/oom_kill.c > +++ b/mm/oom_kill.c > @@ -381,6 +381,8 @@ static void dump_header(struct task_struct *p, gfp_t gfp_mask, int order, > */ > static void __oom_kill_task(struct task_struct *p, int verbose) > { > + struct task_struct *t; > + > if (is_global_init(p)) { > WARN_ON(1); > printk(KERN_WARNING "tried to kill init!\n"); > @@ -412,6 +414,8 @@ static void __oom_kill_task(struct task_struct *p, int verbose) > */ > p->rt.time_slice = HZ; > set_tsk_thread_flag(p, TIF_MEMDIE); > + for (t = next_thread(p); t != p; t = next_thread(t)) > + set_tsk_thread_flag(t, TIF_MEMDIE); > > force_sig(SIGKILL, p); > } > -- > 1.6.4.rc1 > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo(a)kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: <a href=mailto:"dont(a)kvack.org"> email(a)kvack.org </a> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Andrew Morton on 26 Mar 2010 18:10 On Thu, 25 Mar 2010 00:25:05 +0800 Anfei Zhou <anfei.zhou(a)gmail.com> wrote: > In multi-threading environment, if the current task(A) have got > the mm->mmap_sem semaphore, and the thread(B) in the same process > is selected to be oom killed, because they shares the same semaphore, > thread B can not really be killed. So __alloc_pages_slowpath turns > to be a infinite loop. Here set all the threads in the group to > TIF_MEMDIE, it gets a chance to break and exit. > > Signed-off-by: Anfei Zhou <anfei.zhou(a)gmail.com> > --- > mm/oom_kill.c | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > index 9b223af..aab9892 100644 > --- a/mm/oom_kill.c > +++ b/mm/oom_kill.c > @@ -381,6 +381,8 @@ static void dump_header(struct task_struct *p, gfp_t gfp_mask, int order, > */ > static void __oom_kill_task(struct task_struct *p, int verbose) > { > + struct task_struct *t; > + > if (is_global_init(p)) { > WARN_ON(1); > printk(KERN_WARNING "tried to kill init!\n"); > @@ -412,6 +414,8 @@ static void __oom_kill_task(struct task_struct *p, int verbose) > */ > p->rt.time_slice = HZ; > set_tsk_thread_flag(p, TIF_MEMDIE); > + for (t = next_thread(p); t != p; t = next_thread(t)) > + set_tsk_thread_flag(t, TIF_MEMDIE); > > force_sig(SIGKILL, p); Don't we need some sort of locking while walking that ring? Unintuitively it appears to be spin_lock_irq(&p->sighand->siglock). -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Oleg Nesterov on 26 Mar 2010 18:40 On 03/26, Andrew Morton wrote: > > On Thu, 25 Mar 2010 00:25:05 +0800 > Anfei Zhou <anfei.zhou(a)gmail.com> wrote: > > > --- a/mm/oom_kill.c > > +++ b/mm/oom_kill.c > > @@ -381,6 +381,8 @@ static void dump_header(struct task_struct *p, gfp_t gfp_mask, int order, > > */ > > static void __oom_kill_task(struct task_struct *p, int verbose) > > { > > + struct task_struct *t; > > + > > if (is_global_init(p)) { > > WARN_ON(1); > > printk(KERN_WARNING "tried to kill init!\n"); > > @@ -412,6 +414,8 @@ static void __oom_kill_task(struct task_struct *p, int verbose) > > */ > > p->rt.time_slice = HZ; > > set_tsk_thread_flag(p, TIF_MEMDIE); > > + for (t = next_thread(p); t != p; t = next_thread(t)) > > + set_tsk_thread_flag(t, TIF_MEMDIE); > > > > force_sig(SIGKILL, p); > > Don't we need some sort of locking while walking that ring? This should be always called under tasklist_lock, I think. At least this seems to be true in Linus's tree. I'd suggest to do - set_tsk_thread_flag(p, TIF_MEMDIE); + t = p; + do { + set_tsk_thread_flag(t, TIF_MEMDIE); + } while_each_thread(p, t); but this is matter of taste. Off-topic, but we shouldn't use force_sig(), SIGKILL doesn't need "force" semantics. I'd wish I could understand the changelog ;) Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: David Rientjes on 27 Mar 2010 22:50 On Thu, 25 Mar 2010, Anfei Zhou wrote: > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > index 9b223af..aab9892 100644 > --- a/mm/oom_kill.c > +++ b/mm/oom_kill.c > @@ -381,6 +381,8 @@ static void dump_header(struct task_struct *p, gfp_t gfp_mask, int order, > */ > static void __oom_kill_task(struct task_struct *p, int verbose) > { > + struct task_struct *t; > + > if (is_global_init(p)) { > WARN_ON(1); > printk(KERN_WARNING "tried to kill init!\n"); > @@ -412,6 +414,8 @@ static void __oom_kill_task(struct task_struct *p, int verbose) > */ > p->rt.time_slice = HZ; > set_tsk_thread_flag(p, TIF_MEMDIE); > + for (t = next_thread(p); t != p; t = next_thread(t)) > + set_tsk_thread_flag(t, TIF_MEMDIE); > > force_sig(SIGKILL, p); > } I like the concept, but I agree that it would probably be better to write it as Oleg suggested. The oom killer has been rewritten in the -mm tree and so this patch doesn't apply cleanly, would it be possible to rebase to mmotm with the suggested coding sytle and post this again? See http://userweb.kernel.org/~akpm/mmotm/mmotm-readme.txt Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: anfei on 28 Mar 2010 11:00 On Fri, Mar 26, 2010 at 11:33:56PM +0100, Oleg Nesterov wrote: > On 03/26, Andrew Morton wrote: > > > > On Thu, 25 Mar 2010 00:25:05 +0800 > > Anfei Zhou <anfei.zhou(a)gmail.com> wrote: > > > > > --- a/mm/oom_kill.c > > > +++ b/mm/oom_kill.c > > > @@ -381,6 +381,8 @@ static void dump_header(struct task_struct *p, gfp_t gfp_mask, int order, > > > */ > > > static void __oom_kill_task(struct task_struct *p, int verbose) > > > { > > > + struct task_struct *t; > > > + > > > if (is_global_init(p)) { > > > WARN_ON(1); > > > printk(KERN_WARNING "tried to kill init!\n"); > > > @@ -412,6 +414,8 @@ static void __oom_kill_task(struct task_struct *p, int verbose) > > > */ > > > p->rt.time_slice = HZ; > > > set_tsk_thread_flag(p, TIF_MEMDIE); > > > + for (t = next_thread(p); t != p; t = next_thread(t)) > > > + set_tsk_thread_flag(t, TIF_MEMDIE); > > > > > > force_sig(SIGKILL, p); > > > > Don't we need some sort of locking while walking that ring? > > This should be always called under tasklist_lock, I think. > At least this seems to be true in Linus's tree. > Yes, this function is always called with read_lock(&tasklist_lock), so it should be okay. > I'd suggest to do > > - set_tsk_thread_flag(p, TIF_MEMDIE); > + t = p; > + do { > + set_tsk_thread_flag(t, TIF_MEMDIE); > + } while_each_thread(p, t); > > but this is matter of taste. > Yes, this is better. > Off-topic, but we shouldn't use force_sig(), SIGKILL doesn't > need "force" semantics. > This may need a dedicated patch, there are some other places to force_sig(SIGKILL, ...) too. > I'd wish I could understand the changelog ;) > Assume thread A and B are in the same group. If A runs into the oom, and selects B as the victim, B won't exit because at least in exit_mm(), it can not get the mm->mmap_sem semaphore which A has already got. So no memory is freed, and no other task will be selected to kill. I formatted the patch for -mm tree as David suggested. --- mm/oom_kill.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) --- a/mm/oom_kill.c +++ b/mm/oom_kill.c @@ -418,8 +418,15 @@ static void dump_header(struct task_stru */ static void __oom_kill_task(struct task_struct *p) { + struct task_struct *t; + p->rt.time_slice = HZ; - set_tsk_thread_flag(p, TIF_MEMDIE); + + t = p; + do { + set_tsk_thread_flag(t, TIF_MEMDIE); + } while_each_thread(p, t); + force_sig(SIGKILL, p); } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|
Next
|
Last
Pages: 1 2 3 4 5 6 Prev: [GIT PULL] ext4 fixes Next: [PATCH v2] PL330: Add PL330 DMA controller driver |