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.
