Skip to contentSkip to conversation

rps: add flow director support

PATCH In review Changli Gao 3 participants 7 messages Apr 11, 2010 – Apr 13, 2010

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Changli Gao

author
#0 lore raw

PATCH 1/1 rps: add flow director support

add rps flow director support

with rps flow director, users can do weighted packet dispatching among CPUs. For example, CPU0:CPU1 is 1:3 for eth0's rx-0:

localhost linux # echo 4 > /sys/class/net/eth0/queues/rx-0/rps_flows localhost linux # echo 0 > /sys/class/net/eth0/queues/rx-0/rps_flow_0 localhost linux # echo 1 > /sys/class/net/eth0/queues/rx-0/rps_flow_1 localhost linux # echo 1 > /sys/class/net/eth0/queues/rx-0/rps_flow_2 localhost linux # echo 1 > /sys/class/net/eth0/queues/rx-0/rps_flow_3

Signed-off-by Changli Gao <[email protected]>

----

 net/core/net-sysfs.c |  176 +++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 172 insertions(+), 4 deletions(-)
net/core/net-sysfs.c +172 -4
index 1e7fdd6..d904610 100644
@@ -511,6 +511,109 @@ static struct sysfs_ops rx_queue_sysfs_ops = {
 	.store = rx_queue_attr_store,
 };
 
+static DEFINE_MUTEX(rps_map_lock);
+
+static ssize_t show_rps_flow(struct netdev_rx_queue *queue,
+			     struct rx_queue_attribute *attribute, char *buf)
+{
+	unsigned long flowid;
+	struct rps_map *map;
+	u16 cpu;
+
+	strict_strtoul(attribute->attr.name + strlen("rps_flow_"), 10, &flowid);
+	rcu_read_lock();
+	map = rcu_dereference(queue->rps_map);
+	if (map && flowid < map->len)
+		cpu = map->cpus[flowid];
+	else
+		cpu = 0;
+	rcu_read_unlock();
+	return sprintf(buf, "%hu\n", cpu);
+}
+
+static ssize_t store_rps_flow(struct netdev_rx_queue *queue,
+			      struct rx_queue_attribute *attribute,
+			      const char *buf, size_t len)
+{
+	unsigned long flowid, cpu;
+	struct rps_map *map;
+
+	if (!capable(CAP_NET_ADMIN))
+		return -EPERM;
+
+	if (strict_strtoul(buf, 0, &cpu))
+		return -EINVAL;
+	strict_strtoul(attribute->attr.name + strlen("rps_flow_"), 10, &flowid);
+
+	mutex_lock(&rps_map_lock);
+	map = queue->rps_map;
+	if (map && flowid < map->len)
+		map->cpus[flowid] = cpu;
+	mutex_unlock(&rps_map_lock);
+
+	return len;
+}
+
+static struct rx_queue_attribute **rps_flow_attribute;
+static int rps_flow_attribute_size;
+
+/* must be called with rps_map_lock locked */
+static int update_rps_flow_files(struct kobject *kobj,
+				 struct rps_map *old_map, struct rps_map *map)
+{
+	int i;
+	int old_map_len = old_map ? old_map->len : 0;
+	int map_len = map ? map->len : 0;
+
+	if (old_map_len >= map_len) {
+		for (i = map_len; i < old_map_len; i++)
+			sysfs_remove_file(kobj, &rps_flow_attribute[i]->attr);
+		return 0;
+	}
+
+	if (map_len > rps_flow_attribute_size) {
+		struct rx_queue_attribute **attrs;
+		char name[sizeof("rps_flow_4294967295")];
+		char *pname;
+
+		attrs = krealloc(rps_flow_attribute, map_len * sizeof(void *),
+				 GFP_KERNEL);
+		if (attrs == NULL)
+			return -ENOMEM;
+		rps_flow_attribute = attrs;
+		for (i = rps_flow_attribute_size; i < map_len; i++) {
+			rps_flow_attribute[i] = kmalloc(sizeof(**attrs),
+							GFP_KERNEL);
+			if (rps_flow_attribute[i] == NULL)
+				break;
+			sprintf(name, "rps_flow_%d", i);
+			pname = kstrdup(name, GFP_KERNEL);
+			if (pname == NULL) {
+				kfree(rps_flow_attribute[i]);
+				break;
+			}
+			rps_flow_attribute[i]->attr.name = pname;
+			rps_flow_attribute[i]->attr.mode = S_IRUGO | S_IWUSR;
+			rps_flow_attribute[i]->show = show_rps_flow;
+			rps_flow_attribute[i]->store = store_rps_flow;
+		}
+		rps_flow_attribute_size = i;
+		if (i != map_len)
+			return -ENOMEM;
+	}
+
+	for (i = old_map_len; i < map_len; i++) {
+		if (sysfs_create_file(kobj, &rps_flow_attribute[i]->attr)) {
+			while (--i >= old_map_len)
+				sysfs_remove_file(kobj,
+						  &rps_flow_attribute[i]->attr);
+			return -ENOMEM;
+		}
+	}
+
+	return 0;
+}
+
 static ssize_t show_rps_map(struct netdev_rx_queue *queue,
 			    struct rx_queue_attribute *attribute, char *buf)
 {

@@ -555,7 +658,6 @@ ssize_t store_rps_map(struct netdev_rx_queue *queue,
 	struct rps_map *old_map, *map;
 	cpumask_var_t mask;
 	int err, cpu, i;
-	static DEFINE_SPINLOCK(rps_map_lock);
 
 	if (!capable(CAP_NET_ADMIN))
 		return -EPERM;

@@ -588,10 +690,15 @@ ssize_t store_rps_map(struct netdev_rx_queue *queue,
 		map = NULL;
 	}
 
-	spin_lock(&rps_map_lock);
+	mutex_lock(&rps_map_lock);
 	old_map = queue->rps_map;
-	rcu_assign_pointer(queue->rps_map, map);
-	spin_unlock(&rps_map_lock);
+	err = update_rps_flow_files(&queue->kobj, old_map, map);
+	if (!err)
+		rcu_assign_pointer(queue->rps_map, map);
+	mutex_unlock(&rps_map_lock);
+
+	if (err)
+		return err;
 
 	if (old_map)
 		call_rcu(&old_map->rcu, rps_map_release);

@@ -603,8 +710,69 @@ ssize_t store_rps_map(struct netdev_rx_queue *queue,
 static struct rx_queue_attribute rps_cpus_attribute =
 	__ATTR(rps_cpus, S_IRUGO | S_IWUSR, show_rps_map, store_rps_map);
 
+static ssize_t show_rps_flows(struct netdev_rx_queue *queue,
+		struct rx_queue_attribute *attribute, char *buf)
+{
+	struct rps_map *map;
+	unsigned int len;
+
+	rcu_read_lock();
+	map = rcu_dereference(queue->rps_map);
+	len = map ? map->len : 0;
+	rcu_read_unlock();
+	return sprintf(buf, "%u\n", len);
+}
+
+static ssize_t store_rps_flows(struct netdev_rx_queue *queue,
+			       struct rx_queue_attribute *attribute,
+			       const char *buf, size_t len)
+{
+	struct rps_map *old_map, *map;
+	unsigned long flows;
+	int err;
+
+	if (!capable(CAP_NET_ADMIN))
+		return -EPERM;
+
+	if (strict_strtoul(buf, 0, &flows))
+		return -EINVAL;
+	if (flows != 0) {
+		map = kzalloc(max_t(unsigned, RPS_MAP_SIZE(flows),
+				    L1_CACHE_BYTES), GFP_KERNEL);
+		if (map == NULL)
+			return -ENOMEM;
+		map->len = flows;
+	} else {
+		map = NULL;
+	}
+
+	mutex_lock(&rps_map_lock);
+	old_map = queue->rps_map;
+	err = update_rps_flow_files(&queue->kobj, old_map, map);
+	if (!err) {
+		if (old_map && map)
+			memcpy(map->cpus, old_map->cpus,
+			       sizeof(map->cpus[0]) *
+			       min_t(unsigned int, flows, old_map->len));
+		rcu_assign_pointer(queue->rps_map, map);
+	}
+	mutex_unlock(&rps_map_lock);
+
+	if (err)
+		return err;
+
+	if (old_map)
+		call_rcu(&old_map->rcu, rps_map_release);
+
+	return len;
+}
+
+static struct rx_queue_attribute rps_flows_attribute =
+	__ATTR(rps_flows, S_IRUGO | S_IWUSR, show_rps_flows, store_rps_flows);
+
 static struct attribute *rx_queue_default_attrs[] = {
 	&rps_cpus_attribute.attr,
+	&rps_flows_attribute.attr,
 	NULL
 };
#1 lore raw
Quoted 5 lines · Changli Gao

add rps flow director support

with rps flow director, users can do weighted packet dispatching among CPUs. For example, CPU0:CPU1 is 1:3 for eth0's rx-0:

Quoted 8 lines

localhost linux # echo 4 > /sys/class/net/eth0/queues/rx-0/rps_flows localhost linux # echo 0 > /sys/class/net/eth0/queues/rx-0/rps_flow_0 localhost linux # echo 1 > /sys/class/net/eth0/queues/rx-0/rps_flow_1 localhost linux # echo 1 > /sys/class/net/eth0/queues/rx-0/rps_flow_2 localhost linux # echo 1 > /sys/class/net/eth0/queues/rx-0/rps_flow_3

Signed-off-by Changli Gao <[email protected]>

----

Changli

I am a bit disappointed to find so many bugs in your patch.

I believe this is over engineering at this stage, we yet have to get some benches or real world results.

Plus it conflicts with the much more interesting upcoming stuff (RFS). You name this patch 'flow director', to get our attention, but it's an old idea of you, to get different weights on cpus, that RPS is not yet able to perform.

Maybe this is the reason you forgot to CC Tom Herbert (and me) ?

Consider now :

1) echo 65000 >/sys/class/net/eth0/queues/rx-0/rps_flow_0 possible crash, dereferencing a smaller cpumap.

2) echo 3000000000 >/sys/class/net/eth0/queues/rx-0/rps_flow_0 probable crash, because of overflow in RPS_MAP_SIZE(flows)

3) How can rps_flow_attribute & rps_flow_attribute_size be static (one instance for whole kernel), if your intent is to have a per rxqueue attributes ? (/sys/class/net/eth0/queues/rx-0/ ...). Or the first lines of update_rps_flow_files() are completely wrong...

echo 10 > /sys/class/net/eth0/queues/rx-0/rps_flows echo 2 > /sys/class/net/eth1/queues/rx-0/rps_flows cat /sys/class/net/eth0/queues/rx-0/rps_flow_9

4) Lack of atomic changes of the RPS flows -> many packet reordering can occur.

5) Many possible memory leaks in update_rps_flow_files(), you obviously were very lazy. We try to build a bug-free kernel, not only a 'cool kernel', and if you are lazy, your patches wont be accepted.

Quoted 38 lines
 net/core/net-sysfs.c |  176 +++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 172 insertions(+), 4 deletions(-)
net/core/net-sysfs.c +28 -0
index 1e7fdd6..d904610 100644
@@ -511,6 +511,109 @@ static struct sysfs_ops rx_queue_sysfs_ops = {
 	.store = rx_queue_attr_store,
 };
 
+static DEFINE_MUTEX(rps_map_lock);
+
+static ssize_t show_rps_flow(struct netdev_rx_queue *queue,
+			     struct rx_queue_attribute *attribute, char *buf)
+{
+	unsigned long flowid;
+	struct rps_map *map;
+	u16 cpu;
+
+	strict_strtoul(attribute->attr.name + strlen("rps_flow_"), 10, &flowid);
+	rcu_read_lock();
+	map = rcu_dereference(queue->rps_map);
+	if (map && flowid < map->len)
+		cpu = map->cpus[flowid];
+	else
+		cpu = 0;
+	rcu_read_unlock();
+	return sprintf(buf, "%hu\n", cpu);
+}
+
+static ssize_t store_rps_flow(struct netdev_rx_queue *queue,
+			      struct rx_queue_attribute *attribute,
+			      const char *buf, size_t len)
+{
+	unsigned long flowid, cpu;
+	struct rps_map *map;
+
+	if (!capable(CAP_NET_ADMIN))

net/core/net-sysfs.c static struct sysfs_ops rx_queue_sysfs_ops = { · line 551

Quoted 10 lines
net/core/net-sysfs.c +10 -0
+		return -EPERM;
+
+	if (strict_strtoul(buf, 0, &cpu))
+		return -EINVAL;
+	strict_strtoul(attribute->attr.name + strlen("rps_flow_"), 10, &flowid);
+
+	mutex_lock(&rps_map_lock);
+	map = queue->rps_map;
+	if (map && flowid < map->len)
+		map->cpus[flowid] = cpu;

what can happen is cpu=65000, and NR_CPUS=32 ?

Quoted 9 lines
net/core/net-sysfs.c +9 -0
+	mutex_unlock(&rps_map_lock);
+
+	return len;
+}
+
+static struct rx_queue_attribute **rps_flow_attribute;
+static int rps_flow_attribute_size;
+
+/* must be called with rps_map_lock locked */

net/core/net-sysfs.c static struct sysfs_ops rx_queue_sysfs_ops = { · line 570

Quoted 10 lines
net/core/net-sysfs.c +10 -0
+static int update_rps_flow_files(struct kobject *kobj,
+				 struct rps_map *old_map, struct rps_map *map)
+{
+	int i;
+	int old_map_len = old_map ? old_map->len : 0;
+	int map_len = map ? map->len : 0;
+
+	if (old_map_len >= map_len) {
+		for (i = map_len; i < old_map_len; i++)
+			sysfs_remove_file(kobj, &rps_flow_attribute[i]->attr);

Removing attributes for this rxqueue, while anothe might need them ?

Quoted 29 lines
net/core/net-sysfs.c +29 -0
+		return 0;
+	}
+
+	if (map_len > rps_flow_attribute_size) {
+		struct rx_queue_attribute **attrs;
+		char name[sizeof("rps_flow_4294967295")];
+		char *pname;
+
+		attrs = krealloc(rps_flow_attribute, map_len * sizeof(void *),
+				 GFP_KERNEL);
+		if (attrs == NULL)
+			return -ENOMEM;
+		rps_flow_attribute = attrs;
+		for (i = rps_flow_attribute_size; i < map_len; i++) {
+			rps_flow_attribute[i] = kmalloc(sizeof(**attrs),
+							GFP_KERNEL);
+			if (rps_flow_attribute[i] == NULL)
+				break;
+			sprintf(name, "rps_flow_%d", i);
+			pname = kstrdup(name, GFP_KERNEL);
+			if (pname == NULL) {
+				kfree(rps_flow_attribute[i]);
+				break;
+			}
+			rps_flow_attribute[i]->attr.name = pname;
+			rps_flow_attribute[i]->attr.mode = S_IRUGO | S_IWUSR;
+			rps_flow_attribute[i]->show = show_rps_flow;
+			rps_flow_attribute[i]->store = store_rps_flow;
+		}

net/core/net-sysfs.c static struct sysfs_ops rx_queue_sysfs_ops = { · line 609

Quoted 10 lines
net/core/net-sysfs.c +10 -0
+		rps_flow_attribute_size = i;
+		if (i != map_len)
+			return -ENOMEM;
+	}
+
+	for (i = old_map_len; i < map_len; i++) {
+		if (sysfs_create_file(kobj, &rps_flow_attribute[i]->attr)) {
+			while (--i >= old_map_len)
+				sysfs_remove_file(kobj,
+						  &rps_flow_attribute[i]->attr);

No changes to rps_flow_atribute_size ?

Quoted 57 lines
net/core/net-sysfs.c +31 -4
+			return -ENOMEM;
+		}
+	}
+
+	return 0;
+}
+
 static ssize_t show_rps_map(struct netdev_rx_queue *queue,
 			    struct rx_queue_attribute *attribute, char *buf)
 {

@@ -555,7 +658,6 @@ ssize_t store_rps_map(struct netdev_rx_queue *queue,
 	struct rps_map *old_map, *map;
 	cpumask_var_t mask;
 	int err, cpu, i;
-	static DEFINE_SPINLOCK(rps_map_lock);
 
 	if (!capable(CAP_NET_ADMIN))
 		return -EPERM;

@@ -588,10 +690,15 @@ ssize_t store_rps_map(struct netdev_rx_queue *queue,
 		map = NULL;
 	}
 
-	spin_lock(&rps_map_lock);
+	mutex_lock(&rps_map_lock);
 	old_map = queue->rps_map;
-	rcu_assign_pointer(queue->rps_map, map);
-	spin_unlock(&rps_map_lock);
+	err = update_rps_flow_files(&queue->kobj, old_map, map);
+	if (!err)
+		rcu_assign_pointer(queue->rps_map, map);
+	mutex_unlock(&rps_map_lock);
+
+	if (err)
+		return err;
 
 	if (old_map)
 		call_rcu(&old_map->rcu, rps_map_release);

@@ -603,8 +710,69 @@ ssize_t store_rps_map(struct netdev_rx_queue *queue,
 static struct rx_queue_attribute rps_cpus_attribute =
 	__ATTR(rps_cpus, S_IRUGO | S_IWUSR, show_rps_map, store_rps_map);
 
+static ssize_t show_rps_flows(struct netdev_rx_queue *queue,
+		struct rx_queue_attribute *attribute, char *buf)
+{
+	struct rps_map *map;
+	unsigned int len;
+
+	rcu_read_lock();
+	map = rcu_dereference(queue->rps_map);
+	len = map ? map->len : 0;
+	rcu_read_unlock();
+	return sprintf(buf, "%u\n", len);
+}
+
+static ssize_t store_rps_flows(struct netdev_rx_queue *queue,
+			       struct rx_queue_attribute *attribute,
+			       const char *buf, size_t len)

net/core/net-sysfs.c ssize_t store_rps_map(struct netdev_rx_queue *queue, · line 738

Quoted 10 lines
net/core/net-sysfs.c +10 -0
+{
+	struct rps_map *old_map, *map;
+	unsigned long flows;
+	int err;
+
+	if (!capable(CAP_NET_ADMIN))
+		return -EPERM;
+
+	if (strict_strtoul(buf, 0, &flows))
+		return -EINVAL;

Are you aware RPS_MAP_SIZE(0x80000000) can overflow ?

Quoted 41 lines
net/core/net-sysfs.c +35 -1
+	if (flows != 0) {
+		map = kzalloc(max_t(unsigned, RPS_MAP_SIZE(flows),
+				    L1_CACHE_BYTES), GFP_KERNEL);
+		if (map == NULL)
+			return -ENOMEM;
+		map->len = flows;
+	} else {
+		map = NULL;
+	}
+
+	mutex_lock(&rps_map_lock);
+	old_map = queue->rps_map;
+	err = update_rps_flow_files(&queue->kobj, old_map, map);
+	if (!err) {
+		if (old_map && map)
+			memcpy(map->cpus, old_map->cpus,
+			       sizeof(map->cpus[0]) *
+			       min_t(unsigned int, flows, old_map->len));
+		rcu_assign_pointer(queue->rps_map, map);
+	}
+	mutex_unlock(&rps_map_lock);
+
+	if (err)
+		return err;
+
+	if (old_map)
+		call_rcu(&old_map->rcu, rps_map_release);
+
+	return len;
+}
+
+static struct rx_queue_attribute rps_flows_attribute =
+	__ATTR(rps_flows, S_IRUGO | S_IWUSR, show_rps_flows, store_rps_flows);
+
 static struct attribute *rx_queue_default_attrs[] = {
 	&rps_cpus_attribute.attr,
+	&rps_flows_attribute.attr,
 	NULL
 };
 
--
author
#2 lore raw
Quoted 5 lines · Eric Dumazet

Le lundi 12 avril 2010 à 05:42 +0800, Changli Gao a écrit :

Changli

I am a bit disappointed to find so many bugs in your patch.

:(. Thanks for your patiently review.

Quoted 3 lines

I believe this is over engineering at this stage, we yet have to get some benches or real world results.

OK. Leave this patch here for testing. And I'll post benchmarking after I do.

Quoted 6 lines

Plus it conflicts with the much more interesting upcoming stuff (RFS). You name this patch 'flow director', to get our attention, but it's an old idea of you, to get different weights on cpus, that RPS is not yet able to perform.

I don't think it conflicts with RFS. RFS is for applications, and RPS flow director is for firewalls and routers. Because softirqs aren't under scheduler control, in order to do softirq load balancing, we have to scheduler flows internally. At the same time, RPS flow director exposes more than the old design, and keeps the interface rps_cpus there.

Quoted 2 lines

Maybe this is the reason you forgot to CC Tom Herbert (and me) ?

Sorry for forgetting adding you and Tom to CC list.

Quoted 4 lines

Consider now :

1) echo 65000 >/sys/class/net/eth0/queues/rx-0/rps_flow_0 possible crash, dereferencing a smaller cpumap.

Yea, I supposed cpu_online() would check cpu was valid or not. After checking the code, I found that you were right. OK, I'll add this sanity check.

if (cpu >= nr_cpumask_bits)

      return -EINVAL;
Quoted 3 lines

2) echo 3000000000 >/sys/class/net/eth0/queues/rx-0/rps_flow_0 probable crash, because of overflow in RPS_MAP_SIZE(flows)

did you mean rps_flows? Yea, RPS_MAP_SIZE may overflow. Maybe I need check the upper boundary.

if (flows > USHORT_MAX)

     return -EINVAL;
Quoted 9 lines

3) How can rps_flow_attribute & rps_flow_attribute_size be static (one instance for whole kernel), if your intent is to have a per rxqueue attributes ? (/sys/class/net/eth0/queues/rx-0/ ...). Or the first lines of update_rps_flow_files() are completely wrong...

echo 10 > /sys/class/net/eth0/queues/rx-0/rps_flows echo 2 > /sys/class/net/eth1/queues/rx-0/rps_flows cat /sys/class/net/eth0/queues/rx-0/rps_flow_9

Yea, each rxqueue has his own attributes. rps_flow_attribute is much like rps_cpus_attribute. As sysfs_create_file() and sysfs_remove_file() don't modify them, and only use them constantly, we can make them static for all the rxqueues.

Quoted 3 lines

4) Lack of atomic changes of the RPS flows -> many packet reordering can occur.

Yea, if you do packet dispatching dynamically. I don't know how to avoid packet reordering totally. If OOO doesn't occur frequently, it isn't a real problem.

Quoted 5 lines

5) Many possible memory leaks in update_rps_flow_files(), you obviously were very lazy. We try to build a bug-free kernel, not only a 'cool kernel', and if you are lazy, your patches wont be accepted.

Yea, I was lazy, and didn't shrink the rps_flow_attribute. I'll add reference counters for rps_flow_attributes to trace them usage, and free them when they aren't needed.

Signature · 2 lines
Regards，
Changli Gao([email protected])
#3 lore raw
In reply to Changli Gao
Quoted 5 lines · Changli Gao

add rps flow director support

with rps flow director, users can do weighted packet dispatching among CPUs. For example, CPU0:CPU1 is 1:3 for eth0's rx-0:

"Flow director" is a misnomer here in that it has no per flow awareness, that is what RFS provides. Please use a different name.

Quoted 6 lines

 localhost linux # echo 4 > /sys/class/net/eth0/queues/rx-0/rps_flows localhost linux # echo 0 > /sys/class/net/eth0/queues/rx-0/rps_flow_0 localhost linux # echo 1 > /sys/class/net/eth0/queues/rx-0/rps_flow_1 localhost linux # echo 1 > /sys/class/net/eth0/queues/rx-0/rps_flow_2 localhost linux # echo 1 > /sys/class/net/eth0/queues/rx-0/rps_flow_3

It might be better to put this in its own directory and also do it per CPU instead of hash entry. This should result in a lot fewer entries and I'm not sure how you would deal with holes in the hash table for unspecified entries. Also, it would be nice not to have to specify a number of entries. Maybe something like:

localhost linux # echo 1 > /sys/class/net/eth0/queues/rx-0/rps_cpu_map/0 localhost linux # echo 3 > /sys/class/net/eth0/queues/rx-0/rps_cpu_map/1

To specify CPU 0 with weight 1, CPU 1 with weight 3.

Quoted 220 lines
Signed-off-by Changli Gao <[email protected]>

----
 net/core/net-sysfs.c |  176 +++++++++++++++++++++++++++++++++++++++++++++++++--

 1 file changed, 172 insertions(+), 4 deletions(-)
net/core/net-sysfs.c +111 -4
index 1e7fdd6..d904610 100644
@@ -511,6 +511,109 @@ static struct sysfs_ops rx_queue_sysfs_ops = {
        .store = rx_queue_attr_store,
  };
 
+static DEFINE_MUTEX(rps_map_lock);
+
+static ssize_t show_rps_flow(struct netdev_rx_queue *queue,
+                            struct rx_queue_attribute *attribute, char *buf)
+{
+       unsigned long flowid;
+       struct rps_map *map;
+       u16 cpu;
+
+       strict_strtoul(attribute->attr.name + strlen("rps_flow_"), 10, &flowid);
+       rcu_read_lock();
+       map = rcu_dereference(queue->rps_map);
+       if (map && flowid < map->len)
+               cpu = map->cpus[flowid];
+       else
+               cpu = 0;
+       rcu_read_unlock();
+       return sprintf(buf, "%hu\n", cpu);
+}
+
+static ssize_t store_rps_flow(struct netdev_rx_queue *queue,
+                             struct rx_queue_attribute *attribute,
+                             const char *buf, size_t len)
+{
+       unsigned long flowid, cpu;
+       struct rps_map *map;
+
+       if (!capable(CAP_NET_ADMIN))
+               return -EPERM;
+
+       if (strict_strtoul(buf, 0, &cpu))
+               return -EINVAL;
+       strict_strtoul(attribute->attr.name + strlen("rps_flow_"), 10, &flowid);
+
+       mutex_lock(&rps_map_lock);
+       map = queue->rps_map;
+       if (map && flowid < map->len)
+               map->cpus[flowid] = cpu;
+       mutex_unlock(&rps_map_lock);
+
+       return len;
+}
+
+static struct rx_queue_attribute **rps_flow_attribute;
+static int rps_flow_attribute_size;
+
+/* must be called with rps_map_lock locked */
+static int update_rps_flow_files(struct kobject *kobj,
+                                struct rps_map *old_map, struct rps_map *map)
+{
+       int i;
+       int old_map_len = old_map ? old_map->len : 0;
+       int map_len = map ? map->len : 0;
+
+       if (old_map_len >= map_len) {
+               for (i = map_len; i < old_map_len; i++)
+                       sysfs_remove_file(kobj, &rps_flow_attribute[i]->attr);
+               return 0;
+       }
+
+       if (map_len > rps_flow_attribute_size) {
+               struct rx_queue_attribute **attrs;
+               char name[sizeof("rps_flow_4294967295")];
+               char *pname;
+
+               attrs = krealloc(rps_flow_attribute, map_len * sizeof(void *),
+                                GFP_KERNEL);
+               if (attrs == NULL)
+                       return -ENOMEM;
+               rps_flow_attribute = attrs;
+               for (i = rps_flow_attribute_size; i < map_len; i++) {
+                       rps_flow_attribute[i] = kmalloc(sizeof(**attrs),
+                                                       GFP_KERNEL);
+                       if (rps_flow_attribute[i] == NULL)
+                               break;
+                       sprintf(name, "rps_flow_%d", i);
+                       pname = kstrdup(name, GFP_KERNEL);
+                       if (pname == NULL) {
+                               kfree(rps_flow_attribute[i]);
+                               break;
+                       }
+                       rps_flow_attribute[i]->attr.name = pname;
+                       rps_flow_attribute[i]->attr.mode = S_IRUGO | S_IWUSR;
+                       rps_flow_attribute[i]->show = show_rps_flow;
+                       rps_flow_attribute[i]->store = store_rps_flow;
+               }
+               rps_flow_attribute_size = i;
+               if (i != map_len)
+                       return -ENOMEM;
+       }
+
+       for (i = old_map_len; i < map_len; i++) {
+               if (sysfs_create_file(kobj, &rps_flow_attribute[i]->attr)) {
+                       while (--i >= old_map_len)
+                               sysfs_remove_file(kobj,
+                                                 &rps_flow_attribute[i]->attr);
+                       return -ENOMEM;
+               }
+       }
+
+       return 0;
+}
+
  static ssize_t show_rps_map(struct netdev_rx_queue *queue,
                            struct rx_queue_attribute *attribute, char *buf)
  {

@@ -555,7 +658,6 @@ ssize_t store_rps_map(struct netdev_rx_queue *queue,
        struct rps_map *old_map, *map;
        cpumask_var_t mask;
        int err, cpu, i;
-       static DEFINE_SPINLOCK(rps_map_lock);
 
        if (!capable(CAP_NET_ADMIN))
                return -EPERM;

@@ -588,10 +690,15 @@ ssize_t store_rps_map(struct netdev_rx_queue *queue,
                map = NULL;
        }
 
-       spin_lock(&rps_map_lock);
+       mutex_lock(&rps_map_lock);
        old_map = queue->rps_map;
-       rcu_assign_pointer(queue->rps_map, map);
-       spin_unlock(&rps_map_lock);
+       err = update_rps_flow_files(&queue->kobj, old_map, map);
+       if (!err)
+               rcu_assign_pointer(queue->rps_map, map);
+       mutex_unlock(&rps_map_lock);
+
+       if (err)
+               return err;
 
        if (old_map)
                call_rcu(&old_map->rcu, rps_map_release);

@@ -603,8 +710,69 @@ ssize_t store_rps_map(struct netdev_rx_queue *queue,

 static struct rx_queue_attribute rps_cpus_attribute = __ATTR(rps_cpus, S_IRUGO | S_IWUSR, show_rps_map, store_rps_map);

net/core/net-sysfs.c +60 -0
+static ssize_t show_rps_flows(struct netdev_rx_queue *queue,
+               struct rx_queue_attribute *attribute, char *buf)
+{
+       struct rps_map *map;
+       unsigned int len;
+
+       rcu_read_lock();
+       map = rcu_dereference(queue->rps_map);
+       len = map ? map->len : 0;
+       rcu_read_unlock();
+       return sprintf(buf, "%u\n", len);
+}
+
+static ssize_t store_rps_flows(struct netdev_rx_queue *queue,
+                              struct rx_queue_attribute *attribute,
+                              const char *buf, size_t len)
+{
+       struct rps_map *old_map, *map;
+       unsigned long flows;
+       int err;
+
+       if (!capable(CAP_NET_ADMIN))
+               return -EPERM;
+
+       if (strict_strtoul(buf, 0, &flows))
+               return -EINVAL;
+       if (flows != 0) {
+               map = kzalloc(max_t(unsigned, RPS_MAP_SIZE(flows),
+                                   L1_CACHE_BYTES), GFP_KERNEL);
+               if (map == NULL)
+                       return -ENOMEM;
+               map->len = flows;
+       } else {
+               map = NULL;
+       }
+
+       mutex_lock(&rps_map_lock);
+       old_map = queue->rps_map;
+       err = update_rps_flow_files(&queue->kobj, old_map, map);
+       if (!err) {
+               if (old_map && map)
+                       memcpy(map->cpus, old_map->cpus,
+                              sizeof(map->cpus[0]) *
+                              min_t(unsigned int, flows, old_map->len));
+               rcu_assign_pointer(queue->rps_map, map);
+       }
+       mutex_unlock(&rps_map_lock);
+
+       if (err)
+               return err;
+
+       if (old_map)
+               call_rcu(&old_map->rcu, rps_map_release);
+
+       return len;
+}
+
+static struct rx_queue_attribute rps_flows_attribute =
+       __ATTR(rps_flows, S_IRUGO | S_IWUSR, show_rps_flows, store_rps_flows);
+

 static struct attribute *rx_queue_default_attrs[] = { &rps_cpus_attribute.attr,
+       &rps_flows_attribute.attr,
       NULL
 };

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

author
#4 lore raw
Quoted 8 lines · Tom Herbert
Quoted 5 lines · Changli Gao

add rps flow director support

with rps flow director, users can do weighted packet dispatching among CPUs. For example, CPU0:CPU1 is 1:3 for eth0's rx-0:

"Flow director" is a misnomer here in that it has no per flow awareness, that is what RFS provides.  Please use a different name.

Flow here is a bundle of flow, not the original meaning. How about "rps_buckets" and "rps_bucket_x"?

Quoted 8 lines
Quoted 7 lines

 localhost linux # echo 4 > /sys/class/net/eth0/queues/rx-0/rps_flows localhost linux # echo 0 > /sys/class/net/eth0/queues/rx-0/rps_flow_0 localhost linux # echo 1 > /sys/class/net/eth0/queues/rx-0/rps_flow_1 localhost linux # echo 1 > /sys/class/net/eth0/queues/rx-0/rps_flow_2 localhost linux # echo 1 > /sys/class/net/eth0/queues/rx-0/rps_flow_3

It might be better to put this in its own directory

I have thought that before, but since they control the same data in kernel as rps_cpus does, I put them in the same directory.

Quoted 11 lines

and also do it per
CPU instead of hash entry.  This should result in a lot fewer entries and I'm not sure how you would deal with holes in the hash table for unspecified entries.  Also, it would be nice not to have to specify a number of entries.  Maybe something like:

localhost linux # echo 1 > /sys/class/net/eth0/queues/rx-0/rps_cpu_map/0 localhost linux # echo 3 > /sys/class/net/eth0/queues/rx-0/rps_cpu_map/1

To specify CPU 0 with weight 1, CPU 1 with weight 3.

Your way is more simple and straightforward. My idea has it own advantage:
1. control the rate precision through rps_flows.
2. do dynamic weighted packet dispatching by migrating some flows from some CPUs to other CPUs. During this operations, only the flows migrated are affected, and OOO only occurs in these flows.

Signature · 2 lines
Regards，
Changli Gao([email protected])
#5 lore raw
Quoted 4 lines · Changli Gao
Quoted 3 lines · Tom Herbert
Quoted 2 lines · Changli Gao

add rps flow director support

Quoted 9 lines
Quoted 5 lines
Quoted 3 lines

with rps flow director, users can do weighted packet dispatching among CPUs. For example, CPU0:CPU1 is 1:3 for eth0's rx-0:

"Flow director" is a misnomer here in that it has no per flow awareness, that is what RFS provides.  Please use a different name.

Flow here is a bundle of flow, not the original meaning. How about "rps_buckets" and "rps_bucket_x"?

Ideally, this should replace rps_cpus if it's a better interface.... right now these would be conflicting interfaces.

Quoted 21 lines
Quoted 8 lines
Quoted 7 lines

 localhost linux # echo 4 > /sys/class/net/eth0/queues/rx-0/rps_flows localhost linux # echo 0 > /sys/class/net/eth0/queues/rx-0/rps_flow_0 localhost linux # echo 1 > /sys/class/net/eth0/queues/rx-0/rps_flow_1 localhost linux # echo 1 > /sys/class/net/eth0/queues/rx-0/rps_flow_2 localhost linux # echo 1 > /sys/class/net/eth0/queues/rx-0/rps_flow_3

It might be better to put this in its own directory

I have thought that before, but since they control the same data in kernel as rps_cpus does, I put them in the same directory.

Quoted 9 lines

and also do it per
CPU instead of hash entry.  This should result in a lot fewer entries and I'm not sure how you would deal with holes in the hash table for unspecified entries.  Also, it would be nice not to have to specify a number of entries.  Maybe something like:

localhost linux # echo 1 > /sys/class/net/eth0/queues/rx-0/rps_cpu_map/0 localhost linux # echo 3 > /sys/class/net/eth0/queues/rx-0/rps_cpu_map/1

Quoted 8 lines
Quoted 2 lines

To specify CPU 0 with weight 1, CPU 1 with weight 3.

Your way is more simple and straightforward. My idea has it own advantage:
1. control the rate precision through rps_flows.
2. do dynamic weighted packet dispatching by migrating some flows from some CPUs to other CPUs. During this operations, only the flows migrated are affected, and OOO only occurs in these flows.

It's probably a little more work, but the CPU->weight mappings could be implemented to cause minimal disruption in the rps_map. Also, if OOO is an issue, then the mitigation technique in RFS could be applied (this will work best when hash table is larger I believe).

Tom

Quoted 5 lines

--
Regards，
Changli Gao([email protected])

author
#6 lore raw
Quoted 7 lines · Tom Herbert
Quoted 2 lines · Changli Gao

On Mon, Apr 12, 2010 at 9:34 PM, Tom Herbert <[email protected]> wrote:

Ideally, this should replace rps_cpus if it's a better interface.... right now these would be conflicting interfaces.

How about sw-rxs and sw-rx-$ (SoftWare Receive queue). It is a software emulation of hardware receive queue, as softnet to NIC. sw-rx-$ is equivalent of /proc/irq/$/smp_affinity, only cpuid is instead of cpumask.

Does anyone support this interface replacing the current rps_cpus? I do. :)

Quoted 6 lines

It's probably a little more work, but the CPU->weight mappings could be implemented to cause minimal disruption in the rps_map.  Also, if OOO is an issue, then the mitigation technique in RFS could be applied (this will work best when hash table is larger I believe).

I am thinking about the cost of keeping packets in order. Is it really worth for this kind of random migration?

Signature · 2 lines
Regards，
Changli Gao([email protected])