[PATCH net-next] net: avoid strange behavior with skb_defer_max == 1

[PATCH net-next] net: avoid strange behavior with skb_defer_max == 1

[Jakub Kicinski]

When user sets skb_defer_max to 1 the kick threshold is 0
(half of 1). If we increment queue length before the check
the kick will never happen, and the skb may get stranded.
This is likely harmless but can be avoided by moving the
increment after the check. This way skb_defer_max == 1
will always kick. Still a silly config to have, but
somehow that feels more correct.

While at it drop a comment which seems to be outdated
or confusing, and wrap the defer_count write with
a WRITE_ONCE() since it's read on the fast path
that avoids taking the lock.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/core/skbuff.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 1d10bb4adec1..5b3559cb1d82 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -6512,18 +6512,15 @@ nodefer:	__kfree_skb(skb);
 	if (READ_ONCE(sd->defer_count) >= defer_max)
 		goto nodefer;
 
-	/* We do not send an IPI or any signal.
-	 * Remote cpu will eventually call skb_defer_free_flush()
-	 */
 	spin_lock_irqsave(&sd->defer_lock, flags);
+	/* Send an IPI every time queue reaches half capacity. */
+	kick = sd->defer_count == (defer_max >> 1);
+	/* Paired with the READ_ONCE() few lines above */
+	WRITE_ONCE(sd->defer_count, sd->defer_count + 1);
+
 	skb->next = sd->defer_list;
 	/* Paired with READ_ONCE() in skb_defer_free_flush() */
 	WRITE_ONCE(sd->defer_list, skb);
-	sd->defer_count++;
-
-	/* Send an IPI every time queue reaches half capacity. */
-	kick = sd->defer_count == (defer_max >> 1);
-
 	spin_unlock_irqrestore(&sd->defer_lock, flags);
 
 	/* Make sure to trigger NET_RX_SOFTIRQ on the remote CPU
-- 
2.34.3

Re: [PATCH net-next] net: avoid strange behavior with skb_defer_max == 1

[Eric Dumazet]

On Wed, May 18, 2022 at 11:55 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> When user sets skb_defer_max to 1 the kick threshold is 0
> (half of 1). If we increment queue length before the check
> the kick will never happen, and the skb may get stranded.
> This is likely harmless but can be avoided by moving the
> increment after the check. This way skb_defer_max == 1
> will always kick. Still a silly config to have, but
> somehow that feels more correct.
>
> While at it drop a comment which seems to be outdated
> or confusing, and wrap the defer_count write with
> a WRITE_ONCE() since it's read on the fast path
> that avoids taking the lock.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---

SGTM thanks.

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

Re: [PATCH net-next] net: avoid strange behavior with skb_defer_max == 1

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 18 May 2022 11:55:22 -0700 you wrote:
> When user sets skb_defer_max to 1 the kick threshold is 0
> (half of 1). If we increment queue length before the check
> the kick will never happen, and the skb may get stranded.
> This is likely harmless but can be avoided by moving the
> increment after the check. This way skb_defer_max == 1
> will always kick. Still a silly config to have, but
> somehow that feels more correct.
> 
> [...]

Here is the summary with links:
  - [net-next] net: avoid strange behavior with skb_defer_max == 1
    https://git.kernel.org/netdev/net-next/c/c09b0cd2cc6c

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