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

Groups > linux.kernel > #1372554 > unrolled thread

[PATCH 0/3] oom reaper follow ups v1

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] oom reaper follow ups v1 Michal Hocko <mhocko@kernel.org> - 2016-04-06 16:20 +0200
    [PATCH 3/3] mm, oom_reaper: clear TIF_MEMDIE for all tasks queued for oom_reaper Michal Hocko <mhocko@kernel.org> - 2016-04-06 16:20 +0200
      Re: [PATCH 3/3] mm, oom_reaper: clear TIF_MEMDIE for all tasks queued for oom_reaper Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-04-07 14:00 +0200
        Re: [PATCH 3/3] mm, oom_reaper: clear TIF_MEMDIE for all tasks  queued for oom_reaper Michal Hocko <mhocko@kernel.org> - 2016-04-08 13:40 +0200
      Re: [PATCH 3/3] mm, oom_reaper: clear TIF_MEMDIE for all tasks  queued for oom_reaper Michal Hocko <mhocko@kernel.org> - 2016-04-08 15:10 +0200
    [PATCH 1/3] mm, oom: move GFP_NOFS check to out_of_memory Michal Hocko <mhocko@kernel.org> - 2016-04-06 16:20 +0200

#1372554 — [PATCH 0/3] oom reaper follow ups v1

Hi,
the following three patches should help to reduce the corner case space
for oom livelocks even further.

Patch1 is something that we should have probably done quite some time
ago. GFP_NOFS requests never got access to memory reserves even when a
task was killed. As this has some side effect to oom notifiers I have
CCed curret users of this interface to hear from them. The patch contains
more detailed information.

Patch2 builds on top and allows tasks which skip the regular OOM killer
(e.g. those with fatal_signal_pending) to queue them for oom reaper if
there is not a risk that somebody sharing the mm with them could see
this from the userspace. I have cced Oleg on this patch because I am not
entirely sure I am doing it properly.

Finally the last patch relaxes TIF_MEMDIE clearing and makes sure that
no task queued for the oom reaper will keep it once it is processed
(either successfully or not).

Any feedback is highly appreciated.

[toc] | [next] | [standalone]

#1372558 — [PATCH 3/3] mm, oom_reaper: clear TIF_MEMDIE for all tasks queued for oom_reaper

From: Michal Hocko <mhocko@suse.com>

Right now the oom reaper will clear TIF_MEMDIE only for tasks which
were successfully reaped. This is the safest option because we know
that such an oom victim would only block forward progress of the oom
killer without a good reason because it is highly unlikely it would
release much more memory. Basically most of its memory has been already
torn down.

We can relax this assumption to catch more corner cases though.

The first obvious one is when the oom victim clears its mm and gets
stuck later on. oom_reaper would back of on find_lock_task_mm returning
NULL. We can safely try to clear TIF_MEMDIE in this case because such a
task would be ignored by the oom killer anyway. The flag would be
cleared by that time already most of the time anyway.

The less obvious one is when the oom reaper fails due to mmap_sem
contention. Even if we clear TIF_MEMDIE for this task then it is not
very likely that we would select another task too easily because
we haven't reaped the last victim and so it would be still the #1
candidate. There is a rare race condition possible when the current
victim terminates before the next select_bad_process but considering
that oom_reap_task had retried several times before giving up then
this sounds like a borderline thing.

After this patch we should have a guarantee that the OOM killer will
not be block for unbounded amount of time for most cases.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/oom_kill.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 74c38f5fffef..7098104b7475 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -510,14 +510,10 @@ static bool __oom_reap_task(struct task_struct *tsk)
 	up_read(&mm->mmap_sem);
 
 	/*
-	 * Clear TIF_MEMDIE because the task shouldn't be sitting on a
-	 * reasonably reclaimable memory anymore. OOM killer can continue
-	 * by selecting other victim if unmapping hasn't led to any
-	 * improvements. This also means that selecting this task doesn't
-	 * make any sense.
+	 * This task can be safely ignored because we cannot do much more
+	 * to release its memory.
 	 */
 	tsk->signal->oom_score_adj = OOM_SCORE_ADJ_MIN;
-	exit_oom_victim(tsk);
 out:
 	mmput(mm);
 	return ret;
@@ -538,6 +534,14 @@ static void oom_reap_task(struct task_struct *tsk)
 		debug_show_all_locks();
 	}
 
+	/*
+	 * 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.
+	 */
+	exit_oom_victim(tsk);
+
 	/* Drop a reference taken by wake_oom_reaper */
 	put_task_struct(tsk);
 }
-- 
2.8.0.rc3

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

#1373311 — Re: [PATCH 3/3] mm, oom_reaper: clear TIF_MEMDIE for all tasks queued for oom_reaper

Michal Hocko wrote:
> The first obvious one is when the oom victim clears its mm and gets
> stuck later on. oom_reaper would back of on find_lock_task_mm returning
> NULL. We can safely try to clear TIF_MEMDIE in this case because such a
> task would be ignored by the oom killer anyway. The flag would be
> cleared by that time already most of the time anyway.

I didn't understand what this wants to tell. The OOM victim will clear
TIF_MEMDIE as soon as it sets current->mm = NULL. Even if the oom victim
clears its mm and gets stuck later on (e.g. at exit_task_work()),
TIF_MEMDIE was already cleared by that moment by the OOM victim.

> 
> The less obvious one is when the oom reaper fails due to mmap_sem
> contention. Even if we clear TIF_MEMDIE for this task then it is not
> very likely that we would select another task too easily because
> we haven't reaped the last victim and so it would be still the #1
> candidate. There is a rare race condition possible when the current
> victim terminates before the next select_bad_process but considering
> that oom_reap_task had retried several times before giving up then
> this sounds like a borderline thing.

Is it helpful? Allowing the OOM killer to select the same thread again
simply makes the kernel log buffer flooded with the OOM kill messages.

I think we should not allow the OOM killer to select the same thread again
by e.g. doing tsk->signal->oom_score_adj = OOM_SCORE_ADJ_MIN regardless of
whether reaping that thread's memory succeeded or not.

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

#1374171 — Re: [PATCH 3/3] mm, oom_reaper: clear TIF_MEMDIE for all tasks queued for oom_reaper

On Thu 07-04-16 20:55:34, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > The first obvious one is when the oom victim clears its mm and gets
> > stuck later on. oom_reaper would back of on find_lock_task_mm returning
> > NULL. We can safely try to clear TIF_MEMDIE in this case because such a
> > task would be ignored by the oom killer anyway. The flag would be
> > cleared by that time already most of the time anyway.
> 
> I didn't understand what this wants to tell. The OOM victim will clear
> TIF_MEMDIE as soon as it sets current->mm = NULL.

No it clears the flag _after_ it returns from mmput. There is no
guarantee it won't get stuck somewhere on the way there - e.g. exit_aio
waits for completion and who knows what else might get stuck.

> Even if the oom victim
> clears its mm and gets stuck later on (e.g. at exit_task_work()),
> TIF_MEMDIE was already cleared by that moment by the OOM victim.
> 
> > 
> > The less obvious one is when the oom reaper fails due to mmap_sem
> > contention. Even if we clear TIF_MEMDIE for this task then it is not
> > very likely that we would select another task too easily because
> > we haven't reaped the last victim and so it would be still the #1
> > candidate. There is a rare race condition possible when the current
> > victim terminates before the next select_bad_process but considering
> > that oom_reap_task had retried several times before giving up then
> > this sounds like a borderline thing.
> 
> Is it helpful? Allowing the OOM killer to select the same thread again
> simply makes the kernel log buffer flooded with the OOM kill messages.

I am trying to be as conservative as possible here. The likelyhood of
mmap sem contention will be reduced considerably after my
down_write_killable series will get merged. If this turns out to be a
problem (trivial to spot as the same task will be killed again) then we
can think about a fix for that (e.g. ignore the task if the has been
selected more than N times).

> I think we should not allow the OOM killer to select the same thread again
> by e.g. doing tsk->signal->oom_score_adj = OOM_SCORE_ADJ_MIN regardless of
> whether reaping that thread's memory succeeded or not.

I think this comes with some risk and so it should go as a separate
patch with a full justification why the outcome is better. Especially
after the mmap_sem contention will be reduced by other means.
-- 
Michal Hocko
SUSE Labs

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

#1374226 — Re: [PATCH 3/3] mm, oom_reaper: clear TIF_MEMDIE for all tasks queued for oom_reaper

Andrew, could you please fold this in?
---
From 4e23df3584966f6d7885245f3071c15669b8a5fc Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Fri, 8 Apr 2016 15:04:29 +0200
Subject: [PATCH] mm, oom_reaper: clear oom_reaper_list before clearing
 TIF_MEMDIE

As per Tetsuo:
: oom_reaper() will need to do "tsk->oom_reaper_list = NULL;" due to
:
:         if (tsk == oom_reaper_list || tsk->oom_reaper_list)
:                 return;
:
: test in wake_oom_reaper() if "[PATCH 3/3] mm, oom_reaper: clear
: TIF_MEMDIE for all tasks queued for oom_reaper" will select the same
: thread again

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/oom_kill.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 7098104b7475..ca34036f3ae1 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -540,6 +540,7 @@ static void oom_reap_task(struct task_struct *tsk)
 	 * 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 */
-- 
2.8.0.rc3

-- 
Michal Hocko
SUSE Labs

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

#1372561 — [PATCH 1/3] mm, oom: move GFP_NOFS check to out_of_memory

From: Michal Hocko <mhocko@suse.com>

__alloc_pages_may_oom is the central place to decide when the
out_of_memory should be invoked. This is a good approach for most checks
there because they are page allocator specific and the allocation fails
right after for all of them.

The notable exception is GFP_NOFS context which is faking
did_some_progress and keep the page allocator looping even though there
couldn't have been any progress from the OOM killer. This patch doesn't
change this behavior because we are not ready to allow those allocation
requests to fail yet (and maybe we will face the reality that we will
never manage to safely fail these request). Instead __GFP_FS check
is moved down to out_of_memory and prevent from OOM victim selection
there. There are two reasons for that
	- OOM notifiers might release some memory even from this context
	  as none of the registered notifier seems to be FS related
	- this might help a dying thread to get an access to memory
          reserves and move on which will make the behavior more
          consistent with the case when the task gets killed from a
          different context.

Keep a comment in __alloc_pages_may_oom to make sure we do not forget
how GFP_NOFS is special and that we really want to do something about
it.

Note to the current oom_notifier users:
The observable difference for you is that oom notifiers cannot depend on
any fs locks because we could deadlock. Not that this would be allowed
today because that would just lockup machine in most of the cases and
ruling out the OOM killer along the way. Another difference is that
callbacks might be invoked sooner now because GFP_NOFS is a weaker
reclaim context and so there could be reclaimable memory which is just
not reachable now. That would require GFP_NOFS only loads which are
really rare and more importantly the observable result would be dropping
of reconstructible object and potential performance drop which is not
such a big deal when we are struggling to fulfill other important
allocation requests.

Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Raushaniya Maksudova <rmaksudova@parallels.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/oom_kill.c   |  9 +++++++++
 mm/page_alloc.c | 24 ++++++++++--------------
 2 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 86349586eacb..32d8210b8773 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -877,6 +877,15 @@ bool out_of_memory(struct oom_control *oc)
 	}
 
 	/*
+	 * The OOM killer does not compensate for IO-less reclaim.
+	 * pagefault_out_of_memory lost its gfp context so we have to
+	 * make sure exclude 0 mask - all other users should have at least
+	 * ___GFP_DIRECT_RECLAIM to get here.
+	 */
+	if (oc->gfp_mask && !(oc->gfp_mask & (__GFP_FS|__GFP_NOFAIL)))
+		return true;
+
+	/*
 	 * Check if there were limitations on the allocation (only relevant for
 	 * NUMA) that may require different handling.
 	 */
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 1b889dba7bd4..736ea28abfcf 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2872,22 +2872,18 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
 		/* The OOM killer does not needlessly kill tasks for lowmem */
 		if (ac->high_zoneidx < ZONE_NORMAL)
 			goto out;
-		/* The OOM killer does not compensate for IO-less reclaim */
-		if (!(gfp_mask & __GFP_FS)) {
-			/*
-			 * XXX: Page reclaim didn't yield anything,
-			 * and the OOM killer can't be invoked, but
-			 * keep looping as per tradition.
-			 *
-			 * But do not keep looping if oom_killer_disable()
-			 * was already called, for the system is trying to
-			 * enter a quiescent state during suspend.
-			 */
-			*did_some_progress = !oom_killer_disabled;
-			goto out;
-		}
 		if (pm_suspended_storage())
 			goto out;
+		/*
+		 * XXX: GFP_NOFS allocations should rather fail than rely on
+		 * other request to make a forward progress.
+		 * We are in an unfortunate situation where out_of_memory cannot
+		 * do much for this context but let's try it to at least get
+		 * access to memory reserved if the current task is killed (see
+		 * out_of_memory). Once filesystems are ready to handle allocation
+		 * failures more gracefully we should just bail out here.
+		 */
+
 		/* The OOM killer may not free memory on a specific node */
 		if (gfp_mask & __GFP_THISNODE)
 			goto out;
-- 
2.8.0.rc3

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web