Messages in this thread | | | Subject | Re: [PATCH 2/2] mm: do not loop over ALLOC_NO_WATERMARKS without triggering reclaim | From | Tetsuo Handa <> | Date | Tue, 17 Nov 2015 19:58:09 +0900 |
| |
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?
> /* 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. 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?
> + } > 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);
than wait_iff_congested() because not all yield calls use wait_iff_congested() and giving pending workqueue jobs a chance to be processed is anyway preferable?
int __sched _cond_resched(void) { if (should_resched(0)) { if ((current->flags & PF_WQ_WORKER) && workqueue_has_pending_jobs()) schedule_timeout(1); else preempt_schedule_common(); return 1; } return 0; }
| |