Re: [REPOST] [PATCH 1/2] mm: Fix race between setting TIF_MEMDIE and __alloc_pages_high_priority().

[Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>]

Michal Hocko wrote:
> The TIF_MEMDIE check was explicit for a good reason IMO. The race is not
> really that important AFAICS because we would only fail the allocation
> sooner for the OOM victim and that one might fail already. I might be
> missing something of course but your change has a higher risk of
> undesired behavior than the original code.

In a different thread, you are trying to allow giving up !__GFP_FS
allocations without retrying because giving up __GFP_FS allocations
without retrying increases possibility of hitting obsucure bugs in the
error recovery paths. This TIF_MEMDIE v.s. __alloc_pages_high_priority()
race condition allows giving up not only !__GFP_FS allocations but also
__GFP_FS allocations; i.e. fixing this race reduces possibility of
hitting obsucure bugs.

Thus, here is an updated patch.
------------------------------------------------------------
>From 5300fa1b78130113189e72a0a09e9a49090b5f1e Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Thu, 27 Aug 2015 22:30:13 +0900
Subject: [PATCH] mm: Fix race between setting TIF_MEMDIE and __alloc_pages_high_priority().

Currently, TIF_MEMDIE is checked at gfp_to_alloc_flags() which is before
calling __alloc_pages_high_priority() and at

  /* Avoid allocations with no watermarks from looping endlessly */
  if (test_thread_flag(TIF_MEMDIE) && !(gfp_mask & __GFP_NOFAIL))

which is after returning from __alloc_pages_high_priority(). This means
that if TIF_MEMDIE is set between returning from gfp_to_alloc_flags() and
checking test_thread_flag(TIF_MEMDIE), the allocation will fail without
calling __alloc_pages_high_priority().

For now, we need to try to avoid failing small __GFP_FS allocations
because many of error recovery paths are untested, resulting in obscure
bugs. This patch replaces "test_thread_flag(TIF_MEMDIE)" with "whether
TIF_MEMDIE was already set as of calling gfp_to_alloc_flags()" in order
to try to avoid such failures.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 mm/page_alloc.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 2ff998c..8880b17 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3012,6 +3012,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	enum migrate_mode migration_mode = MIGRATE_ASYNC;
 	bool deferred_compaction = false;
 	int contended_compaction = COMPACT_CONTENDED_NONE;
+	bool memdie_pending;
 
 	/*
 	 * In the slowpath, we sanity check order to avoid ever trying to
@@ -3036,6 +3037,7 @@ retry:
 	if (!(gfp_mask & __GFP_NO_KSWAPD))
 		wake_all_kswapds(order, ac);
 
+	memdie_pending = test_thread_flag(TIF_MEMDIE);
 	/*
 	 * OK, we're below the kswapd watermark and have kicked background
 	 * reclaim. Now things get more complex, so set up alloc_flags according
@@ -3091,8 +3093,13 @@ retry:
 	if (current->flags & PF_MEMALLOC)
 		goto nopage;
 
-	/* Avoid allocations with no watermarks from looping endlessly */
-	if (test_thread_flag(TIF_MEMDIE) && !(gfp_mask & __GFP_NOFAIL))
+	/*
+	 * Give up if chosen as an OOM victim. But if the context allows,
+	 * make sure that __alloc_pages_high_priority() was called before
+	 * giving up, for failing small __GFP_FS allocations are prone to
+	 * trigger obscure bugs.
+	 */
+	if (memdie_pending && !(gfp_mask & __GFP_NOFAIL))
 		goto nopage;
 
 	/*
-- 
1.8.3.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>