Re: [PATCH v2] memcg, oom: check memcg margin for parallel oom

[David Rientjes <rientjes@google.com>]

On Wed, 15 Jul 2020, Yafang Shao wrote:

> > > If it is the race which causes this issue and we want to reduce the
> > > race window, I don't know whether it is proper to check the memcg
> > > margin in out_of_memory() or  do it before calling do_send_sig_info().
> > > Because per my understanding, dump_header() always takes much more
> > > time than select_bad_process() especially if there're slow consoles.
> > > So the race might easily happen when doing dump_header()  or dumping
> > > other information, but if we check the memcg margin after dumping this
> > > oom info, it would be strange to dump so much oom logs without killing
> > > a process.
> > >
> >
> > Absolutely correct :)  In my proposed patch, we declare dump_header() as
> > the "point of no return" since we don't want to dump oom kill information
> > to the kernel log when nothing is actually killed.  We could abort at the
> > very last minute, as you mention, but I think that may have an adverse
> > impact on anything that cares about that log message.
> 
> How about storing the memcg information in oom_control when the memcg
> oom is triggered, and then show this information in dump_header() ?
> IOW, the OOM info really shows the memcg status when oom occurs,
> rather than the memcg status when this info is printed.
> 

We actually do that too in our kernel but for slightly other reasons :)  
It's pretty interesting how a lot of our previous concerns with memcg oom 
killing have been echoed by you in this thread.  But yes, we store vital 
information about the memcg at the time of the first oom event when the 
oom killer is disabled (to allow userspace to determine what the best 
course of action is).

But regardless of whether we present previous data to the user in the 
kernel log or not, we've determined that oom killing a process is a 
serious matter and go to any lengths possible to avoid having to do it.  
For us, that means waiting until the "point of no return" to either go 
ahead with oom killing a process or aborting and retrying the charge.

I don't think moving the mem_cgroup_margin() check to out_of_memory() 
right before printing the oom info and killing the process is a very 
invasive patch.  Any strong preference against doing it that way?  I think 
moving the check as late as possible to save a process from being killed 
when racing with an exiter or killed process (including perhaps current) 
has a pretty clear motivation.