Date: Thu, 18 May 2023 01:00:40 +0200
From: Ilya Maximets <i.maximets@....org>
To: Antoine Tenart <atenart@...nel.org>, Dumitru Ceara <dceara@...hat.com>,
 Eric Dumazet <edumazet@...gle.com>
Cc: i.maximets@....org, davem@...emloft.net, kuba@...nel.org,
 pabeni@...hat.com, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 4/4] net: skbuff: fix l4_hash comment

On 5/17/23 14:05, Antoine Tenart wrote:
> 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.

I would read the above as 'at least these fields'.  In the continuation
of the comment above it's, for example, allowed to set L3 when it is, in
fact, L4.

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

Only after the documentation change.

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

Following the same logic we can't really say that it "provides a uniform
distribution over L4 flows" either.  The fact that L4 fields were used
to calculate the hash, doesn't mean the hash function is any good.
So, the same way as stability can't be part of the definition, the
uniform distribution can't be as well.  And the 'l4' part of the field
name looses the meaning completely.  So, we will end up with:

  *	@l4_hash: Some number is stored in the 'hash' field.

At this point the is no reason to keep the flag at all.

In practice though, such setups are unlikley to be common.  Surely, some
adequate distribution and some stability for hashes is implied.
Not the best possible distribution and not the absolute stability, but
good enough to rely on in many cases.  And that's why the quoted comment
defines "Properties of hashes" this way.

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

Sure.

> 
> Thanks,
> Antoine

Powered by blists - more mailing lists