Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1420722 > unrolled thread

Re: mm, oom_reaper: How to handle race with oom_killer_disable() ?

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: mm, oom_reaper: How to handle race with oom_killer_disable() ? Michal Hocko <mhocko@kernel.org> - 2016-06-13 13:20 +0200
    Re: mm, oom_reaper: How to handle race with oom_killer_disable() ? Michal Hocko <mhocko@kernel.org> - 2016-06-21 10:40 +0200
      Re: mm, oom_reaper: How to handle race with oom_killer_disable() ? Michal Hocko <mhocko@kernel.org> - 2016-06-21 13:50 +0200
        Re: mm, oom_reaper: How to handle race with oom_killer_disable() ? Michal Hocko <mhocko@kernel.org> - 2016-06-21 15:30 +0200
          Re: mm, oom_reaper: How to handle race with oom_killer_disable() ? Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-06-21 17:40 +0200
            Re: mm, oom_reaper: How to handle race with oom_killer_disable() ? Michal Hocko <mhocko@kernel.org> - 2016-06-21 20:00 +0200
              Re: mm, oom_reaper: How to handle race with oom_killer_disable() ? Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-06-21 23:50 +0200
                Re: mm, oom_reaper: How to handle race with oom_killer_disable() ? Michal Hocko <mhocko@kernel.org> - 2016-06-22 08:50 +0200
                  Re: mm, oom_reaper: How to handle race with oom_killer_disable() ? Michal Hocko <mhocko@kernel.org> - 2016-06-22 09:00 +0200
                    Re: mm, oom_reaper: How to handle race with oom_killer_disable() ? Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-06-22 13:00 +0200
                      Re: mm, oom_reaper: How to handle race with oom_killer_disable() ? Michal Hocko <mhocko@kernel.org> - 2016-06-22 14:10 +0200
                        Re: mm, oom_reaper: How to handle race with oom_killer_disable() ? Michal Hocko <mhocko@kernel.org> - 2016-06-22 14:20 +0200
      Re: mm, oom_reaper: How to handle race with oom_killer_disable() ? Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-06-21 14:00 +0200

#1420722 — Re: mm, oom_reaper: How to handle race with oom_killer_disable() ?

On Fri 10-06-16 23:23:36, Tetsuo Handa wrote:
[...]
>   (1) freeze_processes() starts freezing user space threads.
>   (2) Somebody (maybe a kenrel thread) calls out_of_memory().
>   (3) The OOM killer calls mark_oom_victim() on a user space thread
>       P1 which is already in __refrigerator().
>   (4) oom_killer_disable() sets oom_killer_disabled = true.
>   (5) P1 leaves __refrigerator() and enters do_exit().
>   (6) The OOM reaper calls exit_oom_victim(P1) before P1 can call
>       exit_oom_victim(P1).
>   (7) oom_killer_disable() returns while P1 is not yet frozen
>       again (i.e. not yet marked as PF_FROZEN).
>   (8) P1 perform IO/interfere with the freezer.

You are right. I missed that kernel threads are still alive when writing
e26796066fdf929c ("oom: make oom_reaper freezable").

I am trying to remember why we are disabling oom killer before kernel
threads are frozen but not really sure about that right away. I guess it
has something to do with freeze_kernel_threads being called from
different contexts as well so freeze_processes was just more convinient
and was OK for correctness at the time.

> try_to_freeze_tasks(false) from freeze_kernel_threads() will freeze
> P1 again, but it seems to me that freeze_kernel_threads() is not
> always called when freeze_processes() suceeded.
> 
> Therefore, we need to do like
> 
> -	exit_oom_victim(tsk);
> +	mutex_lock(&oom_lock);
> +	if (!oom_killer_disabled)
> +		exit_oom_victim(tsk);
> +	mutex_unlock(&oom_lock);
> 
> in oom_reap_task(), don't we?

I do not like this very much. I would rather make sure that all
freezable kernel threads are frozen when disabling the oom killer.

[...]

> But we might be able to do like below patch rather than above patch.
> If below approach is OK, "[PATCH 10/10] mm, oom: hide mm which is shared
> with kthread or global init" will be able to call exit_oom_victim() when
> can_oom_reap became false.

I believe this is not really needed. I will follow up on the 10/10
later.
-- 
Michal Hocko
SUSE Labs

[toc] | [next] | [standalone]

#1427461

On Mon 13-06-16 13:19:43, Michal Hocko wrote:
[...]
> I am trying to remember why we are disabling oom killer before kernel
> threads are frozen but not really sure about that right away.

OK, I guess I remember now. Say that a task would depend on a freezable
kernel thread to get to do_exit (stuck in wait_event etc...). We would
simply get stuck in oom_killer_disable for ever. So we need to address
it a different way.

One way would be what you are proposing but I guess it would be more
systematic to never call exit_oom_victim on a remote task.  After [1] we
have a solid foundation to rely only on MMF_REAPED even when TIF_MEMDIE
is set. It is more code than your patch so I can see a reason to go with
yours if the following one seems too large or ugly.

[1] http://lkml.kernel.org/r/1466426628-15074-1-git-send-email-mhocko@kernel.org

What do you think about the following?
---
From f7ffecf944648f16d98fc70275041433ed68f7e4 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Mon, 20 Jun 2016 17:28:18 +0200
Subject: [PATCH] oom, suspend: fix oom_reaper vs. oom_killer_disable race

Tetsuo has reported the following potential oom_killer_disable vs.
oom_reaper race:

(1) freeze_processes() starts freezing user space threads.
(2) Somebody (maybe a kenrel thread) calls out_of_memory().
(3) The OOM killer calls mark_oom_victim() on a user space thread
    P1 which is already in __refrigerator().
(4) oom_killer_disable() sets oom_killer_disabled = true.
(5) P1 leaves __refrigerator() and enters do_exit().
(6) The OOM reaper calls exit_oom_victim(P1) before P1 can call
    exit_oom_victim(P1).
(7) oom_killer_disable() returns while P1 not yet finished
(8) P1 perform IO/interfere with the freezer.

This patch effectivelly reverts 449d777d7ad6 ("mm, oom_reaper: clear
TIF_MEMDIE for all tasks queued for oom_reaper"). We have a more
efficient way to achieve the same. We can rely on MMF_REAPED because
"mm, oom: hide mm which is shared with kthread or global init" made sure
that we skip tasks with MMF_REAPED even when they have TIF_MEMDIE.

The only missing part is that we have to handle __oom_reap_task failure
gracefully. Currently we mark the mm by MMF_OOM_NOT_REAPABLE to later
set MMF_REAPED should we fail __oom_reap_task again. This relies on
clearing TIF_MEMDIE to select that task again. We can achieve a similar
thing by requeuing the task to the oom_reaper_list. Introduce
oom_reaper_add_tail to add the task to the tail of the oom_reaper_list.
While we are at it reuse it for wake_oom_reaper as well because the
queueing should be FIFO.

In the result only the oom victim will be responsible to clear its
TIF_MEMDIE flag so the above race is not possible anymore. oom_reaper
will be allowed to play only with MMF_* flags so the responsibility
model will be more clear.

Fixes: 449d777d7ad6 ("mm, oom_reaper: clear TIF_MEMDIE for all tasks queued for oom_reaper")
Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/oom_kill.c | 86 ++++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 55 insertions(+), 31 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 4c21f744daa6..dbc62a69fee8 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -558,6 +558,51 @@ static bool __oom_reap_task(struct task_struct *tsk)
 	return ret;
 }
 
+static void oom_reaper_add_tail(struct task_struct *tsk)
+{
+	struct task_struct *q;
+
+	spin_lock(&oom_reaper_lock);
+	if (!oom_reaper_list) {
+		oom_reaper_list = tsk;
+		goto unlock;
+	}
+
+	for (q = oom_reaper_list; q->oom_reaper_list; q = q->oom_reaper_list)
+		;
+	q->oom_reaper_list = tsk;
+
+unlock:
+	spin_unlock(&oom_reaper_lock);
+}
+
+static inline bool requeue_oom_victim(struct task_struct *tsk)
+{
+	struct task_struct *p;
+	bool ret;
+
+	/*
+	 * If we've already tried to reap this task in the past and
+	 * failed it probably doesn't make much sense to try yet again
+	 * so hide the mm from the oom killer so that it can move on
+	 * to another task with a different mm struct.
+	 */
+	p = find_lock_task_mm(tsk);
+	if (!p)
+		return false;
+
+	if (test_and_set_bit(MMF_OOM_NOT_REAPABLE, &p->mm->flags))
+		set_bit(MMF_OOM_REAPED, &p->mm->flags);
+	else
+		ret = true;
+
+	task_unlock(p);
+
+	if (ret)
+		oom_reaper_add_tail(tsk);
+	return ret;
+}
+
 #define MAX_OOM_REAP_RETRIES 10
 static void oom_reap_task(struct task_struct *tsk)
 {
@@ -567,40 +612,23 @@ static void oom_reap_task(struct task_struct *tsk)
 	while (attempts++ < MAX_OOM_REAP_RETRIES && !__oom_reap_task(tsk))
 		schedule_timeout_idle(HZ/10);
 
-	if (attempts > MAX_OOM_REAP_RETRIES) {
-		struct task_struct *p;
+	tsk->oom_reaper_list = NULL;
 
+	if (attempts > MAX_OOM_REAP_RETRIES) {
 		pr_info("oom_reaper: unable to reap pid:%d (%s)\n",
 				task_pid_nr(tsk), tsk->comm);
 
-		/*
-		 * If we've already tried to reap this task in the past and
-		 * failed it probably doesn't make much sense to try yet again
-		 * so hide the mm from the oom killer so that it can move on
-		 * to another task with a different mm struct.
-		 */
-		p = find_lock_task_mm(tsk);
-		if (p) {
-			if (test_and_set_bit(MMF_OOM_NOT_REAPABLE, &p->mm->flags)) {
-				pr_info("oom_reaper: giving up pid:%d (%s)\n",
-						task_pid_nr(tsk), tsk->comm);
-				set_bit(MMF_OOM_REAPED, &p->mm->flags);
-			}
-			task_unlock(p);
+		debug_show_all_locks();
+
+		if (requeue_oom_victim(tsk)) {
+			cond_resched();
+			return;
 		}
 
-		debug_show_all_locks();
+		pr_info("oom_reaper: giving up pid:%d (%s)\n",
+				task_pid_nr(tsk), tsk->comm);
 	}
 
-	/*
-	 * Clear TIF_MEMDIE because the task shouldn't be sitting on a
-	 * reasonably reclaimable memory anymore or it is not a good candidate
-	 * for the oom victim right now because it cannot release its memory
-	 * itself nor by the oom reaper.
-	 */
-	tsk->oom_reaper_list = NULL;
-	exit_oom_victim(tsk);
-
 	/* Drop a reference taken by wake_oom_reaper */
 	put_task_struct(tsk);
 }
@@ -637,11 +665,7 @@ void wake_oom_reaper(struct task_struct *tsk)
 		return;
 
 	get_task_struct(tsk);
-
-	spin_lock(&oom_reaper_lock);
-	tsk->oom_reaper_list = oom_reaper_list;
-	oom_reaper_list = tsk;
-	spin_unlock(&oom_reaper_lock);
+	oom_reaper_add_tail(tsk);
 	wake_up(&oom_reaper_wait);
 }
 
-- 
2.8.1

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1427656

On Tue 21-06-16 20:03:17, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Mon 13-06-16 13:19:43, Michal Hocko wrote:
> > [...]
> > > I am trying to remember why we are disabling oom killer before kernel
> > > threads are frozen but not really sure about that right away.
> > 
> > OK, I guess I remember now. Say that a task would depend on a freezable
> > kernel thread to get to do_exit (stuck in wait_event etc...). We would
> > simply get stuck in oom_killer_disable for ever. So we need to address
> > it a different way.
> > 
> > One way would be what you are proposing but I guess it would be more
> > systematic to never call exit_oom_victim on a remote task.  After [1] we
> > have a solid foundation to rely only on MMF_REAPED even when TIF_MEMDIE
> > is set. It is more code than your patch so I can see a reason to go with
> > yours if the following one seems too large or ugly.
> > 
> > [1] http://lkml.kernel.org/r/1466426628-15074-1-git-send-email-mhocko@kernel.org
> > 
> > What do you think about the following?
> 
> I'm OK with not clearing TIF_MEMDIE from a remote task. But this patch is racy.
> 
> > @@ -567,40 +612,23 @@ static void oom_reap_task(struct task_struct *tsk)
> >  	while (attempts++ < MAX_OOM_REAP_RETRIES && !__oom_reap_task(tsk))
> >  		schedule_timeout_idle(HZ/10);
> >  
> > -	if (attempts > MAX_OOM_REAP_RETRIES) {
> > -		struct task_struct *p;
> > +	tsk->oom_reaper_list = NULL;
> >  
> > +	if (attempts > MAX_OOM_REAP_RETRIES) {
> 
> attempts > MAX_OOM_REAP_RETRIES would mean that down_read_trylock()
> continuously failed. But it does not guarantee that the offending task
> shall not call up_write(&mm->mmap_sem) and arrives at mmput() from exit_mm()
> (as well as other threads which are blocked at down_read(&mm->mmap_sem) in
> exit_mm() by the offending task arrive at mmput() from exit_mm()) when the
> OOM reaper was preempted at this point.
> 
> Therefore, find_lock_task_mm() in requeue_oom_victim() could return NULL and
> the OOM reaper could fail to set MMF_OOM_REAPED (and find_lock_task_mm() in
> oom_scan_process_thread() could return NULL and the OOM killer could fail to
> select next OOM victim as well) when __mmput() got stuck.

Fair enough. As this would break no-lockup requirement we cannot go that
way. Let me think about it more.
 
> So, from the point of view of correctness, there remains an unhandled race
> window as long as you depend on find_lock_task_mm() not returning NULL.
> You will again ask "does it really matter/occur", and I can't make progress.

Sigh...
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1427739

On Tue 21-06-16 13:46:43, Michal Hocko wrote:
> On Tue 21-06-16 20:03:17, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > On Mon 13-06-16 13:19:43, Michal Hocko wrote:
> > > [...]
> > > > I am trying to remember why we are disabling oom killer before kernel
> > > > threads are frozen but not really sure about that right away.
> > > 
> > > OK, I guess I remember now. Say that a task would depend on a freezable
> > > kernel thread to get to do_exit (stuck in wait_event etc...). We would
> > > simply get stuck in oom_killer_disable for ever. So we need to address
> > > it a different way.
> > > 
> > > One way would be what you are proposing but I guess it would be more
> > > systematic to never call exit_oom_victim on a remote task.  After [1] we
> > > have a solid foundation to rely only on MMF_REAPED even when TIF_MEMDIE
> > > is set. It is more code than your patch so I can see a reason to go with
> > > yours if the following one seems too large or ugly.
> > > 
> > > [1] http://lkml.kernel.org/r/1466426628-15074-1-git-send-email-mhocko@kernel.org
> > > 
> > > What do you think about the following?
> > 
> > I'm OK with not clearing TIF_MEMDIE from a remote task. But this patch is racy.
> > 
> > > @@ -567,40 +612,23 @@ static void oom_reap_task(struct task_struct *tsk)
> > >  	while (attempts++ < MAX_OOM_REAP_RETRIES && !__oom_reap_task(tsk))
> > >  		schedule_timeout_idle(HZ/10);
> > >  
> > > -	if (attempts > MAX_OOM_REAP_RETRIES) {
> > > -		struct task_struct *p;
> > > +	tsk->oom_reaper_list = NULL;
> > >  
> > > +	if (attempts > MAX_OOM_REAP_RETRIES) {
> > 
> > attempts > MAX_OOM_REAP_RETRIES would mean that down_read_trylock()
> > continuously failed. But it does not guarantee that the offending task
> > shall not call up_write(&mm->mmap_sem) and arrives at mmput() from exit_mm()
> > (as well as other threads which are blocked at down_read(&mm->mmap_sem) in
> > exit_mm() by the offending task arrive at mmput() from exit_mm()) when the
> > OOM reaper was preempted at this point.
> > 
> > Therefore, find_lock_task_mm() in requeue_oom_victim() could return NULL and
> > the OOM reaper could fail to set MMF_OOM_REAPED (and find_lock_task_mm() in
> > oom_scan_process_thread() could return NULL and the OOM killer could fail to
> > select next OOM victim as well) when __mmput() got stuck.
> 
> Fair enough. As this would break no-lockup requirement we cannot go that
> way. Let me think about it more.

Hmm, what about the following instead. It is rather a workaround than a
full flaged fix but it seems much more easier and shouldn't introduce
new issues.
---
From 86bf010d2a6086491bb356494fab0e0fca80dee9 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Mon, 20 Jun 2016 17:28:18 +0200
Subject: [PATCH] oom, suspend: fix oom_reaper vs. oom_killer_disable race

Tetsuo has reported the following potential oom_killer_disable vs.
oom_reaper race:

(1) freeze_processes() starts freezing user space threads.
(2) Somebody (maybe a kenrel thread) calls out_of_memory().
(3) The OOM killer calls mark_oom_victim() on a user space thread
    P1 which is already in __refrigerator().
(4) oom_killer_disable() sets oom_killer_disabled = true.
(5) P1 leaves __refrigerator() and enters do_exit().
(6) The OOM reaper calls exit_oom_victim(P1) before P1 can call
    exit_oom_victim(P1).
(7) oom_killer_disable() returns while P1 not yet finished
(8) P1 perform IO/interfere with the freezer.

This situation is unfortunate. We cannot move oom_killer_disable after
all the freezable kernel threads are frozen because the oom victim might
depend on some of those kthreads to make a forward progress to exit so
we could deadlock. It is also far from trivial to teach the oom_reaper
to not call exit_oom_victim() because then we would lose a guarantee of
the OOM killer and oom_killer_disable forward progress because
exit_mm->mmput might block and never call exit_oom_victim.

It seems the easiest way forward is to workaround this race by calling
try_to_freeze_tasks again after oom_killer_disable. This will make sure
that all the tasks are frozen or it bails out.

Fixes: 449d777d7ad6 ("mm, oom_reaper: clear TIF_MEMDIE for all tasks queued for oom_reaper")
Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 kernel/power/process.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/kernel/power/process.c b/kernel/power/process.c
index df058bed53ce..0c2ee9761d57 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -146,6 +146,18 @@ int freeze_processes(void)
 	if (!error && !oom_killer_disable())
 		error = -EBUSY;
 
+	/*
+	 * There is a hard to fix race between oom_reaper kernel thread
+	 * and oom_killer_disable. oom_reaper calls exit_oom_victim
+	 * before the victim reaches exit_mm so try to freeze all the tasks
+	 * again and catch such a left over task.
+	 */
+	if (!error) {
+		pr_info("Double checking all user space processes after OOM killer disable... ");
+		error = try_to_freeze_tasks(true);
+		pr_cont("\n");
+	}
+
 	if (error)
 		thaw_processes();
 	return error;
-- 
2.8.1


-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1427884

Michal Hocko wrote:
> On Tue 21-06-16 13:46:43, Michal Hocko wrote:
> > On Tue 21-06-16 20:03:17, Tetsuo Handa wrote:
> > > Michal Hocko wrote:
> > > > On Mon 13-06-16 13:19:43, Michal Hocko wrote:
> > > > [...]
> > > > > I am trying to remember why we are disabling oom killer before kernel
> > > > > threads are frozen but not really sure about that right away.
> > > > 
> > > > OK, I guess I remember now. Say that a task would depend on a freezable
> > > > kernel thread to get to do_exit (stuck in wait_event etc...). We would
> > > > simply get stuck in oom_killer_disable for ever. So we need to address
> > > > it a different way.
> > > > 
> > > > One way would be what you are proposing but I guess it would be more
> > > > systematic to never call exit_oom_victim on a remote task.  After [1] we
> > > > have a solid foundation to rely only on MMF_REAPED even when TIF_MEMDIE
> > > > is set. It is more code than your patch so I can see a reason to go with
> > > > yours if the following one seems too large or ugly.
> > > > 
> > > > [1] http://lkml.kernel.org/r/1466426628-15074-1-git-send-email-mhocko@kernel.org
> > > > 
> > > > What do you think about the following?
> > > 
> > > I'm OK with not clearing TIF_MEMDIE from a remote task. But this patch is racy.
> > > 
> > > > @@ -567,40 +612,23 @@ static void oom_reap_task(struct task_struct *tsk)
> > > >  	while (attempts++ < MAX_OOM_REAP_RETRIES && !__oom_reap_task(tsk))
> > > >  		schedule_timeout_idle(HZ/10);
> > > >  
> > > > -	if (attempts > MAX_OOM_REAP_RETRIES) {
> > > > -		struct task_struct *p;
> > > > +	tsk->oom_reaper_list = NULL;
> > > >  
> > > > +	if (attempts > MAX_OOM_REAP_RETRIES) {
> > > 
> > > attempts > MAX_OOM_REAP_RETRIES would mean that down_read_trylock()
> > > continuously failed. But it does not guarantee that the offending task
> > > shall not call up_write(&mm->mmap_sem) and arrives at mmput() from exit_mm()
> > > (as well as other threads which are blocked at down_read(&mm->mmap_sem) in
> > > exit_mm() by the offending task arrive at mmput() from exit_mm()) when the
> > > OOM reaper was preempted at this point.
> > > 
> > > Therefore, find_lock_task_mm() in requeue_oom_victim() could return NULL and
> > > the OOM reaper could fail to set MMF_OOM_REAPED (and find_lock_task_mm() in
> > > oom_scan_process_thread() could return NULL and the OOM killer could fail to
> > > select next OOM victim as well) when __mmput() got stuck.
> > 
> > Fair enough. As this would break no-lockup requirement we cannot go that
> > way. Let me think about it more.
> 
> Hmm, what about the following instead. It is rather a workaround than a
> full flaged fix but it seems much more easier and shouldn't introduce
> new issues.

Yes, I think that will work. But I think below patch (marking signal_struct
to ignore TIF_MEMDIE instead of clearing TIF_MEMDIE from task_struct) on top of
current linux.git will implement no-lockup requirement. No race is possible unlike
"[PATCH 10/10] mm, oom: hide mm which is shared with kthread or global init".

 include/linux/oom.h   |  1 +
 include/linux/sched.h |  2 ++
 mm/memcontrol.c       |  3 ++-
 mm/oom_kill.c         | 60 ++++++++++++++++++++++++++++++---------------------
 4 files changed, 40 insertions(+), 26 deletions(-)

diff --git a/include/linux/oom.h b/include/linux/oom.h
index 8346952..f072c6c 100644
--- a/include/linux/oom.h
+++ b/include/linux/oom.h
@@ -69,6 +69,7 @@ static inline bool oom_task_origin(const struct task_struct *p)
 
 extern void mark_oom_victim(struct task_struct *tsk);
 
+extern bool task_is_reapable(struct task_struct *tsk);
 #ifdef CONFIG_MMU
 extern void try_oom_reaper(struct task_struct *tsk);
 #else
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 6e42ada..9248f90 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -799,6 +799,7 @@ struct signal_struct {
 	 * oom
 	 */
 	bool oom_flag_origin;
+	bool oom_ignore_me;
 	short oom_score_adj;		/* OOM kill score adjustment */
 	short oom_score_adj_min;	/* OOM kill score adjustment min value.
 					 * Only settable by CAP_SYS_RESOURCE. */
@@ -1545,6 +1546,7 @@ struct task_struct {
 	/* unserialized, strictly 'current' */
 	unsigned in_execve:1; /* bit to tell LSMs we're in execve */
 	unsigned in_iowait:1;
+	unsigned oom_shortcut_done:1;
 #ifdef CONFIG_MEMCG
 	unsigned memcg_may_oom:1;
 #ifndef CONFIG_SLOB
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 75e7440..af162f6 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1275,7 +1275,8 @@ static bool 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) || task_will_free_mem(current)) {
+	if (task_is_reapable(current) && !current->oom_shortcut_done) {
+		current->oom_shortcut_done = true;
 		mark_oom_victim(current);
 		try_oom_reaper(current);
 		goto unlock;
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index acbc432..e20d889 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -149,7 +149,7 @@ static bool oom_unkillable_task(struct task_struct *p,
 	if (!has_intersects_mems_allowed(p, nodemask))
 		return true;
 
-	return false;
+	return p->signal->oom_ignore_me;
 }
 
 /**
@@ -555,15 +555,15 @@ static void oom_reap_task(struct task_struct *tsk)
 	}
 
 	/*
-	 * Clear TIF_MEMDIE because the task shouldn't be sitting on a
+	 * Ignore TIF_MEMDIE because the task shouldn't be sitting on a
 	 * reasonably reclaimable memory anymore or it is not a good candidate
 	 * for the oom victim right now because it cannot release its memory
 	 * itself nor by the oom reaper.
 	 */
 	tsk->oom_reaper_list = NULL;
-	exit_oom_victim(tsk);
+	tsk->signal->oom_ignore_me = true;
 
-	/* Drop a reference taken by wake_oom_reaper */
+	/* Drop a reference taken by try_oom_reaper */
 	put_task_struct(tsk);
 }
 
@@ -589,7 +589,7 @@ static int oom_reaper(void *unused)
 	return 0;
 }
 
-static void wake_oom_reaper(struct task_struct *tsk)
+void try_oom_reaper(struct task_struct *tsk)
 {
 	if (!oom_reaper_th)
 		return;
@@ -610,13 +610,13 @@ static void wake_oom_reaper(struct task_struct *tsk)
 /* Check if we can reap the given task. This has to be called with stable
  * tsk->mm
  */
-void try_oom_reaper(struct task_struct *tsk)
+bool task_is_reapable(struct task_struct *tsk)
 {
 	struct mm_struct *mm = tsk->mm;
 	struct task_struct *p;
 
 	if (!mm)
-		return;
+		return false;
 
 	/*
 	 * There might be other threads/processes which are either not
@@ -639,12 +639,11 @@ void try_oom_reaper(struct task_struct *tsk)
 
 			/* Give up */
 			rcu_read_unlock();
-			return;
+			return false;
 		}
 		rcu_read_unlock();
 	}
-
-	wake_oom_reaper(tsk);
+	return true;
 }
 
 static int __init oom_init(void)
@@ -659,8 +658,10 @@ static int __init oom_init(void)
 }
 subsys_initcall(oom_init)
 #else
-static void wake_oom_reaper(struct task_struct *tsk)
+bool task_is_reapable(struct task_struct *tsk)
 {
+	return tsk->mm &&
+		(fatal_signal_pending(tsk) || task_will_free_mem(tsk));
 }
 #endif
 
@@ -753,20 +754,28 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
 	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 set TIF_MEMDIE so it can die quickly
 	 */
 	task_lock(p);
-	if (p->mm && task_will_free_mem(p)) {
+#ifdef CONFIG_MMU
+	if (task_is_reapable(p)) {
 		mark_oom_victim(p);
 		try_oom_reaper(p);
 		task_unlock(p);
 		put_task_struct(p);
 		return;
 	}
+#else
+	if (p->mm && task_will_free_mem(p)) {
+		mark_oom_victim(p);
+		task_unlock(p);
+		put_task_struct(p);
+		return;
+	}
+#endif
 	task_unlock(p);
 
 	if (__ratelimit(&oom_rs))
@@ -846,21 +855,22 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
 		if (same_thread_group(p, victim))
 			continue;
 		if (unlikely(p->flags & PF_KTHREAD) || is_global_init(p) ||
-		    p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN) {
-			/*
-			 * We cannot use oom_reaper for the mm shared by this
-			 * process because it wouldn't get killed and so the
-			 * memory might be still used.
-			 */
-			can_oom_reap = false;
+		    p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
 			continue;
-		}
+
 		do_send_sig_info(SIGKILL, SEND_SIG_FORCED, p, true);
 	}
 	rcu_read_unlock();
 
-	if (can_oom_reap)
-		wake_oom_reaper(victim);
+#ifdef CONFIG_MMU
+	p = find_lock_task_mm(victim);
+	if (p && task_is_reapable(p))
+		try_oom_reaper(victim);
+	else
+		victim->signal->oom_ignore_me = true;
+	if (p)
+		task_unlock(p);
+#endif
 
 	mmdrop(mm);
 	put_task_struct(victim);
@@ -939,8 +949,8 @@ bool out_of_memory(struct oom_control *oc)
 	 * But don't select if current has already released its mm and cleared
 	 * TIF_MEMDIE flag at exit_mm(), otherwise an OOM livelock may occur.
 	 */
-	if (current->mm &&
-	    (fatal_signal_pending(current) || task_will_free_mem(current))) {
+	if (!current->oom_shortcut_done && task_is_reapable(current)) {
+		current->oom_shortcut_done = true;
 		mark_oom_victim(current);
 		try_oom_reaper(current);
 		return true;
-- 
1.8.3.1

[toc] | [prev] | [next] | [standalone]

#1428020

On Wed 22-06-16 00:32:29, Tetsuo Handa wrote:
> Michal Hocko wrote:
[...]
> > Hmm, what about the following instead. It is rather a workaround than a
> > full flaged fix but it seems much more easier and shouldn't introduce
> > new issues.
> 
> Yes, I think that will work. But I think below patch (marking signal_struct
> to ignore TIF_MEMDIE instead of clearing TIF_MEMDIE from task_struct) on top of
> current linux.git will implement no-lockup requirement. No race is possible unlike
> "[PATCH 10/10] mm, oom: hide mm which is shared with kthread or global init".

Not really. Because without the exit_oom_victim from oom_reaper you have
no guarantee that the oom_killer_disable will ever return. I have
mentioned that in the changelog. There is simply no guarantee the oom
victim will ever reach exit_mm->exit_oom_victim.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1428209

Michal Hocko wrote:
> On Wed 22-06-16 00:32:29, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> [...]
> > > Hmm, what about the following instead. It is rather a workaround than a
> > > full flaged fix but it seems much more easier and shouldn't introduce
> > > new issues.
> > 
> > Yes, I think that will work. But I think below patch (marking signal_struct
> > to ignore TIF_MEMDIE instead of clearing TIF_MEMDIE from task_struct) on top of
> > current linux.git will implement no-lockup requirement. No race is possible unlike
> > "[PATCH 10/10] mm, oom: hide mm which is shared with kthread or global init".
> 
> Not really. Because without the exit_oom_victim from oom_reaper you have
> no guarantee that the oom_killer_disable will ever return. I have
> mentioned that in the changelog. There is simply no guarantee the oom
> victim will ever reach exit_mm->exit_oom_victim.

Why? Since any allocation after setting oom_killer_disabled = true will be
forced to fail, nobody will be blocked on waiting for memory allocation. Thus,
the TIF_MEMDIE tasks will eventually reach exit_mm->exit_oom_victim, won't it?

The only possibility that the TIF_MEMDIE tasks won't reach exit_mm->exit_oom_victim
is __GFP_NOFAIL allocations failing to make forward progress even after
ALLOC_NO_WATERMARKS is used. But that is a different problem which I think
we can call panic() when __GFP_NOFAIL allocations failed after setting
oom_killer_disabled = true.

[toc] | [prev] | [next] | [standalone]

#1428465

On Wed 22-06-16 06:47:48, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Wed 22-06-16 00:32:29, Tetsuo Handa wrote:
> > > Michal Hocko wrote:
> > [...]
> > > > Hmm, what about the following instead. It is rather a workaround than a
> > > > full flaged fix but it seems much more easier and shouldn't introduce
> > > > new issues.
> > > 
> > > Yes, I think that will work. But I think below patch (marking signal_struct
> > > to ignore TIF_MEMDIE instead of clearing TIF_MEMDIE from task_struct) on top of
> > > current linux.git will implement no-lockup requirement. No race is possible unlike
> > > "[PATCH 10/10] mm, oom: hide mm which is shared with kthread or global init".
> > 
> > Not really. Because without the exit_oom_victim from oom_reaper you have
> > no guarantee that the oom_killer_disable will ever return. I have
> > mentioned that in the changelog. There is simply no guarantee the oom
> > victim will ever reach exit_mm->exit_oom_victim.
> 
> Why? Since any allocation after setting oom_killer_disabled = true will be
> forced to fail, nobody will be blocked on waiting for memory allocation. Thus,
> the TIF_MEMDIE tasks will eventually reach exit_mm->exit_oom_victim, won't it?

What if it gets blocked waiting for an operation which cannot make any
forward progress because it cannot proceed with an allocation (e.g.
an open coded allocation retry loop - not that uncommon when sending
a bio)? I mean if we want to guarantee a forward progress then there has
to be something to clear the flag no matter in what state the oom victim
is or give up on oom_killer_disable.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1428477

On Wed 22-06-16 08:40:15, Michal Hocko wrote:
> On Wed 22-06-16 06:47:48, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > On Wed 22-06-16 00:32:29, Tetsuo Handa wrote:
> > > > Michal Hocko wrote:
> > > [...]
> > > > > Hmm, what about the following instead. It is rather a workaround than a
> > > > > full flaged fix but it seems much more easier and shouldn't introduce
> > > > > new issues.
> > > > 
> > > > Yes, I think that will work. But I think below patch (marking signal_struct
> > > > to ignore TIF_MEMDIE instead of clearing TIF_MEMDIE from task_struct) on top of
> > > > current linux.git will implement no-lockup requirement. No race is possible unlike
> > > > "[PATCH 10/10] mm, oom: hide mm which is shared with kthread or global init".
> > > 
> > > Not really. Because without the exit_oom_victim from oom_reaper you have
> > > no guarantee that the oom_killer_disable will ever return. I have
> > > mentioned that in the changelog. There is simply no guarantee the oom
> > > victim will ever reach exit_mm->exit_oom_victim.
> > 
> > Why? Since any allocation after setting oom_killer_disabled = true will be
> > forced to fail, nobody will be blocked on waiting for memory allocation. Thus,
> > the TIF_MEMDIE tasks will eventually reach exit_mm->exit_oom_victim, won't it?
> 
> What if it gets blocked waiting for an operation which cannot make any
> forward progress because it cannot proceed with an allocation (e.g.
> an open coded allocation retry loop - not that uncommon when sending
> a bio)? I mean if we want to guarantee a forward progress then there has
> to be something to clear the flag no matter in what state the oom victim
> is or give up on oom_killer_disable.

That being said I guess the patch to try_to_freeze_tasks after
oom_killer_disable should be simple enough to go for now and stable
trees and we can come up with something less hackish later. I do not
like the fact that oom_killer_disable doesn't act as a full "barrier"
anymore.

What do you think?
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1428682

Michal Hocko wrote:
> On Wed 22-06-16 08:40:15, Michal Hocko wrote:
> > On Wed 22-06-16 06:47:48, Tetsuo Handa wrote:
> > > Michal Hocko wrote:
> > > > On Wed 22-06-16 00:32:29, Tetsuo Handa wrote:
> > > > > Michal Hocko wrote:
> > > > [...]
> > > > > > Hmm, what about the following instead. It is rather a workaround than a
> > > > > > full flaged fix but it seems much more easier and shouldn't introduce
> > > > > > new issues.
> > > > > 
> > > > > Yes, I think that will work. But I think below patch (marking signal_struct
> > > > > to ignore TIF_MEMDIE instead of clearing TIF_MEMDIE from task_struct) on top of
> > > > > current linux.git will implement no-lockup requirement. No race is possible unlike
> > > > > "[PATCH 10/10] mm, oom: hide mm which is shared with kthread or global init".
> > > > 
> > > > Not really. Because without the exit_oom_victim from oom_reaper you have
> > > > no guarantee that the oom_killer_disable will ever return. I have
> > > > mentioned that in the changelog. There is simply no guarantee the oom
> > > > victim will ever reach exit_mm->exit_oom_victim.
> > > 
> > > Why? Since any allocation after setting oom_killer_disabled = true will be
> > > forced to fail, nobody will be blocked on waiting for memory allocation. Thus,
> > > the TIF_MEMDIE tasks will eventually reach exit_mm->exit_oom_victim, won't it?
> > 
> > What if it gets blocked waiting for an operation which cannot make any
> > forward progress because it cannot proceed with an allocation (e.g.
> > an open coded allocation retry loop - not that uncommon when sending
> > a bio)? I mean if we want to guarantee a forward progress then there has
> > to be something to clear the flag no matter in what state the oom victim
> > is or give up on oom_killer_disable.

That sounds as if CONFIG_MMU=n kernels do OOM livelock at __mmput() regardless
of oom_killer_disabled.

> 
> That being said I guess the patch to try_to_freeze_tasks after
> oom_killer_disable should be simple enough to go for now and stable
> trees and we can come up with something less hackish later. I do not
> like the fact that oom_killer_disable doesn't act as a full "barrier"
> anymore.
> 
> What do you think?

I'm OK with calling try_to_freeze_tasks(true) again for Linux 4.6 and 4.7 kernels.

But if free memory is little such that oom_killer_disable() can not expect TIF_MEMDIE
threads to clear TIF_MEMDIE by themselves (and therefore has to depend on the OOM
reaper to clear TIF_MEMDIE on behalf of them after the OOM reaper reaped some memory),
subsequent operations would be as well blocked waiting for an operation which cannot
make any forward progress because it cannot proceed with an allocation. Then,
oom_killer_disable() returns false after some timeout (i.e. "do not try to suspend
when the system is almost OOM") will be a safer reaction.

[toc] | [prev] | [next] | [standalone]

#1428728

On Wed 22-06-16 19:57:17, Tetsuo Handa wrote:
> Michal Hocko wrote:
[...]
> > That being said I guess the patch to try_to_freeze_tasks after
> > oom_killer_disable should be simple enough to go for now and stable
> > trees and we can come up with something less hackish later. I do not
> > like the fact that oom_killer_disable doesn't act as a full "barrier"
> > anymore.
> > 
> > What do you think?
> 
> I'm OK with calling try_to_freeze_tasks(true) again for Linux 4.6 and 4.7 kernels.

OK, I will resend the patch CC Rafael and stable.
 
> But if free memory is little such that oom_killer_disable() can not expect TIF_MEMDIE
> threads to clear TIF_MEMDIE by themselves (and therefore has to depend on the OOM
> reaper to clear TIF_MEMDIE on behalf of them after the OOM reaper reaped some memory),
> subsequent operations would be as well blocked waiting for an operation which cannot
> make any forward progress because it cannot proceed with an allocation. Then,
> oom_killer_disable() returns false after some timeout (i.e. "do not try to suspend
> when the system is almost OOM") will be a safer reaction.

Yes that is exactly what I meant by "oom_killer_disable has to give up"
alternative. pm suspend already has a notion of timeout for back off
and oom_killer_disable can use wait_even_timeout. But let's do that
separately.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1428738

On Wed 22-06-16 14:08:43, Michal Hocko wrote:
> On Wed 22-06-16 19:57:17, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> [...]
> > > That being said I guess the patch to try_to_freeze_tasks after
> > > oom_killer_disable should be simple enough to go for now and stable
> > > trees and we can come up with something less hackish later. I do not
> > > like the fact that oom_killer_disable doesn't act as a full "barrier"
> > > anymore.
> > > 
> > > What do you think?
> > 
> > I'm OK with calling try_to_freeze_tasks(true) again for Linux 4.6 and 4.7 kernels.
> 
> OK, I will resend the patch CC Rafael and stable.

Ohh, I've just realized that 449d777d7ad6 ("mm, oom_reaper: clear
TIF_MEMDIE for all tasks queued for oom_reaper") went in in this merge
window. So I've asked to push it to the next 4.7 rc.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1427665

Michal Hocko wrote:
> On Mon 13-06-16 13:19:43, Michal Hocko wrote:
> [...]
> > I am trying to remember why we are disabling oom killer before kernel
> > threads are frozen but not really sure about that right away.
> 
> OK, I guess I remember now. Say that a task would depend on a freezable
> kernel thread to get to do_exit (stuck in wait_event etc...). We would
> simply get stuck in oom_killer_disable for ever. So we need to address
> it a different way.
> 
> One way would be what you are proposing but I guess it would be more
> systematic to never call exit_oom_victim on a remote task.  After [1] we
> have a solid foundation to rely only on MMF_REAPED even when TIF_MEMDIE
> is set. It is more code than your patch so I can see a reason to go with
> yours if the following one seems too large or ugly.
> 
> [1] http://lkml.kernel.org/r/1466426628-15074-1-git-send-email-mhocko@kernel.org
> 
> What do you think about the following?

I'm OK with not clearing TIF_MEMDIE from a remote task. But this patch is racy.

> @@ -567,40 +612,23 @@ static void oom_reap_task(struct task_struct *tsk)
>  	while (attempts++ < MAX_OOM_REAP_RETRIES && !__oom_reap_task(tsk))
>  		schedule_timeout_idle(HZ/10);
>  
> -	if (attempts > MAX_OOM_REAP_RETRIES) {
> -		struct task_struct *p;
> +	tsk->oom_reaper_list = NULL;
>  
> +	if (attempts > MAX_OOM_REAP_RETRIES) {

attempts > MAX_OOM_REAP_RETRIES would mean that down_read_trylock()
continuously failed. But it does not guarantee that the offending task
shall not call up_write(&mm->mmap_sem) and arrives at mmput() from exit_mm()
(as well as other threads which are blocked at down_read(&mm->mmap_sem) in
exit_mm() by the offending task arrive at mmput() from exit_mm()) when the
OOM reaper was preempted at this point.

Therefore, find_lock_task_mm() in requeue_oom_victim() could return NULL and
the OOM reaper could fail to set MMF_OOM_REAPED (and find_lock_task_mm() in
oom_scan_process_thread() could return NULL and the OOM killer could fail to
select next OOM victim as well) when __mmput() got stuck.

So, from the point of view of correctness, there remains an unhandled race
window as long as you depend on find_lock_task_mm() not returning NULL.
You will again ask "does it really matter/occur", and I can't make progress.

>  		pr_info("oom_reaper: unable to reap pid:%d (%s)\n",
>  				task_pid_nr(tsk), tsk->comm);
>  

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web