[BUGFIX][PATCH] vmscan: don't use return value trick when oom_killer_disabled

[BUGFIX][PATCH] vmscan: don't use return value trick when oom_killer_disabled

[KOSAKI Motohiro]

M. Vefa Bicakci reported 2.6.35 kernel hang up when hibernation on his
32bit 3GB mem machine. (https://bugzilla.kernel.org/show_bug.cgi?id=16771)
Also he was bisected first bad commit is below

  commit bb21c7ce18eff8e6e7877ca1d06c6db719376e3c
  Author: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
  Date:   Fri Jun 4 14:15:05 2010 -0700

     vmscan: fix do_try_to_free_pages() return value when priority==0 reclaim failure

At first impression, this seemed very strange because the above commit only
chenged function return value and hibernate_preallocate_memory() ignore
return value of shrink_all_memory(). But it's related.

Now, page allocation from hibernation code may enter infinite loop if
the system has highmem.

The reasons are two. 1) hibernate_preallocate_memory() call
alloc_pages() wrong order 2) vmscan don't care enough OOM case when
oom_killer_disabled.

This patch only fix (2). Why is oom_killer_disabled so special?
because when hibernation case, zone->all_unreclaimable never be turned on.
hibernation freeze all tasks at first, then kswapd can't works in this
case, and zone->all_unreclaimable is only turned from kswapd.

Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: M. Vefa Bicakci <bicave@superonline.com>
Cc: stable@kernel.org
Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
---
 mm/vmscan.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index c391c32..1919d8a 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -40,6 +40,7 @@
 #include <linux/memcontrol.h>
 #include <linux/delayacct.h>
 #include <linux/sysctl.h>
+#include <linux/oom.h>
 
 #include <asm/tlbflush.h>
 #include <asm/div64.h>
@@ -1931,7 +1932,7 @@ out:
 		return sc->nr_reclaimed;
 
 	/* top priority shrink_zones still had more to do? don't OOM, then */
-	if (scanning_global_lru(sc) && !all_unreclaimable)
+	if (scanning_global_lru(sc) && !all_unreclaimable && !oom_killer_disabled)
 		return 1;
 
 	return 0;
-- 
1.6.5.2



--
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: [BUGFIX][PATCH] vmscan: don't use return value trick when oom_killer_disabled

[Minchan Kim]

Hi KOSAKI,

On Wed, Sep 1, 2010 at 9:31 AM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
> M. Vefa Bicakci reported 2.6.35 kernel hang up when hibernation on his
> 32bit 3GB mem machine. (https://bugzilla.kernel.org/show_bug.cgi?id=16771)
> Also he was bisected first bad commit is below
>
>  commit bb21c7ce18eff8e6e7877ca1d06c6db719376e3c
>  Author: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>  Date:   Fri Jun 4 14:15:05 2010 -0700
>
>     vmscan: fix do_try_to_free_pages() return value when priority==0 reclaim failure
>
> At first impression, this seemed very strange because the above commit only
> chenged function return value and hibernate_preallocate_memory() ignore
> return value of shrink_all_memory(). But it's related.
>
> Now, page allocation from hibernation code may enter infinite loop if
> the system has highmem.
>
> The reasons are two. 1) hibernate_preallocate_memory() call
> alloc_pages() wrong order 2) vmscan don't care enough OOM case when
> oom_killer_disabled.
>
> This patch only fix (2). Why is oom_killer_disabled so special?
> because when hibernation case, zone->all_unreclaimable never be turned on

Re: [BUGFIX][PATCH] vmscan: don't use return value trick when oom_killer_disabled

[KOSAKI Motohiro]

Hi

Thank you for good commenting!


> I don't like use oom_killer_disabled directly.
> That's because we have wrapper inline functions to handle the
> variable(ex, oom_killer_[disable/enable]).
> It means we are reluctant to use the global variable directly.
> So should we make new function as is_oom_killer_disable?
> 
> I think NO.
> 
> As I read your description, this problem is related to only hibernation.
> Since hibernation freezes all processes(include kswapd), this problem
> happens. Of course, now oom_killer_disabled is used by only
> hibernation. But it can be used others in future(Off-topic : I don't
> want it). Others can use it without freezing processes. Then kswapd
> can set zone->all_unreclaimable and the problem can't happen.
> 
> So I want to use sc->hibernation_mode which is already used
> do_try_to_free_pages instead of oom_killer_disabled.

Unfortunatelly, It's impossible. shrink_all_memory() turn on
sc->hibernation_mode. but other hibernation caller merely call
alloc_pages(). so we don't have any hint.



--
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: [BUGFIX][PATCH] vmscan: don't use return value trick when oom_killer_disabled

[Minchan Kim]

On Wed, Sep 1, 2010 at 10:55 AM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
> Hi
>
> Thank you for good commenting!
>
>
>> I don't like use oom_killer_disabled directly.
>> That's because we have wrapper inline functions to handle the
>> variable(ex, oom_killer_[disable/enable]).
>> It means we are reluctant to use the global variable directly.
>> So should we make new function as is_oom_killer_disable?
>>
>> I think NO.
>>
>> As I read your description, this problem is related to only hibernation.
>> Since hibernation freezes all processes(include kswapd), this problem
>> happens. Of course, now oom_killer_disabled is used by only
>> hibernation. But it can be used others in future(Off-topic : I don't
>> want it). Others can use it without freezing processes. Then kswapd
>> can set zone->all_unreclaimable and the problem can't happen.
>>
>> So I want to use sc->hibernation_mode which is already used
>> do_try_to_free_pages instead of oom_killer_disabled.
>
> Unfortunatelly, It's impossible. shrink_all_memory() turn on
> sc->hibernation_mode. but other hibernation caller merely call
> alloc_pages(). so we don't have any hint.
>
Ahh.. True. Sorry for that.
I will think some better method.
if I can't find it, I don't mind this patch. :)

Thanks.


-- 
Kind regards,
Minchan Kim

--
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: [BUGFIX][PATCH] vmscan: don't use return value trick when oom_killer_disabled

[Minchan Kim]

On Wed, Sep 01, 2010 at 11:01:43AM +0900, Minchan Kim wrote:
> On Wed, Sep 1, 2010 at 10:55 AM, KOSAKI Motohiro
> <kosaki.motohiro@jp.fujitsu.com> wrote:
> > Hi
> >
> > Thank you for good commenting!
> >
> >
> >> I don't like use oom_killer_disabled directly.
> >> That's because we have wrapper inline functions to handle the
> >> variable(ex, oom_killer_[disable/enable]).
> >> It means we are reluctant to use the global variable directly.
> >> So should we make new function as is_oom_killer_disable?
> >>
> >> I think NO.
> >>
> >> As I read your description, this problem is related to only hibernation.
> >> Since hibernation freezes all processes(include kswapd), this problem
> >> happens. Of course, now oom_killer_disabled is used by only
> >> hibernation. But it can be used others in future(Off-topic : I don't
> >> want it). Others can use it without freezing processes. Then kswapd
> >> can set zone->all_unreclaimable and the problem can't happen.
> >>
> >> So I want to use sc->hibernation_mode which is already used
> >> do_try_to_free_pages instead of oom_killer_disabled.
> >
> > Unfortunatelly, It's impossible. shrink_all_memory() turn on
> > sc->hibernation_mode. but other hibernation caller merely call
> > alloc_pages(). so we don't have any hint.
> >
> Ahh.. True. Sorry for that.
> I will think some better method.
> if I can't find it, I don't mind this patch. :)

It seems that the poblem happens following as. 
(I might miss something since I just read theyour description)

hibernation
oom_disable
alloc_pages
do_try_to_free_pages
        if (scanning_global_lru(sc) && !all_unreclaimable)
                return 1;
If kswapd is not freezed, it would set zone->all_unreclaimable to 1 and then
shrink_zones maybe return true. so alloc_pages could go to _nopage_.
If it is, it's no problem. 
Right?

I think the problem would come from shrink_zones. 
It set false to all_unreclaimable blindly even though shrink_zone can't reclaim
any page. It doesn't make sense. 
How about this?
I think we need this regardless of the problem.
What do you think about?

diff --git a/mm/vmscan.c b/mm/vmscan.c
index d8fd87d..22017b3 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1901,7 +1901,8 @@ static bool shrink_zones(int priority, struct zonelist *zonelist,
                }
 
                shrink_zone(priority, zone, sc);
-               all_unreclaimable = false;
+               if (sc->nr_reclaimed)
+                       all_unreclaimable = false;
        }
        return all_unreclaimable;
 }

-- 
Kind regards,
Minchan Kim

--
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: [BUGFIX][PATCH] vmscan: don't use return value trick when oom_killer_disabled

[KOSAKI Motohiro]

> On Wed, Sep 01, 2010 at 11:01:43AM +0900, Minchan Kim wrote:
> > On Wed, Sep 1, 2010 at 10:55 AM, KOSAKI Motohiro
> > <kosaki.motohiro@jp.fujitsu.com> wrote:
> > > Hi
> > >
> > > Thank you for good commenting!
> > >
> > >
> > >> I don't like use oom_killer_disabled directly.
> > >> That's because we have wrapper inline functions to handle the
> > >> variable(ex, oom_killer_[disable/enable]).
> > >> It means we are reluctant to use the global variable directly.
> > >> So should we make new function as is_oom_killer_disable?
> > >>
> > >> I think NO.
> > >>
> > >> As I read your description, this problem is related to only hibernation.
> > >> Since hibernation freezes all processes(include kswapd), this problem
> > >> happens. Of course, now oom_killer_disabled is used by only
> > >> hibernation. But it can be used others in future(Off-topic : I don't
> > >> want it). Others can use it without freezing processes. Then kswapd
> > >> can set zone->all_unreclaimable and the problem can't happen.
> > >>
> > >> So I want to use sc->hibernation_mode which is already used
> > >> do_try_to_free_pages instead of oom_killer_disabled.
> > >
> > > Unfortunatelly, It's impossible. shrink_all_memory() turn on
> > > sc->hibernation_mode. but other hibernation caller merely call
> > > alloc_pages(). so we don't have any hint.
> > >
> > Ahh.. True. Sorry for that.
> > I will think some better method.
> > if I can't find it, I don't mind this patch. :)
> 
> It seems that the poblem happens following as. 
> (I might miss something since I just read theyour description)
> 
> hibernation
> oom_disable
> alloc_pages
> do_try_to_free_pages
>         if (scanning_global_lru(sc) && !all_unreclaimable)
>                 return 1;
> If kswapd is not freezed, it would set zone->all_unreclaimable to 1 and then
> shrink_zones maybe return true. so alloc_pages could go to _nopage_.
> If it is, it's no problem. 
> Right?
> 
> I think the problem would come from shrink_zones. 
> It set false to all_unreclaimable blindly even though shrink_zone can't reclaim
> any page. It doesn't make sense. 
> How about this?
> I think we need this regardless of the problem.
> What do you think about?
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index d8fd87d..22017b3 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1901,7 +1901,8 @@ static bool shrink_zones(int priority, struct zonelist *zonelist,
>                 }
>  
>                 shrink_zone(priority, zone, sc);
> -               all_unreclaimable = false;
> +               if (sc->nr_reclaimed)
> +                       all_unreclaimable = false;
>         }
>         return all_unreclaimable;
>  }

here is brief of shrink_zones().

        for_each_zone_zonelist_nodemask(zone, z, zonelist,
                                        gfp_zone(sc->gfp_mask), sc->nodemask) {
                if (!populated_zone(zone))
                        continue;
                if (zone->all_unreclaimable && priority != DEF_PRIORITY)
                            continue;       /* Let kswapd poll it */
                shrink_zone(priority, zone, sc);
                all_unreclaimable = false;
        }

That said,
	all zone's zone->all_unreclaimable are true
		-> all_unreclaimable local variable become true.
	otherwise
		-> all_unreclaimable local variable become false.

The intention is, we don't want to invoke oom-killer if there are
!all_unreclaimable zones. So your patch makes big design change and
seems to increase OOM risk.

I don't want to send risky patch to -stable. 




--
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: [BUGFIX][PATCH] vmscan: don't use return value trick when oom_killer_disabled

[Minchan Kim]

[-- Attachment #1: Type: text/plain, Size: 7209 bytes --]

On Thu, Sep 2, 2010 at 9:57 AM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
>> On Wed, Sep 01, 2010 at 11:01:43AM +0900, Minchan Kim wrote:
>> > On Wed, Sep 1, 2010 at 10:55 AM, KOSAKI Motohiro
>> > <kosaki.motohiro@jp.fujitsu.com> wrote:
>> > > Hi
>> > >
>> > > Thank you for good commenting!
>> > >
>> > >
>> > >> I don't like use oom_killer_disabled directly.
>> > >> That's because we have wrapper inline functions to handle the
>> > >> variable(ex, oom_killer_[disable/enable]).
>> > >> It means we are reluctant to use the global variable directly.
>> > >> So should we make new function as is_oom_killer_disable?
>> > >>
>> > >> I think NO.
>> > >>
>> > >> As I read your description, this problem is related to only hibernation.
>> > >> Since hibernation freezes all processes(include kswapd), this problem
>> > >> happens. Of course, now oom_killer_disabled is used by only
>> > >> hibernation. But it can be used others in future(Off-topic : I don't
>> > >> want it). Others can use it without freezing processes. Then kswapd
>> > >> can set zone->all_unreclaimable and the problem can't happen.
>> > >>
>> > >> So I want to use sc->hibernation_mode which is already used
>> > >> do_try_to_free_pages instead of oom_killer_disabled.
>> > >
>> > > Unfortunatelly, It's impossible. shrink_all_memory() turn on
>> > > sc->hibernation_mode. but other hibernation caller merely call
>> > > alloc_pages(). so we don't have any hint.
>> > >
>> > Ahh.. True. Sorry for that.
>> > I will think some better method.
>> > if I can't find it, I don't mind this patch. :)
>>
>> It seems that the poblem happens following as.
>> (I might miss something since I just read theyour description)
>>
>> hibernation
>> oom_disable
>> alloc_pages
>> do_try_to_free_pages
>>         if (scanning_global_lru(sc) && !all_unreclaimable)
>>                 return 1;
>> If kswapd is not freezed, it would set zone->all_unreclaimable to 1 and then
>> shrink_zones maybe return true. so alloc_pages could go to _nopage_.
>> If it is, it's no problem.
>> Right?
>>
>> I think the problem would come from shrink_zones.
>> It set false to all_unreclaimable blindly even though shrink_zone can't reclaim
>> any page. It doesn't make sense.
>> How about this?
>> I think we need this regardless of the problem.
>> What do you think about?
>>
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index d8fd87d..22017b3 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -1901,7 +1901,8 @@ static bool shrink_zones(int priority, struct zonelist *zonelist,
>>                 }
>>
>>                 shrink_zone(priority, zone, sc);
>> -               all_unreclaimable = false;
>> +               if (sc->nr_reclaimed)
>> +                       all_unreclaimable = false;
>>         }
>>         return all_unreclaimable;
>>  }
>
> here is brief of shrink_zones().
>
>        for_each_zone_zonelist_nodemask(zone, z, zonelist,
>                                        gfp_zone(sc->gfp_mask), sc->nodemask) {
>                if (!populated_zone(zone))
>                        continue;
>                if (zone->all_unreclaimable && priority != DEF_PRIORITY)
>                            continue;       /* Let kswapd poll it */
>                shrink_zone(priority, zone, sc);
>                all_unreclaimable = false;
>        }
>
> That said,
>        all zone's zone->all_unreclaimable are true
>                -> all_unreclaimable local variable become true.
>        otherwise
>                -> all_unreclaimable local variable become false.
>
> The intention is, we don't want to invoke oom-killer if there are
> !all_unreclaimable zones. So your patch makes big design change and
> seems to increase OOM risk.

Right. Thanks for pointing me out.

> I don't want to send risky patch to -stable.

Still I don't want to use oom_killer_disabled magic.
But I don't want to prevent urgent stable patch due to my just nitpick.

This is my last try(just quick patch, even I didn't tried compile test).
If this isn't good, first of all, let's try merge yours.
And then we can fix it later.

Thanks for comment.

-- CUT HERE --

Why do we check zone->all_unreclaimable in only kswapd?
If kswapd is freezed in hibernation, OOM can happen.
Let's the check in direct reclaim path, too.


diff --git a/mm/vmscan.c b/mm/vmscan.c
index 3109ff7..41493ba 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1878,12 +1878,11 @@ static void shrink_zone(int priority, struct zone *zone,
 * If a zone is deemed to be full of pinned pages then just give it a light
  * scan then give up on it.
  */
-static bool shrink_zones(int priority, struct zonelist *zonelist,
+static void shrink_zones(int priority, struct zonelist *zonelist,
                                        struct scan_control *sc)
 {
        struct zoneref *z;
        struct zone *zone;
-       bool all_unreclaimable = true;

        for_each_zone_zonelist_nodemask(zone, z, zonelist,
                                        gfp_zone(sc->gfp_mask), sc->nodemask) {
@@ -1901,8 +1900,25 @@ static bool shrink_zones(int priority, struct
zonelist *zonelist,
                }

                shrink_zone(priority, zone, sc);
-               all_unreclaimable = false;
        }
+}
+
+static inline int all_unreclaimable(struct zonelist *zonelist, struct
scan_control *sc)
+{
+       struct zoneref *z;
+       struct zone *zone;
+       bool all_unreclaimable = true;
+
+       for_each_zone_zonelist_nodemask(zone, z, zonelist,
+                                       gfp_zone(sc->gfp_mask), sc->nodemask) {
+               if (!populated_zone(zone))
+                       continue;
+               if (zone->pages_scanned < (zone_reclaimable_pages(zone) * 6)) {
+                       all_unreclaimable = false;
+                       break;
+               }
+       }
+
        return all_unreclaimable;
 }

@@ -1926,7 +1942,6 @@ static unsigned long do_try_to_free_pages(struct
zonelist *zonelist,
                                        struct scan_control *sc)
 {
        int priority;
-       bool all_unreclaimable;
        unsigned long total_scanned = 0;
        struct reclaim_state *reclaim_state = current->reclaim_state;
        struct zoneref *z;
@@ -1943,7 +1958,7 @@ static unsigned long do_try_to_free_pages(struct
zonelist *zonelist,
                sc->nr_scanned = 0;
                if (!priority)
                        disable_swap_token();
-               all_unreclaimable = shrink_zones(priority, zonelist, sc);
+               shrink_zones(priority, zonelist, sc);
                /*
                 * Don't shrink slabs when reclaiming memory from
                 * over limit cgroups
@@ -2005,7 +2020,7 @@ out:
                return sc->nr_reclaimed;

        /* top priority shrink_zones still had more to do? don't OOM, then */
-       if (scanning_global_lru(sc) && !all_unreclaimable)
+       if (scanning_global_lru(sc) && !all_unreclaimable(zonelist, sc))
                return 1;

        return 0;


-- 
Kind regards,
Minchan Kim

[-- Attachment #2: all_unreclaimable.patch --]
[-- Type: text/x-diff, Size: 2184 bytes --]

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 3109ff7..41493ba 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1878,12 +1878,11 @@ static void shrink_zone(int priority, struct zone *zone,
  * If a zone is deemed to be full of pinned pages then just give it a light
  * scan then give up on it.
  */
-static bool shrink_zones(int priority, struct zonelist *zonelist,
+static void shrink_zones(int priority, struct zonelist *zonelist,
 					struct scan_control *sc)
 {
 	struct zoneref *z;
 	struct zone *zone;
-	bool all_unreclaimable = true;
 
 	for_each_zone_zonelist_nodemask(zone, z, zonelist,
 					gfp_zone(sc->gfp_mask), sc->nodemask) {
@@ -1901,8 +1900,25 @@ static bool shrink_zones(int priority, struct zonelist *zonelist,
 		}
 
 		shrink_zone(priority, zone, sc);
-		all_unreclaimable = false;
 	}
+}
+
+static inline int all_unreclaimable(struct zonelist *zonelist, struct scan_control *sc)
+{
+	struct zoneref *z;
+	struct zone *zone;
+	bool all_unreclaimable = true;
+
+	for_each_zone_zonelist_nodemask(zone, z, zonelist,
+					gfp_zone(sc->gfp_mask), sc->nodemask) {
+		if (!populated_zone(zone))
+			continue;
+		if (zone->pages_scanned < (zone_reclaimable_pages(zone) * 6)) {
+			all_unreclaimable = false;
+			break;
+		}
+	}
+
 	return all_unreclaimable;
 }
 
@@ -1926,7 +1942,6 @@ static unsigned long do_try_to_free_pages(struct zonelist *zonelist,
 					struct scan_control *sc)
 {
 	int priority;
-	bool all_unreclaimable;
 	unsigned long total_scanned = 0;
 	struct reclaim_state *reclaim_state = current->reclaim_state;
 	struct zoneref *z;
@@ -1943,7 +1958,7 @@ static unsigned long do_try_to_free_pages(struct zonelist *zonelist,
 		sc->nr_scanned = 0;
 		if (!priority)
 			disable_swap_token();
-		all_unreclaimable = shrink_zones(priority, zonelist, sc);
+		shrink_zones(priority, zonelist, sc);
 		/*
 		 * Don't shrink slabs when reclaiming memory from
 		 * over limit cgroups
@@ -2005,7 +2020,7 @@ out:
 		return sc->nr_reclaimed;
 
 	/* top priority shrink_zones still had more to do? don't OOM, then */
-	if (scanning_global_lru(sc) && !all_unreclaimable)
+	if (scanning_global_lru(sc) && !all_unreclaimable(zonelist, sc))
 		return 1;
 
 	return 0;

Re: [BUGFIX][PATCH] vmscan: don't use return value trick when oom_killer_disabled

[KOSAKI Motohiro]

> > I don't want to send risky patch to -stable.
> 
> Still I don't want to use oom_killer_disabled magic.
> But I don't want to prevent urgent stable patch due to my just nitpick.
> 
> This is my last try(just quick patch, even I didn't tried compile test).

Looks like conceptually correct. If you will test it and fix whitespace damage,
I'll ack this one gladly.

Thanks.



> If this isn't good, first of all, let's try merge yours.
> And then we can fix it later.
> 
> Thanks for comment.
> 
> -- CUT HERE --
> 
> Why do we check zone->all_unreclaimable in only kswapd?
> If kswapd is freezed in hibernation, OOM can happen.
> Let's the check in direct reclaim path, too.
> 
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 3109ff7..41493ba 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1878,12 +1878,11 @@ static void shrink_zone(int priority, struct zone *zone,
>  * If a zone is deemed to be full of pinned pages then just give it a light
>   * scan then give up on it.
>   */
> -static bool shrink_zones(int priority, struct zonelist *zonelist,
> +static void shrink_zones(int priority, struct zonelist *zonelist,
>                                         struct scan_control *sc)
>  {
>         struct zoneref *z;
>         struct zone *zone;
> -       bool all_unreclaimable = true;
> 
>         for_each_zone_zonelist_nodemask(zone, z, zonelist,
>                                         gfp_zone(sc->gfp_mask), sc->nodemask) {
> @@ -1901,8 +1900,25 @@ static bool shrink_zones(int priority, struct
> zonelist *zonelist,
>                 }
> 
>                 shrink_zone(priority, zone, sc);
> -               all_unreclaimable = false;
>         }
> +}
> +
> +static inline int all_unreclaimable(struct zonelist *zonelist, struct
> scan_control *sc)
> +{
> +       struct zoneref *z;
> +       struct zone *zone;
> +       bool all_unreclaimable = true;
> +
> +       for_each_zone_zonelist_nodemask(zone, z, zonelist,
> +                                       gfp_zone(sc->gfp_mask), sc->nodemask) {
> +               if (!populated_zone(zone))
> +                       continue;
> +               if (zone->pages_scanned < (zone_reclaimable_pages(zone) * 6)) {
> +                       all_unreclaimable = false;
> +                       break;
> +               }
> +       }
> +
>         return all_unreclaimable;
>  }
> 
> @@ -1926,7 +1942,6 @@ static unsigned long do_try_to_free_pages(struct
> zonelist *zonelist,
>                                         struct scan_control *sc)
>  {
>         int priority;
> -       bool all_unreclaimable;
>         unsigned long total_scanned = 0;
>         struct reclaim_state *reclaim_state = current->reclaim_state;
>         struct zoneref *z;
> @@ -1943,7 +1958,7 @@ static unsigned long do_try_to_free_pages(struct
> zonelist *zonelist,
>                 sc->nr_scanned = 0;
>                 if (!priority)
>                         disable_swap_token();
> -               all_unreclaimable = shrink_zones(priority, zonelist, sc);
> +               shrink_zones(priority, zonelist, sc);
>                 /*
>                  * Don't shrink slabs when reclaiming memory from
>                  * over limit cgroups
> @@ -2005,7 +2020,7 @@ out:
>                 return sc->nr_reclaimed;
> 
>         /* top priority shrink_zones still had more to do? don't OOM, then */
> -       if (scanning_global_lru(sc) && !all_unreclaimable)
> +       if (scanning_global_lru(sc) && !all_unreclaimable(zonelist, sc))
>                 return 1;
> 
>         return 0;
> 
> 
> -- 
> Kind regards,
> Minchan Kim



--
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: [BUGFIX][PATCH] vmscan: don't use return value trick when oom_killer_disabled

[Minchan Kim]

On Thu, Sep 2, 2010 at 12:05 PM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
>> > I don't want to send risky patch to -stable.
>>
>> Still I don't want to use oom_killer_disabled magic.
>> But I don't want to prevent urgent stable patch due to my just nitpick.
>>
>> This is my last try(just quick patch, even I didn't tried compile test).
>
> Looks like conceptually correct. If you will test it and fix whitespace damage,
> I'll ack this one gladly.
>
> Thanks.
>

I will resend formal patch ASAP. Maybe tonight after out of office.
Thanks for quick reply.


-- 
Kind regards,
Minchan Kim

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