RSS fix for ixgbe/ixgbevf

PATCH Superseded Fan Du 3 participants 7 messages Apr 23, 2015 – Apr 28, 2015 1 message not in the archive

Subsystems (2)

Network drivers Intel Ethernet

lore

Share & tools

lore

Fan Du

author

2015-04-23 07:31 UTC #0 lore raw

cover RSS fix for ixgbe/ixgbevf

This patchset provide RSS hash value for upper stack by indicating whether it's a L3 or L4 hash. Thus other component of network stack could utilize this hash value providing by NIC without caculating by themselves.

Test:
* Simple iperf test using ixgbe for 82599ES
* Build test only for ixgbevf
* make C=1 CF=-D__CHECK_ENDIAN__, no warnings introduced by this patchset

Fan Du (2):
ixgbe: Specify rx hash type wrt rx desc RSS type
ixgbevf: Set rx hash type for ingress packets

 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |   25 ++++++++++++++++---
 drivers/net/ethernet/intel/ixgbevf/defines.h      |   12 +++++++++
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   27 +++++++++++++++++++++
 3 files changed, 60 insertions(+), 4 deletions(-)

Fan Du

author

2015-04-23 07:31 UTC #1 lore raw

PATCH 1/2 ixgbe: Specify rx hash type wrt rx desc RSS type

RSS could be leveraged by taking account L4 src/dst ports as ingredients, thus ingress skb rx hash type should honor such the real configuration.

Signed-off-by Fan Du <[email protected]>

---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   25 +++++++++++++++++++++----
 1 files changed, 21 insertions(+), 4 deletions(-)

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +21 -4

index d3f4b0c..5df896b 100644
@@ -1357,14 +1357,31 @@ static int __ixgbe_notify_dca(struct device *dev, void *data)
 }
 
 #endif /* CONFIG_IXGBE_DCA */
+
+#define IXGBE_RSS_L4_TYPES_MASK \
+	((1ul << IXGBE_RXDADV_RSSTYPE_IPV4_TCP) | \
+	 (1ul << IXGBE_RXDADV_RSSTYPE_IPV4_UDP) | \
+	 (1ul << IXGBE_RXDADV_RSSTYPE_IPV6_TCP) | \
+	 (1ul << IXGBE_RXDADV_RSSTYPE_IPV6_UDP))
+
 static inline void ixgbe_rx_hash(struct ixgbe_ring *ring,
 				 union ixgbe_adv_rx_desc *rx_desc,
 				 struct sk_buff *skb)
 {
-	if (ring->netdev->features & NETIF_F_RXHASH)
-		skb_set_hash(skb,
-			     le32_to_cpu(rx_desc->wb.lower.hi_dword.rss),
-			     PKT_HASH_TYPE_L3);
+	u16 rss_type;
+
+	if (!(ring->netdev->features & NETIF_F_RXHASH))
+		return;
+
+	rss_type = le16_to_cpu(rx_desc->wb.lower.lo_dword.hs_rss.pkt_info) &
+		   IXGBE_RXDADV_RSSTYPE_MASK;
+
+	if (!rss_type)
+		return;
+
+	skb_set_hash(skb, le32_to_cpu(rx_desc->wb.lower.hi_dword.rss),
+		     (IXGBE_RSS_L4_TYPES_MASK & (1ul << rss_type)) ?
+		     PKT_HASH_TYPE_L4 : PKT_HASH_TYPE_L3);
 }
 
 #ifdef IXGBE_FCOE

Signature · 1 line

1.7.1

Alexander Duyck

2015-04-27 19:41 UTC #2 lore raw

PATCH 1/2 ixgbe: Specify rx hash type wrt rx desc RSS type

Quoted 39 lines · Fan Du

RSS could be leveraged by taking account L4 src/dst ports as ingredients, thus ingress skb rx hash type should honor such the real configuration.

Signed-off-by Fan Du <[email protected]>

---
  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   25 +++++++++++++++++++++----
  1 files changed, 21 insertions(+), 4 deletions(-)

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +14 -4

index d3f4b0c..5df896b 100644
@@ -1357,14 +1357,31 @@ static int __ixgbe_notify_dca(struct device *dev, void *data)
  }
 
  #endif /* CONFIG_IXGBE_DCA */
+
+#define IXGBE_RSS_L4_TYPES_MASK \
+	((1ul << IXGBE_RXDADV_RSSTYPE_IPV4_TCP) | \
+	 (1ul << IXGBE_RXDADV_RSSTYPE_IPV4_UDP) | \
+	 (1ul << IXGBE_RXDADV_RSSTYPE_IPV6_TCP) | \
+	 (1ul << IXGBE_RXDADV_RSSTYPE_IPV6_UDP))
+
  static inline void ixgbe_rx_hash(struct ixgbe_ring *ring,
  				 union ixgbe_adv_rx_desc *rx_desc,
  				 struct sk_buff *skb)
  {
-	if (ring->netdev->features & NETIF_F_RXHASH)
-		skb_set_hash(skb,
-			     le32_to_cpu(rx_desc->wb.lower.hi_dword.rss),
-			     PKT_HASH_TYPE_L3);
+	u16 rss_type;
+
+	if (!(ring->netdev->features & NETIF_F_RXHASH))
+		return;
+
+	rss_type = le16_to_cpu(rx_desc->wb.lower.lo_dword.hs_rss.pkt_info) &
+		   IXGBE_RXDADV_RSSTYPE_MASK;

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c static int __ixgbe_notify_dca(struct device *dev, void *data) · line 1387

Quoted 10 lines

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +7 -0

+
+	if (!rss_type)
+		return;
+
+	skb_set_hash(skb, le32_to_cpu(rx_desc->wb.lower.hi_dword.rss),
+		     (IXGBE_RSS_L4_TYPES_MASK & (1ul << rss_type)) ?
+		     PKT_HASH_TYPE_L4 : PKT_HASH_TYPE_L3);
  }
 
  #ifdef IXGBE_FCOE

The patch itself here is fine, however there is a complication in that with the current configuration ntuple filters will cause the RSS hash to be overwritten with the signature and bucket hash values. In order to resolve that you need to update ixgbe_init_fdir_perfect_82599 so that it doesn't set the IXGBE_FDIRCTRL_REPORT_STATUS bit. Otherwise there is the potential for an IPv4 rule to be defined that would set the same hash value for multiple flows.

- Alex

Du

2015-04-28 06:43 UTC #3 lore raw

PATCH 1/2 ixgbe: Specify rx hash type wrt rx desc RSS type

Quoted 63 lines

-----Original Message-----
From: Alexander Duyck [mailto:alexander.h.duyck at redhat.com]
Sent: Tuesday, April 28, 2015 3:41 AM
To: Du, Fan; Kirsher, Jeffrey T
Cc: e1000-devel at lists.sourceforge.net; intel-wired-lan at lists.osuosl.org
Subject: Re: [Intel-wired-lan] [PATCH 1/2] ixgbe: Specify rx hash type wrt rx desc RSS type
Quoted 7 lines · Fan Du
RSS could be leveraged by taking account L4 src/dst ports as ingredients, thus ingress skb rx hash type should honor such the real configuration.
Signed-off-by Fan Du <[email protected]>
---
  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   25
+++++++++++++++++++++----
Review
Quoted 3 lines
  1 files changed, 21 insertions(+), 4 deletions(-)
(quoted diff) +0 -0
b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c static int __ixgbe_notify_dca(struct device *dev,
Quoted 4 lines
index d3f4b0c..5df896b 100644
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0 -0
@@ -1357,14 +1357,31 @@ static int __ixgbe_notify_dca(struct device *dev,
void *data)
Quoted 34 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +21 -4
  }
 
  #endif /* CONFIG_IXGBE_DCA */
+
+#define IXGBE_RSS_L4_TYPES_MASK \
+	((1ul << IXGBE_RXDADV_RSSTYPE_IPV4_TCP) | \
+	 (1ul << IXGBE_RXDADV_RSSTYPE_IPV4_UDP) | \
+	 (1ul << IXGBE_RXDADV_RSSTYPE_IPV6_TCP) | \
+	 (1ul << IXGBE_RXDADV_RSSTYPE_IPV6_UDP))
+
  static inline void ixgbe_rx_hash(struct ixgbe_ring *ring,
  				 union ixgbe_adv_rx_desc *rx_desc,
  				 struct sk_buff *skb)
  {
-	if (ring->netdev->features & NETIF_F_RXHASH)
-		skb_set_hash(skb,
-			     le32_to_cpu(rx_desc->wb.lower.hi_dword.rss),
-			     PKT_HASH_TYPE_L3);
+	u16 rss_type;
+
+	if (!(ring->netdev->features & NETIF_F_RXHASH))
+		return;
+
+	rss_type = le16_to_cpu(rx_desc->wb.lower.lo_dword.hs_rss.pkt_info) &
+		   IXGBE_RXDADV_RSSTYPE_MASK;
+
+	if (!rss_type)
+		return;
+
+	skb_set_hash(skb, le32_to_cpu(rx_desc->wb.lower.hi_dword.rss),
+		     (IXGBE_RSS_L4_TYPES_MASK & (1ul << rss_type)) ?
+		     PKT_HASH_TYPE_L4 : PKT_HASH_TYPE_L3);
  }
 

Quoted 9 lines

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c static int __ixgbe_notify_dca(struct device *dev, void *data) · line 1387
Quoted 1 line
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0 -0
  #ifdef IXGBE_FCOE
The patch itself here is fine, however there is a complication in that with the current configuration ntuple filters will cause the RSS hash to be overwritten with the signature and bucket hash values. In order to resolve that you need to update ixgbe_init_fdir_perfect_82599 so that it doesn't set the IXGBE_FDIRCTRL_REPORT_STATUS bit. Otherwise there is the potential for an IPv4 rule to be defined that would set the same hash value for multiple flows.

Yes, flow director filters mechanism could set its own hash value, which is unionized with RSS hash value. But by my understanding, if a packet is filtered by flow director, RSS_TYPE in the write back descriptor won't be set, meaning rss_type is ZERO, and here the patch has covered this by checking whether rss_type is true or not.

Quoted 1 line

- Alex

Alexander Duyck

2015-04-28 15:25 UTC #4 lore raw

PATCH 1/2 ixgbe: Specify rx hash type wrt rx desc RSS type

Quoted 71 lines · Fan

Quoted 62 lines
-----Original Message-----
From: Alexander Duyck [mailto:alexander.h.duyck at redhat.com]
Sent: Tuesday, April 28, 2015 3:41 AM
To: Du, Fan; Kirsher, Jeffrey T
Cc: e1000-devel at lists.sourceforge.net; intel-wired-lan at lists.osuosl.org
Subject: Re: [Intel-wired-lan] [PATCH 1/2] ixgbe: Specify rx hash type wrt rx desc RSS type
Quoted 7 lines · Fan Du
RSS could be leveraged by taking account L4 src/dst ports as ingredients, thus ingress skb rx hash type should honor such the real configuration.
Signed-off-by Fan Du <[email protected]>
---
   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   25
+++++++++++++++++++++----
Review
Quoted 3 lines
   1 files changed, 21 insertions(+), 4 deletions(-)
(quoted diff) +0 -0
b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c static int __ixgbe_notify_dca(struct device *dev,
Quoted 4 lines
index d3f4b0c..5df896b 100644
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0 -0
@@ -1357,14 +1357,31 @@ static int __ixgbe_notify_dca(struct device *dev,
void *data)
Quoted 34 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +21 -4
   }
 
   #endif /* CONFIG_IXGBE_DCA */
+
+#define IXGBE_RSS_L4_TYPES_MASK \
+	((1ul << IXGBE_RXDADV_RSSTYPE_IPV4_TCP) | \
+	 (1ul << IXGBE_RXDADV_RSSTYPE_IPV4_UDP) | \
+	 (1ul << IXGBE_RXDADV_RSSTYPE_IPV6_TCP) | \
+	 (1ul << IXGBE_RXDADV_RSSTYPE_IPV6_UDP))
+
   static inline void ixgbe_rx_hash(struct ixgbe_ring *ring,
   				 union ixgbe_adv_rx_desc *rx_desc,
   				 struct sk_buff *skb)
   {
-	if (ring->netdev->features & NETIF_F_RXHASH)
-		skb_set_hash(skb,
-			     le32_to_cpu(rx_desc->wb.lower.hi_dword.rss),
-			     PKT_HASH_TYPE_L3);
+	u16 rss_type;
+
+	if (!(ring->netdev->features & NETIF_F_RXHASH))
+		return;
+
+	rss_type = le16_to_cpu(rx_desc->wb.lower.lo_dword.hs_rss.pkt_info) &
+		   IXGBE_RXDADV_RSSTYPE_MASK;
+
+	if (!rss_type)
+		return;
+
+	skb_set_hash(skb, le32_to_cpu(rx_desc->wb.lower.hi_dword.rss),
+		     (IXGBE_RSS_L4_TYPES_MASK & (1ul << rss_type)) ?
+		     PKT_HASH_TYPE_L4 : PKT_HASH_TYPE_L3);
   }
 
Quoted 8 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c static int __ixgbe_notify_dca(struct device *dev, void *data) · line 1387
Quoted 1 line
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0 -0
   #ifdef IXGBE_FCOE
The patch itself here is fine, however there is a complication in that with the current configuration ntuple filters will cause the RSS hash to be overwritten with the signature and bucket hash values. In order to resolve that you need to update ixgbe_init_fdir_perfect_82599 so that it doesn't set the IXGBE_FDIRCTRL_REPORT_STATUS bit. Otherwise there is the potential for an IPv4 rule to be defined that would set the same hash value for multiple flows.

Quoted 6 lines

Yes, flow director filters mechanism could set its own hash value, which is unionized with RSS hash value. But by my understanding, if a packet is filtered by flow director, RSS_TYPE in the write back descriptor won't be set, meaning rss_type is ZERO, and here the patch has covered this by checking whether rss_type is true or not.

I just reread that section of the data sheet and it looks like you are correct. The RSS Type should be reported as 0xF in the case of a flow director filter being written to that location.

Still it might be useful to add one more patch so that you disable the flow director reporting since I don't believe there is anything that actually makes use of it, and it will limit the functionality you have added.

- Alex

Fan Du

author

2015-04-23 07:31 UTC #5 lore raw

PATCH 2/2 ixgbevf: Set rx hash type for ingress packets

In reply to Fan Du

Set hash type for ingress packets according to NIC advanced receive descriptors RSS type part.

Signed-off-by Fan Du <[email protected]>

---
 drivers/net/ethernet/intel/ixgbevf/defines.h      |   12 +++++++++
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   27 +++++++++++++++++++++
 2 files changed, 39 insertions(+), 0 deletions(-)

drivers/net/ethernet/intel/ixgbevf/defines.h +12 -0

index 770e21a..af28ef3 100644
@@ -161,6 +161,18 @@ typedef u32 ixgbe_link_speed;
 #define IXGBE_RXDADV_SPLITHEADER_EN	0x00001000
 #define IXGBE_RXDADV_SPH		0x8000
 
+/* RSS Hash results */
+#define IXGBE_RXDADV_RSSTYPE_NONE       0x00000000
+#define IXGBE_RXDADV_RSSTYPE_IPV4_TCP   0x00000001
+#define IXGBE_RXDADV_RSSTYPE_IPV4       0x00000002
+#define IXGBE_RXDADV_RSSTYPE_IPV6_TCP   0x00000003
+#define IXGBE_RXDADV_RSSTYPE_IPV6_EX    0x00000004
+#define IXGBE_RXDADV_RSSTYPE_IPV6       0x00000005
+#define IXGBE_RXDADV_RSSTYPE_IPV6_TCP_EX 0x00000006
+#define IXGBE_RXDADV_RSSTYPE_IPV4_UDP   0x00000007
+#define IXGBE_RXDADV_RSSTYPE_IPV6_UDP   0x00000008
+#define IXGBE_RXDADV_RSSTYPE_IPV6_UDP_EX 0x00000009
+
 #define IXGBE_RXD_ERR_FRAME_ERR_MASK ( \
 				      IXGBE_RXD_ERR_CE |  \
 				      IXGBE_RXD_ERR_LE |  \

drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c +27 -0

index a16d267..2a4ae83 100644
@@ -457,6 +457,32 @@ static void ixgbevf_rx_skb(struct ixgbevf_q_vector *q_vector,
 	napi_gro_receive(&q_vector->napi, skb);
 }
 
+#define IXGBE_RSS_L4_TYPES_MASK \
+	((1ul << IXGBE_RXDADV_RSSTYPE_IPV4_TCP) | \
+	 (1ul << IXGBE_RXDADV_RSSTYPE_IPV4_UDP) | \
+	 (1ul << IXGBE_RXDADV_RSSTYPE_IPV6_TCP) | \
+	 (1ul << IXGBE_RXDADV_RSSTYPE_IPV6_UDP))
+
+static inline void ixgbevf_rx_hash(struct ixgbevf_ring *ring,
+				   union ixgbe_adv_rx_desc *rx_desc,
+				   struct sk_buff *skb)
+{
+	u16 rss_type;
+
+	if (!(ring->netdev->features & NETIF_F_RXHASH))
+		return;
+
+	rss_type = le16_to_cpu(rx_desc->wb.lower.lo_dword.hs_rss.pkt_info) &
+		   IXGBE_RXDADV_RSSTYPE_MASK;
+
+	if (!rss_type)
+		return;
+
+	skb_set_hash(skb, le32_to_cpu(rx_desc->wb.lower.hi_dword.rss),
+		     (IXGBE_RSS_L4_TYPES_MASK & (1ul << rss_type)) ?
+		     PKT_HASH_TYPE_L4 : PKT_HASH_TYPE_L3);
+}
+
 /**
  * ixgbevf_rx_checksum - indicate in skb if hw indicated a good cksum
  * @ring: structure containig ring specific data

@@ -506,6 +532,7 @@ static void ixgbevf_process_skb_fields(struct ixgbevf_ring *rx_ring,
 				       union ixgbe_adv_rx_desc *rx_desc,
 				       struct sk_buff *skb)
 {
+	ixgbevf_rx_hash(rx_ring, rx_desc, skb);
 	ixgbevf_rx_checksum(rx_ring, rx_desc, skb);
 
 	if (ixgbevf_test_staterr(rx_desc, IXGBE_RXD_STAT_VP)) {

Signature · 1 line

1.7.1

A message that is not in the archive (sent off-list or not yet received): D38E8E86660E514AB505863C19C9287C74187E5E@ORSMSX102.amr.corp.intel.com

Du

2015-04-26 08:02 UTC #7 lore raw

cover RSS fix for ixgbe/ixgbevf

Quoted 9 lines

-----Original Message-----
From: Ronciak, John
Sent: Thursday, April 23, 2015 11:50 PM
To: Du, Fan; Kirsher, Jeffrey T
Cc: Du, Fan; Guo, Chaohong
Subject: RE: [Intel-wired-lan] [PATCH 0/2] RSS fix for ixgbe/ixgbevf
Lists removed, Minskey added

Thanks for notifying Minskey about the extra effort for community that beyond my work scope. Nothing in your comments as an INTEL ETHERNET DRIVERS Reviewer should be out of mail list, so I insist all the discussion & challenges must be kept on the arena.

Quoted 4 lines

You have not explained why you want these patches. It's not obvious as to why the patches would be needed. You say what it does but you don't explain why this would be needed. Please redo the patch set and add the reasons why you want this functionality.

You didn't read the cover letter thoroughly, the second sentence explained why this patchset is needed as below:

Quoted 3 lines

Quoted 3 lines
This patchset provide RSS hash value for upper stack by indicating whether it's a L3 or L4 hash. Thus other component of network stack could utilize this hash value providing by NIC without calculating by themselves.

If you are not sure how upper stack will use skb hash, there is one scenario in OpenvSiwtch, Which skb L4 hash could be queried by OVS_ACTION_ATTR_HASH action. There are other Scenarios too, it's one grep of skb_get_hash away from why this functionality is need.

Btw, i40e/i40evf has already incorporates this functionality.

Quoted 37 lines

Cheers,
John
Quoted 19 lines
-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Fan Du
Sent: Thursday, April 23, 2015 12:31 AM
To: Kirsher, Jeffrey T
Cc: e1000-devel at lists.sourceforge.net; Du, Fan; intel-wired- lan at lists.osuosl.org
Subject: [Intel-wired-lan] [PATCH 0/2] RSS fix for ixgbe/ixgbevf
This patchset provide RSS hash value for upper stack by indicating whether it's a L3 or L4 hash. Thus other component of network stack could utilize this hash value providing by NIC without caculating by themselves.
Test:
* Simple iperf test using ixgbe for 82599ES
* Build test only for ixgbevf
* make C=1 CF=-D__CHECK_ENDIAN__, no warnings introduced by this patchset
Quoted 5 lines
Fan Du (2):
ixgbe: Specify rx hash type wrt rx desc RSS type
ixgbevf: Set rx hash type for ingress packets
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |   25
++++++++++++++++---
Quoted 9 lines
 drivers/net/ethernet/intel/ixgbevf/defines.h      |   12 +++++++++
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   27
+++++++++++++++++++++
 3 files changed, 60 insertions(+), 4 deletions(-)
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan at lists.osuosl.org
http://lists.osuosl.org/mailman/listinfo/intel-wired-lan