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

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

[Eric Dumazet]

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 <jsperbeck@google.com>
Tested-by: Jian Yang <jianyang@google.com>
Cc: Maxim Mikityanskiy <maximmi@mellanox.com>
Cc: Alexander Lobakin <alobakin@dlink.ru>
Cc: Saeed Mahameed <saeedm@mellanox.com>
Cc: Edward Cree <ecree@solarflare.com>
---
 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

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

[Saeed Mahameed]

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.

> 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 <jsperbeck@google.com>
> Tested-by: Jian Yang <jianyang@google.com>
> Cc: Maxim Mikityanskiy <maximmi@mellanox.com>
> Cc: Alexander Lobakin <alobakin@dlink.ru>
> Cc: Saeed Mahameed <saeedm@mellanox.com>
> Cc: Edward Cree <ecree@solarflare.com>
> ---
>  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>

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

[Eric Dumazet]

On Thu, Feb 4, 2021 at 11:14 PM Saeed Mahameed <saeed@kernel.org> wrote:
>
> 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


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.

>
>
> > 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 <jsperbeck@google.com>
> > Tested-by: Jian Yang <jianyang@google.com>
> > Cc: Maxim Mikityanskiy <maximmi@mellanox.com>
> > Cc: Alexander Lobakin <alobakin@dlink.ru>
> > Cc: Saeed Mahameed <saeedm@mellanox.com>
> > Cc: Edward Cree <ecree@solarflare.com>
> > ---
> >  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>
>
>

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

[Edward Cree]

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 <jsperbeck@google.com>
> Tested-by: Jian Yang <jianyang@google.com>
> Cc: Maxim Mikityanskiy <maximmi@mellanox.com>
> Cc: Alexander Lobakin <alobakin@dlink.ru>
> Cc: Saeed Mahameed <saeedm@mellanox.com>
> Cc: Edward Cree <ecree@solarflare.com>

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

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

[Alexander Lobakin]

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 <saeed@kernel.org> wrote:
> >
> > 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
>
>
> 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.
>
>
> >
> >
> > but maybe i am missing some information about the actual issue you are
> > hitting.
>
>
> Well, the issue is precisely described in the changelog.
>
> >
> >
> > > 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 <jsperbeck@google.com>
> > > Tested-by: Jian Yang <jianyang@google.com>
> > > Cc: Maxim Mikityanskiy <maximmi@mellanox.com>
> > > Cc: Alexander Lobakin <alobakin@dlink.ru>

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 <alobakin@pm.me>

> > > Cc: Saeed Mahameed <saeedm@mellanox.com>
> > > Cc: Edward Cree <ecree@solarflare.com>
> > > ---
> > >  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

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

[Eric Dumazet]

On Fri, Feb 5, 2021 at 2:03 PM Alexander Lobakin <alobakin@pm.me> 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 <alobakin@pm.me>
>

Thanks.

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

[patchwork-bot+netdevbpf]

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