Skip to contentSkip to conversation

macvlan: add multiqueue capability

PATCH Applied Eric Dumazet 3 participants 7 messages Sep 3, 2009 – Sep 4, 2009

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

Eric Dumazet

author
#0 lore raw

PATCH 1/1 macvlan: add multiqueue capability

macvlan devices are currently not multi-queue capable.

We can do that defining rtnl_link_ops method, get_tx_queues(), called from rtnl_create_link()

This new method gets num_tx_queues/real_num_tx_queues from lower device.

macvlan_get_tx_queues() is a copy of vlan_get_tx_queues().

Because macvlan_start_xmit() has to update netdev_queue stats only (and not dev->stats), I chose to change tx_errors/tx_aborted_errors accounting to tx_dropped, since netdev_queue structure doesnt define tx_errors / tx_aborted_errors.

Signed-off-by Eric Dumazet <[email protected]>
---
 drivers/net/macvlan.c |   33 +++++++++++++++++++++++++++------
 1 files changed, 27 insertions(+), 6 deletions(-)
drivers/net/macvlan.c +27 -6
index c85c46d..3aabfd9 100644
@@ -187,6 +187,8 @@ static struct sk_buff *macvlan_handle_frame(struct sk_buff *skb)
 static netdev_tx_t macvlan_start_xmit(struct sk_buff *skb,
 				      struct net_device *dev)
 {
+	int i = skb_get_queue_mapping(skb);
+	struct netdev_queue *txq = netdev_get_tx_queue(dev, i);
 	const struct macvlan_dev *vlan = netdev_priv(dev);
 	unsigned int len = skb->len;
 	int ret;

@@ -195,12 +197,11 @@ static netdev_tx_t macvlan_start_xmit(struct sk_buff *skb,
 	ret = dev_queue_xmit(skb);
 
 	if (likely(ret == NET_XMIT_SUCCESS)) {
-		dev->stats.tx_packets++;
-		dev->stats.tx_bytes += len;
-	} else {
-		dev->stats.tx_errors++;
-		dev->stats.tx_aborted_errors++;
-	}
+		txq->tx_packets++;
+		txq->tx_bytes += len;
+	} else
+		txq->tx_dropped++;
+
 	return NETDEV_TX_OK;
 }
 

@@ -484,6 +485,25 @@ static int macvlan_validate(struct nlattr *tb[], struct nlattr *data[])
 	return 0;
 }
 
+static int macvlan_get_tx_queues(struct net *net,
+				 struct nlattr *tb[],
+				 unsigned int *num_tx_queues,
+				 unsigned int *real_num_tx_queues)
+{
+	struct net_device *real_dev;
+
+	if (!tb[IFLA_LINK])
+		return -EINVAL;
+
+	real_dev = __dev_get_by_index(net, nla_get_u32(tb[IFLA_LINK]));
+	if (!real_dev)
+		return -ENODEV;
+
+	*num_tx_queues      = real_dev->num_tx_queues;
+	*real_num_tx_queues = real_dev->real_num_tx_queues;
+	return 0;
+}
+
 static int macvlan_newlink(struct net_device *dev,
 			   struct nlattr *tb[], struct nlattr *data[])
 {

@@ -550,6 +570,7 @@ static void macvlan_dellink(struct net_device *dev)
 static struct rtnl_link_ops macvlan_link_ops __read_mostly = {
 	.kind		= "macvlan",
 	.priv_size	= sizeof(struct macvlan_dev),
+	.get_tx_queues  = macvlan_get_tx_queues,
 	.setup		= macvlan_setup,
 	.validate	= macvlan_validate,
 	.newlink	= macvlan_newlink,
#1 lore raw
Quoted 8 lines · Eric Dumazet

macvlan devices are currently not multi-queue capable.

We can do that defining rtnl_link_ops method, get_tx_queues(), called from rtnl_create_link()

This new method gets num_tx_queues/real_num_tx_queues from lower device.

Quoted 7 lines

macvlan_get_tx_queues() is a copy of vlan_get_tx_queues().

Because macvlan_start_xmit() has to update netdev_queue stats only (and not dev->stats), I chose to change tx_errors/tx_aborted_errors accounting to tx_dropped, since netdev_queue structure doesnt define tx_errors / tx_aborted_errors.

The patch looks fine, but it just occured to me that this won't have any effect since both VLAN and macvlan use a tx_queue_len of 0, so they will by default have queueing disabled. In fact this will increase costs for the default case since we're now hashing every packet.

author
#2 lore raw
Quoted 17 lines · Patrick McHardy
Quoted 8 lines · Eric Dumazet

macvlan devices are currently not multi-queue capable.

We can do that defining rtnl_link_ops method, get_tx_queues(), called from rtnl_create_link()

This new method gets num_tx_queues/real_num_tx_queues from lower device.

Quoted 7 lines

macvlan_get_tx_queues() is a copy of vlan_get_tx_queues().

Because macvlan_start_xmit() has to update netdev_queue stats only (and not dev->stats), I chose to change tx_errors/tx_aborted_errors accounting to tx_dropped, since netdev_queue structure doesnt define tx_errors / tx_aborted_errors.

Quoted 5 lines

The patch looks fine, but it just occured to me that this won't have any effect since both VLAN and macvlan use a tx_queue_len of 0, so they will by default have queueing disabled. In fact this will increase costs for the default case since we're now hashing every packet.

Good point !

We'll have to hash the packet later when hitting the lowerdevice, which is multiqueue. No ?

Also, what's wrong with

ip link add link eth0 eth0.103 txqueuelen 100 type vlan id 103

;)

#3 lore raw
Quoted 11 lines · Eric Dumazet
Quoted 5 lines · Patrick McHardy

The patch looks fine, but it just occured to me that this won't have any effect since both VLAN and macvlan use a tx_queue_len of 0, so they will by default have queueing disabled. In fact this will increase costs for the default case since we're now hashing every packet.

Good point !

We'll have to hash the packet later when hitting the lowerdevice, which is multiqueue. No ?

Right. But we don't reuse that decision from what I can tell.

Quoted 3 lines

Also, what's wrong with

ip link add link eth0 eth0.103 txqueuelen 100 type vlan id 103

There's nothing wrong, but its kind of pointless since with the default qdisc the queue will be bypassed, other qdiscs are shared between the queues and defeat multiqueue.

I guess it could make sense if you want to apply TC actions or something like that once we support using different (non-shared) qdiscs for each queue.

author
#4 lore raw
In reply to Patrick McHardy
Quoted 17 lines · Patrick McHardy
Quoted 8 lines · Eric Dumazet

macvlan devices are currently not multi-queue capable.

We can do that defining rtnl_link_ops method, get_tx_queues(), called from rtnl_create_link()

This new method gets num_tx_queues/real_num_tx_queues from lower device.

Quoted 7 lines

macvlan_get_tx_queues() is a copy of vlan_get_tx_queues().

Because macvlan_start_xmit() has to update netdev_queue stats only (and not dev->stats), I chose to change tx_errors/tx_aborted_errors accounting to tx_dropped, since netdev_queue structure doesnt define tx_errors / tx_aborted_errors.

Quoted 5 lines

The patch looks fine, but it just occured to me that this won't have any effect since both VLAN and macvlan use a tx_queue_len of 0, so they will by default have queueing disabled. In fact this will increase costs for the default case since we're now hashing every packet.

Just read again dev_queue_xmit(), in case we have no queueing on macvlan/vlan

Having mutiple txq should help multi flow / multi cpus setups, since hashing will provide more chances to hit different txq/locks, and let several cpus run concurrently, each one on a different queue.

So I dont understand why you think it'll increase costs...

#5 lore raw
Quoted 7 lines · Eric Dumazet
Quoted 5 lines · Patrick McHardy

The patch looks fine, but it just occured to me that this won't have any effect since both VLAN and macvlan use a tx_queue_len of 0, so they will by default have queueing disabled. In fact this will increase costs for the default case since we're now hashing every packet.

Quoted 6 lines

Just read again dev_queue_xmit(), in case we have no queueing on macvlan/vlan

Having mutiple txq should help multi flow / multi cpus setups, since hashing will provide more chances to hit different txq/locks, and let several cpus run concurrently, each one on a different queue.

You're right, I missed that we're also perfoming locking in the noqueue case. Sorry :)

maintainer Applied
#6 lore raw
In reply to Eric Dumazet

From: Eric Dumazet <[email protected]>
Date: Thu, 03 Sep 2009 12:11:45 +0200

Quoted 10 lines

macvlan devices are currently not multi-queue capable.

We can do that defining rtnl_link_ops method, get_tx_queues(), called from rtnl_create_link()

This new method gets num_tx_queues/real_num_tx_queues from lower device.

macvlan_get_tx_queues() is a copy of vlan_get_tx_queues().

Quoted 8 lines

Because macvlan_start_xmit() has to update netdev_queue stats only (and not dev->stats), I chose to change tx_errors/tx_aborted_errors accounting to tx_dropped, since netdev_queue structure doesnt define tx_errors / tx_aborted_errors.

Signed-off-by Eric Dumazet <[email protected]>

Applied.