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

On 03/29, Jesper Dangaard Brouer wrote:

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

quoted

quoted

quoted

quoted

when calculating RSS hash value. The RSS types are focused on  

Internet

quoted

quoted

quoted

quoted

traffic protocols at OSI layers L3 and L4. L2 (e.g. ARP) often  

get hash

quoted

quoted

quoted

quoted

value zero and no RSS type. For L3 focused on IPv4 vs. IPv6, and  

L4

quoted

quoted

quoted

quoted

primarily TCP vs UDP, but some hardware supports SCTP.

quoted

Hardware RSS types are differently encoded for each hardware NIC.  

Most

quoted

quoted

quoted

quoted

hardware represent RSS hash type as a number. Determining L3 vs  

L4 often

quoted

quoted

quoted

quoted

requires a mapping table as there often isn't a pattern or sorting
according to ISO layer.

quoted

The patch introduce a XDP RSS hash type (xdp_rss_hash_type) that  

can both

quoted

quoted

quoted

quoted

be seen as a number that is ordered according by ISO layer, and  

can be bit

quoted

quoted

quoted

quoted

masked to separate IPv4 and IPv6 types for L4 protocols. Room is  

available

quoted

quoted

quoted

quoted

for extending later while keeping these properties. This maps and  

unifies

quoted

quoted

quoted

quoted

difference to hardware specific hashes.

Looks good overall. Any reason we're making this specific layout?

quoted

One important goal is to have a simple/fast way to determining L3 vs  

L4,

quoted

quoted

because a L4 hash can be used for flow handling (e.g. load-balancing).

quoted

We below layout you can:

quoted

  if (rss_type & XDP_RSS_TYPE_L4_MASK)
    bool hw_hash_do_LB = true;

quoted

Or using it as a number:

quoted

  if (rss_type > XDP_RSS_TYPE_L4)
    bool hw_hash_do_LB = true;

Why is it strictly better then the following?

if (rss_type & (TYPE_UDP | TYPE_TCP | TYPE_SCTP)) {}

See V2 I dropped the idea of this being a number (that idea was not a
good idea).

👍

quoted

If we add some new L4 format, the bpf programs can be updated to support
it?

quoted

I'm very open to changes to my "specific" layout.  I am in doubt if
using it as a number is the right approach and worth the trouble.

quoted

quoted

Why not simply the following?

enum {
 ����XDP_RSS_TYPE_NONE = 0,
 ����XDP_RSS_TYPE_IPV4 = BIT(0),
 ����XDP_RSS_TYPE_IPV6 = BIT(1),
 ����/* IPv6 with extension header. */
 ����/* let's note ^^^ it in the UAPI? */
 ����XDP_RSS_TYPE_IPV6_EX = BIT(2),
 ����XDP_RSS_TYPE_UDP = BIT(3),
 ����XDP_RSS_TYPE_TCP = BIT(4),
 ����XDP_RSS_TYPE_SCTP = BIT(5),

quoted

We know these bits for UDP, TCP, SCTP (and IPSEC) are exclusive, they
cannot be set at the same time, e.g. as a packet cannot both be UDP  

and

quoted

quoted

TCP.  Thus, using these bits as a number make sense to me, and is more
compact.

[..]

quoted

This BIT() approach also have the issue of extending it later (forward
compatibility).  As mentioned a common task will be to check if
hash-type is a L4 type.  See mlx5 [patch 4/4] needed to extend with
IPSEC. Notice how my XDP_RSS_TYPE_L4_MASK covers all the bits that  

this

quoted

quoted

can be extended with new L4 types, such that existing progs will still
work checking for L4 check.  It can of-cause be solved in the same way
for this BIT() approach by reserving some bits upfront in a mask.

We're using 6 bits out of 64, we should be good for awhile? If there
is ever a forward compatibility issue, we can always come up with
a new kfunc.

I want/need store the RSS-type in the xdp_frame, for XDP_REDIRECT and
SKB use-cases.  Thus, I don't want to use 64-bit/8-bytes, as xdp_frame
size is limited (given it reduces headroom expansion).

quoted

One other related question I have is: should we export the type
over some additional new kfunc argument? (instead of abusing the return
type)

Good question. I was also wondering if it wouldn't be better to add
another kfunc argument with the rss_hash_type?

That will change the call signature, so that will not be easy to handle
between kernel releases.

Agree with Toke on a separate thread; might not be too late to fit it
into an rc..

quoted

Maybe that will let us drop the explicit BTF_TYPE_EMIT as well?

Sure, if we define it as an argument, then it will automatically
exported as BTF.

quoted

quoted

quoted

}

And then using XDP_RSS_TYPE_IPV4|XDP_RSS_TYPE_UDP vs
XDP_RSS_TYPE_IPV6|XXX ?

quoted

Do notice, that I already does some level of or'ing ("|") in this
proposal.  The main difference is that I hide this from the driver,  

and

quoted

quoted

kind of pre-combine the valid combination (enum's) drivers can select
from. I do get the point, and I think I will come up with a combined
solution based on your input.

quoted

The RSS hashing types and combinations comes from M$ standards:
  [1]  

https://learn.microsoft.com/en-us/windows-hardware/drivers/network/rss-hashing-types#ipv4-hash-type-combinations

quoted

My main concern here is that we're over-complicating it with the masks
and the format. With the explicit bits we can easily map to that
spec you mention.

See if you like my RFC-V2 proposal better.
It should go more in your direction.

Yeah, I like it better. Btw, why have a separate bit for XDP_RSS_BIT_EX?
Any reason it's not a XDP_RSS_L3_IPV6_EX within XDP_RSS_L3_MASK?

And the following part seems like a leftover from the earlier version:

+/* For partitioning of xdp_rss_hash_type */
+#define RSS_L3		GENMASK(2,0) /* 3-bits = values between 1-7 */
+#define L4_BIT		BIT(3)       /* 1-bit - L4 indication */
+#define RSS_L4_IPV4	GENMASK(6,4) /* 3-bits */
+#define RSS_L4_IPV6	GENMASK(9,7) /* 3-bits */
+#define RSS_L4		GENMASK(9,3) /* = 7-bits - covering L4 IPV4+IPV6 */
+#define L4_IPV6_EX_BIT	BIT(9)       /* 1-bit - L4 IPv6 with Extension hdr  
*/
+				     /* 11-bits in total */

quoted

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

people

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

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

quoted

quoted

quoted

This proposal change the kfunc API bpf_xdp_metadata_rx_hash() to   

return

quoted

quoted

quoted

quoted

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?
Maybe also extend the tests to drop/decode/verify the mask?

--Jesper