Re: [External] Re: [PATCH] mm: memcontrol: fix missing wakeup oom task

[Michal Hocko]

On Fri 05-02-21 19:04:19, Muchun Song wrote:
> On Fri, Feb 5, 2021 at 6:21 PM Michal Hocko <mhocko@xxxxxxxx> wrote:
> >
> > On Fri 05-02-21 17:55:10, Muchun Song wrote:
> > > On Fri, Feb 5, 2021 at 4:24 PM Michal Hocko <mhocko@xxxxxxxx> wrote:
> > > >
> > > > On Fri 05-02-21 14:23:10, Muchun Song wrote:
> > > > > We call memcg_oom_recover() in the uncharge_batch() to wakeup OOM task
> > > > > when page uncharged, but for the slab pages, we do not do this when page
> > > > > uncharged.
> > > >
> > > > How does the patch deal with this?
> > >
> > > When we uncharge a slab page via __memcg_kmem_uncharge,
> > > actually, this path forgets to do this for us compared to
> > > uncharge_batch(). Right?
> >
> > Yes this was more more or less clear (still would have been nicer to be
> > explicit). But you still haven't replied to my question I believe. I
> > assume you rely on refill_stock doing draining but how does this address
> > the problem? Is it sufficient to do wakeups in the batched way?
> 
> Sorry, the subject title may not be suitable. IIUC, memcg_oom_recover
> aims to wake up the OOM task when we uncharge the page.

Yes, your understanding is correct. This is a way to pro-actively wake
up oom victims when the memcg oom handling is outsourced to the
userspace. Please note that I haven't objected to the problem statement.

I was questioning the fix for the problem.

> I see uncharge_batch always do this. I am confused why
> __memcg_kmem_uncharge does not.

Very likely an omission. I haven't checked closely but I suspect this
has been introduced by the recent kmem accounting changes.

Why didn't you simply do the same thing and call memcg_oom_recover
unconditionally and instead depend on the draining? I suspect this was
because you wanted to recover also when draining which is not necessary
as pointed out in other email.

[...]
> > > > Does this lead to any code generation improvements? I would expect
> > > > compiler to be clever enough to inline static functions if that pays
> > > > off. If yes make this a patch on its own.
> > >
> > > I have disassembled the code, I see memcg_oom_recover is not
> > > inline. Maybe because memcg_oom_recover has a lot of callers.
> > > Just guess.
> > >
> > > (gdb) disassemble uncharge_batch
> > >  [...]
> > >  0xffffffff81341c73 <+227>: callq  0xffffffff8133c420 <page_counter_uncharge>
> > >  0xffffffff81341c78 <+232>: jmpq   0xffffffff81341bc0 <uncharge_batch+48>
> > >  0xffffffff81341c7d <+237>: callq  0xffffffff8133e2c0 <memcg_oom_recover>
> >
> > So does it really help to do the inlining?
> 
> I just think memcg_oom_recover is very small, inline maybe
> a good choice. Maybe I am wrong.

In general I am not overly keen on changes without a proper
justification. In this particular case I would understand that a
function call that will almost never do anything but the test (because
oom_disabled is a rarely used) is just waste of cycles in some hot
paths (e.g. kmem uncharge). Maybe this even has some visible performance
benefit. If this is really the case then would it make sense to guard
this test by the existing cgroup_subsys_on_dfl(memory_cgrp_subsys)?
-- 
Michal Hocko
SUSE Labs