[patch] mm, memcg: do not allow tasks to be attached with zero limit

[patch] mm, memcg: do not allow tasks to be attached with zero limit

[David Rientjes]

This patch prevents tasks from being attached to a memcg if there is a
hard limit of zero.  Additionally, the hard limit may not be changed to
zero if there are tasks attached.

This is consistent with cpusets which do not allow tasks to be attached
if there are no mems and prevents all mems from being removed if there
are tasks attached.

Signed-off-by: David Rientjes <rientjes-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
 mm/memcontrol.c |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3868,9 +3868,14 @@ static int mem_cgroup_write(struct cgroup *cont, struct cftype *cft,
 		ret = res_counter_memparse_write_strategy(buffer, &val);
 		if (ret)
 			break;
-		if (type == _MEM)
+		if (type == _MEM) {
+			/* Don't allow zero limit with tasks attached */
+			if (!val && cgroup_task_count(cont)) {
+				ret = -ENOSPC;
+				break;
+			}
 			ret = mem_cgroup_resize_limit(memcg, val);
-		else
+		} else
 			ret = mem_cgroup_resize_memsw_limit(memcg, val);
 		break;
 	case RES_SOFT_LIMIT:
@@ -5306,6 +5311,10 @@ static int mem_cgroup_can_attach(struct cgroup_subsys *ss,
 	int ret = 0;
 	struct mem_cgroup *memcg = mem_cgroup_from_cont(cgroup);
 
+	/* Don't allow tasks attached with a zero limit */
+	if (!res_counter_read_u64(&memcg->res, RES_LIMIT))
+		return -ENOSPC;
+
 	if (memcg->move_charge_at_immigrate) {
 		struct mm_struct *mm;
 		struct mem_cgroup *from = mem_cgroup_from_task(p);

Re: [patch] mm, memcg: do not allow tasks to be attached with zero limit

[KAMEZAWA Hiroyuki]

On Wed, 7 Mar 2012 19:14:49 -0800 (PST)
David Rientjes <rientjes-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:

> This patch prevents tasks from being attached to a memcg if there is a
> hard limit of zero.  Additionally, the hard limit may not be changed to
> zero if there are tasks attached.
> 
> This is consistent with cpusets which do not allow tasks to be attached
> if there are no mems and prevents all mems from being removed if there
> are tasks attached.
> 
> Signed-off-by: David Rientjes <rientjes-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

I hope Documenation/cgroup/memory.txt should be updated and make this behavior as 'spec'.

Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>


> ---
>  mm/memcontrol.c |   13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3868,9 +3868,14 @@ static int mem_cgroup_write(struct cgroup *cont, struct cftype *cft,
>  		ret = res_counter_memparse_write_strategy(buffer, &val);
>  		if (ret)
>  			break;
> -		if (type == _MEM)
> +		if (type == _MEM) {
> +			/* Don't allow zero limit with tasks attached */
> +			if (!val && cgroup_task_count(cont)) {
> +				ret = -ENOSPC;
> +				break;
> +			}
>  			ret = mem_cgroup_resize_limit(memcg, val);
> -		else
> +		} else
>  			ret = mem_cgroup_resize_memsw_limit(memcg, val);
>  		break;
>  	case RES_SOFT_LIMIT:
> @@ -5306,6 +5311,10 @@ static int mem_cgroup_can_attach(struct cgroup_subsys *ss,
>  	int ret = 0;
>  	struct mem_cgroup *memcg = mem_cgroup_from_cont(cgroup);
>  
> +	/* Don't allow tasks attached with a zero limit */
> +	if (!res_counter_read_u64(&memcg->res, RES_LIMIT))
> +		return -ENOSPC;
> +
>  	if (memcg->move_charge_at_immigrate) {
>  		struct mm_struct *mm;
>  		struct mem_cgroup *from = mem_cgroup_from_task(p);
> 

Re: [patch] mm, memcg: do not allow tasks to be attached with zero limit

[Andrew Morton]

On Wed, 7 Mar 2012 19:14:49 -0800 (PST)
David Rientjes <rientjes@google.com> wrote:

> This patch prevents tasks from being attached to a memcg if there is a
> hard limit of zero.

We're talking about the memcg's limit_in_bytes here, yes?

> Additionally, the hard limit may not be changed to
> zero if there are tasks attached.

hm, well...  why?  That would be user error, wouldn't it?  What is
special about limit_in_bytes=0?  The memcg will also be unviable if
limit_in_bytes=1, but we permit that.

IOW, confused.

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] mm, memcg: do not allow tasks to be attached with zero limit

[KAMEZAWA Hiroyuki]

On Thu, 8 Mar 2012 12:29:51 -0800
Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote:

> On Wed, 7 Mar 2012 19:14:49 -0800 (PST)
> David Rientjes <rientjes-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> 
> > This patch prevents tasks from being attached to a memcg if there is a
> > hard limit of zero.
> 
> We're talking about the memcg's limit_in_bytes here, yes?
> 
> > Additionally, the hard limit may not be changed to
> > zero if there are tasks attached.
> 
> hm, well...  why?  That would be user error, wouldn't it?  What is
> special about limit_in_bytes=0?  The memcg will also be unviable if
> limit_in_bytes=1, but we permit that.
> 
> IOW, confused.
> 
Ah, yes. limit_in_bytes < some small size can cause the same trouble.
Hmm... should we have configurable min_limit_in_bytes as sysctl or root memcg's
attaribute.. ?

-Kame

Re: [patch] mm, memcg: do not allow tasks to be attached with zero limit

[Andrew Morton]

On Fri, 9 Mar 2012 10:22:55 +0900 KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org> wrote:

> On Thu, 8 Mar 2012 12:29:51 -0800
> Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote:
> 
> > On Wed, 7 Mar 2012 19:14:49 -0800 (PST)
> > David Rientjes <rientjes-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> > 
> > > This patch prevents tasks from being attached to a memcg if there is a
> > > hard limit of zero.
> > 
> > We're talking about the memcg's limit_in_bytes here, yes?
> > 
> > > Additionally, the hard limit may not be changed to
> > > zero if there are tasks attached.
> > 
> > hm, well...  why?  That would be user error, wouldn't it?  What is
> > special about limit_in_bytes=0?  The memcg will also be unviable if
> > limit_in_bytes=1, but we permit that.
> > 
> > IOW, confused.
> > 
> Ah, yes. limit_in_bytes < some small size can cause the same trouble.
> Hmm... should we have configurable min_limit_in_bytes as sysctl or root memcg's
> attaribute.. ?

Why do *anything*?  If the operator chose an irrational configuration
then things won't work correctly and the operator will then fix the
configuration?

Re: [patch] mm, memcg: do not allow tasks to be attached with zero limit

[KAMEZAWA Hiroyuki]

On Thu, 8 Mar 2012 17:38:18 -0800
Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote:

> On Fri, 9 Mar 2012 10:22:55 +0900 KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org> wrote:
> 
> > On Thu, 8 Mar 2012 12:29:51 -0800
> > Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote:
> > 
> > > On Wed, 7 Mar 2012 19:14:49 -0800 (PST)
> > > David Rientjes <rientjes-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> > > 
> > > > This patch prevents tasks from being attached to a memcg if there is a
> > > > hard limit of zero.
> > > 
> > > We're talking about the memcg's limit_in_bytes here, yes?
> > > 
> > > > Additionally, the hard limit may not be changed to
> > > > zero if there are tasks attached.
> > > 
> > > hm, well...  why?  That would be user error, wouldn't it?  What is
> > > special about limit_in_bytes=0?  The memcg will also be unviable if
> > > limit_in_bytes=1, but we permit that.
> > > 
> > > IOW, confused.
> > > 
> > Ah, yes. limit_in_bytes < some small size can cause the same trouble.
> > Hmm... should we have configurable min_limit_in_bytes as sysctl or root memcg's
> > attaribute.. ?
> 
> Why do *anything*?  If the operator chose an irrational configuration
> then things won't work correctly and the operator will then fix the
> configuration?
> 

Because the result of 'error operaton' is SIGKILL to a task, which may be
owned by very importang customer of hosting service.

Isn't this severe punishment for error operation ?

Considering again, I have 2 thoughts.

- it should be guarded by MiddleWare, it's not kernel job !
- memcg should be more easy-to-use, friendly to users.

If the result is just an error as EINVAL or EBUSY, I may not be nervous....

Thanks,
-Kame 

Re: [patch] mm, memcg: do not allow tasks to be attached with zero limit

[Johannes Weiner]

On Fri, Mar 09, 2012 at 10:57:06AM +0900, KAMEZAWA Hiroyuki wrote:
> On Thu, 8 Mar 2012 17:38:18 -0800
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Fri, 9 Mar 2012 10:22:55 +0900 KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> > 
> > > On Thu, 8 Mar 2012 12:29:51 -0800
> > > Andrew Morton <akpm@linux-foundation.org> wrote:
> > > 
> > > > On Wed, 7 Mar 2012 19:14:49 -0800 (PST)
> > > > David Rientjes <rientjes@google.com> wrote:
> > > > 
> > > > > This patch prevents tasks from being attached to a memcg if there is a
> > > > > hard limit of zero.
> > > > 
> > > > We're talking about the memcg's limit_in_bytes here, yes?
> > > > 
> > > > > Additionally, the hard limit may not be changed to
> > > > > zero if there are tasks attached.
> > > > 
> > > > hm, well...  why?  That would be user error, wouldn't it?  What is
> > > > special about limit_in_bytes=0?  The memcg will also be unviable if
> > > > limit_in_bytes=1, but we permit that.
> > > > 
> > > > IOW, confused.
> > > > 
> > > Ah, yes. limit_in_bytes < some small size can cause the same trouble.
> > > Hmm... should we have configurable min_limit_in_bytes as sysctl or root memcg's
> > > attaribute.. ?
> > 
> > Why do *anything*?  If the operator chose an irrational configuration
> > then things won't work correctly and the operator will then fix the
> > configuration?
> > 
> 
> Because the result of 'error operaton' is SIGKILL to a task, which may be
> owned by very importang customer of hosting service.
> 
> Isn't this severe punishment for error operation ?
> 
> Considering again, I have 2 thoughts.
> 
> - it should be guarded by MiddleWare, it's not kernel job !
> - memcg should be more easy-to-use, friendly to users.
> 
> If the result is just an error as EINVAL or EBUSY, I may not be nervous....

You can still disable the OOM killer.  If you don't, you can always
get killed, so I'm not convinced by this patch or a sysctl, either.

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] mm, memcg: do not allow tasks to be attached with zero limit

[Michal Hocko]

On Tue 13-03-12 17:51:18, Johannes Weiner wrote:
> On Fri, Mar 09, 2012 at 10:57:06AM +0900, KAMEZAWA Hiroyuki wrote:
> > On Thu, 8 Mar 2012 17:38:18 -0800
> > Andrew Morton <akpm@linux-foundation.org> wrote:
> > 
> > > On Fri, 9 Mar 2012 10:22:55 +0900 KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> > > 
> > > > On Thu, 8 Mar 2012 12:29:51 -0800
> > > > Andrew Morton <akpm@linux-foundation.org> wrote:
> > > > 
> > > > > On Wed, 7 Mar 2012 19:14:49 -0800 (PST)
> > > > > David Rientjes <rientjes@google.com> wrote:
> > > > > 
> > > > > > This patch prevents tasks from being attached to a memcg if there is a
> > > > > > hard limit of zero.
> > > > > 
> > > > > We're talking about the memcg's limit_in_bytes here, yes?
> > > > > 
> > > > > > Additionally, the hard limit may not be changed to
> > > > > > zero if there are tasks attached.
> > > > > 
> > > > > hm, well...  why?  That would be user error, wouldn't it?  What is
> > > > > special about limit_in_bytes=0?  The memcg will also be unviable if
> > > > > limit_in_bytes=1, but we permit that.
> > > > > 
> > > > > IOW, confused.
> > > > > 
> > > > Ah, yes. limit_in_bytes < some small size can cause the same trouble.
> > > > Hmm... should we have configurable min_limit_in_bytes as sysctl or root memcg's
> > > > attaribute.. ?
> > > 
> > > Why do *anything*?  If the operator chose an irrational configuration
> > > then things won't work correctly and the operator will then fix the
> > > configuration?
> > > 
> > 
> > Because the result of 'error operaton' is SIGKILL to a task, which may be
> > owned by very importang customer of hosting service.
> > 
> > Isn't this severe punishment for error operation ?
> > 
> > Considering again, I have 2 thoughts.
> > 
> > - it should be guarded by MiddleWare, it's not kernel job !
> > - memcg should be more easy-to-use, friendly to users.
> > 
> > If the result is just an error as EINVAL or EBUSY, I may not be nervous....
> 
> You can still disable the OOM killer.  If you don't, you can always
> get killed, so I'm not convinced by this patch or a sysctl, either.

Agreed, kernel usually doesn't care about insane settings and it does
what it is told to do (why to safe somebody from shooting its own
foot?).

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>