lkml.org 
[lkml]   [2011]   [Nov]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] mm: avoid livelock on !__GFP_FS allocations
    On Tue, Nov 15, 2011 at 05:36:56PM +0000, Mel Gorman wrote:
    > On Wed, Nov 16, 2011 at 01:13:30AM +0900, Minchan Kim wrote:
    > > On Mon, Nov 14, 2011 at 11:04 PM, Mel Gorman <mgorman@suse.de> wrote:
    > > > 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.
    > > >
    > > > [mgorman@suse.de: Rework fix to be suspend specific]
    > > > Reported-and-tested-by: Colin Cross <ccross@android.com>
    > > > Signed-off-by: Mel Gorman <mgorman@suse.de>
    > > > ---
    > > >  mm/page_alloc.c |   22 ++++++++++++++++++++++
    > > >  1 files changed, 22 insertions(+), 0 deletions(-)
    > > >
    > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
    > > > index 9dd443d..5402897 100644
    > > > --- a/mm/page_alloc.c
    > > > +++ b/mm/page_alloc.c
    > > > @@ -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;
    > > >        }
    > > >
    > > >        /* 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?
    >

    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2011-11-16 01:25    [W:0.038 / U:153.552 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site