Re: [PATCH 0/3] OOM detection rework v4

[Joonsoo Kim]

On Mon, Feb 29, 2016 at 10:02:13PM +0100, Michal Hocko wrote:
> Andrew,
> could you queue this one as well, please? This is more a band aid than a
> real solution which I will be working on as soon as I am able to
> reproduce the issue but the patch should help to some degree at least.

I'm not sure that this is a way to go. See below.

> 
> On Thu 25-02-16 10:23:15, Michal Hocko wrote:
> > From d09de26cee148b4d8c486943b4e8f3bd7ad6f4be Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@xxxxxxxx>
> > Date: Thu, 4 Feb 2016 14:56:59 +0100
> > Subject: [PATCH] mm, oom: protect !costly allocations some more
> > 
> > should_reclaim_retry will give up retries for higher order allocations
> > if none of the eligible zones has any requested or higher order pages
> > available even if we pass the watermak check for order-0. This is done
> > because there is no guarantee that the reclaimable and currently free
> > pages will form the required order.
> > 
> > This can, however, lead to situations were the high-order request (e.g.
> > order-2 required for the stack allocation during fork) will trigger
> > OOM too early - e.g. after the first reclaim/compaction round. Such a
> > system would have to be highly fragmented and the OOM killer is just a
> > matter of time but let's stick to our MAX_RECLAIM_RETRIES for the high
> > order and not costly requests to make sure we do not fail prematurely.
> > 
> > This also means that we do not reset no_progress_loops at the
> > __alloc_pages_slowpath for high order allocations to guarantee a bounded
> > number of retries.
> > 
> > Longterm it would be much better to communicate with the compaction
> > and retry only if the compaction considers it meaningfull.
> > 
> > Signed-off-by: Michal Hocko <mhocko@xxxxxxxx>
> > ---
> >  mm/page_alloc.c | 20 ++++++++++++++++----
> >  1 file changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 269a04f20927..f05aca36469b 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -3106,6 +3106,18 @@ should_reclaim_retry(gfp_t gfp_mask, unsigned order,
> >  		}
> >  	}
> >  
> > +	/*
> > +	 * OK, so the watermak check has failed. Make sure we do all the
> > +	 * retries for !costly high order requests and hope that multiple
> > +	 * runs of compaction will generate some high order ones for us.
> > +	 *
> > +	 * XXX: ideally we should teach the compaction to try _really_ hard
> > +	 * if we are in the retry path - something like priority 0 for the
> > +	 * reclaim
> > +	 */
> > +	if (order && order <= PAGE_ALLOC_COSTLY_ORDER)
> > +		return true;
> > +
> >  	return false;

This seems not a proper fix. Checking watermark with high order has
another meaning that there is high order page or not. This isn't
what we want here. So, following fix is needed.

'if (order)' check isn't needed. It is used to clarify the meaning of
this fix. You can remove it.

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 1993894..8c80375 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3125,6 +3125,10 @@ should_reclaim_retry(gfp_t gfp_mask, unsigned order,
        if (order > PAGE_ALLOC_COSTLY_ORDER && !(gfp_mask & __GFP_REPEAT))
                return false;
 
+       /* To check whether compaction is available or not */
+       if (order)
+               order = 0;
+
        /*
         * Keep reclaiming pages while there is a chance this will lead
         * somewhere.  If none of the target zones can satisfy our allocation

> >  }
> >  
> > @@ -3281,11 +3293,11 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> >  		goto noretry;
> >  
> >  	/*
> > -	 * Costly allocations might have made a progress but this doesn't mean
> > -	 * their order will become available due to high fragmentation so do
> > -	 * not reset the no progress counter for them
> > +	 * High order allocations might have made a progress but this doesn't
> > +	 * mean their order will become available due to high fragmentation so
> > +	 * do not reset the no progress counter for them
> >  	 */
> > -	if (did_some_progress && order <= PAGE_ALLOC_COSTLY_ORDER)
> > +	if (did_some_progress && !order)
> >  		no_progress_loops = 0;
> >  	else
> >  		no_progress_loops++;

This unconditionally increases no_progress_loops for high order
allocation, so, after 16 iterations, it will fail. If compaction isn't
enabled in Kconfig, 16 times reclaim attempt would not be sufficient
to make high order page. Should we consider this case also?

Thanks.