[PATCH 0/7] cgroups: New max number of tasks subsystem (was: cgroups rlim subsystem)

[PATCH 0/7] cgroups: New max number of tasks subsystem (was: cgroups rlim subsystem)

[Frederic Weisbecker]

Hi all,

Changes in this release are:

* switch from general rlim subsystem to self-contained max number of tasks
subsystem

* Don't use subsys->fork() callback but a static call to avoid cgroups
complications.

* When moving a task accross cgroups, don't charge/uncharge the common
ancestors, to fix race against concurrent forks.
To fix that, Paul proposed a res_counter_move_charge() that do
all in once. But we need to be able to cancel the attachment in case
a following subsystem refuses the attachment itself. Thus the charge
and uncharge still need to be seperate. More specifically, we can
only uncharge the old cgroup once we are sure of the attachment. Because
we can't cancel that uncharge later due to possible forks in the middle.

Thanks.

Frederic Weisbecker (7):
  cgroups: Add res_counter_write_u64() API
  cgroups: New resource counter inheritance API
  cgroups: Add previous cgroup in can_attach_task/attach_task callbacks
  cgroups: New cancel_attach_task subsystem callback
  cgroups: Ability to stop res charge propagation on bounded ancestor
  cgroups: Add res counter common ancestor searching
  cgroups: Add a max number of tasks subsystem

 block/blk-cgroup.c            |   10 ++-
 include/linux/cgroup.h        |   15 +++-
 include/linux/cgroup_subsys.h |    8 ++
 include/linux/res_counter.h   |   12 +++
 init/Kconfig                  |    7 ++
 kernel/Makefile               |    1 +
 kernel/cgroup.c               |   25 ++++--
 kernel/cgroup_freezer.c       |    3 +-
 kernel/cgroup_max_tasks.c     |  176 +++++++++++++++++++++++++++++++++++++++++
 kernel/cpuset.c               |    6 +-
 kernel/events/core.c          |    5 +-
 kernel/fork.c                 |    4 +
 kernel/res_counter.c          |   88 ++++++++++++++++++---
 kernel/sched.c                |    6 +-
 14 files changed, 335 insertions(+), 31 deletions(-)
 create mode 100644 kernel/cgroup_max_tasks.c

-- 
1.7.5.4

[PATCH 1/7] cgroups: Add res_counter_write_u64() API

[Frederic Weisbecker]

Extend the resource counter API with a mirror of
res_counter_read_u64() to make it handy to update a resource
counter value from a cgroup subsystem u64 value file.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Paul Menage <menage@google.com>
Cc: Li Zefan <lizf@cn.fujitsu.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Aditya Kali <adityakali@google.com>
---
 include/linux/res_counter.h |    2 ++
 kernel/res_counter.c        |   21 +++++++++++++++------
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/include/linux/res_counter.h b/include/linux/res_counter.h
index c9d625c..1b3fe05 100644
--- a/include/linux/res_counter.h
+++ b/include/linux/res_counter.h
@@ -82,6 +82,8 @@ int res_counter_memparse_write_strategy(const char *buf,
 int res_counter_write(struct res_counter *counter, int member,
 		      const char *buffer, write_strategy_fn write_strategy);
 
+void res_counter_write_u64(struct res_counter *counter, int member, u64 val);
+
 /*
  * the field descriptors. one for each member of res_counter
  */
diff --git a/kernel/res_counter.c b/kernel/res_counter.c
index 34683ef..806d041 100644
--- a/kernel/res_counter.c
+++ b/kernel/res_counter.c
@@ -168,12 +168,22 @@ int res_counter_memparse_write_strategy(const char *buf,
 	return 0;
 }
 
+void res_counter_write_u64(struct res_counter *counter, int member, u64 val)
+{
+	unsigned long long *target;
+	unsigned long flags;
+
+	spin_lock_irqsave(&counter->lock, flags);
+	target = res_counter_member(counter, member);
+	*target = val;
+	spin_unlock_irqrestore(&counter->lock, flags);
+}
+
 int res_counter_write(struct res_counter *counter, int member,
 		      const char *buf, write_strategy_fn write_strategy)
 {
 	char *end;
-	unsigned long flags;
-	unsigned long long tmp, *val;
+	unsigned long long tmp;
 
 	if (write_strategy) {
 		if (write_strategy(buf, &tmp))
@@ -183,9 +193,8 @@ int res_counter_write(struct res_counter *counter, int member,
 		if (*end != '\0')
 			return -EINVAL;
 	}
-	spin_lock_irqsave(&counter->lock, flags);
-	val = res_counter_member(counter, member);
-	*val = tmp;
-	spin_unlock_irqrestore(&counter->lock, flags);
+
+	res_counter_write_u64(counter, member, tmp);
+
 	return 0;
 }
-- 
1.7.5.4

Re: [PATCH 1/7] cgroups: Add res_counter_write_u64() API

[Paul Menage]

On Mon, Jul 11, 2011 at 7:15 AM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> Extend the resource counter API with a mirror of
> res_counter_read_u64() to make it handy to update a resource
> counter value from a cgroup subsystem u64 value file.
>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>

Acked-by: Paul Menage <menage@google.com>

Thanks,
Paul

> Cc: Paul Menage <menage@google.com>
> Cc: Li Zefan <lizf@cn.fujitsu.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Aditya Kali <adityakali@google.com>
> ---
>  include/linux/res_counter.h |    2 ++
>  kernel/res_counter.c        |   21 +++++++++++++++------
>  2 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/res_counter.h b/include/linux/res_counter.h
> index c9d625c..1b3fe05 100644
> --- a/include/linux/res_counter.h
> +++ b/include/linux/res_counter.h
> @@ -82,6 +82,8 @@ int res_counter_memparse_write_strategy(const char *buf,
>  int res_counter_write(struct res_counter *counter, int member,
>                      const char *buffer, write_strategy_fn write_strategy);
>
> +void res_counter_write_u64(struct res_counter *counter, int member, u64 val);
> +
>  /*
>  * the field descriptors. one for each member of res_counter
>  */
> diff --git a/kernel/res_counter.c b/kernel/res_counter.c
> index 34683ef..806d041 100644
> --- a/kernel/res_counter.c
> +++ b/kernel/res_counter.c
> @@ -168,12 +168,22 @@ int res_counter_memparse_write_strategy(const char *buf,
>        return 0;
>  }
>
> +void res_counter_write_u64(struct res_counter *counter, int member, u64 val)
> +{
> +       unsigned long long *target;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&counter->lock, flags);
> +       target = res_counter_member(counter, member);
> +       *target = val;
> +       spin_unlock_irqrestore(&counter->lock, flags);
> +}
> +
>  int res_counter_write(struct res_counter *counter, int member,
>                      const char *buf, write_strategy_fn write_strategy)
>  {
>        char *end;
> -       unsigned long flags;
> -       unsigned long long tmp, *val;
> +       unsigned long long tmp;
>
>        if (write_strategy) {
>                if (write_strategy(buf, &tmp))
> @@ -183,9 +193,8 @@ int res_counter_write(struct res_counter *counter, int member,
>                if (*end != '\0')
>                        return -EINVAL;
>        }
> -       spin_lock_irqsave(&counter->lock, flags);
> -       val = res_counter_member(counter, member);
> -       *val = tmp;
> -       spin_unlock_irqrestore(&counter->lock, flags);
> +
> +       res_counter_write_u64(counter, member, tmp);
> +
>        return 0;
>  }
> --
> 1.7.5.4
>
>

[PATCH 2/7] cgroups: New resource counter inheritance API

[Frederic Weisbecker]

Provide an API to inherit a counter value from a parent.
This can be useful to implement cgroup.clone_children on
a resource counter.

Still the resources of the children are limited by those
of the parent, so this is only to provide a default setting
behaviour when clone_children is set.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Paul Menage <menage@google.com>
Cc: Li Zefan <lizf@cn.fujitsu.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Aditya Kali <adityakali@google.com>
---
 include/linux/res_counter.h |    2 ++
 kernel/res_counter.c        |   25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/include/linux/res_counter.h b/include/linux/res_counter.h
index 1b3fe05..109d118 100644
--- a/include/linux/res_counter.h
+++ b/include/linux/res_counter.h
@@ -84,6 +84,8 @@ int res_counter_write(struct res_counter *counter, int member,
 
 void res_counter_write_u64(struct res_counter *counter, int member, u64 val);
 
+void res_counter_inherit(struct res_counter *counter, int member);
+
 /*
  * the field descriptors. one for each member of res_counter
  */
diff --git a/kernel/res_counter.c b/kernel/res_counter.c
index 806d041..8cb0362 100644
--- a/kernel/res_counter.c
+++ b/kernel/res_counter.c
@@ -198,3 +198,28 @@ int res_counter_write(struct res_counter *counter, int member,
 
 	return 0;
 }
+
+void res_counter_inherit(struct res_counter *counter, int member)
+{
+	struct res_counter *parent;
+	unsigned long long *pval, val;
+	unsigned long flags;
+
+	parent = counter->parent;
+	if (!parent)
+		return;
+
+	local_irq_save(flags);
+
+	spin_lock(&counter->lock);
+	pval = res_counter_member(counter, member);
+	val = *pval;
+	spin_unlock(&counter->lock);
+
+	spin_lock(&parent->lock);
+	pval = res_counter_member(parent, member);
+	*pval = val;
+	spin_unlock(&parent->lock);
+
+	local_irq_restore(flags);
+}
-- 
1.7.5.4

Re: [PATCH 2/7] cgroups: New resource counter inheritance API

[Paul Menage]

On Mon, Jul 11, 2011 at 7:15 AM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> Provide an API to inherit a counter value from a parent.
> This can be useful to implement cgroup.clone_children on
> a resource counter.
>
> Still the resources of the children are limited by those
> of the parent, so this is only to provide a default setting
> behaviour when clone_children is set.
>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>

Can't this be just:

void res_counter_inherit(struct res_counter *counter, int member) {
  struct res_counter *parent;
  parent = counter->parent;
  if (parent)
    res_counter_write_u64(counter, member,
res_counter_read_u64(parent, member));
}

This is just used at cgroup creation time, right? So the performance
impact of an extra cli/sti shouldn't matter.

Also, looking at the code res_counter_read_u64() appears to not do any
locking. I don't recall why I added it like that, but it probably
ought to do at least an atomic64_read().

Paul

Re: [PATCH 2/7] cgroups: New resource counter inheritance API

[Frederic Weisbecker]

On Mon, Jul 11, 2011 at 01:41:31PM -0700, Paul Menage wrote:
> On Mon, Jul 11, 2011 at 7:15 AM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > Provide an API to inherit a counter value from a parent.
> > This can be useful to implement cgroup.clone_children on
> > a resource counter.
> >
> > Still the resources of the children are limited by those
> > of the parent, so this is only to provide a default setting
> > behaviour when clone_children is set.
> >
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> 
> Can't this be just:
> 
> void res_counter_inherit(struct res_counter *counter, int member) {
>   struct res_counter *parent;
>   parent = counter->parent;
>   if (parent)
>     res_counter_write_u64(counter, member,
> res_counter_read_u64(parent, member));
> }
> 
> This is just used at cgroup creation time, right? So the performance
> impact of an extra cli/sti shouldn't matter.

Yeah indeed.
 
> Also, looking at the code res_counter_read_u64() appears to not do any
> locking. I don't recall why I added it like that, but it probably
> ought to do at least an atomic64_read().

It does in 32 bits, using the res counter spinlock.

[PATCH 3/7] cgroups: Add previous cgroup in can_attach_task/attach_task callbacks

[Frederic Weisbecker]

This is to prepare the integration of a new max number of proc
cgroup subsystem. We'll need to release some resources from the
previous cgroup.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Paul Menage <menage@google.com>
Cc: Li Zefan <lizf@cn.fujitsu.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Aditya Kali <adityakali@google.com>
---
 block/blk-cgroup.c      |   10 ++++++----
 include/linux/cgroup.h  |    5 +++--
 kernel/cgroup.c         |   10 ++++++----
 kernel/cgroup_freezer.c |    3 ++-
 kernel/cpuset.c         |    6 ++++--
 kernel/events/core.c    |    5 +++--
 kernel/sched.c          |    6 ++++--
 7 files changed, 28 insertions(+), 17 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index bcaf16e..d1bfe88 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -30,8 +30,8 @@ EXPORT_SYMBOL_GPL(blkio_root_cgroup);
 
 static struct cgroup_subsys_state *blkiocg_create(struct cgroup_subsys *,
 						  struct cgroup *);
-static int blkiocg_can_attach_task(struct cgroup *, struct task_struct *);
-static void blkiocg_attach_task(struct cgroup *, struct task_struct *);
+static int blkiocg_can_attach_task(struct cgroup *, struct cgroup *, struct task_struct *);
+static void blkiocg_attach_task(struct cgroup *, struct cgroup *, struct task_struct *);
 static void blkiocg_destroy(struct cgroup_subsys *, struct cgroup *);
 static int blkiocg_populate(struct cgroup_subsys *, struct cgroup *);
 
@@ -1614,7 +1614,8 @@ done:
  * of the main cic data structures.  For now we allow a task to change
  * its cgroup only if it's the only owner of its ioc.
  */
-static int blkiocg_can_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
+static int blkiocg_can_attach_task(struct cgroup *cgrp, struct cgroup *old_cgrp,
+				   struct task_struct *tsk)
 {
 	struct io_context *ioc;
 	int ret = 0;
@@ -1629,7 +1630,8 @@ static int blkiocg_can_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
 	return ret;
 }
 
-static void blkiocg_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
+static void blkiocg_attach_task(struct cgroup *cgrp, struct cgroup *old_cgrp,
+				struct task_struct *tsk)
 {
 	struct io_context *ioc;
 
diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index ab4ac0c..e8288a0 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -468,11 +468,12 @@ struct cgroup_subsys {
 	void (*destroy)(struct cgroup_subsys *ss, struct cgroup *cgrp);
 	int (*can_attach)(struct cgroup_subsys *ss, struct cgroup *cgrp,
 			  struct task_struct *tsk);
-	int (*can_attach_task)(struct cgroup *cgrp, struct task_struct *tsk);
+	int (*can_attach_task)(struct cgroup *cgrp, struct cgroup *old_cgrp,
+			       struct task_struct *tsk);
 	void (*cancel_attach)(struct cgroup_subsys *ss, struct cgroup *cgrp,
 			      struct task_struct *tsk);
 	void (*pre_attach)(struct cgroup *cgrp);
-	void (*attach_task)(struct cgroup *cgrp, struct task_struct *tsk);
+	void (*attach_task)(struct cgroup *cgrp, struct cgroup *old_cgrp, struct task_struct *tsk);
 	void (*attach)(struct cgroup_subsys *ss, struct cgroup *cgrp,
 		       struct cgroup *old_cgrp, struct task_struct *tsk);
 	void (*fork)(struct cgroup_subsys *ss, struct task_struct *task);
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 2731d11..c3ee4cf 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1841,7 +1841,7 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
 			}
 		}
 		if (ss->can_attach_task) {
-			retval = ss->can_attach_task(cgrp, tsk);
+			retval = ss->can_attach_task(cgrp, oldcgrp, tsk);
 			if (retval) {
 				failed_ss = ss;
 				goto out;
@@ -1857,7 +1857,7 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
 		if (ss->pre_attach)
 			ss->pre_attach(cgrp);
 		if (ss->attach_task)
-			ss->attach_task(cgrp, tsk);
+			ss->attach_task(cgrp, oldcgrp, tsk);
 		if (ss->attach)
 			ss->attach(ss, cgrp, oldcgrp, tsk);
 	}
@@ -2072,7 +2072,9 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 			/* run on each task in the threadgroup. */
 			for (i = 0; i < group_size; i++) {
 				tsk = flex_array_get_ptr(group, i);
-				retval = ss->can_attach_task(cgrp, tsk);
+				oldcgrp = task_cgroup_from_root(tsk, root);
+
+				retval = ss->can_attach_task(cgrp, oldcgrp, tsk);
 				if (retval) {
 					failed_ss = ss;
 					cancel_failed_ss = true;
@@ -2135,7 +2137,7 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 		/* attach each task to each subsystem */
 		for_each_subsys(root, ss) {
 			if (ss->attach_task)
-				ss->attach_task(cgrp, tsk);
+				ss->attach_task(cgrp, oldcgrp, tsk);
 		}
 		/* if the thread is PF_EXITING, it can just get skipped. */
 		retval = cgroup_task_migrate(cgrp, oldcgrp, tsk, true);
diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
index e691818..c1421a1 100644
--- a/kernel/cgroup_freezer.c
+++ b/kernel/cgroup_freezer.c
@@ -175,7 +175,8 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
 	return 0;
 }
 
-static int freezer_can_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
+static int freezer_can_attach_task(struct cgroup *cgrp, struct cgroup *old_cgrp,
+				   struct task_struct *tsk)
 {
 	rcu_read_lock();
 	if (__cgroup_freezing_or_frozen(tsk)) {
diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index 9c9b754..f66c9b4 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -1390,7 +1390,8 @@ static int cpuset_can_attach(struct cgroup_subsys *ss, struct cgroup *cont,
 	return 0;
 }
 
-static int cpuset_can_attach_task(struct cgroup *cgrp, struct task_struct *task)
+static int cpuset_can_attach_task(struct cgroup *cgrp, struct cgroup *old_cgrp,
+				  struct task_struct *task)
 {
 	return security_task_setscheduler(task);
 }
@@ -1418,7 +1419,8 @@ static void cpuset_pre_attach(struct cgroup *cont)
 }
 
 /* Per-thread attachment work. */
-static void cpuset_attach_task(struct cgroup *cont, struct task_struct *tsk)
+static void cpuset_attach_task(struct cgroup *cont, struct cgroup *old,
+			       struct task_struct *tsk)
 {
 	int err;
 	struct cpuset *cs = cgroup_cs(cont);
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 9efe710..3daf0eb 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -7403,7 +7403,8 @@ static int __perf_cgroup_move(void *info)
 }
 
 static void
-perf_cgroup_attach_task(struct cgroup *cgrp, struct task_struct *task)
+perf_cgroup_attach_task(struct cgroup *cgrp, struct cgroup *old_cgrp,
+			struct task_struct *task)
 {
 	task_function_call(task, __perf_cgroup_move, task);
 }
@@ -7419,7 +7420,7 @@ static void perf_cgroup_exit(struct cgroup_subsys *ss, struct cgroup *cgrp,
 	if (!(task->flags & PF_EXITING))
 		return;
 
-	perf_cgroup_attach_task(cgrp, task);
+	perf_cgroup_attach_task(cgrp, old_cgrp, task);
 }
 
 struct cgroup_subsys perf_subsys = {
diff --git a/kernel/sched.c b/kernel/sched.c
index 3f2e502..adec58d 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -8790,7 +8790,8 @@ cpu_cgroup_destroy(struct cgroup_subsys *ss, struct cgroup *cgrp)
 }
 
 static int
-cpu_cgroup_can_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
+cpu_cgroup_can_attach_task(struct cgroup *cgrp, struct cgroup *old_cgrp,
+			   struct task_struct *tsk)
 {
 #ifdef CONFIG_RT_GROUP_SCHED
 	if (!sched_rt_can_attach(cgroup_tg(cgrp), tsk))
@@ -8804,7 +8805,8 @@ cpu_cgroup_can_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
 }
 
 static void
-cpu_cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
+cpu_cgroup_attach_task(struct cgroup *cgrp, struct cgroup *old_cgrp,
+		       struct task_struct *tsk)
 {
 	sched_move_task(tsk);
 }
-- 
1.7.5.4

Re: [PATCH 3/7] cgroups: Add previous cgroup in can_attach_task/attach_task callbacks

[Paul Menage]

On Mon, Jul 11, 2011 at 7:15 AM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> This is to prepare the integration of a new max number of proc
> cgroup subsystem. We'll need to release some resources from the
> previous cgroup.
>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>

Acked-by: Paul Menage <menage@google.com>


> Cc: Paul Menage <menage@google.com>
> Cc: Li Zefan <lizf@cn.fujitsu.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Aditya Kali <adityakali@google.com>
> ---
>  block/blk-cgroup.c      |   10 ++++++----
>  include/linux/cgroup.h  |    5 +++--
>  kernel/cgroup.c         |   10 ++++++----
>  kernel/cgroup_freezer.c |    3 ++-
>  kernel/cpuset.c         |    6 ++++--
>  kernel/events/core.c    |    5 +++--
>  kernel/sched.c          |    6 ++++--
>  7 files changed, 28 insertions(+), 17 deletions(-)
>
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index bcaf16e..d1bfe88 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -30,8 +30,8 @@ EXPORT_SYMBOL_GPL(blkio_root_cgroup);
>
>  static struct cgroup_subsys_state *blkiocg_create(struct cgroup_subsys *,
>                                                  struct cgroup *);
> -static int blkiocg_can_attach_task(struct cgroup *, struct task_struct *);
> -static void blkiocg_attach_task(struct cgroup *, struct task_struct *);
> +static int blkiocg_can_attach_task(struct cgroup *, struct cgroup *, struct task_struct *);
> +static void blkiocg_attach_task(struct cgroup *, struct cgroup *, struct task_struct *);
>  static void blkiocg_destroy(struct cgroup_subsys *, struct cgroup *);
>  static int blkiocg_populate(struct cgroup_subsys *, struct cgroup *);
>
> @@ -1614,7 +1614,8 @@ done:
>  * of the main cic data structures.  For now we allow a task to change
>  * its cgroup only if it's the only owner of its ioc.
>  */
> -static int blkiocg_can_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
> +static int blkiocg_can_attach_task(struct cgroup *cgrp, struct cgroup *old_cgrp,
> +                                  struct task_struct *tsk)
>  {
>        struct io_context *ioc;
>        int ret = 0;
> @@ -1629,7 +1630,8 @@ static int blkiocg_can_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
>        return ret;
>  }
>
> -static void blkiocg_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
> +static void blkiocg_attach_task(struct cgroup *cgrp, struct cgroup *old_cgrp,
> +                               struct task_struct *tsk)
>  {
>        struct io_context *ioc;
>
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index ab4ac0c..e8288a0 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -468,11 +468,12 @@ struct cgroup_subsys {
>        void (*destroy)(struct cgroup_subsys *ss, struct cgroup *cgrp);
>        int (*can_attach)(struct cgroup_subsys *ss, struct cgroup *cgrp,
>                          struct task_struct *tsk);
> -       int (*can_attach_task)(struct cgroup *cgrp, struct task_struct *tsk);
> +       int (*can_attach_task)(struct cgroup *cgrp, struct cgroup *old_cgrp,
> +                              struct task_struct *tsk);
>        void (*cancel_attach)(struct cgroup_subsys *ss, struct cgroup *cgrp,
>                              struct task_struct *tsk);
>        void (*pre_attach)(struct cgroup *cgrp);
> -       void (*attach_task)(struct cgroup *cgrp, struct task_struct *tsk);
> +       void (*attach_task)(struct cgroup *cgrp, struct cgroup *old_cgrp, struct task_struct *tsk);
>        void (*attach)(struct cgroup_subsys *ss, struct cgroup *cgrp,
>                       struct cgroup *old_cgrp, struct task_struct *tsk);
>        void (*fork)(struct cgroup_subsys *ss, struct task_struct *task);
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 2731d11..c3ee4cf 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -1841,7 +1841,7 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
>                        }
>                }
>                if (ss->can_attach_task) {
> -                       retval = ss->can_attach_task(cgrp, tsk);
> +                       retval = ss->can_attach_task(cgrp, oldcgrp, tsk);
>                        if (retval) {
>                                failed_ss = ss;
>                                goto out;
> @@ -1857,7 +1857,7 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
>                if (ss->pre_attach)
>                        ss->pre_attach(cgrp);
>                if (ss->attach_task)
> -                       ss->attach_task(cgrp, tsk);
> +                       ss->attach_task(cgrp, oldcgrp, tsk);
>                if (ss->attach)
>                        ss->attach(ss, cgrp, oldcgrp, tsk);
>        }
> @@ -2072,7 +2072,9 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
>                        /* run on each task in the threadgroup. */
>                        for (i = 0; i < group_size; i++) {
>                                tsk = flex_array_get_ptr(group, i);
> -                               retval = ss->can_attach_task(cgrp, tsk);
> +                               oldcgrp = task_cgroup_from_root(tsk, root);
> +
> +                               retval = ss->can_attach_task(cgrp, oldcgrp, tsk);
>                                if (retval) {
>                                        failed_ss = ss;
>                                        cancel_failed_ss = true;
> @@ -2135,7 +2137,7 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
>                /* attach each task to each subsystem */
>                for_each_subsys(root, ss) {
>                        if (ss->attach_task)
> -                               ss->attach_task(cgrp, tsk);
> +                               ss->attach_task(cgrp, oldcgrp, tsk);
>                }
>                /* if the thread is PF_EXITING, it can just get skipped. */
>                retval = cgroup_task_migrate(cgrp, oldcgrp, tsk, true);
> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
> index e691818..c1421a1 100644
> --- a/kernel/cgroup_freezer.c
> +++ b/kernel/cgroup_freezer.c
> @@ -175,7 +175,8 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
>        return 0;
>  }
>
> -static int freezer_can_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
> +static int freezer_can_attach_task(struct cgroup *cgrp, struct cgroup *old_cgrp,
> +                                  struct task_struct *tsk)
>  {
>        rcu_read_lock();
>        if (__cgroup_freezing_or_frozen(tsk)) {
> diff --git a/kernel/cpuset.c b/kernel/cpuset.c
> index 9c9b754..f66c9b4 100644
> --- a/kernel/cpuset.c
> +++ b/kernel/cpuset.c
> @@ -1390,7 +1390,8 @@ static int cpuset_can_attach(struct cgroup_subsys *ss, struct cgroup *cont,
>        return 0;
>  }
>
> -static int cpuset_can_attach_task(struct cgroup *cgrp, struct task_struct *task)
> +static int cpuset_can_attach_task(struct cgroup *cgrp, struct cgroup *old_cgrp,
> +                                 struct task_struct *task)
>  {
>        return security_task_setscheduler(task);
>  }
> @@ -1418,7 +1419,8 @@ static void cpuset_pre_attach(struct cgroup *cont)
>  }
>
>  /* Per-thread attachment work. */
> -static void cpuset_attach_task(struct cgroup *cont, struct task_struct *tsk)
> +static void cpuset_attach_task(struct cgroup *cont, struct cgroup *old,
> +                              struct task_struct *tsk)
>  {
>        int err;
>        struct cpuset *cs = cgroup_cs(cont);
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 9efe710..3daf0eb 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -7403,7 +7403,8 @@ static int __perf_cgroup_move(void *info)
>  }
>
>  static void
> -perf_cgroup_attach_task(struct cgroup *cgrp, struct task_struct *task)
> +perf_cgroup_attach_task(struct cgroup *cgrp, struct cgroup *old_cgrp,
> +                       struct task_struct *task)
>  {
>        task_function_call(task, __perf_cgroup_move, task);
>  }
> @@ -7419,7 +7420,7 @@ static void perf_cgroup_exit(struct cgroup_subsys *ss, struct cgroup *cgrp,
>        if (!(task->flags & PF_EXITING))
>                return;
>
> -       perf_cgroup_attach_task(cgrp, task);
> +       perf_cgroup_attach_task(cgrp, old_cgrp, task);
>  }
>
>  struct cgroup_subsys perf_subsys = {
> diff --git a/kernel/sched.c b/kernel/sched.c
> index 3f2e502..adec58d 100644
> --- a/kernel/sched.c
> +++ b/kernel/sched.c
> @@ -8790,7 +8790,8 @@ cpu_cgroup_destroy(struct cgroup_subsys *ss, struct cgroup *cgrp)
>  }
>
>  static int
> -cpu_cgroup_can_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
> +cpu_cgroup_can_attach_task(struct cgroup *cgrp, struct cgroup *old_cgrp,
> +                          struct task_struct *tsk)
>  {
>  #ifdef CONFIG_RT_GROUP_SCHED
>        if (!sched_rt_can_attach(cgroup_tg(cgrp), tsk))
> @@ -8804,7 +8805,8 @@ cpu_cgroup_can_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
>  }
>
>  static void
> -cpu_cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
> +cpu_cgroup_attach_task(struct cgroup *cgrp, struct cgroup *old_cgrp,
> +                      struct task_struct *tsk)
>  {
>        sched_move_task(tsk);
>  }
> --
> 1.7.5.4
>
>

[PATCH 4/7] cgroups: New cancel_attach_task subsystem callback

[Frederic Weisbecker]

To cancel a process attachment on a subsystem, we only call the
cancel_attach() callback once on the leader but we have no
way to cancel the attachment individually for each member of
the process group.

This is going to be needed for the max number of tasks susbystem
that is coming.

To prepare for this integration, call a new cancel_attach_task()
callback on each task of the group until we reach the member that
failed to attach.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Paul Menage <menage@google.com>
Cc: Li Zefan <lizf@cn.fujitsu.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Aditya Kali <adityakali@google.com>
---
 include/linux/cgroup.h |    1 +
 kernel/cgroup.c        |   15 ++++++++++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index e8288a0..94454143 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -472,6 +472,7 @@ struct cgroup_subsys {
 			       struct task_struct *tsk);
 	void (*cancel_attach)(struct cgroup_subsys *ss, struct cgroup *cgrp,
 			      struct task_struct *tsk);
+	void (*cancel_attach_task)(struct cgroup *cgrp, struct task_struct *tsk);
 	void (*pre_attach)(struct cgroup *cgrp);
 	void (*attach_task)(struct cgroup *cgrp, struct cgroup *old_cgrp, struct task_struct *tsk);
 	void (*attach)(struct cgroup_subsys *ss, struct cgroup *cgrp,
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index c3ee4cf..210dc05 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1989,7 +1989,7 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 {
 	int retval, i, group_size;
 	struct cgroup_subsys *ss, *failed_ss = NULL;
-	bool cancel_failed_ss = false;
+	struct task_struct *failed_task = NULL;
 	/* guaranteed to be initialized later, but the compiler needs this */
 	struct cgroup *oldcgrp = NULL;
 	struct css_set *oldcg;
@@ -2077,7 +2077,7 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 				retval = ss->can_attach_task(cgrp, oldcgrp, tsk);
 				if (retval) {
 					failed_ss = ss;
-					cancel_failed_ss = true;
+					failed_task = tsk;
 					goto out_cancel_attach;
 				}
 			}
@@ -2172,8 +2172,17 @@ out_cancel_attach:
 	/* same deal as in cgroup_attach_task */
 	if (retval) {
 		for_each_subsys(root, ss) {
+			if (ss->cancel_attach_task && (ss != failed_ss || failed_task)) {
+				for (i = 0; i < group_size; i++) {
+					tsk = flex_array_get_ptr(group, i);
+					if (tsk == failed_task)
+						break;
+					ss->cancel_attach_task(cgrp, tsk);
+				}
+			}
+
 			if (ss == failed_ss) {
-				if (cancel_failed_ss && ss->cancel_attach)
+				if (failed_task && ss->cancel_attach)
 					ss->cancel_attach(ss, cgrp, leader);
 				break;
 			}
-- 
1.7.5.4

Re: [PATCH 4/7] cgroups: New cancel_attach_task subsystem callback

[Paul Menage]

On Mon, Jul 11, 2011 at 7:15 AM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> To cancel a process attachment on a subsystem, we only call the
> cancel_attach() callback once on the leader but we have no
> way to cancel the attachment individually for each member of
> the process group.
>
> This is going to be needed for the max number of tasks susbystem
> that is coming.
>
> To prepare for this integration, call a new cancel_attach_task()
> callback on each task of the group until we reach the member that
> failed to attach.
>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>

Acked-by: Paul Menage <menage@google.com>


> Cc: Paul Menage <menage@google.com>
> Cc: Li Zefan <lizf@cn.fujitsu.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Aditya Kali <adityakali@google.com>
> ---
>  include/linux/cgroup.h |    1 +
>  kernel/cgroup.c        |   15 ++++++++++++---
>  2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index e8288a0..94454143 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -472,6 +472,7 @@ struct cgroup_subsys {
>                               struct task_struct *tsk);
>        void (*cancel_attach)(struct cgroup_subsys *ss, struct cgroup *cgrp,
>                              struct task_struct *tsk);
> +       void (*cancel_attach_task)(struct cgroup *cgrp, struct task_struct *tsk);
>        void (*pre_attach)(struct cgroup *cgrp);
>        void (*attach_task)(struct cgroup *cgrp, struct cgroup *old_cgrp, struct task_struct *tsk);
>        void (*attach)(struct cgroup_subsys *ss, struct cgroup *cgrp,
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index c3ee4cf..210dc05 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -1989,7 +1989,7 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
>  {
>        int retval, i, group_size;
>        struct cgroup_subsys *ss, *failed_ss = NULL;
> -       bool cancel_failed_ss = false;
> +       struct task_struct *failed_task = NULL;
>        /* guaranteed to be initialized later, but the compiler needs this */
>        struct cgroup *oldcgrp = NULL;
>        struct css_set *oldcg;
> @@ -2077,7 +2077,7 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
>                                retval = ss->can_attach_task(cgrp, oldcgrp, tsk);
>                                if (retval) {
>                                        failed_ss = ss;
> -                                       cancel_failed_ss = true;
> +                                       failed_task = tsk;
>                                        goto out_cancel_attach;
>                                }
>                        }
> @@ -2172,8 +2172,17 @@ out_cancel_attach:
>        /* same deal as in cgroup_attach_task */
>        if (retval) {
>                for_each_subsys(root, ss) {
> +                       if (ss->cancel_attach_task && (ss != failed_ss || failed_task)) {
> +                               for (i = 0; i < group_size; i++) {
> +                                       tsk = flex_array_get_ptr(group, i);
> +                                       if (tsk == failed_task)
> +                                               break;
> +                                       ss->cancel_attach_task(cgrp, tsk);
> +                               }
> +                       }
> +
>                        if (ss == failed_ss) {
> -                               if (cancel_failed_ss && ss->cancel_attach)
> +                               if (failed_task && ss->cancel_attach)
>                                        ss->cancel_attach(ss, cgrp, leader);
>                                break;
>                        }
> --
> 1.7.5.4
>
>

[PATCH 5/7] cgroups: Ability to stop res charge propagation on bounded ancestor

[Frederic Weisbecker]

Moving a task from a cgroup to another may require to substract
its resource charge from the old cgroup and add it to the new one.

For this to happen, the uncharge/charge propagation can just stop
when we reach the common ancestor for the two cgroups. Further
the performance reasons, we also want to avoid to temporarily
overload the common ancestors with a non-accurate resource
counter usage if we charge first the new cgroup and uncharge the
old one thereafter. This is going to be a requirement for the coming
max number of task subsystem.

To solve this, provide a pair of new API that can charge/uncharge
a resource counter until we reach a given ancestor.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Paul Menage <menage@google.com>
Cc: Li Zefan <lizf@cn.fujitsu.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Aditya Kali <adityakali@google.com>
---
 include/linux/res_counter.h |    6 ++++++
 kernel/res_counter.c        |   23 ++++++++++++++++++-----
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/include/linux/res_counter.h b/include/linux/res_counter.h
index 109d118..8c421ac 100644
--- a/include/linux/res_counter.h
+++ b/include/linux/res_counter.h
@@ -117,6 +117,10 @@ void res_counter_init(struct res_counter *counter, struct res_counter *parent);
 
 int __must_check res_counter_charge_locked(struct res_counter *counter,
 		unsigned long val);
+int __must_check res_counter_charge_until(struct res_counter *counter,
+					  struct res_counter *limit,
+					  unsigned long val,
+					  struct res_counter **limit_fail_at);
 int __must_check res_counter_charge(struct res_counter *counter,
 		unsigned long val, struct res_counter **limit_fail_at);
 
@@ -131,6 +135,8 @@ int __must_check res_counter_charge(struct res_counter *counter,
  */
 
 void res_counter_uncharge_locked(struct res_counter *counter, unsigned long val);
+void res_counter_uncharge_until(struct res_counter *counter, struct res_counter *limit,
+				unsigned long val);
 void res_counter_uncharge(struct res_counter *counter, unsigned long val);
 
 /**
diff --git a/kernel/res_counter.c b/kernel/res_counter.c
index 8cb0362..39f2513 100644
--- a/kernel/res_counter.c
+++ b/kernel/res_counter.c
@@ -35,8 +35,9 @@ int res_counter_charge_locked(struct res_counter *counter, unsigned long val)
 	return 0;
 }
 
-int res_counter_charge(struct res_counter *counter, unsigned long val,
-			struct res_counter **limit_fail_at)
+int res_counter_charge_until(struct res_counter *counter,
+			     struct res_counter *limit, unsigned long val,
+			     struct res_counter **limit_fail_at)
 {
 	int ret;
 	unsigned long flags;
@@ -44,7 +45,7 @@ int res_counter_charge(struct res_counter *counter, unsigned long val,
 
 	*limit_fail_at = NULL;
 	local_irq_save(flags);
-	for (c = counter; c != NULL; c = c->parent) {
+	for (c = counter; c != limit; c = c->parent) {
 		spin_lock(&c->lock);
 		ret = res_counter_charge_locked(c, val);
 		spin_unlock(&c->lock);
@@ -66,6 +67,12 @@ done:
 	return ret;
 }
 
+int res_counter_charge(struct res_counter *counter, unsigned long val,
+			struct res_counter **limit_fail_at)
+{
+	return res_counter_charge_until(counter, NULL, val, limit_fail_at);
+}
+
 void res_counter_uncharge_locked(struct res_counter *counter, unsigned long val)
 {
 	if (WARN_ON(counter->usage < val))
@@ -74,13 +81,15 @@ void res_counter_uncharge_locked(struct res_counter *counter, unsigned long val)
 	counter->usage -= val;
 }
 
-void res_counter_uncharge(struct res_counter *counter, unsigned long val)
+void res_counter_uncharge_until(struct res_counter *counter,
+				struct res_counter *limit,
+				unsigned long val)
 {
 	unsigned long flags;
 	struct res_counter *c;
 
 	local_irq_save(flags);
-	for (c = counter; c != NULL; c = c->parent) {
+	for (c = counter; c != limit; c = c->parent) {
 		spin_lock(&c->lock);
 		res_counter_uncharge_locked(c, val);
 		spin_unlock(&c->lock);
@@ -88,6 +97,10 @@ void res_counter_uncharge(struct res_counter *counter, unsigned long val)
 	local_irq_restore(flags);
 }
 
+void res_counter_uncharge(struct res_counter *counter, unsigned long val)
+{
+	res_counter_uncharge_until(counter, NULL, val);
+}
 
 static inline unsigned long long *
 res_counter_member(struct res_counter *counter, int member)
-- 
1.7.5.4

Re: [PATCH 5/7] cgroups: Ability to stop res charge propagation on bounded ancestor

[KAMEZAWA Hiroyuki]

On Mon, 11 Jul 2011 16:15:04 +0200
Frederic Weisbecker <fweisbec@gmail.com> wrote:

> Moving a task from a cgroup to another may require to substract
> its resource charge from the old cgroup and add it to the new one.
> 
> For this to happen, the uncharge/charge propagation can just stop
> when we reach the common ancestor for the two cgroups. Further
> the performance reasons, we also want to avoid to temporarily
> overload the common ancestors with a non-accurate resource
> counter usage if we charge first the new cgroup and uncharge the
> old one thereafter. This is going to be a requirement for the coming
> max number of task subsystem.
> 
> To solve this, provide a pair of new API that can charge/uncharge
> a resource counter until we reach a given ancestor.
> 
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Paul Menage <menage@google.com>
> Cc: Li Zefan <lizf@cn.fujitsu.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Aditya Kali <adityakali@google.com>


Hmm, do you have the number to show the benefit of this new function ?
And....tasks is moving among cgroups so frequently as to show the benefit
of this function in your environment ??

Thanks,
-Kame

Re: [PATCH 5/7] cgroups: Ability to stop res charge propagation on bounded ancestor

[Frederic Weisbecker]

On Tue, Jul 12, 2011 at 09:11:31AM +0900, KAMEZAWA Hiroyuki wrote:
> On Mon, 11 Jul 2011 16:15:04 +0200
> Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > Moving a task from a cgroup to another may require to substract
> > its resource charge from the old cgroup and add it to the new one.
> > 
> > For this to happen, the uncharge/charge propagation can just stop
> > when we reach the common ancestor for the two cgroups. Further
> > the performance reasons, we also want to avoid to temporarily
> > overload the common ancestors with a non-accurate resource
> > counter usage if we charge first the new cgroup and uncharge the
> > old one thereafter. This is going to be a requirement for the coming
> > max number of task subsystem.
> > 
> > To solve this, provide a pair of new API that can charge/uncharge
> > a resource counter until we reach a given ancestor.
> > 
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > Cc: Paul Menage <menage@google.com>
> > Cc: Li Zefan <lizf@cn.fujitsu.com>
> > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > Cc: Aditya Kali <adityakali@google.com>
> 
> 
> Hmm, do you have the number to show the benefit of this new function ?
> And....tasks is moving among cgroups so frequently as to show the benefit
> of this function in your environment ??

So the benefit is not really in the optimization, although that's a side effect.

Let me clarify the point in the changelog.
Imagine we have these cgroups:


                 A  (usage = 2, limit = 2)
                 |
                / \
               /   \
              /     \
             /       \
            /         \
           /           \
          B             C  (usage = 1, limit = 2)
(usage = 1, limit = 2)


The usage in A is the accumulation of the usage in B and C.
Imagine i want to move a task from C to B. This should work well.
We need to first check if we can charge B and do it, and then later
uncharge C.

But if we do:

	err = res_counter_charge(B)
	if (err)
		exit
	res_counter_uncharge(C)

it is going to fail because charging B will also charge A. And A
will refuse because it's already full. Ideally we should first uncharge
C and then charge B, so that A doesn't reject:

	res_counter_uncharge(C)
	err = res_countrer_charge(B)
	if (err)
		res_counter_charge(C)

The problem is that if charging B fails we need to rollback on C, but it might
be too late as a fork might have happen inside C since we uncharged it, so we couldn't
charge it back.

So the only solution is to first charge B but stop the charge propagation on A.
And then uncharge on C but stop uncharge on A.

Re: [PATCH 5/7] cgroups: Ability to stop res charge propagation on bounded ancestor

[Paul Menage]

On Mon, Jul 11, 2011 at 7:15 AM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> Moving a task from a cgroup to another may require to substract
> its resource charge from the old cgroup and add it to the new one.
>
> For this to happen, the uncharge/charge propagation can just stop
> when we reach the common ancestor for the two cgroups. Further
> the performance reasons, we also want to avoid to temporarily
> overload the common ancestors with a non-accurate resource
> counter usage if we charge first the new cgroup and uncharge the
> old one thereafter. This is going to be a requirement for the coming
> max number of task subsystem.
>
> To solve this, provide a pair of new API that can charge/uncharge
> a resource counter until we reach a given ancestor.
>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>

Acked-by: Paul Menage <menage@google.com>

Looks good, thanks.
Paul

> Cc: Paul Menage <menage@google.com>
> Cc: Li Zefan <lizf@cn.fujitsu.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Aditya Kali <adityakali@google.com>
> ---
>  include/linux/res_counter.h |    6 ++++++
>  kernel/res_counter.c        |   23 ++++++++++++++++++-----
>  2 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/res_counter.h b/include/linux/res_counter.h
> index 109d118..8c421ac 100644
> --- a/include/linux/res_counter.h
> +++ b/include/linux/res_counter.h
> @@ -117,6 +117,10 @@ void res_counter_init(struct res_counter *counter, struct res_counter *parent);
>
>  int __must_check res_counter_charge_locked(struct res_counter *counter,
>                unsigned long val);
> +int __must_check res_counter_charge_until(struct res_counter *counter,
> +                                         struct res_counter *limit,
> +                                         unsigned long val,
> +                                         struct res_counter **limit_fail_at);
>  int __must_check res_counter_charge(struct res_counter *counter,
>                unsigned long val, struct res_counter **limit_fail_at);
>
> @@ -131,6 +135,8 @@ int __must_check res_counter_charge(struct res_counter *counter,
>  */
>
>  void res_counter_uncharge_locked(struct res_counter *counter, unsigned long val);
> +void res_counter_uncharge_until(struct res_counter *counter, struct res_counter *limit,
> +                               unsigned long val);
>  void res_counter_uncharge(struct res_counter *counter, unsigned long val);
>
>  /**
> diff --git a/kernel/res_counter.c b/kernel/res_counter.c
> index 8cb0362..39f2513 100644
> --- a/kernel/res_counter.c
> +++ b/kernel/res_counter.c
> @@ -35,8 +35,9 @@ int res_counter_charge_locked(struct res_counter *counter, unsigned long val)
>        return 0;
>  }
>
> -int res_counter_charge(struct res_counter *counter, unsigned long val,
> -                       struct res_counter **limit_fail_at)
> +int res_counter_charge_until(struct res_counter *counter,
> +                            struct res_counter *limit, unsigned long val,
> +                            struct res_counter **limit_fail_at)
>  {
>        int ret;
>        unsigned long flags;
> @@ -44,7 +45,7 @@ int res_counter_charge(struct res_counter *counter, unsigned long val,
>
>        *limit_fail_at = NULL;
>        local_irq_save(flags);
> -       for (c = counter; c != NULL; c = c->parent) {
> +       for (c = counter; c != limit; c = c->parent) {
>                spin_lock(&c->lock);
>                ret = res_counter_charge_locked(c, val);
>                spin_unlock(&c->lock);
> @@ -66,6 +67,12 @@ done:
>        return ret;
>  }
>
> +int res_counter_charge(struct res_counter *counter, unsigned long val,
> +                       struct res_counter **limit_fail_at)
> +{
> +       return res_counter_charge_until(counter, NULL, val, limit_fail_at);
> +}
> +
>  void res_counter_uncharge_locked(struct res_counter *counter, unsigned long val)
>  {
>        if (WARN_ON(counter->usage < val))
> @@ -74,13 +81,15 @@ void res_counter_uncharge_locked(struct res_counter *counter, unsigned long val)
>        counter->usage -= val;
>  }
>
> -void res_counter_uncharge(struct res_counter *counter, unsigned long val)
> +void res_counter_uncharge_until(struct res_counter *counter,
> +                               struct res_counter *limit,
> +                               unsigned long val)
>  {
>        unsigned long flags;
>        struct res_counter *c;
>
>        local_irq_save(flags);
> -       for (c = counter; c != NULL; c = c->parent) {
> +       for (c = counter; c != limit; c = c->parent) {
>                spin_lock(&c->lock);
>                res_counter_uncharge_locked(c, val);
>                spin_unlock(&c->lock);
> @@ -88,6 +97,10 @@ void res_counter_uncharge(struct res_counter *counter, unsigned long val)
>        local_irq_restore(flags);
>  }
>
> +void res_counter_uncharge(struct res_counter *counter, unsigned long val)
> +{
> +       res_counter_uncharge_until(counter, NULL, val);
> +}
>
>  static inline unsigned long long *
>  res_counter_member(struct res_counter *counter, int member)
> --
> 1.7.5.4
>
>

[PATCH 6/7] cgroups: Add res counter common ancestor searching

[Frederic Weisbecker]

Add a new API to find the common ancestor between two resource
counters. This includes the passed resource counter themselves.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Paul Menage <menage@google.com>
Cc: Li Zefan <lizf@cn.fujitsu.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Aditya Kali <adityakali@google.com>
---
 include/linux/res_counter.h |    2 ++
 kernel/res_counter.c        |   19 +++++++++++++++++++
 2 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/include/linux/res_counter.h b/include/linux/res_counter.h
index 8c421ac..354ed30 100644
--- a/include/linux/res_counter.h
+++ b/include/linux/res_counter.h
@@ -139,6 +139,8 @@ void res_counter_uncharge_until(struct res_counter *counter, struct res_counter
 				unsigned long val);
 void res_counter_uncharge(struct res_counter *counter, unsigned long val);
 
+struct res_counter *res_counter_common_ancestor(struct res_counter *l, struct res_counter *r);
+
 /**
  * res_counter_margin - calculate chargeable space of a counter
  * @cnt: the counter
diff --git a/kernel/res_counter.c b/kernel/res_counter.c
index 39f2513..1b2efd6 100644
--- a/kernel/res_counter.c
+++ b/kernel/res_counter.c
@@ -102,6 +102,25 @@ void res_counter_uncharge(struct res_counter *counter, unsigned long val)
 	res_counter_uncharge_until(counter, NULL, val);
 }
 
+struct res_counter *
+res_counter_common_ancestor(struct res_counter *r1, struct res_counter *r2)
+{
+	struct res_counter *iter;
+
+	while (r1) {
+		iter = r2;
+		while (iter) {
+			if (iter == r1)
+				return iter;
+			iter = iter->parent;
+		}
+
+		r1 = r1->parent;
+	}
+
+	return NULL;
+}
+
 static inline unsigned long long *
 res_counter_member(struct res_counter *counter, int member)
 {
-- 
1.7.5.4

Re: [PATCH 6/7] cgroups: Add res counter common ancestor searching

[Paul Menage]

On Mon, Jul 11, 2011 at 7:15 AM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> Add a new API to find the common ancestor between two resource
> counters. This includes the passed resource counter themselves.
>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>

Acked-by: Paul Menage <menage@google.com>

There's something distasteful about doing an O(N^2) search on every
charge/uncharge operation, but I guess in general N isn't going to be
more than 2 or 3 so it's not worth optimizing.

If you wanted to avoid the N^2 search then you could add a "depth"
field in struct res_counter, which would be initialized to
parent->depth+1, and then easily get both r1 and r2 to the same depth
before proceeding up the tree in lock step, but that would bloat all
instances of res_counter, which might work out as more expensive in
the long run.

Paul

> Cc: Paul Menage <menage@google.com>
> Cc: Li Zefan <lizf@cn.fujitsu.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Aditya Kali <adityakali@google.com>
> ---
>  include/linux/res_counter.h |    2 ++
>  kernel/res_counter.c        |   19 +++++++++++++++++++
>  2 files changed, 21 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/res_counter.h b/include/linux/res_counter.h
> index 8c421ac..354ed30 100644
> --- a/include/linux/res_counter.h
> +++ b/include/linux/res_counter.h
> @@ -139,6 +139,8 @@ void res_counter_uncharge_until(struct res_counter *counter, struct res_counter
>                                unsigned long val);
>  void res_counter_uncharge(struct res_counter *counter, unsigned long val);
>
> +struct res_counter *res_counter_common_ancestor(struct res_counter *l, struct res_counter *r);
> +
>  /**
>  * res_counter_margin - calculate chargeable space of a counter
>  * @cnt: the counter
> diff --git a/kernel/res_counter.c b/kernel/res_counter.c
> index 39f2513..1b2efd6 100644
> --- a/kernel/res_counter.c
> +++ b/kernel/res_counter.c
> @@ -102,6 +102,25 @@ void res_counter_uncharge(struct res_counter *counter, unsigned long val)
>        res_counter_uncharge_until(counter, NULL, val);
>  }
>
> +struct res_counter *
> +res_counter_common_ancestor(struct res_counter *r1, struct res_counter *r2)
> +{
> +       struct res_counter *iter;
> +
> +       while (r1) {
> +               iter = r2;
> +               while (iter) {
> +                       if (iter == r1)
> +                               return iter;
> +                       iter = iter->parent;
> +               }
> +
> +               r1 = r1->parent;
> +       }
> +
> +       return NULL;
> +}
> +
>  static inline unsigned long long *
>  res_counter_member(struct res_counter *counter, int member)
>  {
> --
> 1.7.5.4
>
>

Re: [PATCH 6/7] cgroups: Add res counter common ancestor searching

[Frederic Weisbecker]

On Mon, Jul 25, 2011 at 06:05:00PM -0700, Paul Menage wrote:
> On Mon, Jul 11, 2011 at 7:15 AM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > Add a new API to find the common ancestor between two resource
> > counters. This includes the passed resource counter themselves.
> >
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> 
> Acked-by: Paul Menage <menage@google.com>
> 
> There's something distasteful about doing an O(N^2) search on every
> charge/uncharge operation, but I guess in general N isn't going to be
> more than 2 or 3 so it's not worth optimizing.
> 
> If you wanted to avoid the N^2 search then you could add a "depth"
> field in struct res_counter, which would be initialized to
> parent->depth+1, and then easily get both r1 and r2 to the same depth
> before proceeding up the tree in lock step, but that would bloat all
> instances of res_counter, which might work out as more expensive in
> the long run.

Yeah it only happens on slow paths and probably cgroups hierarchies don't
tend to be so deep.

But if that's a problem we can still optimize the way you said, and perhaps
move that depth field into the struct task_counter instead.

Thanks.

> 
> Paul
> 
> > Cc: Paul Menage <menage@google.com>
> > Cc: Li Zefan <lizf@cn.fujitsu.com>
> > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > Cc: Aditya Kali <adityakali@google.com>
> > ---
> >  include/linux/res_counter.h |    2 ++
> >  kernel/res_counter.c        |   19 +++++++++++++++++++
> >  2 files changed, 21 insertions(+), 0 deletions(-)
> >
> > diff --git a/include/linux/res_counter.h b/include/linux/res_counter.h
> > index 8c421ac..354ed30 100644
> > --- a/include/linux/res_counter.h
> > +++ b/include/linux/res_counter.h
> > @@ -139,6 +139,8 @@ void res_counter_uncharge_until(struct res_counter *counter, struct res_counter
> >                                unsigned long val);
> >  void res_counter_uncharge(struct res_counter *counter, unsigned long val);
> >
> > +struct res_counter *res_counter_common_ancestor(struct res_counter *l, struct res_counter *r);
> > +
> >  /**
> >  * res_counter_margin - calculate chargeable space of a counter
> >  * @cnt: the counter
> > diff --git a/kernel/res_counter.c b/kernel/res_counter.c
> > index 39f2513..1b2efd6 100644
> > --- a/kernel/res_counter.c
> > +++ b/kernel/res_counter.c
> > @@ -102,6 +102,25 @@ void res_counter_uncharge(struct res_counter *counter, unsigned long val)
> >        res_counter_uncharge_until(counter, NULL, val);
> >  }
> >
> > +struct res_counter *
> > +res_counter_common_ancestor(struct res_counter *r1, struct res_counter *r2)
> > +{
> > +       struct res_counter *iter;
> > +
> > +       while (r1) {
> > +               iter = r2;
> > +               while (iter) {
> > +                       if (iter == r1)
> > +                               return iter;
> > +                       iter = iter->parent;
> > +               }
> > +
> > +               r1 = r1->parent;
> > +       }
> > +
> > +       return NULL;
> > +}
> > +
> >  static inline unsigned long long *
> >  res_counter_member(struct res_counter *counter, int member)
> >  {
> > --
> > 1.7.5.4
> >
> >

[PATCH 7/7] cgroups: Add a max number of tasks subsystem

[Frederic Weisbecker]

Add a new subsystem to limit the number of running tasks,
similar to the NR_PROC rlimit but in the scope of a cgroup.

This is a step to be able to isolate a bit more a cgroup against
the rest of the system and limit the global impact of a fork bomb
inside a given cgroup.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Paul Menage <menage@google.com>
Cc: Li Zefan <lizf@cn.fujitsu.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Aditya Kali <adityakali@google.com>
---
 include/linux/cgroup.h        |    9 ++
 include/linux/cgroup_subsys.h |    8 ++
 init/Kconfig                  |    7 ++
 kernel/Makefile               |    1 +
 kernel/cgroup_max_tasks.c     |  176 +++++++++++++++++++++++++++++++++++++++++
 kernel/fork.c                 |    4 +
 6 files changed, 205 insertions(+), 0 deletions(-)
 create mode 100644 kernel/cgroup_max_tasks.c

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 94454143..e6dec8c 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -660,4 +660,13 @@ static inline int cgroup_attach_task_current_cg(struct task_struct *t)
 
 #endif /* !CONFIG_CGROUPS */
 
+#ifdef CONFIG_CGROUP_MAX_TASKS
+int cgroup_max_tasks_fork(struct task_struct *child);
+#else
+static inline int cgroup_max_tasks_fork(struct task_struct *child)
+{
+	return 0;
+}
+#endif /* CONFIG_CGROUP_MAX_TASKS */
+
 #endif /* _LINUX_CGROUP_H */
diff --git a/include/linux/cgroup_subsys.h b/include/linux/cgroup_subsys.h
index ac663c1..227540d 100644
--- a/include/linux/cgroup_subsys.h
+++ b/include/linux/cgroup_subsys.h
@@ -59,8 +59,16 @@ SUBSYS(net_cls)
 SUBSYS(blkio)
 #endif
 
+/* */
+
 #ifdef CONFIG_CGROUP_PERF
 SUBSYS(perf)
 #endif
 
 /* */
+
+#ifdef CONFIG_CGROUP_MAX_TASKS
+SUBSYS(max_tasks)
+#endif
+
+/* */
diff --git a/init/Kconfig b/init/Kconfig
index 412c21b..46e4c03 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -690,6 +690,13 @@ config CGROUP_MEM_RES_CTLR_SWAP_ENABLED
 	  select this option (if, for some reason, they need to disable it
 	  then noswapaccount does the trick).
 
+config CGROUP_MAX_TASKS
+        bool "Control max number of tasks in a cgroup"
+	depends on RESOURCE_COUNTERS
+	help
+	  This option let the user to set up an upper bound allowed number
+	  of tasks.
+
 config CGROUP_PERF
 	bool "Enable perf_event per-cpu per-container group (cgroup) monitoring"
 	depends on PERF_EVENTS && CGROUPS
diff --git a/kernel/Makefile b/kernel/Makefile
index 2d64cfc..1b5722e 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -60,6 +60,7 @@ obj-$(CONFIG_BACKTRACE_SELF_TEST) += backtracetest.o
 obj-$(CONFIG_COMPAT) += compat.o
 obj-$(CONFIG_CGROUPS) += cgroup.o
 obj-$(CONFIG_CGROUP_FREEZER) += cgroup_freezer.o
+obj-$(CONFIG_CGROUP_MAX_TASKS) += cgroup_max_tasks.o
 obj-$(CONFIG_CPUSETS) += cpuset.o
 obj-$(CONFIG_UTS_NS) += utsname.o
 obj-$(CONFIG_USER_NS) += user_namespace.o
diff --git a/kernel/cgroup_max_tasks.c b/kernel/cgroup_max_tasks.c
new file mode 100644
index 0000000..2c38105
--- /dev/null
+++ b/kernel/cgroup_max_tasks.c
@@ -0,0 +1,176 @@
+/*
+ * Limits on number of tasks subsystem for cgroups
+ *
+ * Copyright (C) 2011 Red Hat, Inc., Frederic Weisbecker <fweisbec@redhat.com>
+ *
+ * Thanks to Johannes Weiner, Li Zefan, Paul Menage for their suggestions.
+ *
+ */
+
+#include <linux/cgroup.h>
+#include <linux/slab.h>
+#include <linux/res_counter.h>
+
+
+struct task_counter {
+	struct res_counter		res;
+	struct cgroup_subsys_state	css;
+};
+
+/*
+ * The root task counter is ignored and is not part of the
+ * whole task counting in order to optimize the trivial case
+ * of only one root cgroup living.
+ */
+static struct task_counter root_counter;
+
+
+static inline struct task_counter *cgroup_task_counter(struct cgroup *cont)
+{
+	return container_of(cgroup_subsys_state(cont, max_tasks_subsys_id),
+			    struct task_counter, css);
+}
+
+static struct cgroup_subsys_state *
+task_counter_create(struct cgroup_subsys *ss, struct cgroup *cgrp)
+{
+	struct task_counter *cnt, *parent_cnt;
+
+	if (!cgrp->parent) {
+		res_counter_init(&root_counter.res, NULL);
+		return &root_counter.css;
+	}
+
+	cnt = kzalloc(sizeof(*cnt), GFP_KERNEL);
+	if (!cnt)
+		return ERR_PTR(-ENOMEM);
+
+	parent_cnt = cgroup_task_counter(cgrp->parent);
+	res_counter_init(&cnt->res, &parent_cnt->res);
+
+	return &cnt->css;
+}
+
+static void task_counter_post_clone(struct cgroup_subsys *ss, struct cgroup *cgrp)
+{
+	struct task_counter *cnt = cgroup_task_counter(cgrp);
+
+	res_counter_inherit(&cnt->res, RES_LIMIT);
+}
+
+static void task_counter_destroy(struct cgroup_subsys *ss, struct cgroup *cont)
+{
+	struct task_counter *cnt = cgroup_task_counter(cont);
+
+	kfree(cnt);
+}
+
+static void task_counter_exit(struct cgroup_subsys *ss, struct cgroup *cgrp,
+			      struct cgroup *old_cgrp, struct task_struct *task)
+{
+	struct task_counter *cnt = cgroup_task_counter(old_cgrp);
+
+	if (cnt != &root_counter)
+		res_counter_uncharge_until(&cnt->res, &root_counter.res, 1);
+}
+
+/* Protected amongst can_attach_task/attach_task/cancel_attach_task by cgroup mutex */
+static struct res_counter *common_ancestor;
+
+static int task_counter_can_attach_task(struct cgroup *cgrp, struct cgroup *old_cgrp,
+					struct task_struct *tsk)
+{
+	struct task_counter *cnt = cgroup_task_counter(cgrp);
+	struct task_counter *old_cnt = cgroup_task_counter(old_cgrp);
+	struct res_counter *limit_fail_at;
+
+	common_ancestor = res_counter_common_ancestor(&cnt->res, &old_cnt->res);
+
+	WARN_ON_ONCE(common_ancestor == NULL);
+
+	return res_counter_charge_until(&cnt->res, common_ancestor, 1, &limit_fail_at);
+}
+
+static void task_counter_cancel_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
+{
+	struct task_counter *cnt = cgroup_task_counter(cgrp);
+
+	res_counter_uncharge_until(&cnt->res, common_ancestor, 1);
+}
+
+static void task_counter_attach_task(struct cgroup *cgrp, struct cgroup *old_cgrp,
+				     struct task_struct *tsk)
+{
+	struct task_counter *cnt = cgroup_task_counter(old_cgrp);
+
+	res_counter_uncharge_until(&cnt->res, common_ancestor, 1);
+}
+
+static u64 max_proc_read_u64(struct cgroup *cont, struct cftype *cft)
+{
+	struct task_counter *cnt = cgroup_task_counter(cont);
+	int type = cft->private;
+
+	return res_counter_read_u64(&cnt->res, type);
+}
+
+static int max_proc_write_u64(struct cgroup *cgrp, struct cftype *cft, u64 val)
+{
+	struct task_counter *cnt = cgroup_task_counter(cgrp);
+	int type = cft->private;
+
+	res_counter_write_u64(&cnt->res, type, val);
+
+	return 0;
+}
+
+static struct cftype files[] = {
+	{
+		.name		= "max_proc",
+		.read_u64	= max_proc_read_u64,
+		.write_u64	= max_proc_write_u64,
+		.private	= RES_LIMIT,
+	},
+
+	{
+		.name		= "nr_proc",
+		.read_u64	= max_proc_read_u64,
+		.private	= RES_USAGE,
+	},
+};
+
+static int task_counter_populate(struct cgroup_subsys *ss, struct cgroup *cgrp)
+{
+	if (!cgrp->parent)
+		return 0;
+
+	return cgroup_add_files(cgrp, ss, files, ARRAY_SIZE(files));
+}
+
+int cgroup_max_tasks_fork(struct task_struct *child)
+{
+	struct cgroup_subsys_state *css = child->cgroups->subsys[max_tasks_subsys_id];
+	struct cgroup *cgrp = css->cgroup;
+	struct task_counter *cnt = cgroup_task_counter(cgrp);
+	struct res_counter *limit_fail_at;
+
+	/* Ignore root cgroup directory to optimize trivial common case */
+	if (cnt == &root_counter)
+		return 0;
+
+	return res_counter_charge_until(&cnt->res, &root_counter.res, 1, &limit_fail_at);
+}
+
+struct cgroup_subsys max_tasks_subsys = {
+	.name			= "max_tasks",
+	.subsys_id		= max_tasks_subsys_id,
+	.create			= task_counter_create,
+	.post_clone		= task_counter_post_clone,
+	.destroy		= task_counter_destroy,
+	.exit			= task_counter_exit,
+	.can_attach_task	= task_counter_can_attach_task,
+	.cancel_attach_task	= task_counter_cancel_attach_task,
+	.attach_task		= task_counter_attach_task,
+	.populate		= task_counter_populate,
+	.early_init		= 1,
+};
diff --git a/kernel/fork.c b/kernel/fork.c
index 0276c30..feca609 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1295,6 +1295,10 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 	p->group_leader = p;
 	INIT_LIST_HEAD(&p->thread_group);
 
+	retval = cgroup_max_tasks_fork(p);
+	if (retval)
+		goto bad_fork_free_pid;
+
 	/* Now that the task is set up, run cgroup callbacks if
 	 * necessary. We need to run them before the task is visible
 	 * on the tasklist. */
-- 
1.7.5.4

Re: [PATCH 7/7] cgroups: Add a max number of tasks subsystem

[Paul Menage]

On Mon, Jul 11, 2011 at 7:15 AM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
>  /* */
> +
> +#ifdef CONFIG_CGROUP_MAX_TASKS
> +SUBSYS(max_tasks)
> +#endif

I think I'd be inclined to make the naming slightly (IMO) cleaner:
call the subsystem 'tasks' and the files 'limit' and 'count' (or maybe
'usage' to be more in line with memcg).
>
> +config CGROUP_MAX_TASKS
> +        bool "Control max number of tasks in a cgroup"
> +       depends on RESOURCE_COUNTERS
> +       help
> +         This option let the user to set up an upper bound allowed number
> +         of tasks.
> +

Needs to depend on CGROUPS too?

> +
> +
> +struct task_counter {
> +       struct res_counter              res;
> +       struct cgroup_subsys_state      css;
> +};

All other CSS structures put the "css" field as the first thing in the
structure. Not sure that anything relies on that, but consistency
can't hurt and it probably makes the code fractionally smaller since
the translation between CSS and task_counter becomes a no-op.

> +static void task_counter_exit(struct cgroup_subsys *ss, struct cgroup *cgrp,
> +                             struct cgroup *old_cgrp, struct task_struct *task)
> +{
> +       struct task_counter *cnt = cgroup_task_counter(old_cgrp);
> +
> +       if (cnt != &root_counter)
> +               res_counter_uncharge_until(&cnt->res, &root_counter.res, 1);
> +}

Do we even need the root_counter to be exposed in any way? Why not
make children of the root cgroup just have a NULL parent res_counter?

You'll still need a task_counter object so that the cgroups framework
has a CSS object for cgroups housekeeping, but I don't think it needs
to actually be used for anything.

Paul

Re: [PATCH 7/7] cgroups: Add a max number of tasks subsystem

[Li Zefan]

Paul Menage wrote:
> On Mon, Jul 11, 2011 at 7:15 AM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
>>  /* */
>> +
>> +#ifdef CONFIG_CGROUP_MAX_TASKS
>> +SUBSYS(max_tasks)
>> +#endif
> 
> I think I'd be inclined to make the naming slightly (IMO) cleaner:
> call the subsystem 'tasks' and the files 'limit' and 'count' (or maybe
> 'usage' to be more in line with memcg).
>>
>> +config CGROUP_MAX_TASKS
>> +        bool "Control max number of tasks in a cgroup"
>> +       depends on RESOURCE_COUNTERS
>> +       help
>> +         This option let the user to set up an upper bound allowed number
>> +         of tasks.
>> +
> 
> Needs to depend on CGROUPS too?
> 

it's done implicitly, because it's inside "menuconfig CGROUPS".

>> +
>> +
>> +struct task_counter {
>> +       struct res_counter              res;
>> +       struct cgroup_subsys_state      css;
>> +};
> 
> All other CSS structures put the "css" field as the first thing in the
> structure. Not sure that anything relies on that, but consistency

no as far as I know, and that's a bad thing.

> can't hurt and it probably makes the code fractionally smaller since
> the translation between CSS and task_counter becomes a no-op.
> 
>> +static void task_counter_exit(struct cgroup_subsys *ss, struct cgroup *cgrp,
>> +                             struct cgroup *old_cgrp, struct task_struct *task)
>> +{
>> +       struct task_counter *cnt = cgroup_task_counter(old_cgrp);
>> +
>> +       if (cnt != &root_counter)
>> +               res_counter_uncharge_until(&cnt->res, &root_counter.res, 1);
>> +}
> 
> Do we even need the root_counter to be exposed in any way? Why not
> make children of the root cgroup just have a NULL parent res_counter?
> 
> You'll still need a task_counter object so that the cgroups framework
> has a CSS object for cgroups housekeeping, but I don't think it needs
> to actually be used for anything.
> 
> Paul
> 

Re: [PATCH 7/7] cgroups: Add a max number of tasks subsystem

[Frederic Weisbecker]

On Mon, Jul 25, 2011 at 06:17:08PM -0700, Paul Menage wrote:
> On Mon, Jul 11, 2011 at 7:15 AM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> >  /* */
> > +
> > +#ifdef CONFIG_CGROUP_MAX_TASKS
> > +SUBSYS(max_tasks)
> > +#endif
> 
> I think I'd be inclined to make the naming slightly (IMO) cleaner:
> call the subsystem 'tasks' and the files 'limit' and 'count' (or maybe
> 'usage' to be more in line with memcg).

Sure. Will rename.

> >
> > +config CGROUP_MAX_TASKS
> > +        bool "Control max number of tasks in a cgroup"
> > +       depends on RESOURCE_COUNTERS
> > +       help
> > +         This option let the user to set up an upper bound allowed number
> > +         of tasks.
> > +
> 
> Needs to depend on CGROUPS too?
> 
> > +
> > +
> > +struct task_counter {
> > +       struct res_counter              res;
> > +       struct cgroup_subsys_state      css;
> > +};
> 
> All other CSS structures put the "css" field as the first thing in the
> structure. Not sure that anything relies on that, but consistency
> can't hurt and it probably makes the code fractionally smaller since
> the translation between CSS and task_counter becomes a no-op.

I guess Li responded to the two above points.

> > +static void task_counter_exit(struct cgroup_subsys *ss, struct cgroup *cgrp,
> > +                             struct cgroup *old_cgrp, struct task_struct *task)
> > +{
> > +       struct task_counter *cnt = cgroup_task_counter(old_cgrp);
> > +
> > +       if (cnt != &root_counter)
> > +               res_counter_uncharge_until(&cnt->res, &root_counter.res, 1);
> > +}
> 
> Do we even need the root_counter to be exposed in any way? Why not
> make children of the root cgroup just have a NULL parent res_counter?
> 
> You'll still need a task_counter object so that the cgroups framework
> has a CSS object for cgroups housekeeping, but I don't think it needs
> to actually be used for anything.
> 
> Paul

Right, will try to get that.

Thanks.

Re: [PATCH 0/7] cgroups: New max number of tasks subsystem (was: cgroups rlim subsystem)

[Frederic Weisbecker]

On Mon, Jul 11, 2011 at 04:14:59PM +0200, Frederic Weisbecker wrote:
> Hi all,
> 
> Changes in this release are:
> 
> * switch from general rlim subsystem to self-contained max number of tasks
> subsystem
> 
> * Don't use subsys->fork() callback but a static call to avoid cgroups
> complications.
> 
> * When moving a task accross cgroups, don't charge/uncharge the common
> ancestors, to fix race against concurrent forks.
> To fix that, Paul proposed a res_counter_move_charge() that do
> all in once. But we need to be able to cancel the attachment in case
> a following subsystem refuses the attachment itself. Thus the charge
> and uncharge still need to be seperate. More specifically, we can
> only uncharge the old cgroup once we are sure of the attachment. Because
> we can't cancel that uncharge later due to possible forks in the middle.

Ah and following Li's suggestion, the root cgroup is not touched anymore.
Considering this feature is to isolate a bit cgroups local impact from
the rest of the system, we don't need to limit in the global scope.
And we already have ways to do it.

So we avoid the overhead of this subsystem for the trivial (only one
cgroup, root) case. Although I believe that cgroups are more and more used
and the trivial case is moving toward using more cgroups :) 

Also Li suggested I use atomic ops rather than res counter. I'm still
not sure I can really do that as it exposes the counters with possibly
temporary overloaded values in the fail case.

Thanks.

> 
> Thanks.
> 
> Frederic Weisbecker (7):
>   cgroups: Add res_counter_write_u64() API
>   cgroups: New resource counter inheritance API
>   cgroups: Add previous cgroup in can_attach_task/attach_task callbacks
>   cgroups: New cancel_attach_task subsystem callback
>   cgroups: Ability to stop res charge propagation on bounded ancestor
>   cgroups: Add res counter common ancestor searching
>   cgroups: Add a max number of tasks subsystem
> 
>  block/blk-cgroup.c            |   10 ++-
>  include/linux/cgroup.h        |   15 +++-
>  include/linux/cgroup_subsys.h |    8 ++
>  include/linux/res_counter.h   |   12 +++
>  init/Kconfig                  |    7 ++
>  kernel/Makefile               |    1 +
>  kernel/cgroup.c               |   25 ++++--
>  kernel/cgroup_freezer.c       |    3 +-
>  kernel/cgroup_max_tasks.c     |  176 +++++++++++++++++++++++++++++++++++++++++
>  kernel/cpuset.c               |    6 +-
>  kernel/events/core.c          |    5 +-
>  kernel/fork.c                 |    4 +
>  kernel/res_counter.c          |   88 ++++++++++++++++++---
>  kernel/sched.c                |    6 +-
>  14 files changed, 335 insertions(+), 31 deletions(-)
>  create mode 100644 kernel/cgroup_max_tasks.c
> 
> -- 
> 1.7.5.4
>