Re: [PATCH v2] mm, memcg: skip killing processes under memcg protection at first scan

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

On Tue, Aug 20, 2019 at 10:01 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> On Tue, Aug 20, 2019 at 9:40 AM Roman Gushchin <guro@fb.com> wrote:
> >
> > On Tue, Aug 20, 2019 at 09:16:01AM +0800, Yafang Shao wrote:
> > > On Tue, Aug 20, 2019 at 5:12 AM Roman Gushchin <guro@fb.com> wrote:
> > > >
> > > > On Sun, Aug 18, 2019 at 09:18:06PM -0400, Yafang Shao wrote:
> > > > > In the current memory.min design, the system is going to do OOM instead
> > > > > of reclaiming the reclaimable pages protected by memory.min if the
> > > > > system is lack of free memory. While under this condition, the OOM
> > > > > killer may kill the processes in the memcg protected by memory.min.
> > > > > This behavior is very weird.
> > > > > In order to make it more reasonable, I make some changes in the OOM
> > > > > killer. In this patch, the OOM killer will do two-round scan. It will
> > > > > skip the processes under memcg protection at the first scan, and if it
> > > > > can't kill any processes it will rescan all the processes.
> > > > >
> > > > > Regarding the overhead this change may takes, I don't think it will be a
> > > > > problem because this only happens under system  memory pressure and
> > > > > the OOM killer can't find any proper victims which are not under memcg
> > > > > protection.
> > > >
> > > > Hi Yafang!
> > > >
> > > > The idea makes sense at the first glance, but actually I'm worried
> > > > about mixing per-memcg and per-process characteristics.
> > > > Actually, it raises many questions:
> > > > 1) if we do respect memory.min, why not memory.low too?
> > >
> > > memroy.low is different with memory.min, as the OOM killer will not be
> > > invoked when it is reached.
> > > If memory.low should be considered as well, we can use
> > > mem_cgroup_protected() here to repclace task_under_memcg_protection()
> > > here.
> > >
> > > > 2) if the task is 200Gb large, does 10Mb memory protection make any
> > > > difference? if so, why would we respect it?
> > >
> > > Same with above, only consider it when the proctecion is enabled.
> >
> > Right, but memory.min is a number, not a boolean flag. It defines
> > how much memory is protected. You're using it as an on-off knob,
> > which is sub-optimal from my point of view.
> >
>
> I mean using mem_cgroup_protected(), sam with memory.min is
> implementad in the global reclaim path.
>
> > >
> > > > 3) if it works for global OOMs, why not memcg-level OOMs?
> > >
> > > memcg OOM is when the memory limit is reached and it can't find
> > > something to relcaim in the memcg and have to kill processes in this
> > > memcg.
> > > That is different with global OOM, because the global OOM can chose
> > > processes outside the memcg but the memcg OOM can't.
> >
> > Imagine the following hierarchy:
> >      /
> >      |
> >      A         memory.max = 10G, memory.min = 2G
> >     / \
> >    B   C       memory.min = 1G, memory.min = 0
> >
> > Say, you have memcg OOM in A, why B's memory min is not respected?
> > How it's different to the system-wide OOM?
> >
>
> Ah, this should be considered as well. Thanks for pointing out.
>
> > >
> > > > 4) if the task is prioritized to be killed by OOM (via oom_score_adj),
> > > > why even small memory.protection prevents it completely?
> > >
> > > Would you pls. show me some examples that when we will set both
> > > memory.min(meaning the porcesses in this memcg is very important) and
> > > higher oom_score_adj(meaning the porcesses in this memcg is not
> > > improtant at all) ?
> > > Note that the memory.min don't know which processes is important,
> > > while it only knows is if this process in this memcg.
> >
> > For instance, to prefer a specific process to be killed in case
> > of memcg OOM.
> > Also, memory.min can be used mostly to preserve the pagecache,
> > and an OOM kill means nothing but some anon memory leak.
> > In this case, it makes no sense to protect the leaked task.
> >
>
> But actually what memory.min protected is the memory usage, instead of
> pagecache,
> e.g. if the anon memory is higher than memory.min, then memroy.min
> can't protect file memory when swap is off.
>
> Even there is no anon memory leak, the OOM killer can also be invoked
> due to excess use of memroy.
> Plus, the memory.min can also protect the leaked anon memroy in
> current implementation.
>

BTW, if there are two different memcgs open the same file, the memcg
proection will not work if one memcg is protected while another memcg
is not protected.
But that may be a rare case.

> > >
> > > > 5) if there are two tasks similar in size and both protected,
> > > > should we prefer one with the smaller protection?
> > > > etc.
> > >
> > > Same with the answer in 1).
> >
> > So the problem is not that your patch is incorrect (or the idea is bad),
> > but you're defining a new policy, which will be impossible or very hard
> > to change further (as any other policy).
> >
> > So it's important to define it very well. Using the memory.min
> > number as a binary flag for selecting tasks seems a bit limited.
> >
> >
> > Thanks!