Re: Stalled MM patches for review

[Johannes Weiner <hannes@cmpxchg.org>]

On Tue, Dec 16, 2014 at 05:07:16PM -0800, David Rientjes wrote:
> On Mon, 15 Dec 2014, Johannes Weiner wrote:
> >  	/* Check if we should retry the allocation */
> >  	pages_reclaimed += did_some_progress;
> >  	if (should_alloc_retry(gfp_mask, order, did_some_progress,
> >  						pages_reclaimed)) {
> > +		/*
> > +		 * If we fail to make progress by freeing individual
> > +		 * pages, but the allocation wants us to keep going,
> > +		 * start OOM killing tasks.
> > +		 */
> > +		if (!did_some_progress) {
> > +			page = __alloc_pages_may_oom(gfp_mask, order, zonelist,
> > +						high_zoneidx, nodemask,
> > +						preferred_zone, classzone_idx,
> > +						migratetype,&did_some_progress);
> 
> Missing a space.

That was because of the 80 character limit, it seemed preferrable over
a linewrap.

> > +			if (page)
> > +				goto got_pg;
> > +			if (!did_some_progress)
> > +				goto nopage;
> > +		}
> >  		/* Wait for some write requests to complete then retry */
> >  		wait_iff_congested(preferred_zone, BLK_RW_ASYNC, HZ/50);
> >  		goto rebalance;
> 
> This is broken because it does not recall gfp_to_alloc_flags().  If 
> current is the oom kill victim, then ALLOC_NO_WATERMARKS never gets set 
> properly and the slowpath will end up looping forever.  The "restart" 
> label which was removed in this patch needs to be reintroduced, and it can 
> probably be moved to directly before gfp_to_alloc_flags().

Thanks for catching this.  gfp_to_alloc_flags()'s name doesn't exactly
imply such side effects...  Here is a fixlet on top:

---