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

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

[Shakeel Butt]

+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())
> >               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>

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

[Yosry Ahmed]

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?

>
> > >               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>
>

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

[Li,Liguang]

在 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.

> >
> > > >               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>
> >

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

[Yosry Ahmed]

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>
> > >
>

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

[Li,Liguang]

> 在 2022/8/15 下午4:10，“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.
> 

The compressed size of swapped out pages is nearly a quarter of its RAM,
and a page in the zswap can store multiple compressed swapped out
pages. So objcg is used here. 

Please check this post for more information.
https://lore.kernel.org/lkml/20220510152847.230957-1-hannes@cmpxchg.org/T/#mbd0254ffd377bf843ac50850bf0a6d41505a925a

> > > >
> > > > > >               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>
> > > >
> >

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

[Yosry Ahmed]

On Mon, Aug 15, 2022 at 4:48 AM Li,Liguang <liliguang@baidu.com> wrote:
>
>
> > 在 2022/8/15 下午4:10，“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.
> >
>
> The compressed size of swapped out pages is nearly a quarter of its RAM,
> and a page in the zswap can store multiple compressed swapped out
> pages. So objcg is used here.
>
> Please check this post for more information.
> https://lore.kernel.org/lkml/20220510152847.230957-1-hannes@cmpxchg.org/T/#mbd0254ffd377bf843ac50850bf0a6d41505a925a

Yeah I understand this much, what I don't understand is why we charge
the zswap memory through objcg (thus tying it to memcg kmem charging)
rather than directly through memcg.

>
> > > > >
> > > > > > >               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>
> > > > >
> > >
>

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

[Johannes Weiner]

On Mon, Aug 15, 2022 at 06:46:46AM -0700, Yosry Ahmed wrote:
> Yeah I understand this much, what I don't understand is why we charge
> the zswap memory through objcg (thus tying it to memcg kmem charging)
> rather than directly through memcg.

The charged quantities are smaller than a page, so we have to use the
byte interface.

The byte interface (objcg) was written for slab originally, hence the
link to the kmem option. But note that CONFIG_MEMCG_KMEM is no longer
a user-visible option, and for all intents and purposes a fixed part
of CONFIG_MEMCG.

(There is the SLOB quirk. But I'm not sure anybody uses slob, let
alone slob + memcg.)

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

[Yosry Ahmed]

On Mon, Aug 15, 2022 at 8:31 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Mon, Aug 15, 2022 at 06:46:46AM -0700, Yosry Ahmed wrote:
> > Yeah I understand this much, what I don't understand is why we charge
> > the zswap memory through objcg (thus tying it to memcg kmem charging)
> > rather than directly through memcg.
>
> The charged quantities are smaller than a page, so we have to use the
> byte interface.
>
> The byte interface (objcg) was written for slab originally, hence the
> link to the kmem option. But note that CONFIG_MEMCG_KMEM is no longer
> a user-visible option, and for all intents and purposes a fixed part
> of CONFIG_MEMCG.
>
> (There is the SLOB quirk. But I'm not sure anybody uses slob, let
> alone slob + memcg.)

Thanks for the clarification, it makes sense to use the byte interface
here for this, and thanks for pointing out that CONFIC_MEMCG_KMEM is
not part of CONFIG_MEMCG.

One more question :) memcg kmem charging can still be disabled even
with !CONFIG_MEMCG_KMEM, right? In this case zswap charging will also
be off, which seems like an unintended side effect, right?

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

[Yosry Ahmed]

On Mon, Aug 15, 2022 at 8:42 AM Yosry Ahmed <yosryahmed@google.com> wrote:
>
> On Mon, Aug 15, 2022 at 8:31 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
> >
> > On Mon, Aug 15, 2022 at 06:46:46AM -0700, Yosry Ahmed wrote:
> > > Yeah I understand this much, what I don't understand is why we charge
> > > the zswap memory through objcg (thus tying it to memcg kmem charging)
> > > rather than directly through memcg.
> >
> > The charged quantities are smaller than a page, so we have to use the
> > byte interface.
> >
> > The byte interface (objcg) was written for slab originally, hence the
> > link to the kmem option. But note that CONFIG_MEMCG_KMEM is no longer
> > a user-visible option, and for all intents and purposes a fixed part
> > of CONFIG_MEMCG.
> >
> > (There is the SLOB quirk. But I'm not sure anybody uses slob, let
> > alone slob + memcg.)
>
> Thanks for the clarification, it makes sense to use the byte interface
> here for this, and thanks for pointing out that CONFIC_MEMCG_KMEM is
> not part of CONFIG_MEMCG.
>
> One more question :) memcg kmem charging can still be disabled even
> with !CONFIG_MEMCG_KMEM, right? In this case zswap charging will also
> be off, which seems like an unintended side effect, right?

memcg kmem charging can still be disabled even
with CONFIG_MEMCG_KMEM***

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

[Johannes Weiner]

On Mon, Aug 15, 2022 at 09:16:08AM -0700, Yosry Ahmed wrote:
> On Mon, Aug 15, 2022 at 8:42 AM Yosry Ahmed <yosryahmed@google.com> wrote:
> >
> > On Mon, Aug 15, 2022 at 8:31 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
> > >
> > > On Mon, Aug 15, 2022 at 06:46:46AM -0700, Yosry Ahmed wrote:
> > > > Yeah I understand this much, what I don't understand is why we charge
> > > > the zswap memory through objcg (thus tying it to memcg kmem charging)
> > > > rather than directly through memcg.
> > >
> > > The charged quantities are smaller than a page, so we have to use the
> > > byte interface.
> > >
> > > The byte interface (objcg) was written for slab originally, hence the
> > > link to the kmem option. But note that CONFIG_MEMCG_KMEM is no longer
> > > a user-visible option, and for all intents and purposes a fixed part
> > > of CONFIG_MEMCG.
> > >
> > > (There is the SLOB quirk. But I'm not sure anybody uses slob, let
> > > alone slob + memcg.)
> >
> > Thanks for the clarification, it makes sense to use the byte interface
> > here for this, and thanks for pointing out that CONFIC_MEMCG_KMEM is
> > not part of CONFIG_MEMCG.
> >
> > One more question :) memcg kmem charging can still be disabled even
> > with !CONFIG_MEMCG_KMEM, right? In this case zswap charging will also
> > be off, which seems like an unintended side effect, right?
> 
> memcg kmem charging can still be disabled even
> with CONFIG_MEMCG_KMEM***

Yes, indeed, if the host is booted with the nokmem flag. Doing so will
turn off slab, percpu, and (as of recently) zswap.

The zswap backing storage *is* kernel memory, so that seems like the
correct semantics for the flag.

That said, the distinction between kernel and user memory is becoming
increasingly odd. The more kernel memory we track, the more ridiculous
the size of the hole you punch into resource control by disabling it.

Maybe we should just deprecate that knob altogether.

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

[Yosry Ahmed]

On Mon, Aug 15, 2022 at 10:47 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Mon, Aug 15, 2022 at 09:16:08AM -0700, Yosry Ahmed wrote:
> > On Mon, Aug 15, 2022 at 8:42 AM Yosry Ahmed <yosryahmed@google.com> wrote:
> > >
> > > On Mon, Aug 15, 2022 at 8:31 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
> > > >
> > > > On Mon, Aug 15, 2022 at 06:46:46AM -0700, Yosry Ahmed wrote:
> > > > > Yeah I understand this much, what I don't understand is why we charge
> > > > > the zswap memory through objcg (thus tying it to memcg kmem charging)
> > > > > rather than directly through memcg.
> > > >
> > > > The charged quantities are smaller than a page, so we have to use the
> > > > byte interface.
> > > >
> > > > The byte interface (objcg) was written for slab originally, hence the
> > > > link to the kmem option. But note that CONFIG_MEMCG_KMEM is no longer
> > > > a user-visible option, and for all intents and purposes a fixed part
> > > > of CONFIG_MEMCG.
> > > >
> > > > (There is the SLOB quirk. But I'm not sure anybody uses slob, let
> > > > alone slob + memcg.)
> > >
> > > Thanks for the clarification, it makes sense to use the byte interface
> > > here for this, and thanks for pointing out that CONFIC_MEMCG_KMEM is
> > > not part of CONFIG_MEMCG.
> > >
> > > One more question :) memcg kmem charging can still be disabled even
> > > with !CONFIG_MEMCG_KMEM, right? In this case zswap charging will also
> > > be off, which seems like an unintended side effect, right?
> >
> > memcg kmem charging can still be disabled even
> > with CONFIG_MEMCG_KMEM***
>
> Yes, indeed, if the host is booted with the nokmem flag. Doing so will
> turn off slab, percpu, and (as of recently) zswap.
>
> The zswap backing storage *is* kernel memory, so that seems like the
> correct semantics for the flag.

I honestly didn't consider it first as kernel memory, as the memory is
coming from LRU pages. It can be confusing to think about, given that
it is memory that the kernel allocates and manages, but it has user
data. Anyway, maybe it's not worth overthinking this, since it can be
considered as kernel memory.

>
> That said, the distinction between kernel and user memory is becoming
> increasingly odd. The more kernel memory we track, the more ridiculous
> the size of the hole you punch into resource control by disabling it.
>
> Maybe we should just deprecate that knob altogether.

Yeah a lot of used memory will just go unaccounted even with memcg
enabled if the knob is disabled (which is what memcg is all about).
This is even more significant now with zswap in the picture.

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

[Shakeel Butt]

On Mon, Aug 15, 2022 at 10:56 AM Yosry Ahmed <yosryahmed@google.com> wrote:
>
> On Mon, Aug 15, 2022 at 10:47 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
> >
> > On Mon, Aug 15, 2022 at 09:16:08AM -0700, Yosry Ahmed wrote:
> > > On Mon, Aug 15, 2022 at 8:42 AM Yosry Ahmed <yosryahmed@google.com> wrote:
> > > >
> > > > On Mon, Aug 15, 2022 at 8:31 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
> > > > >
> > > > > On Mon, Aug 15, 2022 at 06:46:46AM -0700, Yosry Ahmed wrote:
> > > > > > Yeah I understand this much, what I don't understand is why we charge
> > > > > > the zswap memory through objcg (thus tying it to memcg kmem charging)
> > > > > > rather than directly through memcg.
> > > > >
> > > > > The charged quantities are smaller than a page, so we have to use the
> > > > > byte interface.
> > > > >
> > > > > The byte interface (objcg) was written for slab originally, hence the
> > > > > link to the kmem option. But note that CONFIG_MEMCG_KMEM is no longer
> > > > > a user-visible option, and for all intents and purposes a fixed part
> > > > > of CONFIG_MEMCG.
> > > > >
> > > > > (There is the SLOB quirk. But I'm not sure anybody uses slob, let
> > > > > alone slob + memcg.)
> > > >
> > > > Thanks for the clarification, it makes sense to use the byte interface
> > > > here for this, and thanks for pointing out that CONFIC_MEMCG_KMEM is
> > > > not part of CONFIG_MEMCG.
> > > >
> > > > One more question :) memcg kmem charging can still be disabled even
> > > > with !CONFIG_MEMCG_KMEM, right? In this case zswap charging will also
> > > > be off, which seems like an unintended side effect, right?
> > >
> > > memcg kmem charging can still be disabled even
> > > with CONFIG_MEMCG_KMEM***
> >
> > Yes, indeed, if the host is booted with the nokmem flag. Doing so will
> > turn off slab, percpu, and (as of recently) zswap.
> >
> > The zswap backing storage *is* kernel memory, so that seems like the
> > correct semantics for the flag.
>
> I honestly didn't consider it first as kernel memory, as the memory is
> coming from LRU pages. It can be confusing to think about, given that
> it is memory that the kernel allocates and manages, but it has user
> data. Anyway, maybe it's not worth overthinking this, since it can be
> considered as kernel memory.

Yup not worth overthinking this.

>
> >
> > That said, the distinction between kernel and user memory is becoming
> > increasingly odd. The more kernel memory we track, the more ridiculous
> > the size of the hole you punch into resource control by disabling it.
> >
> > Maybe we should just deprecate that knob altogether.
>
> Yeah a lot of used memory will just go unaccounted even with memcg
> enabled if the knob is disabled (which is what memcg is all about).
> This is even more significant now with zswap in the picture.

Yes, we should deprecate this knob. Last I remember there were users
facing zombie issues due to kernel memory for kernels before objcg
infrastructure and used this knob to resolve the zombie issue. So, I
think this can be deprecated. Though it would be safe to first warn
and deprecate in later release.

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

[Muchun Song]

On Thu, Aug 11, 2022 at 4:19 PM liliguang <liliguang@baidu.com> 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>

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Thanks.

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

[Johannes Weiner]

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>

Great catch. I think this qualifies for stable.

Cc: stable@vger.kernel.org # 5.19
Fixes: f4840ccfca25 ("zswap: memcg accounting")
Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Andrew, can you please take this through the MM tree?

> ---
>  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())
>  		return NULL;
>  
>  	if (PageMemcgKmem(page)) {
> -- 
> 2.32.0 (Apple Git-132)
> 

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

[Andrew Morton]

On Mon, 15 Aug 2022 11:20:11 -0400 Johannes Weiner <hannes@cmpxchg.org> wrote:

> Great catch. I think this qualifies for stable.
> 
> Cc: stable@vger.kernel.org # 5.19
> Fixes: f4840ccfca25 ("zswap: memcg accounting")
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> 
> Andrew, can you please take this through the MM tree?

Sure, but I'm not subscribed to cgroups@ and it wasn't cc'ed to
linux-kernel so no patch for me.

I could reconstitute it from the quoted patch but that's a bit lame and
we don't get a usable Link: tag.

So please redo, refresh, retest and resend?

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

[Johannes Weiner]

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.

Link: https://lore.kernel.org/linux-mm/CALvZod4nnn8BHYqAM4xtcR0Ddo2-Wr8uKm9h_CHWUaXw7g_DCg@mail.gmail.com/
Fixes: f4840ccfca25 ("zswap: memcg accounting")
Cc: stable@vger.kernel.org # 5.19+
Acked-by: Shakeel Butt <shakeelb@google.com>
Acked-by: Shakeel Butt <shakeelb@google.com>
Reviewed-by: Muchun Song <songmuchun@bytedance.com>
Signed-off-by: Li Liguang <liliguang@baidu.com>
Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/memcontrol.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

This fell through the cracks in summer as it didn't make it to the
right mailing lists. Resending.

I know it's close to 6.1-final, but the fix should be safe to put
in. It's straight-forward and obvious code-wise. We also have large
parts of production running on it without problems.

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 2d8549ae1b30..a1a35c12635e 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3026,7 +3026,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())
 		return NULL;
 
 	if (PageMemcgKmem(page)) {
-- 
2.38.1