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

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

[Muchun Song]

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. When we drain per cpu stock, we also should do this.

The memcg_oom_recover() is small, so make it inline. And the parameter
of memcg cannot be NULL, so remove the check.

Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 mm/memcontrol.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 8c035846c7a4..8569f4dbea2a 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1925,7 +1925,7 @@ static int memcg_oom_wake_function(wait_queue_entry_t *wait,
 	return autoremove_wake_function(wait, mode, sync, arg);
 }
 
-static void memcg_oom_recover(struct mem_cgroup *memcg)
+static inline void memcg_oom_recover(struct mem_cgroup *memcg)
 {
 	/*
 	 * For the following lockless ->under_oom test, the only required
@@ -1935,7 +1935,7 @@ static void memcg_oom_recover(struct mem_cgroup *memcg)
 	 * achieved by invoking mem_cgroup_mark_under_oom() before
 	 * triggering notification.
 	 */
-	if (memcg && memcg->under_oom)
+	if (memcg->under_oom)
 		__wake_up(&memcg_oom_waitq, TASK_NORMAL, 0, memcg);
 }
 
@@ -2313,6 +2313,7 @@ static void drain_stock(struct memcg_stock_pcp *stock)
 		page_counter_uncharge(&old->memory, stock->nr_pages);
 		if (do_memsw_account())
 			page_counter_uncharge(&old->memsw, stock->nr_pages);
+		memcg_oom_recover(old);
 		stock->nr_pages = 0;
 	}
 
-- 
2.11.0

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

[Michal Hocko]

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 drain per cpu stock, we also should do this.

Can we have anything the per-cpu stock while entering the OOM path. IIRC
we do drain all cpus before entering oom path.

> The memcg_oom_recover() is small, so make it inline.

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.

> And the parameter
> of memcg cannot be NULL, so remove the check.

2bd9bb206b338 has introduced the check without any explanation
whatsoever. I indeed do not see any potential path to provide a NULL
memcg here. This is an internal function so the check is unnecessary
indeed. Please make it a patch on its own.

> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  mm/memcontrol.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 8c035846c7a4..8569f4dbea2a 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1925,7 +1925,7 @@ static int memcg_oom_wake_function(wait_queue_entry_t *wait,
>  	return autoremove_wake_function(wait, mode, sync, arg);
>  }
>  
> -static void memcg_oom_recover(struct mem_cgroup *memcg)
> +static inline void memcg_oom_recover(struct mem_cgroup *memcg)
>  {
>  	/*
>  	 * For the following lockless ->under_oom test, the only required
> @@ -1935,7 +1935,7 @@ static void memcg_oom_recover(struct mem_cgroup *memcg)
>  	 * achieved by invoking mem_cgroup_mark_under_oom() before
>  	 * triggering notification.
>  	 */
> -	if (memcg && memcg->under_oom)
> +	if (memcg->under_oom)
>  		__wake_up(&memcg_oom_waitq, TASK_NORMAL, 0, memcg);
>  }
>  
> @@ -2313,6 +2313,7 @@ static void drain_stock(struct memcg_stock_pcp *stock)
>  		page_counter_uncharge(&old->memory, stock->nr_pages);
>  		if (do_memsw_account())
>  			page_counter_uncharge(&old->memsw, stock->nr_pages);
> +		memcg_oom_recover(old);
>  		stock->nr_pages = 0;
>  	}
>  
> -- 
> 2.11.0

-- 
Michal Hocko
SUSE Labs

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

[Muchun Song]

On Fri, Feb 5, 2021 at 4:24 PM Michal Hocko <mhocko@suse.com> 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?

>
> > When we drain per cpu stock, we also should do this.
>
> Can we have anything the per-cpu stock while entering the OOM path. IIRC
> we do drain all cpus before entering oom path.

You are right. I did not notice this. Thank you.

>
> > The memcg_oom_recover() is small, so make it inline.
>
> 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>

>
> > And the parameter
> > of memcg cannot be NULL, so remove the check.
>
> 2bd9bb206b338 has introduced the check without any explanation
> whatsoever. I indeed do not see any potential path to provide a NULL
> memcg here. This is an internal function so the check is unnecessary
> indeed. Please make it a patch on its own.

OK. Will do this. Thanks.

>
> > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > ---
> >  mm/memcontrol.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 8c035846c7a4..8569f4dbea2a 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -1925,7 +1925,7 @@ static int memcg_oom_wake_function(wait_queue_entry_t *wait,
> >       return autoremove_wake_function(wait, mode, sync, arg);
> >  }
> >
> > -static void memcg_oom_recover(struct mem_cgroup *memcg)
> > +static inline void memcg_oom_recover(struct mem_cgroup *memcg)
> >  {
> >       /*
> >        * For the following lockless ->under_oom test, the only required
> > @@ -1935,7 +1935,7 @@ static void memcg_oom_recover(struct mem_cgroup *memcg)
> >        * achieved by invoking mem_cgroup_mark_under_oom() before
> >        * triggering notification.
> >        */
> > -     if (memcg && memcg->under_oom)
> > +     if (memcg->under_oom)
> >               __wake_up(&memcg_oom_waitq, TASK_NORMAL, 0, memcg);
> >  }
> >
> > @@ -2313,6 +2313,7 @@ static void drain_stock(struct memcg_stock_pcp *stock)
> >               page_counter_uncharge(&old->memory, stock->nr_pages);
> >               if (do_memsw_account())
> >                       page_counter_uncharge(&old->memsw, stock->nr_pages);
> > +             memcg_oom_recover(old);
> >               stock->nr_pages = 0;
> >       }
> >
> > --
> > 2.11.0
>
> --
> Michal Hocko
> SUSE Labs

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

[Michal Hocko]

On Fri 05-02-21 17:55:10, Muchun Song wrote:
> On Fri, Feb 5, 2021 at 4:24 PM Michal Hocko <mhocko@suse.com> 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?

> > > When we drain per cpu stock, we also should do this.
> >
> > Can we have anything the per-cpu stock while entering the OOM path. IIRC
> > we do drain all cpus before entering oom path.
> 
> You are right. I did not notice this. Thank you.
> 
> >
> > > The memcg_oom_recover() is small, so make it inline.
> >
> > 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?
-- 
Michal Hocko
SUSE Labs

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

[Muchun Song]

On Fri, Feb 5, 2021 at 6:21 PM Michal Hocko <mhocko@suse.com> wrote:
>
> On Fri 05-02-21 17:55:10, Muchun Song wrote:
> > On Fri, Feb 5, 2021 at 4:24 PM Michal Hocko <mhocko@suse.com> 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.
I see uncharge_batch always do this. I am confused why
__memcg_kmem_uncharge does not. Both paths do the same
thing (uncharge pages). So actually, this patch want to keep
the two paths consistent. Thanks.

>
> > > > When we drain per cpu stock, we also should do this.
> > >
> > > Can we have anything the per-cpu stock while entering the OOM path. IIRC
> > > we do drain all cpus before entering oom path.
> >
> > You are right. I did not notice this. Thank you.
> >
> > >
> > > > The memcg_oom_recover() is small, so make it inline.
> > >
> > > 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.

> --
> Michal Hocko
> SUSE Labs

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@suse.com> wrote:
> >
> > On Fri 05-02-21 17:55:10, Muchun Song wrote:
> > > On Fri, Feb 5, 2021 at 4:24 PM Michal Hocko <mhocko@suse.com> 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

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

[Muchun Song]

On Fri, Feb 5, 2021 at 8:20 PM Michal Hocko <mhocko@suse.com> wrote:
>
> On Fri 05-02-21 19:04:19, Muchun Song wrote:
> > On Fri, Feb 5, 2021 at 6:21 PM Michal Hocko <mhocko@suse.com> wrote:
> > >
> > > On Fri 05-02-21 17:55:10, Muchun Song wrote:
> > > > On Fri, Feb 5, 2021 at 4:24 PM Michal Hocko <mhocko@suse.com> 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.

Thanks for your explanations. You are right. It is my fault to depend
on the draining. I should call memcg_oom_recover directly in the
__memcg_kmem_uncharge. Right?

>
> [...]
> > > > > 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)?

Agree. I think it can improve performance when this
function is inline. Guarding the test should be also
an improvement on cgroup v2.


> --
> Michal Hocko
> SUSE Labs

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

[Michal Hocko]

On Fri 05-02-21 23:30:36, Muchun Song wrote:
> On Fri, Feb 5, 2021 at 8:20 PM Michal Hocko <mhocko@suse.com> wrote:
> >
> > On Fri 05-02-21 19:04:19, Muchun Song wrote:
> > > On Fri, Feb 5, 2021 at 6:21 PM Michal Hocko <mhocko@suse.com> wrote:
> > > >
> > > > On Fri 05-02-21 17:55:10, Muchun Song wrote:
> > > > > On Fri, Feb 5, 2021 at 4:24 PM Michal Hocko <mhocko@suse.com> 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.
> 
> Thanks for your explanations. You are right. It is my fault to depend
> on the draining. I should call memcg_oom_recover directly in the
> __memcg_kmem_uncharge. Right?

Yes.

> > [...]
> > > > > > 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)?
> 
> Agree. I think it can improve performance when this
> function is inline. Guarding the test should be also
> an improvement on cgroup v2.

I would be surprised if this was measurable but you can give it a try. A
static key would be a reasonable argument for inlining on its own.
-- 
Michal Hocko
SUSE Labs