Re: [PATCH v2 2/2] mm, memcg: don't try to kill a process if memcg is not populated

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

On Mon, May 4, 2020 at 8:46 PM Michal Hocko <mhocko@kernel.org> wrote:
>
> On Mon 04-05-20 20:34:01, Yafang Shao wrote:
> > On Mon, May 4, 2020 at 4:18 PM Michal Hocko <mhocko@kernel.org> wrote:
> > >
> > > [It would be really great if a newer version was posted only after there
> > > was a wider consensus on the approach.]
> > >
> > > On Mon 04-05-20 00:26:21, Yafang Shao wrote:
> > > > Recently Shakeel reported a issue which also confused me several months
> > > > earlier. Bellow is his report -
> > > > Lowering memory.max can trigger an oom-kill if the reclaim does not
> > > > succeed. However if oom-killer does not find a process for killing, it
> > > > dumps a lot of warnings.
> > > > Deleting a memcg does not reclaim memory from it and the memory can
> > > > linger till there is a memory pressure. One normal way to proactively
> > > > reclaim such memory is to set memory.max to 0 just before deleting the
> > > > memcg. However if some of the memcg's memory is pinned by others, this
> > > > operation can trigger an oom-kill without any process and thus can log a
> > > > lot of un-needed warnings. So, ignore all such warnings from memory.max.
> > > >
> > > > A better way to avoid this issue is to avoid trying to kill a process if
> > > > memcg is not populated.
> > > > Note that OOM is different from OOM kill. OOM is a status that the
> > > > system or memcg is out of memory, while OOM kill is a result that a
> > > > process inside this memcg is killed when this memcg is in OOM status.
> > >
> > > Agreed.
> > >
> > > > That is the same reason why there're both MEMCG_OOM event and
> > > > MEMCG_OOM_KILL event. If we have already known that there's nothing to
> > > > kill, i.e. the memcg is not populated, then we don't need a try.
> > >
> > > OK, but you are not explaining why a silent failure is really better
> > > than no oom report under oom situation. With your patch, there is
> > > no failure reported to the user and there is also no sign that there
> > > might be a problem that memcg leaves memory behind that is not bound to
> > > any (killable) process. This could be an important information.
> > >
> >
> > That is not a silent failure. An oom event will be reported.
> > The user can get this event by memory.events or memory.events.local if
> > he really care about it.
>
> You are right. The oom situation will be reported (somehow) but the
> reason why no task has been killed might be several and there is no way
> to report no eligible tasks.
>
> > Especially when the admin set memory.max to 0 to drop all the caches,
> > many oom logs are a noise, besides that there are some side effect,
> > for example two many oom logs printed to a slow console may cause some
> > latency spike.
>
> But the oom situation and the oom report is simply something an admin
> has to expect especially when the hard limit is set to 0. With kmem
> accounting there is no guarantee that the target will be met.

I'm always wondering that why not moving the kmem from this memcg to
the root_mem_cgroup in this situation ?
Then this memcg can be easily reclaimed.

> >
> >
> > > Besides that I really do not see any actual problem that this would be
> > > fixing.
> >
> > Avoid printing two many oom logs.
>
> There is only a single oom report printed so I disagree this is really a
> proper justification.
>
> Unless you can come up with a better justification I am against this
> patch. It unnecessarily reduce debugging tools while it doesn't really
> provide any huge advantage. Changing the hard limit to impossible target
> is known to trigger the oom kernel and the oom report is a part of that.
> If the oom report is too noisy then we can discuss on how to make it
> more compact but making ad-hoc exceptions like this one is not a good
> solution.
> --

No better justification yet. But I think more memcg users will
complaining about it.

-- 
Thanks
Yafang