Re: [PATCH] mm,page_alloc: softlockup on warn_alloc on

[Michal Hocko <mhocko@suse.com>]

On Mon 18-09-17 15:31:31, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > > > The synchronization has worked this way for a long time (trylock
> > > > failure assuming progress, but the order/NOFS/zone bailouts from
> > > > actually OOM-killing inside the locked section). We should really fix
> > > > *that* rather than serializing warn_alloc().
> > > > 
> > > > For GFP_NOFS, it seems to go back to 9879de7373fc ("mm: page_alloc:
> > > > embed OOM killing naturally into allocation slowpath"). Before that we
> > > > didn't use to call __alloc_pages_may_oom() for NOFS allocations. So I
> > > > still wonder why this only now appears to be causing problems.
> > > > 
> > > > In any case, converting that trylock to a sleeping lock in this case
> > > > makes sense to me. Nobody is blocking under this lock (except that one
> > > > schedule_timeout_killable(1) after dispatching a victim) and it's not
> > > > obvious to me why we'd need that level of concurrency under OOM.
> > > 
> > > You can try http://lkml.kernel.org/r/1500202791-5427-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp
> > > and http://lkml.kernel.org/r/1503577106-9196-2-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp together.
> > > Then, we can remove mutex_lock(&oom_lock) serialization from __oom_reap_task_mm()
> > > which still exists because Andrea's patch was accepted instead of Michal's patch.
> > 
> > We can safely drop the oom_lock from __oom_reap_task_mm now. Andrea
> > didn't want to do it in his patch because that is a separate thing
> > logically. But nothing should prefent the removal now that AFAICS.
> 
> No! The oom_lock in __oom_reap_task_mm() is still required due to lack of
> really last second allocation attempt. If we do really last second
> allocation attempt, we can remove the oom_lock from __oom_reap_task_mm().

Yes, there is a race possible but this is not a _correctness_ issue. It is
a mere suboptimality. This can and should be addressed separately. I was
not really opposed to your patch to move the last allocation attempt
before oom_kill_process once all the concerns are clarified.
 
> Enter __alloc_pages_may_oom()              Enter __oom_reap_task_mm()
> 
>   Take oom_lock
> 
>   Try last get_page_from_freelist()
> 
>                                              No "take oom_lock" here
> 
>                                              Reap memory
> 
>                                              Set MMF_OOM_SKIP
> 
>                                              No "release oom_lock" here
> 
>                                            Leave __oom_reap_task_mm()
> 
>   Enter out_of_memory()
> 
>     Enter select_bad_process()
> 
>       Enter oom_evaluate_task()
> 
>         Check if MMF_OOM_SKIP is already set
> 
>       Leave oom_evaluate_task()
> 
>     Leave select_bad_process()
> 
>     No "really last get_page_from_freelist()" here
> 
>     Kill the next victim needlessly
> 
>   Leave out_of_memory()
> 
>   Release oom_lock
> 
> Leave __alloc_pages_may_oom()

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>