[BUG?] [PATCH] soft limits and root cgroups

[BUG?] [PATCH] soft limits and root cgroups

[Kirill A. Shutemov]

Currently, mem_cgroup_update_tree() on root cgroup calls only on
uncharge, not on charge.

Is it a bug or not?

Patch to fix, if it's a bug:

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 8aa6026..6babef1 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1366,13 +1366,15 @@ static int __mem_cgroup_try_charge(struct mm_struct *mm
                        goto nomem;
                }
        }
+
+done:
        /*
         * Insert ancestor (and ancestor's ancestors), to softlimit RB-tree.
         * if they exceeds softlimit.
         */
        if (mem_cgroup_soft_limit_check(mem))
                mem_cgroup_update_tree(mem, page);
-done:
+
        return 0;
 nomem:
        css_put(&mem->css);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[cleanup][PATCH mmotm]memcg: don't call mem_cgroup_soft_limit_check() against root cgroup (Re: [BUG?] [PATCH] soft limits and root cgroups)

[Daisuke Nishimura]

hi,

On Mon, 7 Dec 2009 20:41:16 +0200, "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> Currently, mem_cgroup_update_tree() on root cgroup calls only on
> uncharge, not on charge.
> 
> Is it a bug or not?
> 
> Patch to fix, if it's a bug:
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 8aa6026..6babef1 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1366,13 +1366,15 @@ static int __mem_cgroup_try_charge(struct mm_struct *mm
>                         goto nomem;
>                 }
>         }
> +
> +done:
>         /*
>          * Insert ancestor (and ancestor's ancestors), to softlimit RB-tree.
>          * if they exceeds softlimit.
>          */
>         if (mem_cgroup_soft_limit_check(mem))
>                 mem_cgroup_update_tree(mem, page);
> -done:
> +
>         return 0;
>  nomem:
>         css_put(&mem->css);
> 
I think it's not a bug, because softlimit doesn't work for root cgroup.
(IIUC, it's not disabled to write to <root cgroup>/memory.soft_limit_in_bytes, but
it has no use because root cgroup doesn't use res_counter.)

So, I think not to call mem_cgroup_soft_limit_check()(and mem_cgroup_update_tree)
against root cgroup on uncharge path would be a right fix.


How about this ?
===
From: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>

current memory cgroup doesn't use res_counter about root cgroup, so soft limits
on root cgroup has no use.
This patch disables writing to <root cgroup>/memory.soft_limit_in_bytes and
changes uncharge path not to call mem_cgroup_soft_limit_check() against root
cgroup.

Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
---
 Documentation/cgroups/memory.txt |    1 +
 mm/memcontrol.c                  |    6 +++++-
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
index b871f25..e1b5328 100644
--- a/Documentation/cgroups/memory.txt
+++ b/Documentation/cgroups/memory.txt
@@ -413,6 +413,7 @@ NOTE1: Soft limits take effect over a long period of time, since they involve
        reclaiming memory for balancing between memory cgroups
 NOTE2: It is recommended to set the soft limit always below the hard limit,
        otherwise the hard limit will take precedence.
+NOTE3: We cannot set soft limits on the root cgroup any more.
 
 8. TODO
 
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 661b8c6..0751533 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2056,7 +2056,7 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
 	mz = page_cgroup_zoneinfo(pc);
 	unlock_page_cgroup(pc);
 
-	if (mem_cgroup_soft_limit_check(mem))
+	if (!mem_cgroup_is_root(mem) && mem_cgroup_soft_limit_check(mem))
 		mem_cgroup_update_tree(mem, page);
 	/* at swapout, this memcg will be accessed to record to swap */
 	if (ctype != MEM_CGROUP_CHARGE_TYPE_SWAPOUT)
@@ -2787,6 +2787,10 @@ static int mem_cgroup_write(struct cgroup *cont, struct cftype *cft,
 			ret = mem_cgroup_resize_memsw_limit(memcg, val);
 		break;
 	case RES_SOFT_LIMIT:
+		if (mem_cgroup_is_root(memcg)) { /* Can't set limit on root */
+			ret = -EINVAL;
+			break;
+		}
 		ret = res_counter_memparse_write_strategy(buffer, &val);
 		if (ret)
 			break;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [cleanup][PATCH mmotm]memcg: don't call mem_cgroup_soft_limit_check() against root cgroup (Re: [BUG?] [PATCH] soft limits and root cgroups)

[Balbir Singh]

* nishimura@mxp.nes.nec.co.jp <nishimura@mxp.nes.nec.co.jp> [2009-12-08 10:09:54]:

> hi,
> 
> On Mon, 7 Dec 2009 20:41:16 +0200, "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> > Currently, mem_cgroup_update_tree() on root cgroup calls only on
> > uncharge, not on charge.
> > 
> > Is it a bug or not?
> > 
> > Patch to fix, if it's a bug:
> > 
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 8aa6026..6babef1 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -1366,13 +1366,15 @@ static int __mem_cgroup_try_charge(struct mm_struct *mm
> >                         goto nomem;
> >                 }
> >         }
> > +
> > +done:
> >         /*
> >          * Insert ancestor (and ancestor's ancestors), to softlimit RB-tree.
> >          * if they exceeds softlimit.
> >          */
> >         if (mem_cgroup_soft_limit_check(mem))
> >                 mem_cgroup_update_tree(mem, page);
> > -done:
> > +
> >         return 0;
> >  nomem:
> >         css_put(&mem->css);
> > 
> I think it's not a bug, because softlimit doesn't work for root cgroup.
> (IIUC, it's not disabled to write to <root cgroup>/memory.soft_limit_in_bytes, but
> it has no use because root cgroup doesn't use res_counter.)
> 
> So, I think not to call mem_cgroup_soft_limit_check()(and mem_cgroup_update_tree)
> against root cgroup on uncharge path would be a right fix.
> 
> 
> How about this ?
> ===
> From: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> 
> current memory cgroup doesn't use res_counter about root cgroup, so soft limits
> on root cgroup has no use.
> This patch disables writing to <root cgroup>/memory.soft_limit_in_bytes and
> changes uncharge path not to call mem_cgroup_soft_limit_check() against root
> cgroup.
> 
> Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> ---
>  Documentation/cgroups/memory.txt |    1 +
>  mm/memcontrol.c                  |    6 +++++-
>  2 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
> index b871f25..e1b5328 100644
> --- a/Documentation/cgroups/memory.txt
> +++ b/Documentation/cgroups/memory.txt
> @@ -413,6 +413,7 @@ NOTE1: Soft limits take effect over a long period of time, since they involve
>         reclaiming memory for balancing between memory cgroups
>  NOTE2: It is recommended to set the soft limit always below the hard limit,
>         otherwise the hard limit will take precedence.
> +NOTE3: We cannot set soft limits on the root cgroup any more.
> 
>  8. TODO
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 661b8c6..0751533 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2056,7 +2056,7 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
>  	mz = page_cgroup_zoneinfo(pc);
>  	unlock_page_cgroup(pc);
> 
> -	if (mem_cgroup_soft_limit_check(mem))
> +	if (!mem_cgroup_is_root(mem) && mem_cgroup_soft_limit_check(mem))
>  		mem_cgroup_update_tree(mem, page);

May be the mem_cgroup_is_root() check should go inside
mem_cgroup_soft_limit_check() for future call sites as well.

>  	/* at swapout, this memcg will be accessed to record to swap */
>  	if (ctype != MEM_CGROUP_CHARGE_TYPE_SWAPOUT)
> @@ -2787,6 +2787,10 @@ static int mem_cgroup_write(struct cgroup *cont, struct cftype *cft,
>  			ret = mem_cgroup_resize_memsw_limit(memcg, val);
>  		break;
>  	case RES_SOFT_LIMIT:
> +		if (mem_cgroup_is_root(memcg)) { /* Can't set limit on root */
> +			ret = -EINVAL;
> +			break;
> +		}
>  		ret = res_counter_memparse_write_strategy(buffer, &val);
>  		if (ret)
>  			break;
> 

looks good to me

Acked-by: Balbir Singh <balbir@linux.vnet.ibm.com>
 

-- 
	Balbir

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [cleanup][PATCH mmotm]memcg: don't call mem_cgroup_soft_limit_check() against root cgroup (Re: [BUG?] [PATCH] soft limits and root cgroups)

[Daisuke Nishimura]

> > From: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> > 
> > current memory cgroup doesn't use res_counter about root cgroup, so soft limits
> > on root cgroup has no use.
> > This patch disables writing to <root cgroup>/memory.soft_limit_in_bytes and
> > changes uncharge path not to call mem_cgroup_soft_limit_check() against root
> > cgroup.
> > 
> > Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> > ---
> >  Documentation/cgroups/memory.txt |    1 +
> >  mm/memcontrol.c                  |    6 +++++-
> >  2 files changed, 6 insertions(+), 1 deletions(-)
> > 
> > diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
> > index b871f25..e1b5328 100644
> > --- a/Documentation/cgroups/memory.txt
> > +++ b/Documentation/cgroups/memory.txt
> > @@ -413,6 +413,7 @@ NOTE1: Soft limits take effect over a long period of time, since they involve
> >         reclaiming memory for balancing between memory cgroups
> >  NOTE2: It is recommended to set the soft limit always below the hard limit,
> >         otherwise the hard limit will take precedence.
> > +NOTE3: We cannot set soft limits on the root cgroup any more.
> > 
> >  8. TODO
> > 
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 661b8c6..0751533 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -2056,7 +2056,7 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
> >  	mz = page_cgroup_zoneinfo(pc);
> >  	unlock_page_cgroup(pc);
> > 
> > -	if (mem_cgroup_soft_limit_check(mem))
> > +	if (!mem_cgroup_is_root(mem) && mem_cgroup_soft_limit_check(mem))
> >  		mem_cgroup_update_tree(mem, page);
> 
> May be the mem_cgroup_is_root() check should go inside
> mem_cgroup_soft_limit_check() for future call sites as well.
> 
I agree, thank you for your suggestion.
I'll update this patch with your ack, but considering it's in merge window now,
I'll hold this patch a while(this is not a so urgent patch).

Thanks,
Daisuke Nishimura.

> >  	/* at swapout, this memcg will be accessed to record to swap */
> >  	if (ctype != MEM_CGROUP_CHARGE_TYPE_SWAPOUT)
> > @@ -2787,6 +2787,10 @@ static int mem_cgroup_write(struct cgroup *cont, struct cftype *cft,
> >  			ret = mem_cgroup_resize_memsw_limit(memcg, val);
> >  		break;
> >  	case RES_SOFT_LIMIT:
> > +		if (mem_cgroup_is_root(memcg)) { /* Can't set limit on root */
> > +			ret = -EINVAL;
> > +			break;
> > +		}
> >  		ret = res_counter_memparse_write_strategy(buffer, &val);
> >  		if (ret)
> >  			break;
> > 
> 
> looks good to me
> 
> Acked-by: Balbir Singh <balbir@linux.vnet.ibm.com>
>  
> 
> -- 
> 	Balbir

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [cleanup][PATCH mmotm]memcg: don't call mem_cgroup_soft_limit_check() against root cgroup (Re: [BUG?] [PATCH] soft limits and root cgroups)

[KAMEZAWA Hiroyuki]

On Tue, 8 Dec 2009 10:09:54 +0900
Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> wrote:
> From: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> 
> current memory cgroup doesn't use res_counter about root cgroup, so soft limits
> on root cgroup has no use.
> This patch disables writing to <root cgroup>/memory.soft_limit_in_bytes and
> changes uncharge path not to call mem_cgroup_soft_limit_check() against root
> cgroup.
> 
> Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>

Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Thank you.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUG?] [PATCH] soft limits and root cgroups

[Kirill A. Shutemov]

On Mon, Dec 7, 2009 at 8:41 PM, Kirill A. Shutemov <kirill@shutemov.name> wrote:
> Currently, mem_cgroup_update_tree() on root cgroup calls only on
> uncharge, not on charge.
>
> Is it a bug or not?

Any comments?

> Patch to fix, if it's a bug:
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 8aa6026..6babef1 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1366,13 +1366,15 @@ static int __mem_cgroup_try_charge(struct mm_struct *mm
>                        goto nomem;
>                }
>        }
> +
> +done:
>        /*
>         * Insert ancestor (and ancestor's ancestors), to softlimit RB-tree.
>         * if they exceeds softlimit.
>         */
>        if (mem_cgroup_soft_limit_check(mem))
>                mem_cgroup_update_tree(mem, page);
> -done:
> +
>        return 0;
>  nomem:
>        css_put(&mem->css);
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUG?] [PATCH] soft limits and root cgroups

[Daisuke Nishimura]

On Wed, 9 Dec 2009 11:37:30 +0200
"Kirill A. Shutemov" <kirill@shutemov.name> wrote:

> On Mon, Dec 7, 2009 at 8:41 PM, Kirill A. Shutemov <kirill@shutemov.name> wrote:
> > Currently, mem_cgroup_update_tree() on root cgroup calls only on
> > uncharge, not on charge.
> >
> > Is it a bug or not?
> 
> Any comments?
> 
It's not a bug.
Please see my comments and patch ;)

Thanks,
Daisuke Nishimura.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>