Re: [PATCH net-next 4/4] net: skbuff: fix l4_hash comment

[Antoine Tenart <atenart@kernel.org>]

Quoting Ilya Maximets (2023-05-16 23:25:19)
> On 5/16/23 09:36, Antoine Tenart wrote:
> > 
> > What about "indicates hash was set by layer 4 stack and provides a
> > uniform distribution over flows"? Or/and we should we also add a
> > disclaimer like "no guarantee on how the hash was computed"?
> 
> I'm still not sure this is correct.  Is a NIC driver part of layer 4
> stack?

Offloading logic with L4 fields for csum, RSS, etc; we can argue it does
something at L4. What about this: "Provides a uniform distribution over
L4 flows"? I does look better than the previous proposal IMHO.

> And there are lots of other inconsistencies around skb hash.  The following
> is probably the most colorful that I found:
> 
>    skb_set_hash(skb, tcp_rsk(req)->txhash, PKT_HASH_TYPE_L4);

>  * Hash types refer to the protocol layer addresses which are used to
>  * construct a packet's hash. The hashes are used to differentiate or identify
>  * flows of the protocol layer for the hash type. Hash types are either
>  * layer-2 (L2), layer-3 (L3), or layer-4 (L4).
>  *
>  * Properties of hashes:
>  *
>  * 1) Two packets in different flows have different hash values
>  * 2) Two packets in the same flow should have the same hash value

> enum pkt_hash_types {
>         PKT_HASH_TYPE_L4,       /* Input: src_IP, dst_IP, src_port, dst_port */
> };
> 
> Here we see that PKT_HASH_TYPE_L4 supposed to use particular fields
> as an input.

If we strictly follow the above, do all NIC provide a L4 hash using only
the above fields (src_IP, dst_IP, src_port, dst_port)? Having a quick
look I'm pretty sure no, both 4 and 5-tuple can be used. What is
important is at what level the distribution is.

So yes strictly speaking the above PKT_HASH_TYPE_L4 use can be a little
surprising, but to me it's a shortcut or a missing update. For perfect
correctness we could use
__skb_set_hash(skb, tcp_rsk(req)->txhash, false, true) FWIW.

Even l4_hash w/o taking the rnd case into account does not guarantee a
stable hash for the lifetime of a flow; what happens if packets from the
same flow are received on two NICs using different keys and/or algs?
Being computed from L4 fields does not mean it is stable. If the stable
property is needed, the hash has to be computed locally. And then comes
the other topic of caching it for reuse and potential sharing across
different consumers, sure.

Now, I'll let some time to give a chance for others to chime in.

Thanks,
Antoine