[RFC][net-next-2.6 PATCH 1/4] net: consolidate 8021q tagging

[RFC][net-next-2.6 PATCH 1/4] net: consolidate 8021q tagging

[John Fastabend]

Now that VLAN packets are tagged in dev_hard_start_xmit()
at the bottom of the stack we no longer need to tag them
in the 8021Q module (Except in the !VLAN_FLAG_REORDER_HDR
case).

This allows the accel path and non accel paths to be consolidated.
Here the vlan_tci in the skb is always set and we allow the
stack to add the actual tag in dev_hard_start_xmit().

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---

 net/8021q/vlan_dev.c |  105 +++-----------------------------------------------
 1 files changed, 7 insertions(+), 98 deletions(-)

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 14e3d1f..78b1618 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -326,24 +326,12 @@ static netdev_tx_t vlan_dev_hard_start_xmit(struct sk_buff *skb,
 	 */
 	if (veth->h_vlan_proto != htons(ETH_P_8021Q) ||
 	    vlan_dev_info(dev)->flags & VLAN_FLAG_REORDER_HDR) {
-		unsigned int orig_headroom = skb_headroom(skb);
 		u16 vlan_tci;
-
-		vlan_dev_info(dev)->cnt_encap_on_xmit++;
-
 		vlan_tci = vlan_dev_info(dev)->vlan_id;
 		vlan_tci |= vlan_dev_get_egress_qos_mask(dev, skb);
-		skb = __vlan_put_tag(skb, vlan_tci);
-		if (!skb) {
-			txq->tx_dropped++;
-			return NETDEV_TX_OK;
-		}
-
-		if (orig_headroom < VLAN_HLEN)
-			vlan_dev_info(dev)->cnt_inc_headroom_on_tx++;
+		skb = __vlan_hwaccel_put_tag(skb, vlan_tci);
 	}
 
-
 	skb_set_dev(skb, vlan_dev_info(dev)->real_dev);
 	len = skb->len;
 	ret = dev_queue_xmit(skb);
@@ -357,32 +345,6 @@ static netdev_tx_t vlan_dev_hard_start_xmit(struct sk_buff *skb,
 	return ret;
 }
 
-static netdev_tx_t vlan_dev_hwaccel_hard_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);
-	u16 vlan_tci;
-	unsigned int len;
-	int ret;
-
-	vlan_tci = vlan_dev_info(dev)->vlan_id;
-	vlan_tci |= vlan_dev_get_egress_qos_mask(dev, skb);
-	skb = __vlan_hwaccel_put_tag(skb, vlan_tci);
-
-	skb->dev = vlan_dev_info(dev)->real_dev;
-	len = skb->len;
-	ret = dev_queue_xmit(skb);
-
-	if (likely(ret == NET_XMIT_SUCCESS || ret == NET_XMIT_CN)) {
-		txq->tx_packets++;
-		txq->tx_bytes += len;
-	} else
-		txq->tx_dropped++;
-
-	return ret;
-}
-
 static u16 vlan_dev_select_queue(struct net_device *dev, struct sk_buff *skb)
 {
 	struct net_device *rdev = vlan_dev_info(dev)->real_dev;
@@ -719,8 +681,7 @@ static const struct header_ops vlan_header_ops = {
 	.parse	 = eth_header_parse,
 };
 
-static const struct net_device_ops vlan_netdev_ops, vlan_netdev_accel_ops,
-		    vlan_netdev_ops_sq, vlan_netdev_accel_ops_sq;
+static const struct net_device_ops vlan_netdev_ops, vlan_netdev_ops_sq;
 
 static int vlan_dev_init(struct net_device *dev)
 {
@@ -755,19 +716,16 @@ static int vlan_dev_init(struct net_device *dev)
 	if (real_dev->features & NETIF_F_HW_VLAN_TX) {
 		dev->header_ops      = real_dev->header_ops;
 		dev->hard_header_len = real_dev->hard_header_len;
-		if (real_dev->netdev_ops->ndo_select_queue)
-			dev->netdev_ops = &vlan_netdev_accel_ops_sq;
-		else
-			dev->netdev_ops = &vlan_netdev_accel_ops;
 	} else {
 		dev->header_ops      = &vlan_header_ops;
 		dev->hard_header_len = real_dev->hard_header_len + VLAN_HLEN;
-		if (real_dev->netdev_ops->ndo_select_queue)
-			dev->netdev_ops = &vlan_netdev_ops_sq;
-		else
-			dev->netdev_ops = &vlan_netdev_ops;
 	}
 
+	if (real_dev->netdev_ops->ndo_select_queue)
+		dev->netdev_ops = &vlan_netdev_ops_sq;
+	else
+		dev->netdev_ops = &vlan_netdev_ops;
+
 	if (is_vlan_dev(real_dev))
 		subclass = 1;
 
@@ -908,30 +866,6 @@ static const struct net_device_ops vlan_netdev_ops = {
 #endif
 };
 
-static const struct net_device_ops vlan_netdev_accel_ops = {
-	.ndo_change_mtu		= vlan_dev_change_mtu,
-	.ndo_init		= vlan_dev_init,
-	.ndo_uninit		= vlan_dev_uninit,
-	.ndo_open		= vlan_dev_open,
-	.ndo_stop		= vlan_dev_stop,
-	.ndo_start_xmit =  vlan_dev_hwaccel_hard_start_xmit,
-	.ndo_validate_addr	= eth_validate_addr,
-	.ndo_set_mac_address	= vlan_dev_set_mac_address,
-	.ndo_set_rx_mode	= vlan_dev_set_rx_mode,
-	.ndo_set_multicast_list	= vlan_dev_set_rx_mode,
-	.ndo_change_rx_flags	= vlan_dev_change_rx_flags,
-	.ndo_do_ioctl		= vlan_dev_ioctl,
-	.ndo_neigh_setup	= vlan_dev_neigh_setup,
-	.ndo_get_stats64	= vlan_dev_get_stats64,
-#if defined(CONFIG_FCOE) || defined(CONFIG_FCOE_MODULE)
-	.ndo_fcoe_ddp_setup	= vlan_dev_fcoe_ddp_setup,
-	.ndo_fcoe_ddp_done	= vlan_dev_fcoe_ddp_done,
-	.ndo_fcoe_enable	= vlan_dev_fcoe_enable,
-	.ndo_fcoe_disable	= vlan_dev_fcoe_disable,
-	.ndo_fcoe_get_wwn	= vlan_dev_fcoe_get_wwn,
-#endif
-};
-
 static const struct net_device_ops vlan_netdev_ops_sq = {
 	.ndo_select_queue	= vlan_dev_select_queue,
 	.ndo_change_mtu		= vlan_dev_change_mtu,
@@ -957,31 +891,6 @@ static const struct net_device_ops vlan_netdev_ops_sq = {
 #endif
 };
 
-static const struct net_device_ops vlan_netdev_accel_ops_sq = {
-	.ndo_select_queue	= vlan_dev_select_queue,
-	.ndo_change_mtu		= vlan_dev_change_mtu,
-	.ndo_init		= vlan_dev_init,
-	.ndo_uninit		= vlan_dev_uninit,
-	.ndo_open		= vlan_dev_open,
-	.ndo_stop		= vlan_dev_stop,
-	.ndo_start_xmit =  vlan_dev_hwaccel_hard_start_xmit,
-	.ndo_validate_addr	= eth_validate_addr,
-	.ndo_set_mac_address	= vlan_dev_set_mac_address,
-	.ndo_set_rx_mode	= vlan_dev_set_rx_mode,
-	.ndo_set_multicast_list	= vlan_dev_set_rx_mode,
-	.ndo_change_rx_flags	= vlan_dev_change_rx_flags,
-	.ndo_do_ioctl		= vlan_dev_ioctl,
-	.ndo_neigh_setup	= vlan_dev_neigh_setup,
-	.ndo_get_stats64	= vlan_dev_get_stats64,
-#if defined(CONFIG_FCOE) || defined(CONFIG_FCOE_MODULE)
-	.ndo_fcoe_ddp_setup	= vlan_dev_fcoe_ddp_setup,
-	.ndo_fcoe_ddp_done	= vlan_dev_fcoe_ddp_done,
-	.ndo_fcoe_enable	= vlan_dev_fcoe_enable,
-	.ndo_fcoe_disable	= vlan_dev_fcoe_disable,
-	.ndo_fcoe_get_wwn	= vlan_dev_fcoe_get_wwn,
-#endif
-};
-
 void vlan_setup(struct net_device *dev)
 {
 	ether_setup(dev);

[RFC][net-next-2.6 PATCH 2/4] net: 8021Q consolidate header_ops routines

[John Fastabend]

The only thing the 8021Q header ops routines are required
for is the VLAN_FLAG_REORDER_HDR otherwise by the time
the VLAN tag has been added the packet is already on
its way down the stack. In this case using the Ethernet
ops works OK.

At present the VLAN_FLAG_REORDER_HDR flag does not work
with vlan offloads. As I understand the flag the intent
is to allow taps on the vlan device and possibly the
QOS layer to see the vlan tag info.

By inserting the tag in vlan_tci any taps or QOS policies
should be able to retrieve the vlan info. This allows
the flag to work the same in both the offload case and
non-offloaded case. And allows us to use the underlying
ethernet ops.

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---

 net/8021q/vlan_dev.c |   83 +++++++++++++-------------------------------------
 1 files changed, 21 insertions(+), 62 deletions(-)

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 78b1618..1645c3c 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -32,39 +32,6 @@
 #include "vlanproc.h"
 #include <linux/if_vlan.h>
 
-/*
- *	Rebuild the Ethernet MAC header. This is called after an ARP
- *	(or in future other address resolution) has completed on this
- *	sk_buff. We now let ARP fill in the other fields.
- *
- *	This routine CANNOT use cached dst->neigh!
- *	Really, it is used only when dst->neigh is wrong.
- *
- * TODO:  This needs a checkup, I'm ignorant here. --BLG
- */
-static int vlan_dev_rebuild_header(struct sk_buff *skb)
-{
-	struct net_device *dev = skb->dev;
-	struct vlan_ethhdr *veth = (struct vlan_ethhdr *)(skb->data);
-
-	switch (veth->h_vlan_encapsulated_proto) {
-#ifdef CONFIG_INET
-	case htons(ETH_P_IP):
-
-		/* TODO:  Confirm this will work with VLAN headers... */
-		return arp_find(veth->h_dest, skb);
-#endif
-	default:
-		pr_debug("%s: unable to resolve type %X addresses.\n",
-			 dev->name, ntohs(veth->h_vlan_encapsulated_proto));
-
-		memcpy(veth->h_source, dev->dev_addr, ETH_ALEN);
-		break;
-	}
-
-	return 0;
-}
-
 static inline struct sk_buff *vlan_check_reorder_header(struct sk_buff *skb)
 {
 	if (vlan_dev_info(skb->dev)->flags & VLAN_FLAG_REORDER_HDR) {
@@ -269,33 +236,26 @@ static int vlan_dev_hard_header(struct sk_buff *skb, struct net_device *dev,
 				const void *daddr, const void *saddr,
 				unsigned int len)
 {
-	struct vlan_hdr *vhdr;
-	unsigned int vhdrlen = 0;
-	u16 vlan_tci = 0;
 	int rc;
 
 	if (WARN_ON(skb_headroom(skb) < dev->hard_header_len))
 		return -ENOSPC;
 
+	/* When this flag is not set we make the vlan_tci visible
+	 * by setting the skb field.
+	 *
+	 * We do not immediately insert the tag here the intent
+	 * of setting VLAN_FLAG_REORDER_HDR is to make the vlan
+	 * info avaiable to tap devices and the QOS layer. Adding
+	 * the tag present bit shoould be enough for other layers
+	 * to learn the vlan tag.
+	 */
 	if (!(vlan_dev_info(dev)->flags & VLAN_FLAG_REORDER_HDR)) {
-		vhdr = (struct vlan_hdr *) skb_push(skb, VLAN_HLEN);
+		u16 vlan_tci = 0;
 
 		vlan_tci = vlan_dev_info(dev)->vlan_id;
 		vlan_tci |= vlan_dev_get_egress_qos_mask(dev, skb);
-		vhdr->h_vlan_TCI = htons(vlan_tci);
-
-		/*
-		 *  Set the protocol type. For a packet of type ETH_P_802_3/2 we
-		 *  put the length in here instead.
-		 */
-		if (type != ETH_P_802_3 && type != ETH_P_802_2)
-			vhdr->h_vlan_encapsulated_proto = htons(type);
-		else
-			vhdr->h_vlan_encapsulated_proto = htons(len);
-
-		skb->protocol = htons(ETH_P_8021Q);
-		type = ETH_P_8021Q;
-		vhdrlen = VLAN_HLEN;
+		skb = __vlan_hwaccel_put_tag(skb, vlan_tci);
 	}
 
 	/* Before delegating work to the lower layer, enter our MAC-address */
@@ -304,9 +264,7 @@ static int vlan_dev_hard_header(struct sk_buff *skb, struct net_device *dev,
 
 	/* Now make the underlying real hard header */
 	dev = vlan_dev_info(dev)->real_dev;
-	rc = dev_hard_header(skb, dev, type, daddr, saddr, len + vhdrlen);
-	if (rc > 0)
-		rc += vhdrlen;
+	rc = dev_hard_header(skb, dev, type, daddr, saddr, len);
 	return rc;
 }
 
@@ -676,9 +634,11 @@ static void vlan_dev_set_lockdep_class(struct net_device *dev, int subclass)
 }
 
 static const struct header_ops vlan_header_ops = {
-	.create	 = vlan_dev_hard_header,
-	.rebuild = vlan_dev_rebuild_header,
-	.parse	 = eth_header_parse,
+	.create		= vlan_dev_hard_header,
+	.rebuild	= eth_rebuild_header,
+	.parse		= eth_header_parse,
+	.cache		= eth_header_cache,
+	.cache_update	= eth_header_cache_update,
 };
 
 static const struct net_device_ops vlan_netdev_ops, vlan_netdev_ops_sq;
@@ -713,13 +673,12 @@ static int vlan_dev_init(struct net_device *dev)
 	dev->fcoe_ddp_xid = real_dev->fcoe_ddp_xid;
 #endif
 
-	if (real_dev->features & NETIF_F_HW_VLAN_TX) {
-		dev->header_ops      = real_dev->header_ops;
+	dev->header_ops = &vlan_header_ops;
+
+	if (real_dev->features & NETIF_F_HW_VLAN_TX)
 		dev->hard_header_len = real_dev->hard_header_len;
-	} else {
-		dev->header_ops      = &vlan_header_ops;
+	else
 		dev->hard_header_len = real_dev->hard_header_len + VLAN_HLEN;
-	}
 
 	if (real_dev->netdev_ops->ndo_select_queue)
 		dev->netdev_ops = &vlan_netdev_ops_sq;

Re: [RFC][net-next-2.6 PATCH 2/4] net: 8021Q consolidate header_ops routines

[Jesse Gross]

On Thu, Oct 21, 2010 at 3:10 PM, John Fastabend
<john.r.fastabend@intel.com> wrote:
>  static inline struct sk_buff *vlan_check_reorder_header(struct sk_buff *skb)
>  {
>        if (vlan_dev_info(skb->dev)->flags & VLAN_FLAG_REORDER_HDR) {
> @@ -269,33 +236,26 @@ static int vlan_dev_hard_header(struct sk_buff *skb, struct net_device *dev,
>                                const void *daddr, const void *saddr,
>                                unsigned int len)
>  {
> -       struct vlan_hdr *vhdr;
> -       unsigned int vhdrlen = 0;
> -       u16 vlan_tci = 0;
>        int rc;
>
>        if (WARN_ON(skb_headroom(skb) < dev->hard_header_len))
>                return -ENOSPC;
>
> +       /* When this flag is not set we make the vlan_tci visible
> +        * by setting the skb field.
> +        *
> +        * We do not immediately insert the tag here the intent
> +        * of setting VLAN_FLAG_REORDER_HDR is to make the vlan
> +        * info avaiable to tap devices and the QOS layer. Adding
> +        * the tag present bit shoould be enough for other layers
> +        * to learn the vlan tag.
> +        */

There's a typo in the comment: "shoould".

>        if (!(vlan_dev_info(dev)->flags & VLAN_FLAG_REORDER_HDR)) {
> -               vhdr = (struct vlan_hdr *) skb_push(skb, VLAN_HLEN);
> +               u16 vlan_tci = 0;
>
>                vlan_tci = vlan_dev_info(dev)->vlan_id;
>                vlan_tci |= vlan_dev_get_egress_qos_mask(dev, skb);
> -               vhdr->h_vlan_TCI = htons(vlan_tci);
> -
> -               /*
> -                *  Set the protocol type. For a packet of type ETH_P_802_3/2 we
> -                *  put the length in here instead.
> -                */
> -               if (type != ETH_P_802_3 && type != ETH_P_802_2)
> -                       vhdr->h_vlan_encapsulated_proto = htons(type);
> -               else
> -                       vhdr->h_vlan_encapsulated_proto = htons(len);
> -
> -               skb->protocol = htons(ETH_P_8021Q);
> -               type = ETH_P_8021Q;
> -               vhdrlen = VLAN_HLEN;
> +               skb = __vlan_hwaccel_put_tag(skb, vlan_tci);
>        }

The only possible downside that I can see to this is that in the
non-accelerated case it requires some extra work because we'll need to
move the MAC addresses around as well.  However, given that
VLAN_FLAG_REORDER_HDR is generally set, I think this is a good code
consolidation.

>
>        /* Before delegating work to the lower layer, enter our MAC-address */
> @@ -304,9 +264,7 @@ static int vlan_dev_hard_header(struct sk_buff *skb, struct net_device *dev,
>
>        /* Now make the underlying real hard header */
>        dev = vlan_dev_info(dev)->real_dev;
> -       rc = dev_hard_header(skb, dev, type, daddr, saddr, len + vhdrlen);
> -       if (rc > 0)
> -               rc += vhdrlen;
> +       rc = dev_hard_header(skb, dev, type, daddr, saddr, len);
>        return rc;

Might as well just drop the rc variable.  It's not adding anything anymore.

Re: [RFC][net-next-2.6 PATCH 2/4] net: 8021Q consolidate header_ops routines

[Jesse Gross]

On Thu, Oct 21, 2010 at 3:10 PM, John Fastabend
<john.r.fastabend@intel.com> wrote:
> The only thing the 8021Q header ops routines are required
> for is the VLAN_FLAG_REORDER_HDR otherwise by the time
> the VLAN tag has been added the packet is already on
> its way down the stack. In this case using the Ethernet
> ops works OK.
>
> At present the VLAN_FLAG_REORDER_HDR flag does not work
> with vlan offloads. As I understand the flag the intent
> is to allow taps on the vlan device and possibly the
> QOS layer to see the vlan tag info.
>
> By inserting the tag in vlan_tci any taps or QOS policies
> should be able to retrieve the vlan info. This allows
> the flag to work the same in both the offload case and
> non-offloaded case. And allows us to use the underlying
> ethernet ops.
>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>

I noticed that you dropped this patch from your most recent series, so
I went back to take a look at it.  I realized that it probably works
inconsistently since header caching doesn't take into account
skb->vlan_tci, so whether you see the tag depends on the state of the
cache.

It would be really good to have this type of code consolidation, both
for the sake of sanity and to eliminate the inconsistent behavior.  We
could do that by either not using header caching or making it work
with vlan offloading somehow.  However, I'm not sure that there's
really much point in that.  VLAN_FLAG_REORDER_HDR doesn't work with
cards that do vlan offloading, which is a pretty significant number of
them.  It similarly works inconsistently on the rx side.  So it's
broken most of the time and worse, the behavior changes depending on
the NIC (and now the ethtool setting).  Can we just eliminate it?

Re: [RFC][net-next-2.6 PATCH 2/4] net: 8021Q consolidate header_ops routines

[John Fastabend]

On 11/3/2010 5:47 PM, Jesse Gross wrote:
> On Thu, Oct 21, 2010 at 3:10 PM, John Fastabend
> <john.r.fastabend@intel.com> wrote:
>> The only thing the 8021Q header ops routines are required
>> for is the VLAN_FLAG_REORDER_HDR otherwise by the time
>> the VLAN tag has been added the packet is already on
>> its way down the stack. In this case using the Ethernet
>> ops works OK.
>>
>> At present the VLAN_FLAG_REORDER_HDR flag does not work
>> with vlan offloads. As I understand the flag the intent
>> is to allow taps on the vlan device and possibly the
>> QOS layer to see the vlan tag info.
>>
>> By inserting the tag in vlan_tci any taps or QOS policies
>> should be able to retrieve the vlan info. This allows
>> the flag to work the same in both the offload case and
>> non-offloaded case. And allows us to use the underlying
>> ethernet ops.
>>
>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> 
> I noticed that you dropped this patch from your most recent series, so
> I went back to take a look at it.  I realized that it probably works
> inconsistently since header caching doesn't take into account
> skb->vlan_tci, so whether you see the tag depends on the state of the
> cache.
> 
> It would be really good to have this type of code consolidation, both
> for the sake of sanity and to eliminate the inconsistent behavior.  We
> could do that by either not using header caching or making it work
> with vlan offloading somehow.  However, I'm not sure that there's
> really much point in that.  VLAN_FLAG_REORDER_HDR doesn't work with
> cards that do vlan offloading, which is a pretty significant number of
> them.  It similarly works inconsistently on the rx side.  So it's
> broken most of the time and worse, the behavior changes depending on
> the NIC (and now the ethtool setting).  Can we just eliminate it?

Yes this is why I have dropped it for now. Also rebuild is broke as best I can tell. Although I doubt anyone would notice you would need to clear VLAN_FLAG_REORDER_HDR and be using one of the ARPHRD_{ROSE|AX25|NETROM}.

The problem with caching the vlan header is the skb priority to vlan priority map. So we could cache the vid, sa, da, and protocols but I can not see anyway to cache the vlan priority. Also the cache would have to be flushed when the flag is toggled.

Thanks,
John.

Re: [RFC][net-next-2.6 PATCH 2/4] net: 8021Q consolidate header_ops routines

[Jesse Gross]

On Thu, Nov 4, 2010 at 6:43 AM, John Fastabend
<john.r.fastabend@intel.com> wrote:
> On 11/3/2010 5:47 PM, Jesse Gross wrote:
>> On Thu, Oct 21, 2010 at 3:10 PM, John Fastabend
>> <john.r.fastabend@intel.com> wrote:
>>> The only thing the 8021Q header ops routines are required
>>> for is the VLAN_FLAG_REORDER_HDR otherwise by the time
>>> the VLAN tag has been added the packet is already on
>>> its way down the stack. In this case using the Ethernet
>>> ops works OK.
>>>
>>> At present the VLAN_FLAG_REORDER_HDR flag does not work
>>> with vlan offloads. As I understand the flag the intent
>>> is to allow taps on the vlan device and possibly the
>>> QOS layer to see the vlan tag info.
>>>
>>> By inserting the tag in vlan_tci any taps or QOS policies
>>> should be able to retrieve the vlan info. This allows
>>> the flag to work the same in both the offload case and
>>> non-offloaded case. And allows us to use the underlying
>>> ethernet ops.
>>>
>>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>>
>> I noticed that you dropped this patch from your most recent series, so
>> I went back to take a look at it.  I realized that it probably works
>> inconsistently since header caching doesn't take into account
>> skb->vlan_tci, so whether you see the tag depends on the state of the
>> cache.
>>
>> It would be really good to have this type of code consolidation, both
>> for the sake of sanity and to eliminate the inconsistent behavior.  We
>> could do that by either not using header caching or making it work
>> with vlan offloading somehow.  However, I'm not sure that there's
>> really much point in that.  VLAN_FLAG_REORDER_HDR doesn't work with
>> cards that do vlan offloading, which is a pretty significant number of
>> them.  It similarly works inconsistently on the rx side.  So it's
>> broken most of the time and worse, the behavior changes depending on
>> the NIC (and now the ethtool setting).  Can we just eliminate it?
>
> Yes this is why I have dropped it for now. Also rebuild is broke as best I can tell. Although I doubt anyone would notice you would need to clear VLAN_FLAG_REORDER_HDR and be using one of the ARPHRD_{ROSE|AX25|NETROM}.
>
> The problem with caching the vlan header is the skb priority to vlan priority map. So we could cache the vid, sa, da, and protocols but I can not see anyway to cache the vlan priority. Also the cache would have to be flushed when the flag is toggled.

I agree, fixing this so that !VLAN_FLAG_REORDER_HDR works correctly in
all cases would be messy.

However, this has been broken for a long time and I don't know of
anyone complaining.  Since it is already a no-op in the accelerated
case, I would like to just drop the flag so we get consistent behavior
and less code.

[RFC][net-next-2.6 PATCH 3/4] ethtool: set hard_header_len using ETH_FLAG_{TX|RX}VLAN

[John Fastabend]

Toggling the vlan tx|rx hw offloads needs to set the hard_header_len
as well otherwise we end up using LL_RESERVED_SPACE incorrectly.
This results in pskb_expand_head() being used unnecessarily.

This add a check in ethtool_op_set_flags to catch the ETH_FLAG_TXVLAN
flag and set the header length.

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---

 net/core/ethtool.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 956a9f4..4f7fe26 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -21,6 +21,7 @@
 #include <linux/uaccess.h>
 #include <linux/vmalloc.h>
 #include <linux/slab.h>
+#include <linux/if_vlan.h>
 
 /*
  * Some useful ethtool_ops methods that're device independent.
@@ -151,6 +152,14 @@ int ethtool_op_set_flags(struct net_device *dev, u32 data, u32 supported)
 	if (data & ~supported)
 		return -EINVAL;
 
+	/* is ETH_FLAGS_TXVLAN being toggled */
+	if ((dev->features & ETH_FLAG_TXVLAN) ^ (data & ETH_FLAG_TXVLAN)) {
+		if (data & ETH_FLAG_TXVLAN)
+			dev->hard_header_len -= VLAN_HLEN;
+		else
+			dev->hard_header_len += VLAN_HLEN;
+	}
+
 	dev->features = ((dev->features & ~flags_dup_features) |
 			 (data & flags_dup_features));
 	return 0;

Re: [RFC][net-next-2.6 PATCH 3/4] ethtool: set hard_header_len using ETH_FLAG_{TX|RX}VLAN

[Ben Hutchings]

On Thu, 2010-10-21 at 15:10 -0700, John Fastabend wrote:
> Toggling the vlan tx|rx hw offloads needs to set the hard_header_len
> as well otherwise we end up using LL_RESERVED_SPACE incorrectly.
> This results in pskb_expand_head() being used unnecessarily.
> 
> This add a check in ethtool_op_set_flags to catch the ETH_FLAG_TXVLAN
> flag and set the header length.
[...]

Note that not every driver that implements the set_flags operation calls
back to ethtool_op_set_flags().

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [RFC][net-next-2.6 PATCH 3/4] ethtool: set hard_header_len using ETH_FLAG_{TX|RX}VLAN

[Jesse Gross]

On Fri, Oct 22, 2010 at 6:00 AM, Ben Hutchings
<bhutchings@solarflare.com> wrote:
> On Thu, 2010-10-21 at 15:10 -0700, John Fastabend wrote:
>> Toggling the vlan tx|rx hw offloads needs to set the hard_header_len
>> as well otherwise we end up using LL_RESERVED_SPACE incorrectly.
>> This results in pskb_expand_head() being used unnecessarily.
>>
>> This add a check in ethtool_op_set_flags to catch the ETH_FLAG_TXVLAN
>> flag and set the header length.
> [...]
>
> Note that not every driver that implements the set_flags operation calls
> back to ethtool_op_set_flags().

Currently all of the drivers that support toggling this using ethtool
call into ethtool_op_set_flags.  Even if they don't, things will
continue to work correctly, albeit with a performance hit, so it's not
a catastrophe.

This does assume that drivers which support offloading will start with
it enabled.  If they don't and just use the non-vlan header length
then this will drop the header length down even further when
offloading is enabled.  All current drivers that support toggling do
start with offloading enabled, so maybe it's not that big a deal.

Another issue is that cards that don't support vlan offloading at all
probably won't take the header into account, so they'll get hit every
time.

When we are using vlan devices we also manually add the vlan header
length but it doesn't update if we change the underlying device.  It
seems a little redundant to have to do it in both places.

I like that this is generic and independent of vlan devices.
Hopefully we can figure out these corner cases (or maybe decide that
they're not important or this is strictly an improvement).

Re: [RFC][net-next-2.6 PATCH 3/4] ethtool: set hard_header_len using ETH_FLAG_{TX|RX}VLAN

[John Fastabend]

On 10/25/2010 3:45 PM, Jesse Gross wrote:
> On Fri, Oct 22, 2010 at 6:00 AM, Ben Hutchings
> <bhutchings@solarflare.com> wrote:
>> On Thu, 2010-10-21 at 15:10 -0700, John Fastabend wrote:
>>> Toggling the vlan tx|rx hw offloads needs to set the hard_header_len
>>> as well otherwise we end up using LL_RESERVED_SPACE incorrectly.
>>> This results in pskb_expand_head() being used unnecessarily.
>>>
>>> This add a check in ethtool_op_set_flags to catch the ETH_FLAG_TXVLAN
>>> flag and set the header length.
>> [...]
>>
>> Note that not every driver that implements the set_flags operation calls
>> back to ethtool_op_set_flags().
> 
> Currently all of the drivers that support toggling this using ethtool
> call into ethtool_op_set_flags.  Even if they don't, things will
> continue to work correctly, albeit with a performance hit, so it's not
> a catastrophe.
> 
> This does assume that drivers which support offloading will start with
> it enabled.  If they don't and just use the non-vlan header length
> then this will drop the header length down even further when
> offloading is enabled.  All current drivers that support toggling do
> start with offloading enabled, so maybe it's not that big a deal.
> 
> Another issue is that cards that don't support vlan offloading at all
> probably won't take the header into account, so they'll get hit every
> time.
> 

The lower layer driver should not include the vlan tag in
hard_header_len because pkts pushed to the real net device will not add
the vlan tag. The vlan device however should increment/dec the len value
depending on if the underlying net device is offloading the vlan tagging.

> When we are using vlan devices we also manually add the vlan header
> length but it doesn't update if we change the underlying device.  It
> seems a little redundant to have to do it in both places.

Right, I think doing this in vlan_transfer_features() should work.
	
> 
> I like that this is generic and independent of vlan devices.
> Hopefully we can figure out these corner cases (or maybe decide that
> they're not important or this is strictly an improvement).

I'll post an update. Thanks for the comments.

-- John

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

[RFC][net-next-2.6 PATCH 4/4] net: remove check for headroom in vlan_dev_create

[John Fastabend]

It is possible for the headroom to be smaller then the
hard_header_len for a short period of time after toggling
the vlan offload setting.

This is not a hard error and skb_cow_head is called in
__vlan_put_tag() to resolve this.

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---

 net/8021q/vlan_dev.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 1645c3c..e043389 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -238,9 +238,6 @@ static int vlan_dev_hard_header(struct sk_buff *skb, struct net_device *dev,
 {
 	int rc;
 
-	if (WARN_ON(skb_headroom(skb) < dev->hard_header_len))
-		return -ENOSPC;
-
 	/* When this flag is not set we make the vlan_tci visible
 	 * by setting the skb field.
 	 *

Re: [RFC][net-next-2.6 PATCH 4/4] net: remove check for headroom in vlan_dev_create

[Jesse Gross]

On Thu, Oct 21, 2010 at 3:10 PM, John Fastabend
<john.r.fastabend@intel.com> wrote:
> It is possible for the headroom to be smaller then the
> hard_header_len for a short period of time after toggling
> the vlan offload setting.
>
> This is not a hard error and skb_cow_head is called in
> __vlan_put_tag() to resolve this.
>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>

How is it possible that the hard_header_len changes on the vlan
device?  It looks like the header length never gets changed after it
is initialized.  There's no set_flags method in the vlan device to
toggle whether it is using offloading or not, it just rides on top of
the underlying device.

On the other hand, I agree that this check isn't actually necessary.

Re: [RFC][net-next-2.6 PATCH 4/4] net: remove check for headroom in vlan_dev_create

[John Fastabend]

On 10/25/2010 3:45 PM, Jesse Gross wrote:
> On Thu, Oct 21, 2010 at 3:10 PM, John Fastabend
> <john.r.fastabend@intel.com> wrote:
>> It is possible for the headroom to be smaller then the
>> hard_header_len for a short period of time after toggling
>> the vlan offload setting.
>>
>> This is not a hard error and skb_cow_head is called in
>> __vlan_put_tag() to resolve this.
>>
>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> 
> How is it possible that the hard_header_len changes on the vlan
> device?  It looks like the header length never gets changed after it
> is initialized.  There's no set_flags method in the vlan device to
> toggle whether it is using offloading or not, it just rides on top of
> the underlying device.

Your right and I think this is why my previous patch was broken. If we
can toggle the underlying offloads we should set the header length as
well. With the updated patch I just sent this should be true now.

Thanks,
John.

> On the other hand, I agree that this check isn't actually necessary.

Re: [RFC][net-next-2.6 PATCH 4/4] net: remove check for headroom in vlan_dev_create

[Jesse Gross]

On Tue, Oct 26, 2010 at 3:05 PM, John Fastabend
<john.r.fastabend@intel.com> wrote:
> On 10/25/2010 3:45 PM, Jesse Gross wrote:
>> On Thu, Oct 21, 2010 at 3:10 PM, John Fastabend
>> <john.r.fastabend@intel.com> wrote:
>>> It is possible for the headroom to be smaller then the
>>> hard_header_len for a short period of time after toggling
>>> the vlan offload setting.
>>>
>>> This is not a hard error and skb_cow_head is called in
>>> __vlan_put_tag() to resolve this.
>>>
>>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>>
>> How is it possible that the hard_header_len changes on the vlan
>> device?  It looks like the header length never gets changed after it
>> is initialized.  There's no set_flags method in the vlan device to
>> toggle whether it is using offloading or not, it just rides on top of
>> the underlying device.
>
> Your right and I think this is why my previous patch was broken. If we
> can toggle the underlying offloads we should set the header length as
> well. With the updated patch I just sent this should be true now.

OK, it makes sense it that context.
Acked-by: Jesse Gross <jesse@nicira.com>

Thanks.

Re: [RFC][net-next-2.6 PATCH 1/4] net: consolidate 8021q tagging

[Jesse Gross]

On Thu, Oct 21, 2010 at 3:10 PM, John Fastabend
<john.r.fastabend@intel.com> wrote:
> Now that VLAN packets are tagged in dev_hard_start_xmit()
> at the bottom of the stack we no longer need to tag them
> in the 8021Q module (Except in the !VLAN_FLAG_REORDER_HDR
> case).
>
> This allows the accel path and non accel paths to be consolidated.
> Here the vlan_tci in the skb is always set and we allow the
> stack to add the actual tag in dev_hard_start_xmit().
>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>

This makes sense.  The only other thing that I would do is drop the
vlan_dev_info(dev)->cnt_encap_on_xmit and
vlan_dev_info(dev)->cnt_inc_headroom_on_tx variables.  Nothing will
ever increment them anymore.