[PATCH net-next-2.6] macvlan: add multiqueue capability

[PATCH net-next-2.6] macvlan: add multiqueue capability

[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.

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 <eric.dumazet@gmail.com>
---
 drivers/net/macvlan.c |   33 +++++++++++++++++++++++++++------
 1 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index c85c46d..3aabfd9 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -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,

Re: [PATCH net-next-2.6] macvlan: add multiqueue capability

[Patrick McHardy]

Eric Dumazet wrote:
> 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.

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.

Re: [PATCH net-next-2.6] macvlan: add multiqueue capability

[Eric Dumazet]

Patrick McHardy a écrit :
> Eric Dumazet wrote:
>> 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.
> 
> 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

;)

Re: [PATCH net-next-2.6] macvlan: add multiqueue capability

[Eric Dumazet]

Patrick McHardy a écrit :
> Eric Dumazet wrote:
>> 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.
> 
> 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...

Re: [PATCH net-next-2.6] macvlan: add multiqueue capability

[Patrick McHardy]

Eric Dumazet wrote:
> Patrick McHardy a écrit :
>> 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.

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

Re: [PATCH net-next-2.6] macvlan: add multiqueue capability

[Patrick McHardy]

Eric Dumazet wrote:
> Patrick McHardy a écrit :
>> 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.

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

Re: [PATCH net-next-2.6] macvlan: add multiqueue capability

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 03 Sep 2009 12:11:45 +0200

> 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 <eric.dumazet@gmail.com>

Applied.