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
