[PATCH] mm/memcontrol: fix warning in mem_cgroup_page_lruvec()

[PATCH] mm/memcontrol: fix warning in mem_cgroup_page_lruvec()

[Hugh Dickins]

Boot a CONFIG_MEMCG=y kernel with "cgroup_disabled=memory" and you are
met by a series of warnings from the VM_WARN_ON_ONCE_PAGE(!memcg, page)
recently added to the inline mem_cgroup_page_lruvec().

An earlier attempt to place that warning, in mem_cgroup_lruvec(), had
been careful to do so after weeding out the mem_cgroup_disabled() case;
but was itself invalid because of the mem_cgroup_lruvec(NULL, pgdat) in
clear_pgdat_congested() and age_active_anon().

Warning in mem_cgroup_page_lruvec() was once useful in detecting a KSM
charge bug, so may be worth keeping: but skip if mem_cgroup_disabled().

Fixes: 9a1ac2288cf1 ("mm/memcontrol:rewrite mem_cgroup_page_lruvec()")
Signed-off-by: Hugh Dickins <hughd@google.com>
---

 include/linux/memcontrol.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- 5.11-rc2/include/linux/memcontrol.h	2020-12-27 20:39:36.751923135 -0800
+++ linux/include/linux/memcontrol.h	2021-01-03 19:38:24.822978559 -0800
@@ -665,7 +665,7 @@ static inline struct lruvec *mem_cgroup_
 {
 	struct mem_cgroup *memcg = page_memcg(page);
 
-	VM_WARN_ON_ONCE_PAGE(!memcg, page);
+	VM_WARN_ON_ONCE_PAGE(!memcg && !mem_cgroup_disabled(), page);
 	return mem_cgroup_lruvec(memcg, pgdat);
 }
 

Re: [PATCH] mm/memcontrol: fix warning in mem_cgroup_page_lruvec()

[Alex Shi]

Reviewed-by: Alex Shi <alex.shi@linux.alibaba.com>

在 2021/1/4 下午1:03, Hugh Dickins 写道:
> Boot a CONFIG_MEMCG=y kernel with "cgroup_disabled=memory" and you are
> met by a series of warnings from the VM_WARN_ON_ONCE_PAGE(!memcg, page)
> recently added to the inline mem_cgroup_page_lruvec().
> 
> An earlier attempt to place that warning, in mem_cgroup_lruvec(), had
> been careful to do so after weeding out the mem_cgroup_disabled() case;
> but was itself invalid because of the mem_cgroup_lruvec(NULL, pgdat) in
> clear_pgdat_congested() and age_active_anon().
> 
> Warning in mem_cgroup_page_lruvec() was once useful in detecting a KSM
> charge bug, so may be worth keeping: but skip if mem_cgroup_disabled().
> 
> Fixes: 9a1ac2288cf1 ("mm/memcontrol:rewrite mem_cgroup_page_lruvec()")
> Signed-off-by: Hugh Dickins <hughd@google.com>
> ---
> 
>  include/linux/memcontrol.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- 5.11-rc2/include/linux/memcontrol.h	2020-12-27 20:39:36.751923135 -0800
> +++ linux/include/linux/memcontrol.h	2021-01-03 19:38:24.822978559 -0800
> @@ -665,7 +665,7 @@ static inline struct lruvec *mem_cgroup_
>  {
>  	struct mem_cgroup *memcg = page_memcg(page);
>  
> -	VM_WARN_ON_ONCE_PAGE(!memcg, page);
> +	VM_WARN_ON_ONCE_PAGE(!memcg && !mem_cgroup_disabled(), page);
>  	return mem_cgroup_lruvec(memcg, pgdat);
>  }
>  
> 

Re: [PATCH] mm/memcontrol: fix warning in mem_cgroup_page_lruvec()

[Baoquan He]

On 01/03/21 at 09:03pm, Hugh Dickins wrote:
> Boot a CONFIG_MEMCG=y kernel with "cgroup_disabled=memory" and you are
> met by a series of warnings from the VM_WARN_ON_ONCE_PAGE(!memcg, page)
> recently added to the inline mem_cgroup_page_lruvec().
> 
> An earlier attempt to place that warning, in mem_cgroup_lruvec(), had
> been careful to do so after weeding out the mem_cgroup_disabled() case;
> but was itself invalid because of the mem_cgroup_lruvec(NULL, pgdat) in
> clear_pgdat_congested() and age_active_anon().
> 
> Warning in mem_cgroup_page_lruvec() was once useful in detecting a KSM
> charge bug, so may be worth keeping: but skip if mem_cgroup_disabled().
> 
> Fixes: 9a1ac2288cf1 ("mm/memcontrol:rewrite mem_cgroup_page_lruvec()")
> Signed-off-by: Hugh Dickins <hughd@google.com>
> ---
> 
>  include/linux/memcontrol.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- 5.11-rc2/include/linux/memcontrol.h	2020-12-27 20:39:36.751923135 -0800
> +++ linux/include/linux/memcontrol.h	2021-01-03 19:38:24.822978559 -0800
> @@ -665,7 +665,7 @@ static inline struct lruvec *mem_cgroup_
>  {
>  	struct mem_cgroup *memcg = page_memcg(page);
>  
> -	VM_WARN_ON_ONCE_PAGE(!memcg, page);
> +	VM_WARN_ON_ONCE_PAGE(!memcg && !mem_cgroup_disabled(), page);
>  	return mem_cgroup_lruvec(memcg, pgdat);

Thanks for fixing this. We also encountered this issue in kdump kernel
with the mainline 5.10 kernel since 'cgroup_disable=memory' is added.

Reviewed-by: Baoquan He <bhe@redhat.com>

Re: [PATCH] mm/memcontrol: fix warning in mem_cgroup_page_lruvec()

[Andrew Morton]

On Wed, 6 Jan 2021 14:49:35 +0800 Baoquan He <bhe@redhat.com> wrote:

> > --- 5.11-rc2/include/linux/memcontrol.h	2020-12-27 20:39:36.751923135 -0800
> > +++ linux/include/linux/memcontrol.h	2021-01-03 19:38:24.822978559 -0800
> > @@ -665,7 +665,7 @@ static inline struct lruvec *mem_cgroup_
> >  {
> >  	struct mem_cgroup *memcg = page_memcg(page);
> >  
> > -	VM_WARN_ON_ONCE_PAGE(!memcg, page);
> > +	VM_WARN_ON_ONCE_PAGE(!memcg && !mem_cgroup_disabled(), page);
> >  	return mem_cgroup_lruvec(memcg, pgdat);
> 
> Thanks for fixing this. We also encountered this issue in kdump kernel
> with the mainline 5.10 kernel since 'cgroup_disable=memory' is added.

Thanks - I added the cc:stable.

Re: [PATCH] mm/memcontrol: fix warning in mem_cgroup_page_lruvec()

[Andrew Morton]

On Wed, 6 Jan 2021 14:49:35 +0800 Baoquan He <bhe@redhat.com> wrote:

> > Fixes: 9a1ac2288cf1 ("mm/memcontrol:rewrite mem_cgroup_page_lruvec()")
> 
> ...
>
> Thanks for fixing this. We also encountered this issue in kdump kernel
> with the mainline 5.10 kernel since 'cgroup_disable=memory' is added.

Wait.  9a1ac2288cf1 isn't present in 5.10?

Re: [PATCH] mm/memcontrol: fix warning in mem_cgroup_page_lruvec()

[Baoquan He]

On 01/06/21 at 11:35am, Andrew Morton wrote:
> On Wed, 6 Jan 2021 14:49:35 +0800 Baoquan He <bhe@redhat.com> wrote:
> 
> > > Fixes: 9a1ac2288cf1 ("mm/memcontrol:rewrite mem_cgroup_page_lruvec()")
> > 
> > ...
> >
> > Thanks for fixing this. We also encountered this issue in kdump kernel
> > with the mainline 5.10 kernel since 'cgroup_disable=memory' is added.
> 
> Wait.  9a1ac2288cf1 isn't present in 5.10?
> 

Yes, just checked, commit 9a1ac2288cf1 was merged in 5.11-rc1, not in
5.10.0. Seems Redhat CKI doesn't treat the kernel release correctly, it
calls all kernel 5.11-rcx as 5.10.0. Sorry for the confusion, I will
send mail to them to change this.

I got the failure report from redhat's CKI test, the kernel repo is as
below, but the subject of failure report and 'uname -r' told it's
5.10.0.

       Kernel repo: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
            Commit: 58cf05f597b0 - Merge tag 'sound-fix-5.11-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound

Re: [PATCH] mm/memcontrol: fix warning in mem_cgroup_page_lruvec()

[Chris Down]

Hugh Dickins writes:
>Boot a CONFIG_MEMCG=y kernel with "cgroup_disabled=memory" and you are
>met by a series of warnings from the VM_WARN_ON_ONCE_PAGE(!memcg, page)
>recently added to the inline mem_cgroup_page_lruvec().
>
>An earlier attempt to place that warning, in mem_cgroup_lruvec(), had
>been careful to do so after weeding out the mem_cgroup_disabled() case;
>but was itself invalid because of the mem_cgroup_lruvec(NULL, pgdat) in
>clear_pgdat_congested() and age_active_anon().
>
>Warning in mem_cgroup_page_lruvec() was once useful in detecting a KSM
>charge bug, so may be worth keeping: but skip if mem_cgroup_disabled().
>
>Fixes: 9a1ac2288cf1 ("mm/memcontrol:rewrite mem_cgroup_page_lruvec()")
>Signed-off-by: Hugh Dickins <hughd@google.com>

Thanks.

Acked-by: Chris Down <chris@chrisdown.name>

Re: [PATCH] mm/memcontrol: fix warning in mem_cgroup_page_lruvec()

[Vlastimil Babka]

On 1/4/21 6:03 AM, Hugh Dickins wrote:
> Boot a CONFIG_MEMCG=y kernel with "cgroup_disabled=memory" and you are
> met by a series of warnings from the VM_WARN_ON_ONCE_PAGE(!memcg, page)
> recently added to the inline mem_cgroup_page_lruvec().
> 
> An earlier attempt to place that warning, in mem_cgroup_lruvec(), had
> been careful to do so after weeding out the mem_cgroup_disabled() case;
> but was itself invalid because of the mem_cgroup_lruvec(NULL, pgdat) in
> clear_pgdat_congested() and age_active_anon().
> 
> Warning in mem_cgroup_page_lruvec() was once useful in detecting a KSM
> charge bug, so may be worth keeping: but skip if mem_cgroup_disabled().
> 
> Fixes: 9a1ac2288cf1 ("mm/memcontrol:rewrite mem_cgroup_page_lruvec()")
> Signed-off-by: Hugh Dickins <hughd@google.com>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

> ---
> 
>  include/linux/memcontrol.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- 5.11-rc2/include/linux/memcontrol.h	2020-12-27 20:39:36.751923135 -0800
> +++ linux/include/linux/memcontrol.h	2021-01-03 19:38:24.822978559 -0800
> @@ -665,7 +665,7 @@ static inline struct lruvec *mem_cgroup_
>  {
>  	struct mem_cgroup *memcg = page_memcg(page);
>  
> -	VM_WARN_ON_ONCE_PAGE(!memcg, page);
> +	VM_WARN_ON_ONCE_PAGE(!memcg && !mem_cgroup_disabled(), page);

Nit: I would reverse the order of conditions as mem_cgroup_disabled() is either
"return true" or a static key. Not that it matters too much on DEBUG_VM configs...

>  	return mem_cgroup_lruvec(memcg, pgdat);
>  }
>  
> 

Re: [PATCH] mm/memcontrol: fix warning in mem_cgroup_page_lruvec()

[Hugh Dickins]

On Thu, 7 Jan 2021, Vlastimil Babka wrote:
> On 1/4/21 6:03 AM, Hugh Dickins wrote:
> > Boot a CONFIG_MEMCG=y kernel with "cgroup_disabled=memory" and you are
> > met by a series of warnings from the VM_WARN_ON_ONCE_PAGE(!memcg, page)
> > recently added to the inline mem_cgroup_page_lruvec().
> > 
> > An earlier attempt to place that warning, in mem_cgroup_lruvec(), had
> > been careful to do so after weeding out the mem_cgroup_disabled() case;
> > but was itself invalid because of the mem_cgroup_lruvec(NULL, pgdat) in
> > clear_pgdat_congested() and age_active_anon().
> > 
> > Warning in mem_cgroup_page_lruvec() was once useful in detecting a KSM
> > charge bug, so may be worth keeping: but skip if mem_cgroup_disabled().
> > 
> > Fixes: 9a1ac2288cf1 ("mm/memcontrol:rewrite mem_cgroup_page_lruvec()")
> > Signed-off-by: Hugh Dickins <hughd@google.com>
> 
> Acked-by: Vlastimil Babka <vbabka@suse.cz>

Thanks.

> 
> > ---
> > 
> >  include/linux/memcontrol.h |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > --- 5.11-rc2/include/linux/memcontrol.h	2020-12-27 20:39:36.751923135 -0800
> > +++ linux/include/linux/memcontrol.h	2021-01-03 19:38:24.822978559 -0800
> > @@ -665,7 +665,7 @@ static inline struct lruvec *mem_cgroup_
> >  {
> >  	struct mem_cgroup *memcg = page_memcg(page);
> >  
> > -	VM_WARN_ON_ONCE_PAGE(!memcg, page);
> > +	VM_WARN_ON_ONCE_PAGE(!memcg && !mem_cgroup_disabled(), page);
> 
> Nit: I would reverse the order of conditions as mem_cgroup_disabled() is either
> "return true" or a static key. Not that it matters too much on DEBUG_VM configs...

tl;dr I'm going to leave the patch as is.

You are certainly right that I was forgetting the static-key-ness of
mem_cgroup_disabled() when I put the tests that way round: I was thinking
of the already-in-a-register-ness of "memcg"; but had also not realized
that page_memcg() just did an "&", so condition bits nicely set already.

And I think you are right in principle, that the tests should be better
the way you suggest, when static key is in use - in the (unusual)
mem_cgroup_disabled() case, though not in the usual enabled case.

I refuse to confess how many hours I've spent poring over "objdump -ld"s
of lock_page_lruvec_irqsave(), and comparing with how it is patched when
the kernel is booted with "cgroup_disable=memory".

But I have seen builds where my way round worked out better than yours,
for both the enabled and disabled cases (SUSE gcc 9.3.1 was good, in
the config I was trying on it); and builds where disabled was treated
rather poorly my way (with external call to mem_cgroup_disabled() from
lock_page_lruvec() and lock_page_lruvec_irqsave(), but inlined into
lock_page_lruvec_irq() - go figure! - with SUSE gcc 10.2.1).

I suspect a lot depends on what inlining is done, and on that prior
page_memcg() doing its "&", and the second mem_cgroup_disabled() which
follows immediately in mem_cgroup_lruvec(): different compilers will
make different choices, favouring one or the other ordering.

I've grown rather tired of it all (and discovered on the way that
static keys depend on CONFIG_JUMP_LABEL=y, which I didn't have in
a config I've carried forward through "make oldconfig"s for years -
thanks); but not found a decisive reason to change the patch.

Hugh

> 
> >  	return mem_cgroup_lruvec(memcg, pgdat);
> >  }
> >  
> > 

Re: [PATCH] mm/memcontrol: fix warning in mem_cgroup_page_lruvec()

[Roman Gushchin]

> Boot a CONFIG_MEMCG=y kernel with "cgroup_disabled=memory" and you are
> met by a series of warnings from the VM_WARN_ON_ONCE_PAGE(!memcg, page)
> recently added to the inline mem_cgroup_page_lruvec().
> 
> An earlier attempt to place that warning, in mem_cgroup_lruvec(), had
> been careful to do so after weeding out the mem_cgroup_disabled() case;
> but was itself invalid because of the mem_cgroup_lruvec(NULL, pgdat) in
> clear_pgdat_congested() and age_active_anon().
> 
> Warning in mem_cgroup_page_lruvec() was once useful in detecting a KSM
> charge bug, so may be worth keeping: but skip if mem_cgroup_disabled().
> 
> Fixes: 9a1ac2288cf1 ("mm/memcontrol:rewrite mem_cgroup_page_lruvec()")
> Signed-off-by: Hugh Dickins <hughd@google.com>

Acked-by: Roman Gushchin <guro@fb.com>

Thanks!

> ---
> 
>  include/linux/memcontrol.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- 5.11-rc2/include/linux/memcontrol.h	2020-12-27 20:39:36.751923135 -0800
> +++ linux/include/linux/memcontrol.h	2021-01-03 19:38:24.822978559 -0800
> @@ -665,7 +665,7 @@ static inline struct lruvec *mem_cgroup_
>  {
>  	struct mem_cgroup *memcg = page_memcg(page);
>  
> -	VM_WARN_ON_ONCE_PAGE(!memcg, page);
> +	VM_WARN_ON_ONCE_PAGE(!memcg && !mem_cgroup_disabled(), page);
>  	return mem_cgroup_lruvec(memcg, pgdat);
>  }
>