From: Alexander Lobakin <hidden> Date: 2021-03-12 16:22:52
This random series addresses some of suboptimal paths used in
the main GRO entry point.
The main body is patches 3-4 which simplify the code and improve
flow distribution. Two others are mostly cosmetic to make code
more readable.
The benetifs are not so huge and mostly depend on NIC RSS hash
function and a number of Rx flows per single NAPI instance. I got
something like +10-15 Mbps on 4-8 flows NATing.
Alexander Lobakin (4):
gro: give 'hash' variable in dev_gro_receive() a less confusing name
gro: don't dereference napi->gro_hash[x] multiple times in
dev_gro_receive()
gro: simplify gro_list_prepare()
gro: improve flow distribution across GRO buckets in dev_gro_receive()
net/core/dev.c | 31 ++++++++++++++-----------------
1 file changed, 14 insertions(+), 17 deletions(-)
--
2.30.2
From: Alexander Lobakin <hidden> Date: 2021-03-12 16:22:53
GRO bucket index doesn't change through the entire function.
Store a pointer to the corresponding bucket on stack once and use
it later instead of dereferencing again and again.
Signed-off-by: Alexander Lobakin <redacted>
---
net/core/dev.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
From: Alexander Lobakin <hidden> Date: 2021-03-12 16:22:53
'hash' stores not the flow hash, but the index of the GRO bucket
corresponding to it.
Change its name to 'bucket' to avoid confusion while reading lines
like '__set_bit(hash, &napi->gro_bitmask)'.
Signed-off-by: Alexander Lobakin <redacted>
---
net/core/dev.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
From: Alexander Lobakin <hidden> Date: 2021-03-12 16:22:53
gro_list_prepare() always returns &napi->gro_hash[bucket].list,
without any variations. Moreover, it uses 'napi' argument only to
have access to this list, and calculates the bucket index for the
second time (firstly it happens at the beginning of
dev_gro_receive()) to do that.
Given that dev_gro_receive() already has a pointer to the needed
list, just pass it as the first argument to eliminate redundant
calculations, and make gro_list_prepare() return void.
Also, both arguments of gro_list_prepare() can be constified since
this function can only modify the skbs from the bucket list.
Signed-off-by: Alexander Lobakin <redacted>
---
net/core/dev.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
From: Alexander Lobakin <hidden> Date: 2021-03-12 16:22:54
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(-)
From: Eric Dumazet <edumazet@google.com> Date: 2021-03-12 16:35:09
On Fri, Mar 12, 2021 at 5:22 PM Alexander Lobakin [off-list ref] wrote:
quoted hunk
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(-)
This is going to use 3 high order bits instead of 3 low-order bits.
Now, had you use hash_32(skb_get_hash_raw(skb), 3), you could have
claimed to use "more bits"
Toeplitz already shuffles stuff.
Adding a multiply here seems not needed.
Please provide experimental results, because this looks unnecessary to me.
From: Eric Dumazet <edumazet@google.com> Date: 2021-03-12 16:47:57
On Fri, Mar 12, 2021 at 5:22 PM Alexander Lobakin [off-list ref] wrote:
quoted hunk
GRO bucket index doesn't change through the entire function.
Store a pointer to the corresponding bucket on stack once and use
it later instead of dereferencing again and again.
Signed-off-by: Alexander Lobakin <redacted>
---
net/core/dev.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
This adds more register pressure, do you have precise measures to
confirm this change is a win ?
Presumably the compiler should be able to optimize the code just fine,
it can see @bucket does not change.
From: Alexander Lobakin <hidden> Date: 2021-03-12 18:29:10
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(-)
From: Alexander Lobakin <hidden> Date: 2021-03-12 18:38:13
From: Eric Dumazet <edumazet@google.com>
Date: Fri, 12 Mar 2021 17:47:04 +0100
On Fri, Mar 12, 2021 at 5:22 PM Alexander Lobakin [off-list ref] wrote:
quoted
GRO bucket index doesn't change through the entire function.
Store a pointer to the corresponding bucket on stack once and use
it later instead of dereferencing again and again.
Signed-off-by: Alexander Lobakin <redacted>
---
net/core/dev.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
This adds more register pressure, do you have precise measures to
confirm this change is a win ?
Presumably the compiler should be able to optimize the code just fine,
it can see @bucket does not change.
This is mostly (if not purely) cosmetic, I don't think it changes
anything at all for the most of sane compilers.
Regarding registers, since @gro_list and @gro_head are pretty the
same, we could drop @gro_head in favour of @gro_list and just use
@gro_list->list instead.
Al