[PATCH net-next] net: don't keep lonely packets forever in the gro hash

[PATCH net-next] net: don't keep lonely packets forever in the gro hash

[Paolo Abeni]

Eric noted that with UDP GRO and NAPI timeout, we could keep a single
UDP packet inside the GRO hash forever, if the related NAPI instance
calls napi_gro_complete() at an higher frequency than the NAPI timeout.
Willem noted that even TCP packets could be trapped there, till the
next retransmission.
This patch tries to address the issue, flushing the oldest packets before
scheduling the NAPI timeout. The rationale is that such a timeout should be
well below a jiffy and we are not flushing packets eligible for sane GRO.

RFC -> v1:
 - added 'Fixes tags', cleaned-up the wording.

Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
Fixes: 3b47d30396ba ("net: gro: add a per device gro flush timer")
Fixes: e20cf8d3f1f7 ("udp: implement GRO for plain UDP sockets.")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
--
Note: since one of the fixed commit is currently only on net-next,
the other one is really old, and the affected scenario without the
more recent commit is really a corner case, targeting net-next.
---
 net/core/dev.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 5927f6a7c301..b6eb4e0bfa91 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5975,11 +5975,14 @@ bool napi_complete_done(struct napi_struct *n, int work_done)
 		if (work_done)
 			timeout = n->dev->gro_flush_timeout;
 
+		/* When the NAPI instance uses a timeout, we still need to
+		 * somehow bound the time packets are keept in the GRO layer
+		 * under heavy traffic
+		 */
+		napi_gro_flush(n, !!timeout);
 		if (timeout)
 			hrtimer_start(&n->timer, ns_to_ktime(timeout),
 				      HRTIMER_MODE_REL_PINNED);
-		else
-			napi_gro_flush(n, false);
 	}
 	if (unlikely(!list_empty(&n->poll_list))) {
 		/* If n->poll_list is not empty, we need to mask irqs */
-- 
2.17.2

Re: [PATCH net-next] net: don't keep lonely packets forever in the gro hash

[Willem de Bruijn]

On Tue, Nov 20, 2018 at 1:19 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> Eric noted that with UDP GRO and NAPI timeout, we could keep a single
> UDP packet inside the GRO hash forever, if the related NAPI instance
> calls napi_gro_complete() at an higher frequency than the NAPI timeout.
> Willem noted that even TCP packets could be trapped there, till the
> next retransmission.
> This patch tries to address the issue, flushing the oldest packets before
> scheduling the NAPI timeout. The rationale is that such a timeout should be
> well below a jiffy and we are not flushing packets eligible for sane GRO.

Might be useful to be a bit more exact: oldest packets -> old packets,
those with a NAPI_GRO_CB age before the current jiffy. That helps
explain the "well below a jiffy" comment. I had to reread to code to
understand that this did not just flush everything, obviation the
purpose of the gro timer.

Agreed that something more fine-grained than jiffies would help. In
the case of gro_timer we know the interval, so even an age expressed
with an epoch counter would suffice? Anyway, out of scope for this patch.

> RFC -> v1:
>  - added 'Fixes tags', cleaned-up the wording.
>
> Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> Fixes: 3b47d30396ba ("net: gro: add a per device gro flush timer")
> Fixes: e20cf8d3f1f7 ("udp: implement GRO for plain UDP sockets.")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Acked on the change. Thanks, Paolo. Just a few comments and questions
about the description and target. Feel free to ignore, in which case
I'll just add my Acked-by.

> --
> Note: since one of the fixed commit is currently only on net-next,
> the other one is really old, and the affected scenario without the
> more recent commit is really a corner case, targeting net-next.

IMHO there is nothing UDP specific here. Any sender that applies GRO
and that does not use push bits can trigger this, including non-native
TCP stacks. The gro_timer is a bit of an edge case, but that makes it
fine to send it to net, too.

> ---
>  net/core/dev.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 5927f6a7c301..b6eb4e0bfa91 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5975,11 +5975,14 @@ bool napi_complete_done(struct napi_struct *n, int work_done)
>                 if (work_done)
>                         timeout = n->dev->gro_flush_timeout;
>
> +               /* When the NAPI instance uses a timeout, we still need to
> +                * somehow bound the time packets are keept in the GRO layer

nit: keept -> kept

> +                * under heavy traffic

this only requires one packet per gro_flush_timeout?

> +                */
> +               napi_gro_flush(n, !!timeout);
>                 if (timeout)
>                         hrtimer_start(&n->timer, ns_to_ktime(timeout),
>                                       HRTIMER_MODE_REL_PINNED);
> -               else
> -                       napi_gro_flush(n, false);
>         }
>         if (unlikely(!list_empty(&n->poll_list))) {
>                 /* If n->poll_list is not empty, we need to mask irqs */
> --
> 2.17.2
>