Date:   Thu, 30 Mar 2023 20:52:15 +0200
From:   Jesper Dangaard Brouer <jbrouer@...hat.com>
To:     Stanislav Fomichev <sdf@...gle.com>,
        Jesper Dangaard Brouer <jbrouer@...hat.com>
Cc:     brouer@...hat.com, bpf@...r.kernel.org, netdev@...r.kernel.org,
        martin.lau@...nel.org, ast@...nel.org, daniel@...earbox.net,
        alexandr.lobakin@...el.com, larysa.zaremba@...el.com,
        xdp-hints@...-project.net, anthony.l.nguyen@...el.com,
        yoong.siang.song@...el.com, boon.leong.ong@...el.com,
        intel-wired-lan@...ts.osuosl.org, pabeni@...hat.com,
        jesse.brandeburg@...el.com, kuba@...nel.org, edumazet@...gle.com,
        john.fastabend@...il.com, hawk@...nel.org, davem@...emloft.net
Subject: Re: [PATCH bpf RFC 1/4] xdp: rss hash types representation



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.

Powered by blists - more mailing lists