Re: [PATCH net-next 4/4] gro: improve flow distribution across GRO buckets in dev_gro_receive()

From: Eric Dumazet <edumazet@google.com>
Date: Fri, 12 Mar 2021 17:33:53 +0100

On Fri, Mar 12, 2021 at 5:22 PM Alexander Lobakin [off-list ref] wrote:

quoted

Most of the functions that "convert" hash value into an index
(when RPS is configured / XPS is not configured / etc.) set
reciprocal_scale() on it. Its logics is simple, but fair enough and
accounts the entire input value.
On the opposite side, 'hash & (GRO_HASH_BUCKETS - 1)' expression uses
only 3 least significant bits of the value, which is far from
optimal (especially for XOR RSS hashers, where the hashes of two
different flows may differ only by 1 bit somewhere in the middle).

Use reciprocal_scale() here too to take the entire hash value into
account and improve flow dispersion between GRO hash buckets.

Signed-off-by: Alexander Lobakin <redacted>
---
 net/core/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 65d9e7d9d1e8..bd7c9ba54623 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -5952,7 +5952,7 @@ static void gro_flush_oldest(struct napi_struct *napi, struct list_head *head)

 static enum gro_result dev_gro_receive(struct napi_struct *napi, struct sk_buff *skb)
 {
-       u32 bucket = skb_get_hash_raw(skb) & (GRO_HASH_BUCKETS - 1);
+       u32 bucket = reciprocal_scale(skb_get_hash_raw(skb), GRO_HASH_BUCKETS);

This is going to use 3 high order bits instead of 3 low-order bits.

We-e-ell, seems like it.

Now, had you use hash_32(skb_get_hash_raw(skb), 3), you could have
claimed to use "more bits"

Nice suggestion, I'll try. If there won't be any visible improvements,
I'll just drop this one.

Toeplitz already shuffles stuff.

As well as CRC and others, but I feel like we shouldn't rely only on
the hardware.

Adding a multiply here seems not needed.

Please provide experimental results, because this looks unnecessary to me.

Thanks,
Al