netcls/prio_cgroup: update hierarchy support

It turns out that we'll have to live with attributes which are inherited at cgroup creation time but not affected by further updates to the parent afterwards - such attributes are already in wide use e.g. for cpuset.

So, there's nothing to do for netcls_cgroup for hierarchy support. Its current behavior - inherit only during creation - is good enough.

Move config inheriting from ->css_alloc() to ->css_online() for consistency, which doesn't change behavior at all, and remove .broken_hierarchy marking.

Signed-off-by Tejun Heo <[email protected]>

sscanf() doesn't bite.

Signed-off-by Tejun Heo <[email protected]>

Acked-by Neil Horman <[email protected]>

The function is about to go through a rewrite. In preparation, shorten the variable names so that we don't repeat "priomap" so often.

This patch is cosmetic.

Signed-off-by Tejun Heo <[email protected]>

Acked-by Neil Horman <[email protected]>

netprio kept track of the highest prioidx allocated and resized priomaps accordingly when necessary. This makes it necessary to keep track of prioidx allocation and may end up resizing on every new prioidx.

Update extend_netdev_table() such that it takes @target_idx which the priomap should be able to accomodate. If the priomap is large enough, nothing happens; otherwise, the size is doubled until @target_idx can be accomodated.

This makes max_prioidx and write_update_netdev_table() unnecessary. write_priomap() now calls extend_netdev_table() directly.

Signed-off-by Tejun Heo <[email protected]>

Acked-by Neil Horman <[email protected]>

---
  net/core/netprio_cgroup.c | 56 ++++++++++++++++++++++++++++-------------------
  1 file changed, 33 insertions(+), 23 deletions(-)

net/core/netprio_cgroup.c +31 -8

index 92cc54c..569d83d 100644
@@ -27,11 +27,11 @@
 
  #include <linux/fdtable.h>
 
+#define PRIOMAP_MIN_SZ		128
  #define PRIOIDX_SZ 128
 
  static unsigned long prioidx_map[PRIOIDX_SZ];
  static DEFINE_SPINLOCK(prioidx_map_lock);
-static atomic_t max_prioidx = ATOMIC_INIT(0);
 
  static inline struct cgroup_netprio_state *cgrp_netprio_state(struct cgroup *cgrp)
  {

@@ -51,8 +51,6 @@ static int get_prioidx(u32 *prio)
  		return -ENOSPC;
  	}
  	set_bit(prioidx, prioidx_map);
-	if (atomic_read(&max_prioidx) < prioidx)
-		atomic_set(&max_prioidx, prioidx);
  	spin_unlock_irqrestore(&prioidx_map_lock, flags);
  	*prio = prioidx;
  	return 0;

@@ -67,15 +65,40 @@ static void put_prioidx(u32 idx)
  	spin_unlock_irqrestore(&prioidx_map_lock, flags);
  }
 
-static int extend_netdev_table(struct net_device *dev, u32 new_len)
+/*
+ * Extend @dev->priomap so that it's large enough to accomodate
+ * @target_idx.  @dev->priomap.priomap_len > @target_idx after successful
+ * return.  Must be called under rtnl lock.
+ */
+static int extend_netdev_table(struct net_device *dev, u32 target_idx)
  {
-	size_t new_size = sizeof(struct netprio_map) +
-			   ((sizeof(u32) * new_len));
-	struct netprio_map *new = kzalloc(new_size, GFP_KERNEL);
-	struct netprio_map *old;
+	struct netprio_map *old, *new;
+	size_t new_sz, new_len;
 
+	/* is the existing priomap large enough? */
  	old = rtnl_dereference(dev->priomap);
+	if (old && old->priomap_len > target_idx)
+		return 0;
+
+	/*
+	 * Determine the new size.  Let's keep it power-of-two.  We start
+	 * from PRIOMAP_MIN_SZ and double it until it's large enough to
+	 * accommodate @target_idx.
+	 */
+	new_sz = PRIOMAP_MIN_SZ;
+	while (true) {
+		new_len = (new_sz - offsetof(struct netprio_map, priomap)) /
+			sizeof(new->priomap[0]);
+		if (new_len > target_idx)
+			break;
+		new_sz *= 2;
+		/* overflowed? */
+		if (WARN_ON(new_sz < PRIOMAP_MIN_SZ))
+			return -ENOSPC;
+	}
 
+	/* allocate & copy */
+	new = kzalloc(new_sz, GFP_KERNEL);
  	if (!new) {
  		pr_warn("Unable to alloc new priomap!\n");
  		return -ENOMEM;

struct netprio_map {
	struct rcu_head rcu;
	struct netprio_aux *aux;	/* auxiliary config array */
	u32 priomap_len;
	u32 priomap[];
};

Is there a specific reason why aux and priomap is handled differently? Couldn't you just use same approach for both variables, e.g. re/allocating only them here and leave the allocation struct netprio_map in cgrp_css_alloc()?

Also the algorithm to figure out the size of the array might be a bit too aggressive in my opinion. So you always start at PRIOMAP_MIN_SIZE and then try to double the size until target_idx fits. Wouldn't it make sense to start to look for the new size beginning at old->priomap_len and then do the power-of-two increase?

Hello, Daniel.

Quoted 7 lines · Daniel Wagner

struct netprio_map {
	struct rcu_head rcu;
	struct netprio_aux *aux;	/* auxiliary config array */
	u32 priomap_len;
	u32 priomap[];
};

Quoted 4 lines

Is there a specific reason why aux and priomap is handled differently? Couldn't you just use same approach for both variables, e.g. re/allocating only them here and leave the allocation struct netprio_map in cgrp_css_alloc()?

->aux is no longer added, so the consistency issue doesn't exist anymore.

The reason why they were handled differently before (or rather why I didn't change priomap[] to be allocated separately) was that pointer chasing tends to be more expensive than offsetting. I don't know how much effect it would have in this case but things sitting in packet in/out paths can be very hot so didn't wanna disturb it.

Quoted 5 lines

Also the algorithm to figure out the size of the array might be a bit too aggressive in my opinion. So you always start at PRIOMAP_MIN_SIZE and then try to double the size until target_idx fits. Wouldn't it make sense to start to look for the new size beginning at old->priomap_len and then do the power-of-two increase?

The only downside of always starting from PRIOMAP_MIN_SIZE is iterating several more times in the sizing loop which isn't really anything to worry about. The loop is structured that way because I wanted to keep the size of the whole thing power-of-two. Due to the fields before priomap[], if we size priomap_len power-of-two, we'll always end up with something slightly over power-of-two, which is usually the worst size to allocate.

Thanks for the explanation. I was pondering if the new size in power of two could be a bit too excessive and the allocation step could be linear, e.g. stick at 4096. target_id will increase linear, therefore linear increase might also be enough, no?

Hello,

Quoted 4 lines · Daniel Wagner

Thanks for the explanation. I was pondering if the new size in power of two could be a bit too excessive and the allocation step could be linear, e.g. stick at 4096. target_id will increase linear, therefore linear increase might also be enough, no?

Well, power-of-two resizing tends to behave relatively well under most cases and slab allocations are binned by power-of-two sizes, so linearly growing it doesn't really buy anything.

Hello,

Quoted 4 lines · Daniel Wagner

Thanks for the explanation. I was pondering if the new size in power of two could be a bit too excessive and the allocation step could be linear, e.g. stick at 4096. target_id will increase linear, therefore linear increase might also be enough, no?

Well, power-of-two resizing tends to behave relatively well under most cases and slab allocations are binned by power-of-two sizes, so linearly growing it doesn't really buy anything.

Hello, Daniel.

Quoted 7 lines · Daniel Wagner

struct netprio_map {
	struct rcu_head rcu;
	struct netprio_aux *aux;	/* auxiliary config array */
	u32 priomap_len;
	u32 priomap[];
};

Quoted 4 lines

Is there a specific reason why aux and priomap is handled differently? Couldn't you just use same approach for both variables, e.g. re/allocating only them here and leave the allocation struct netprio_map in cgrp_css_alloc()?

->aux is no longer added, so the consistency issue doesn't exist anymore.

The reason why they were handled differently before (or rather why I didn't change priomap[] to be allocated separately) was that pointer chasing tends to be more expensive than offsetting. I don't know how much effect it would have in this case but things sitting in packet in/out paths can be very hot so didn't wanna disturb it.

Quoted 5 lines

Also the algorithm to figure out the size of the array might be a bit too aggressive in my opinion. So you always start at PRIOMAP_MIN_SIZE and then try to double the size until target_idx fits. Wouldn't it make sense to start to look for the new size beginning at old->priomap_len and then do the power-of-two increase?

The only downside of always starting from PRIOMAP_MIN_SIZE is iterating several more times in the sizing loop which isn't really anything to worry about. The loop is structured that way because I wanted to keep the size of the whole thing power-of-two. Due to the fields before priomap[], if we size priomap_len power-of-two, we'll always end up with something slightly over power-of-two, which is usually the worst size to allocate.

netprio kept track of the highest prioidx allocated and resized priomaps accordingly when necessary. This makes it necessary to keep track of prioidx allocation and may end up resizing on every new prioidx.

Update extend_netdev_table() such that it takes @target_idx which the priomap should be able to accomodate. If the priomap is large enough, nothing happens; otherwise, the size is doubled until @target_idx can be accomodated.

This makes max_prioidx and write_update_netdev_table() unnecessary. write_priomap() now calls extend_netdev_table() directly.

Signed-off-by Tejun Heo <[email protected]>

Acked-by Neil Horman <[email protected]>

---
  net/core/netprio_cgroup.c | 56 ++++++++++++++++++++++++++++-------------------
  1 file changed, 33 insertions(+), 23 deletions(-)

net/core/netprio_cgroup.c +31 -8

index 92cc54c..569d83d 100644
@@ -27,11 +27,11 @@
 
  #include <linux/fdtable.h>
 
+#define PRIOMAP_MIN_SZ		128
  #define PRIOIDX_SZ 128
 
  static unsigned long prioidx_map[PRIOIDX_SZ];
  static DEFINE_SPINLOCK(prioidx_map_lock);
-static atomic_t max_prioidx = ATOMIC_INIT(0);
 
  static inline struct cgroup_netprio_state *cgrp_netprio_state(struct cgroup *cgrp)
  {

@@ -51,8 +51,6 @@ static int get_prioidx(u32 *prio)
  		return -ENOSPC;
  	}
  	set_bit(prioidx, prioidx_map);
-	if (atomic_read(&max_prioidx) < prioidx)
-		atomic_set(&max_prioidx, prioidx);
  	spin_unlock_irqrestore(&prioidx_map_lock, flags);
  	*prio = prioidx;
  	return 0;

@@ -67,15 +65,40 @@ static void put_prioidx(u32 idx)
  	spin_unlock_irqrestore(&prioidx_map_lock, flags);
  }
 
-static int extend_netdev_table(struct net_device *dev, u32 new_len)
+/*
+ * Extend @dev->priomap so that it's large enough to accomodate
+ * @target_idx.  @dev->priomap.priomap_len > @target_idx after successful
+ * return.  Must be called under rtnl lock.
+ */
+static int extend_netdev_table(struct net_device *dev, u32 target_idx)
  {
-	size_t new_size = sizeof(struct netprio_map) +
-			   ((sizeof(u32) * new_len));
-	struct netprio_map *new = kzalloc(new_size, GFP_KERNEL);
-	struct netprio_map *old;
+	struct netprio_map *old, *new;
+	size_t new_sz, new_len;
 
+	/* is the existing priomap large enough? */
  	old = rtnl_dereference(dev->priomap);
+	if (old && old->priomap_len > target_idx)
+		return 0;
+
+	/*
+	 * Determine the new size.  Let's keep it power-of-two.  We start
+	 * from PRIOMAP_MIN_SZ and double it until it's large enough to
+	 * accommodate @target_idx.
+	 */
+	new_sz = PRIOMAP_MIN_SZ;
+	while (true) {
+		new_len = (new_sz - offsetof(struct netprio_map, priomap)) /
+			sizeof(new->priomap[0]);
+		if (new_len > target_idx)
+			break;
+		new_sz *= 2;
+		/* overflowed? */
+		if (WARN_ON(new_sz < PRIOMAP_MIN_SZ))
+			return -ENOSPC;
+	}
 
+	/* allocate & copy */
+	new = kzalloc(new_sz, GFP_KERNEL);
  	if (!new) {
  		pr_warn("Unable to alloc new priomap!\n");
  		return -ENOMEM;

With priomap expansion no longer depending on knowing max id allocated, netprio_cgroup can use cgroup->id insted of cs->prioidx. Drop prioidx alloc/free logic and convert all uses to cgroup->id.

* In cgrp_css_alloc(), parent->id test is moved above @cs allocation to simplify error path.

* In cgrp_css_free(), @cs assignment is made initialization.

Signed-off-by Tejun Heo <[email protected]>

Acked-by Neil Horman <[email protected]>

Introduce two helpers - netprio_prio() and netprio_set_prio() - which hide the details of priomap access and expansion. This will help implementing hierarchy support.

Signed-off-by Tejun Heo <[email protected]>

Acked-by Neil Horman <[email protected]>

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 <[email protected]>

---
  Documentation/cgroups/net_prio.txt |  2 ++
  net/core/netprio_cgroup.c          | 42 ++++++++++++++++++++++----------------
  2 files changed, 26 insertions(+), 18 deletions(-)

Documentation/cgroups/net_prio.txt +2 -0

index 01b3226..a82cbd2 100644
@@ -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.

net/core/netprio_cgroup.c +0 -3

index b2af0d0..bde53da 100644
@@ -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;
  }
 

Quoted 5 lines

net/core/netprio_cgroup.c +2 -2

-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;

net/core/netprio_cgroup.c static struct cgroup_subsys_state *cgrp_css_alloc(struct cgroup *cgrp) · line 153

Quoted 5 lines

net/core/netprio_cgroup.c +4 -0

 	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.

Hello, Daniel.

Quoted 5 lines · Daniel Wagner

Quoted 5 lines

net/core/netprio_cgroup.c +2 -2

-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;

Quoted 8 lines

net/core/netprio_cgroup.c static struct cgroup_subsys_state *cgrp_css_alloc(struct cgroup *cgrp) · line 153

Quoted 5 lines

struct net_device *dev;

net/core/netprio_cgroup.c +4 -0

+	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?

0001-netcls_cgroup-move-config-inheritance-to-css_online-.patch 0002-netprio_cgroup-simplify-write_priomap.patch 0003-netprio_cgroup-shorten-variable-names-in-extend_netd.patch 0004-netprio_cgroup-reimplement-priomap-expansion.patch 0005-netprio_cgroup-use-cgroup-id-instead-of-cgroup_netpr.patch 0006-netprio_cgroup-implement-netprio-_set-_prio-helpers.patch 0007-netprio_cgroup-allow-nesting-and-inherit-config-on-c.patch