Re: Stalled MM patches for review

Johannes Weiner
2014-12-17T02:13:07Z

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:

---