netdev - Re: [xdp-hints] Re: [Intel-wired-lan] [PATCH bpf-next V1] igc: enable and fix RX hash usage by netstack

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 16 Feb 2023 16:34:19 +0100
From:   Alexander Lobakin <aleksander.lobakin@...el.com>
To:     Jesper Dangaard Brouer <jbrouer@...hat.com>
CC:     Paul Menzel <pmenzel@...gen.mpg.de>, <brouer@...hat.com>,
        <bpf@...r.kernel.org>, <xdp-hints@...-project.net>,
        <martin.lau@...nel.org>, <daniel@...earbox.net>,
        <netdev@...r.kernel.org>, <ast@...nel.org>,
        "Stanislav Fomichev" <sdf@...gle.com>,
        <yoong.siang.song@...el.com>, <anthony.l.nguyen@...el.com>,
        <intel-wired-lan@...ts.osuosl.org>
Subject: Re: [xdp-hints] Re: [Intel-wired-lan] [PATCH bpf-next V1] igc: enable
 and fix RX hash usage by netstack

From: Jesper Dangaard Brouer <jbrouer@...hat.com>
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