mm: avoid livelock on !__GFP_FS allocations

In his fix, he avoided retrying the allocation if reclaim made no progress and __GFP_FS was not set. The problem is that this would result in GFP_NOIO allocations failing that previously succeeded which would be very unfortunate.

Quoted 4 lines · Mel Gorman

In his fix, he avoided retrying the allocation if reclaim made no progress and __GFP_FS was not set. The problem is that this would result in GFP_NOIO allocations failing that previously succeeded which would be very unfortunate.

GFP_NOFS are made by filesystems/buffers to avoid locking up on fs/vfs locking. Those also should be able to handle failure gracefully but userland is more likely to get a -ENOMEM from these (for example during direct-io) if those fs allocs fails.

So clearly it sounds risky
to apply the modification quoted above and risk having any GFP_NOFS fail. Said that I'm afraid we're not deadlock safe with current code that cannot fail but there's no easy solution and no way to fix it in the short term, and it's only a theoretical concern.

For !__GFP_FS allocations, __GFP_NOFAIL is the default for order <= PAGE_ALLOC_COSTLY_ORDER and __GFP_NORETRY is the default for order > PAGE_ALLOC_COSTLY_ORDER. This inconsistency is not so clean in my view.

Also for GFP_KERNEL/USER/__GFP_FS regular allocations the __GFP_NOFAIL looks more like a __GFP_MAY_OOM. But if we fix that and we drop __GFP_NORETRY, and we set __GFP_NOFAIL within the GFP_NOFS/NOIO #defines (to remove the magic PAGE_ALLOC_COSTLY_ORDER check in should_alloc_retry) we may loop forever if somebody allocates several mbytes of huge contiguous RAM with GFP_NOIO. So at least there's a practical explanation for the current code.

Patch looks good to me (and safer) even if I don't like keeping infinite loops from a purely theoretical standpoint.

From a more practical point of view, I am generally more concerned

This patch seems to have gotten lost in the cracks and the discussion on alternatives that started here https://lkml.org/lkml/2011/10/25/24
petered out without any alternative patches being posted. Lacking a viable alternative patch, I'm reposting this patch because AFAIK, this bug still exists.

Colin Cross reported;

Under the following conditions, __alloc_pages_slowpath can loop forever:
gfp_mask & __GFP_WAIT is true
gfp_mask & __GFP_FS is false
reclaim and compaction make no progress
order <= PAGE_ALLOC_COSTLY_ORDER

These conditions happen very often during suspend and resume, when pm_restrict_gfp_mask() effectively converts all GFP_KERNEL allocations into __GFP_WAIT.

The oom killer is not run because gfp_mask & __GFP_FS is false, but should_alloc_retry will always return true when order is less than PAGE_ALLOC_COSTLY_ORDER.

In his fix, he avoided retrying the allocation if reclaim made no progress and __GFP_FS was not set. The problem is that this would result in GFP_NOIO allocations failing that previously succeeded which would be very unfortunate.

The big difference between GFP_NOIO and suspend converting GFP_KERNEL to behave like GFP_NOIO is that normally flushers will be cleaning pages and kswapd reclaims pages allowing GFP_NOIO to succeed after a short delay. The same does not necessarily apply during suspend as the storage device may be suspended. Hence, this patch special cases the suspend case to fail the page allocation if reclaim cannot make progress. This might cause suspend to abort but that is better than a livelock.

mm/page_alloc.c +0 -0

index 9dd443d..5402897 100644

mm/page_alloc.c +6 -0

+#else
+
+static bool pm_suspending(void)
+{
+	return false;
+}
 #endif /* CONFIG_PM_SLEEP */
 
 #ifdef CONFIG_HUGETLB_PAGE_SIZE_VARIABLE

@@ -2214,6 +2228,14 @@ rebalance:
 

Quoted 12 lines

<SNIP>

mm/page_alloc.c +3 -0

index 9dd443d..5402897 100644
@@ -127,6 +127,20 @@ void pm_restrict_gfp_mask(void)
 	saved_gfp_mask = gfp_allowed_mask;
 	gfp_allowed_mask &= ~GFP_IOFS;
 }
+
+static bool pm_suspending(void)
+{

mm/page_alloc.c void pm_restrict_gfp_mask(void) · line 136

Quoted 4 lines

mm/page_alloc.c +4 -0

+	if ((gfp_allowed_mask & GFP_IOFS) == GFP_IOFS)
+		return false;
+	return true;
+}

This doesn't seem a terribly reliable way of detecting that PM has disabled the storage devices (which is what we really want to know
here: kswapd got crippled).

I guess it's safe for now, because PM is the only caller who alters gfp_allowed_mask (I assume).

But an explicit storage_is_unavaliable
global which is set and cleared at exactly the correct time is clearer, more direct and future-safer, no?

Quoted 11 lines

mm/page_alloc.c +6 -0

+#else
+
+static bool pm_suspending(void)
+{
+	return false;
+}
 #endif /* CONFIG_PM_SLEEP */
 
 #ifdef CONFIG_HUGETLB_PAGE_SIZE_VARIABLE

@@ -2214,6 +2228,14 @@ rebalance:
 

mm/page_alloc.c rebalance: · line 2238

Quoted 10 lines

mm/page_alloc.c +8 -0

 			goto restart;
 		}
+
+		/*
+		 * Suspend converts GFP_KERNEL to __GFP_WAIT which can
+		 * prevent reclaim making forward progress without
+		 * invoking OOM. Bail if we are suspending
+		 */
+		if (pm_suspending())
+			goto nopage;

The comment doesn't tell the whole story: it's important that kswapd writeout was disabled?

<SNIP>
It feels overkill to allocate more global storage for it when gfp_allowed_mask is already there but I could rename pm_suspending() to pm_disabled_storage(), make try_to_free_swap() use the same helper but leave the implementation the same. This would clarify the situation.

This patch seems to have gotten lost in the cracks and the discussion on alternatives that started here https://lkml.org/lkml/2011/10/25/24
petered out without any alternative patches being posted. Lacking a viable alternative patch, I'm reposting this patch because AFAIK, this bug still exists.

Colin Cross reported;

 Under the following conditions, __alloc_pages_slowpath can loop forever:
 gfp_mask & __GFP_WAIT is true
 gfp_mask & __GFP_FS is false
 reclaim and compaction make no progress
 order <= PAGE_ALLOC_COSTLY_ORDER

 These conditions happen very often during suspend and resume, when pm_restrict_gfp_mask() effectively converts all GFP_KERNEL allocations into __GFP_WAIT.

 The oom killer is not run because gfp_mask & __GFP_FS is false, but should_alloc_retry will always return true when order is less than PAGE_ALLOC_COSTLY_ORDER.

In his fix, he avoided retrying the allocation if reclaim made no progress and __GFP_FS was not set. The problem is that this would result in GFP_NOIO allocations failing that previously succeeded which would be very unfortunate.

The big difference between GFP_NOIO and suspend converting GFP_KERNEL to behave like GFP_NOIO is that normally flushers will be cleaning pages and kswapd reclaims pages allowing GFP_NOIO to succeed after a short delay. The same does not necessarily apply during suspend as the storage device may be suspended.  Hence, this patch special cases the suspend case to fail the page allocation if reclaim cannot make progress. This might cause suspend to abort but that is better than a livelock.

[[email protected]: Rework fix to be suspend specific]

Reported-and-tested-by Colin Cross <[email protected]>

Signed-off-by Mel Gorman <[email protected]>

---
 mm/page_alloc.c |   22 ++++++++++++++++++++++

 1 files changed, 22 insertions(+), 0 deletions(-)

mm/page_alloc.c +15 -0

index 9dd443d..5402897 100644
@@ -127,6 +127,20 @@ void pm_restrict_gfp_mask(void)
        saved_gfp_mask = gfp_allowed_mask;
        gfp_allowed_mask &= ~GFP_IOFS;
  }
+
+static bool pm_suspending(void)
+{
+       if ((gfp_allowed_mask & GFP_IOFS) == GFP_IOFS)
+               return false;
+       return true;
+}
+
+#else
+
+static bool pm_suspending(void)
+{
+       return false;
+}
  #endif /* CONFIG_PM_SLEEP */
 
  #ifdef CONFIG_HUGETLB_PAGE_SIZE_VARIABLE

@@ -2214,6 +2228,14 @@ rebalance:
 
                        goto restart;
                }
+

Quoted 82 lines · Mel Gorman

This patch seems to have gotten lost in the cracks and the discussion on alternatives that started here https://lkml.org/lkml/2011/10/25/24
petered out without any alternative patches being posted. Lacking a viable alternative patch, I'm reposting this patch because AFAIK, this bug still exists.

Colin Cross reported;

Under the following conditions, __alloc_pages_slowpath can loop forever:
gfp_mask & __GFP_WAIT is true
gfp_mask & __GFP_FS is false
reclaim and compaction make no progress
order <= PAGE_ALLOC_COSTLY_ORDER

These conditions happen very often during suspend and resume, when pm_restrict_gfp_mask() effectively converts all GFP_KERNEL allocations into __GFP_WAIT.

The oom killer is not run because gfp_mask & __GFP_FS is false, but should_alloc_retry will always return true when order is less than PAGE_ALLOC_COSTLY_ORDER.

In his fix, he avoided retrying the allocation if reclaim made no progress and __GFP_FS was not set. The problem is that this would result in GFP_NOIO allocations failing that previously succeeded which would be very unfortunate.

The big difference between GFP_NOIO and suspend converting GFP_KERNEL to behave like GFP_NOIO is that normally flushers will be cleaning pages and kswapd reclaims pages allowing GFP_NOIO to succeed after a short delay. The same does not necessarily apply during suspend as the storage device may be suspended. Hence, this patch special cases the suspend case to fail the page allocation if reclaim cannot make progress. This might cause suspend to abort but that is better than a livelock.

[[email protected]: Rework fix to be suspend specific]

Reported-and-tested-by Colin Cross <[email protected]>

Signed-off-by Mel Gorman <[email protected]>

---
 mm/page_alloc.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

mm/page_alloc.c +22 -0

index 9dd443d..5402897 100644
@@ -127,6 +127,20 @@ void pm_restrict_gfp_mask(void)
       saved_gfp_mask = gfp_allowed_mask;
       gfp_allowed_mask &= ~GFP_IOFS;
 }
+
+static bool pm_suspending(void)
+{
+       if ((gfp_allowed_mask & GFP_IOFS) == GFP_IOFS)
+               return false;
+       return true;
+}
+
+#else
+
+static bool pm_suspending(void)
+{
+       return false;
+}
 #endif /* CONFIG_PM_SLEEP */
 
 #ifdef CONFIG_HUGETLB_PAGE_SIZE_VARIABLE

@@ -2214,6 +2228,14 @@ rebalance:
 
                       goto restart;
               }
+
+               /*
+                * Suspend converts GFP_KERNEL to __GFP_WAIT which can
+                * prevent reclaim making forward progress without
+                * invoking OOM. Bail if we are suspending
+                */
+               if (pm_suspending())
+                       goto nopage;
       }
 

mm/page_alloc.c rebalance: · line 2241

Quoted 2 lines

mm/page_alloc.c +0 -0

       /* Check if we should retry the allocation */

I don't have much time to look into this problem so I miss some things. But the feeling I have a mind when I faced this problem is why we should make another special case handling function.
Already we have such thing for hibernation - oom_killer_disabled in vm Could we use it instead of making new branch for very special case?

Maybe It would be better to rename oom_killer_disabled with pm_is_going or something.

Quoted 83 lines · Minchan Kim

Quoted 82 lines · Mel Gorman

This patch seems to have gotten lost in the cracks and the discussion on alternatives that started here https://lkml.org/lkml/2011/10/25/24
petered out without any alternative patches being posted. Lacking a viable alternative patch, I'm reposting this patch because AFAIK, this bug still exists.

Colin Cross reported;

 Under the following conditions, __alloc_pages_slowpath can loop forever:
 gfp_mask & __GFP_WAIT is true
 gfp_mask & __GFP_FS is false
 reclaim and compaction make no progress
 order <= PAGE_ALLOC_COSTLY_ORDER

 These conditions happen very often during suspend and resume, when pm_restrict_gfp_mask() effectively converts all GFP_KERNEL allocations into __GFP_WAIT.

 The oom killer is not run because gfp_mask & __GFP_FS is false, but should_alloc_retry will always return true when order is less than PAGE_ALLOC_COSTLY_ORDER.

In his fix, he avoided retrying the allocation if reclaim made no progress and __GFP_FS was not set. The problem is that this would result in GFP_NOIO allocations failing that previously succeeded which would be very unfortunate.

The big difference between GFP_NOIO and suspend converting GFP_KERNEL to behave like GFP_NOIO is that normally flushers will be cleaning pages and kswapd reclaims pages allowing GFP_NOIO to succeed after a short delay. The same does not necessarily apply during suspend as the storage device may be suspended.  Hence, this patch special cases the suspend case to fail the page allocation if reclaim cannot make progress. This might cause suspend to abort but that is better than a livelock.

[[email protected]: Rework fix to be suspend specific]

Reported-and-tested-by Colin Cross <[email protected]>

Signed-off-by Mel Gorman <[email protected]>

---
 mm/page_alloc.c |   22 ++++++++++++++++++++++

 1 files changed, 22 insertions(+), 0 deletions(-)

mm/page_alloc.c +22 -0

index 9dd443d..5402897 100644
@@ -127,6 +127,20 @@ void pm_restrict_gfp_mask(void)
        saved_gfp_mask = gfp_allowed_mask;
        gfp_allowed_mask &= ~GFP_IOFS;
  }
+
+static bool pm_suspending(void)
+{
+       if ((gfp_allowed_mask & GFP_IOFS) == GFP_IOFS)
+               return false;
+       return true;
+}
+
+#else
+
+static bool pm_suspending(void)
+{
+       return false;
+}
  #endif /* CONFIG_PM_SLEEP */
 
  #ifdef CONFIG_HUGETLB_PAGE_SIZE_VARIABLE

@@ -2214,6 +2228,14 @@ rebalance:
 
                        goto restart;
                }
+
+               /*
+                * Suspend converts GFP_KERNEL to __GFP_WAIT which can
+                * prevent reclaim making forward progress without
+                * invoking OOM. Bail if we are suspending
+                */
+               if (pm_suspending())
+                       goto nopage;
        }
 

Quoted 8 lines

mm/page_alloc.c rebalance: · line 2241

Quoted 2 lines

mm/page_alloc.c +0 -0

        /* Check if we should retry the allocation */

I don't have much time to look into this problem so I miss some things. But the feeling I have a mind when I faced this problem is why we should make another special case handling function.
Already we have such thing for hibernation - oom_killer_disabled in vm Could we use it instead of making new branch for very special case?

Fair question!

Suspend is a multi-stage process and the OOM killer is disabled at a different time to the GFP flags being restricted. This is another reason why renaming to pm_suspending to pm_suspended_storage is a good idea (pm_suspending is misleading at best).

I am vague on all the steps hibernation takes but initially processes are frozen and if they are successfully frozen then the OOM killer is disabled. At this point, storage is still active so the GFP allowed mask is the same. When preparing to write the image, kernel threads are suspended so there is no new IO being initiated and then the GFP mask is restricted to prevent any memory allocation trying to write pages to storage. It then writes the image to disk.

So what we have now is

        if (!did_some_progress) {
                if ((gfp_mask & __GFP_FS) && !(gfp_mask & __GFP_NORETRY)) {
                        if (oom_killer_disabled)
                                goto nopage;

Lets say we changed that to

        if (!did_some_progress) {
                if (oom_killer_disabled)
                        goto nopage;
                if ((gfp_mask & __GFP_FS) && !(gfp_mask & __GFP_NORETRY)) {

The impact would be that during the time between processes been frozen and storage being suspended, GFP_NOIO allocations that used to call wait_iff_congested and retry while kswapd does its thing will return failure instead. These GFP_NOIO allocations that used to succeed will now fail in rare cases during suspend and I don't think we want that.

Is this what you meant or had you something else in mind?

Quoted 9 lines · Mel Gorman

On Wed, Nov 16, 2011 at 01:13:30AM +0900, Minchan Kim wrote:
The impact would be that during the time between processes been frozen and storage being suspended, GFP_NOIO allocations that used to call wait_iff_congested and retry while kswapd does its thing will return failure instead. These GFP_NOIO allocations that used to succeed will now fail in rare cases during suspend and I don't think we want that.

Is this what you meant or had you something else in mind?

You read my mind exactly!

I thought hibernation process is as follows,

freeze user processes
oom_disable
hibernate_preallocate_memory
freeze kernel processes(include kswapd)
pm_restrict_gfp_mask
swsusp_save

My guessing is hibernate_prealocate_memory should reserve all memory needed for hibernation for reclaimaing pages of kswapd because kswapd just would be stopped so during swsusp_save, page reclaim should not be occured.

But being see description of patch, my guess seems wrong.
Now the problem happens and it means page reclaim happens during swsusp_save. Colin or someone could confirm this?

If so, could we reserve more memory when we preallocate hibernation memory for avoiding page reclaim without kswapd?

--
Kind regards,
Minchan Kim

Quoted 22 lines · Minchan Kim

Quoted 9 lines · Mel Gorman

On Wed, Nov 16, 2011 at 01:13:30AM +0900, Minchan Kim wrote:
The impact would be that during the time between processes been frozen and storage being suspended, GFP_NOIO allocations that used to call wait_iff_congested and retry while kswapd does its thing will return failure instead. These GFP_NOIO allocations that used to succeed will now fail in rare cases during suspend and I don't think we want that.

Is this what you meant or had you something else in mind?

You read my mind exactly!

I thought hibernation process is as follows,

freeze user processes
oom_disable
hibernate_preallocate_memory
freeze kernel processes(include kswapd)
pm_restrict_gfp_mask
swsusp_save

Quoted 7 lines

My guessing is hibernate_prealocate_memory should reserve all memory needed for hibernation for reclaimaing pages of kswapd because kswapd just would be stopped so during swsusp_save, page reclaim should not be occured.

But being see description of patch, my guess seems wrong.
Now the problem happens and it means page reclaim happens during swsusp_save. Colin or someone could confirm this?

The problem I see is during suspend, not hibernation. The particular allocation that usually causes the problem is the pgd_alloc for page tables when re-enabling the 2nd cpu during resume, which is odd as those same page tables were freed during suspend. I guess an unfreezable kernel thread allocated that memory between the free and re-allocation.

Then, How about this?

[barrios@barrios-laptop linux-2.6]$ git diff

kernel/power/suspend.c +2 -0

index fdd4263..01aa9b5 100644
@@ -297,9 +297,11 @@ int enter_state(suspend_state_t state)
               goto Finish;
 
       pr_debug("PM: Entering %s sleep\n", pm_states[state]);
+       oom_killer_disable();
       pm_restrict_gfp_mask();
       error = suspend_devices_and_enter(state);
       pm_restore_gfp_mask();
+       oom_killer_enable();
 
 Finish:
       pr_debug("PM: Finishing wakeup.\n");

mm/page_alloc.c +0 -0

index 6e8ecb6..d8c31b7 100644
@@ -2177,9 +2177,9 @@ rebalance:
        * running out of options and have to consider going OOM

Quoted 14 lines

kernel/power/suspend.c +2 -0

index fdd4263..01aa9b5 100644
@@ -297,9 +297,11 @@ int enter_state(suspend_state_t state)
               goto Finish;
 
       pr_debug("PM: Entering %s sleep\n", pm_states[state]);
+       oom_killer_disable();
       pm_restrict_gfp_mask();
       error = suspend_devices_and_enter(state);
       pm_restore_gfp_mask();
+       oom_killer_enable();
 

Quoted 7 lines

kernel/power/suspend.c +0 -0

 Finish:
       pr_debug("PM: Finishing wakeup.\n");

mm/page_alloc.c +0 -0

index 6e8ecb6..d8c31b7 100644

mm/page_alloc.c rebalance: · line 2181

Quoted 7 lines

mm/page_alloc.c +1 -2

@@ -2177,9 +2177,9 @@ rebalance:
        * running out of options and have to consider going OOM
        */
       if (!did_some_progress) {
-               if ((gfp_mask & __GFP_FS) && !(gfp_mask & __GFP_NORETRY)) {
-                       if (oom_killer_disabled)
+               if (oom_killer_disabled)
                               goto nopage;

mm/page_alloc.c rebalance: · line 2185

Quoted 5 lines

mm/page_alloc.c +1 -0

+               if ((gfp_mask & __GFP_FS) && !(gfp_mask & __GFP_NORETRY)) {
                       page = __alloc_pages_may_oom(gfp_mask, order,
                                       zonelist, high_zoneidx,
                                       nodemask, preferred_zone,

I'd prefer something like this. The whole 'gfp_allowed_flags' thing was designed to make GFP_KERNEL work during boot time where it's obviously safe to do that. I really don't think that's going to work suspend cleanly.

Quoted 14 lines · Pekka Enberg

Quoted 14 lines

kernel/power/suspend.c +2 -0

index fdd4263..01aa9b5 100644
@@ -297,9 +297,11 @@ int enter_state(suspend_state_t state)
               goto Finish;
 
       pr_debug("PM: Entering %s sleep\n", pm_states[state]);
+       oom_killer_disable();
       pm_restrict_gfp_mask();
       error = suspend_devices_and_enter(state);
       pm_restore_gfp_mask();
+       oom_killer_enable();
 

Quoted 14 lines

Quoted 7 lines

kernel/power/suspend.c +0 -0

 Finish:
       pr_debug("PM: Finishing wakeup.\n");

mm/page_alloc.c +0 -0

index 6e8ecb6..d8c31b7 100644

mm/page_alloc.c rebalance: · line 2181

Quoted 7 lines

mm/page_alloc.c +1 -2

@@ -2177,9 +2177,9 @@ rebalance:
        * running out of options and have to consider going OOM
        */
       if (!did_some_progress) {
-               if ((gfp_mask & __GFP_FS) && !(gfp_mask & __GFP_NORETRY)) {
-                       if (oom_killer_disabled)
+               if (oom_killer_disabled)
                               goto nopage;

You're allowing __GFP_NOFAIL allocations to fail.

Quoted 10 lines

mm/page_alloc.c rebalance: · line 2185

Quoted 5 lines

mm/page_alloc.c +1 -0

+               if ((gfp_mask & __GFP_FS) && !(gfp_mask & __GFP_NORETRY)) {
                       page = __alloc_pages_may_oom(gfp_mask, order,
                                       zonelist, high_zoneidx,
                                       nodemask, preferred_zone,

I'd prefer something like this. The whole 'gfp_allowed_flags' thing was designed to make GFP_KERNEL work during boot time where it's obviously safe to do that. I really don't think that's going to work suspend cleanly.

Adding Rafael to the cc.

This has been done since 2.6.34 and presumably has been working quite well.

I don't have a specific objection to gfp_allowed_flags to be used outside of boot since it seems plausible that there are system-level contexts that would need different behavior in the page allocator and this does it effectively without major surgery or a slower fastpath. Suspend is using it just like boot does before irqs are enabled, so I don't have an objection to it.

Quoted 14 lines · Pekka Enberg

Quoted 14 lines

kernel/power/suspend.c +2 -0

index fdd4263..01aa9b5 100644
@@ -297,9 +297,11 @@ int enter_state(suspend_state_t state)
                goto Finish;
 
        pr_debug("PM: Entering %s sleep\n", pm_states[state]);
+       oom_killer_disable();
        pm_restrict_gfp_mask();
        error = suspend_devices_and_enter(state);
        pm_restore_gfp_mask();
+       oom_killer_enable();
 

Quoted 14 lines

Quoted 7 lines

 Finish:
       pr_debug("PM: Finishing wakeup.\n");

mm/page_alloc.c +0 -0

index 6e8ecb6..d8c31b7 100644

mm/page_alloc.c rebalance: · line 2181

Quoted 7 lines

mm/page_alloc.c +1 -2

@@ -2177,9 +2177,9 @@ rebalance:
         * running out of options and have to consider going OOM
         */
        if (!did_some_progress) {
-               if ((gfp_mask & __GFP_FS) && !(gfp_mask & __GFP_NORETRY)) {
-                       if (oom_killer_disabled)
+               if (oom_killer_disabled)
                                goto nopage;

You're allowing __GFP_NOFAIL allocations to fail.

Quoted 10 lines

mm/page_alloc.c rebalance: · line 2185

Quoted 5 lines

mm/page_alloc.c +1 -0

+               if ((gfp_mask & __GFP_FS) && !(gfp_mask & __GFP_NORETRY)) {
                        page = __alloc_pages_may_oom(gfp_mask, order,
                                        zonelist, high_zoneidx,
                                        nodemask, preferred_zone,

I'd prefer something like this. The whole 'gfp_allowed_flags' thing was designed to make GFP_KERNEL work during boot time where it's obviously safe to do that. I really don't think that's going to work suspend cleanly.

Adding Rafael to the cc.

This has been done since 2.6.34 and presumably has been working quite well.  I don't have a specific objection to gfp_allowed_flags to be used outside of boot since it seems plausible that there are system-level contexts that would need different behavior in the page allocator and this does it effectively without major surgery or a slower fastpath.  Suspend is using it just like boot does before irqs are enabled, so I don't have an objection to it.

Quoted 44 lines

Quoted 14 lines

Quoted 14 lines

kernel/power/suspend.c +2 -0

index fdd4263..01aa9b5 100644
@@ -297,9 +297,11 @@ int enter_state(suspend_state_t state)
 A  A  A  A  A  A  A  A goto Finish;
 
 A  A  A  A pr_debug("PM: Entering %s sleep\n", pm_states[state]);
+ A  A  A  oom_killer_disable();
 A  A  A  A pm_restrict_gfp_mask();
 A  A  A  A error = suspend_devices_and_enter(state);
 A  A  A  A pm_restore_gfp_mask();
+ A  A  A  oom_killer_enable();
 

Quoted 14 lines

Quoted 13 lines

A Finish:
A A A A pr_debug("PM: Finishing wakeup.\n");

mm/page_alloc.c +0 -0

index 6e8ecb6..d8c31b7 100644
@@ -2177,9 +2177,9 @@ rebalance:

A  A  A  A  * running out of options and have to consider going OOM
A  A  A  A  */
A  A  A  A if (!did_some_progress) {
- A  A  A  A  A  A  A  if ((gfp_mask & __GFP_FS) && !(gfp_mask & __GFP_NORETRY)) {
- A  A  A  A  A  A  A  A  A  A  A  if (oom_killer_disabled)
+ A  A  A  A  A  A  A  if (oom_killer_disabled)

Quoted 1 line

A A A A A A A A A A A A A A A A goto nopage;

You're allowing __GFP_NOFAIL allocations to fail.

Quoted 10 lines

Quoted 5 lines

+ A A A A A A A if ((gfp_mask & __GFP_FS) && !(gfp_mask & __GFP_NORETRY)) {

A  A  A  A  A  A  A  A  A  A  A  A page = __alloc_pages_may_oom(gfp_mask, order,
A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A zonelist, high_zoneidx,
A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A nodemask, preferred_zone,

I'd prefer something like this. The whole 'gfp_allowed_flags' thing was designed to make GFP_KERNEL work during boot time where it's obviously safe to do that. I really don't think that's going to work suspend cleanly.

Adding Rafael to the cc.

Quoted 8 lines

This has been done since 2.6.34 and presumably has been working quite well. A I don't have a specific objection to gfp_allowed_flags to be used outside of boot since it seems plausible that there are system-level contexts that would need different behavior in the page allocator and this does it effectively without major surgery or a slower fastpath. A Suspend is using it just like boot does before irqs are enabled, so I don't have an objection to it.

My point isn't using gfp_allowed_flags(maybe it's Pekka's concern) but why adding new special case handling code like pm_suspended_storage. I think we can handle the issue with oom_killer_disabled(but the naming is bad)

mm/page_alloc.c +15 -0

index 9dd443d..5402897 100644
@@ -127,6 +127,20 @@ void pm_restrict_gfp_mask(void)
 	saved_gfp_mask = gfp_allowed_mask;
 	gfp_allowed_mask &= ~GFP_IOFS;
 }
+
+static bool pm_suspending(void)
+{
+	if ((gfp_allowed_mask & GFP_IOFS) == GFP_IOFS)
+		return false;
+	return true;
+}
+
+#else
+
+static bool pm_suspending(void)
+{
+	return false;
+}
 #endif /* CONFIG_PM_SLEEP */
 
 #ifdef CONFIG_HUGETLB_PAGE_SIZE_VARIABLE

@@ -2214,6 +2228,14 @@ rebalance:
 
 			goto restart;
 		}
+

Quoted 15 lines

<SNIP>

(quoted diff) +8 -0

@@ -2214,6 +2228,14 @@ rebalance:
 
 			goto restart;
 		}
+
+		/*
+		 * Suspend converts GFP_KERNEL to __GFP_WAIT which can
+		 * prevent reclaim making forward progress without
+		 * invoking OOM. Bail if we are suspending
+		 */
+		if (pm_suspending())
+			goto nopage;
 	}
 

Review rebalance: · line 2241

Quoted 1 line

(quoted diff) +0 -0

 	/* Check if we should retry the allocation */

This allows all __GFP_NOFAIL allocations to fail while pm_restrict_gfp_mask() is in effect, so I disagree with this unless it is moved into the should_alloc_retry() logic. If you pass did_some_progress into that function and then moved the check for __GFP_NOFAIL right under the check for __GFP_NORETRY and checked for pm_suspending() there (and before the check for PAGE_ALLOC_COSTLY_ORDER) then it would allow the infinite loop for __GFP_NOFAIL which is required if __GFP_WAIT.

Good point. I agree that it would be more consistent although there is still the risk of infinite looping with __GFP_NOFAIL if storage devices are disabled.

Colin reported elsewhere in this thread that "the particular allocation that usually causes the problem is the pgd_alloc for page tables when re-enabling the 2nd cpu during resume". On X86, those allocations are using the flags

GFP_KERNEL | __GFP_NOTRACK | __GFP_REPEAT | __GFP_ZERO

so they should not be trapped in an infinite loop due to __GFP_NOFAIL. On ARM, they use GFP_KERNEL so should also be ok.

David, is this what you meant? This patch includes all the documentation-related updates that were discussed in this thread as well as updated the check in mm/swapfile.c for hibernation.

==== CUT HERE ====
mm: avoid livelock on !__GFP_FS allocations v2

Changelog since V1
o Move PM check to should_alloc_retry (David Rientjes)
o Add some additional documentation

Colin Cross reported;

Under the following conditions, __alloc_pages_slowpath can loop forever:
gfp_mask & __GFP_WAIT is true
gfp_mask & __GFP_FS is false
reclaim and compaction make no progress
order <= PAGE_ALLOC_COSTLY_ORDER

These conditions happen very often during suspend and resume, when pm_restrict_gfp_mask() effectively converts all GFP_KERNEL allocations into __GFP_WAIT.

The oom killer is not run because gfp_mask & __GFP_FS is false, but should_alloc_retry will always return true when order is less than PAGE_ALLOC_COSTLY_ORDER.

In his fix, he avoided retrying the allocation if reclaim made no progress and __GFP_FS was not set. The problem is that this would result in GFP_NOIO allocations failing that previously succeeded which would be very unfortunate.

The big difference between GFP_NOIO and suspend converting GFP_KERNEL to behave like GFP_NOIO is that normally flushers will be cleaning pages and kswapd reclaims pages allowing GFP_NOIO to succeed after a short delay. The same does not necessarily apply during suspend as the storage device may be suspended.

This patch special cases the suspend case to fail the page allocation if reclaim cannot make progress and adds some documentation on how gfp_allowed_mask is currently used. Failing allocations like this may cause suspend to abort but that is better than a livelock.

[[email protected]: Rework fix to be suspend specific]

[[email protected]: Move suspended device check to should_alloc_retry]

Reported-by Colin Cross <[email protected]>

Signed-off-by Mel Gorman <[email protected]>