Re: [PATCH] mm: correctly charge compressed memory to its memcg

[Yosry Ahmed <yosryahmed@google.com>]

On Sun, Aug 14, 2022 at 7:52 PM Li,Liguang <liliguang@baidu.com> wrote:
>
> 在 2022/8/13 上午8:44，“Yosry Ahmed”<yosryahmed@google.com> 写入:
>
> > On Fri, Aug 12, 2022 at 2:56 PM Shakeel Butt <shakeelb@google.com> wrote:
> > >
> > > +Andrew & linux-mm
> > >
> > > On Thu, Aug 11, 2022 at 5:12 PM Roman Gushchin <roman.gushchin@linux.dev> wrote:
> > > >
> > > > On Thu, Aug 11, 2022 at 04:19:13PM +0800, liliguang wrote:
> > > > > From: Li Liguang <liliguang@baidu.com>
> > > > >
> > > > > Kswapd will reclaim memory when memory pressure is high, the
> > > > > annonymous memory will be compressed and stored in the zpool
> > > > > if zswap is enabled. The memcg_kmem_bypass() in
> > > > > get_obj_cgroup_from_page() will bypass the kernel thread and
> > > > > cause the compressed memory not charged to its memory cgroup.
> > > > >
> > > > > Remove the memcg_kmem_bypass() and properly charge compressed
> > > > > memory to its corresponding memory cgroup.
> > > > >
> > > > > Signed-off-by: Li Liguang <liliguang@baidu.com>
> > > > > ---
> > > > >  mm/memcontrol.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > > > index b69979c9ced5..6a95ea7c5ee7 100644
> > > > > --- a/mm/memcontrol.c
> > > > > +++ b/mm/memcontrol.c
> > > > > @@ -2971,7 +2971,7 @@ struct obj_cgroup *get_obj_cgroup_from_page(struct page *page)
> > > > >  {
> > > > >       struct obj_cgroup *objcg;
> > > > >
> > > > > -     if (!memcg_kmem_enabled() || memcg_kmem_bypass())
> > > > > +     if (!memcg_kmem_enabled())
> >
> >
> > Won't the memcg_kmem_enabled() check also cause a problem in that same
> > scenario (e.g. if CONFIG_MEMCG_KMEM=n)? or am I missing something
> > here?
> >
>
> Please notes that the return value is a pointer to obj_cgroup, not memcg.
> If CONFIG_MEMCG_KMEM=n or memcg kmem charge is disabled, the NULL need
> to be returned.
>

Right. I am not implying that the check should be removed, or that
this is something this patch should address for that matter.

I just realized while looking at this patch that because we are using
objcg in zswap charging, it is dependent on memcg kmem charging. I am
not sure I understand if such dependency is needed? IIUC swapped out
pages hold references to the memcg they are charged to anyway, so why
do we need to use objcgs in charging zswap? I feel like I am missing
something.

> > >
> > > > >               return NULL;
> > > > >
> > > > >       if (PageMemcgKmem(page)) {
> > > > > --
> > > > > 2.32.0 (Apple Git-132)
> > > > >
> > > >
> > > > Hi Li!
> > > >
> > > > The fix looks good to me! As we get objcg pointer from a page and not from
> > > > the current task, memcg_kmem_bypass() doesn't makes much sense.
> > > >
> > > > Acked-by: Roman Gushchin <roman.gushchin@linux.dev>
> > > >
> > > > Probably, we need to add
> > > > Fixes: f4840ccfca25 ("zswap: memcg accounting")
> > > >
> > > > Thank you!
> > >
> > > You can add:
> > >
> > > Acked-by: Shakeel Butt <shakeelb@google.com>
> > >
>