[Intel-wired-lan] [PATCH 0/2] RSS fix for ixgbe/ixgbevf

[Intel-wired-lan] [PATCH 0/2] RSS fix for ixgbe/ixgbevf

[Fan Du]

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

[Intel-wired-lan] [PATCH 1/2] ixgbe: Specify rx hash type wrt rx desc RSS type

[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 <fan.du@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   25 +++++++++++++++++++++----
 1 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index d3f4b0c..5df896b 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -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
-- 
1.7.1

[Intel-wired-lan] [PATCH 2/2] ixgbevf: Set rx hash type for ingress packets

[Fan Du]

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

Signed-off-by: Fan Du <fan.du@intel.com>
---
 drivers/net/ethernet/intel/ixgbevf/defines.h      |   12 +++++++++
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   27 +++++++++++++++++++++
 2 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbevf/defines.h b/drivers/net/ethernet/intel/ixgbevf/defines.h
index 770e21a..af28ef3 100644
--- a/drivers/net/ethernet/intel/ixgbevf/defines.h
+++ b/drivers/net/ethernet/intel/ixgbevf/defines.h
@@ -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 |  \
diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index a16d267..2a4ae83 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -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)) {
-- 
1.7.1

[Intel-wired-lan] [PATCH 0/2] RSS fix for ixgbe/ixgbevf

[Du, Fan]

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

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

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


>Cheers,
>John
>
>> -----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
>>
>> 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(-)
>>
>> _______________________________________________
>> Intel-wired-lan mailing list
>> Intel-wired-lan at lists.osuosl.org
>> http://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH 1/2] ixgbe: Specify rx hash type wrt rx desc RSS type

[Alexander Duyck]

On 04/23/2015 12:31 AM, Fan Du wrote:
> 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 <fan.du@intel.com>
> ---
>   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   25 +++++++++++++++++++++----
>   1 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index d3f4b0c..5df896b 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -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

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

[Intel-wired-lan] [PATCH 1/2] ixgbe: Specify rx hash type wrt rx desc RSS type

[Du, Fan]

>-----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
>
>
>On 04/23/2015 12:31 AM, Fan Du wrote:
>> 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 <fan.du@intel.com>
>> ---
>>   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   25
>+++++++++++++++++++++----
>>   1 files changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index d3f4b0c..5df896b 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -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
>
>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.   


>- Alex

[Intel-wired-lan] [PATCH 1/2] ixgbe: Specify rx hash type wrt rx desc RSS type

[Alexander Duyck]

On 04/27/2015 11:43 PM, Du, Fan wrote:
>
>> -----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
>>
>>
>> On 04/23/2015 12:31 AM, Fan Du wrote:
>>> 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 <fan.du@intel.com>
>>> ---
>>>    drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   25
>> +++++++++++++++++++++----
>>>    1 files changed, 21 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> index d3f4b0c..5df896b 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> @@ -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
>> 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.
>

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