| Message ID | 167656636587.1912541.8039324850101942090.stgit@firesoul (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [bpf-next,V2] igc: enable and fix RX hash usage by netstack | expand |
On 2/16/23 5:52 PM, Jesper Dangaard Brouer wrote: > When function igc_rx_hash() was introduced in v4.20 via commit 0507ef8a0372 > ("igc: Add transmit and receive fastpath and interrupt handlers"), the > hardware wasn't configured to provide RSS hash, thus it made sense to not > enable net_device NETIF_F_RXHASH feature bit. > > The NIC hardware was configured to enable RSS hash info in v5.2 via commit > 2121c2712f82 ("igc: Add multiple receive queues control supporting"), but > forgot to set the NETIF_F_RXHASH feature bit. > > The original implementation of igc_rx_hash() didn't extract the associated > pkt_hash_type, but statically set PKT_HASH_TYPE_L3. The largest portions of > this patch are about extracting the RSS Type from the hardware and mapping > this to enum pkt_hash_types. This was based on Foxville i225 software user > manual rev-1.3.1 and tested on Intel Ethernet Controller I225-LM (rev 03). > > For UDP it's worth noting that RSS (type) hashing have been disabled both for > IPv4 and IPv6 (see IGC_MRQC_RSS_FIELD_IPV4_UDP + IGC_MRQC_RSS_FIELD_IPV6_UDP) > because hardware RSS doesn't handle fragmented pkts well when enabled (can > cause out-of-order). This results in PKT_HASH_TYPE_L3 for UDP packets, and > hash value doesn't include UDP port numbers. Not being PKT_HASH_TYPE_L4, have > the effect that netstack will do a software based hash calc calling into > flow_dissect, but only when code calls skb_get_hash(), which doesn't > necessary happen for local delivery. > > Fixes: 2121c2712f82 ("igc: Add multiple receive queues control supporting") > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> I presume this should go via net-next, not bpf-next? (Didn't find specific dependencies, so moved to patchwork netdev bucket..)
On 17/02/2023 21.59, Daniel Borkmann wrote: > On 2/16/23 5:52 PM, Jesper Dangaard Brouer wrote: >> When function igc_rx_hash() was introduced in v4.20 via commit 0507ef8a0372 >> ("igc: Add transmit and receive fastpath and interrupt handlers"), the >> hardware wasn't configured to provide RSS hash, thus it made sense to not >> enable net_device NETIF_F_RXHASH feature bit. >> >> The NIC hardware was configured to enable RSS hash info in v5.2 via commit >> 2121c2712f82 ("igc: Add multiple receive queues control supporting"), but >> forgot to set the NETIF_F_RXHASH feature bit. >> >> The original implementation of igc_rx_hash() didn't extract the associated >> pkt_hash_type, but statically set PKT_HASH_TYPE_L3. The largest portions of >> this patch are about extracting the RSS Type from the hardware and mapping >> this to enum pkt_hash_types. This was based on Foxville i225 software user >> manual rev-1.3.1 and tested on Intel Ethernet Controller I225-LM (rev 03). >> >> For UDP it's worth noting that RSS (type) hashing have been disabled both for >> IPv4 and IPv6 (see IGC_MRQC_RSS_FIELD_IPV4_UDP + IGC_MRQC_RSS_FIELD_IPV6_UDP) >> because hardware RSS doesn't handle fragmented pkts well when enabled (can >> cause out-of-order). This results in PKT_HASH_TYPE_L3 for UDP packets, and >> hash value doesn't include UDP port numbers. Not being PKT_HASH_TYPE_L4, have >> the effect that netstack will do a software based hash calc calling into >> flow_dissect, but only when code calls skb_get_hash(), which doesn't >> necessary happen for local delivery. >> >> Fixes: 2121c2712f82 ("igc: Add multiple receive queues control supporting") >> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > > I presume this should go via net-next, not bpf-next? (Didn't find specific > dependencies, so moved to patchwork netdev bucket..) Thanks for letting me/us know. I posted against bpf-next, because I have patches (for kfunc XDP-hints) that depend on this patch. As we are at rc8, netdev maintainers feel free to simply drop the patch as the patch isn't critical. I will just include this patch as part of my kfunc XDP-hints series later. --Jesper
diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h index df3e26c0cf01..97f629362d20 100644 --- a/drivers/net/ethernet/intel/igc/igc.h +++ b/drivers/net/ethernet/intel/igc/igc.h @@ -311,6 +311,62 @@ extern char igc_driver_name[]; #define IGC_MRQC_RSS_FIELD_IPV4_UDP 0x00400000 #define IGC_MRQC_RSS_FIELD_IPV6_UDP 0x00800000 +/* RX-desc Write-Back format RSS Type's */ +enum igc_rss_type_num { + IGC_RSS_TYPE_NO_HASH = 0, + IGC_RSS_TYPE_HASH_TCP_IPV4 = 1, + IGC_RSS_TYPE_HASH_IPV4 = 2, + IGC_RSS_TYPE_HASH_TCP_IPV6 = 3, + IGC_RSS_TYPE_HASH_IPV6_EX = 4, + IGC_RSS_TYPE_HASH_IPV6 = 5, + IGC_RSS_TYPE_HASH_TCP_IPV6_EX = 6, + IGC_RSS_TYPE_HASH_UDP_IPV4 = 7, + IGC_RSS_TYPE_HASH_UDP_IPV6 = 8, + IGC_RSS_TYPE_HASH_UDP_IPV6_EX = 9, + IGC_RSS_TYPE_MAX = 10, +}; +#define IGC_RSS_TYPE_MAX_TABLE 16 +#define IGC_RSS_TYPE_MASK 0xF + +/* igc_rss_type - Rx descriptor RSS type field */ +static inline u32 igc_rss_type(const union igc_adv_rx_desc *rx_desc) +{ + /* RSS Type 4-bit number: 0-9 (above 9 is reserved) + * Accessing the same bits via u16 (wb.lower.lo_dword.hs_rss.pkt_info) + * is slightly slower than via u32 (wb.lower.lo_dword.data) + */ + return le32_to_cpu(rx_desc->wb.lower.lo_dword.data) & IGC_RSS_TYPE_MASK; +} + +/* Packet header type identified by hardware (when BIT(11) is zero). + * Even when UDP ports are not part of RSS hash HW still parse and mark UDP bits + */ +enum igc_pkt_type_bits { + IGC_PKT_TYPE_HDR_IPV4 = BIT(0), + IGC_PKT_TYPE_HDR_IPV4_WITH_OPT= BIT(1), /* IPv4 Hdr includes IP options */ + IGC_PKT_TYPE_HDR_IPV6 = BIT(2), + IGC_PKT_TYPE_HDR_IPV6_WITH_EXT= BIT(3), /* IPv6 Hdr includes extensions */ + IGC_PKT_TYPE_HDR_L4_TCP = BIT(4), + IGC_PKT_TYPE_HDR_L4_UDP = BIT(5), + IGC_PKT_TYPE_HDR_L4_SCTP= BIT(6), + IGC_PKT_TYPE_HDR_NFS = BIT(7), + /* Above only valid when BIT(11) is zero */ + IGC_PKT_TYPE_L2 = BIT(11), + IGC_PKT_TYPE_VLAN = BIT(12), + IGC_PKT_TYPE_MASK = GENMASK(16,4), /* 13-bits (16:4) */ + IGC_PKT_TYPE_SHIFT = 4 +}; + +/* igc_pkt_type - Rx descriptor Packet type field */ +static inline u32 igc_pkt_type(const union igc_adv_rx_desc *rx_desc) +{ + u32 data = le32_to_cpu(rx_desc->wb.lower.lo_dword.data); + /* Packet type is 13-bits - as bits (16:4) in lower.lo_dword*/ + u32 pkt_type = (data & IGC_PKT_TYPE_MASK) >> IGC_PKT_TYPE_SHIFT; + + return pkt_type; +} + /* Interrupt defines */ #define IGC_START_ITR 648 /* ~6000 ints/sec */ #define IGC_4K_ITR 980 diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index 8b572cd2c350..27c028f86a9e 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -1677,14 +1677,40 @@ static void igc_rx_checksum(struct igc_ring *ring, le32_to_cpu(rx_desc->wb.upper.status_error)); } +/* Mapping HW RSS Type to enum pkt_hash_types */ +struct igc_rss_type { + u32 hash_type; /* can contain enum pkt_hash_types */ +} igc_rss_type_table[IGC_RSS_TYPE_MAX_TABLE] = { + [IGC_RSS_TYPE_NO_HASH].hash_type = PKT_HASH_TYPE_L2, + [IGC_RSS_TYPE_HASH_TCP_IPV4].hash_type = PKT_HASH_TYPE_L4, + [IGC_RSS_TYPE_HASH_IPV4].hash_type = PKT_HASH_TYPE_L3, + [IGC_RSS_TYPE_HASH_TCP_IPV6].hash_type = PKT_HASH_TYPE_L4, + [IGC_RSS_TYPE_HASH_IPV6_EX].hash_type = PKT_HASH_TYPE_L3, + [IGC_RSS_TYPE_HASH_IPV6].hash_type = PKT_HASH_TYPE_L3, + [IGC_RSS_TYPE_HASH_TCP_IPV6_EX].hash_type = PKT_HASH_TYPE_L4, + [IGC_RSS_TYPE_HASH_UDP_IPV4].hash_type = PKT_HASH_TYPE_L4, + [IGC_RSS_TYPE_HASH_UDP_IPV6].hash_type = PKT_HASH_TYPE_L4, + [IGC_RSS_TYPE_HASH_UDP_IPV6_EX].hash_type = PKT_HASH_TYPE_L4, + [10].hash_type = PKT_HASH_TYPE_L2, /* RSS Type above 9 "Reserved" by HW */ + [11].hash_type = PKT_HASH_TYPE_L2, + [12].hash_type = PKT_HASH_TYPE_L2, + [13].hash_type = PKT_HASH_TYPE_L2, + [14].hash_type = PKT_HASH_TYPE_L2, + [15].hash_type = PKT_HASH_TYPE_L2, +}; + static inline void igc_rx_hash(struct igc_ring *ring, union igc_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); + if (ring->netdev->features & NETIF_F_RXHASH) { + u32 rss_hash = le32_to_cpu(rx_desc->wb.lower.hi_dword.rss); + u32 rss_type = igc_rss_type(rx_desc); + enum pkt_hash_types hash_type; + + hash_type = igc_rss_type_table[rss_type].hash_type; + skb_set_hash(skb, rss_hash, hash_type); + } } static void igc_rx_vlan(struct igc_ring *rx_ring, @@ -6501,6 +6527,7 @@ static int igc_probe(struct pci_dev *pdev, netdev->features |= NETIF_F_TSO; netdev->features |= NETIF_F_TSO6; netdev->features |= NETIF_F_TSO_ECN; + netdev->features |= NETIF_F_RXHASH; netdev->features |= NETIF_F_RXCSUM; netdev->features |= NETIF_F_HW_CSUM; netdev->features |= NETIF_F_SCTP_CRC;
When function igc_rx_hash() was introduced in v4.20 via commit 0507ef8a0372 ("igc: Add transmit and receive fastpath and interrupt handlers"), the hardware wasn't configured to provide RSS hash, thus it made sense to not enable net_device NETIF_F_RXHASH feature bit. The NIC hardware was configured to enable RSS hash info in v5.2 via commit 2121c2712f82 ("igc: Add multiple receive queues control supporting"), but forgot to set the NETIF_F_RXHASH feature bit. The original implementation of igc_rx_hash() didn't extract the associated pkt_hash_type, but statically set PKT_HASH_TYPE_L3. The largest portions of this patch are about extracting the RSS Type from the hardware and mapping this to enum pkt_hash_types. This was based on Foxville i225 software user manual rev-1.3.1 and tested on Intel Ethernet Controller I225-LM (rev 03). For UDP it's worth noting that RSS (type) hashing have been disabled both for IPv4 and IPv6 (see IGC_MRQC_RSS_FIELD_IPV4_UDP + IGC_MRQC_RSS_FIELD_IPV6_UDP) because hardware RSS doesn't handle fragmented pkts well when enabled (can cause out-of-order). This results in PKT_HASH_TYPE_L3 for UDP packets, and hash value doesn't include UDP port numbers. Not being PKT_HASH_TYPE_L4, have the effect that netstack will do a software based hash calc calling into flow_dissect, but only when code calls skb_get_hash(), which doesn't necessary happen for local delivery. Fixes: 2121c2712f82 ("igc: Add multiple receive queues control supporting") Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> --- drivers/net/ethernet/intel/igc/igc.h | 56 +++++++++++++++++++++++++++++ drivers/net/ethernet/intel/igc/igc_main.c | 35 ++++++++++++++++-- 2 files changed, 87 insertions(+), 4 deletions(-)