[PATCH] mm: introduce entrance for root_mem_cgroup's current

[PATCH] mm: introduce entrance for root_mem_cgroup's current

[zhaoyang.huang]

From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>

Introducing memory.root_current for the memory charges on root_mem_cgroup.

Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
---
 mm/memcontrol.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index ab457f0..158d4232 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -6681,6 +6681,11 @@ static ssize_t memory_reclaim(struct kernfs_open_file *of, char *buf,
 
 static struct cftype memory_files[] = {
 	{
+		.name = "root_current",
+		.flags = CFTYPE_ONLY_ON_ROOT,
+		.read_u64 = memory_current_read,
+	},
+	{
 		.name = "current",
 		.flags = CFTYPE_NOT_ON_ROOT,
 		.read_u64 = memory_current_read,
-- 
1.9.1

Re: [PATCH] mm: introduce entrance for root_mem_cgroup's current

[Shakeel Butt]

On Wed, Feb 1, 2023 at 8:34 PM zhaoyang.huang <zhaoyang.huang@unisoc.com> wrote:
>
> From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
>
> Introducing memory.root_current for the memory charges on root_mem_cgroup.

Why and how are you planning to use it?

Re: [PATCH] mm: introduce entrance for root_mem_cgroup's current

[Zhaoyang Huang]

On Thu, Feb 2, 2023 at 2:30 PM Shakeel Butt <shakeelb@google.com> wrote:
>
> On Wed, Feb 1, 2023 at 8:34 PM zhaoyang.huang <zhaoyang.huang@unisoc.com> wrote:
> >
> > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> >
> > Introducing memory.root_current for the memory charges on root_mem_cgroup.
>
> Why and how are you planning to use it?
root_mem_cgroup is lack of such statistic entry for debug purpose,
which is the counterpart of memory.current within other memcgs

Re: [PATCH] mm: introduce entrance for root_mem_cgroup's current

[Michal Hocko]

On Thu 02-02-23 12:32:57, zhaoyang.huang wrote:
> From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> 
> Introducing memory.root_current for the memory charges on root_mem_cgroup.

Charges are not currently accounted for the root memcg universally. See
try_charge which is used for all user space and skmem charges. I am not
100% sure about objcg based accounting because there is no explicit
check for the root memcg but this might be hidden somewhere as well.

That means that the patch as is doesn't really provide and usable value.
The root exemption has been removed in the past but that has been
reverted due to a regression. See ce00a967377b ("mm: memcontrol: revert
use of root_mem_cgroup res_counter") for more.

> Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> ---
>  mm/memcontrol.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index ab457f0..158d4232 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -6681,6 +6681,11 @@ static ssize_t memory_reclaim(struct kernfs_open_file *of, char *buf,
>  
>  static struct cftype memory_files[] = {
>  	{
> +		.name = "root_current",
> +		.flags = CFTYPE_ONLY_ON_ROOT,
> +		.read_u64 = memory_current_read,
> +	},
> +	{
>  		.name = "current",
>  		.flags = CFTYPE_NOT_ON_ROOT,
>  		.read_u64 = memory_current_read,
> -- 
> 1.9.1

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: introduce entrance for root_mem_cgroup's current

[Shakeel Butt]

On Thu, Feb 2, 2023 at 12:27 AM Michal Hocko <mhocko@suse.com> wrote:
>
> On Thu 02-02-23 12:32:57, zhaoyang.huang wrote:
> > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> >
> > Introducing memory.root_current for the memory charges on root_mem_cgroup.
>
> Charges are not currently accounted for the root memcg universally. See
> try_charge which is used for all user space and skmem charges. I am not
> 100% sure about objcg based accounting because there is no explicit
> check for the root memcg but this might be hidden somewhere as well.

Yes in __get_obj_cgroup_from_memcg(). However the reason to use
try_charge_memcg() to bypass root check was to avoid the race with
reparenting. More details in c5c8b16b596e ("mm: memcontrol: fix
root_mem_cgroup charging").

>
> That means that the patch as is doesn't really provide and usable value.
> The root exemption has been removed in the past but that has been
> reverted due to a regression. See ce00a967377b ("mm: memcontrol: revert
> use of root_mem_cgroup res_counter") for more.
>

One advantage I can see is if someone is looking for usage for all top
containers (alive or zombie) but I wanted to know if that was the real
motivation behind the patch.

Re: [PATCH] mm: introduce entrance for root_mem_cgroup's current

[Michal Hocko]

On Thu 02-02-23 10:24:08, Shakeel Butt wrote:
> On Thu, Feb 2, 2023 at 12:27 AM Michal Hocko <mhocko@suse.com> wrote:
> >
> > On Thu 02-02-23 12:32:57, zhaoyang.huang wrote:
> > > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> > >
> > > Introducing memory.root_current for the memory charges on root_mem_cgroup.
> >
> > Charges are not currently accounted for the root memcg universally. See
> > try_charge which is used for all user space and skmem charges. I am not
> > 100% sure about objcg based accounting because there is no explicit
> > check for the root memcg but this might be hidden somewhere as well.
> 
> Yes in __get_obj_cgroup_from_memcg(). However the reason to use
> try_charge_memcg() to bypass root check was to avoid the race with
> reparenting. More details in c5c8b16b596e ("mm: memcontrol: fix
> root_mem_cgroup charging").

Thanks for the pointer!

> > That means that the patch as is doesn't really provide and usable value.
> > The root exemption has been removed in the past but that has been
> > reverted due to a regression. See ce00a967377b ("mm: memcontrol: revert
> > use of root_mem_cgroup res_counter") for more.
> >
> 
> One advantage I can see is if someone is looking for usage for all top
> containers (alive or zombie) but I wanted to know if that was the real
> motivation behind the patch.

Isn't that just a global stats that we already display via /proc files?

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: introduce entrance for root_mem_cgroup's current

[Shakeel Butt]

On Fri, Feb 03, 2023 at 09:58:56AM +0100, Michal Hocko wrote:
[...]
> > 
> > One advantage I can see is if someone is looking for usage for all top
> > containers (alive or zombie) but I wanted to know if that was the real
> > motivation behind the patch.
> 
> Isn't that just a global stats that we already display via /proc files?
> 

Things are a bit complicated for kernel memory. Let's take a simple
example where there are no processes in the root memcg. In this case the
user memory stats should be similar to the global stats under /proc
because we always charge user memory. However the kernel memory has to
be opted-in to be accounted. So, we have a lot of allocations which are
in the global stats but not in the memcg stats. We can traverse the top
level memcgs to get kernel stats and subtract it from the global stats
which will give the sum of zombie kernel memory and unaccounted kernel
memory. For debugging and history/analysis purpose, differentiating
between zombie and unaccounted makes sense.

Re: [PATCH] mm: introduce entrance for root_mem_cgroup's current

[Roman Gushchin]

On Thu, Feb 02, 2023 at 09:27:39AM +0100, Michal Hocko wrote:
> On Thu 02-02-23 12:32:57, zhaoyang.huang wrote:
> > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> > 
> > Introducing memory.root_current for the memory charges on root_mem_cgroup.
> 
> Charges are not currently accounted for the root memcg universally. See
> try_charge which is used for all user space and skmem charges. I am not
> 100% sure about objcg based accounting because there is no explicit
> check for the root memcg but this might be hidden somewhere as well.

root_mem_cgroup has no corresponding obj_cgroup: see memcg_online_kmem().

Thanks