[PATCH net] net: gro: do not keep too many GRO packets in napi->rx_list

From: Eric Dumazet <edumazet@google.com>

Commit c80794323e82 ("net: Fix packet reordering caused by GRO and
listified RX cooperation") had the unfortunate effect of adding
latencies in common workloads.

Before the patch, GRO packets were immediately passed to
upper stacks.

After the patch, we can accumulate quite a lot of GRO
packets (depdending on NAPI budget).

My fix is counting in napi->rx_count number of segments
instead of number of logical packets.

Fixes: c80794323e82 ("net: Fix packet reordering caused by GRO and listified RX cooperation")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Bisected-by: John Sperbeck [off-list ref]
Tested-by: Jian Yang <redacted>
Cc: Maxim Mikityanskiy <redacted>
Cc: Alexander Lobakin <redacted>
Cc: Saeed Mahameed <redacted>
Cc: Edward Cree <redacted>
---
 net/core/dev.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index a979b86dbacda9dfe31dd8b269024f7f0f5a8ef1..449b45b843d40ece7dd1e2ed6a5996ee1db9f591 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -5735,10 +5735,11 @@ static void gro_normal_list(struct napi_struct *napi)
 /* Queue one GRO_NORMAL SKB up for list processing. If batch size exceeded,
  * pass the whole batch up to the stack.
  */
-static void gro_normal_one(struct napi_struct *napi, struct sk_buff *skb)
+static void gro_normal_one(struct napi_struct *napi, struct sk_buff *skb, int segs)
 {
 	list_add_tail(&skb->list, &napi->rx_list);
-	if (++napi->rx_count >= gro_normal_batch)
+	napi->rx_count += segs;
+	if (napi->rx_count >= gro_normal_batch)
 		gro_normal_list(napi);
 }
 

@@ -5777,7 +5778,7 @@ static int napi_gro_complete(struct napi_struct *napi, struct sk_buff *skb)
 	}
 
 out:
-	gro_normal_one(napi, skb);
+	gro_normal_one(napi, skb, NAPI_GRO_CB(skb)->count);
 	return NET_RX_SUCCESS;
 }
 

@@ -6067,7 +6068,7 @@ static gro_result_t napi_skb_finish(struct napi_struct *napi,
 {
 	switch (ret) {
 	case GRO_NORMAL:
-		gro_normal_one(napi, skb);
+		gro_normal_one(napi, skb, 1);
 		break;
 
 	case GRO_DROP:

@@ -6155,7 +6156,7 @@ static gro_result_t napi_frags_finish(struct napi_struct *napi,
 		__skb_push(skb, ETH_HLEN);
 		skb->protocol = eth_type_trans(skb, skb->dev);
 		if (ret == GRO_NORMAL)
-			gro_normal_one(napi, skb);
+			gro_normal_one(napi, skb, 1);
 		break;
 
 	case GRO_DROP:

-- 
2.30.0.365.g02bc693789-goog

On Thu, 2021-02-04 at 13:31 -0800, Eric Dumazet wrote:

From: Eric Dumazet <edumazet@google.com>

Commit c80794323e82 ("net: Fix packet reordering caused by GRO and
listified RX cooperation") had the unfortunate effect of adding
latencies in common workloads.

Before the patch, GRO packets were immediately passed to
upper stacks.

After the patch, we can accumulate quite a lot of GRO
packets (depdending on NAPI budget).

Why napi budget ? looking at the code it seems to be more related to
MAX_GRO_SKBS * gro_normal_batch, since we are counting GRO SKBs as 1

but maybe i am missing some information about the actual issue you are
hitting.

Seems correct to me,

Reviewed-by: Saeed Mahameed <saeedm@nvidia.com>

On Thu, Feb 4, 2021 at 11:14 PM Saeed Mahameed [off-list ref] wrote:

On Thu, 2021-02-04 at 13:31 -0800, Eric Dumazet wrote:

quoted

From: Eric Dumazet <edumazet@google.com>

Commit c80794323e82 ("net: Fix packet reordering caused by GRO and
listified RX cooperation") had the unfortunate effect of adding
latencies in common workloads.

Before the patch, GRO packets were immediately passed to
upper stacks.

After the patch, we can accumulate quite a lot of GRO
packets (depdending on NAPI budget).

Why napi budget ? looking at the code it seems to be more related to
MAX_GRO_SKBS * gro_normal_batch, since we are counting GRO SKBs as 1

Simply because we call gro_normal_list() from napi_poll(),

So we flush the napi rx_list every 64 packets under stress.(assuming
NIC driver uses NAPI_POLL_WEIGHT),
or more often if napi_complete_done() is called if the budget was not exhausted.

GRO always has been able to keep MAX_GRO_SKBS in its layer, but no recent patch
has changed this part.

but maybe i am missing some information about the actual issue you are
hitting.

Well, the issue is precisely described in the changelog.

quoted

My fix is counting in napi->rx_count number of segments
instead of number of logical packets.

Fixes: c80794323e82 ("net: Fix packet reordering caused by GRO and
listified RX cooperation")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Bisected-by: John Sperbeck [off-list ref]
Tested-by: Jian Yang <redacted>
Cc: Maxim Mikityanskiy <redacted>
Cc: Alexander Lobakin <redacted>
Cc: Saeed Mahameed <redacted>
Cc: Edward Cree <redacted>
---
 net/core/dev.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c

index
a979b86dbacda9dfe31dd8b269024f7f0f5a8ef1..449b45b843d40ece7dd1e2ed6a5
996ee1db9f591 100644

--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -5735,10 +5735,11 @@ static void gro_normal_list(struct

napi_struct *napi)
 /* Queue one GRO_NORMAL SKB up for list processing. If batch size
exceeded,
  * pass the whole batch up to the stack.
  */
-static void gro_normal_one(struct napi_struct *napi, struct sk_buff
*skb)
+static void gro_normal_one(struct napi_struct *napi, struct sk_buff
*skb, int segs)
 {
        list_add_tail(&skb->list, &napi->rx_list);
-       if (++napi->rx_count >= gro_normal_batch)
+       napi->rx_count += segs;
+       if (napi->rx_count >= gro_normal_batch)
                gro_normal_list(napi);
 }

@@ -5777,7 +5778,7 @@ static int napi_gro_complete(struct napi_struct

*napi, struct sk_buff *skb)
        }

 out:
-       gro_normal_one(napi, skb);
+       gro_normal_one(napi, skb, NAPI_GRO_CB(skb)->count);

Seems correct to me,

Reviewed-by: Saeed Mahameed <saeedm@nvidia.com>

From: Eric Dumazet <edumazet@google.com>
Date: Thu, 4 Feb 2021 23:44:17 +0100

On Thu, Feb 4, 2021 at 11:14 PM Saeed Mahameed [off-list ref] wrote:

quoted

On Thu, 2021-02-04 at 13:31 -0800, Eric Dumazet wrote:

quoted

From: Eric Dumazet <edumazet@google.com>

Commit c80794323e82 ("net: Fix packet reordering caused by GRO and
listified RX cooperation") had the unfortunate effect of adding
latencies in common workloads.

Before the patch, GRO packets were immediately passed to
upper stacks.

After the patch, we can accumulate quite a lot of GRO
packets (depdending on NAPI budget).

Why napi budget ? looking at the code it seems to be more related to
MAX_GRO_SKBS * gro_normal_batch, since we are counting GRO SKBs as 1

Simply because we call gro_normal_list() from napi_poll(),

So we flush the napi rx_list every 64 packets under stress.(assuming
NIC driver uses NAPI_POLL_WEIGHT),
or more often if napi_complete_done() is called if the budget was not exhausted.

Saeed,

Eric means that if we have e.g. 8 GRO packets with 8 segs each, then
rx_list will be flushed only after processing of 64 ingress frames.

GRO always has been able to keep MAX_GRO_SKBS in its layer, but no recent patch
has changed this part.

quoted

but maybe i am missing some information about the actual issue you are
hitting.

Well, the issue is precisely described in the changelog.

quoted

quoted

My fix is counting in napi->rx_count number of segments
instead of number of logical packets.

Fixes: c80794323e82 ("net: Fix packet reordering caused by GRO and
listified RX cooperation")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Bisected-by: John Sperbeck [off-list ref]
Tested-by: Jian Yang <redacted>
Cc: Maxim Mikityanskiy <redacted>
Cc: Alexander Lobakin <redacted>

It's strange why mailmap didn't pick up my active email at pm.me.

Anyways, this fix is correct for me. It restores the original Edward's
logics, but without spurious out-of-order deliveries.
Moreover, the pre-patch behaviour can easily be achieved by increasing
net.core.gro_normal_batch if needed.

Thanks!

Reviewed-by: Alexander Lobakin <redacted>

quoted

quoted

Cc: Saeed Mahameed <redacted>
Cc: Edward Cree <redacted>
---
 net/core/dev.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c

index
a979b86dbacda9dfe31dd8b269024f7f0f5a8ef1..449b45b843d40ece7dd1e2ed6a5
996ee1db9f591 100644

--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -5735,10 +5735,11 @@ static void gro_normal_list(struct

napi_struct *napi)
 /* Queue one GRO_NORMAL SKB up for list processing. If batch size
exceeded,
  * pass the whole batch up to the stack.
  */
-static void gro_normal_one(struct napi_struct *napi, struct sk_buff
*skb)
+static void gro_normal_one(struct napi_struct *napi, struct sk_buff
*skb, int segs)
 {
        list_add_tail(&skb->list, &napi->rx_list);
-       if (++napi->rx_count >= gro_normal_batch)
+       napi->rx_count += segs;
+       if (napi->rx_count >= gro_normal_batch)
                gro_normal_list(napi);
 }

@@ -5777,7 +5778,7 @@ static int napi_gro_complete(struct napi_struct

*napi, struct sk_buff *skb)
        }

 out:
-       gro_normal_one(napi, skb);
+       gro_normal_one(napi, skb, NAPI_GRO_CB(skb)->count);

Seems correct to me,

Reviewed-by: Saeed Mahameed <saeedm@nvidia.com>

Al

On Fri, Feb 5, 2021 at 2:03 PM Alexander Lobakin [off-list ref] wrote:

It's strange why mailmap didn't pick up my active email at pm.me.

I took the signatures from c80794323e82, I CCed all people involved in
this recent patch.

It is very rare I use scripts/get_maintainer.pl since it tends to be noisy.

Anyways, this fix is correct for me. It restores the original Edward's
logics, but without spurious out-of-order deliveries.
Moreover, the pre-patch behaviour can easily be achieved by increasing
net.core.gro_normal_batch if needed.

Thanks!

Reviewed-by: Alexander Lobakin <redacted>

Thanks.

On 04/02/2021 21:31, Eric Dumazet wrote:

From: Eric Dumazet <edumazet@google.com>

Commit c80794323e82 ("net: Fix packet reordering caused by GRO and
listified RX cooperation") had the unfortunate effect of adding
latencies in common workloads.

Before the patch, GRO packets were immediately passed to
upper stacks.

After the patch, we can accumulate quite a lot of GRO
packets (depdending on NAPI budget).

My fix is counting in napi->rx_count number of segments
instead of number of logical packets.

Fixes: c80794323e82 ("net: Fix packet reordering caused by GRO and listified RX cooperation")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Bisected-by: John Sperbeck [off-list ref]
Tested-by: Jian Yang <redacted>
Cc: Maxim Mikityanskiy <redacted>
Cc: Alexander Lobakin <redacted>
Cc: Saeed Mahameed <redacted>
Cc: Edward Cree <redacted>

Reviewed-by: Edward Cree <ecree.xilinx@gmail.com>

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Thu,  4 Feb 2021 13:31:46 -0800 you wrote:

From: Eric Dumazet <edumazet@google.com>

Commit c80794323e82 ("net: Fix packet reordering caused by GRO and
listified RX cooperation") had the unfortunate effect of adding
latencies in common workloads.

Before the patch, GRO packets were immediately passed to
upper stacks.

[...]

Here is the summary with links:
  - [net] net: gro: do not keep too many GRO packets in napi->rx_list
    https://git.kernel.org/netdev/net/c/8dc1c444df19

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html