[PATCH 1/3] offloading: Make scatter/gather more tolerant of vlans.

[PATCH 1/3] offloading: Make scatter/gather more tolerant of vlans.

[Jesse Gross]

When checking if it is necessary to linearize a packet, we currently
use vlan_features if the packet contains either an in-band or out-
of-band vlan tag.  However, in-band tags aren't special in any way
for scatter/gather since they are part of the packet buffer and are
simply more data to DMA.  Therefore, only use vlan_features for out-
of-band tags, which could potentially have some interaction with
scatter/gather.

Signed-off-by: Jesse Gross <jesse@nicira.com>
CC: Ben Hutchings <bhutchings@solarflare.com>
---
 net/core/dev.c |   19 ++++++++++++-------
 1 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 35dfb83..d21d655 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1977,15 +1977,20 @@ static inline void skb_orphan_try(struct sk_buff *skb)
 static inline int skb_needs_linearize(struct sk_buff *skb,
 				      struct net_device *dev)
 {
-	int features = dev->features;
+	if (skb_is_nonlinear(skb)) {
+		int features = dev->features;
 
-	if (skb->protocol == htons(ETH_P_8021Q) || vlan_tx_tag_present(skb))
-		features &= dev->vlan_features;
+		if (vlan_tx_tag_present(skb))
+			features &= dev->vlan_features;
 
-	return skb_is_nonlinear(skb) &&
-	       ((skb_has_frag_list(skb) && !(features & NETIF_F_FRAGLIST)) ||
-		(skb_shinfo(skb)->nr_frags && (!(features & NETIF_F_SG) ||
-					      illegal_highdma(dev, skb))));
+		return (skb_has_frag_list(skb) &&
+			!(features & NETIF_F_FRAGLIST)) ||
+			(skb_shinfo(skb)->nr_frags &&
+			(!(features & NETIF_F_SG) ||
+			illegal_highdma(dev, skb)));
+	}
+
+	return 0;
 }
 
 int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
-- 
1.7.1

[PATCH 2/3] offloading: Support multiple vlan tags in GSO.

[Jesse Gross]

We assume that hardware TSO can't support multiple levels of vlan tags
but we allow it to be done.  Therefore, enable GSO to parse these tags
so we can fallback to software.

Signed-off-by: Jesse Gross <jesse@nicira.com>
CC: Ben Hutchings <bhutchings@solarflare.com>
---
 net/core/dev.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index d21d655..8bdda70 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1794,16 +1794,18 @@ struct sk_buff *skb_gso_segment(struct sk_buff *skb, int features)
 	struct sk_buff *segs = ERR_PTR(-EPROTONOSUPPORT);
 	struct packet_type *ptype;
 	__be16 type = skb->protocol;
+	int vlan_depth = ETH_HLEN;
 	int err;
 
-	if (type == htons(ETH_P_8021Q)) {
-		struct vlan_ethhdr *veh;
+	while (type == htons(ETH_P_8021Q)) {
+		struct vlan_hdr *vh;
 
-		if (unlikely(!pskb_may_pull(skb, VLAN_ETH_HLEN)))
+		if (unlikely(!pskb_may_pull(skb, vlan_depth + VLAN_HLEN)))
 			return ERR_PTR(-EINVAL);
 
-		veh = (struct vlan_ethhdr *)skb->data;
-		type = veh->h_vlan_encapsulated_proto;
+		vh = (struct vlan_hdr *)(skb->data + vlan_depth);
+		type = vh->h_vlan_encapsulated_proto;
+		vlan_depth += VLAN_HLEN;
 	}
 
 	skb_reset_mac_header(skb);
-- 
1.7.1

Re: [PATCH 2/3] offloading: Support multiple vlan tags in GSO.

[Ben Hutchings]

On Fri, 2010-10-29 at 15:14 -0700, Jesse Gross wrote:
> We assume that hardware TSO can't support multiple levels of vlan tags
> but we allow it to be done.  Therefore, enable GSO to parse these tags
> so we can fallback to software.
> 
> Signed-off-by: Jesse Gross <jesse@nicira.com>
> CC: Ben Hutchings <bhutchings@solarflare.com>
[...]

I can't see how TSO would be enabled on a second-level VLAN device;
presumably you're thinking about GSO being enabled there?

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: [PATCH 2/3] offloading: Support multiple vlan tags in GSO.

[Jesse Gross]

On Mon, Nov 1, 2010 at 1:33 PM, Ben Hutchings <bhutchings@solarflare.com> wrote:
> On Fri, 2010-10-29 at 15:14 -0700, Jesse Gross wrote:
>> We assume that hardware TSO can't support multiple levels of vlan tags
>> but we allow it to be done.  Therefore, enable GSO to parse these tags
>> so we can fallback to software.
>>
>> Signed-off-by: Jesse Gross <jesse@nicira.com>
>> CC: Ben Hutchings <bhutchings@solarflare.com>
> [...]
>
> I can't see how TSO would be enabled on a second-level VLAN device;
> presumably you're thinking about GSO being enabled there?

Right, TSO can't be enabled on a stacked vlan device.  However, there
are a few ways that I can think of to hit this code path.  One is GSO,
another is encapsulating a bridged packet that is already tagged.

These are definitely corner cases and I can think of a few other
caveats when using them.  Mostly I'm just thinking about making it
robust in the face of weird corner cases, one piece at a time at
least.

Re: [PATCH 2/3] offloading: Support multiple vlan tags in GSO.

[David Miller]

From: Jesse Gross <jesse@nicira.com>
Date: Fri, 29 Oct 2010 15:14:54 -0700

> We assume that hardware TSO can't support multiple levels of vlan tags
> but we allow it to be done.  Therefore, enable GSO to parse these tags
> so we can fallback to software.
> 
> Signed-off-by: Jesse Gross <jesse@nicira.com>

Applied.

[PATCH 3/3] offloading: Force software GSO for multiple vlan tags.

[Jesse Gross]

We currently use vlan_features to check for TSO support if there is
a vlan tag.  However, it's quite likely that the NIC is not able to
do TSO when there is an arbitrary number of tags.  Therefore if there
is more than one tag (in-band or out-of-band), fall back to software
emulation.

Signed-off-by: Jesse Gross <jesse@nicira.com>
CC: Ben Hutchings <bhutchings@solarflare.com>
---
 include/linux/netdevice.h |    7 +++----
 net/core/dev.c            |   16 ++++++++++++++++
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 072652d..980c752 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2234,6 +2234,8 @@ unsigned long netdev_fix_features(unsigned long features, const char *name);
 void netif_stacked_transfer_operstate(const struct net_device *rootdev,
 					struct net_device *dev);
 
+int netif_get_vlan_features(struct sk_buff *skb, struct net_device *dev);
+
 static inline int net_gso_ok(int features, int gso_type)
 {
 	int feature = gso_type << NETIF_F_GSO_SHIFT;
@@ -2249,10 +2251,7 @@ static inline int skb_gso_ok(struct sk_buff *skb, int features)
 static inline int netif_needs_gso(struct net_device *dev, struct sk_buff *skb)
 {
 	if (skb_is_gso(skb)) {
-		int features = dev->features;
-
-		if (skb->protocol == htons(ETH_P_8021Q) || skb->vlan_tci)
-			features &= dev->vlan_features;
+		int features = netif_get_vlan_features(skb, dev);
 
 		return (!skb_gso_ok(skb, features) ||
 			unlikely(skb->ip_summed != CHECKSUM_PARTIAL));
diff --git a/net/core/dev.c b/net/core/dev.c
index 8bdda70..8d74988 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1969,6 +1969,22 @@ static inline void skb_orphan_try(struct sk_buff *skb)
 	}
 }
 
+int netif_get_vlan_features(struct sk_buff *skb, struct net_device *dev)
+{
+	__be16 protocol = skb->protocol;
+
+	if (protocol == htons(ETH_P_8021Q)) {
+		struct vlan_ethhdr *veh = (struct vlan_ethhdr *)skb->data;
+		protocol = veh->h_vlan_encapsulated_proto;
+	} else if (!skb->vlan_tci)
+		return dev->features;
+
+	if (protocol != htons(ETH_P_8021Q))
+		return dev->features & dev->vlan_features;
+	else
+		return 0;
+}
+
 /*
  * Returns true if either:
  *	1. skb has frag_list and the device doesn't support FRAGLIST, or
-- 
1.7.1

Re: [PATCH 3/3] offloading: Force software GSO for multiple vlan tags.

[David Miller]

From: Jesse Gross <jesse@nicira.com>
Date: Fri, 29 Oct 2010 15:14:55 -0700

> We currently use vlan_features to check for TSO support if there is
> a vlan tag.  However, it's quite likely that the NIC is not able to
> do TSO when there is an arbitrary number of tags.  Therefore if there
> is more than one tag (in-band or out-of-band), fall back to software
> emulation.
> 
> Signed-off-by: Jesse Gross <jesse@nicira.com>

Applied.

Re: [PATCH 3/3] offloading: Force software GSO for multiple vlan tags.

[Michał Mirosław]

Hi,

Sorry for late reply, I noticed this patch only after it went to Linus' tree.

2010/10/30 Jesse Gross <jesse@nicira.com>:
> We currently use vlan_features to check for TSO support if there is
> a vlan tag.  However, it's quite likely that the NIC is not able to
> do TSO when there is an arbitrary number of tags.  Therefore if there
> is more than one tag (in-band or out-of-band), fall back to software
> emulation.
>
> Signed-off-by: Jesse Gross <jesse@nicira.com>
> CC: Ben Hutchings <bhutchings@solarflare.com>
> ---
>  include/linux/netdevice.h |    7 +++----
>  net/core/dev.c            |   16 ++++++++++++++++
>  2 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 072652d..980c752 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2234,6 +2234,8 @@ unsigned long netdev_fix_features(unsigned long features, const char *name);
>  void netif_stacked_transfer_operstate(const struct net_device *rootdev,
>                                        struct net_device *dev);
>
> +int netif_get_vlan_features(struct sk_buff *skb, struct net_device *dev);
> +
>  static inline int net_gso_ok(int features, int gso_type)
>  {
>        int feature = gso_type << NETIF_F_GSO_SHIFT;
> @@ -2249,10 +2251,7 @@ static inline int skb_gso_ok(struct sk_buff *skb, int features)
>  static inline int netif_needs_gso(struct net_device *dev, struct sk_buff *skb)
>  {
>        if (skb_is_gso(skb)) {
> -               int features = dev->features;
> -
> -               if (skb->protocol == htons(ETH_P_8021Q) || skb->vlan_tci)
> -                       features &= dev->vlan_features;
> +               int features = netif_get_vlan_features(skb, dev);
>
>                return (!skb_gso_ok(skb, features) ||
>                        unlikely(skb->ip_summed != CHECKSUM_PARTIAL));
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 8bdda70..8d74988 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1969,6 +1969,22 @@ static inline void skb_orphan_try(struct sk_buff *skb)
>        }
>  }
>
> +int netif_get_vlan_features(struct sk_buff *skb, struct net_device *dev)
> +{
> +       __be16 protocol = skb->protocol;
> +
> +       if (protocol == htons(ETH_P_8021Q)) {
> +               struct vlan_ethhdr *veh = (struct vlan_ethhdr *)skb->data;
> +               protocol = veh->h_vlan_encapsulated_proto;
> +       } else if (!skb->vlan_tci)
> +               return dev->features;
> +
> +       if (protocol != htons(ETH_P_8021Q))
> +               return dev->features & dev->vlan_features;
> +       else
> +               return 0;
> +}

This clears all features for multiply-tagged frames. At least SG,
FRAGLIST and HW_CSUM are perfectly valid for those frames.

This doesn't really matter if this function stays used only in
netif_needs_gso(). It's name and placement suggests otherwise, though.

Best Regards,
Michał Mirosław

Re: [PATCH 3/3] offloading: Force software GSO for multiple vlan tags.

[Jesse Gross]

2011/1/7 Michał Mirosław <mirqus@gmail.com>:
> Hi,
>
> Sorry for late reply, I noticed this patch only after it went to Linus' tree.
>
> 2010/10/30 Jesse Gross <jesse@nicira.com>:
>> We currently use vlan_features to check for TSO support if there is
>> a vlan tag.  However, it's quite likely that the NIC is not able to
>> do TSO when there is an arbitrary number of tags.  Therefore if there
>> is more than one tag (in-band or out-of-band), fall back to software
>> emulation.
>>
>> Signed-off-by: Jesse Gross <jesse@nicira.com>
>> CC: Ben Hutchings <bhutchings@solarflare.com>
>> ---
>>  include/linux/netdevice.h |    7 +++----
>>  net/core/dev.c            |   16 ++++++++++++++++
>>  2 files changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 072652d..980c752 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -2234,6 +2234,8 @@ unsigned long netdev_fix_features(unsigned long features, const char *name);
>>  void netif_stacked_transfer_operstate(const struct net_device *rootdev,
>>                                        struct net_device *dev);
>>
>> +int netif_get_vlan_features(struct sk_buff *skb, struct net_device *dev);
>> +
>>  static inline int net_gso_ok(int features, int gso_type)
>>  {
>>        int feature = gso_type << NETIF_F_GSO_SHIFT;
>> @@ -2249,10 +2251,7 @@ static inline int skb_gso_ok(struct sk_buff *skb, int features)
>>  static inline int netif_needs_gso(struct net_device *dev, struct sk_buff *skb)
>>  {
>>        if (skb_is_gso(skb)) {
>> -               int features = dev->features;
>> -
>> -               if (skb->protocol == htons(ETH_P_8021Q) || skb->vlan_tci)
>> -                       features &= dev->vlan_features;
>> +               int features = netif_get_vlan_features(skb, dev);
>>
>>                return (!skb_gso_ok(skb, features) ||
>>                        unlikely(skb->ip_summed != CHECKSUM_PARTIAL));
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 8bdda70..8d74988 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -1969,6 +1969,22 @@ static inline void skb_orphan_try(struct sk_buff *skb)
>>        }
>>  }
>>
>> +int netif_get_vlan_features(struct sk_buff *skb, struct net_device *dev)
>> +{
>> +       __be16 protocol = skb->protocol;
>> +
>> +       if (protocol == htons(ETH_P_8021Q)) {
>> +               struct vlan_ethhdr *veh = (struct vlan_ethhdr *)skb->data;
>> +               protocol = veh->h_vlan_encapsulated_proto;
>> +       } else if (!skb->vlan_tci)
>> +               return dev->features;
>> +
>> +       if (protocol != htons(ETH_P_8021Q))
>> +               return dev->features & dev->vlan_features;
>> +       else
>> +               return 0;
>> +}
>
> This clears all features for multiply-tagged frames. At least SG,
> FRAGLIST and HW_CSUM are perfectly valid for those frames.
>
> This doesn't really matter if this function stays used only in
> netif_needs_gso(). It's name and placement suggests otherwise, though.

You're right and in fact I have some upcoming changes that expands the
use of this function to places that do care about these offloads.
I've generalized it as you suggest.

Thanks.

Re: [PATCH 1/3] offloading: Make scatter/gather more tolerant of vlans.

[Ben Hutchings]

On Fri, 2010-10-29 at 15:14 -0700, Jesse Gross wrote:
> When checking if it is necessary to linearize a packet, we currently
> use vlan_features if the packet contains either an in-band or out-
> of-band vlan tag.  However, in-band tags aren't special in any way
> for scatter/gather since they are part of the packet buffer and are
> simply more data to DMA.  Therefore, only use vlan_features for out-
> of-band tags, which could potentially have some interaction with
> scatter/gather.
> 
> Signed-off-by: Jesse Gross <jesse@nicira.com>
> CC: Ben Hutchings <bhutchings@solarflare.com>
[...]

Reviewed-by: Ben Hutchings <bhutchings@solarflare.com>

-- 
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: [PATCH 1/3] offloading: Make scatter/gather more tolerant of vlans.

[David Miller]

From: Jesse Gross <jesse@nicira.com>
Date: Fri, 29 Oct 2010 15:14:53 -0700

> When checking if it is necessary to linearize a packet, we currently
> use vlan_features if the packet contains either an in-band or out-
> of-band vlan tag.  However, in-band tags aren't special in any way
> for scatter/gather since they are part of the packet buffer and are
> simply more data to DMA.  Therefore, only use vlan_features for out-
> of-band tags, which could potentially have some interaction with
> scatter/gather.
> 
> Signed-off-by: Jesse Gross <jesse@nicira.com>

Applied.