Michal Hocko wrote: > On Thu 22-03-18 19:51:56, Tetsuo Handa wrote: > [...] > > The whole point of the sleep is to give the OOM victim some time to exit. > > Yes, and that is why we sleep under the lock because that would rule all > other potential out_of_memory callers from jumping in. As long as there is !MMF_OOM_SKIP mm, jumping in does not cause problems. But since this patch did not remove mutex_lock() from the OOM reaper, nobody can jump in until the OOM reaper completes the first reclaim attempt. And since it is likely that mutex_trylock() by the OOM reaper succeeds, somebody unlikely finds !MMF_OOM_SKIP mm when it jumped in. > > > However, the sleep can prevent contending allocating paths from hitting > > the OOM path again even if the OOM victim was able to exit. We need to > > make sure that the thread which called out_of_memory() will release > > oom_lock shortly. Thus, this patch brings the sleep to outside of the OOM > > path. Since the OOM reaper waits for the oom_lock, this patch unlikely > > allows contending allocating paths to hit the OOM path earlier than now. > > The sleep outside of the lock doesn't make much sense to me. It is > basically contradicting its original purpose. If we do want to throttle > direct reclaimers than OK but this patch is not the way how to do that. > > If you really believe that the sleep is more harmful than useful, then > fair enough, I would rather see it removed than shuffled all over > outside the lock. Yes, I do believe that the sleep with oom_lock held is more harmful than useful. Please remove the sleep (but be careful not to lose the guaranteed sleep for PF_WQ_WORKER). > > So > Nacked-by: Michal Hocko <mhocko@xxxxxxxx> > -- > Michal Hocko > SUSE Labs >
