Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 0/3] igb, ixgbe, i40e UDP segmentation offload support
@ 2019-10-11  0:24 Josh Hunt
  2019-10-11  0:25 ` [PATCH v2 1/3] igb: Add " Josh Hunt
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Josh Hunt @ 2019-10-11  0:24 UTC (permalink / raw)
  To: netdev, intel-wired-lan, jeffrey.t.kirsher
  Cc: willemb, sridhar.samudrala, aaron.f.brown, alexander.h.duyck, Josh Hunt

Alexander Duyck posted a series in 2018 proposing adding UDP segmentation
offload support to ixgbe and ixgbevf, but those patches were never
accepted:

https://lore.kernel.org/netdev/20180504003556.4769.11407.stgit@localhost.localdomain/

This series is a repost of his ixgbe patch along with a similar
change to the igb and i40e drivers. Testing using the udpgso_bench_tx
benchmark shows a noticeable performance improvement with these changes
applied.

I've shared the benchmark script I've used to generate the #s in this mail:
https://lore.kernel.org/netdev/0e0e706c-4ce9-c27a-af55-339b4eb6d524@akamai.com/T/#mfe4c35c57a3860cda5306b63f61068f837242ee5

v2 changes:
 * Added Alex's suggested-by tag to ixgbe patch
 * Checking for gso_type, suggested by Sridhar
 * Fixed bug in ixgbe patch where I accidentally left the old type_tucmd
   set to TCP
 * Updated perf #s below
   * includes changes above
   * fixed bad config on my igb machine
   * added chip used for testing next to driver name

All #s below were run with:
udpgso_bench_tx -C 1 -4 -D 172.25.43.133 -z -l 30 -S 0 -u -s $pkt_size

igb (i210)::
SW GSO (ethtool -K eth0 tx-udp-segmentation off):
$pkt_size	kB/s(sar)	MB/s	Calls/s	Msg/s	CPU	MB2CPU
========================================================================
1472		120167.13	114	81278	81278	16.24	7.01
2944		120166.83	114	40641	40641	12.82	8.89
5888		120166.42	114	20320	20320	10.14	11.24
11776		120166.43	114	10160	10160	8.55	13.33
23552		120167.24	114	5080	5080	7.76	14.69
47104		120166.83	114	2540	2540	7.07	16.12
61824		120167.08	114	1935	1935	7.07	16.12

HW GSO offload (ethtool -K eth0 tx-udp-segmentation on):
$pkt_size	kB/s(sar)	MB/s	Calls/s	Msg/s	CPU	MB2CPU
========================================================================
1472		120167.05	114	81276	81276	16.13	7.06
2944		120166.94	114	40640	40640	8.66	13.16
5888		120166.84	114	20320	20320	5.34	21.34
11776		120166.84	114	10160	10160	3.34	34.13
23552		120167.25	114	5080	5080	2.55	44.70
47104		120149.30	113	2539	2539	2.14	52.80
61824		120165.41	114	1935	1935	2.04	55.88

ixgbe (82599ES)::
SW GSO:
$pkt_size	kB/s(sar)	MB/s	Calls/s	Msg/s	CPU	MB2CPU
========================================================================
1472		1043972.83	990	706122	706122	100	9.90
2944		1201623.17	1140	406367	406367	95.97	11.87
5888		1201629.46	1140	203184	203184	56	20.35
11776		1201631.38	1140	101590	101590	42.72	26.68
23552		1201633.94	1140	50796	50796	36.31	31.39
47104		1201631.05	1140	25397	25397	33.91	33.61
61824		1201629.96	1140	19350	19350	33.38	34.15

HW GSO Offload:
$pkt_size	kB/s(sar)	MB/s	Calls/s	Msg/s	CPU	MB2CPU
========================================================================
1472		1053393.41	999	712679	712679	100	9.99
2944		1201631.19	1140	406357	406357	57.92	19.68
5888		1201622.51	1140	203178	203178	30.66	37.18
11776		1201626.60	1140	101590	101590	16.89	67.49
23552		1201617.19	1140	50795	50795	10.11	112.75
47104		1200218.72	1138	25368	25368	6.86	165.88
61824		1201619.66	1140	19350	19350	5.38	211.89

i40e (x710)::
SW GSO:
$pkt_size	kB/s(sar)	MB/s	Calls/s	Msg/s	CPU	MB2CPU
========================================================================
1472		642718.15	609	434585	434585	100	6.09
2944		957988.80	909	324029	324029	100	9.09
5888		1199207.69	1138	202845	202845	81.51	13.96
11776		1200767.60	1139	101517	101517	63.93	17.81
23552		1201197.22	1140	50794	50794	59.14	19.27
47104		1201410.91	1139	25393	25393	57.15	19.93
61824		1201609.48	1140	19350	19350	55.12	20.68

HW GSO offload:
$pkt_size	kB/s(sar)	MB/s	Calls/s	Msg/s	CPU	MB2CPU
========================================================================
1472		666626.15	632	450920	450920	100	6.32
2944		1200831.63	1139	406093	406093	89.68	12.70
5888		1201603.35	1140	203178	203178	56.38	20.21
11776		1201597.29	1140	101588	101588	36.9	30.89
23552		1201596.98	1140	50794	50794	24.23	47.04
47104		1201491.67	1139	25394	25394	17.14	66.45
61824		1201598.88	1140	19350	19350	15.11	75.44

Josh Hunt (3):
  igb: Add UDP segmentation offload support
  ixgbe: Add UDP segmentation offload support
  i40e: Add UDP segmentation offload support

 drivers/net/ethernet/intel/i40e/i40e_main.c   |  1 +
 drivers/net/ethernet/intel/i40e/i40e_txrx.c   | 12 +++++++++---
 drivers/net/ethernet/intel/igb/e1000_82575.h  |  1 +
 drivers/net/ethernet/intel/igb/igb_main.c     | 23 +++++++++++++++++------
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 24 ++++++++++++++++++------
 5 files changed, 46 insertions(+), 15 deletions(-)

-- 
2.7.4


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 1/3] igb: Add UDP segmentation offload support
  2019-10-11  0:24 [PATCH v2 0/3] igb, ixgbe, i40e UDP segmentation offload support Josh Hunt
@ 2019-10-11  0:25 ` Josh Hunt
  2019-10-11 15:29   ` Alexander Duyck
  2019-10-11  0:25 ` [PATCH v2 2/3] ixgbe: " Josh Hunt
  2019-10-11  0:25 ` [PATCH v2 3/3] i40e: " Josh Hunt
  2 siblings, 1 reply; 6+ messages in thread
From: Josh Hunt @ 2019-10-11  0:25 UTC (permalink / raw)
  To: netdev, intel-wired-lan, jeffrey.t.kirsher
  Cc: willemb, sridhar.samudrala, aaron.f.brown, alexander.h.duyck,
	Josh Hunt, Alexander Duyck

Based on a series from Alexander Duyck this change adds UDP segmentation
offload support to the igb driver.

CC: Alexander Duyck <alexander.h.duyck@intel.com>
CC: Willem de Bruijn <willemb@google.com>
Signed-off-by: Josh Hunt <johunt@akamai.com>
---
 drivers/net/ethernet/intel/igb/e1000_82575.h |  1 +
 drivers/net/ethernet/intel/igb/igb_main.c    | 23 +++++++++++++++++------
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.h b/drivers/net/ethernet/intel/igb/e1000_82575.h
index 6ad775b1a4c5..63ec253ac788 100644
--- a/drivers/net/ethernet/intel/igb/e1000_82575.h
+++ b/drivers/net/ethernet/intel/igb/e1000_82575.h
@@ -127,6 +127,7 @@ struct e1000_adv_tx_context_desc {
 };
 
 #define E1000_ADVTXD_MACLEN_SHIFT    9  /* Adv ctxt desc mac len shift */
+#define E1000_ADVTXD_TUCMD_L4T_UDP 0x00000000  /* L4 Packet TYPE of UDP */
 #define E1000_ADVTXD_TUCMD_IPV4    0x00000400  /* IP Packet Type: 1=IPv4 */
 #define E1000_ADVTXD_TUCMD_L4T_TCP 0x00000800  /* L4 Packet TYPE of TCP */
 #define E1000_ADVTXD_TUCMD_L4T_SCTP 0x00001000 /* L4 packet TYPE of SCTP */
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 105b0624081a..4131bc8b079e 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -2516,6 +2516,7 @@ igb_features_check(struct sk_buff *skb, struct net_device *dev,
 	if (unlikely(mac_hdr_len > IGB_MAX_MAC_HDR_LEN))
 		return features & ~(NETIF_F_HW_CSUM |
 				    NETIF_F_SCTP_CRC |
+				    NETIF_F_GSO_UDP_L4 |
 				    NETIF_F_HW_VLAN_CTAG_TX |
 				    NETIF_F_TSO |
 				    NETIF_F_TSO6);
@@ -2524,6 +2525,7 @@ igb_features_check(struct sk_buff *skb, struct net_device *dev,
 	if (unlikely(network_hdr_len >  IGB_MAX_NETWORK_HDR_LEN))
 		return features & ~(NETIF_F_HW_CSUM |
 				    NETIF_F_SCTP_CRC |
+				    NETIF_F_GSO_UDP_L4 |
 				    NETIF_F_TSO |
 				    NETIF_F_TSO6);
 
@@ -3120,7 +3122,7 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 			    NETIF_F_HW_CSUM;
 
 	if (hw->mac.type >= e1000_82576)
-		netdev->features |= NETIF_F_SCTP_CRC;
+		netdev->features |= NETIF_F_SCTP_CRC | NETIF_F_GSO_UDP_L4;
 
 	if (hw->mac.type >= e1000_i350)
 		netdev->features |= NETIF_F_HW_TC;
@@ -5694,6 +5696,7 @@ static int igb_tso(struct igb_ring *tx_ring,
 	} ip;
 	union {
 		struct tcphdr *tcp;
+		struct udphdr *udp;
 		unsigned char *hdr;
 	} l4;
 	u32 paylen, l4_offset;
@@ -5713,7 +5716,8 @@ static int igb_tso(struct igb_ring *tx_ring,
 	l4.hdr = skb_checksum_start(skb);
 
 	/* ADV DTYP TUCMD MKRLOC/ISCSIHEDLEN */
-	type_tucmd = E1000_ADVTXD_TUCMD_L4T_TCP;
+	type_tucmd = (!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4)) ?
+		      E1000_ADVTXD_TUCMD_L4T_TCP : E1000_ADVTXD_TUCMD_L4T_UDP;
 
 	/* initialize outer IP header fields */
 	if (ip.v4->version == 4) {
@@ -5741,12 +5745,19 @@ static int igb_tso(struct igb_ring *tx_ring,
 	/* determine offset of inner transport header */
 	l4_offset = l4.hdr - skb->data;
 
-	/* compute length of segmentation header */
-	*hdr_len = (l4.tcp->doff * 4) + l4_offset;
-
 	/* remove payload length from inner checksum */
 	paylen = skb->len - l4_offset;
-	csum_replace_by_diff(&l4.tcp->check, htonl(paylen));
+	if (type_tucmd & E1000_ADVTXD_TUCMD_L4T_TCP) {
+		/* compute length of segmentation header */
+		*hdr_len = (l4.tcp->doff * 4) + l4_offset;
+		csum_replace_by_diff(&l4.tcp->check,
+			(__force __wsum)htonl(paylen));
+	} else {
+		/* compute length of segmentation header */
+		*hdr_len = sizeof(*l4.udp) + l4_offset;
+		csum_replace_by_diff(&l4.udp->check,
+				     (__force __wsum)htonl(paylen));
+	}
 
 	/* update gso size and bytecount with header size */
 	first->gso_segs = skb_shinfo(skb)->gso_segs;
-- 
2.7.4


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 2/3] ixgbe: Add UDP segmentation offload support
  2019-10-11  0:24 [PATCH v2 0/3] igb, ixgbe, i40e UDP segmentation offload support Josh Hunt
  2019-10-11  0:25 ` [PATCH v2 1/3] igb: Add " Josh Hunt
@ 2019-10-11  0:25 ` Josh Hunt
  2019-10-11  0:25 ` [PATCH v2 3/3] i40e: " Josh Hunt
  2 siblings, 0 replies; 6+ messages in thread
From: Josh Hunt @ 2019-10-11  0:25 UTC (permalink / raw)
  To: netdev, intel-wired-lan, jeffrey.t.kirsher
  Cc: willemb, sridhar.samudrala, aaron.f.brown, alexander.h.duyck,
	Josh Hunt, Alexander Duyck

Repost from a series by Alexander Duyck to add UDP segmentation offload
support to the igb driver:
https://lore.kernel.org/netdev/20180504003916.4769.66271.stgit@localhost.localdomain/

CC: Alexander Duyck <alexander.h.duyck@intel.com>
CC: Willem de Bruijn <willemb@google.com>
Suggested-by: Alexander Duyck <alexander.h.duyck@intel.com>
Signed-off-by: Josh Hunt <johunt@akamai.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 1ce2397306b9..7d50c1a4a3be 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7946,6 +7946,7 @@ static int ixgbe_tso(struct ixgbe_ring *tx_ring,
 	} ip;
 	union {
 		struct tcphdr *tcp;
+		struct udphdr *udp;
 		unsigned char *hdr;
 	} l4;
 	u32 paylen, l4_offset;
@@ -7969,7 +7970,8 @@ static int ixgbe_tso(struct ixgbe_ring *tx_ring,
 	l4.hdr = skb_checksum_start(skb);
 
 	/* ADV DTYP TUCMD MKRLOC/ISCSIHEDLEN */
-	type_tucmd = IXGBE_ADVTXD_TUCMD_L4T_TCP;
+	type_tucmd = (!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4)) ?
+		      IXGBE_ADVTXD_TUCMD_L4T_TCP : IXGBE_ADVTXD_TUCMD_L4T_UDP;
 
 	/* initialize outer IP header fields */
 	if (ip.v4->version == 4) {
@@ -7999,12 +8001,20 @@ static int ixgbe_tso(struct ixgbe_ring *tx_ring,
 	/* determine offset of inner transport header */
 	l4_offset = l4.hdr - skb->data;
 
-	/* compute length of segmentation header */
-	*hdr_len = (l4.tcp->doff * 4) + l4_offset;
-
 	/* remove payload length from inner checksum */
 	paylen = skb->len - l4_offset;
-	csum_replace_by_diff(&l4.tcp->check, (__force __wsum)htonl(paylen));
+
+	if (type_tucmd & IXGBE_ADVTXD_TUCMD_L4T_TCP) {
+		/* compute length of segmentation header */
+		*hdr_len = (l4.tcp->doff * 4) + l4_offset;
+		csum_replace_by_diff(&l4.tcp->check,
+				     (__force __wsum)htonl(paylen));
+	} else {
+		/* compute length of segmentation header */
+		*hdr_len = sizeof(*l4.udp) + l4_offset;
+		csum_replace_by_diff(&l4.udp->check,
+				     (__force __wsum)htonl(paylen));
+	}
 
 	/* update gso size and bytecount with header size */
 	first->gso_segs = skb_shinfo(skb)->gso_segs;
@@ -10190,6 +10200,7 @@ ixgbe_features_check(struct sk_buff *skb, struct net_device *dev,
 	if (unlikely(mac_hdr_len > IXGBE_MAX_MAC_HDR_LEN))
 		return features & ~(NETIF_F_HW_CSUM |
 				    NETIF_F_SCTP_CRC |
+				    NETIF_F_GSO_UDP_L4 |
 				    NETIF_F_HW_VLAN_CTAG_TX |
 				    NETIF_F_TSO |
 				    NETIF_F_TSO6);
@@ -10198,6 +10209,7 @@ ixgbe_features_check(struct sk_buff *skb, struct net_device *dev,
 	if (unlikely(network_hdr_len >  IXGBE_MAX_NETWORK_HDR_LEN))
 		return features & ~(NETIF_F_HW_CSUM |
 				    NETIF_F_SCTP_CRC |
+				    NETIF_F_GSO_UDP_L4 |
 				    NETIF_F_TSO |
 				    NETIF_F_TSO6);
 
@@ -10907,7 +10919,7 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 			    IXGBE_GSO_PARTIAL_FEATURES;
 
 	if (hw->mac.type >= ixgbe_mac_82599EB)
-		netdev->features |= NETIF_F_SCTP_CRC;
+		netdev->features |= NETIF_F_SCTP_CRC | NETIF_F_GSO_UDP_L4;
 
 #ifdef CONFIG_IXGBE_IPSEC
 #define IXGBE_ESP_FEATURES	(NETIF_F_HW_ESP | \
-- 
2.7.4


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 3/3] i40e: Add UDP segmentation offload support
  2019-10-11  0:24 [PATCH v2 0/3] igb, ixgbe, i40e UDP segmentation offload support Josh Hunt
  2019-10-11  0:25 ` [PATCH v2 1/3] igb: Add " Josh Hunt
  2019-10-11  0:25 ` [PATCH v2 2/3] ixgbe: " Josh Hunt
@ 2019-10-11  0:25 ` Josh Hunt
  2 siblings, 0 replies; 6+ messages in thread
From: Josh Hunt @ 2019-10-11  0:25 UTC (permalink / raw)
  To: netdev, intel-wired-lan, jeffrey.t.kirsher
  Cc: willemb, sridhar.samudrala, aaron.f.brown, alexander.h.duyck,
	Josh Hunt, Alexander Duyck

Based on a series from Alexander Duyck this change adds UDP segmentation
offload support to the i40e driver.

CC: Alexander Duyck <alexander.h.duyck@intel.com>
CC: Willem de Bruijn <willemb@google.com>
Signed-off-by: Josh Hunt <johunt@akamai.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c |  1 +
 drivers/net/ethernet/intel/i40e/i40e_txrx.c | 12 +++++++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 6031223eafab..56f8c52cbba1 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -12911,6 +12911,7 @@ static int i40e_config_netdev(struct i40e_vsi *vsi)
 			  NETIF_F_GSO_IPXIP6		|
 			  NETIF_F_GSO_UDP_TUNNEL	|
 			  NETIF_F_GSO_UDP_TUNNEL_CSUM	|
+			  NETIF_F_GSO_UDP_L4		|
 			  NETIF_F_SCTP_CRC		|
 			  NETIF_F_RXHASH		|
 			  NETIF_F_RXCSUM		|
diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index e3f29dc8b290..2250284e27fd 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -2960,10 +2960,16 @@ static int i40e_tso(struct i40e_tx_buffer *first, u8 *hdr_len,
 
 	/* remove payload length from inner checksum */
 	paylen = skb->len - l4_offset;
-	csum_replace_by_diff(&l4.tcp->check, (__force __wsum)htonl(paylen));
 
-	/* compute length of segmentation header */
-	*hdr_len = (l4.tcp->doff * 4) + l4_offset;
+	if (!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4)) {
+		csum_replace_by_diff(&l4.tcp->check, (__force __wsum)htonl(paylen));
+		/* compute length of segmentation header */
+		*hdr_len = (l4.tcp->doff * 4) + l4_offset;
+	} else {
+		csum_replace_by_diff(&l4.udp->check, (__force __wsum)htonl(paylen));
+		/* compute length of segmentation header */
+		*hdr_len = sizeof(*l4.udp) + l4_offset;
+	}
 
 	/* pull values out of skb_shinfo */
 	gso_size = skb_shinfo(skb)->gso_size;
-- 
2.7.4


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/3] igb: Add UDP segmentation offload support
  2019-10-11  0:25 ` [PATCH v2 1/3] igb: Add " Josh Hunt
@ 2019-10-11 15:29   ` Alexander Duyck
  2019-10-11 16:18     ` Josh Hunt
  0 siblings, 1 reply; 6+ messages in thread
From: Alexander Duyck @ 2019-10-11 15:29 UTC (permalink / raw)
  To: Josh Hunt, netdev, intel-wired-lan, jeffrey.t.kirsher
  Cc: willemb, sridhar.samudrala, aaron.f.brown

On Thu, 2019-10-10 at 20:25 -0400, Josh Hunt wrote:
> Based on a series from Alexander Duyck this change adds UDP segmentation
> offload support to the igb driver.
> 
> CC: Alexander Duyck <alexander.h.duyck@intel.com>
> CC: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Josh Hunt <johunt@akamai.com>
> ---
>  drivers/net/ethernet/intel/igb/e1000_82575.h |  1 +
>  drivers/net/ethernet/intel/igb/igb_main.c    | 23 +++++++++++++++++------
>  2 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.h b/drivers/net/ethernet/intel/igb/e1000_82575.h
> index 6ad775b1a4c5..63ec253ac788 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_82575.h
> +++ b/drivers/net/ethernet/intel/igb/e1000_82575.h
> @@ -127,6 +127,7 @@ struct e1000_adv_tx_context_desc {
>  };
>  
>  #define E1000_ADVTXD_MACLEN_SHIFT    9  /* Adv ctxt desc mac len shift */
> +#define E1000_ADVTXD_TUCMD_L4T_UDP 0x00000000  /* L4 Packet TYPE of UDP */
>  #define E1000_ADVTXD_TUCMD_IPV4    0x00000400  /* IP Packet Type: 1=IPv4 */
>  #define E1000_ADVTXD_TUCMD_L4T_TCP 0x00000800  /* L4 Packet TYPE of TCP */
>  #define E1000_ADVTXD_TUCMD_L4T_SCTP 0x00001000 /* L4 packet TYPE of SCTP */
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 105b0624081a..4131bc8b079e 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -2516,6 +2516,7 @@ igb_features_check(struct sk_buff *skb, struct net_device *dev,
>  	if (unlikely(mac_hdr_len > IGB_MAX_MAC_HDR_LEN))
>  		return features & ~(NETIF_F_HW_CSUM |
>  				    NETIF_F_SCTP_CRC |
> +				    NETIF_F_GSO_UDP_L4 |
>  				    NETIF_F_HW_VLAN_CTAG_TX |
>  				    NETIF_F_TSO |
>  				    NETIF_F_TSO6);
> @@ -2524,6 +2525,7 @@ igb_features_check(struct sk_buff *skb, struct net_device *dev,
>  	if (unlikely(network_hdr_len >  IGB_MAX_NETWORK_HDR_LEN))
>  		return features & ~(NETIF_F_HW_CSUM |
>  				    NETIF_F_SCTP_CRC |
> +				    NETIF_F_GSO_UDP_L4 |
>  				    NETIF_F_TSO |
>  				    NETIF_F_TSO6);
>  
> @@ -3120,7 +3122,7 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  			    NETIF_F_HW_CSUM;
>  
>  	if (hw->mac.type >= e1000_82576)
> -		netdev->features |= NETIF_F_SCTP_CRC;
> +		netdev->features |= NETIF_F_SCTP_CRC | NETIF_F_GSO_UDP_L4;
>  
>  	if (hw->mac.type >= e1000_i350)
>  		netdev->features |= NETIF_F_HW_TC;
> @@ -5694,6 +5696,7 @@ static int igb_tso(struct igb_ring *tx_ring,
>  	} ip;
>  	union {
>  		struct tcphdr *tcp;
> +		struct udphdr *udp;
>  		unsigned char *hdr;
>  	} l4;
>  	u32 paylen, l4_offset;
> @@ -5713,7 +5716,8 @@ static int igb_tso(struct igb_ring *tx_ring,
>  	l4.hdr = skb_checksum_start(skb);
>  
>  	/* ADV DTYP TUCMD MKRLOC/ISCSIHEDLEN */
> -	type_tucmd = E1000_ADVTXD_TUCMD_L4T_TCP;
> +	type_tucmd = (!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4)) ?
> +		      E1000_ADVTXD_TUCMD_L4T_TCP : E1000_ADVTXD_TUCMD_L4T_UDP;

The logic here seems a bit convoluted. Instead of testing for
'!SKB_GSO_UDP_L4' why not just make L4T_UDP the first option and drop the
'!'? That will make the TCP offload the default case rather than the UDP
offload.

The same applies to the other 2 patches.

>  	/* initialize outer IP header fields */
>  	if (ip.v4->version == 4) {
> @@ -5741,12 +5745,19 @@ static int igb_tso(struct igb_ring *tx_ring,
>  	/* determine offset of inner transport header */
>  	l4_offset = l4.hdr - skb->data;
>  
> -	/* compute length of segmentation header */
> -	*hdr_len = (l4.tcp->doff * 4) + l4_offset;
> -
>  	/* remove payload length from inner checksum */
>  	paylen = skb->len - l4_offset;
> -	csum_replace_by_diff(&l4.tcp->check, htonl(paylen));
> +	if (type_tucmd & E1000_ADVTXD_TUCMD_L4T_TCP) {
> +		/* compute length of segmentation header */
> +		*hdr_len = (l4.tcp->doff * 4) + l4_offset;
> +		csum_replace_by_diff(&l4.tcp->check,
> +			(__force __wsum)htonl(paylen));
> +	} else {
> +		/* compute length of segmentation header */
> +		*hdr_len = sizeof(*l4.udp) + l4_offset;
> +		csum_replace_by_diff(&l4.udp->check,
> +				     (__force __wsum)htonl(paylen));
> +	}
>  
>  	/* update gso size and bytecount with header size */
>  	first->gso_segs = skb_shinfo(skb)->gso_segs;



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/3] igb: Add UDP segmentation offload support
  2019-10-11 15:29   ` Alexander Duyck
@ 2019-10-11 16:18     ` Josh Hunt
  0 siblings, 0 replies; 6+ messages in thread
From: Josh Hunt @ 2019-10-11 16:18 UTC (permalink / raw)
  To: Alexander Duyck, netdev, intel-wired-lan, jeffrey.t.kirsher
  Cc: willemb, sridhar.samudrala, aaron.f.brown



On 10/11/19 8:29 AM, Alexander Duyck wrote:
> On Thu, 2019-10-10 at 20:25 -0400, Josh Hunt wrote:
>> Based on a series from Alexander Duyck this change adds UDP segmentation
>> offload support to the igb driver.
>>
>> CC: Alexander Duyck <alexander.h.duyck@intel.com>
>> CC: Willem de Bruijn <willemb@google.com>
>> Signed-off-by: Josh Hunt <johunt@akamai.com>
>> ---
>>   drivers/net/ethernet/intel/igb/e1000_82575.h |  1 +
>>   drivers/net/ethernet/intel/igb/igb_main.c    | 23 +++++++++++++++++------
>>   2 files changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.h b/drivers/net/ethernet/intel/igb/e1000_82575.h
>> index 6ad775b1a4c5..63ec253ac788 100644
>> --- a/drivers/net/ethernet/intel/igb/e1000_82575.h
>> +++ b/drivers/net/ethernet/intel/igb/e1000_82575.h
>> @@ -127,6 +127,7 @@ struct e1000_adv_tx_context_desc {
>>   };
>>   
>>   #define E1000_ADVTXD_MACLEN_SHIFT    9  /* Adv ctxt desc mac len shift */
>> +#define E1000_ADVTXD_TUCMD_L4T_UDP 0x00000000  /* L4 Packet TYPE of UDP */
>>   #define E1000_ADVTXD_TUCMD_IPV4    0x00000400  /* IP Packet Type: 1=IPv4 */
>>   #define E1000_ADVTXD_TUCMD_L4T_TCP 0x00000800  /* L4 Packet TYPE of TCP */
>>   #define E1000_ADVTXD_TUCMD_L4T_SCTP 0x00001000 /* L4 packet TYPE of SCTP */
>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
>> index 105b0624081a..4131bc8b079e 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>> @@ -2516,6 +2516,7 @@ igb_features_check(struct sk_buff *skb, struct net_device *dev,
>>   	if (unlikely(mac_hdr_len > IGB_MAX_MAC_HDR_LEN))
>>   		return features & ~(NETIF_F_HW_CSUM |
>>   				    NETIF_F_SCTP_CRC |
>> +				    NETIF_F_GSO_UDP_L4 |
>>   				    NETIF_F_HW_VLAN_CTAG_TX |
>>   				    NETIF_F_TSO |
>>   				    NETIF_F_TSO6);
>> @@ -2524,6 +2525,7 @@ igb_features_check(struct sk_buff *skb, struct net_device *dev,
>>   	if (unlikely(network_hdr_len >  IGB_MAX_NETWORK_HDR_LEN))
>>   		return features & ~(NETIF_F_HW_CSUM |
>>   				    NETIF_F_SCTP_CRC |
>> +				    NETIF_F_GSO_UDP_L4 |
>>   				    NETIF_F_TSO |
>>   				    NETIF_F_TSO6);
>>   
>> @@ -3120,7 +3122,7 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>   			    NETIF_F_HW_CSUM;
>>   
>>   	if (hw->mac.type >= e1000_82576)
>> -		netdev->features |= NETIF_F_SCTP_CRC;
>> +		netdev->features |= NETIF_F_SCTP_CRC | NETIF_F_GSO_UDP_L4;
>>   
>>   	if (hw->mac.type >= e1000_i350)
>>   		netdev->features |= NETIF_F_HW_TC;
>> @@ -5694,6 +5696,7 @@ static int igb_tso(struct igb_ring *tx_ring,
>>   	} ip;
>>   	union {
>>   		struct tcphdr *tcp;
>> +		struct udphdr *udp;
>>   		unsigned char *hdr;
>>   	} l4;
>>   	u32 paylen, l4_offset;
>> @@ -5713,7 +5716,8 @@ static int igb_tso(struct igb_ring *tx_ring,
>>   	l4.hdr = skb_checksum_start(skb);
>>   
>>   	/* ADV DTYP TUCMD MKRLOC/ISCSIHEDLEN */
>> -	type_tucmd = E1000_ADVTXD_TUCMD_L4T_TCP;
>> +	type_tucmd = (!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4)) ?
>> +		      E1000_ADVTXD_TUCMD_L4T_TCP : E1000_ADVTXD_TUCMD_L4T_UDP;
> 
> The logic here seems a bit convoluted. Instead of testing for
> '!SKB_GSO_UDP_L4' why not just make L4T_UDP the first option and drop the
> '!'? That will make the TCP offload the default case rather than the UDP
> offload.
> 
> The same applies to the other 2 patches.

OK sure. I think I was trying to keep the TCP case first for some 
reason, but agree that I should probably just reverse the order. Will 
send a v3.

Josh

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-11  0:24 [PATCH v2 0/3] igb, ixgbe, i40e UDP segmentation offload support Josh Hunt
2019-10-11  0:25 ` [PATCH v2 1/3] igb: Add " Josh Hunt
2019-10-11 15:29   ` Alexander Duyck
2019-10-11 16:18     ` Josh Hunt
2019-10-11  0:25 ` [PATCH v2 2/3] ixgbe: " Josh Hunt
2019-10-11  0:25 ` [PATCH v2 3/3] i40e: " Josh Hunt

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git