[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b9e5077f-fbc4-8904-74a8-cda94d91cfbf@redhat.com>
Date: Wed, 29 Mar 2023 20:19:59 +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 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
Powered by blists - more mailing lists