[PATCHSET cgroup/for-3.8] netcls_cgroup: implement hierarchy support

[PATCHSET cgroup/for-3.8] netcls_cgroup: implement hierarchy support

[Tejun Heo]

Hello, guys.

This patchset implements proper hierarchy support for netcls_cgroup.
Pretty simliar to the netprio one[3].  Simpler as each cgroup has
single config value instead of array of them.

This patchset contains the following three patches.

 0001-netcls_cgroup-introduce-netcls_mutex.patch
 0002-netcls_cgroup-introduce-cgroup_cls_state-is_local.patch
 0003-netcls_cgroup-implement-proper-hierarchy-support.patch

This patchset is on top of

cgroup/for-3.8 ef9fe980c6 ("cgroup_freezer: implement proper hierarchy support")
+ [1] "[PATCHSET cgroup/for-3.8] cgroup: allow ->post_create() to fail"
+ [2] "[PATCH 1/2] cgroup: s/CGRP_CLONE_CHILDREN/CGRP_CPUSET_CLONE_CHILDREN/"
      "[PATCH 2/2] cgroup, cpuset: remove cgroup_subsys->post_clone()"
+ [3] "[PATCHSET cgroup/for-3.8] netprio_cgroup: implement hierarchy support"

and available in the following git branch.

 git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git review-netcls_cgroup-hierarchy

diffstat follows.

 include/net/cls_cgroup.h |    1
 net/sched/cls_cgroup.c   |  102 ++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 88 insertions(+), 15 deletions(-)

Thanks.

--
tejun

[1] http://thread.gmane.org/gmane.linux.kernel.cgroups/5047
[2] http://thread.gmane.org/gmane.linux.kernel/1393151
[3] https://lkml.org/lkml/2012/11/16/514

[PATCH 1/3] netcls_cgroup: introduce netcls_mutex

[Tejun Heo]

Introduce netcls_mutex to synchronize modifications to
cgroup_cls_state.  New cgrp now inherits classid from ->css_online()
and write_classid() updates classid while holdin netcls_mutex.

As write_classid() doesn't propagate new configuration downwards, this
currently doesn't make any userland-visible difference, but will help
implementing proper hierarchy support.

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 net/sched/cls_cgroup.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index 8cdc18e..80a80c4 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -17,11 +17,14 @@
 #include <linux/skbuff.h>
 #include <linux/cgroup.h>
 #include <linux/rcupdate.h>
+#include <linux/mutex.h>
 #include <net/rtnetlink.h>
 #include <net/pkt_cls.h>
 #include <net/sock.h>
 #include <net/cls_cgroup.h>
 
+static DEFINE_MUTEX(netcls_mutex);
+
 static inline struct cgroup_cls_state *cgrp_cls_state(struct cgroup *cgrp)
 {
 	return container_of(cgroup_subsys_state(cgrp, net_cls_subsys_id),
@@ -42,12 +45,21 @@ static struct cgroup_subsys_state *cgrp_css_alloc(struct cgroup *cgrp)
 	if (!cs)
 		return ERR_PTR(-ENOMEM);
 
-	if (cgrp->parent)
-		cs->classid = cgrp_cls_state(cgrp->parent)->classid;
-
 	return &cs->css;
 }
 
+/* @cgrp coming online, inherit the parent's classid */
+static int cgrp_css_online(struct cgroup *cgrp)
+{
+	if (!cgrp->parent)
+		return 0;
+
+	mutex_lock(&netcls_mutex);
+	cgrp_cls_state(cgrp)->classid = cgrp_cls_state(cgrp->parent)->classid;
+	mutex_unlock(&netcls_mutex);
+	return 0;
+}
+
 static void cgrp_css_free(struct cgroup *cgrp)
 {
 	kfree(cgrp_cls_state(cgrp));
@@ -60,7 +72,9 @@ static u64 read_classid(struct cgroup *cgrp, struct cftype *cft)
 
 static int write_classid(struct cgroup *cgrp, struct cftype *cft, u64 value)
 {
+	mutex_lock(&netcls_mutex);
 	cgrp_cls_state(cgrp)->classid = (u32) value;
+	mutex_unlock(&netcls_mutex);
 	return 0;
 }
 
@@ -76,6 +90,7 @@ static struct cftype ss_files[] = {
 struct cgroup_subsys net_cls_subsys = {
 	.name		= "net_cls",
 	.css_alloc	= cgrp_css_alloc,
+	.css_online	= cgrp_css_online,
 	.css_free	= cgrp_css_free,
 	.subsys_id	= net_cls_subsys_id,
 	.base_cftypes	= ss_files,
-- 
1.7.11.7

Re: [PATCH 1/3] netcls_cgroup: introduce netcls_mutex

[Daniel Wagner]

On 17.11.2012 04:31, Tejun Heo wrote:
> Introduce netcls_mutex to synchronize modifications to
> cgroup_cls_state.  New cgrp now inherits classid from ->css_online()
> and write_classid() updates classid while holdin netcls_mutex.
>
> As write_classid() doesn't propagate new configuration downwards, this
> currently doesn't make any userland-visible difference, but will help
> implementing proper hierarchy support.
>
> Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Acked-by: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>

[PATCH 2/3] netcls_cgroup: introduce cgroup_cls_state->is_local

[Tejun Heo]

cs->is_local will be used to indicate whether the cgroup has its own
configuration or inherited from the parent.  It's set when classid is
configured by writing a positive value to cgroup file
"net_cls.classid" and cleared when a negative value is written.

is_local is visible to userland via cgroup file "net_cls.is_local" so
that userland can know whether a cgroup has its config or not.

This patch doesn't yet change hierarchy behavior.  The next patch will
use is_local to implement proper hierarchy.

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 include/net/cls_cgroup.h |  1 +
 net/sched/cls_cgroup.c   | 23 ++++++++++++++++++++---
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/include/net/cls_cgroup.h b/include/net/cls_cgroup.h
index b6a6eeb..5759d98 100644
--- a/include/net/cls_cgroup.h
+++ b/include/net/cls_cgroup.h
@@ -22,6 +22,7 @@ struct cgroup_cls_state
 {
 	struct cgroup_subsys_state css;
 	u32 classid;
+	bool is_local;	/* class id is explicitly configured for this cgroup */
 };
 
 extern void sock_update_classid(struct sock *sk);
diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index 80a80c4..6e3ef64 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -70,19 +70,36 @@ static u64 read_classid(struct cgroup *cgrp, struct cftype *cft)
 	return cgrp_cls_state(cgrp)->classid;
 }
 
-static int write_classid(struct cgroup *cgrp, struct cftype *cft, u64 value)
+static int write_classid(struct cgroup *cgrp, struct cftype *cft,
+			 const char *buf)
 {
+	struct cgroup_cls_state *cs = cgrp_cls_state(cgrp);
+	s64 v;
+
+	if (sscanf(buf, "%lld", &v) != 1)
+		return -EINVAL;
+
 	mutex_lock(&netcls_mutex);
-	cgrp_cls_state(cgrp)->classid = (u32) value;
+	cs->classid = clamp_val(v, 0, UINT_MAX);
+	cs->is_local = v >= 0;
 	mutex_unlock(&netcls_mutex);
 	return 0;
 }
 
+static u64 read_is_local(struct cgroup *cgrp, struct cftype *cft)
+{
+	return cgrp_cls_state(cgrp)->is_local;
+}
+
 static struct cftype ss_files[] = {
 	{
 		.name = "classid",
 		.read_u64 = read_classid,
-		.write_u64 = write_classid,
+		.write_string = write_classid,
+	},
+	{
+		.name = "is_local",
+		.read_u64 = read_is_local,
 	},
 	{ }	/* terminate */
 };
-- 
1.7.11.7

Re: [PATCH 2/3] netcls_cgroup: introduce cgroup_cls_state->is_local

[Daniel Wagner]

Hi Tejun,

On 17.11.2012 04:31, Tejun Heo wrote:
> -static int write_classid(struct cgroup *cgrp, struct cftype *cft, u64 value)
> +static int write_classid(struct cgroup *cgrp, struct cftype *cft,
> +			 const char *buf)
>   {
> +	struct cgroup_cls_state *cs = cgrp_cls_state(cgrp);
> +	s64 v;
> +
> +	if (sscanf(buf, "%lld", &v) != 1)
> +		return -EINVAL;
> +

This changes the user API slightly. cgroup_write_u64() uses 
simple_stroull() to parse the string. simple_stroull() allows to use 
either 0x1234 or 1234 as input. sscanf() will only handle the later type 
of input.

I noticed this because my test script stopped working:

    mkdir /sys/fs/cgroup/net_cls/a
    echo 0x100002 > /sys/fs/cgroup/net_cls/a/net_cls.classid # 10:2

    tc qdisc add dev $DEV root handle 10: htb
    tc class add dev $DEV parent 10: classid 10:2 htb rate 1mbit
    tc qdisc add dev $DEV parent 10:2 handle 20: sfq perturb 10

I am not completely sure if my setup is 100% correct, but at least
it seems to make something :)

cheers,
daniel

[PATCH v2 2/3] netcls_cgroup: introduce cgroup_cls_state->is_local

[Tejun Heo]

cs->is_local will be used to indicate whether the cgroup has its own
configuration or inherited from the parent.  It's set when classid is
configured by writing a positive value to cgroup file
"net_cls.classid" and cleared when a negative value is written.

is_local is visible to userland via cgroup file "net_cls.is_local" so
that userland can know whether a cgroup has its config or not.

This patch doesn't yet change hierarchy behavior.  The next patch will
use is_local to implement proper hierarchy.

v2: Daniel pointed out that cftype->write_u64() accepts base prefix
    (e.g. 0x10 for 16).  Updated "%lld" to "%lli".

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Acked-by: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Acked-by: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Cc: Daniel Wagner <wagi-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>
---
 include/net/cls_cgroup.h |    1 +
 net/sched/cls_cgroup.c   |   23 ++++++++++++++++++++---
 2 files changed, 21 insertions(+), 3 deletions(-)

--- a/include/net/cls_cgroup.h
+++ b/include/net/cls_cgroup.h
@@ -22,6 +22,7 @@ struct cgroup_cls_state
 {
 	struct cgroup_subsys_state css;
 	u32 classid;
+	bool is_local;	/* class id is explicitly configured for this cgroup */
 };
 
 extern void sock_update_classid(struct sock *sk);
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -70,19 +70,36 @@ static u64 read_classid(struct cgroup *c
 	return cgrp_cls_state(cgrp)->classid;
 }
 
-static int write_classid(struct cgroup *cgrp, struct cftype *cft, u64 value)
+static int write_classid(struct cgroup *cgrp, struct cftype *cft,
+			 const char *buf)
 {
+	struct cgroup_cls_state *cs = cgrp_cls_state(cgrp);
+	s64 v;
+
+	if (sscanf(buf, "%lli", &v) != 1)
+		return -EINVAL;
+
 	mutex_lock(&netcls_mutex);
-	cgrp_cls_state(cgrp)->classid = (u32) value;
+	cs->classid = clamp_val(v, 0, UINT_MAX);
+	cs->is_local = v >= 0;
 	mutex_unlock(&netcls_mutex);
 	return 0;
 }
 
+static u64 read_is_local(struct cgroup *cgrp, struct cftype *cft)
+{
+	return cgrp_cls_state(cgrp)->is_local;
+}
+
 static struct cftype ss_files[] = {
 	{
 		.name = "classid",
 		.read_u64 = read_classid,
-		.write_u64 = write_classid,
+		.write_string = write_classid,
+	},
+	{
+		.name = "is_local",
+		.read_u64 = read_is_local,
 	},
 	{ }	/* terminate */
 };

Re: [PATCH v2 2/3] netcls_cgroup: introduce cgroup_cls_state->is_local

[Daniel Wagner]

Hi Tejun,

On 19.11.2012 18:30, Tejun Heo wrote:
> cs->is_local will be used to indicate whether the cgroup has its own
> configuration or inherited from the parent.  It's set when classid is
> configured by writing a positive value to cgroup file
> "net_cls.classid" and cleared when a negative value is written.
>
> is_local is visible to userland via cgroup file "net_cls.is_local" so
> that userland can know whether a cgroup has its config or not.
>
> This patch doesn't yet change hierarchy behavior.  The next patch will
> use is_local to implement proper hierarchy.
>
> v2: Daniel pointed out that cftype->write_u64() accepts base prefix
>      (e.g. 0x10 for 16).  Updated "%lld" to "%lli".
>
> Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Acked-by: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
> Acked-by: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
> Cc: Daniel Wagner <wagi-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>

Acked-by: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>

thanks,
daniel

[PATCH 3/3] netcls_cgroup: implement proper hierarchy support

[Tejun Heo]

netcls_cgroup implemented rather weird hierarchy behavior.  A new
cgroup would inherit configuration from its parent but once created it
operates independently from its parent - updates to a parent doesn't
affect its children.

Proper hierarchy behavior can easily be implemented using cgroup
descendant iterator and the is_local flag.  Writing a positive value
to "net_cls.classid" updates the cgroup's classid and propagates the
classid downwards.  Writing a negative value removes the local config
and makes it inherit the parent's classid, and the inherited classid
is propagated downwards.

This makes netcls_cgroup properly hierarchical and behave the same way
as netprio_cgroup.

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 net/sched/cls_cgroup.c | 62 +++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 51 insertions(+), 11 deletions(-)

diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index 6e3ef64..e9e24ac 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -70,6 +70,44 @@ static u64 read_classid(struct cgroup *cgrp, struct cftype *cft)
 	return cgrp_cls_state(cgrp)->classid;
 }
 
+/**
+ * propagate_classid - proapgate classid configuration downwards
+ * @root: cgroup to propagate classid down from
+ *
+ * Propagate @root's classid to descendants of @root.  Each descendant of
+ * @root re-inherits from its parent in pre-order tree walk.  This should
+ * be called after the classid of @root is changed to keep the descendants
+ * up-to-date.
+ *
+ * This may race with a new cgroup coming online and propagation may happen
+ * before finishing ->css_online() or while being taken offline.  As a
+ * cgroup_cls_state is ready after ->css_alloc() and propagation doesn't
+ * affect the parent, this is safe.
+ *
+ * Should be called with netcls_mutex held.
+ */
+static void propagate_classid(struct cgroup *root)
+{
+	struct cgroup *pos;
+
+	lockdep_assert_held(&netcls_mutex);
+	rcu_read_lock();
+
+	cgroup_for_each_descendant_pre(pos, root) {
+		struct cgroup_cls_state *cs = cgrp_cls_state(pos);
+
+		/*
+		 * Don't propagate if @pos has local configuration.  We can
+		 * skip @pos's subtree but don't have to.  Just propagate
+		 * through for simplicity.
+		 */
+		if (!cs->is_local)
+			cs->classid = cgrp_cls_state(pos->parent)->classid;
+	}
+
+	rcu_read_unlock();
+}
+
 static int write_classid(struct cgroup *cgrp, struct cftype *cft,
 			 const char *buf)
 {
@@ -80,8 +118,19 @@ static int write_classid(struct cgroup *cgrp, struct cftype *cft,
 		return -EINVAL;
 
 	mutex_lock(&netcls_mutex);
-	cs->classid = clamp_val(v, 0, UINT_MAX);
-	cs->is_local = v >= 0;
+
+	if (v >= 0) {
+		cs->classid = clamp_val(v, 0, UINT_MAX);
+		cs->is_local = true;
+	} else {
+		if (cgrp->parent)
+			cs->classid = cgrp_cls_state(cgrp->parent)->classid;
+		else
+			cs->classid = 0;
+		cs->is_local = false;
+	}
+	propagate_classid(cgrp);
+
 	mutex_unlock(&netcls_mutex);
 	return 0;
 }
@@ -112,15 +161,6 @@ struct cgroup_subsys net_cls_subsys = {
 	.subsys_id	= net_cls_subsys_id,
 	.base_cftypes	= ss_files,
 	.module		= THIS_MODULE,
-
-	/*
-	 * While net_cls cgroup has the rudimentary hierarchy support of
-	 * inheriting the parent's classid on cgroup creation, it doesn't
-	 * properly propagates config changes in ancestors to their
-	 * descendents.  A child should follow the parent's configuration
-	 * but be allowed to override it.  Fix it and remove the following.
-	 */
-	.broken_hierarchy = true,
 };
 
 struct cls_cgroup_head {
-- 
1.7.11.7

Re: [PATCH 3/3] netcls_cgroup: implement proper hierarchy support

[Daniel Wagner]

Hi Tejun,

On 17.11.2012 04:31, Tejun Heo wrote:
> @@ -112,15 +161,6 @@ struct cgroup_subsys net_cls_subsys = {
>   	.subsys_id	= net_cls_subsys_id,
>   	.base_cftypes	= ss_files,
>   	.module		= THIS_MODULE,
> -
> -	/*
> -	 * While net_cls cgroup has the rudimentary hierarchy support of
> -	 * inheriting the parent's classid on cgroup creation, it doesn't
> -	 * properly propagates config changes in ancestors to their
> -	 * descendents.  A child should follow the parent's configuration
> -	 * but be allowed to override it.  Fix it and remove the following.
> -	 */
> -	.broken_hierarchy = true,
>   };

Are you sure you want to set the default to false at this point?

cheers,
daniel

Re: [PATCH 3/3] netcls_cgroup: implement proper hierarchy support

[Tejun Heo]

Hello, Daniel.

On Mon, Nov 19, 2012 at 02:47:06PM +0100, Daniel Wagner wrote:
> On 17.11.2012 04:31, Tejun Heo wrote:
> >@@ -112,15 +161,6 @@ struct cgroup_subsys net_cls_subsys = {
> >  	.subsys_id	= net_cls_subsys_id,
> >  	.base_cftypes	= ss_files,
> >  	.module		= THIS_MODULE,
> >-
> >-	/*
> >-	 * While net_cls cgroup has the rudimentary hierarchy support of
> >-	 * inheriting the parent's classid on cgroup creation, it doesn't
> >-	 * properly propagates config changes in ancestors to their
> >-	 * descendents.  A child should follow the parent's configuration
> >-	 * but be allowed to override it.  Fix it and remove the following.
> >-	 */
> >-	.broken_hierarchy = true,
> >  };
> 
> Are you sure you want to set the default to false at this point?

Hmmm.... why not?  It's now implementing proper hierarchy behavior.

Thanks.

-- 
tejun

Re: [PATCH 3/3] netcls_cgroup: implement proper hierarchy support

[Daniel Wagner]

Hi Tejun,

On 19.11.2012 18:08, Tejun Heo wrote:
> Hello, Daniel.
> 
> On Mon, Nov 19, 2012 at 02:47:06PM +0100, Daniel Wagner wrote:
>> On 17.11.2012 04:31, Tejun Heo wrote:
>>> @@ -112,15 +161,6 @@ struct cgroup_subsys net_cls_subsys = {
>>>   	.subsys_id	= net_cls_subsys_id,
>>>   	.base_cftypes	= ss_files,
>>>   	.module		= THIS_MODULE,
>>> -
>>> -	/*
>>> -	 * While net_cls cgroup has the rudimentary hierarchy support of
>>> -	 * inheriting the parent's classid on cgroup creation, it doesn't
>>> -	 * properly propagates config changes in ancestors to their
>>> -	 * descendents.  A child should follow the parent's configuration
>>> -	 * but be allowed to override it.  Fix it and remove the following.
>>> -	 */
>>> -	.broken_hierarchy = true,
>>>   };
>>
>> Are you sure you want to set the default to false at this point?
> 
> Hmmm.... why not?  It's now implementing proper hierarchy behavior.

Well, I though the rule is to keep API as stable as possible. On second
though this change will only extend the API, so for the existing
user base nothing changes.

Acked-by: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>

thanks,
daniel

Re: [PATCHSET cgroup/for-3.8] netcls_cgroup: implement hierarchy support

[David Miller]

From: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Date: Fri, 16 Nov 2012 19:30:59 -0800

> Hello, guys.
> 
> This patchset implements proper hierarchy support for netcls_cgroup.
> Pretty simliar to the netprio one[3].  Simpler as each cgroup has
> single config value instead of array of them.
> 
> This patchset contains the following three patches.
> 
>  0001-netcls_cgroup-introduce-netcls_mutex.patch
>  0002-netcls_cgroup-introduce-cgroup_cls_state-is_local.patch
>  0003-netcls_cgroup-implement-proper-hierarchy-support.patch
> 
> This patchset is on top of

I guess you therefore want to put it through your cgroup tree?

If so:

Acked-by: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>

Re: [PATCHSET cgroup/for-3.8] netcls_cgroup: implement hierarchy support

[Tejun Heo]

Hello,

On Sat, Nov 17, 2012 at 10:04:21PM -0500, David Miller wrote:
> > This patchset implements proper hierarchy support for netcls_cgroup.
> > Pretty simliar to the netprio one[3].  Simpler as each cgroup has
> > single config value instead of array of them.
> > 
> > This patchset contains the following three patches.
> > 
> >  0001-netcls_cgroup-introduce-netcls_mutex.patch
> >  0002-netcls_cgroup-introduce-cgroup_cls_state-is_local.patch
> >  0003-netcls_cgroup-implement-proper-hierarchy-support.patch
> > 
> > This patchset is on top of
> 
> I guess you therefore want to put it through your cgroup tree?

Yeap.

> Acked-by: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>

Great, I forgot to cc you on netprio_cgroup changes.  It's similar to
this one but larger partly because the configuration is per
cgroup-netdev pair.  Would it be okay to route the following through
the cgroup tree?

  https://lkml.org/lkml/2012/11/16/514

Thanks.

-- 
tejun

Re: [PATCHSET cgroup/for-3.8] netcls_cgroup: implement hierarchy support

[Daniel Wagner]

Hi Tejun,

On 17.11.2012 04:30, Tejun Heo wrote:
> Hello, guys.
> 
> This patchset implements proper hierarchy support for netcls_cgroup.
> Pretty simliar to the netprio one[3].  Simpler as each cgroup has
> single config value instead of array of them.
> 
> This patchset contains the following three patches.
> 
>   0001-netcls_cgroup-introduce-netcls_mutex.patch
>   0002-netcls_cgroup-introduce-cgroup_cls_state-is_local.patch
>   0003-netcls_cgroup-implement-proper-hierarchy-support.patch
> 
> This patchset is on top of
> 
> cgroup/for-3.8 ef9fe980c6 ("cgroup_freezer: implement proper hierarchy support")
> + [1] "[PATCHSET cgroup/for-3.8] cgroup: allow ->post_create() to fail"
> + [2] "[PATCH 1/2] cgroup: s/CGRP_CLONE_CHILDREN/CGRP_CPUSET_CLONE_CHILDREN/"
>        "[PATCH 2/2] cgroup, cpuset: remove cgroup_subsys->post_clone()"
> + [3] "[PATCHSET cgroup/for-3.8] netprio_cgroup: implement hierarchy support"
> 
> and available in the following git branch.
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git review-netcls_cgroup-hierarchy
> 
> diffstat follows.
> 
>   include/net/cls_cgroup.h |    1
>   net/sched/cls_cgroup.c   |  102 ++++++++++++++++++++++++++++++++++++++++-------
>   2 files changed, 88 insertions(+), 15 deletions(-)
> 
> Thanks.

I played a bit around with this series, e.g. creating a hierarchy and and changing 
the root classid and seeing the change propagating through the hierarchy. Everything
worked as described in the documentation.

Sorry to bring this up again: how should to root cgroup behave? If the ultimate
goal to have only one single hierarchy then I would assume it is important that
the semantic for all controllers are the same. As you pointed out the networking
controllers are kind of a strange beast in the zoo of the cgroup controllers. 
But still I would assume that all root controllers behave the same. memcg or cpu*
are not expected to do any work in the root cgroup.

cheers,
daniel

Re: [PATCHSET cgroup/for-3.8] netcls_cgroup: implement hierarchy support

[Tejun Heo]

Hello, Daniel.

On Mon, Nov 19, 2012 at 02:59:58PM +0100, Daniel Wagner wrote:
> Sorry to bring this up again: how should to root cgroup behave? If
> the ultimate goal to have only one single hierarchy then I would
> assume it is important that the semantic for all controllers are the
> same. As you pointed out the networking controllers are kind of a
> strange beast in the zoo of the cgroup controllers.  But still I
> would assume that all root controllers behave the same. memcg or
> cpu* are not expected to do any work in the root cgroup.

I think the implemented behavior is fine.  memcg or cpu* don't do
anything on root cgroup as it doesn't make much (or any) sense.  For
net_prio and cls, I don't see any reason to treat root cgroup
differently and how that would cause conflict when mounted together
with other controllers.  So, yeah, things seem okay to me.

Thanks.

-- 
tejun

Re: [PATCHSET cgroup/for-3.8] netcls_cgroup: implement hierarchy support

[Neil Horman]

On Fri, Nov 16, 2012 at 07:30:59PM -0800, Tejun Heo wrote:
> Hello, guys.
> 
> This patchset implements proper hierarchy support for netcls_cgroup.
> Pretty simliar to the netprio one[3].  Simpler as each cgroup has
> single config value instead of array of them.
> 
> This patchset contains the following three patches.
> 
>  0001-netcls_cgroup-introduce-netcls_mutex.patch
>  0002-netcls_cgroup-introduce-cgroup_cls_state-is_local.patch
>  0003-netcls_cgroup-implement-proper-hierarchy-support.patch
> 
> This patchset is on top of
> 
> cgroup/for-3.8 ef9fe980c6 ("cgroup_freezer: implement proper hierarchy support")
> + [1] "[PATCHSET cgroup/for-3.8] cgroup: allow ->post_create() to fail"
> + [2] "[PATCH 1/2] cgroup: s/CGRP_CLONE_CHILDREN/CGRP_CPUSET_CLONE_CHILDREN/"
>       "[PATCH 2/2] cgroup, cpuset: remove cgroup_subsys->post_clone()"
> + [3] "[PATCHSET cgroup/for-3.8] netprio_cgroup: implement hierarchy support"
> 
> and available in the following git branch.
> 
>  git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git review-netcls_cgroup-hierarchy
> 
> diffstat follows.
> 
>  include/net/cls_cgroup.h |    1
>  net/sched/cls_cgroup.c   |  102 ++++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 88 insertions(+), 15 deletions(-)
> 
> Thanks.
> 
> --
> tejun
> 
> [1] http://thread.gmane.org/gmane.linux.kernel.cgroups/5047
> [2] http://thread.gmane.org/gmane.linux.kernel/1393151
> [3] https://lkml.org/lkml/2012/11/16/514
> 

Acked-by: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>

Re: [PATCHSET cgroup/for-3.8] netcls_cgroup: implement hierarchy support

[Tejun Heo]

On Fri, Nov 16, 2012 at 07:30:59PM -0800, Tejun Heo wrote:
> Hello, guys.
> 
> This patchset implements proper hierarchy support for netcls_cgroup.
> Pretty simliar to the netprio one[3].  Simpler as each cgroup has
> single config value instead of array of them.

Applied to cgroup/for-3.8.

Thanks.

-- 
tejun

Re: [PATCHSET cgroup/for-3.8] netcls_cgroup: implement hierarchy support

[Tejun Heo]

Hello, guys.

On Mon, Nov 19, 2012 at 11:21:42AM -0800, Tejun Heo wrote:
> On Fri, Nov 16, 2012 at 07:30:59PM -0800, Tejun Heo wrote:
> > Hello, guys.
> > 
> > This patchset implements proper hierarchy support for netcls_cgroup.
> > Pretty simliar to the netprio one[3].  Simpler as each cgroup has
> > single config value instead of array of them.
> 
> Applied to cgroup/for-3.8.

I'm kinda having a second thought about this and reverted it for the
time being.  The problem is that there are other configurations which
are only inherited while creating a new child cgroup without
propagating new configuration afterwards -
e.g. cpuset.memory_spread_*, and given that cpuset has been around for
quite some time and has been one of the popular controllers, I don't
think we can change the behavior at this point.

It looks like we'll have to have some mixture of both behaviors.
Anything resource limit related should affect its descendants while
other per-cgroup attributes don't, with the distinction between the
two muddy at times.

So, if we're gonna have to do that anyway, I can't find a strong
rationale for changing net_cls's behavior of not propagating on
ancestor update, and net_prio should match net_cls's behavior -
ie. inherit on cgroup creation but then operate separately.

I'll prep a new patchset to update netprio accordingly.

Thanks.

-- 
tejun