Patchwork [7/7] netprio_cgroup: allow nesting and inherit config on cgroup creation

login
register
mail settings
Submitter Tejun Heo
Date Nov. 20, 2012, 8:30 a.m.
Message ID <1353400211-5182-8-git-send-email-tj@kernel.org>
Download mbox | patch
Permalink /patch/200252/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Tejun Heo - Nov. 20, 2012, 8:30 a.m.
Inherit netprio configuration from ->css_online(), allow nesting and
remove .broken_hierarchy marking.  This makes netprio_cgroup's
behavior match netcls_cgroup's.

Note that this patch changes userland-visible behavior.  Nesting is
allowed and the first level cgroups below the root cgroup behave
differently - they inherit priorities from the root cgroup on creation
instead of starting with 0.  This is unfortunate but not doing so is
much crazier.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 Documentation/cgroups/net_prio.txt |  2 ++
 net/core/netprio_cgroup.c          | 42 ++++++++++++++++++++++----------------
 2 files changed, 26 insertions(+), 18 deletions(-)
Daniel Wagner - Nov. 20, 2012, 8:57 a.m.
Hi Tejun,

On 20.11.2012 09:30, Tejun Heo wrote:
> Inherit netprio configuration from ->css_online(), allow nesting and
> remove .broken_hierarchy marking.  This makes netprio_cgroup's
> behavior match netcls_cgroup's.
>
> Note that this patch changes userland-visible behavior.  Nesting is
> allowed and the first level cgroups below the root cgroup behave
> differently - they inherit priorities from the root cgroup on creation
> instead of starting with 0.  This is unfortunate but not doing so is
> much crazier.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>

Tested and Acked-by: Daniel Wagner <daniel.wagner@bmw-carit.de>

> ---
>   Documentation/cgroups/net_prio.txt |  2 ++
>   net/core/netprio_cgroup.c          | 42 ++++++++++++++++++++++----------------
>   2 files changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/Documentation/cgroups/net_prio.txt b/Documentation/cgroups/net_prio.txt
> index 01b3226..a82cbd2 100644
> --- a/Documentation/cgroups/net_prio.txt
> +++ b/Documentation/cgroups/net_prio.txt
> @@ -51,3 +51,5 @@ One usage for the net_prio cgroup is with mqprio qdisc allowing application
>   traffic to be steered to hardware/driver based traffic classes. These mappings
>   can then be managed by administrators or other networking protocols such as
>   DCBX.
> +
> +A new net_prio cgroup inherits the parent's configuration.
> diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
> index b2af0d0..bde53da 100644
> --- a/net/core/netprio_cgroup.c
> +++ b/net/core/netprio_cgroup.c
> @@ -136,9 +136,6 @@ static struct cgroup_subsys_state *cgrp_css_alloc(struct cgroup *cgrp)
>   {
>   	struct cgroup_netprio_state *cs;
>
> -	if (cgrp->parent && cgrp->parent->id)
> -		return ERR_PTR(-EINVAL);
> -
>   	cs = kzalloc(sizeof(*cs), GFP_KERNEL);
>   	if (!cs)
>   		return ERR_PTR(-ENOMEM);
> @@ -146,16 +143,34 @@ static struct cgroup_subsys_state *cgrp_css_alloc(struct cgroup *cgrp)
>   	return &cs->css;
>   }
>
> -static void cgrp_css_free(struct cgroup *cgrp)
> +static int cgrp_css_online(struct cgroup *cgrp)
>   {
> -	struct cgroup_netprio_state *cs = cgrp_netprio_state(cgrp);
> +	struct cgroup *parent = cgrp->parent;
>   	struct net_device *dev;
> +	int ret = 0;
> +
> +	if (!parent)
> +		return 0;

BTW, parent is always != NULL, because the root cgroup will be attached 
to the dummytop cgroup.

This is the reason why there are this check in netprio_prio()

static u32 netprio_prio(struct cgroup *cgrp, struct net_device *dev)
{
	struct netprio_map *map = rcu_dereference_rtnl(dev->priomap);

	if (map && cgrp->id < map->priomap_len)
		return map->priomap[cgrp->id];

is necessary.

cheer,
daniel
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - Nov. 20, 2012, 2:40 p.m.
Hello, Daniel.

On Tue, Nov 20, 2012 at 09:57:14AM +0100, Daniel Wagner wrote:
> >-static void cgrp_css_free(struct cgroup *cgrp)
> >+static int cgrp_css_online(struct cgroup *cgrp)
> >  {
> >-	struct cgroup_netprio_state *cs = cgrp_netprio_state(cgrp);
> >+	struct cgroup *parent = cgrp->parent;
> >  	struct net_device *dev;
> >+	int ret = 0;
> >+
> >+	if (!parent)
> >+		return 0;
> 
> BTW, parent is always != NULL, because the root cgroup will be
> attached to the dummytop cgroup.

Hmmm?  I'm confused.  When ->css_online() is called for dummytop in
cgroup_init_subsys(), its parent is NULL.  What am I missing?

Thanks.
Daniel Wagner - Nov. 20, 2012, 3:10 p.m.
Hi Tejun,

On 20.11.2012 15:40, Tejun Heo wrote:
> Hello, Daniel.
>
> On Tue, Nov 20, 2012 at 09:57:14AM +0100, Daniel Wagner wrote:
>>> -static void cgrp_css_free(struct cgroup *cgrp)
>>> +static int cgrp_css_online(struct cgroup *cgrp)
>>>   {
>>> -	struct cgroup_netprio_state *cs = cgrp_netprio_state(cgrp);
>>> +	struct cgroup *parent = cgrp->parent;
>>>   	struct net_device *dev;
>>> +	int ret = 0;
>>> +
>>> +	if (!parent)
>>> +		return 0;
>>
>> BTW, parent is always != NULL, because the root cgroup will be
>> attached to the dummytop cgroup.
>
> Hmmm?  I'm confused.  When ->css_online() is called for dummytop in
> cgroup_init_subsys(), its parent is NULL.  What am I missing?

Forget my comment, I was really confused when writing this. I was 
looking only at cgroups which were created after bootup.

cheers,
daniel

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/Documentation/cgroups/net_prio.txt b/Documentation/cgroups/net_prio.txt
index 01b3226..a82cbd2 100644
--- a/Documentation/cgroups/net_prio.txt
+++ b/Documentation/cgroups/net_prio.txt
@@ -51,3 +51,5 @@  One usage for the net_prio cgroup is with mqprio qdisc allowing application
 traffic to be steered to hardware/driver based traffic classes. These mappings
 can then be managed by administrators or other networking protocols such as
 DCBX.
+
+A new net_prio cgroup inherits the parent's configuration.
diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
index b2af0d0..bde53da 100644
--- a/net/core/netprio_cgroup.c
+++ b/net/core/netprio_cgroup.c
@@ -136,9 +136,6 @@  static struct cgroup_subsys_state *cgrp_css_alloc(struct cgroup *cgrp)
 {
 	struct cgroup_netprio_state *cs;
 
-	if (cgrp->parent && cgrp->parent->id)
-		return ERR_PTR(-EINVAL);
-
 	cs = kzalloc(sizeof(*cs), GFP_KERNEL);
 	if (!cs)
 		return ERR_PTR(-ENOMEM);
@@ -146,16 +143,34 @@  static struct cgroup_subsys_state *cgrp_css_alloc(struct cgroup *cgrp)
 	return &cs->css;
 }
 
-static void cgrp_css_free(struct cgroup *cgrp)
+static int cgrp_css_online(struct cgroup *cgrp)
 {
-	struct cgroup_netprio_state *cs = cgrp_netprio_state(cgrp);
+	struct cgroup *parent = cgrp->parent;
 	struct net_device *dev;
+	int ret = 0;
+
+	if (!parent)
+		return 0;
 
 	rtnl_lock();
-	for_each_netdev(&init_net, dev)
-		WARN_ON_ONCE(netprio_set_prio(cgrp, dev, 0));
+	/*
+	 * Inherit prios from the parent.  As all prios are set during
+	 * onlining, there is no need to clear them on offline.
+	 */
+	for_each_netdev(&init_net, dev) {
+		u32 prio = netprio_prio(parent, dev);
+
+		ret = netprio_set_prio(cgrp, dev, prio);
+		if (ret)
+			break;
+	}
 	rtnl_unlock();
-	kfree(cs);
+	return ret;
+}
+
+static void cgrp_css_free(struct cgroup *cgrp)
+{
+	kfree(cgrp_netprio_state(cgrp));
 }
 
 static u64 read_prioidx(struct cgroup *cgrp, struct cftype *cft)
@@ -237,21 +252,12 @@  static struct cftype ss_files[] = {
 struct cgroup_subsys net_prio_subsys = {
 	.name		= "net_prio",
 	.css_alloc	= cgrp_css_alloc,
+	.css_online	= cgrp_css_online,
 	.css_free	= cgrp_css_free,
 	.attach		= net_prio_attach,
 	.subsys_id	= net_prio_subsys_id,
 	.base_cftypes	= ss_files,
 	.module		= THIS_MODULE,
-
-	/*
-	 * net_prio has artificial limit on the number of cgroups and
-	 * disallows nesting making it impossible to co-mount it with other
-	 * hierarchical subsystems.  Remove the artificially low PRIOIDX_SZ
-	 * limit and properly nest configuration such that children follow
-	 * their parents' configurations by default and are allowed to
-	 * override and remove the following.
-	 */
-	.broken_hierarchy = true,
 };
 
 static int netprio_device_event(struct notifier_block *unused,