[PATCH] mm/page_alloc: Don't fail costly __GFP_NOFAIL allocations.

[PATCH] mm/page_alloc: Don't fail costly __GFP_NOFAIL allocations.

[Tetsuo Handa]

Filesystem code might request costly __GFP_NOFAIL !__GFP_REPEAT GFP_NOFS
allocations. But commit 0a0337e0d1d13446 ("mm, oom: rework oom detection")
overlooked that __GFP_NOFAIL allocation requests need to invoke the OOM
killer and retry even if order > PAGE_ALLOC_COSTLY_ORDER && !__GFP_REPEAT.
The caller will crash if such allocation request failed.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Michal Hocko <mhocko@suse.com>
Cc: <stable@vger.kernel.org> # 4.7+
---
 mm/page_alloc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 6de9440..b458f00 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3650,9 +3650,10 @@ bool gfp_pfmemalloc_allowed(gfp_t gfp_mask)
 
 	/*
 	 * Do not retry costly high order allocations unless they are
-	 * __GFP_REPEAT
+	 * __GFP_REPEAT or __GFP_NOFAIL
 	 */
-	if (order > PAGE_ALLOC_COSTLY_ORDER && !(gfp_mask & __GFP_REPEAT))
+	if (order > PAGE_ALLOC_COSTLY_ORDER &&
+	    !(gfp_mask & (__GFP_REPEAT | __GFP_NOFAIL)))
 		goto nopage;
 
 	/* Make sure we know about allocations which stall for too long */
-- 
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>

[PATCH] mm/page_alloc: Don't fail costly __GFP_NOFAIL allocations.

[Tetsuo Handa]

Filesystem code might request costly __GFP_NOFAIL !__GFP_REPEAT GFP_NOFS
allocations. But commit 0a0337e0d1d13446 ("mm, oom: rework oom detection")
overlooked that __GFP_NOFAIL allocation requests need to invoke the OOM
killer and retry even if order > PAGE_ALLOC_COSTLY_ORDER && !__GFP_REPEAT.
The caller will crash if such allocation request failed.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Michal Hocko <mhocko@suse.com>
Cc: <stable@vger.kernel.org> # 4.7+
---
 mm/page_alloc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 6de9440..b458f00 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3650,9 +3650,10 @@ bool gfp_pfmemalloc_allowed(gfp_t gfp_mask)
 
 	/*
 	 * Do not retry costly high order allocations unless they are
-	 * __GFP_REPEAT
+	 * __GFP_REPEAT or __GFP_NOFAIL
 	 */
-	if (order > PAGE_ALLOC_COSTLY_ORDER && !(gfp_mask & __GFP_REPEAT))
+	if (order > PAGE_ALLOC_COSTLY_ORDER &&
+	    !(gfp_mask & (__GFP_REPEAT | __GFP_NOFAIL)))
 		goto nopage;
 
 	/* Make sure we know about allocations which stall for too long */
-- 
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>

Re: [PATCH] mm/page_alloc: Don't fail costly __GFP_NOFAIL allocations.

[Michal Hocko]

On Thu 17-11-16 21:50:04, Tetsuo Handa wrote:
> Filesystem code might request costly __GFP_NOFAIL !__GFP_REPEAT GFP_NOFS
> allocations. But commit 0a0337e0d1d13446 ("mm, oom: rework oom detection")
> overlooked that __GFP_NOFAIL allocation requests need to invoke the OOM
> killer and retry even if order > PAGE_ALLOC_COSTLY_ORDER && !__GFP_REPEAT.
> The caller will crash if such allocation request failed.

Could you point to such an allocation request please? Costly GFP_NOFAIL
requests are a really high requirement and I am even not sure we should
support them. buffered_rmqueue already warns about order > 1 NOFAIL
allocations.

I am not saying the patch is incorrect but it sounds more a theoretical
than practical issue which should be considered when involving the
stable tree here. To be honest I would rather see a single place which
handles all NOFAIL fallbacks rather than make the code even more
convoluted than it is already.

> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: <stable@vger.kernel.org> # 4.7+
> ---
>  mm/page_alloc.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 6de9440..b458f00 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3650,9 +3650,10 @@ bool gfp_pfmemalloc_allowed(gfp_t gfp_mask)
>  
>  	/*
>  	 * Do not retry costly high order allocations unless they are
> -	 * __GFP_REPEAT
> +	 * __GFP_REPEAT or __GFP_NOFAIL
>  	 */
> -	if (order > PAGE_ALLOC_COSTLY_ORDER && !(gfp_mask & __GFP_REPEAT))
> +	if (order > PAGE_ALLOC_COSTLY_ORDER &&
> +	    !(gfp_mask & (__GFP_REPEAT | __GFP_NOFAIL)))
>  		goto nopage;
>  
>  	/* Make sure we know about allocations which stall for too long */
> -- 
> 1.8.3.1
> 

-- 
Michal Hocko
SUSE Labs

--
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>

Re: [PATCH] mm/page_alloc: Don't fail costly __GFP_NOFAIL allocations.

[Tetsuo Handa]

Michal Hocko wrote:
> On Thu 17-11-16 21:50:04, Tetsuo Handa wrote:
> > Filesystem code might request costly __GFP_NOFAIL !__GFP_REPEAT GFP_NOFS
> > allocations. But commit 0a0337e0d1d13446 ("mm, oom: rework oom detection")
> > overlooked that __GFP_NOFAIL allocation requests need to invoke the OOM
> > killer and retry even if order > PAGE_ALLOC_COSTLY_ORDER && !__GFP_REPEAT.
> > The caller will crash if such allocation request failed.
>
> Could you point to such an allocation request please? Costly GFP_NOFAIL
> requests are a really high requirement and I am even not sure we should
> support them. buffered_rmqueue already warns about order > 1 NOFAIL
> allocations.

That question is pointless. You are simply lucky that you see only order 0 or
order 1. There are many __GFP_NOFAIL allocations where order is determined at
runtime. There is no guarantee that order 2 and above never happens.
http://lkml.kernel.org/r/56F8F5DA.6040206@kyup.com is a case where an XFS user
had trouble due to order 4 and order 5 allocations because XFS uses opencoded
endless loop around allocator than __GFP_NOFAIL.

There is WARN_ON_ONCE((gfp_flags & __GFP_NOFAIL) && (order > 1)); in
buffered_rmqueue(), but there is no guarantee that the caller hits this
warning. It is theoretically possible that order > PAGE_ALLOC_COSTLY_ORDER &&
__GFP_NOFAIL && !__GFP_REPEAT allocation requests fail to call buffered_rmqueue()
 from get_page_from_freelist() due to "continue;" statements around zone
watermark checks. It is possible that an order == 2 && __GFP_NOFAIL allocation
reuest hit this WARN_ON_ONCE() and subsequent order > PAGE_ALLOC_COSTLY_ORDER &&
__GFP_NOFAIL && !__GFP_REPEAT allocation requests no longer hits this WARN_ON_ONCE().

So, you want to mess up the definition of __GFP_NOFAIL like below?
I think my patch is better than below change.

--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -138,9 +138,16 @@
  * __GFP_NOFAIL: The VM implementation _must_ retry infinitely: the caller
  *   cannot handle allocation failures. New users should be evaluated carefully
  *   (and the flag should be used only when there is no reasonable failure
  *   policy) but it is definitely preferable to use the flag rather than
- *   opencode endless loop around allocator.
+ *   opencode endless loop around allocator. However, the VM implementation
+ *   is allowed to fail if order > PAGE_ALLOC_COSTLY_ORDER but __GFP_REPEAT
+ *   is not set. That is, all users which specify __GFP_NOFAIL with order
+ *   determined at runtime (e.g. sizeof(struct) * num_elements) had better
+ *   specify __GFP_REPEAT as well, for the VM implementation does not invoke
+ *   the OOM killer unless __GFP_REPEAT is also specified and the caller has
+ *   to be prepared for allocation failures using opencode endless loop
+ *   around allocator.
  *
  * __GFP_NORETRY: The VM implementation must not retry indefinitely and will
  *   return NULL when direct reclaim and memory compaction have failed to allow
  *   the allocation to succeed.  The OOM killer is not called with the current

--
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>

Re: [PATCH] mm/page_alloc: Don't fail costly __GFP_NOFAIL allocations.

[Michal Hocko]

On Mon 21-11-16 20:16:40, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Thu 17-11-16 21:50:04, Tetsuo Handa wrote:
> > > Filesystem code might request costly __GFP_NOFAIL !__GFP_REPEAT GFP_NOFS
> > > allocations. But commit 0a0337e0d1d13446 ("mm, oom: rework oom detection")
> > > overlooked that __GFP_NOFAIL allocation requests need to invoke the OOM
> > > killer and retry even if order > PAGE_ALLOC_COSTLY_ORDER && !__GFP_REPEAT.
> > > The caller will crash if such allocation request failed.
> >
> > Could you point to such an allocation request please? Costly GFP_NOFAIL
> > requests are a really high requirement and I am even not sure we should
> > support them. buffered_rmqueue already warns about order > 1 NOFAIL
> > allocations.
> 
> That question is pointless. You are simply lucky that you see only order 0 or
> order 1. There are many __GFP_NOFAIL allocations where order is determined at
> runtime. There is no guarantee that order 2 and above never happens.

You are pushing to the extreme again! Your changelog stated this might
be an existing and the real life problem and that is the reason I've
asked. Especially because you have marked the patch for stable. As I've
said in my previous response. Your patch looks correct, I am just not
entirely happy to clutter the code path even more for GFP_NOFAIL for
something we maybe even do not support. All the checks we have there are
head spinning already.

So we have two options, either we have real users of GFP_NOFAIL for
costly orders and handle that properly with all that information in the
changelog or simply rely on the warning and fix callers who do that
accidentally. But please stop this, theoretically something might do
$THIS_RANDOM_GFP_FLAGS + order combination and we absolutely must handle
that in the allocator.

Thanks!
-- 
Michal Hocko
SUSE Labs

--
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>

Re: [PATCH] mm/page_alloc: Don't fail costly __GFP_NOFAIL allocations.

[Michal Hocko]

On Mon 21-11-16 13:54:31, Michal Hocko wrote:
> On Mon 21-11-16 20:16:40, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > On Thu 17-11-16 21:50:04, Tetsuo Handa wrote:
> > > > Filesystem code might request costly __GFP_NOFAIL !__GFP_REPEAT GFP_NOFS
> > > > allocations. But commit 0a0337e0d1d13446 ("mm, oom: rework oom detection")
> > > > overlooked that __GFP_NOFAIL allocation requests need to invoke the OOM
> > > > killer and retry even if order > PAGE_ALLOC_COSTLY_ORDER && !__GFP_REPEAT.
> > > > The caller will crash if such allocation request failed.
> > >
> > > Could you point to such an allocation request please? Costly GFP_NOFAIL
> > > requests are a really high requirement and I am even not sure we should
> > > support them. buffered_rmqueue already warns about order > 1 NOFAIL
> > > allocations.
> > 
> > That question is pointless. You are simply lucky that you see only order 0 or
> > order 1. There are many __GFP_NOFAIL allocations where order is determined at
> > runtime. There is no guarantee that order 2 and above never happens.
> 
> You are pushing to the extreme again! Your changelog stated this might
> be an existing and the real life problem and that is the reason I've
> asked. Especially because you have marked the patch for stable. As I've
> said in my previous response. Your patch looks correct, I am just not
> entirely happy to clutter the code path even more for GFP_NOFAIL for
> something we maybe even do not support. All the checks we have there are
> head spinning already.
> 
> So we have two options, either we have real users of GFP_NOFAIL for
> costly orders and handle that properly with all that information in the
> changelog or simply rely on the warning and fix callers who do that
> accidentally. But please stop this, theoretically something might do
> $THIS_RANDOM_GFP_FLAGS + order combination and we absolutely must handle
> that in the allocator.

So if we really want to pretend to support GFP_NOFAIL + costly order,
and I still do not see any real user in the kernel but I can imagine
that some of the opencoded endless loops around allocator might
eventually become GFP_NOFAIL so there is some merit to be prepared for
that. So this is something I've ended up with (no compilation testing
yet because my gcc decided to not cooperate and fail with
kernel/bounds.c:1:0: error: code model kernel does not support PIC mode)

Anyway the intention should be pretty clear from the diff
---
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index f62860e6dfb9..8170ee8765b7 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3627,26 +3626,12 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 		goto got_pg;
 
 	/* Caller is not willing to reclaim, we can't balance anything */
-	if (!can_direct_reclaim) {
-		/*
-		 * All existing users of the __GFP_NOFAIL are blockable, so warn
-		 * of any new users that actually allow this type of allocation
-		 * to fail.
-		 */
-		WARN_ON_ONCE(gfp_mask & __GFP_NOFAIL);
-		goto fail;
-	}
+	if (!can_direct_reclaim)
+		goto nopage;
 
 	/* Avoid recursion of direct reclaim */
-	if (current->flags & PF_MEMALLOC) {
-		/*
-		 * __GFP_NOFAIL request from this context is rather bizarre
-		 * because we cannot reclaim anything and only can loop waiting
-		 * for somebody to do a work for us.
-		 */
-		WARN_ON_ONCE(gfp_mask & __GFP_NOFAIL);
+	if (current->flags & PF_MEMALLOC)
 		goto nopage;
-	}
 
 	/* Avoid allocations with no watermarks from looping endlessly */
 	if (test_thread_flag(TIF_MEMDIE))
@@ -3717,6 +3702,28 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	 * we always retry
 	 */
 	if (gfp_mask & __GFP_NOFAIL) {
+		/*
+		 * All existing users of the __GFP_NOFAIL are blockable, so warn
+		 * of any new users that actually require GFP_NOWAIT
+		 */
+		if (WARN_ON_ONCE(!can_direct_reclaim))
+			goto fail;
+
+		/*
+		 * PF_MEMALLOC request from this context is rather bizarre
+		 * because we cannot reclaim anything and only can loop waiting
+		 * for somebody to do a work for us
+		 */
+		WARN_ON_ONCE(current->flags & PF_MEMALLOC);
+
+		/*
+		 * non failing costly orders are a hard requirement which we
+		 * are not prepared for much so let's warn about these users
+		 * so that we can identify them and convert them to something
+		 * else.
+		 */
+		WARN_ON_ONCE(order > PAGE_ALLOC_COSTLY_ORDER);
+
 		cond_resched();
 		goto retry;
 	}

I would even go one step further and do the following because, honestly,
I never liked GFP_NOFAIL having OOM side effects.

@@ -3078,32 +3078,31 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
 	if (page)
 		goto out;
 
-	if (!(gfp_mask & __GFP_NOFAIL)) {
-		/* Coredumps can quickly deplete all memory reserves */
-		if (current->flags & PF_DUMPCORE)
-			goto out;
-		/* The OOM killer will not help higher order allocs */
-		if (order > PAGE_ALLOC_COSTLY_ORDER)
-			goto out;
-		/* The OOM killer does not needlessly kill tasks for lowmem */
-		if (ac->high_zoneidx < ZONE_NORMAL)
-			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.
-		 */
+	/* Coredumps can quickly deplete all memory reserves */
+	if (current->flags & PF_DUMPCORE)
+		goto out;
+	/* The OOM killer will not help higher order allocs */
+	if (order > PAGE_ALLOC_COSTLY_ORDER)
+		goto out;
+	/* The OOM killer does not needlessly kill tasks for lowmem */
+	if (ac->high_zoneidx < ZONE_NORMAL)
+		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;
 
-		/* The OOM killer may not free memory on a specific node */
-		if (gfp_mask & __GFP_THISNODE)
-			goto out;
-	}
 	/* Exhausted what can be done so it's blamo time */
 	if (out_of_memory(&oc) || WARN_ON_ONCE(gfp_mask & __GFP_NOFAIL)) {
 		*did_some_progress = 1;
-- 
Michal Hocko
SUSE Labs

--
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>

Re: [PATCH] mm/page_alloc: Don't fail costly __GFP_NOFAIL allocations.

[Michal Hocko]

On Tue 22-11-16 07:29:36, Michal Hocko wrote:
> I would even go one step further and do the following because, honestly,
> I never liked GFP_NOFAIL having OOM side effects.
> 
> @@ -3078,32 +3078,31 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
>  	if (page)
>  		goto out;
>  
> -	if (!(gfp_mask & __GFP_NOFAIL)) {
> -		/* Coredumps can quickly deplete all memory reserves */
> -		if (current->flags & PF_DUMPCORE)
> -			goto out;
> -		/* The OOM killer will not help higher order allocs */
> -		if (order > PAGE_ALLOC_COSTLY_ORDER)
> -			goto out;
> -		/* The OOM killer does not needlessly kill tasks for lowmem */
> -		if (ac->high_zoneidx < ZONE_NORMAL)
> -			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.
> -		 */
> +	/* Coredumps can quickly deplete all memory reserves */
> +	if (current->flags & PF_DUMPCORE)
> +		goto out;
> +	/* The OOM killer will not help higher order allocs */
> +	if (order > PAGE_ALLOC_COSTLY_ORDER)
> +		goto out;
> +	/* The OOM killer does not needlessly kill tasks for lowmem */
> +	if (ac->high_zoneidx < ZONE_NORMAL)
> +		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;
>  
> -		/* The OOM killer may not free memory on a specific node */
> -		if (gfp_mask & __GFP_THISNODE)
> -			goto out;
> -	}
>  	/* Exhausted what can be done so it's blamo time */
>  	if (out_of_memory(&oc) || WARN_ON_ONCE(gfp_mask & __GFP_NOFAIL)) {
>  		*did_some_progress = 1;

Forgot to include this part of course

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index ec9f11d4f094..12a6fce85f61 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -1013,7 +1013,7 @@ bool out_of_memory(struct oom_control *oc)
 	 * 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)))
+	if (oc->gfp_mask && !(oc->gfp_mask & __GFP_FS))
 		return true;
 
 	/*

Anyway I will think about this some more and prepapre patches with the
full changelog for further discussion.
-- 
Michal Hocko
SUSE Labs

--
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>