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

[Jesper Dangaard Brouer <jbrouer@redhat.com>]

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.
>> >
>> > > The patch introduce a XDP RSS hash type (xdp_rss_hash_type) that can both
>> > > be seen as a number that is ordered according by ISO layer, and can be bit
>> > > masked to separate IPv4 and IPv6 types for L4 protocols. Room is available
>> > > for extending later while keeping these properties. This maps and unifies
>> > > difference to hardware specific hashes.
>> >
>> > Looks good overall. Any reason we're making this specific layout?
> 
>> One important goal is to have a simple/fast way to determining L3 vs L4,
>> because a L4 hash can be used for flow handling (e.g. load-balancing).
> 
>> We below layout you can:
> 
>>   if (rss_type & XDP_RSS_TYPE_L4_MASK)
>>     bool hw_hash_do_LB = true;
> 
>> Or using it as a number:
> 
>>   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).

> If we add some new L4 format, the bpf programs can be updated to support
> it?
> 
>> 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.
> 
>> > 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),
> 
>> 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
>> TCP.  Thus, using these bits as a number make sense to me, and is more
>> compact.
> 
> [..]
> 
>> 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
>> 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).

> 
> 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.


> 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.

>> > }
>> >
>> > And then using XDP_RSS_TYPE_IPV4|XDP_RSS_TYPE_UDP vs
>> > XDP_RSS_TYPE_IPV6|XXX ?
> 
>> 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
>> 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.
> 
> 
>> 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
> 
> 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.

> 
> 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.

>> > > 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?

--Jesper