On Tue 17-11-15 19:58:09, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > __alloc_pages_slowpath is looping over ALLOC_NO_WATERMARKS requests if
> > __GFP_NOFAIL is requested. This is fragile because we are basically
> > relying on somebody else to make the reclaim (be it the direct reclaim
> > or OOM killer) for us. The caller might be holding resources (e.g.
> > locks) which block other other reclaimers from making any progress for
> > example. Remove the retry loop and rely on __alloc_pages_slowpath to
> > invoke all allowed reclaim steps and retry logic.
>
> This implies invoking OOM killer, doesn't it?
It does and the changelog is explicit about this.
> > /* Avoid recursion of direct reclaim */
> > - if (current->flags & PF_MEMALLOC)
> > + if (current->flags & PF_MEMALLOC) {
> > + /*
> > + * __GFP_NOFAIL request from this context is rather bizarre
> > + * because we cannot reclaim anything and only can loop waiting
> > + * for somebody to do a work for us.
> > + */
> > + if (WARN_ON_ONCE(gfp_mask & __GFP_NOFAIL)) {
> > + cond_resched();
> > + goto retry;
>
> I think that this "goto retry;" omits call to out_of_memory() which is allowed
> for __GFP_NOFAIL allocations.
It wasn't called for PF_MEMALLOC requests though. Whether invoking OOM
killer is a good idea for this case is a harder question and out of
scope of this patch.
> Even if this is what you meant, current thread
> can be a workqueue, which currently need a short sleep (as with
> wait_iff_congested() changes), can't it?
As the changelog tries to clarify PF_MEMALLOC with __GFP_NOFAIL is
basically a bug. That is the reason I am adding WARN_ON there. I do not
think making this code more complex for abusers/buggy code is really
worthwhile. Besides that I fail to see why a work item would ever
want to set PF_MEMALLOC for legitimate reasons. I have done a quick git
grep over the tree and there doesn't seem to be any user.
>
> > + }
> > goto nopage;
> > + }
> >
> > /* Avoid allocations with no watermarks from looping endlessly */
> > if (test_thread_flag(TIF_MEMDIE) && !(gfp_mask & __GFP_NOFAIL))
> >
>
> Well, is it cond_resched() which should include
>
> if (current->flags & PF_WQ_WORKER)
> schedule_timeout(1);
I believe you are getting off-topic here.
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>