From: Jesper Dangaard Brouer <redacted>
Date: Thu, 16 Feb 2023 14:29:43 +0100

On 14/02/2023 16.00, Paul Menzel wrote:

quoted

Thank you very much for your patch.

Thanks for your review :-)

[...]

quoted

quoted

  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);
+        u8  rss_type = igc_rss_type(rx_desc);

Amongst others, also here.

Do notice I expect compiler to optimize this, such that is doesn't place
this variable on the stack.

That's complicated I'd say. I mean, never trust the compilers.
Yesterday I had a problem when the compiler (Clang 16) wasn't inlining
oneliners (called only once) into one big function and was making
19-byte tiny functions from each of them, resulting in 10+ functions
consisting of one call only => 2x jumps between functions :clownface:
Especially when you use non-native words (u8/u16, also u64 for 32-bit
arches) like this.

quoted

quoted

+        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;

Kind regards,

Paul


[1]: https://notabs.org/coding/smallIntsBigPenalty.htm

Thanks,
Olek