From: Paul Menzel <redacted>
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:

quoted

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/

quoted

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*

quoted

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.

quoted

Fixes: 2121c2712f82 ("igc: Add multiple receive queues control
supporting")
Signed-off-by: Jesper Dangaard Brouer <redacted>

[...]

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