| Message ID | 4A9F9661.7020301@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- 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
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 ;) -- 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
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... -- 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
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. -- 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
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 :) -- 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
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. -- 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
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,
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(-) -- 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