[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <c40de89d-2977-5c8d-e049-006df2431f47@redhat.com>
Date: Fri, 10 Feb 2023 16:23:43 +0100
From: Jesper Dangaard Brouer <jbrouer@...hat.com>
To: bpf@...r.kernel.org
Cc: brouer@...hat.com, netdev@...r.kernel.org,
Stanislav Fomichev <sdf@...gle.com>, martin.lau@...nel.org,
ast@...nel.org, daniel@...earbox.net, alexandr.lobakin@...el.com,
yoong.siang.song@...el.com, anthony.l.nguyen@...el.com,
intel-wired-lan@...ts.osuosl.org, xdp-hints@...-project.net
Subject: Re: [PATCH bpf-next V1] igc: enable and fix RX hash usage by netstack
On 10/02/2023 16.07, Jesper Dangaard Brouer wrote:
> 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.
>
Sending this fix against bpf-next, as I found this issue while playing
with implementing XDP-hints kfunc for xmo_rx_hash. I will hopefully send
kfunc patches next week, on top of this.IMHO this fix isn't very
critical and I hope it can simply go though the
bpf-next tree as it would ease followup kfunc patches.
> 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
> 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
> 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.
>
Intel QA tester wanting to verify this patch can use the small bpftrace
tool I wrote and placed here:
https://github.com/xdp-project/xdp-project/blob/master/areas/hints/monitor_skb_hash_on_dev.bt
Failure scenarios:
$ sudo ./monitor_skb_hash_on_dev.bt igc1
Attaching 2 probes...
Monitor net_device: igc1
Hit Ctrl-C to end.
IFNAME HASH Hash-type:L4 Software-hash
igc1 00000000 0 0
igc1 00000000 0 0
igc1 00000000 0 0
^C
Example output with patch:
$ sudo ./monitor_skb_hash_on_dev.bt igc1
Attaching 2 probes...
Monitor net_device: igc1
Hit Ctrl-C to end.
IFNAME HASH Hash-type:L4 Software-hash
igc1 FEF98EFE 0 0
igc1 00000000 0 0
igc1 00000000 0 0
igc1 FEF98EFE 0 0
igc1 FEF98EFE 0 0
igc1 FEF98EFE 0 0
igc1 310AF9EA 1 0
igc1 A229FA51 1 0
The repeating hash FEF98EFE is UDP packets that as desc note doesn't
have Hash-type:L4. The UDP has is repeating as port numbers aren't part
of the hash, and I was sending to same IP. The hash values with L4=1
were TCP packets.
Hope this eases QA work.
> Fixes: 2121c2712f82 ("igc: Add multiple receive queues control supporting")
> Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
> ---
> drivers/net/ethernet/intel/igc/igc.h | 52 +++++++++++++++++++++++++++++
> drivers/net/ethernet/intel/igc/igc_main.c | 35 +++++++++++++++++---
> 2 files changed, 83 insertions(+), 4 deletions(-)
--Jesper
Powered by blists - more mailing lists