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

[Yafang Shao <laoar.shao@gmail.com>]

On Thu, Jul 16, 2020 at 1:30 AM David Rientjes <rientjes@google.com> wrote:
>
> 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.

These should be common concerns of container users :)
I'm a heavy container user for now.

>  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).
>

It would be better if you could upstream the features in your kernel,
and I think it could also help the other users.

> 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.

I understand what you mean "point of no return", but that seems a
workaround rather than a fix.
If you don't want to kill unnecessary processes, then checking the
memcg margin before sending sigkill is better, because as I said
before the race will be most likely happening in dump_header().
If you don't want to show strange OOM information like "your process
was oom killed and it shows usage is 60MB in a memcg limited
to 100MB", it is better to get the snapshot of the OOM when it is
triggered and then show it later, and I think it could also apply to
the global OOM.

While my patch means to fix the issue caused by parallel OOM, because
the others are waiting oom_lock while one process is doing OOM. And as
explained by Michal before, it is more in
line with the global oom flow and it is much easier to reason about.

-- 
Thanks
Yafang