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 for Android: free password hash cracker in your pocket

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

Message-ID: <b6143e67-a0f1-a238-f901-448b85281154@intel.com>
Date:   Tue, 14 Feb 2023 16:13:03 +0100
From:   Alexander Lobakin <alexandr.lobakin@...el.com>
To:     Paul Menzel <pmenzel@...gen.mpg.de>
CC:     Jesper Dangaard Brouer <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: Paul Menzel <pmenzel@...gen.mpg.de>
Date: Tue, 14 Feb 2023 16:00:52 +0100

> Dear Jesper,
> 
> 
> Thank you very much for your patch.
> 
> Am 10.02.23 um 16:07 schrieb Jesper Dangaard Brouer:
>> 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 were based on Foxville i225 software
>> user
> 
> s/This were/This was/
> 
>> 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 result in PKT_HASH_TYPE_L3 for UDP packets, and
> 
> result*s*
> 
>> 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.
> 
> Excuse my ignorance, but is that bug visible in practice by users
> (performance?) or is that fix needed for future work?

Hash calculation always happens when RPS or RFS is enabled. So having no
hash in skb before hitting the netstack slows down their performance.
Also, no hash in skb passed from the driver results in worse NAPI bucket
distribution when there are more traffic flows than Rx queues / CPUs.
+ Netfilter needs hashes on some configurations.

On default configurations and workloads like browsing the Internet this
usually is not the case, but only then I'd say.

> 
>> Fixes: 2121c2712f82 ("igc: Add multiple receive queues control
>> supporting")
>> Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>

[...]

Nice to see that you also care about (not) using short types on the stack :)

> Kind regards,
> 
> Paul
> 
> 
> [1]: https://notabs.org/coding/smallIntsBigPenalty.htm

Thanks,
Olek