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@xxxxxxxxxx>
> 
> Acked-by: Vlastimil Babka <vbabka@xxxxxxx>

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);
> >  }
> >  
> >