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

[PATCH net v2] 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 old packets -
those with a NAPI_GRO_CB age before the current jiffy - 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.

v1  -> v2:
 - clarified the commit message and comment

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")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 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 066aa902d85c..ddc551f24ba2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5970,11 +5970,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 and keeps postponing
+		 * it, we need to bound somehow the time packets are kept in
+		 * the GRO layer
+		 */
+		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 v2] net: don't keep lonely packets forever in the gro hash

[Willem de Bruijn]

On Wed, Nov 21, 2018 at 12:21 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 old packets -
> those with a NAPI_GRO_CB age before the current jiffy - 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.
>
> v1  -> v2:
>  - clarified the commit message and comment
>
> 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")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Acked-by: Willem de Bruijn <willemb@google.com>

Thanks for making those small changes.

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

[Eric Dumazet]

On 11/21/2018 09:21 AM, Paolo Abeni 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.

Acked-by: Eric Dumazet <edumazet@google.com>

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

[David Miller]

From: Paolo Abeni <pabeni@redhat.com>
Date: Wed, 21 Nov 2018 18:21:35 +0100

> 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 old packets -
> those with a NAPI_GRO_CB age before the current jiffy - 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.
> 
> v1  -> v2:
>  - clarified the commit message and comment
> 
> 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")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Applied and queued up for -stable.