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:

quoted

On 30/03/2023 01.19, Stanislav Fomichev wrote:

quoted

On 03/29, Jesper Dangaard Brouer wrote:

quoted

On 29/03/2023 19.18, Stanislav Fomichev wrote:

quoted

On 03/29, Jesper Dangaard Brouer wrote:

quoted

On 28/03/2023 23.58, Stanislav Fomichev wrote:

quoted

On 03/28, Jesper Dangaard Brouer wrote:

quoted

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.

quoted

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.

[...]

quoted

quoted

Any reason it's not a XDP_RSS_L3_IPV6_EX within XDP_RSS_L3_MASK?

quoted

Hmm... I guess it belongs with L3.

quoted

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.

quoted

[...]

quoted

quoted

quoted

For example, for forward compat, I'm not sure we can assume that 

the people

quoted

quoted

quoted

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)"

quoted

quoted

quoted

quoted

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.

quoted

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.

quoted

quoted

quoted

quoted

quoted

quoted

quoted

This proposal change the kfunc API

bpf_xdp_metadata_rx_hash() > > > > to  return this RSS hash type on
success.

quoted

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?

quoted

Okay, in V3 I will propose adding an argument for the type then.

SG, thx!

quoted

quoted

Maybe also extend the tests to drop/decode/verify the mask?

quoted

Yes, I/we obviously need to update the selftests.

quoted

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.

quoted

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.