From: Jesper Dangaard Brouer <jbrouer@xxxxxxxxxx>
Date: Thu, 16 Feb 2023 14:29:43 +0100
>
> On 14/02/2023 16.00, Paul Menzel wrote:
>>
>> Thank you very much for your patch.
>
> Thanks for your review :-)
[...]
>>> 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.
>
>>> + 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
