Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority

[Michal Hocko]

On Thu 22-09-16 16:08:21, Michal Hocko wrote:
> On Thu 22-09-16 14:51:48, Vlastimil Babka wrote:
> > >From 465e1bd61b7a6d6901a44f09b1a76514dbc220fa Mon Sep 17 00:00:00 2001
> > From: Vlastimil Babka <vbabka@xxxxxxx>
> > Date: Thu, 22 Sep 2016 13:54:32 +0200
> > Subject: [PATCH] mm, compaction: more reliably increase direct compaction
> >  priority-fix
> > 
> > When increasing the compaction priority, also reset retries. Otherwise we can
> > consume all retries on the lower priorities.
> 
> OK, this is an improvement. I am just thinking that we might want to
> pull
> 	if (order && compaction_made_progress(compact_result))
> 		compaction_retries++;
> 
> into should_compact_retry as well. I've had it there originally because
> it was in line with no_progress_loops but now that we have compaction
> priorities it would fit into retry logic better. As a plus it would
> count only those compaction rounds where we we didn't have to rely on
                                                 did that should be

> the compaction retry logic. What do you think?
> 
> > Suggested-by: Michal Hocko <mhocko@xxxxxxxx>
> > Signed-off-by: Vlastimil Babka <vbabka@xxxxxxx>
> 
> Anyway
> Acked-by: Michal Hocko <mhocko@xxxxxxxx>
> 
> > ---
> >  mm/page_alloc.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index f8bed910e3cf..82fdb690ac62 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -3162,7 +3162,7 @@ static inline bool
> >  should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
> >  		     enum compact_result compact_result,
> >  		     enum compact_priority *compact_priority,
> > -		     int compaction_retries)
> > +		     int *compaction_retries)
> >  {
> >  	int max_retries = MAX_COMPACT_RETRIES;
> >  
> > @@ -3196,16 +3196,17 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
> >  	 */
> >  	if (order > PAGE_ALLOC_COSTLY_ORDER)
> >  		max_retries /= 4;
> > -	if (compaction_retries <= max_retries)
> > +	if (*compaction_retries <= max_retries)
> >  		return true;
> >  
> >  	/*
> > -	 * Make sure there is at least one attempt at the highest priority
> > -	 * if we exhausted all retries at the lower priorities
> > +	 * Make sure there are attempts at the highest priority if we exhausted
> > +	 * all retries or failed at the lower priorities.
> >  	 */
> >  check_priority:
> >  	if (*compact_priority > MIN_COMPACT_PRIORITY) {
> >  		(*compact_priority)--;
> > +		*compaction_retries = 0;
> >  		return true;
> >  	}
> >  	return false;
> > @@ -3224,7 +3225,7 @@ static inline bool
> >  should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_flags,
> >  		     enum compact_result compact_result,
> >  		     enum compact_priority *compact_priority,
> > -		     int compaction_retries)
> > +		     int *compaction_retries)
> >  {
> >  	struct zone *zone;
> >  	struct zoneref *z;
> > @@ -3663,7 +3664,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> >  	if (did_some_progress > 0 &&
> >  			should_compact_retry(ac, order, alloc_flags,
> >  				compact_result, &compact_priority,
> > -				compaction_retries))
> > +				&compaction_retries))
> >  		goto retry;
> >  
> >  	/* Reclaim has failed us, start killing things */
> > -- 
> > 2.10.0
> > 
> 
> -- 
> Michal Hocko
> SUSE Labs

-- 
Michal Hocko
SUSE Labs