[RFC PATCH] cgroup: use root_mem_cgroup as css when current is not enabled

[RFC PATCH] cgroup: use root_mem_cgroup as css when current is not enabled

[zhaoyang.huang]

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

Memory charged on group B abserved on belowing v2 hierarchy where we just would
like to only have group E's memory be controlled and B's descendants compete freely
for memory. This should be the consequences of unified hierarchy. Solve this by
have the cgroup without valid memory css alloced use root_mem_cgroup instead of
its ancestor's.

 A(subtree_control = memory) - B(subtree_control = NULL) - C()
                                                         \ D()
			     - E(subtree_control = memory) - F()
							   \ G()

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

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 1779ccd..b29b3f6 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -533,6 +533,14 @@ static struct cgroup_subsys_state *cgroup_e_css_by_mask(struct cgroup *cgrp,
 	 * can't test the csses directly.  Test ss_mask.
 	 */
 	while (!(cgroup_ss_mask(cgrp) & (1 << ss->id))) {
+		/*
+		 * charging to the parent cgroup which hasn't distribute
+		 * memory control to its descendants doesn't make sense
+		 * especially on cgroup v2, where the parent could be configured
+		 * to use memory controller as its sibling want to use it
+		 */
+		if (memory_cgrp_id == ss->id)
+			return &root_mem_cgroup->css;
 		cgrp = cgroup_parent(cgrp);
 		if (!cgrp)
 			return NULL;
-- 
1.9.1

Re: [RFC PATCH] cgroup: use root_mem_cgroup as css when current is not enabled

[Tejun Heo]

On Fri, Aug 12, 2022 at 06:09:26PM +0800, zhaoyang.huang wrote:
> From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> 
> Memory charged on group B abserved on belowing v2 hierarchy where we just would
> like to only have group E's memory be controlled and B's descendants compete freely
> for memory. This should be the consequences of unified hierarchy. Solve this by
> have the cgroup without valid memory css alloced use root_mem_cgroup instead of
> its ancestor's.
> 
>  A(subtree_control = memory) - B(subtree_control = NULL) - C()
>                                                          \ D()
> 			     - E(subtree_control = memory) - F()
> 							   \ G()
> 
> Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> ---
>  kernel/cgroup/cgroup.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index 1779ccd..b29b3f6 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -533,6 +533,14 @@ static struct cgroup_subsys_state *cgroup_e_css_by_mask(struct cgroup *cgrp,
>  	 * can't test the csses directly.  Test ss_mask.
>  	 */
>  	while (!(cgroup_ss_mask(cgrp) & (1 << ss->id))) {
> +		/*
> +		 * charging to the parent cgroup which hasn't distribute
> +		 * memory control to its descendants doesn't make sense
> +		 * especially on cgroup v2, where the parent could be configured
> +		 * to use memory controller as its sibling want to use it
> +		 */
> +		if (memory_cgrp_id == ss->id)
> +			return &root_mem_cgroup->css;

This is gonna be a hard nack. A given cgroup always encompasses all the
resources consumed in its self-including subtree.

Thanks.

-- 
tejun

Re: [RFC PATCH] cgroup: use root_mem_cgroup as css when current is not enabled

[Zhaoyang Huang]

On Sat, Aug 13, 2022 at 3:06 AM Tejun Heo <tj@kernel.org> wrote:
>
> On Fri, Aug 12, 2022 at 06:09:26PM +0800, zhaoyang.huang wrote:
> > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> >
> > Memory charged on group B abserved on belowing v2 hierarchy where we just would
> > like to only have group E's memory be controlled and B's descendants compete freely
> > for memory. This should be the consequences of unified hierarchy. Solve this by
> > have the cgroup without valid memory css alloced use root_mem_cgroup instead of
> > its ancestor's.
> >
> >  A(subtree_control = memory) - B(subtree_control = NULL) - C()
> >                                                          \ D()
> >                            - E(subtree_control = memory) - F()
> >                                                          \ G()
> >
> > Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> > ---
> >  kernel/cgroup/cgroup.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> > index 1779ccd..b29b3f6 100644
> > --- a/kernel/cgroup/cgroup.c
> > +++ b/kernel/cgroup/cgroup.c
> > @@ -533,6 +533,14 @@ static struct cgroup_subsys_state *cgroup_e_css_by_mask(struct cgroup *cgrp,
> >        * can't test the csses directly.  Test ss_mask.
> >        */
> >       while (!(cgroup_ss_mask(cgrp) & (1 << ss->id))) {
> > +             /*
> > +              * charging to the parent cgroup which hasn't distribute
> > +              * memory control to its descendants doesn't make sense
> > +              * especially on cgroup v2, where the parent could be configured
> > +              * to use memory controller as its sibling want to use it
> > +              */
> > +             if (memory_cgrp_id == ss->id)
> > +                     return &root_mem_cgroup->css;
>
> This is gonna be a hard nack. A given cgroup always encompasses all the
> resources consumed in its self-including subtree.
>
> Thanks.
IMHO, I would like to say if it makes more sense as "A given cgroup
always encompasses all the resources consumed in its ENABLED
self-including subtree." Otherwise, how should I couple with the
scenarios I raised in the commit message which I prefer parts of the
subtrees compete for "memory" while others are free for it. The free
here is not only without "min/low/high watermarks" but also not
charged to their own LRU.
>
> --
> tejun

Re: [RFC PATCH] cgroup: use root_mem_cgroup as css when current is not enabled

[Zhaoyang Huang]

On Sun, Aug 14, 2022 at 2:40 PM Zhaoyang Huang <huangzhaoyang@gmail.com> wrote:
>
> On Sat, Aug 13, 2022 at 3:06 AM Tejun Heo <tj@kernel.org> wrote:
> >
> > On Fri, Aug 12, 2022 at 06:09:26PM +0800, zhaoyang.huang wrote:
> > > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> > >
> > > Memory charged on group B abserved on belowing v2 hierarchy where we just would
> > > like to only have group E's memory be controlled and B's descendants compete freely
> > > for memory. This should be the consequences of unified hierarchy. Solve this by
> > > have the cgroup without valid memory css alloced use root_mem_cgroup instead of
> > > its ancestor's.
> > >
> > >  A(subtree_control = memory) - B(subtree_control = NULL) - C()
> > >                                                          \ D()
> > >                            - E(subtree_control = memory) - F()
> > >                                                          \ G()
> > >
> > > Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> > > ---
> > >  kernel/cgroup/cgroup.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> > > index 1779ccd..b29b3f6 100644
> > > --- a/kernel/cgroup/cgroup.c
> > > +++ b/kernel/cgroup/cgroup.c
> > > @@ -533,6 +533,14 @@ static struct cgroup_subsys_state *cgroup_e_css_by_mask(struct cgroup *cgrp,
> > >        * can't test the csses directly.  Test ss_mask.
> > >        */
> > >       while (!(cgroup_ss_mask(cgrp) & (1 << ss->id))) {
> > > +             /*
> > > +              * charging to the parent cgroup which hasn't distribute
> > > +              * memory control to its descendants doesn't make sense
> > > +              * especially on cgroup v2, where the parent could be configured
> > > +              * to use memory controller as its sibling want to use it
> > > +              */
> > > +             if (memory_cgrp_id == ss->id)
> > > +                     return &root_mem_cgroup->css;
> >
> > This is gonna be a hard nack. A given cgroup always encompasses all the
> > resources consumed in its self-including subtree.
> >
> > Thanks.
> IMHO, I would like to say if it makes more sense as "A given cgroup
> always encompasses all the resources consumed in its ENABLED
> self-including subtree." Otherwise, how should I couple with the
> scenarios I raised in the commit message which I prefer parts of the
> subtrees compete for "memory" while others are free for it. The free
> here is not only without "min/low/high watermarks" but also not
> charged to their own LRU.
I would like to state more why these make sense. Memory cgroup is a
little bit different to other cgroups, that is, memcg will have real
physical resources attached, say pages. From perspective of memory
reclaiming, it is odd to find that pages under free memcgs are charged
to separate LRUs but without any management(no watermark control) and
perhaps affect workingset mechanism by LRU reason. Furthermore, v2
should grant the groups with the right to reject the subsys which
introduced by sibling enable, which could be deemed as v2's
inconvenient. The memcg could also apply subtree_control to enroll all
memory back whenever it want.
> >
> > --
> > tejun

Re: [RFC PATCH] cgroup: use root_mem_cgroup as css when current is not enabled

[Zhaoyang Huang]

On Mon, Aug 15, 2022 at 9:38 AM Zhaoyang Huang <huangzhaoyang@gmail.com> wrote:
>
> On Sun, Aug 14, 2022 at 2:40 PM Zhaoyang Huang <huangzhaoyang@gmail.com> wrote:
> >
> > On Sat, Aug 13, 2022 at 3:06 AM Tejun Heo <tj@kernel.org> wrote:
> > >
> > > On Fri, Aug 12, 2022 at 06:09:26PM +0800, zhaoyang.huang wrote:
> > > > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> > > >
> > > > Memory charged on group B abserved on belowing v2 hierarchy where we just would
> > > > like to only have group E's memory be controlled and B's descendants compete freely
> > > > for memory. This should be the consequences of unified hierarchy. Solve this by
> > > > have the cgroup without valid memory css alloced use root_mem_cgroup instead of
> > > > its ancestor's.
> > > >
> > > >  A(subtree_control = memory) - B(subtree_control = NULL) - C()
> > > >                                                          \ D()
> > > >                            - E(subtree_control = memory) - F()
> > > >                                                          \ G()
> > > >
> > > > Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> > > > ---
> > > >  kernel/cgroup/cgroup.c | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > >
> > > > diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> > > > index 1779ccd..b29b3f6 100644
> > > > --- a/kernel/cgroup/cgroup.c
> > > > +++ b/kernel/cgroup/cgroup.c
> > > > @@ -533,6 +533,14 @@ static struct cgroup_subsys_state *cgroup_e_css_by_mask(struct cgroup *cgrp,
> > > >        * can't test the csses directly.  Test ss_mask.
> > > >        */
> > > >       while (!(cgroup_ss_mask(cgrp) & (1 << ss->id))) {
> > > > +             /*
> > > > +              * charging to the parent cgroup which hasn't distribute
> > > > +              * memory control to its descendants doesn't make sense
> > > > +              * especially on cgroup v2, where the parent could be configured
> > > > +              * to use memory controller as its sibling want to use it
> > > > +              */
> > > > +             if (memory_cgrp_id == ss->id)
> > > > +                     return &root_mem_cgroup->css;
> > >
> > > This is gonna be a hard nack. A given cgroup always encompasses all the
> > > resources consumed in its self-including subtree.
> > >
> > > Thanks.
> > IMHO, I would like to say if it makes more sense as "A given cgroup
> > always encompasses all the resources consumed in its ENABLED
> > self-including subtree." Otherwise, how should I couple with the
> > scenarios I raised in the commit message which I prefer parts of the
> > subtrees compete for "memory" while others are free for it. The free
> > here is not only without "min/low/high watermarks" but also not
> > charged to their own LRU.
> I would like to state more why these make sense. Memory cgroup is a
> little bit different to other cgroups, that is, memcg will have real
> physical resources attached, say pages. From perspective of memory
> reclaiming, it is odd to find that pages under free memcgs are charged
> to separate LRUs but without any management(no watermark control) and
> perhaps affect workingset mechanism by LRU reason. Furthermore, v2
> should grant the groups with the right to reject the subsys which
> introduced by sibling enable, which could be deemed as v2's
> inconvenient. The memcg could also apply subtree_control to enroll all
> memory back whenever it want.
As suggested by zefan, raise the practical scenario here to make more
sense. The issue is observed in android system where per-app cgroup is
demanded by freezer subsys and part of them require memory control.
Under this scenario, less efficient memory reclaim is observed when
comparing with no memory control. It is believed that multi LRU
scanning introduces some of the overhead. Furthermore, page thrashing
is also heavier than global LRU which could be the consequences of
partial failure of WORKINGSET mechanism as LRU is too short to protect
the active pages.
> > >
> > > --
> > > tejun

Re: [RFC PATCH] cgroup: use root_mem_cgroup as css when current is not enabled

[Tejun Heo]

Hello,

On Mon, Aug 15, 2022 at 05:17:03PM +0800, Zhaoyang Huang wrote:
> > > IMHO, I would like to say if it makes more sense as "A given cgroup
> > > always encompasses all the resources consumed in its ENABLED
> > > self-including subtree." Otherwise, how should I couple with the
> > > scenarios I raised in the commit message which I prefer parts of the
> > > subtrees compete for "memory" while others are free for it. The free
> > > here is not only without "min/low/high watermarks" but also not
> > > charged to their own LRU.
> > I would like to state more why these make sense. Memory cgroup is a
> > little bit different to other cgroups, that is, memcg will have real
> > physical resources attached, say pages. From perspective of memory
> > reclaiming, it is odd to find that pages under free memcgs are charged
> > to separate LRUs but without any management(no watermark control) and
> > perhaps affect workingset mechanism by LRU reason. Furthermore, v2
> > should grant the groups with the right to reject the subsys which
> > introduced by sibling enable, which could be deemed as v2's
> > inconvenient. The memcg could also apply subtree_control to enroll all
> > memory back whenever it want.
> As suggested by zefan, raise the practical scenario here to make more
> sense. The issue is observed in android system where per-app cgroup is
> demanded by freezer subsys and part of them require memory control.
> Under this scenario, less efficient memory reclaim is observed when
> comparing with no memory control. It is believed that multi LRU
> scanning introduces some of the overhead. Furthermore, page thrashing
> is also heavier than global LRU which could be the consequences of
> partial failure of WORKINGSET mechanism as LRU is too short to protect
> the active pages.

The basic architecture isn't gonna change and there are fundamental reasons
why things are the way they are. The resources, especially the main ones,
are entangled with each other - e.g. memory, io and cpu are entangled with
each other through reclaim. While we aren't capturing the cpu part yet but
we now do a largely acceptable job of controlling memory and io together.

This is reduction of flexibility compared to cgroup1 and can cause some
inconvenience when transitioning from cgroup1 but the experience has been
that the pros clearly outweigh the cons. Even here, none of the problems
you're listing are architectural. If there are memcg behavior problems, we
should fix them in memcg, not work around by twisting the basic
architecture. Are the problematic memcg behaviors reproducible in upstream?
If so, can you please make a detailed report on those?

Thanks.

-- 
tejun