| Message ID | 1429067542-20845-1-git-send-email-fan.du@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
>-----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
>-----Original Message----- >From: Tantilov, Emil S >Sent: Wednesday, April 15, 2015 11:42 AM >To: Du, Fan; Kirsher, Jeffrey T >Cc: intel-wired-lan@lists.osuosl.org; e1000-devel@lists.sourceforge.net; >fengyuleidian0615@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 >
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.
>-----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@lists.osuosl.org; e1000-devel@lists.sourceforge.net; >>fengyuleidian0615@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
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
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);
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(-)