Re: [PATCH bpf RFC 1/4] xdp: rss hash types representation

Jesper Dangaard Brouer
2023-03-30T18:53:23Z

On 30/03/2023 19.11, Stanislav Fomichev wrote:
> On 03/30, Jesper Dangaard Brouer wrote:
>
>> On 30/03/2023 01.19, Stanislav Fomichev wrote:
>> > On 03/29, Jesper Dangaard Brouer wrote:
>> >
>> > > On 29/03/2023 19.18, Stanislav Fomichev wrote:
>> > > > On 03/29, Jesper Dangaard Brouer wrote:
>> > > >
>> > > > > On 28/03/2023 23.58, Stanislav Fomichev wrote:
>> > > > > > On 03/28, Jesper Dangaard Brouer wrote:
>> > > > > > > The RSS hash type specifies what portion of packet data NIC hardware used
>> > > > > > > when calculating RSS hash value. The RSS types are focused on Internet
>> > > > > > > traffic protocols at OSI layers L3 and L4. L2 (e.g. ARP) often get hash
>> > > > > > > value zero and no RSS type. For L3 focused on IPv4 vs. IPv6, and L4
>> > > > > > > primarily TCP vs UDP, but some hardware supports SCTP.
>> > > > > >
>> > > > > > > Hardware RSS types are differently encoded for each hardware NIC. Most
>> > > > > > > hardware represent RSS hash type as a number. Determining L3 vs L4 often
>> > > > > > > requires a mapping table as there often isn't a pattern or sorting
>> > > > > > > according to ISO layer.
>> > > > > >
[...]
>> > Any reason it's not a XDP_RSS_L3_IPV6_EX within XDP_RSS_L3_MASK?
>> >
>
>> Hmm... I guess it belongs with L3.
>
>> Do notice that both IPv4 and IPv6 have a flexible header called either
>> options/extensions headers, after their fixed header. (Mlx4 HW contains this
>> info for IPv4, but I didn't extend xdp_rss_hash_type in that patch).
>> Thus, we could have a single BIT that is valid for both IPv4 and IPv6.
>> (This can help speedup packet parsing having this info).
>
> A separate bit for both v4/v6 sounds good. But thinking more about it,
> not sure what the users are supposed to do with it. Whether the flow is
> hashed over the extension header should a config option, not a per-packet signal?
>

Microsoft defines which part of the IPv6 Extensions headers will be used
for replacing either the Source (Home address) and Dest
(Routing-Header-Type-2) IPv6 Addresses, in the hash calc, here[1]:

[1]
https://learn.microsoft.com/en-us/windows-hardware/drivers/network/rss-hashing-types#ndis_hash_ipv6_ex

The igc/i225 chip returns per-packet the RSS Type's with _EX added.
Thus, I implemented this per-packet basis.

>> [...]
>> >
>> > > > For example, for forward compat, I'm not sure we can assume that
>> the people
>> > > > will do:
>> > > >      "rss_type & XDP_RSS_TYPE_L4_MASK"
>> > > > instead of something like:
>> > > >      "rss_type &
>> (XDP_RSS_TYPE_L4_IPV4_TCP|XDP_RSS_TYPE_L4_IPV4_UDP)"
>> > > >
>> >
>> > > This code is allowed in V2 and should be. It is a choice of
>> > > BPF-programmer in line-2 to not be forward compatible with newer L4
>> > > types.
>> >
>
>> The above code made me realize, I was wrong and you are right, we should
>> represent the L4 types as BITs (and not as numbers).
>> Even-though a single packet cannot be both UDP and TCP at the same time,
>> then it is reasonable to have a code path that want to match both UDP
>> and TCP.  If L4 types are BITs then code can do a single compare (via
>> ORing), while if they are numbers then we need more compares.
>> Thus, I'll change scheme in V3 to use BITs.
>
> So you are saying that the following:
>     if (rss_type & (TCP|UDP)
>
> is much faster than the following:
>     proto = rss_type & L4_MASK;
>     if (proto == TCP || proto == UDP)
>
> ?

For XDP every instruction/cycle counts.
Just to make sure, I tested it with godbolt.org, 3 vs 4 inst.

>
> idk, as long as we have enough bits to represent everything, I'm fine
> with either way, up to you. (not sure how much you want to constrain the
> data
> to fit it into xdp_frame; assuming u16 is fine?)

Yes, u16 is fine.

>
>
>> > > > > > > This proposal change the kfunc API
>> > > bpf_xdp_metadata_rx_hash() > > > > to  return this RSS hash type on
>> > > success.
>> >
>> > > This is the real question (as also raised above)...
>> > > Should we use return value or add an argument for type?
>> >
>> > Let's fix the prototype while it's still early in the rc?
>
>> Okay, in V3 I will propose adding an argument for the type then.
>
> SG, thx!

>
>> > Maybe also extend the tests to drop/decode/verify the mask?
>
>> Yes, I/we obviously need to update the selftests.
>
>> One problem with selftests is that it's using veth SKB-based mode, and
>> SKB's have lost the RSS hash info and converted this into a single BIT
>> telling us if this was L4 based.  Thus, its hard to do some e.g. UDP
>> type verification, but I guess we can check if expected UDP packet is
>> RSS type L4.
>
> Yeah, sounds fair.
>
>> In xdp_hw_metadata, I will add something that uses the RSS type bits.  I
>> was thinking to match against L4-UDP RSS type as program only AF_XDP
>> redirect UDP packets, so we can verify it was a UDP packet by HW info.
>
> Or maybe just dump it, idk.