[net-next-2.6 PATCH 1/3] vlan: adds vlan_dev_select_queue

[net-next-2.6 PATCH 1/3] vlan: adds vlan_dev_select_queue

[Jeff Kirsher]

From: Vasu Dev <vasu.dev@intel.com>

This is required to correctly select vlan tx queue for a driver
supporting multi tx queue with ndo_select_queue implemented since
currently selected vlan tx queue is unaligned to selected queue by
ndo_select_queue.

Unaligned vlan tx queue causes thrash with higher vlan tx lock
contention for least fcoe traffic on ixgbe.

Added vlan_dev_select_queue adds only minimal required code from
dev_pick_tx and preserves queue selection for the case ndo_select_queue
is not implemented.

Also updates vlan real_num_tx_queues in case underlying device
queues has changed.

Signed-off-by: Vasu Dev <vasu.dev@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 net/8021q/vlan.c     |    1 +
 net/8021q/vlan_dev.c |   23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index 4535122..6c70805 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -378,6 +378,7 @@ static void vlan_transfer_features(struct net_device *dev,
 #if defined(CONFIG_FCOE) || defined(CONFIG_FCOE_MODULE)
 	vlandev->fcoe_ddp_xid = dev->fcoe_ddp_xid;
 #endif
+	vlandev->real_num_tx_queues = dev->real_num_tx_queues;
 
 	if (old_features != vlandev->features)
 		netdev_features_change(vlandev);
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 9e83272..7ec246f 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -26,6 +26,7 @@
 #include <linux/etherdevice.h>
 #include <linux/ethtool.h>
 #include <net/arp.h>
+#include <net/sock.h>
 
 #include "vlan.h"
 #include "vlanproc.h"
@@ -361,6 +362,26 @@ static netdev_tx_t vlan_dev_hwaccel_hard_start_xmit(struct sk_buff *skb,
 	return ret;
 }
 
+static u16 vlan_dev_select_queue(struct net_device *dev, struct sk_buff *skb)
+{
+	struct vlan_dev_info *vlan = vlan_dev_info(dev);
+	const struct net_device_ops *ops = vlan->real_dev->netdev_ops;
+	u16 queue_index;
+
+	if (ops->ndo_select_queue)
+		queue_index = ops->ndo_select_queue(vlan->real_dev, skb);
+	else {
+		queue_index = 0;
+		if (dev->real_num_tx_queues > 1)
+			queue_index = skb_tx_hash(dev, skb);
+
+		if (skb->sk && skb->sk->sk_dst_cache)
+			sk_tx_queue_set(skb->sk, queue_index);
+	}
+
+	return queue_index;
+}
+
 static int vlan_dev_change_mtu(struct net_device *dev, int new_mtu)
 {
 	/* TODO: gotta make sure the underlying layer can handle it,
@@ -818,6 +839,7 @@ static const struct ethtool_ops vlan_ethtool_ops = {
 };
 
 static const struct net_device_ops vlan_netdev_ops = {
+	.ndo_select_queue	= vlan_dev_select_queue,
 	.ndo_change_mtu		= vlan_dev_change_mtu,
 	.ndo_init		= vlan_dev_init,
 	.ndo_uninit		= vlan_dev_uninit,
@@ -842,6 +864,7 @@ static const struct net_device_ops vlan_netdev_ops = {
 };
 
 static const struct net_device_ops vlan_netdev_accel_ops = {
+	.ndo_select_queue	= vlan_dev_select_queue,
 	.ndo_change_mtu		= vlan_dev_change_mtu,
 	.ndo_init		= vlan_dev_init,
 	.ndo_uninit		= vlan_dev_uninit,

[net-next-2.6 PATCH 2/3] ixgbe: fix real_num_tx_queues update issue

[Jeff Kirsher]

From: Vasu Dev <vasu.dev@intel.com>

Currently netdev_features_change is called before fcoe tx queues
setup is done, so this patch moves calling of netdev_features_change
after tx queues setup is done in ixgbe_init_interrupt_scheme, so
that real_num_tx_queues is updated correctly on each fcoe enable
and disable.

Signed-off-by: Vasu Dev <vasu.dev@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 drivers/net/ixgbe/ixgbe_fcoe.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_fcoe.c b/drivers/net/ixgbe/ixgbe_fcoe.c
index 4123dec..700cfc0 100644
--- a/drivers/net/ixgbe/ixgbe_fcoe.c
+++ b/drivers/net/ixgbe/ixgbe_fcoe.c
@@ -614,9 +614,9 @@ int ixgbe_fcoe_enable(struct net_device *netdev)
 	netdev->vlan_features |= NETIF_F_FSO;
 	netdev->vlan_features |= NETIF_F_FCOE_MTU;
 	netdev->fcoe_ddp_xid = IXGBE_FCOE_DDP_MAX - 1;
-	netdev_features_change(netdev);
 
 	ixgbe_init_interrupt_scheme(adapter);
+	netdev_features_change(netdev);
 
 	if (netif_running(netdev))
 		netdev->netdev_ops->ndo_open(netdev);
@@ -660,11 +660,11 @@ int ixgbe_fcoe_disable(struct net_device *netdev)
 	netdev->vlan_features &= ~NETIF_F_FSO;
 	netdev->vlan_features &= ~NETIF_F_FCOE_MTU;
 	netdev->fcoe_ddp_xid = 0;
-	netdev_features_change(netdev);
 
 	ixgbe_cleanup_fcoe(adapter);
-
 	ixgbe_init_interrupt_scheme(adapter);
+	netdev_features_change(netdev);
+
 	if (netif_running(netdev))
 		netdev->netdev_ops->ndo_open(netdev);
 	rc = 0;

[net-next-2.6 PATCH 3/3] net: consolidate netif_needs_gso() checks

[Jeff Kirsher]

From: John Fastabend <john.r.fastabend@intel.com>

netif_needs_gso() is checked twice in the TX path once,
before submitting the skb to the qdisc and once after
it is dequeued from the qdisc just before calling
ndo_hard_start().  This opens a window for a user to
change the gso/tso or tx checksum settings that can
cause netif_needs_gso to be true in one check and false
in the other.

Specifically, changing TX checksum setting may cause
the warning in skb_gso_segment() to be triggered if
the checksum is calculated earlier.

This consolidates the netif_needs_gso() calls so that
the stack only checks if gso is needed in
dev_hard_start_xmit().

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 net/core/dev.c |   50 +++++++++++++++++++++++++++++---------------------
 1 files changed, 29 insertions(+), 21 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index d1cf53d..84a0acb 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1835,12 +1835,40 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
 {
 	const struct net_device_ops *ops = dev->netdev_ops;
 	int rc = NETDEV_TX_OK;
+	int need_gso = netif_needs_gso(dev, skb);
+
+	if (!need_gso) {
+		if (skb_has_frags(skb) &&
+		    !(dev->features & NETIF_F_FRAGLIST) &&
+		    __skb_linearize(skb))
+			goto out_kfree_skb;
+
+		/* Fragmented skb is linearized if device does not support SG,
+		 * or if at least one of fragments is in highmem and device
+		 * does not support DMA from it.
+		 */
+		if (skb_shinfo(skb)->nr_frags &&
+		    (!(dev->features & NETIF_F_SG) ||
+		      illegal_highdma(dev, skb)) &&
+		    __skb_linearize(skb))
+			goto out_kfree_skb;
+		/* If packet is not checksummed and device does not support
+		 * checksumming for this protocol, complete checksumming here.
+		 */
+		if (skb->ip_summed == CHECKSUM_PARTIAL) {
+			skb_set_transport_header(skb, skb->csum_start -
+				      skb_headroom(skb));
+			if (!dev_can_checksum(dev, skb) &&
+			     skb_checksum_help(skb))
+				goto out_kfree_skb;
+		}
+	}
 
 	if (likely(!skb->next)) {
 		if (!list_empty(&ptype_all))
 			dev_queue_xmit_nit(skb, dev);
 
-		if (netif_needs_gso(dev, skb)) {
+		if (need_gso) {
 			if (unlikely(dev_gso_segment(skb)))
 				goto out_kfree_skb;
 			if (skb->next)
@@ -2056,25 +2084,6 @@ int dev_queue_xmit(struct sk_buff *skb)
 	struct Qdisc *q;
 	int rc = -ENOMEM;
 
-	/* GSO will handle the following emulations directly. */
-	if (netif_needs_gso(dev, skb))
-		goto gso;
-
-	/* Convert a paged skb to linear, if required */
-	if (skb_needs_linearize(skb, dev) && __skb_linearize(skb))
-		goto out_kfree_skb;
-
-	/* If packet is not checksummed and device does not support
-	 * checksumming for this protocol, complete checksumming here.
-	 */
-	if (skb->ip_summed == CHECKSUM_PARTIAL) {
-		skb_set_transport_header(skb, skb->csum_start -
-					      skb_headroom(skb));
-		if (!dev_can_checksum(dev, skb) && skb_checksum_help(skb))
-			goto out_kfree_skb;
-	}
-
-gso:
 	/* Disable soft irqs for various locks below. Also
 	 * stops preemption for RCU.
 	 */
@@ -2133,7 +2142,6 @@ gso:
 	rc = -ENETDOWN;
 	rcu_read_unlock_bh();
 
-out_kfree_skb:
 	kfree_skb(skb);
 	return rc;
 out:

Re: [net-next-2.6 PATCH 1/3] vlan: adds vlan_dev_select_queue

[Eric Dumazet]

Le mercredi 17 février 2010 à 04:36 -0800, Jeff Kirsher a écrit :
> From: Vasu Dev <vasu.dev@intel.com>
> 
> This is required to correctly select vlan tx queue for a driver
> supporting multi tx queue with ndo_select_queue implemented since
> currently selected vlan tx queue is unaligned to selected queue by
> ndo_select_queue.
> 
> Unaligned vlan tx queue causes thrash with higher vlan tx lock
> contention for least fcoe traffic on ixgbe.
> 
> Added vlan_dev_select_queue adds only minimal required code from
> dev_pick_tx and preserves queue selection for the case ndo_select_queue
> is not implemented.
> 
> Also updates vlan real_num_tx_queues in case underlying device
> queues has changed.
> 
> Signed-off-by: Vasu Dev <vasu.dev@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
> 
>  net/8021q/vlan.c     |    1 +
>  net/8021q/vlan_dev.c |   23 +++++++++++++++++++++++
>  2 files changed, 24 insertions(+), 0 deletions(-)
> 
> diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
> index 4535122..6c70805 100644
> --- a/net/8021q/vlan.c
> +++ b/net/8021q/vlan.c
> @@ -378,6 +378,7 @@ static void vlan_transfer_features(struct net_device *dev,
>  #if defined(CONFIG_FCOE) || defined(CONFIG_FCOE_MODULE)
>  	vlandev->fcoe_ddp_xid = dev->fcoe_ddp_xid;
>  #endif
> +	vlandev->real_num_tx_queues = dev->real_num_tx_queues;

Hi Jeff

This is a bit dangerous and deserves a self contained patch IMHO.

I would add for example a
	BUG_ON(vlandev->real_num_tx_queues > vlandev->num_tx_queues)

------------------

For performance reason, we could avoid calling vlan_dev_select_queue()
for real devices without ndo_select_queue() handler, this would need to
mirror vlan_netdev_ops & vlan_netdev_accel_ops.

It would also make vlan_dev_select_queue shorter and not duplicates
internals of net/dev/core.c :

static u16 vlan_dev_select_queue(struct net_device *dev, struct sk_buff
*skb)
{
       struct net_device *rdev = vlan_dev_info(dev)->real_dev;
       const struct net_device_ops *ops = rdev->netdev_ops;

       return ops->ndo_select_queue(rdev, skb);
}

Re: [net-next-2.6 PATCH 1/3] vlan: adds vlan_dev_select_queue

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 17 Feb 2010 14:51:15 +0100

> This is a bit dangerous and deserves a self contained patch IMHO.
> 
> I would add for example a
> 	BUG_ON(vlandev->real_num_tx_queues > vlandev->num_tx_queues)
> 
> ------------------
> 
> For performance reason, we could avoid calling vlan_dev_select_queue()
> for real devices without ndo_select_queue() handler, this would need to
> mirror vlan_netdev_ops & vlan_netdev_accel_ops.
> 
> It would also make vlan_dev_select_queue shorter and not duplicates
> internals of net/dev/core.c :
> 
> static u16 vlan_dev_select_queue(struct net_device *dev, struct sk_buff
> *skb)
> {
>        struct net_device *rdev = vlan_dev_info(dev)->real_dev;
>        const struct net_device_ops *ops = rdev->netdev_ops;
> 
>        return ops->ndo_select_queue(rdev, skb);
> }
> 
> 

Jeff, please resubmit this patch set after addressing
Eric's feedback.

Thanks!

Re: [net-next-2.6 PATCH 1/3] vlan: adds vlan_dev_select_queue

[Vasu Dev]

On Wed, 2010-02-17 at 13:43 -0800, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, 17 Feb 2010 14:51:15 +0100
> 
> > This is a bit dangerous and deserves a self contained patch IMHO.
> > 

OK, I'll create separate patch for this.
 
> > I would add for example a
> > 	BUG_ON(vlandev->real_num_tx_queues > vlandev->num_tx_queues)
> > 

I'll add this.

> > ------------------
> > 
> > For performance reason, we could avoid calling vlan_dev_select_queue()
> > for real devices without ndo_select_queue() handler, this would need to
> > mirror vlan_netdev_ops & vlan_netdev_accel_ops.
> > 

This means vlan_netdev_ops & vlan_netdev_accel_ops not to be const
anymore so that vlan_dev_select_queue() could be initialized to vlan ops
based on real dev ndo_select_queue() present or not, should be okay to
be not const anymore. I'll update patch as suggested.

> > It would also make vlan_dev_select_queue shorter and not duplicates
> > internals of net/dev/core.c :
> > 

Good reasons.

> > static u16 vlan_dev_select_queue(struct net_device *dev, struct sk_buff
> > *skb)
> > {
> >        struct net_device *rdev = vlan_dev_info(dev)->real_dev;
> >        const struct net_device_ops *ops = rdev->netdev_ops;
> > 
> >        return ops->ndo_select_queue(rdev, skb);
> > }
> > 
> > 
> 
> Jeff, please resubmit this patch set after addressing
> Eric's feedback.
> 

I'm updating patches per Eric's feedback to have Jeff resubmit these
patches.

	Vasu

Re: [net-next-2.6 PATCH 1/3] vlan: adds vlan_dev_select_queue

[Eric Dumazet]

Le mercredi 17 février 2010 à 14:40 -0800, Vasu Dev a écrit :
> > > ------------------
> > > 
> > > For performance reason, we could avoid calling vlan_dev_select_queue()
> > > for real devices without ndo_select_queue() handler, this would need to
> > > mirror vlan_netdev_ops & vlan_netdev_accel_ops.
> > > 
> 
> This means vlan_netdev_ops & vlan_netdev_accel_ops not to be const
> anymore so that vlan_dev_select_queue() could be initialized to vlan ops
> based on real dev ndo_select_queue() present or not, should be okay to
> be not const anymore. I'll update patch as suggested.

No they should stay const

I said you should mirror (copy if you prefer) the structure, one with a
NULL ndo_select_queue pointer, one with a ndo_select_queue =
vlan_dev_select_queue.

Both structures are const.

You must not dynamiucally change the structure, because the same machine
might have a vlan over a device with no ndo_select_queue() method, and
another vlan over a device with a ndo_select_queue()

Thanks

Re: [net-next-2.6 PATCH 1/3] vlan: adds vlan_dev_select_queue

[Vasu Dev]

On Thu, 2010-02-18 at 08:37 +0100, Eric Dumazet wrote:
> > This means vlan_netdev_ops & vlan_netdev_accel_ops not to be const
> > anymore so that vlan_dev_select_queue() could be initialized to vlan
> ops
> > based on real dev ndo_select_queue() present or not, should be okay
> to
> > be not const anymore. I'll update patch as suggested.
> 
> No they should stay const
> 
> I said you should mirror (copy if you prefer) the structure, one with
> a
> NULL ndo_select_queue pointer, one with a ndo_select_queue =
> vlan_dev_select_queue.
> 
> Both structures are const.
> 
> You must not dynamiucally change the structure, because the same
> machine
> might have a vlan over a device with no ndo_select_queue() method, and
> another vlan over a device with a ndo_select_queue()
> 

I see, I'll mirror existing vlan op structs to have them w/ and w/o
vlan_dev_select_queue while having all vlan ops statically defined
const.

	Thanks Eric.
	Vasu