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

[Intel-wired-lan] [PATCH] 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>
---
Sorry for resending, I forgot to subscribe intel-wired-lan at lists.osuosl.org
as previous mail is being held by list moderator.

note:
1. I checked ixgbe_pci_tbl for 82598, 82599 x540 series,
   RSS type in adavanced descriptor is all supported.
   x550 is in trial version, so not checked.

2. ixgbevf looks like didn't even set rx hash type
   I'm not sure why this is left behind. IMHO it should be there.

---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   23 +++++++++++++++++++++--
 drivers/net/ethernet/intel/ixgbe/ixgbe_type.h |    1 +
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 395dc6b..8915992 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -1357,14 +1357,33 @@ static int __ixgbe_notify_dca(struct device *dev, void *data)
 }
 
 #endif /* CONFIG_IXGBE_DCA */
+static inline enum pkt_hash_types ixgbe_get_hash_type(__le16 pkt_info)
+{
+	switch (pkt_info & cpu_to_le16(IXGBE_RXDADV_RSSTYPE_MASK)) {
+	case IXGBE_RXDADV_RSSTYPE_IPV4_TCP:
+	case IXGBE_RXDADV_RSSTYPE_IPV4_UDP:
+	case IXGBE_RXDADV_RSSTYPE_IPV6_TCP:
+	case IXGBE_RXDADV_RSSTYPE_IPV6_UDP:
+		return PKT_HASH_TYPE_L4;
+	case IXGBE_RXDADV_RSSTYPE_IPV4:
+	case IXGBE_RXDADV_RSSTYPE_IPV6:
+		return PKT_HASH_TYPE_L3;
+	default:
+		return PKT_HASH_TYPE_NONE;
+	}
+}
+
 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)
+	if (ring->netdev->features & NETIF_F_RXHASH) {
+		__le16 pkt_info = rx_desc->wb.lower.lo_dword.hs_rss.pkt_info;
+
 		skb_set_hash(skb,
 			     le32_to_cpu(rx_desc->wb.lower.hi_dword.rss),
-			     PKT_HASH_TYPE_L3);
+			     ixgbe_get_hash_type(pkt_info));
+	}
 }
 
 #ifdef IXGBE_FCOE
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
index c3ddc94..97d600e 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
@@ -2225,6 +2225,7 @@ enum {
 #define IXGBE_RXDADV_RSSTYPE_IPV4_UDP   0x00000007
 #define IXGBE_RXDADV_RSSTYPE_IPV6_UDP   0x00000008
 #define IXGBE_RXDADV_RSSTYPE_IPV6_UDP_EX 0x00000009
+#define IXGBE_RXDADV_RSSTYPE_MASK       0x0000000F
 
 /* RSS Packet Types as indicated in the receive descriptor. */
 #define IXGBE_RXDADV_PKTTYPE_NONE       0x00000000
-- 
1.7.1

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

[Jeff Kirsher]

On Tue, 2015-04-14 at 14:56 +0800, 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>
> ---
> Sorry for resending, I forgot to subscribe
> intel-wired-lan at lists.osuosl.org
> as previous mail is being held by list moderator.
> 
> note:
> 1. I checked ixgbe_pci_tbl for 82598, 82599 x540 series,
>    RSS type in adavanced descriptor is all supported.
>    x550 is in trial version, so not checked.
> 
> 2. ixgbevf looks like didn't even set rx hash type
>    I'm not sure why this is left behind. IMHO it should be there.
> 
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   23
> +++++++++++++++++++++--
>  drivers/net/ethernet/intel/ixgbe/ixgbe_type.h |    1 +
>  2 files changed, 22 insertions(+), 2 deletions(-)

I have applied your patch to my queue
-- 
git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue.git
dev-queue
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20150414/7d37f7aa/attachment.asc>

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

[Tantilov, Emil S]

>-----Original Message-----
>From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Fan Du
>Sent: Monday, April 13, 2015 11:56 PM
>Subject: [Intel-wired-lan] [PATCH] 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 <fan.du@intel.com>
>---
>Sorry for resending, I forgot to subscribe intel-wired-lan at lists.osuosl.org
>as previous mail is being held by list moderator.
>
>note:
>1. I checked ixgbe_pci_tbl for 82598, 82599 x540 series,
>   RSS type in adavanced descriptor is all supported.
>   x550 is in trial version, so not checked.
>
>2. ixgbevf looks like didn't even set rx hash type
>   I'm not sure why this is left behind. IMHO it should be there.

I will look into it.

>---
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   23 +++++++++++++++++++++--
> drivers/net/ethernet/intel/ixgbe/ixgbe_type.h |    1 +
> 2 files changed, 22 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>index 395dc6b..8915992 100644
>--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>@@ -1357,14 +1357,33 @@ static int __ixgbe_notify_dca(struct device *dev, void *data)
> }
> 
> #endif /* CONFIG_IXGBE_DCA */
>+static inline enum pkt_hash_types ixgbe_get_hash_type(__le16 pkt_info)
>+{
>+	switch (pkt_info & cpu_to_le16(IXGBE_RXDADV_RSSTYPE_MASK)) {
>+	case IXGBE_RXDADV_RSSTYPE_IPV4_TCP:
>+	case IXGBE_RXDADV_RSSTYPE_IPV4_UDP:
>+	case IXGBE_RXDADV_RSSTYPE_IPV6_TCP:
>+	case IXGBE_RXDADV_RSSTYPE_IPV6_UDP:
>+		return PKT_HASH_TYPE_L4;
>+	case IXGBE_RXDADV_RSSTYPE_IPV4:
>+	case IXGBE_RXDADV_RSSTYPE_IPV6:
>+		return PKT_HASH_TYPE_L3;
>+	default:
>+		return PKT_HASH_TYPE_NONE;
>+	}
>+}
>+
> 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)
>+	if (ring->netdev->features & NETIF_F_RXHASH) {
>+		__le16 pkt_info = rx_desc->wb.lower.lo_dword.hs_rss.pkt_info;
>+
> 		skb_set_hash(skb,
> 			     le32_to_cpu(rx_desc->wb.lower.hi_dword.rss),
>-			     PKT_HASH_TYPE_L3);
>+			     ixgbe_get_hash_type(pkt_info));
>+	}
> }
> 
> #ifdef IXGBE_FCOE
>diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>index c3ddc94..97d600e 100644
>--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>@@ -2225,6 +2225,7 @@ enum {
> #define IXGBE_RXDADV_RSSTYPE_IPV4_UDP   0x00000007
> #define IXGBE_RXDADV_RSSTYPE_IPV6_UDP   0x00000008
> #define IXGBE_RXDADV_RSSTYPE_IPV6_UDP_EX 0x00000009
+#define IXGBE_RXDADV_RSSTYPE_MASK       0x0000000F

This define already exists in ixgbe_type.h.

The out of tree driver has this already implemented. I guess we did not push it upstream:

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 66adbd0..96703de 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -1357,14 +1357,33 @@ 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) | \
+	 (1ul << IXGBE_RXDADV_RSSTYPE_IPV6_TCP_EX) | \
+	 (1ul << IXGBE_RXDADV_RSSTYPE_IPV6_UDP_EX))
+
 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

Thanks,
Emil

[Intel-wired-lan] [PATCH] 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.

also use le16_to_cpu forcing type conversion to slient
endian check warnings.

Signed-off-by: Fan Du <fan.du@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |    2 +-
 drivers/net/ethernet/intel/ixgbevf/defines.h      |   12 ++++++++
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   30 +++++++++++++++++++++
 3 files changed, 43 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 8915992..1b3b5fb 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -1359,7 +1359,7 @@ static int __ixgbe_notify_dca(struct device *dev, void *data)
 #endif /* CONFIG_IXGBE_DCA */
 static inline enum pkt_hash_types ixgbe_get_hash_type(__le16 pkt_info)
 {
-	switch (pkt_info & cpu_to_le16(IXGBE_RXDADV_RSSTYPE_MASK)) {
+	switch (le16_to_cpu(pkt_info) & IXGBE_RXDADV_RSSTYPE_MASK) {
 	case IXGBE_RXDADV_RSSTYPE_IPV4_TCP:
 	case IXGBE_RXDADV_RSSTYPE_IPV4_UDP:
 	case IXGBE_RXDADV_RSSTYPE_IPV6_TCP:
diff --git a/drivers/net/ethernet/intel/ixgbevf/defines.h b/drivers/net/ethernet/intel/ixgbevf/defines.h
index 770e21a..040f7ca 100644
--- a/drivers/net/ethernet/intel/ixgbevf/defines.h
+++ b/drivers/net/ethernet/intel/ixgbevf/defines.h
@@ -151,7 +151,19 @@ typedef u32 ixgbe_link_speed;
 #define IXGBE_RXDADV_STAT_FCSTAT_FCPRSP	0x00000020 /* 10: Recv. FCP_RSP */
 #define IXGBE_RXDADV_STAT_FCSTAT_DDP	0x00000030 /* 11: Ctxt w/ DDP */
 
+/* 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_RXDADV_RSSTYPE_MASK	0x0000000F
+
 #define IXGBE_RXDADV_PKTTYPE_MASK	0x0000FFF0
 #define IXGBE_RXDADV_PKTTYPE_MASK_EX	0x0001FFF0
 #define IXGBE_RXDADV_HDRBUFLEN_MASK	0x00007FE0
diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index 4ee15ad..e529744 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -492,6 +492,35 @@ static inline void ixgbevf_rx_checksum(struct ixgbevf_ring *ring,
 	skb->ip_summed = CHECKSUM_UNNECESSARY;
 }
 
+static inline enum pkt_hash_types ixgbevf_get_hash_type(__le16 pkt_info)
+{
+	switch ((le16_to_cpu(pkt_info) & IXGBE_RXDADV_RSSTYPE_MASK)) {
+	case IXGBE_RXDADV_RSSTYPE_IPV4_TCP:
+	case IXGBE_RXDADV_RSSTYPE_IPV4_UDP:
+	case IXGBE_RXDADV_RSSTYPE_IPV6_TCP:
+	case IXGBE_RXDADV_RSSTYPE_IPV6_UDP:
+		return PKT_HASH_TYPE_L4;
+	case IXGBE_RXDADV_RSSTYPE_IPV4:
+	case IXGBE_RXDADV_RSSTYPE_IPV6:
+		return PKT_HASH_TYPE_L3;
+	default:
+		return PKT_HASH_TYPE_NONE;
+	}
+}
+
+static inline void ixgbevf_rx_hash(struct ixgbevf_ring *ring,
+				   union ixgbe_adv_rx_desc *rx_desc,
+				   struct sk_buff *skb)
+{
+	if (ring->netdev->features & NETIF_F_RXHASH) {
+		__le16 pkt_info = rx_desc->wb.lower.lo_dword.hs_rss.pkt_info;
+
+		skb_set_hash(skb,
+			     le32_to_cpu(rx_desc->wb.lower.hi_dword.rss),
+			     ixgbevf_get_hash_type(pkt_info));
+	}
+}
+
 /**
  * ixgbevf_process_skb_fields - Populate skb header fields from Rx descriptor
  * @rx_ring: rx descriptor ring packet is being transacted on
@@ -507,6 +536,7 @@ static void ixgbevf_process_skb_fields(struct ixgbevf_ring *rx_ring,
 				       struct sk_buff *skb)
 {
 	ixgbevf_rx_checksum(rx_ring, rx_desc, skb);
+	ixgbevf_rx_hash(rx_ring, rx_desc, skb);
 
 	if (ixgbevf_test_staterr(rx_desc, IXGBE_RXD_STAT_VP)) {
 		u16 vid = le16_to_cpu(rx_desc->wb.upper.vlan);
-- 
1.7.1

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

[Tantilov, Emil S]

>-----Original Message-----
>From: Du, Fan 
>Sent: Tuesday, April 14, 2015 8:12 PM
>To: Kirsher, Jeffrey T
>Subject: [PATCH] ixgbevf: Set rx hash type for ingress packets
>
>Set hash type for ingress packets according to NIC
>advanced receive descriptors RSS type part.
>
>also use le16_to_cpu forcing type conversion to slient
>endian check warnings.
>
>Signed-off-by: Fan Du <fan.du@intel.com>
>---
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |    2 +-
> drivers/net/ethernet/intel/ixgbevf/defines.h      |   12 ++++++++
> drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   30 +++++++++++++++++++++
> 3 files changed, 43 insertions(+), 1 deletions(-)
>
>diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>index 8915992..1b3b5fb 100644
>--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>@@ -1359,7 +1359,7 @@ static int __ixgbe_notify_dca(struct device *dev, void *data)
> #endif /* CONFIG_IXGBE_DCA */
> static inline enum pkt_hash_types ixgbe_get_hash_type(__le16 pkt_info)
> {
>-	switch (pkt_info & cpu_to_le16(IXGBE_RXDADV_RSSTYPE_MASK)) {
>+	switch (le16_to_cpu(pkt_info) & IXGBE_RXDADV_RSSTYPE_MASK) {

This appears to be a fix for your ixgbe patch and does not belong here. The proper way to handle it is to submit
version 2. Also checkout my reply to your ixgbe patch - it has the logic we use in ixgbe and there are some differences
that we need to address before we can accept this. Like the IPV4/6 extensions defines are not included and in the case of rss_type being 0 RSS is nor performed and so we probably should not call set_skb_hash() in that situation.

Thanks,
Emil 

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

[Du, Fan]

>-----Original Message-----
>From: Tantilov, Emil S
>Sent: Wednesday, April 15, 2015 11:42 AM
>To: Du, Fan; Kirsher, Jeffrey T
>Cc: intel-wired-lan at lists.osuosl.org; e1000-devel at lists.sourceforge.net;
>fengyuleidian0615 at gmail.com
>Subject: RE: [PATCH] ixgbevf: Set rx hash type for ingress packets
>
>>-----Original Message-----
>>From: Du, Fan
>>Sent: Tuesday, April 14, 2015 8:12 PM
>>To: Kirsher, Jeffrey T
>>Subject: [PATCH] ixgbevf: Set rx hash type for ingress packets
>>
>>Set hash type for ingress packets according to NIC
>>advanced receive descriptors RSS type part.
>>
>>also use le16_to_cpu forcing type conversion to slient
>>endian check warnings.
>>
>>Signed-off-by: Fan Du <fan.du@intel.com>
>>---
>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |    2 +-
>> drivers/net/ethernet/intel/ixgbevf/defines.h      |   12 ++++++++
>> drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   30
>+++++++++++++++++++++
>> 3 files changed, 43 insertions(+), 1 deletions(-)
>>
>>diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>index 8915992..1b3b5fb 100644
>>--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>@@ -1359,7 +1359,7 @@ static int __ixgbe_notify_dca(struct device *dev,
>void *data)
>> #endif /* CONFIG_IXGBE_DCA */
>> static inline enum pkt_hash_types ixgbe_get_hash_type(__le16 pkt_info)
>> {
>>-	switch (pkt_info & cpu_to_le16(IXGBE_RXDADV_RSSTYPE_MASK)) {
>>+	switch (le16_to_cpu(pkt_info) & IXGBE_RXDADV_RSSTYPE_MASK) {
>
>This appears to be a fix for your ixgbe patch and does not belong here. The
>proper way to handle it is to submit
>version 2. 

ok, I will send v2 for ixgbe patch, but Jeffrey has to drop previous ixgbe path he has queued.

>Also checkout my reply to your ixgbe patch - it has the logic we use in
>ixgbe and there are some differences
>that we need to address before we can accept this. Like the IPV4/6 extensions
>defines are not included

All RSSTYPE suffixed with '_EX' are marked as reserved, which hold no valid meaning
from 82599 data sheet labeled February 2015 Revision 3.1 331520-002.

Is there any other official document states what does those bits mean?

>and in the case of rss_type being 0 RSS is nor performed
>and so we probably should not call set_skb_hash() in that situation.

Hmm, this relies on the fact skb hash parts are cleared at the allocation of skb structure.
yes we should not set hash for PKT_HASH_TYPE_NONE, however bnx2x/i40e all call set_skb_hash
to clear skb hash parts for PKT_HASH_TYPE_NONE as a defensive programming probably.

I'm ok with either way, I will fix this part if you insist to do so.
which would you prefer?

>Thanks,
>Emil
>

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

[Jeff Kirsher]

On Wed, 2015-04-15 at 00:27 -0700, Du, Fan wrote:
> >This appears to be a fix for your ixgbe patch and does not belong
> here. The
> >proper way to handle it is to submit
> >version 2. 
> 
> ok, I will send v2 for ixgbe patch, but Jeffrey has to drop previous
> ixgbe path he has queued.

I was going to mention what Emil has already stated...  Please provide a
v2 of your original patch, with the necessary fixes integrated.

I have dropped both of your patches and await v2.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20150415/a386cfb8/attachment.asc>

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

[Tantilov, Emil S]

>-----Original Message-----
>From: Du, Fan 
>Sent: Wednesday, April 15, 2015 12:28 AM
>Subject: RE: [PATCH] ixgbevf: Set rx hash type for ingress packets
>
>
>
>>-----Original Message-----
>>From: Tantilov, Emil S
>>Sent: Wednesday, April 15, 2015 11:42 AM
>>To: Du, Fan; Kirsher, Jeffrey T
>>Cc: intel-wired-lan at lists.osuosl.org; e1000-devel at lists.sourceforge.net;
>>fengyuleidian0615 at gmail.com
>>Subject: RE: [PATCH] ixgbevf: Set rx hash type for ingress packets
>>
>>>-----Original Message-----
>>>From: Du, Fan
>>>Sent: Tuesday, April 14, 2015 8:12 PM
>>>To: Kirsher, Jeffrey T
>>>Subject: [PATCH] ixgbevf: Set rx hash type for ingress packets
>>>
>>>Set hash type for ingress packets according to NIC
>>>advanced receive descriptors RSS type part.
>>>
>>>also use le16_to_cpu forcing type conversion to slient
>>>endian check warnings.
>>>
>>>Signed-off-by: Fan Du <fan.du@intel.com>
>>>---
>>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |    2 +-
>>> drivers/net/ethernet/intel/ixgbevf/defines.h      |   12 ++++++++
>>> drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   30
>>+++++++++++++++++++++
>>> 3 files changed, 43 insertions(+), 1 deletions(-)
>>>
>>>diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>index 8915992..1b3b5fb 100644
>>>--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>@@ -1359,7 +1359,7 @@ static int __ixgbe_notify_dca(struct device *dev,
>>void *data)
>>> #endif /* CONFIG_IXGBE_DCA */
>>> static inline enum pkt_hash_types ixgbe_get_hash_type(__le16 pkt_info)
>>> {
>>>-	switch (pkt_info & cpu_to_le16(IXGBE_RXDADV_RSSTYPE_MASK)) {
>>>+	switch (le16_to_cpu(pkt_info) & IXGBE_RXDADV_RSSTYPE_MASK) {
>>
>>This appears to be a fix for your ixgbe patch and does not belong here. The
>>proper way to handle it is to submit
>>version 2.
>
>ok, I will send v2 for ixgbe patch, but Jeffrey has to drop previous ixgbe path he has queued.
>
>>Also checkout my reply to your ixgbe patch - it has the logic we use in
>>ixgbe and there are some differences
>>that we need to address before we can accept this. Like the IPV4/6 extensions
>>defines are not included
>
>All RSSTYPE suffixed with '_EX' are marked as reserved, which hold no valid meaning
>from 82599 data sheet labeled February 2015 Revision 3.1 331520-002.
>
>Is there any other official document states what does those bits mean?

They are defined, so they do probably have meaning, although you're right that it is not in the datasheet.
I'll have to check.

>>and in the case of rss_type being 0 RSS is nor performed
>>and so we probably should not call set_skb_hash() in that situation.
>
>Hmm, this relies on the fact skb hash parts are cleared at the allocation of skb structure.
>yes we should not set hash for PKT_HASH_TYPE_NONE, however bnx2x/i40e all call set_skb_hash
>to clear skb hash parts for PKT_HASH_TYPE_NONE as a defensive programming probably.
>I'm ok with either way, I will fix this part if you insist to do so.
>which would you prefer?

I'd rather we keep the logic we use in the out of tree driver (see my other email).

Thanks,
Emil

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

[Alexander Duyck]

On 04/14/2015 08:42 PM, Tantilov, Emil S wrote:
>> -----Original Message-----
>> From: Du, Fan
>> Sent: Tuesday, April 14, 2015 8:12 PM
>> To: Kirsher, Jeffrey T
>> Subject: [PATCH] ixgbevf: Set rx hash type for ingress packets
>>
>> Set hash type for ingress packets according to NIC
>> advanced receive descriptors RSS type part.
>>
>> also use le16_to_cpu forcing type conversion to slient
>> endian check warnings.
>>
>> Signed-off-by: Fan Du <fan.du@intel.com>
>> ---
>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |    2 +-
>> drivers/net/ethernet/intel/ixgbevf/defines.h      |   12 ++++++++
>> drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   30 +++++++++++++++++++++
>> 3 files changed, 43 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index 8915992..1b3b5fb 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -1359,7 +1359,7 @@ static int __ixgbe_notify_dca(struct device *dev, void *data)
>> #endif /* CONFIG_IXGBE_DCA */
>> static inline enum pkt_hash_types ixgbe_get_hash_type(__le16 pkt_info)
>> {
>> -	switch (pkt_info & cpu_to_le16(IXGBE_RXDADV_RSSTYPE_MASK)) {
>> +	switch (le16_to_cpu(pkt_info) & IXGBE_RXDADV_RSSTYPE_MASK) {
> This appears to be a fix for your ixgbe patch and does not belong here. The proper way to handle it is to submit
> version 2. Also checkout my reply to your ixgbe patch - it has the logic we use in ixgbe and there are some differences
> that we need to address before we can accept this. Like the IPV4/6 extensions defines are not included and in the case of rss_type being 0 RSS is nor performed and so we probably should not call set_skb_hash() in that situation.
>
> Thanks,
> Emil
>

Also instead of byte swapping the value, why not byte swap the cases in 
the switch statement?  Then the byte swapping can all be done at compile 
time instead of having to do it at run-time on big endian systems.

- Alex

[Intel-wired-lan] [PATCH] 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>
---
note:

1. I checked ixgbe_pci_tbl for 82598, 82599 x540 series,
   RSS type in adavanced descriptor is all supported.
   x550 is in trial version, so not checked.

2. ixgbevf looks like didn't even set rx hash type
   I'm not sure why this is left behind. IMHO it should be there.

---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   23 +++++++++++++++++++++--
 drivers/net/ethernet/intel/ixgbe/ixgbe_type.h |    1 +
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 395dc6b..8915992 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -1357,14 +1357,33 @@ static int __ixgbe_notify_dca(struct device *dev, void *data)
 }
 
 #endif /* CONFIG_IXGBE_DCA */
+static inline enum pkt_hash_types ixgbe_get_hash_type(__le16 pkt_info)
+{
+	switch (pkt_info & cpu_to_le16(IXGBE_RXDADV_RSSTYPE_MASK)) {
+	case IXGBE_RXDADV_RSSTYPE_IPV4_TCP:
+	case IXGBE_RXDADV_RSSTYPE_IPV4_UDP:
+	case IXGBE_RXDADV_RSSTYPE_IPV6_TCP:
+	case IXGBE_RXDADV_RSSTYPE_IPV6_UDP:
+		return PKT_HASH_TYPE_L4;
+	case IXGBE_RXDADV_RSSTYPE_IPV4:
+	case IXGBE_RXDADV_RSSTYPE_IPV6:
+		return PKT_HASH_TYPE_L3;
+	default:
+		return PKT_HASH_TYPE_NONE;
+	}
+}
+
 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)
+	if (ring->netdev->features & NETIF_F_RXHASH) {
+		__le16 pkt_info = rx_desc->wb.lower.lo_dword.hs_rss.pkt_info;
+
 		skb_set_hash(skb,
 			     le32_to_cpu(rx_desc->wb.lower.hi_dword.rss),
-			     PKT_HASH_TYPE_L3);
+			     ixgbe_get_hash_type(pkt_info));
+	}
 }
 
 #ifdef IXGBE_FCOE
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
index c3ddc94..97d600e 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
@@ -2225,6 +2225,7 @@ enum {
 #define IXGBE_RXDADV_RSSTYPE_IPV4_UDP   0x00000007
 #define IXGBE_RXDADV_RSSTYPE_IPV6_UDP   0x00000008
 #define IXGBE_RXDADV_RSSTYPE_IPV6_UDP_EX 0x00000009
+#define IXGBE_RXDADV_RSSTYPE_MASK       0x0000000F
 
 /* RSS Packet Types as indicated in the receive descriptor. */
 #define IXGBE_RXDADV_PKTTYPE_NONE       0x00000000
-- 
1.7.1