Re: [PATCH] mm,oom: Try last second allocation before and after selecting an OOM victim.

[Tetsuo Handa]

Michal Hocko wrote:
> On Tue 31-10-17 19:40:09, Tetsuo Handa wrote:
> > The reason I used __alloc_pages_slowpath() in alloc_pages_before_oomkill() is
> > to avoid duplicating code (such as checking for ALLOC_OOM and rebuilding zone
> > list) which needs to be maintained in sync with __alloc_pages_slowpath().
> >
> > If you don't like calling __alloc_pages_slowpath() from
> > alloc_pages_before_oomkill(), I'm OK with calling __alloc_pages_nodemask()
> > (with __GFP_DIRECT_RECLAIM/__GFP_NOFAIL cleared and __GFP_NOWARN set), for
> > direct reclaim functions can call __alloc_pages_nodemask() (with PF_MEMALLOC
> > set in order to avoid recursion of direct reclaim).
> > 
> > We are rebuilding zone list if selected as an OOM victim, for
> > __gfp_pfmemalloc_flags() returns ALLOC_OOM if oom_reserves_allowed(current)
> > is true.
> 
> So your answer is copy&paste without a deeper understanding, righ?

Right. I wanted to avoid duplicating code.
But I had to duplicate in order to allow OOM victims to try ALLOC_OOM.

> 
> [...]
> 
> > The reason I'm proposing this "mm,oom: Try last second allocation before and
> > after selecting an OOM victim." is that since oom_reserves_allowed(current) can
> > become true when current is between post __gfp_pfmemalloc_flags(gfp_mask) and
> > pre mutex_trylock(&oom_lock), an OOM victim can fail to try ALLOC_OOM attempt
> > before selecting next OOM victim when MMF_OOM_SKIP was set quickly.
> 
> ENOPARSE. I am not even going to finish this email sorry. This is way
> beyond my time budget.
> 
> Can you actually come with something that doesn't make ones head explode
> and yet describe what the actual problem is and how you deal with it?

http://lkml.kernel.org/r/201708191523.BJH90621.MHOOFFQSOLJFtV@xxxxxxxxxxxxxxxxxxx
is least head exploding while it describes what the actual problem is and
how I deal with it.

> 
> E.g something like this
> "
> OOM killer is invoked after all the reclaim attempts have failed and
> there doesn't seem to be a viable chance for the situation to change.
> __alloc_pages_may_oom tries to reduce chances of a race during OOM
> handling by taking oom lock so only one caller is allowed to really
> invoke the oom killer.

OK.
> 
> __alloc_pages_may_oom also tries last time ALLOC_WMARK_HIGH allocation
> request before really invoking out_of_memory handler. This has two
> motivations. The first one is explained by the comment and it aims to
> catch potential parallel OOM killing and the second one was explained by
> Andrea Arcangeli as follows:
> : Elaborating the comment: the reason for the high wmark is to reduce
> : the likelihood of livelocks and be sure to invoke the OOM killer, if
> : we're still under pressure and reclaim just failed. The high wmark is
> : used to be sure the failure of reclaim isn't going to be ignored. If
> : using the min wmark like you propose there's risk of livelock or
> : anyway of delayed OOM killer invocation.
> 

OK.

> While both have some merit, the first reason is mostly historical
> because we have the explicit locking now and it is really unlikely that
> the memory would be available right after we have given up trying.
> Last attempt allocation makes some sense of course but considering that
> the oom victim selection is quite an expensive operation which can take
> a considerable amount of time it makes much more sense to retry the
> allocation after the most expensive part rather than before. Therefore
> move the last attempt right before we are trying to kill an oom victim
> to rule potential races when somebody could have freed a lot of memory
> in the meantime. This will reduce the time window for potentially
> pre-mature OOM killing considerably.

But this is about "doing last second allocation attempt after selecting
an OOM victim". This is not about "allowing OOM victims to try ALLOC_OOM
before selecting next OOM victim" which is the actual problem I'm trying
to deal with. Moving last second allocation attempt from "before" to
"after" does not solve the problem if ALLOC_OOM cannot be used. What I'm
proposing is to allow OOM victims to try ALLOC_OOM.

> "