[PATCH net 0/5] Implement ndo_gso_check() for vxlan nics

Most NICs that report NETIF_F_GSO_UDP_TUNNEL support VXLAN, and not other
UDP-based encapsulation protocols where the format and size of the header may
differ. This patch series implements ndo_gso_check() for these NICs,
restricting the GSO handling to something that looks and smells like VXLAN.

Implementation shamelessly stolen from Tom Herbert (with minor fixups):
http://thread.gmane.org/gmane.linux.network/332428/focus=333111

If there are particular differences for your driver on actual support, I'd like
to hear about it. I adjusted the i40e driver to report support with tunnel
headers of up to 64 octets, perhaps there are other specifics that I've missed.

Joe Stringer (5):
  be2net: Implement ndo_gso_check()
  i40e: Implement ndo_gso_check()
  fm10k: Implement ndo_gso_check()
  net/mlx4_en: Implement ndo_gso_check()
  qlcnic: Implement ndo_gso_check()

 drivers/net/ethernet/emulex/benet/be_main.c      |   12 ++++++++++++
 drivers/net/ethernet/intel/fm10k/fm10k_netdev.c  |   12 ++++++++++++
 drivers/net/ethernet/intel/i40e/i40e_main.c      |   14 +++++++++++++-
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c   |   12 ++++++++++++
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c |   12 ++++++++++++
 5 files changed, 61 insertions(+), 1 deletion(-)

--
1.7.10.4

ndo_gso_check() was recently introduced to allow NICs to report the
offloading support that they have on a per-skb basis. Add an
implementation for this driver which checks for something that looks
like VXLAN.

Implementation shamelessly stolen from Tom Herbert:
http://thread.gmane.org/gmane.linux.network/332428/focus=333111

Signed-off-by: Joe Stringer <redacted>
---
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index f3032fe..aca9908 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c

@@ -2344,6 +2344,17 @@ static void mlx4_en_del_vxlan_port(struct  net_device *dev,
 }
 #endif
 
+static bool mlx4_en_gso_check(struct sk_buff *skb, struct net_device *dev)
+{
+	if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
+	    (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
+	     skb->inner_protocol != htons(ETH_P_TEB) ||
+	     skb_inner_mac_header(skb) - skb_transport_header(skb) != 16))
+		return false;
+
+	return true;
+}
+
 static const struct net_device_ops mlx4_netdev_ops = {
 	.ndo_open		= mlx4_en_open,
 	.ndo_stop		= mlx4_en_close,

@@ -2374,6 +2385,7 @@ static const struct net_device_ops mlx4_netdev_ops = {
 	.ndo_add_vxlan_port	= mlx4_en_add_vxlan_port,
 	.ndo_del_vxlan_port	= mlx4_en_del_vxlan_port,
 #endif
+	.ndo_gso_check		= mlx4_en_gso_check,
 };
 
 static const struct net_device_ops mlx4_netdev_ops_master = {

-- 
1.7.10.4

ndo_gso_check() was recently introduced to allow NICs to report the
offloading support that they have on a per-skb basis. Add an
implementation for this driver which checks for something that looks
like VXLAN.

Implementation shamelessly stolen from Tom Herbert:
http://thread.gmane.org/gmane.linux.network/332428/focus=333111

Signed-off-by: Joe Stringer <redacted>
---
 drivers/net/ethernet/emulex/benet/be_main.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index 9a18e79..bd52b8d 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c

@@ -4423,6 +4423,17 @@ static void be_del_vxlan_port(struct net_device *netdev, sa_family_t sa_family,
 }
 #endif
 
+static bool be_gso_check(struct sk_buff *skb, struct net_device *dev)
+{
+	if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
+	    (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
+	     skb->inner_protocol != htons(ETH_P_TEB) ||
+	     skb_inner_mac_header(skb) - skb_transport_header(skb) != 16))
+		return false;
+
+	return true;
+}
+
 static const struct net_device_ops be_netdev_ops = {
 	.ndo_open		= be_open,
 	.ndo_stop		= be_close,

@@ -4451,6 +4462,7 @@ static const struct net_device_ops be_netdev_ops = {
 	.ndo_add_vxlan_port	= be_add_vxlan_port,
 	.ndo_del_vxlan_port	= be_del_vxlan_port,
 #endif
+	.ndo_gso_check		= be_gso_check,
 };
 
 static void be_netdev_init(struct net_device *netdev)

-- 
1.7.10.4

ndo_gso_check() was recently introduced to allow NICs to report the
offloading support that they have on a per-skb basis. Add an
implementation for this driver which checks for tunnel headers over UDP
of up to 64 octets in length.

Implementation shamelessly stolen from Tom Herbert:
http://thread.gmane.org/gmane.linux.network/332428/focus=333111

Signed-off-by: Joe Stringer <redacted>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index c3a7f4a..21829b5 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c

@@ -7444,9 +7444,20 @@ static int i40e_ndo_fdb_dump(struct sk_buff *skb,
 
 	return idx;
 }
-
 #endif /* USE_DEFAULT_FDB_DEL_DUMP */
 #endif /* HAVE_FDB_OPS */
+
+static bool i40e_gso_check(struct sk_buff *skb, struct net_device *dev)
+{
+	if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
+	    (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
+	     skb->inner_protocol != htons(ETH_P_TEB) ||
+	     skb_inner_mac_header(skb) - skb_transport_header(skb) > 64))
+		return false;
+
+	return true;
+}
+
 static const struct net_device_ops i40e_netdev_ops = {
 	.ndo_open		= i40e_open,
 	.ndo_stop		= i40e_close,

@@ -7487,6 +7498,7 @@ static const struct net_device_ops i40e_netdev_ops = {
 	.ndo_fdb_dump		= i40e_ndo_fdb_dump,
 #endif
 #endif
+	.ndo_gso_check		= i40e_gso_check,
 };
 
 /**

-- 
1.7.10.4

ndo_gso_check() was recently introduced to allow NICs to report the
offloading support that they have on a per-skb basis. Add an
implementation for this driver which checks for something that looks
like VXLAN.

Implementation shamelessly stolen from Tom Herbert:
http://thread.gmane.org/gmane.linux.network/332428/focus=333111

Signed-off-by: Joe Stringer <redacted>
---
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
index f5e29f7..6184f47 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c

@@ -505,6 +505,17 @@ static void qlcnic_del_vxlan_port(struct net_device *netdev,
 }
 #endif
 
+static bool qlcnic_gso_check(struct sk_buff *skb, struct net_device *dev)
+{
+	if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
+	    (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
+	     skb->inner_protocol != htons(ETH_P_TEB) ||
+	     skb_inner_mac_header(skb) - skb_transport_header(skb) != 16))
+		return false;
+
+	return true;
+}
+
 static const struct net_device_ops qlcnic_netdev_ops = {
 	.ndo_open	   = qlcnic_open,
 	.ndo_stop	   = qlcnic_close,

@@ -537,6 +548,7 @@ static const struct net_device_ops qlcnic_netdev_ops = {
 	.ndo_set_vf_vlan	= qlcnic_sriov_set_vf_vlan,
 	.ndo_set_vf_spoofchk	= qlcnic_sriov_set_vf_spoofchk,
 #endif
+	.ndo_gso_check		= qlcnic_gso_check,
 };
 
 static const struct net_device_ops qlcnic_netdev_failed_ops = {

-- 
1.7.10.4

ndo_gso_check() was recently introduced to allow NICs to report the
offloading support that they have on a per-skb basis. Add an
implementation for this driver which checks for something that looks
like VXLAN.

Implementation shamelessly stolen from Tom Herbert:
http://thread.gmane.org/gmane.linux.network/332428/focus=333111

Signed-off-by: Joe Stringer <redacted>
---
Should this driver report support for GSO on packets with tunnel headers
up to 64B like the i40e driver does?
---
 drivers/net/ethernet/intel/fm10k/fm10k_netdev.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
index 8811364..b9ef622 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c

@@ -1350,6 +1350,17 @@ static void fm10k_dfwd_del_station(struct net_device *dev, void *priv)
 	}
 }
 
+static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
+{
+	if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
+	    (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
+	     skb->inner_protocol != htons(ETH_P_TEB) ||
+	     skb_inner_mac_header(skb) - skb_transport_header(skb) != 16))
+		return false;
+
+	return true;
+}
+
 static const struct net_device_ops fm10k_netdev_ops = {
 	.ndo_open		= fm10k_open,
 	.ndo_stop		= fm10k_close,

@@ -1372,6 +1383,7 @@ static const struct net_device_ops fm10k_netdev_ops = {
 	.ndo_do_ioctl		= fm10k_ioctl,
 	.ndo_dfwd_add_station	= fm10k_dfwd_add_station,
 	.ndo_dfwd_del_station	= fm10k_dfwd_del_station,
+	.ndo_gso_check		= fm10k_gso_check,
 };
 
 #define DEFAULT_DEBUG_LEVEL_SHIFT 3

-- 
1.7.10.4

On Tue, Nov 4, 2014 at 1:56 PM, Joe Stringer [off-list ref] wrote:

I think it may be possible to even support a few more things here.
According to the datasheet here:
http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/xl710-10-40-controller-datasheet.pdf

This can actually support 64 bytes beyond the tunnel header, which
would make for a total of 80 bytes. It looks like it can also support
IPv4 or IPv6 beyond just Ethernet as the encapsulated protocol.

Intel guys, can you confirm that this is correct?

Hi Joe,

Yes, qlcnic driver only supports VXLAN offload.
It would be good to put a comment about value 16 to make it more intuitive. 
e.g. 16 is the size of outer UDP header + VXLAN header.

Anyway, patch looks good to me.

Acked-by: Shahed Shaikh <redacted>

Thanks,
Shahed

On Tue, 2014-11-04 at 13:56 -0800, Joe Stringer wrote:

ndo_gso_check() was recently introduced to allow NICs to report the
offloading support that they have on a per-skb basis. Add an
implementation for this driver which checks for something that looks
like VXLAN.

Implementation shamelessly stolen from Tom Herbert:
http://thread.gmane.org/gmane.linux.network/332428/focus=333111

Signed-off-by: Joe Stringer <redacted>
---
Should this driver report support for GSO on packets with tunnel
headers
up to 64B like the i40e driver does?
---
 drivers/net/ethernet/intel/fm10k/fm10k_netdev.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

Thanks Joe, I will add your patch to my queue.

On Tue, 2014-11-04 at 13:56 -0800, Joe Stringer wrote:

ndo_gso_check() was recently introduced to allow NICs to report the
offloading support that they have on a per-skb basis. Add an
implementation for this driver which checks for tunnel headers over
UDP
of up to 64 octets in length.

Implementation shamelessly stolen from Tom Herbert:
http://thread.gmane.org/gmane.linux.network/332428/focus=333111

Signed-off-by: Joe Stringer <redacted>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

Thanks again Joe, I will add your patch to my queue.

On Tue, 2014-11-04 at 15:45 -0800, Jesse Gross wrote:

On Tue, Nov 4, 2014 at 1:56 PM, Joe Stringer [off-list ref] wrote:

quoted

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index c3a7f4a..21829b5 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
+static bool i40e_gso_check(struct sk_buff *skb, struct net_device *dev)
+{
+       if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
+           (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
+            skb->inner_protocol != htons(ETH_P_TEB) ||
+            skb_inner_mac_header(skb) - skb_transport_header(skb) > 64))
+               return false;

I think it may be possible to even support a few more things here.
According to the datasheet here:
http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/xl710-10-40-controller-datasheet.pdf

This can actually support 64 bytes beyond the tunnel header, which
would make for a total of 80 bytes. It looks like it can also support
IPv4 or IPv6 beyond just Ethernet as the encapsulated protocol.

Intel guys, can you confirm that this is correct?

I believe you are correct Jesse, but I will let Shannon Nelson or Jesse
Brandeburg respond since they are the i40e maintainers.

On Tue, Nov 4, 2014 at 11:56 PM, Joe Stringer [off-list ref] wrote:

Most NICs that report NETIF_F_GSO_UDP_TUNNEL support VXLAN, and not other
UDP-based encapsulation protocols where the format and size of the header may
differ. This patch series implements ndo_gso_check() for these NICs,
restricting the GSO handling to something that looks and smells like VXLAN.

Implementation shamelessly stolen from Tom Herbert (with minor fixups):
http://thread.gmane.org/gmane.linux.network/332428/focus=333111

Hi Joe,

1st, thanks for picking this task...2nd, for drivers that currently
support only pure VXLAN, I don't see the point
to replicate the helper suggested by Tom (good catch on the size check
to be 16 and not 12) four times and who know how more in the future.
Let's just have one generic helper and make the mlx4/be/fm10k/benet
drivers to have it as their ndo, OK?

Or.

If there are particular differences for your driver on actual support, I'd like
to hear about it. I adjusted the i40e driver to report support with tunnel
headers of up to 64 octets, perhaps there are other specifics that I've missed.

Joe Stringer (5):
  be2net: Implement ndo_gso_check()
  i40e: Implement ndo_gso_check()
  fm10k: Implement ndo_gso_check()
  net/mlx4_en: Implement ndo_gso_check()
  qlcnic: Implement ndo_gso_check()

 drivers/net/ethernet/emulex/benet/be_main.c      |   12 ++++++++++++
 drivers/net/ethernet/intel/fm10k/fm10k_netdev.c  |   12 ++++++++++++
 drivers/net/ethernet/intel/i40e/i40e_main.c      |   14 +++++++++++++-
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c   |   12 ++++++++++++
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c |   12 ++++++++++++
 5 files changed, 61 insertions(+), 1 deletion(-)

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

On Tue, Nov 4, 2014 at 11:56 PM, Joe Stringer [off-list ref] wrote:

Let's have this 16 constant to be more clear... e.g make it the sum of
sizeof struct udphdr and struct vxlanhdr - you would need to move the
latter from vxlan.c into a public header. All for the general patch I
suggested

Or.

On Wed, Nov 5, 2014 at 2:34 PM, Jeff Kirsher
[off-list ref] wrote:

On Tue, 2014-11-04 at 13:56 -0800, Joe Stringer wrote:

quoted

ndo_gso_check() was recently introduced to allow NICs to report the
offloading support that they have on a per-skb basis. Add an
implementation for this driver which checks for something that looks
like VXLAN.

Implementation shamelessly stolen from Tom Herbert:
http://thread.gmane.org/gmane.linux.network/332428/focus=333111

Signed-off-by: Joe Stringer <redacted>
---
Should this driver report support for GSO on packets with tunnel
headers
up to 64B like the i40e driver does?
---
 drivers/net/ethernet/intel/fm10k/fm10k_netdev.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

Thanks Joe, I will add your patch to my queue.

Hi Jeff, please see my comment on patch 0/5, we're essentially
replicating the same helper four different times (fm10k, mlx4, benet,
qlgc) - I don't see the point in doing so. I asked Joe to come up with
one generic helper and then to pick it up by the four drivers, makes
sense?

On Wed, 2014-11-05 at 14:44 +0200, Or Gerlitz wrote:

On Wed, Nov 5, 2014 at 2:34 PM, Jeff Kirsher
[off-list ref] wrote:

quoted

On Tue, 2014-11-04 at 13:56 -0800, Joe Stringer wrote:

quoted

ndo_gso_check() was recently introduced to allow NICs to report the
offloading support that they have on a per-skb basis. Add an
implementation for this driver which checks for something that looks
like VXLAN.

Implementation shamelessly stolen from Tom Herbert:
http://thread.gmane.org/gmane.linux.network/332428/focus=333111

Signed-off-by: Joe Stringer <redacted>
---
Should this driver report support for GSO on packets with tunnel
headers
up to 64B like the i40e driver does?
---
 drivers/net/ethernet/intel/fm10k/fm10k_netdev.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

Thanks Joe, I will add your patch to my queue.

Hi Jeff, please see my comment on patch 0/5, we're essentially
replicating the same helper four different times (fm10k, mlx4, benet,
qlgc) - I don't see the point in doing so. I asked Joe to come up with
one generic helper and then to pick it up by the four drivers, makes
sense?

Yeah, I just saw your reply Or.  Ok, I will await an update to Joe's
series, thanks!

On 11/04/2014 01:56 PM, Joe Stringer wrote:

I'm thinking this check is far too simplistic.  If you look the fm10k
driver already has fm10k_tx_encap_offload() in the TSO function for
verifying if it can support offloading tunnels or not.  I would
recommend starting there or possibly even just adapting that function to
suit your purpose.

Thanks,

Alex

On Wed, Nov 05, 2014 at 06:54:00PM -0800, Alexander Duyck wrote:

On 11/04/2014 01:56 PM, Joe Stringer wrote:

quoted

ndo_gso_check() was recently introduced to allow NICs to report the
offloading support that they have on a per-skb basis. Add an
implementation for this driver which checks for something that looks
like VXLAN.

Implementation shamelessly stolen from Tom Herbert:
http://thread.gmane.org/gmane.linux.network/332428/focus=333111

Signed-off-by: Joe Stringer <redacted>
---
Should this driver report support for GSO on packets with tunnel headers
up to 64B like the i40e driver does?
---
 drivers/net/ethernet/intel/fm10k/fm10k_netdev.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
index 8811364..b9ef622 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c

@@ -1350,6 +1350,17 @@ static void fm10k_dfwd_del_station(struct net_device *dev, void *priv)
 	}
 }
 
+static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
+{
+	if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
+	    (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
+	     skb->inner_protocol != htons(ETH_P_TEB) ||
+	     skb_inner_mac_header(skb) - skb_transport_header(skb) != 16))
+		return false;
+
+	return true;
+}
+
 static const struct net_device_ops fm10k_netdev_ops = {
 	.ndo_open		= fm10k_open,
 	.ndo_stop		= fm10k_close,

@@ -1372,6 +1383,7 @@ static const struct net_device_ops fm10k_netdev_ops = {
 	.ndo_do_ioctl		= fm10k_ioctl,
 	.ndo_dfwd_add_station	= fm10k_dfwd_add_station,
 	.ndo_dfwd_del_station	= fm10k_dfwd_del_station,
+	.ndo_gso_check		= fm10k_gso_check,
 };
 
 #define DEFAULT_DEBUG_LEVEL_SHIFT 3

I'm thinking this check is far too simplistic.  If you look the fm10k
driver already has fm10k_tx_encap_offload() in the TSO function for
verifying if it can support offloading tunnels or not.  I would
recommend starting there or possibly even just adapting that function to
suit your purpose.

Thanks,

Alex

Would it be enough to just call fm10k_tx_encap_offload() in a way that echoes fm10k_tso()?

+static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
+{
+       if (skb->encapsulation && !fm10k_tx_encap_offload(skb))
+               return false;
+
+       return true;
+}

Thanks,
Joe

On Thu, Nov 06, 2014 at 10:41:15AM -0800, Joe Stringer wrote:

On Wed, Nov 05, 2014 at 06:54:00PM -0800, Alexander Duyck wrote:

quoted

On 11/04/2014 01:56 PM, Joe Stringer wrote:

quoted

ndo_gso_check() was recently introduced to allow NICs to report the
offloading support that they have on a per-skb basis. Add an
implementation for this driver which checks for something that looks
like VXLAN.

Implementation shamelessly stolen from Tom Herbert:
http://thread.gmane.org/gmane.linux.network/332428/focus=333111

Signed-off-by: Joe Stringer <redacted>
---
Should this driver report support for GSO on packets with tunnel headers
up to 64B like the i40e driver does?
---
 drivers/net/ethernet/intel/fm10k/fm10k_netdev.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
index 8811364..b9ef622 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c

@@ -1350,6 +1350,17 @@ static void fm10k_dfwd_del_station(struct net_device *dev, void *priv)
 	}
 }
 
+static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
+{
+	if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
+	    (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
+	     skb->inner_protocol != htons(ETH_P_TEB) ||
+	     skb_inner_mac_header(skb) - skb_transport_header(skb) != 16))
+		return false;
+
+	return true;
+}
+
 static const struct net_device_ops fm10k_netdev_ops = {
 	.ndo_open		= fm10k_open,
 	.ndo_stop		= fm10k_close,

@@ -1372,6 +1383,7 @@ static const struct net_device_ops fm10k_netdev_ops = {
 	.ndo_do_ioctl		= fm10k_ioctl,
 	.ndo_dfwd_add_station	= fm10k_dfwd_add_station,
 	.ndo_dfwd_del_station	= fm10k_dfwd_del_station,
+	.ndo_gso_check		= fm10k_gso_check,
 };
 
 #define DEFAULT_DEBUG_LEVEL_SHIFT 3

I'm thinking this check is far too simplistic.  If you look the fm10k
driver already has fm10k_tx_encap_offload() in the TSO function for
verifying if it can support offloading tunnels or not.  I would
recommend starting there or possibly even just adapting that function to
suit your purpose.

Thanks,

Alex

Would it be enough to just call fm10k_tx_encap_offload() in a way that echoes fm10k_tso()?

+static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
+{
+       if (skb->encapsulation && !fm10k_tx_encap_offload(skb))
+               return false;
+
+       return true;
+}

Oh, I suppose we need to check the gso_type too. More like this?

+static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
+{
+       if ((skb_shinfo(skb)->gso_type & (SKB_GSO_UDP_TUNNEL | SKB_GSO_GRE)) &&
+           !fm10k_tx_encap_offload(skb))
+               return false;
+
+       return true;
+}

On 11/6/14, 1:15 PM, "Joe Stringer" [off-list ref] wrote:

Oh, I suppose we need to check the gso_type too. More like this?

+static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
+{
+       if ((skb_shinfo(skb)->gso_type & (SKB_GSO_UDP_TUNNEL |
SKB_GSO_GRE)) &&
+           !fm10k_tx_encap_offload(skb))
+               return false;
+
+       return true;
+}

It seems like the skb_shinfo(skb)->gso_type check should be in some more
generic ndo_gso_check that drivers can default to/extend. Then, we could
end up with something like

static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
{
	if (skb_gso_check(skb, dev) && !fm10k_tx_encap_offload(skb))
		return false;

	return true;
}

This could even be simplified and still legible as

static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
{
	return !(skb_gso_check(skb, dev) && !fm10k_tx_encap_offload(skb));
}

What do you think of this approach?

On Fri, 07 Nov 2014 14:07:36 Vick, Matthew wrote:

On 11/6/14, 1:15 PM, "Joe Stringer" [off-list ref] wrote:

quoted

Oh, I suppose we need to check the gso_type too. More like this?

+static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
+{
+       if ((skb_shinfo(skb)->gso_type & (SKB_GSO_UDP_TUNNEL |
SKB_GSO_GRE)) &&
+           !fm10k_tx_encap_offload(skb))
+               return false;
+
+       return true;
+}

It seems like the skb_shinfo(skb)->gso_type check should be in some more
generic ndo_gso_check that drivers can default to/extend. Then, we could
end up with something like

static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
{
	if (skb_gso_check(skb, dev) && !fm10k_tx_encap_offload(skb))
		return false;

	return true;
}

This could even be simplified and still legible as

static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
{
	return !(skb_gso_check(skb, dev) && !fm10k_tx_encap_offload(skb));
}

What do you think of this approach?

Let's merge both discussions into one thread (pick here or there). We have 
this suggestion or the one which simply checks for tunnels and inner+outer 
header lengths. Do you have a preference between them?

We could introduce an "skb_is_gso_encap()" or similar for this purpose. 
Checking for SKB_GSO_UDP_TUNNEL or SKB_GSO_GRE is pretty closely tied to what 
fm10k_tx_encap_offload() checks for though; it might not even make sense to call 
it if some of the other SKB_GSO_* flags are raised.

Joe

On 11/6/14, 9:05 PM, "Joe Stringer" [off-list ref] wrote:

Let's merge both discussions into one thread (pick here or there). We
have 
this suggestion or the one which simply checks for tunnels and
inner+outer 
header lengths. Do you have a preference between them?

Agreed with merging the thread--consider it merged!

Reflecting on this some more, I prefer the latter option (checking tunnels
and header lengths). I'm leaning towards pushing the header length check
into fm10k_tx_encap_offload and then making fm10k_gso_check call that with
the gso_type. So, it's really the most recent version of the patch you
proposed:

static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
{
	if ((skb_shinfo(skb)->gso_type & (SKB_GSO_UDP_TUNNEL | SKB_GSO_GRE)) &&
	    !fm10k_tx_encap_offload(skb))
		return false;

	return true;
}


plus the header length being checked in fm10k_tx_encap_offload. The only
nit would be that I'd just return the conditional instead of having
"return true" or "return false" lines.

The tunnel length check really should be there in fm10k_tx_encap_offload
anyway, so I'll get a patch together for that one.

We could introduce an "skb_is_gso_encap()" or similar for this purpose.
Checking for SKB_GSO_UDP_TUNNEL or SKB_GSO_GRE is pretty closely tied to
what 
fm10k_tx_encap_offload() checks for though; it might not even make sense
to call 
it if some of the other SKB_GSO_* flags are raised.

A fair point. On the other hand, we have to check the header length both
in the GSO and non-GSO cases anyway, so only having the check in
fm10k_tx_encap_offload and calling it from fm10k_gso_check wouldn't be as
duplicative. What do you think about that approach?

As an aside: the more I think about this, the more I think Tom's right and
that each driver really should have it's own ndo_gso_check() for this.
With fm10k and i40e being different, we're already at 40% of the current
drivers being different. I'll leave it to Or to comment on whether the
other drivers could share the check in some manner.

Cheers,
Matthew

On Friday, November 07, 2014 11:49:38 Vick, Matthew wrote:

On 11/6/14, 9:05 PM, "Joe Stringer" [off-list ref] wrote:

quoted

Let's merge both discussions into one thread (pick here or there). We
have
this suggestion or the one which simply checks for tunnels and
inner+outer
header lengths. Do you have a preference between them?

Agreed with merging the thread--consider it merged!

Reflecting on this some more, I prefer the latter option (checking tunnels
and header lengths). I'm leaning towards pushing the header length check
into fm10k_tx_encap_offload and then making fm10k_gso_check call that with
the gso_type. So, it's really the most recent version of the patch you
proposed:

static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
{
	if ((skb_shinfo(skb)->gso_type & (SKB_GSO_UDP_TUNNEL | SKB_GSO_GRE)) &&
	    !fm10k_tx_encap_offload(skb))
		return false;

	return true;
}


plus the header length being checked in fm10k_tx_encap_offload. The only
nit would be that I'd just return the conditional instead of having
"return true" or "return false" lines.

OK, that sounds reasonable.

The tunnel length check really should be there in fm10k_tx_encap_offload
anyway, so I'll get a patch together for that one.

Thanks.

quoted

We could introduce an "skb_is_gso_encap()" or similar for this purpose.
Checking for SKB_GSO_UDP_TUNNEL or SKB_GSO_GRE is pretty closely tied to
what
fm10k_tx_encap_offload() checks for though; it might not even make sense
to call
it if some of the other SKB_GSO_* flags are raised.

A fair point. On the other hand, we have to check the header length both
in the GSO and non-GSO cases anyway, so only having the check in
fm10k_tx_encap_offload and calling it from fm10k_gso_check wouldn't be as
duplicative. What do you think about that approach?

Sure, I think fm10k_tx_encap_offload() is a good place for the header length 
check. Separately, my question above was regarding the idea of a helper for 
SKB_GSO_{GRE,UDP_TUNNEL}. The only reason it might be useful for the fm10k 
driver is because all encap is checked in the fm10k_tx_encap_offload() function. 
Other drivers don't seem to handle different tunnels together like this though, 
so I'm inclined to stick with the below for now.
                                                                                
static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)        
{                                                                               
        return (!(skb_shinfo(skb)->gso_type &                                   
                  (SKB_GSO_UDP_TUNNEL | SKB_GSO_GRE)) ||                        
                fm10k_tx_encap_offload(skb));                                   
}

Cheers,
Joe

On 11/7/14, 2:35 PM, "Joe Stringer" [off-list ref] wrote:

Sure, I think fm10k_tx_encap_offload() is a good place for the header
length 
check. Separately, my question above was regarding the idea of a helper
for 
SKB_GSO_{GRE,UDP_TUNNEL}. The only reason it might be useful for the
fm10k 
driver is because all encap is checked in the fm10k_tx_encap_offload()
function. 
Other drivers don't seem to handle different tunnels together like this
though, 
so I'm inclined to stick with the below for now.
                 
     
static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
     
{                 
     
       return (!(skb_shinfo(skb)->gso_type &
     
                 (SKB_GSO_UDP_TUNNEL | SKB_GSO_GRE)) ||
     
               fm10k_tx_encap_offload(skb));
     
}

Cheers,
Joe

I agree. I think that makes the most sense.

Cheers,
Matthew

On Tuesday, November 04, 2014 15:45:22 Jesse Gross wrote:

On Tue, Nov 4, 2014 at 1:56 PM, Joe Stringer [off-list ref] wrote:

quoted

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c

b/drivers/net/ethernet/intel/i40e/i40e_main.c index c3a7f4a..21829b5
100644

--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
+static bool i40e_gso_check(struct sk_buff *skb, struct net_device *dev)
+{
+       if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
+           (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
+            skb->inner_protocol != htons(ETH_P_TEB) ||
+            skb_inner_mac_header(skb) - skb_transport_header(skb) > 64))
+               return false;

I think it may be possible to even support a few more things here.
According to the datasheet here:
http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/xl71
0-10-40-controller-datasheet.pdf

This can actually support 64 bytes beyond the tunnel header, which
would make for a total of 80 bytes. It looks like it can also support
IPv4 or IPv6 beyond just Ethernet as the encapsulated protocol.

Intel guys, can you confirm that this is correct?

I'm just respinning this for v4/6 beyond GRE/UDP tunnel and IPIP, and I found 
the description of max protocol parsing size of 480B (with individual header 
limit of 255B). I couldn't find where you get this 64/80 number or which 
headers it maps to. Could you (or one of the intel guys) expand on this?

On Thu, Nov 20, 2014 at 11:16 AM, Joe Stringer [off-list ref] wrote:

On Tuesday, November 04, 2014 15:45:22 Jesse Gross wrote:

quoted

On Tue, Nov 4, 2014 at 1:56 PM, Joe Stringer [off-list ref] wrote:

quoted

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c

b/drivers/net/ethernet/intel/i40e/i40e_main.c index c3a7f4a..21829b5
100644

--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
+static bool i40e_gso_check(struct sk_buff *skb, struct net_device *dev)
+{
+       if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
+           (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
+            skb->inner_protocol != htons(ETH_P_TEB) ||
+            skb_inner_mac_header(skb) - skb_transport_header(skb) > 64))
+               return false;

I think it may be possible to even support a few more things here.
According to the datasheet here:
http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/xl71
0-10-40-controller-datasheet.pdf

This can actually support 64 bytes beyond the tunnel header, which
would make for a total of 80 bytes. It looks like it can also support
IPv4 or IPv6 beyond just Ethernet as the encapsulated protocol.

Intel guys, can you confirm that this is correct?

I'm just respinning this for v4/6 beyond GRE/UDP tunnel and IPIP, and I found
the description of max protocol parsing size of 480B (with individual header
limit of 255B). I couldn't find where you get this 64/80 number or which
headers it maps to. Could you (or one of the intel guys) expand on this?

The number that I gave was from the section on Geneve support (on page
708), which says that it can support up to 64 bytes of options (this
was also my understanding from previous conversations with Intel
guys). I searched for 480 byte limit and it seems like it for receive
instead of transmit, which could conceivably be different.