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 <[email protected]>
---
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);
}
Reviewed-by: Alex Shi <[email protected]>
?? 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 <[email protected]>
> ---
>
> 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);
> }
>
>
> 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 <[email protected]>
Acked-by: Roman Gushchin <[email protected]>
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);
> }
>
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 <[email protected]>
> ---
>
> 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 <[email protected]>
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 <[email protected]>
Thanks.
Acked-by: Chris Down <[email protected]>
On Wed, 6 Jan 2021 14:49:35 +0800 Baoquan He <[email protected]> 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.
On Wed, 6 Jan 2021 14:49:35 +0800 Baoquan He <[email protected]> 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?
On 01/06/21 at 11:35am, Andrew Morton wrote:
> On Wed, 6 Jan 2021 14:49:35 +0800 Baoquan He <[email protected]> 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
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 <[email protected]>
Acked-by: Vlastimil Babka <[email protected]>
> ---
>
> 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);
> }
>
>
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 <[email protected]>
>
> Acked-by: Vlastimil Babka <[email protected]>
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);
> > }
> >
> >