[PATCH] memcg: expose root cgroup's memory.stat

[PATCH] memcg: expose root cgroup's memory.stat

[Shakeel Butt]

One way to measure the efficiency of memory reclaim is to look at the
ratio (pgscan+pfrefill)/pgsteal. However at the moment these stats are
not updated consistently at the system level and the ratio of these are
not very meaningful. The pgsteal and pgscan are updated for only global
reclaim while pgrefill gets updated for global as well as cgroup
reclaim.

Please note that this difference is only for system level vmstats. The
cgroup stats returned by memory.stat are actually consistent. The
cgroup's pgsteal contains number of reclaimed pages for global as well
as cgroup reclaim. So, one way to get the system level stats is to get
these stats from root's memory.stat, so, expose memory.stat for the root
cgroup.

	from Johannes Weiner:
	There are subtle differences between /proc/vmstat and
	memory.stat, and cgroup-aware code that wants to watch the full
	hierarchy currently has to know about these intricacies and
	translate semantics back and forth.

	Generally having the fully recursive memory.stat at the root
	level could help a broader range of usecases.

Signed-off-by: Shakeel Butt <shakeelb@google.com>
Suggested-by: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/memcontrol.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 05dcb72314b5..c300d52c07a5 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -6230,7 +6230,6 @@ static struct cftype memory_files[] = {
 	},
 	{
 		.name = "stat",
-		.flags = CFTYPE_NOT_ON_ROOT,
 		.seq_show = memory_stat_show,
 	},
 	{
-- 
2.26.2.645.ge9eca65c58-goog

Re: [PATCH] memcg: expose root cgroup's memory.stat

[Johannes Weiner]

On Fri, May 08, 2020 at 10:06:30AM -0700, Shakeel Butt wrote:
> One way to measure the efficiency of memory reclaim is to look at the
> ratio (pgscan+pfrefill)/pgsteal. However at the moment these stats are
> not updated consistently at the system level and the ratio of these are
> not very meaningful. The pgsteal and pgscan are updated for only global
> reclaim while pgrefill gets updated for global as well as cgroup
> reclaim.
> 
> Please note that this difference is only for system level vmstats. The
> cgroup stats returned by memory.stat are actually consistent. The
> cgroup's pgsteal contains number of reclaimed pages for global as well
> as cgroup reclaim. So, one way to get the system level stats is to get
> these stats from root's memory.stat, so, expose memory.stat for the root
> cgroup.
> 
> 	from Johannes Weiner:
> 	There are subtle differences between /proc/vmstat and
> 	memory.stat, and cgroup-aware code that wants to watch the full
> 	hierarchy currently has to know about these intricacies and
> 	translate semantics back and forth.
> 
> 	Generally having the fully recursive memory.stat at the root
> 	level could help a broader range of usecases.

The changelog begs the question why we don't just "fix" the
system-level stats. It may be useful to include the conclusions from
that discussion, and why there is value in keeping the stats this way.

> Signed-off-by: Shakeel Butt <shakeelb@google.com>
> Suggested-by: Johannes Weiner <hannes@cmpxchg.org>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Re: [PATCH] memcg: expose root cgroup's memory.stat

[Shakeel Butt]

On Fri, May 8, 2020 at 2:44 PM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Fri, May 08, 2020 at 10:06:30AM -0700, Shakeel Butt wrote:
> > One way to measure the efficiency of memory reclaim is to look at the
> > ratio (pgscan+pfrefill)/pgsteal. However at the moment these stats are
> > not updated consistently at the system level and the ratio of these are
> > not very meaningful. The pgsteal and pgscan are updated for only global
> > reclaim while pgrefill gets updated for global as well as cgroup
> > reclaim.
> >
> > Please note that this difference is only for system level vmstats. The
> > cgroup stats returned by memory.stat are actually consistent. The
> > cgroup's pgsteal contains number of reclaimed pages for global as well
> > as cgroup reclaim. So, one way to get the system level stats is to get
> > these stats from root's memory.stat, so, expose memory.stat for the root
> > cgroup.
> >
> >       from Johannes Weiner:
> >       There are subtle differences between /proc/vmstat and
> >       memory.stat, and cgroup-aware code that wants to watch the full
> >       hierarchy currently has to know about these intricacies and
> >       translate semantics back and forth.
> >
> >       Generally having the fully recursive memory.stat at the root
> >       level could help a broader range of usecases.
>
> The changelog begs the question why we don't just "fix" the
> system-level stats. It may be useful to include the conclusions from
> that discussion, and why there is value in keeping the stats this way.
>

Right. Andrew, can you please add the following para to the changelog?

Why not fix the stats by including both the global and cgroup reclaim
activity instead of exposing root cgroup's memory.stat? The reason is
the benefit of having metrics exposing the activity that happens
purely due to machine capacity rather than localized activity that
happens due to the limits throughout the cgroup tree. Additionally
there are userspace tools like sysstat(sar) which reads these stats to
inform about the system level reclaim activity. So, we should not
break such use-cases.

> > Signed-off-by: Shakeel Butt <shakeelb@google.com>
> > Suggested-by: Johannes Weiner <hannes@cmpxchg.org>
>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Thanks a lot.

Re: [PATCH] memcg: expose root cgroup's memory.stat

[Yafang Shao]

On Sat, May 9, 2020 at 10:06 PM Shakeel Butt <shakeelb@google.com> wrote:
>
> On Fri, May 8, 2020 at 2:44 PM Johannes Weiner <hannes@cmpxchg.org> wrote:
> >
> > On Fri, May 08, 2020 at 10:06:30AM -0700, Shakeel Butt wrote:
> > > One way to measure the efficiency of memory reclaim is to look at the
> > > ratio (pgscan+pfrefill)/pgsteal. However at the moment these stats are
> > > not updated consistently at the system level and the ratio of these are
> > > not very meaningful. The pgsteal and pgscan are updated for only global
> > > reclaim while pgrefill gets updated for global as well as cgroup
> > > reclaim.
> > >
> > > Please note that this difference is only for system level vmstats. The
> > > cgroup stats returned by memory.stat are actually consistent. The
> > > cgroup's pgsteal contains number of reclaimed pages for global as well
> > > as cgroup reclaim. So, one way to get the system level stats is to get
> > > these stats from root's memory.stat, so, expose memory.stat for the root
> > > cgroup.
> > >
> > >       from Johannes Weiner:
> > >       There are subtle differences between /proc/vmstat and
> > >       memory.stat, and cgroup-aware code that wants to watch the full
> > >       hierarchy currently has to know about these intricacies and
> > >       translate semantics back and forth.
> > >
> > >       Generally having the fully recursive memory.stat at the root
> > >       level could help a broader range of usecases.
> >
> > The changelog begs the question why we don't just "fix" the
> > system-level stats. It may be useful to include the conclusions from
> > that discussion, and why there is value in keeping the stats this way.
> >
>
> Right. Andrew, can you please add the following para to the changelog?
>
> Why not fix the stats by including both the global and cgroup reclaim
> activity instead of exposing root cgroup's memory.stat? The reason is
> the benefit of having metrics exposing the activity that happens
> purely due to machine capacity rather than localized activity that
> happens due to the limits throughout the cgroup tree. Additionally
> there are userspace tools like sysstat(sar) which reads these stats to
> inform about the system level reclaim activity. So, we should not
> break such use-cases.
>

Acked-by: Yafang Shao <laoar.shao@gmail.com>

> > > Signed-off-by: Shakeel Butt <shakeelb@google.com>
> > > Suggested-by: Johannes Weiner <hannes@cmpxchg.org>
> >
> > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
>
> Thanks a lot.



-- 
Thanks
Yafang

Re: [PATCH] memcg: expose root cgroup's memory.stat

[Michal Hocko]

On Sat 09-05-20 07:06:38, Shakeel Butt wrote:
> On Fri, May 8, 2020 at 2:44 PM Johannes Weiner <hannes@cmpxchg.org> wrote:
> >
> > On Fri, May 08, 2020 at 10:06:30AM -0700, Shakeel Butt wrote:
> > > One way to measure the efficiency of memory reclaim is to look at the
> > > ratio (pgscan+pfrefill)/pgsteal. However at the moment these stats are
> > > not updated consistently at the system level and the ratio of these are
> > > not very meaningful. The pgsteal and pgscan are updated for only global
> > > reclaim while pgrefill gets updated for global as well as cgroup
> > > reclaim.
> > >
> > > Please note that this difference is only for system level vmstats. The
> > > cgroup stats returned by memory.stat are actually consistent. The
> > > cgroup's pgsteal contains number of reclaimed pages for global as well
> > > as cgroup reclaim. So, one way to get the system level stats is to get
> > > these stats from root's memory.stat, so, expose memory.stat for the root
> > > cgroup.
> > >
> > >       from Johannes Weiner:
> > >       There are subtle differences between /proc/vmstat and
> > >       memory.stat, and cgroup-aware code that wants to watch the full
> > >       hierarchy currently has to know about these intricacies and
> > >       translate semantics back and forth.

Can we have those subtle differences documented please?

> > >
> > >       Generally having the fully recursive memory.stat at the root
> > >       level could help a broader range of usecases.
> >
> > The changelog begs the question why we don't just "fix" the
> > system-level stats. It may be useful to include the conclusions from
> > that discussion, and why there is value in keeping the stats this way.
> >
> 
> Right. Andrew, can you please add the following para to the changelog?
> 
> Why not fix the stats by including both the global and cgroup reclaim
> activity instead of exposing root cgroup's memory.stat? The reason is
> the benefit of having metrics exposing the activity that happens
> purely due to machine capacity rather than localized activity that
> happens due to the limits throughout the cgroup tree. Additionally
> there are userspace tools like sysstat(sar) which reads these stats to
> inform about the system level reclaim activity. So, we should not
> break such use-cases.
> 
> > > Signed-off-by: Shakeel Butt <shakeelb@google.com>
> > > Suggested-by: Johannes Weiner <hannes@cmpxchg.org>
> >
> > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> 
> Thanks a lot.

I was quite surprised that the patch is so simple TBH. For some reason
I've still had memories that we do not account for root memcg (likely
because mem_cgroup_is_root(memcg) bail out in the try_charge. But stats
are slightly different here. I have started looking at different stat
counters because they are not really all the same. E.g. 
- mem_cgroup_charge_statistics accounts for each memcg
- memcg_charge_kernel_stack relies on pages being associated with a
  memcg and that in turn relies on __memcg_kmem_charge_page which bails
  out on root memcg
- memcg_charge_slab (NR_SLAB*) skips over root memcg as well
- __mod_lruvec_page_state relies on page->mem_cgroup as well but this
  one is ok for paths which go through commit_charge path.

That being said we should really double check which stats are
accounted properly. At least MEMCG_KERNEL_STACK_KB won't unless I am
misreading the code.

I do not mind displaying the root's memcg stats but a) a closer look had
to be done for each counter and b) a clarification of differences from
the global vmstat counters would be really handy.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] memcg: expose root cgroup's memory.stat

[Johannes Weiner]

On Fri, May 15, 2020 at 10:29:55AM +0200, Michal Hocko wrote:
> On Sat 09-05-20 07:06:38, Shakeel Butt wrote:
> > On Fri, May 8, 2020 at 2:44 PM Johannes Weiner <hannes@cmpxchg.org> wrote:
> > >
> > > On Fri, May 08, 2020 at 10:06:30AM -0700, Shakeel Butt wrote:
> > > > One way to measure the efficiency of memory reclaim is to look at the
> > > > ratio (pgscan+pfrefill)/pgsteal. However at the moment these stats are
> > > > not updated consistently at the system level and the ratio of these are
> > > > not very meaningful. The pgsteal and pgscan are updated for only global
> > > > reclaim while pgrefill gets updated for global as well as cgroup
> > > > reclaim.
> > > >
> > > > Please note that this difference is only for system level vmstats. The
> > > > cgroup stats returned by memory.stat are actually consistent. The
> > > > cgroup's pgsteal contains number of reclaimed pages for global as well
> > > > as cgroup reclaim. So, one way to get the system level stats is to get
> > > > these stats from root's memory.stat, so, expose memory.stat for the root
> > > > cgroup.
> > > >
> > > >       from Johannes Weiner:
> > > >       There are subtle differences between /proc/vmstat and
> > > >       memory.stat, and cgroup-aware code that wants to watch the full
> > > >       hierarchy currently has to know about these intricacies and
> > > >       translate semantics back and forth.
> 
> Can we have those subtle differences documented please?
> 
> > > >
> > > >       Generally having the fully recursive memory.stat at the root
> > > >       level could help a broader range of usecases.
> > >
> > > The changelog begs the question why we don't just "fix" the
> > > system-level stats. It may be useful to include the conclusions from
> > > that discussion, and why there is value in keeping the stats this way.
> > >
> > 
> > Right. Andrew, can you please add the following para to the changelog?
> > 
> > Why not fix the stats by including both the global and cgroup reclaim
> > activity instead of exposing root cgroup's memory.stat? The reason is
> > the benefit of having metrics exposing the activity that happens
> > purely due to machine capacity rather than localized activity that
> > happens due to the limits throughout the cgroup tree. Additionally
> > there are userspace tools like sysstat(sar) which reads these stats to
> > inform about the system level reclaim activity. So, we should not
> > break such use-cases.
> > 
> > > > Signed-off-by: Shakeel Butt <shakeelb@google.com>
> > > > Suggested-by: Johannes Weiner <hannes@cmpxchg.org>
> > >
> > > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> > 
> > Thanks a lot.
> 
> I was quite surprised that the patch is so simple TBH. For some reason
> I've still had memories that we do not account for root memcg (likely
> because mem_cgroup_is_root(memcg) bail out in the try_charge. But stats
> are slightly different here.

Yep, we skip the page_counter for root, but keep in mind that cgroup1
*does* have a root-level memory.stat, so (for the most part) we've
been keeping consumer stats for the root level the whole time.

> counters because they are not really all the same. E.g.
> - mem_cgroup_charge_statistics accounts for each memcg

Yep, that's heritage from cgroup1.

> - memcg_charge_kernel_stack relies on pages being associated with a
>   memcg and that in turn relies on __memcg_kmem_charge_page which bails
>   out on root memcg

You're right. It should only bypass the page_counter, but still set
page->mem_cgroup = root_mem_cgroup, just like user pages.

This counter also doesn't get exported on cgroup1, so it would indeed
be a new bug. It needs to be fixed before this patch here.

> - memcg_charge_slab (NR_SLAB*) skips over root memcg as well

Same thing with these two.

Re: [PATCH] memcg: expose root cgroup's memory.stat

[Shakeel Butt]

On Fri, May 15, 2020 at 6:24 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Fri, May 15, 2020 at 10:29:55AM +0200, Michal Hocko wrote:
> > On Sat 09-05-20 07:06:38, Shakeel Butt wrote:
> > > On Fri, May 8, 2020 at 2:44 PM Johannes Weiner <hannes@cmpxchg.org> wrote:
> > > >
> > > > On Fri, May 08, 2020 at 10:06:30AM -0700, Shakeel Butt wrote:
> > > > > One way to measure the efficiency of memory reclaim is to look at the
> > > > > ratio (pgscan+pfrefill)/pgsteal. However at the moment these stats are
> > > > > not updated consistently at the system level and the ratio of these are
> > > > > not very meaningful. The pgsteal and pgscan are updated for only global
> > > > > reclaim while pgrefill gets updated for global as well as cgroup
> > > > > reclaim.
> > > > >
> > > > > Please note that this difference is only for system level vmstats. The
> > > > > cgroup stats returned by memory.stat are actually consistent. The
> > > > > cgroup's pgsteal contains number of reclaimed pages for global as well
> > > > > as cgroup reclaim. So, one way to get the system level stats is to get
> > > > > these stats from root's memory.stat, so, expose memory.stat for the root
> > > > > cgroup.
> > > > >
> > > > >       from Johannes Weiner:
> > > > >       There are subtle differences between /proc/vmstat and
> > > > >       memory.stat, and cgroup-aware code that wants to watch the full
> > > > >       hierarchy currently has to know about these intricacies and
> > > > >       translate semantics back and forth.
> >
> > Can we have those subtle differences documented please?
> >
> > > > >
> > > > >       Generally having the fully recursive memory.stat at the root
> > > > >       level could help a broader range of usecases.
> > > >
> > > > The changelog begs the question why we don't just "fix" the
> > > > system-level stats. It may be useful to include the conclusions from
> > > > that discussion, and why there is value in keeping the stats this way.
> > > >
> > >
> > > Right. Andrew, can you please add the following para to the changelog?
> > >
> > > Why not fix the stats by including both the global and cgroup reclaim
> > > activity instead of exposing root cgroup's memory.stat? The reason is
> > > the benefit of having metrics exposing the activity that happens
> > > purely due to machine capacity rather than localized activity that
> > > happens due to the limits throughout the cgroup tree. Additionally
> > > there are userspace tools like sysstat(sar) which reads these stats to
> > > inform about the system level reclaim activity. So, we should not
> > > break such use-cases.
> > >
> > > > > Signed-off-by: Shakeel Butt <shakeelb@google.com>
> > > > > Suggested-by: Johannes Weiner <hannes@cmpxchg.org>
> > > >
> > > > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> > >
> > > Thanks a lot.
> >
> > I was quite surprised that the patch is so simple TBH. For some reason
> > I've still had memories that we do not account for root memcg (likely
> > because mem_cgroup_is_root(memcg) bail out in the try_charge. But stats
> > are slightly different here.
>
> Yep, we skip the page_counter for root, but keep in mind that cgroup1
> *does* have a root-level memory.stat, so (for the most part) we've
> been keeping consumer stats for the root level the whole time.
>
> > counters because they are not really all the same. E.g.
> > - mem_cgroup_charge_statistics accounts for each memcg
>
> Yep, that's heritage from cgroup1.
>
> > - memcg_charge_kernel_stack relies on pages being associated with a
> >   memcg and that in turn relies on __memcg_kmem_charge_page which bails
> >   out on root memcg
>
> You're right. It should only bypass the page_counter, but still set
> page->mem_cgroup = root_mem_cgroup, just like user pages.
>
> This counter also doesn't get exported on cgroup1, so it would indeed
> be a new bug. It needs to be fixed before this patch here.
>
> > - memcg_charge_slab (NR_SLAB*) skips over root memcg as well
>
> Same thing with these two.

Yes, we skip page_counter for root but not the stats. I will go over
all the stats and make sure we are not skipping the stats for root.

Re: [PATCH] memcg: expose root cgroup's memory.stat

[Roman Gushchin]

On Fri, May 15, 2020 at 06:44:44AM -0700, Shakeel Butt wrote:
> On Fri, May 15, 2020 at 6:24 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
> >
> > On Fri, May 15, 2020 at 10:29:55AM +0200, Michal Hocko wrote:
> > > On Sat 09-05-20 07:06:38, Shakeel Butt wrote:
> > > > On Fri, May 8, 2020 at 2:44 PM Johannes Weiner <hannes@cmpxchg.org> wrote:
> > > > >
> > > > > On Fri, May 08, 2020 at 10:06:30AM -0700, Shakeel Butt wrote:
> > > > > > One way to measure the efficiency of memory reclaim is to look at the
> > > > > > ratio (pgscan+pfrefill)/pgsteal. However at the moment these stats are
> > > > > > not updated consistently at the system level and the ratio of these are
> > > > > > not very meaningful. The pgsteal and pgscan are updated for only global
> > > > > > reclaim while pgrefill gets updated for global as well as cgroup
> > > > > > reclaim.
> > > > > >
> > > > > > Please note that this difference is only for system level vmstats. The
> > > > > > cgroup stats returned by memory.stat are actually consistent. The
> > > > > > cgroup's pgsteal contains number of reclaimed pages for global as well
> > > > > > as cgroup reclaim. So, one way to get the system level stats is to get
> > > > > > these stats from root's memory.stat, so, expose memory.stat for the root
> > > > > > cgroup.
> > > > > >
> > > > > >       from Johannes Weiner:
> > > > > >       There are subtle differences between /proc/vmstat and
> > > > > >       memory.stat, and cgroup-aware code that wants to watch the full
> > > > > >       hierarchy currently has to know about these intricacies and
> > > > > >       translate semantics back and forth.
> > >
> > > Can we have those subtle differences documented please?
> > >
> > > > > >
> > > > > >       Generally having the fully recursive memory.stat at the root
> > > > > >       level could help a broader range of usecases.
> > > > >
> > > > > The changelog begs the question why we don't just "fix" the
> > > > > system-level stats. It may be useful to include the conclusions from
> > > > > that discussion, and why there is value in keeping the stats this way.
> > > > >
> > > >
> > > > Right. Andrew, can you please add the following para to the changelog?
> > > >
> > > > Why not fix the stats by including both the global and cgroup reclaim
> > > > activity instead of exposing root cgroup's memory.stat? The reason is
> > > > the benefit of having metrics exposing the activity that happens
> > > > purely due to machine capacity rather than localized activity that
> > > > happens due to the limits throughout the cgroup tree. Additionally
> > > > there are userspace tools like sysstat(sar) which reads these stats to
> > > > inform about the system level reclaim activity. So, we should not
> > > > break such use-cases.
> > > >
> > > > > > Signed-off-by: Shakeel Butt <shakeelb@google.com>
> > > > > > Suggested-by: Johannes Weiner <hannes@cmpxchg.org>
> > > > >
> > > > > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> > > >
> > > > Thanks a lot.
> > >
> > > I was quite surprised that the patch is so simple TBH. For some reason
> > > I've still had memories that we do not account for root memcg (likely
> > > because mem_cgroup_is_root(memcg) bail out in the try_charge. But stats
> > > are slightly different here.
> >
> > Yep, we skip the page_counter for root, but keep in mind that cgroup1
> > *does* have a root-level memory.stat, so (for the most part) we've
> > been keeping consumer stats for the root level the whole time.
> >
> > > counters because they are not really all the same. E.g.
> > > - mem_cgroup_charge_statistics accounts for each memcg
> >
> > Yep, that's heritage from cgroup1.
> >
> > > - memcg_charge_kernel_stack relies on pages being associated with a
> > >   memcg and that in turn relies on __memcg_kmem_charge_page which bails
> > >   out on root memcg
> >
> > You're right. It should only bypass the page_counter, but still set
> > page->mem_cgroup = root_mem_cgroup, just like user pages.

What about kernel threads? We consider them belonging to the root memory
cgroup. Should their memory consumption being considered in root-level stats?

I'm not sure we really want it, but I guess we need to document how
kernel threads are handled.

> >
> > This counter also doesn't get exported on cgroup1, so it would indeed
> > be a new bug. It needs to be fixed before this patch here.
> >
> > > - memcg_charge_slab (NR_SLAB*) skips over root memcg as well
> >
> > Same thing with these two.
> 
> Yes, we skip page_counter for root but not the stats. I will go over
> all the stats and make sure we are not skipping the stats for root.

Re: [PATCH] memcg: expose root cgroup's memory.stat

[Shakeel Butt]

On Fri, May 15, 2020 at 8:00 AM Roman Gushchin <guro@fb.com> wrote:
>
> On Fri, May 15, 2020 at 06:44:44AM -0700, Shakeel Butt wrote:
> > On Fri, May 15, 2020 at 6:24 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
> > >
> > > On Fri, May 15, 2020 at 10:29:55AM +0200, Michal Hocko wrote:
> > > > On Sat 09-05-20 07:06:38, Shakeel Butt wrote:
> > > > > On Fri, May 8, 2020 at 2:44 PM Johannes Weiner <hannes@cmpxchg.org> wrote:
> > > > > >
> > > > > > On Fri, May 08, 2020 at 10:06:30AM -0700, Shakeel Butt wrote:
> > > > > > > One way to measure the efficiency of memory reclaim is to look at the
> > > > > > > ratio (pgscan+pfrefill)/pgsteal. However at the moment these stats are
> > > > > > > not updated consistently at the system level and the ratio of these are
> > > > > > > not very meaningful. The pgsteal and pgscan are updated for only global
> > > > > > > reclaim while pgrefill gets updated for global as well as cgroup
> > > > > > > reclaim.
> > > > > > >
> > > > > > > Please note that this difference is only for system level vmstats. The
> > > > > > > cgroup stats returned by memory.stat are actually consistent. The
> > > > > > > cgroup's pgsteal contains number of reclaimed pages for global as well
> > > > > > > as cgroup reclaim. So, one way to get the system level stats is to get
> > > > > > > these stats from root's memory.stat, so, expose memory.stat for the root
> > > > > > > cgroup.
> > > > > > >
> > > > > > >       from Johannes Weiner:
> > > > > > >       There are subtle differences between /proc/vmstat and
> > > > > > >       memory.stat, and cgroup-aware code that wants to watch the full
> > > > > > >       hierarchy currently has to know about these intricacies and
> > > > > > >       translate semantics back and forth.
> > > >
> > > > Can we have those subtle differences documented please?
> > > >
> > > > > > >
> > > > > > >       Generally having the fully recursive memory.stat at the root
> > > > > > >       level could help a broader range of usecases.
> > > > > >
> > > > > > The changelog begs the question why we don't just "fix" the
> > > > > > system-level stats. It may be useful to include the conclusions from
> > > > > > that discussion, and why there is value in keeping the stats this way.
> > > > > >
> > > > >
> > > > > Right. Andrew, can you please add the following para to the changelog?
> > > > >
> > > > > Why not fix the stats by including both the global and cgroup reclaim
> > > > > activity instead of exposing root cgroup's memory.stat? The reason is
> > > > > the benefit of having metrics exposing the activity that happens
> > > > > purely due to machine capacity rather than localized activity that
> > > > > happens due to the limits throughout the cgroup tree. Additionally
> > > > > there are userspace tools like sysstat(sar) which reads these stats to
> > > > > inform about the system level reclaim activity. So, we should not
> > > > > break such use-cases.
> > > > >
> > > > > > > Signed-off-by: Shakeel Butt <shakeelb@google.com>
> > > > > > > Suggested-by: Johannes Weiner <hannes@cmpxchg.org>
> > > > > >
> > > > > > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> > > > >
> > > > > Thanks a lot.
> > > >
> > > > I was quite surprised that the patch is so simple TBH. For some reason
> > > > I've still had memories that we do not account for root memcg (likely
> > > > because mem_cgroup_is_root(memcg) bail out in the try_charge. But stats
> > > > are slightly different here.
> > >
> > > Yep, we skip the page_counter for root, but keep in mind that cgroup1
> > > *does* have a root-level memory.stat, so (for the most part) we've
> > > been keeping consumer stats for the root level the whole time.
> > >
> > > > counters because they are not really all the same. E.g.
> > > > - mem_cgroup_charge_statistics accounts for each memcg
> > >
> > > Yep, that's heritage from cgroup1.
> > >
> > > > - memcg_charge_kernel_stack relies on pages being associated with a
> > > >   memcg and that in turn relies on __memcg_kmem_charge_page which bails
> > > >   out on root memcg
> > >
> > > You're right. It should only bypass the page_counter, but still set
> > > page->mem_cgroup = root_mem_cgroup, just like user pages.
>
> What about kernel threads? We consider them belonging to the root memory
> cgroup. Should their memory consumption being considered in root-level stats?
>
> I'm not sure we really want it, but I guess we need to document how
> kernel threads are handled.
>

What will be the cons of updating root-level stats for kthreads?

> > >
> > > This counter also doesn't get exported on cgroup1, so it would indeed
> > > be a new bug. It needs to be fixed before this patch here.
> > >
> > > > - memcg_charge_slab (NR_SLAB*) skips over root memcg as well
> > >
> > > Same thing with these two.
> >
> > Yes, we skip page_counter for root but not the stats. I will go over
> > all the stats and make sure we are not skipping the stats for root.

Re: [PATCH] memcg: expose root cgroup's memory.stat

[Johannes Weiner]

On Fri, May 15, 2020 at 10:49:22AM -0700, Shakeel Butt wrote:
> On Fri, May 15, 2020 at 8:00 AM Roman Gushchin <guro@fb.com> wrote:
> > On Fri, May 15, 2020 at 06:44:44AM -0700, Shakeel Butt wrote:
> > > On Fri, May 15, 2020 at 6:24 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
> > > > You're right. It should only bypass the page_counter, but still set
> > > > page->mem_cgroup = root_mem_cgroup, just like user pages.
> >
> > What about kernel threads? We consider them belonging to the root memory
> > cgroup. Should their memory consumption being considered in root-level stats?
> >
> > I'm not sure we really want it, but I guess we need to document how
> > kernel threads are handled.
> 
> What will be the cons of updating root-level stats for kthreads?

Should kernel threads be doing GFP_ACCOUNT allocations without
memalloc_use_memcg()? GFP_ACCOUNT implies that the memory consumption
can be significant and should be attributed to userspace activity.

If the kernel thread has no userspace entity to blame, it seems to
imply the same thing as a !GFP_ACCOUNT allocation: shared public
infrastructure, not interesting to account to any specific cgroup.

I'm not sure if we have such allocations right now. But IMO we should
not account anything from kthreads, or interrupts for that matter,
/unless/ there is a specific active_memcg that was set by the kthread
or the interrupt.

Re: [PATCH] memcg: expose root cgroup's memory.stat

[Shakeel Butt]

On Fri, May 15, 2020 at 11:09 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Fri, May 15, 2020 at 10:49:22AM -0700, Shakeel Butt wrote:
> > On Fri, May 15, 2020 at 8:00 AM Roman Gushchin <guro@fb.com> wrote:
> > > On Fri, May 15, 2020 at 06:44:44AM -0700, Shakeel Butt wrote:
> > > > On Fri, May 15, 2020 at 6:24 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
> > > > > You're right. It should only bypass the page_counter, but still set
> > > > > page->mem_cgroup = root_mem_cgroup, just like user pages.
> > >
> > > What about kernel threads? We consider them belonging to the root memory
> > > cgroup. Should their memory consumption being considered in root-level stats?
> > >
> > > I'm not sure we really want it, but I guess we need to document how
> > > kernel threads are handled.
> >
> > What will be the cons of updating root-level stats for kthreads?
>
> Should kernel threads be doing GFP_ACCOUNT allocations without
> memalloc_use_memcg()? GFP_ACCOUNT implies that the memory consumption
> can be significant and should be attributed to userspace activity.
>
> If the kernel thread has no userspace entity to blame, it seems to
> imply the same thing as a !GFP_ACCOUNT allocation: shared public
> infrastructure, not interesting to account to any specific cgroup.
>
> I'm not sure if we have such allocations right now. But IMO we should
> not account anything from kthreads, or interrupts for that matter,
> /unless/ there is a specific active_memcg that was set by the kthread
> or the interrupt.

I totally agree with you but I think your response is about memory
charging in IRQ/kthread context, a topic of a parallel patch from
Zefan Li at [1].

Here we are discussing stats update for kthreads e.g. should we update
root memcg's MEMCG_KERNEL_STACK_KB stat when we allocate stack for
kernel threads?

[1] http://lkml.kernel.org/r/3a721f62-5a66-8bc5-247b-5c8b7c51c555@huawei.com

Re: [PATCH] memcg: expose root cgroup's memory.stat

[Roman Gushchin]

On Fri, May 15, 2020 at 10:49:22AM -0700, Shakeel Butt wrote:
> On Fri, May 15, 2020 at 8:00 AM Roman Gushchin <guro@fb.com> wrote:
> >
> > On Fri, May 15, 2020 at 06:44:44AM -0700, Shakeel Butt wrote:
> > > On Fri, May 15, 2020 at 6:24 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
> > > >
> > > > On Fri, May 15, 2020 at 10:29:55AM +0200, Michal Hocko wrote:
> > > > > On Sat 09-05-20 07:06:38, Shakeel Butt wrote:
> > > > > > On Fri, May 8, 2020 at 2:44 PM Johannes Weiner <hannes@cmpxchg.org> wrote:
> > > > > > >
> > > > > > > On Fri, May 08, 2020 at 10:06:30AM -0700, Shakeel Butt wrote:
> > > > > > > > One way to measure the efficiency of memory reclaim is to look at the
> > > > > > > > ratio (pgscan+pfrefill)/pgsteal. However at the moment these stats are
> > > > > > > > not updated consistently at the system level and the ratio of these are
> > > > > > > > not very meaningful. The pgsteal and pgscan are updated for only global
> > > > > > > > reclaim while pgrefill gets updated for global as well as cgroup
> > > > > > > > reclaim.
> > > > > > > >
> > > > > > > > Please note that this difference is only for system level vmstats. The
> > > > > > > > cgroup stats returned by memory.stat are actually consistent. The
> > > > > > > > cgroup's pgsteal contains number of reclaimed pages for global as well
> > > > > > > > as cgroup reclaim. So, one way to get the system level stats is to get
> > > > > > > > these stats from root's memory.stat, so, expose memory.stat for the root
> > > > > > > > cgroup.
> > > > > > > >
> > > > > > > >       from Johannes Weiner:
> > > > > > > >       There are subtle differences between /proc/vmstat and
> > > > > > > >       memory.stat, and cgroup-aware code that wants to watch the full
> > > > > > > >       hierarchy currently has to know about these intricacies and
> > > > > > > >       translate semantics back and forth.
> > > > >
> > > > > Can we have those subtle differences documented please?
> > > > >
> > > > > > > >
> > > > > > > >       Generally having the fully recursive memory.stat at the root
> > > > > > > >       level could help a broader range of usecases.
> > > > > > >
> > > > > > > The changelog begs the question why we don't just "fix" the
> > > > > > > system-level stats. It may be useful to include the conclusions from
> > > > > > > that discussion, and why there is value in keeping the stats this way.
> > > > > > >
> > > > > >
> > > > > > Right. Andrew, can you please add the following para to the changelog?
> > > > > >
> > > > > > Why not fix the stats by including both the global and cgroup reclaim
> > > > > > activity instead of exposing root cgroup's memory.stat? The reason is
> > > > > > the benefit of having metrics exposing the activity that happens
> > > > > > purely due to machine capacity rather than localized activity that
> > > > > > happens due to the limits throughout the cgroup tree. Additionally
> > > > > > there are userspace tools like sysstat(sar) which reads these stats to
> > > > > > inform about the system level reclaim activity. So, we should not
> > > > > > break such use-cases.
> > > > > >
> > > > > > > > Signed-off-by: Shakeel Butt <shakeelb@google.com>
> > > > > > > > Suggested-by: Johannes Weiner <hannes@cmpxchg.org>
> > > > > > >
> > > > > > > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> > > > > >
> > > > > > Thanks a lot.
> > > > >
> > > > > I was quite surprised that the patch is so simple TBH. For some reason
> > > > > I've still had memories that we do not account for root memcg (likely
> > > > > because mem_cgroup_is_root(memcg) bail out in the try_charge. But stats
> > > > > are slightly different here.
> > > >
> > > > Yep, we skip the page_counter for root, but keep in mind that cgroup1
> > > > *does* have a root-level memory.stat, so (for the most part) we've
> > > > been keeping consumer stats for the root level the whole time.
> > > >
> > > > > counters because they are not really all the same. E.g.
> > > > > - mem_cgroup_charge_statistics accounts for each memcg
> > > >
> > > > Yep, that's heritage from cgroup1.
> > > >
> > > > > - memcg_charge_kernel_stack relies on pages being associated with a
> > > > >   memcg and that in turn relies on __memcg_kmem_charge_page which bails
> > > > >   out on root memcg
> > > >
> > > > You're right. It should only bypass the page_counter, but still set
> > > > page->mem_cgroup = root_mem_cgroup, just like user pages.
> >
> > What about kernel threads? We consider them belonging to the root memory
> > cgroup. Should their memory consumption being considered in root-level stats?
> >
> > I'm not sure we really want it, but I guess we need to document how
> > kernel threads are handled.
> >
> 
> What will be the cons of updating root-level stats for kthreads?

It makes total sense for stacks, but not much for the slab memory.
Because it's really "some part of the total slab memory, which is
accounted on the memcg level". And it comes with some performance
overhead.

I'm not really opposing any solution, just saying we need to document
what's included into this statistics and what not.

Thanks!

Re: [PATCH] memcg: expose root cgroup's memory.stat

[Shakeel Butt]

On Fri, May 15, 2020 at 11:09 AM Roman Gushchin <guro@fb.com> wrote:
>
> On Fri, May 15, 2020 at 10:49:22AM -0700, Shakeel Butt wrote:
> > On Fri, May 15, 2020 at 8:00 AM Roman Gushchin <guro@fb.com> wrote:
> > >
> > > On Fri, May 15, 2020 at 06:44:44AM -0700, Shakeel Butt wrote:
> > > > On Fri, May 15, 2020 at 6:24 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
> > > > >
> > > > > On Fri, May 15, 2020 at 10:29:55AM +0200, Michal Hocko wrote:
> > > > > > On Sat 09-05-20 07:06:38, Shakeel Butt wrote:
> > > > > > > On Fri, May 8, 2020 at 2:44 PM Johannes Weiner <hannes@cmpxchg.org> wrote:
> > > > > > > >
> > > > > > > > On Fri, May 08, 2020 at 10:06:30AM -0700, Shakeel Butt wrote:
> > > > > > > > > One way to measure the efficiency of memory reclaim is to look at the
> > > > > > > > > ratio (pgscan+pfrefill)/pgsteal. However at the moment these stats are
> > > > > > > > > not updated consistently at the system level and the ratio of these are
> > > > > > > > > not very meaningful. The pgsteal and pgscan are updated for only global
> > > > > > > > > reclaim while pgrefill gets updated for global as well as cgroup
> > > > > > > > > reclaim.
> > > > > > > > >
> > > > > > > > > Please note that this difference is only for system level vmstats. The
> > > > > > > > > cgroup stats returned by memory.stat are actually consistent. The
> > > > > > > > > cgroup's pgsteal contains number of reclaimed pages for global as well
> > > > > > > > > as cgroup reclaim. So, one way to get the system level stats is to get
> > > > > > > > > these stats from root's memory.stat, so, expose memory.stat for the root
> > > > > > > > > cgroup.
> > > > > > > > >
> > > > > > > > >       from Johannes Weiner:
> > > > > > > > >       There are subtle differences between /proc/vmstat and
> > > > > > > > >       memory.stat, and cgroup-aware code that wants to watch the full
> > > > > > > > >       hierarchy currently has to know about these intricacies and
> > > > > > > > >       translate semantics back and forth.
> > > > > >
> > > > > > Can we have those subtle differences documented please?
> > > > > >
> > > > > > > > >
> > > > > > > > >       Generally having the fully recursive memory.stat at the root
> > > > > > > > >       level could help a broader range of usecases.
> > > > > > > >
> > > > > > > > The changelog begs the question why we don't just "fix" the
> > > > > > > > system-level stats. It may be useful to include the conclusions from
> > > > > > > > that discussion, and why there is value in keeping the stats this way.
> > > > > > > >
> > > > > > >
> > > > > > > Right. Andrew, can you please add the following para to the changelog?
> > > > > > >
> > > > > > > Why not fix the stats by including both the global and cgroup reclaim
> > > > > > > activity instead of exposing root cgroup's memory.stat? The reason is
> > > > > > > the benefit of having metrics exposing the activity that happens
> > > > > > > purely due to machine capacity rather than localized activity that
> > > > > > > happens due to the limits throughout the cgroup tree. Additionally
> > > > > > > there are userspace tools like sysstat(sar) which reads these stats to
> > > > > > > inform about the system level reclaim activity. So, we should not
> > > > > > > break such use-cases.
> > > > > > >
> > > > > > > > > Signed-off-by: Shakeel Butt <shakeelb@google.com>
> > > > > > > > > Suggested-by: Johannes Weiner <hannes@cmpxchg.org>
> > > > > > > >
> > > > > > > > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> > > > > > >
> > > > > > > Thanks a lot.
> > > > > >
> > > > > > I was quite surprised that the patch is so simple TBH. For some reason
> > > > > > I've still had memories that we do not account for root memcg (likely
> > > > > > because mem_cgroup_is_root(memcg) bail out in the try_charge. But stats
> > > > > > are slightly different here.
> > > > >
> > > > > Yep, we skip the page_counter for root, but keep in mind that cgroup1
> > > > > *does* have a root-level memory.stat, so (for the most part) we've
> > > > > been keeping consumer stats for the root level the whole time.
> > > > >
> > > > > > counters because they are not really all the same. E.g.
> > > > > > - mem_cgroup_charge_statistics accounts for each memcg
> > > > >
> > > > > Yep, that's heritage from cgroup1.
> > > > >
> > > > > > - memcg_charge_kernel_stack relies on pages being associated with a
> > > > > >   memcg and that in turn relies on __memcg_kmem_charge_page which bails
> > > > > >   out on root memcg
> > > > >
> > > > > You're right. It should only bypass the page_counter, but still set
> > > > > page->mem_cgroup = root_mem_cgroup, just like user pages.
> > >
> > > What about kernel threads? We consider them belonging to the root memory
> > > cgroup. Should their memory consumption being considered in root-level stats?
> > >
> > > I'm not sure we really want it, but I guess we need to document how
> > > kernel threads are handled.
> > >
> >
> > What will be the cons of updating root-level stats for kthreads?
>
> It makes total sense for stacks, but not much for the slab memory.
> Because it's really "some part of the total slab memory, which is
> accounted on the memcg level". And it comes with some performance
> overhead.
>
> I'm not really opposing any solution, just saying we need to document
> what's included into this statistics and what not.
>

Yes, I agree. I will explore which stats it makes sense and for which
it does not.

Re: [PATCH] memcg: expose root cgroup's memory.stat

[Chris Down]

Hey Shakeel,

Shakeel Butt writes:
>One way to measure the efficiency of memory reclaim is to look at the
>ratio (pgscan+pfrefill)/pgsteal. However at the moment these stats are
>not updated consistently at the system level and the ratio of these are
>not very meaningful. The pgsteal and pgscan are updated for only global
>reclaim while pgrefill gets updated for global as well as cgroup
>reclaim.
>
>Please note that this difference is only for system level vmstats. The
>cgroup stats returned by memory.stat are actually consistent. The
>cgroup's pgsteal contains number of reclaimed pages for global as well
>as cgroup reclaim. So, one way to get the system level stats is to get
>these stats from root's memory.stat, so, expose memory.stat for the root
>cgroup.
>
>	from Johannes Weiner:
>	There are subtle differences between /proc/vmstat and
>	memory.stat, and cgroup-aware code that wants to watch the full
>	hierarchy currently has to know about these intricacies and
>	translate semantics back and forth.
>
>	Generally having the fully recursive memory.stat at the root
>	level could help a broader range of usecases.
>
>Signed-off-by: Shakeel Butt <shakeelb@google.com>
>Suggested-by: Johannes Weiner <hannes@cmpxchg.org>

The patch looks great, thanks. To that extent you can add my ack:

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

One concern about the API now exposed, though: to a new cgroup v2 user this 
looks fairly dodgy as a sole stat file (except for cgroup.stat) at the root. If 
I used cgroup v2 for the first time and only saw memory.stat and cgroup.stat 
there, but for some reason io.stat and cpu.stat are not available at the root 
but are available everywhere else, I think my first thought would be that the 
cgroup v2 developers must have been on some strong stuff when they came up with 
this ;-)

Even if they're only really duplicating information available elsewhere right 
now, have you considered exposing the rest of the stat files as well so that 
the API maintains a bit more consistency? As a bonus, that also means userspace 
applications can parse in the same way from the root down.