GFP_NOFAIL cleanups

From: Michal Hocko <[email protected]>

Tetsuo Handa has pointed out that 0a0337e0d1d1 ("mm, oom: rework oom detection") has subtly changed semantic for costly high order requests with __GFP_NOFAIL and withtout __GFP_REPEAT and those can fail right now. My code inspection didn't reveal any such users in the tree but it is true that this might lead to unexpected allocation failures and subsequent OOPs.

__alloc_pages_slowpath wrt. GFP_NOFAIL is hard to follow currently. There are few special cases but we are lacking a catch all place to be sure we will not miss any case where the non failing allocation might fail. This patch reorganizes the code a bit and puts all those special cases under nopage label which is the generic go-to-fail path. Non failing allocations are retried or those that cannot retry like non-sleeping allocation go to the failure point directly. This should make the code flow much easier to follow and make it less error prone for future changes.

While we are there we have to move the stall check up to catch potentially looping non-failing allocations.

Signed-off-by Michal Hocko <[email protected]>

---
 mm/page_alloc.c | 68 ++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 41 insertions(+), 27 deletions(-)

mm/page_alloc.c +34 -27

index 0fbfead6aa7d..76c0b6bb0baf 100644
@@ -3627,32 +3627,23 @@ __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);
+	if (!can_direct_reclaim)
 		goto nopage;
+
+	/* Make sure we know about allocations which stall for too long */
+	if (time_after(jiffies, alloc_start + stall_timeout)) {
+		warn_alloc(gfp_mask,
+			"page alloction stalls for %ums, order:%u",
+			jiffies_to_msecs(jiffies-alloc_start), order);
+		stall_timeout += 10 * HZ;
 	}
 
 	/* 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.
-		 */
-		if (WARN_ON_ONCE(gfp_mask & __GFP_NOFAIL)) {
-			cond_resched();
-			goto 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))
+	if (test_thread_flag(TIF_MEMDIE))
 		goto nopage;
 
 

@@ -3679,14 +3670,6 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	if (order > PAGE_ALLOC_COSTLY_ORDER && !(gfp_mask & __GFP_REPEAT))
 		goto nopage;
 
-	/* Make sure we know about allocations which stall for too long */
-	if (time_after(jiffies, alloc_start + stall_timeout)) {
-		warn_alloc(gfp_mask,
-			"page alloction stalls for %ums, order:%u",
-			jiffies_to_msecs(jiffies-alloc_start), order);
-		stall_timeout += 10 * HZ;
-	}
-
 	if (should_reclaim_retry(gfp_mask, order, ac, alloc_flags,
 				 did_some_progress > 0, &no_progress_loops))
 		goto retry;

@@ -3715,6 +3698,37 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	}
 
 nopage:
+	/*
+	 * Make sure that __GFP_NOFAIL request doesn't leak out and make sure
+	 * 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.

From: Michal Hocko <[email protected]>

__alloc_pages_may_oom makes sure to skip the OOM killer depending on the allocation request. This includes lowmem requests, costly high order requests and others. For a long time __GFP_NOFAIL acted as an override for all those rules. This is not documented and it can be quite surprising as well. E.g. GFP_NOFS requests are not invoking the OOM killer but GFP_NOFS|__GFP_NOFAIL does so if we try to convert some of the existing open coded loops around allocator to nofail request (and we have done that in the past) then such a change would have a non trivial side effect which is not obvious. Note that the primary motivation for skipping the OOM killer is to prevent from pre-mature invocation.

The exception has been added by 82553a937f12 ("oom: invoke oom killer for __GFP_NOFAIL"). The changelog points out that the oom killer has to be invoked otherwise the request would be looping for ever. But this argument is rather weak because the OOM killer doesn't really guarantee any forward progress for those exceptional cases - e.g. it will hardly help to form costly order - I believe we certainly do not want to kill all processes and eventually panic the system just because there is a nasty driver asking for order-9 page with GFP_NOFAIL not realizing all the consequences - it is much better this request would loop for ever than the massive system disruption, lowmem is also highly unlikely to be freed during OOM killer and GFP_NOFS request could trigger while there is still a lot of memory pinned by filesystems.

This patch simply removes the __GFP_NOFAIL special case in order to have a more clear semantic without surprising side effects. Instead we do allow nofail requests to access memory reserves to move forward in both cases when the OOM killer is invoked and when it should be supressed. __alloc_pages_nowmark helper has been introduced for that purpose.

Signed-off-by Michal Hocko <[email protected]>

---
 mm/oom_kill.c   |  2 +-
 mm/page_alloc.c | 95 +++++++++++++++++++++++++++++++++++----------------------
 2 files changed, 59 insertions(+), 38 deletions(-)

mm/oom_kill.c +1 -1

index ec9f11d4f094..12a6fce85f61 100644
@@ -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;
 
 	/*

mm/page_alloc.c +4 -0

index 76c0b6bb0baf..7102641147c4 100644
@@ -3044,6 +3044,25 @@ void warn_alloc(gfp_t gfp_mask, const char *fmt, ...)
 }
 
 static inline struct page *
+__alloc_pages_nowmark(gfp_t gfp_mask, unsigned int order,
+						const struct alloc_context *ac)
+{
+	struct page *page;

mm/page_alloc.c +7 -11

 		*did_some_progress = 1;
 
-		if (gfp_mask & __GFP_NOFAIL) {
-			page = get_page_from_freelist(gfp_mask, order,
-					ALLOC_NO_WATERMARKS|ALLOC_CPUSET, ac);
-			/*
-			 * fallback to ignore cpuset restriction if our nodes
-			 * are depleted
-			 */
-			if (!page)
-				page = get_page_from_freelist(gfp_mask, order,
-					ALLOC_NO_WATERMARKS, ac);
-		}
+		/*
+		 * Help non-failing allocations by giving them access to memory
+		 * reserves
+		 */
+		if (gfp_mask & __GFP_NOFAIL)
+			page = __alloc_pages_nowmark(gfp_mask, order, ac);
 	}
 out:
 	mutex_unlock(&oom_lock);

@@ -3725,6 +3738,14 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 		 */
 		WARN_ON_ONCE(order > PAGE_ALLOC_COSTLY_ORDER);
 
+		/*

Quoted 32 lines · Michal Hocko

From: Michal Hocko <[email protected]>

__alloc_pages_may_oom makes sure to skip the OOM killer depending on the allocation request. This includes lowmem requests, costly high order requests and others. For a long time __GFP_NOFAIL acted as an override for all those rules. This is not documented and it can be quite surprising as well. E.g. GFP_NOFS requests are not invoking the OOM killer but GFP_NOFS|__GFP_NOFAIL does so if we try to convert some of the existing open coded loops around allocator to nofail request (and we have done that in the past) then such a change would have a non trivial side effect which is not obvious. Note that the primary motivation for skipping the OOM killer is to prevent from pre-mature invocation.

The exception has been added by 82553a937f12 ("oom: invoke oom killer for __GFP_NOFAIL"). The changelog points out that the oom killer has to be invoked otherwise the request would be looping for ever. But this argument is rather weak because the OOM killer doesn't really guarantee any forward progress for those exceptional cases - e.g. it will hardly help to form costly order - I believe we certainly do not want to kill all processes and eventually panic the system just because there is a nasty driver asking for order-9 page with GFP_NOFAIL not realizing all the consequences - it is much better this request would loop for ever than the massive system disruption, lowmem is also highly unlikely to be freed during OOM killer and GFP_NOFS request could trigger while there is still a lot of memory pinned by filesystems.

This patch simply removes the __GFP_NOFAIL special case in order to have a more clear semantic without surprising side effects. Instead we do allow nofail requests to access memory reserves to move forward in both cases when the OOM killer is invoked and when it should be supressed. __alloc_pages_nowmark helper has been introduced for that purpose.

Quoted 1 line

Signed-off-by Michal Hocko <[email protected]>

This makes some sense to me, but there might be unpleasant consequences, e.g. due to allowing costly allocations without reserves.

I guess only testing will show...

Also some comments below.

Quoted 11 lines

static inline struct page *

mm/page_alloc.c +10 -0

+__alloc_pages_nowmark(gfp_t gfp_mask, unsigned int order,
+						const struct alloc_context *ac)
+{
+	struct page *page;
+
+	page = get_page_from_freelist(gfp_mask, order,
+			ALLOC_NO_WATERMARKS|ALLOC_CPUSET, ac);
+	/*
+	 * fallback to ignore cpuset restriction if our nodes
+	 * are depleted

mm/page_alloc.c void warn_alloc(gfp_t gfp_mask, const char *fmt, ...) · line 3060

Quoted 4 lines

mm/page_alloc.c +4 -0

+	 */
+	if (!page)
+		page = get_page_from_freelist(gfp_mask, order,
+				ALLOC_NO_WATERMARKS, ac);

Is this enough? Look at what __alloc_pages_slowpath() does since e46e7b77c909 ("mm, page_alloc: recalculate the preferred zoneref if the context can ignore memory policies").

...

mm/page_alloc.c __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order, · line 3126

Quoted 4 lines

mm/page_alloc.c +1 -2

-	}
 	/* Exhausted what can be done so it's blamo time */
-	if (out_of_memory(&oc) || WARN_ON_ONCE(gfp_mask & __GFP_NOFAIL)) {
+	if (out_of_memory(&oc)) {

This removes the warning, but also the check for __GFP_NOFAIL itself. Was it what you wanted?

Quoted 2 lines

*did_some_progress = 1;

Quoted 2 lines · Vlastimil Babka

This makes some sense to me, but there might be unpleasant consequences, e.g. due to allowing costly allocations without reserves.

I am not sure I understand. Did you mean with reserves? Anyway, my code

inspection shown that we are not really doing GFP_NOFAIL for costly orders. This might change in the future but even if we do that then this shouldn't add a risk of the reserves depletion, right?

Quoted 3 lines

I guess only testing will show...

Also some comments below.

[...]

Quoted 11 lines

Quoted 11 lines

static inline struct page *

mm/page_alloc.c +10 -0

+__alloc_pages_nowmark(gfp_t gfp_mask, unsigned int order,
+						const struct alloc_context *ac)
+{
+	struct page *page;
+
+	page = get_page_from_freelist(gfp_mask, order,
+			ALLOC_NO_WATERMARKS|ALLOC_CPUSET, ac);
+	/*
+	 * fallback to ignore cpuset restriction if our nodes
+	 * are depleted

Quoted 8 lines

mm/page_alloc.c void warn_alloc(gfp_t gfp_mask, const char *fmt, ...) · line 3060

Quoted 4 lines

mm/page_alloc.c +4 -0

+	 */
+	if (!page)
+		page = get_page_from_freelist(gfp_mask, order,
+				ALLOC_NO_WATERMARKS, ac);

Is this enough? Look at what __alloc_pages_slowpath() does since e46e7b77c909 ("mm, page_alloc: recalculate the preferred zoneref if the context can ignore memory policies").

this is a one time attempt to do the nowmark allocation. If we need to do the recalculation then this should happen in the next round. Or am I missing your question?

Quoted 11 lines

...

mm/page_alloc.c __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order, · line 3126

Quoted 4 lines

mm/page_alloc.c +1 -2

-	}
 	/* Exhausted what can be done so it's blamo time */
-	if (out_of_memory(&oc) || WARN_ON_ONCE(gfp_mask & __GFP_NOFAIL)) {
+	if (out_of_memory(&oc)) {

This removes the warning, but also the check for __GFP_NOFAIL itself. Was it what you wanted?

The point of the check was to keep looping for __GFP_NOFAIL requests even when the OOM killer is disabled (out_of_memory returns false). We are accomplishing that by

Quoted 2 lines

Quoted 2 lines

*did_some_progress = 1;

		^^^^ this

it is true we will not have the warning but I am not really sure we care all that much. In any case it wouldn't be all that hard to check for oom killer disabled and warn on in the allocator slow path.

thanks for having a look!

Quoted 1 line · Michal Hocko

On Wed 23-11-16 13:19:20, Vlastimil Babka wrote:

Quoted 20 lines

Quoted 19 lines

Quoted 5 lines

static inline struct page *

mm/page_alloc.c +4 -0

+__alloc_pages_nowmark(gfp_t gfp_mask, unsigned int order,
+						const struct alloc_context *ac)
+{
+	struct page *page;

mm/page_alloc.c void warn_alloc(gfp_t gfp_mask, const char *fmt, ...) · line 3060

Quoted 10 lines

mm/page_alloc.c +10 -0

+
+	page = get_page_from_freelist(gfp_mask, order,
+			ALLOC_NO_WATERMARKS|ALLOC_CPUSET, ac);
+	/*
+	 * fallback to ignore cpuset restriction if our nodes
+	 * are depleted
+	 */
+	if (!page)
+		page = get_page_from_freelist(gfp_mask, order,
+				ALLOC_NO_WATERMARKS, ac);

Is this enough? Look at what __alloc_pages_slowpath() does since e46e7b77c909 ("mm, page_alloc: recalculate the preferred zoneref if the context can ignore memory policies").

Quoted 3 lines

this is a one time attempt to do the nowmark allocation. If we need to do the recalculation then this should happen in the next round. Or am I missing your question?

The next round no-watermarks allocation attempt in __alloc_pages_slowpath() uses different criteria than the new __alloc_pages_nowmark() callers. And it would be nicer to unify this as well, if possible.

Quoted 8 lines

Quoted 7 lines

mm/page_alloc.c __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order, · line 3126

Quoted 4 lines

mm/page_alloc.c +1 -2

-	}
 	/* Exhausted what can be done so it's blamo time */
-	if (out_of_memory(&oc) || WARN_ON_ONCE(gfp_mask & __GFP_NOFAIL)) {
+	if (out_of_memory(&oc)) {

This removes the warning, but also the check for __GFP_NOFAIL itself. Was it what you wanted?

Quoted 6 lines

The point of the check was to keep looping for __GFP_NOFAIL requests even when the OOM killer is disabled (out_of_memory returns false). We are accomplishing that by

Quoted 2 lines

Quoted 2 lines

*did_some_progress = 1;

		^^^^ this

But oom disabled means that this line is not reached?

From: Michal Hocko <[email protected]>

__alloc_pages_may_oom makes sure to skip the OOM killer depending on the allocation request. This includes lowmem requests, costly high order requests and others. For a long time __GFP_NOFAIL acted as an override for all those rules. This is not documented and it can be quite surprising as well. E.g. GFP_NOFS requests are not invoking the OOM killer but GFP_NOFS|__GFP_NOFAIL does so if we try to convert some of the existing open coded loops around allocator to nofail request (and we have done that in the past) then such a change would have a non trivial side effect which is not obvious. Note that the primary motivation for skipping the OOM killer is to prevent from pre-mature invocation.

The exception has been added by 82553a937f12 ("oom: invoke oom killer for __GFP_NOFAIL"). The changelog points out that the oom killer has to be invoked otherwise the request would be looping for ever. But this argument is rather weak because the OOM killer doesn't really guarantee any forward progress for those exceptional cases - e.g. it will hardly help to form costly order - I believe we certainly do not want to kill all processes and eventually panic the system just because there is a nasty driver asking for order-9 page with GFP_NOFAIL not realizing all the consequences - it is much better this request would loop for ever than the massive system disruption, lowmem is also highly unlikely to be freed during OOM killer and GFP_NOFS request could trigger while there is still a lot of memory pinned by filesystems.

This patch simply removes the __GFP_NOFAIL special case in order to have a more clear semantic without surprising side effects. Instead we do allow nofail requests to access memory reserves to move forward in both cases when the OOM killer is invoked and when it should be supressed. __alloc_pages_nowmark helper has been introduced for that purpose.

mm/page_alloc.c +0 -0

 		cond_resched();
 		goto retry;
 	}

Signature · 1 line

2.10.2

Quoted 26 lines · Michal Hocko

From: Michal Hocko <[email protected]>

__alloc_pages_may_oom makes sure to skip the OOM killer depending on the allocation request. This includes lowmem requests, costly high order requests and others. For a long time __GFP_NOFAIL acted as an override for all those rules. This is not documented and it can be quite surprising as well. E.g. GFP_NOFS requests are not invoking the OOM killer but GFP_NOFS|__GFP_NOFAIL does so if we try to convert some of the existing open coded loops around allocator to nofail request (and we have done that in the past) then such a change would have a non trivial side effect which is not obvious. Note that the primary motivation for skipping the OOM killer is to prevent from pre-mature invocation.

The exception has been added by 82553a937f12 ("oom: invoke oom killer for __GFP_NOFAIL"). The changelog points out that the oom killer has to be invoked otherwise the request would be looping for ever. But this argument is rather weak because the OOM killer doesn't really guarantee any forward progress for those exceptional cases - e.g. it will hardly help to form costly order - I believe we certainly do not want to kill all processes and eventually panic the system just because there is a nasty driver asking for order-9 page with GFP_NOFAIL not realizing all the consequences - it is much better this request would loop for ever than the massive system disruption, lowmem is also highly unlikely to be freed during OOM killer and GFP_NOFS request could trigger while there is still a lot of memory pinned by filesystems.

Quoted 5 lines

This patch simply removes the __GFP_NOFAIL special case in order to have a more clear semantic without surprising side effects. Instead we do allow nofail requests to access memory reserves to move forward in both cases when the OOM killer is invoked and when it should be supressed. __alloc_pages_nowmark helper has been introduced for that purpose.

__alloc_pages_nowmark() likely works if order is 0, but there is no guarantee that __alloc_pages_nowmark() can find order > 0 pages.

If __alloc_pages_nowmark() called by __GFP_NOFAIL could not find pages with requested order due to fragmentation, __GFP_NOFAIL should invoke the OOM killer. I believe that risking kill all processes and panic the system eventually is better than __GFP_NOFAIL livelock.

I'm not happy that the caller cannot invoke the OOM killer unless __GFP_FS or __GFP_NOFAIL is specified. I think we should get rid of the concept of premature OOM killer invocation.

That is, whenever requested pages cannot
be allocated and the caller does not want to fail, invoking the OOM killer is no longer premature.

Unfortunately, there seems to be cases where the
caller needs to use GFP_NOFS rather than GFP_KERNEL due to unclear dependency between memory allocation by system calls and memory reclaim by filesystems.

But memory reclaim by filesystems are not the fault of userspace processes which issued system calls. It is unfair to force legitimate processes to fail system calls with ENOMEM when GFP_NOFS is used inside system calls instead of killing memory hog processes using the OOM killer. The root cause is that we

blindly treat all memory allocation requests evenly using the same watermark (with rough-grained exceptions such as __GFP_HIGH) and allow lower priority memory allocations (e.g. memory for buffered writes) to consume memory to the level where higher priority memory allocations (e.g. memory for disk I/O) has to retry looping without invoking the OOM killer, instead of using different watermarks based on purpose/importance/priority of individual memory allocation requests so that higher priority memory allocations can invoke the OOM killer.

Quoted 4 lines

mm/page_alloc.c +0 -0

@@ -3725,6 +3738,14 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 		 */
 		WARN_ON_ONCE(order > PAGE_ALLOC_COSTLY_ORDER);
 

mm/page_alloc.c __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order, · line 3748

Quoted 8 lines

mm/page_alloc.c +8 -0

+		/*
+		 * Help non-failing allocations by giving them access to memory
+		 * reserves
+		 */
+		page = __alloc_pages_nowmark(gfp_mask, order, ac);
+		if (page)
+			goto got_pg;
+

Should no_progress_loops be reset to 0 before retrying?

Quoted 4 lines · Tetsuo Handa

If __alloc_pages_nowmark() called by __GFP_NOFAIL could not find pages with requested order due to fragmentation, __GFP_NOFAIL should invoke the OOM killer. I believe that risking kill all processes and panic the system eventually is better than __GFP_NOFAIL livelock.

I violently disagree. Just imagine a driver which asks for an order-9 page and cannot really continue without it so it uses GFP_NOFAIL. There is absolutely no reason to disrupt or even put the whole system down just because of this particular request. It might take for ever to continue but that is to be expected when asking for such a hard requirement.

Quoted 3 lines

Unfortunately, there seems to be cases where the
caller needs to use GFP_NOFS rather than GFP_KERNEL due to unclear dependency between memory allocation by system calls and memory reclaim by filesystems.

I do not understand your point here. Syscall is an entry point to the kernel where we cannot recurse to the FS code so GFP_NOFS seems wrong thing to ask.

Quoted 6 lines · Michal Hocko

Quoted 4 lines · Tetsuo Handa

If __alloc_pages_nowmark() called by __GFP_NOFAIL could not find pages with requested order due to fragmentation, __GFP_NOFAIL should invoke the OOM killer. I believe that risking kill all processes and panic the system eventually is better than __GFP_NOFAIL livelock.

Quoted 6 lines

I violently disagree. Just imagine a driver which asks for an order-9 page and cannot really continue without it so it uses GFP_NOFAIL. There is absolutely no reason to disrupt or even put the whole system down just because of this particular request. It might take for ever to continue but that is to be expected when asking for such a hard requirement.

Did we find such in-tree drivers? If any, we likely already know it via WARN_ON_ONCE((gfp_flags & __GFP_NOFAIL) && (order > 1)); in buffered_rmqueue(). Even if there were such out-of-tree drivers, we don't need to take care of out-of-tree drivers.

Quoted 7 lines

Quoted 3 lines

Unfortunately, there seems to be cases where the
caller needs to use GFP_NOFS rather than GFP_KERNEL due to unclear dependency between memory allocation by system calls and memory reclaim by filesystems.

I do not understand your point here. Syscall is an entry point to the kernel where we cannot recurse to the FS code so GFP_NOFS seems wrong thing to ask.

Will you look at http://marc.info/?t=120716967100004&r=1&w=2 which lead to commit a02fe13297af26c1 ("selinux: prevent rentry into the FS") and commit 869ab5147e1eead8 ("SELinux: more GFP_NOFS fixups to prevent selinux from re-entering the fs code") ? My understanding is that mkdir() system call caused memory allocation for inode creation and that memory allocation caused memory reclaim which had to be !__GFP_FS.

And whether we need to use GFP_NOFS at specific point is very very unclear.