From: Lorenz Bauer <lmb@xxxxxxxxxxxxx>
Date: Mon, 26 Jun 2023 16:08:58 +0100
> Contrary to TCP, UDP reuseport groups can contain TCP_ESTABLISHED
> sockets. To support these properly we remember whether a group has
> a connected socket and skip the fast reuseport early-return. In
> effect we continue scoring all reuseport sockets and then choose the
> one with the highest score.
>
> The current code fails to re-calculate the score for the result of
> lookup_reuseport. According to Kuniyuki Iwashima:
>
> 1) SO_INCOMING_CPU is set
> -> selected sk might have +1 score
>
> 2) BPF prog returns ESTABLISHED and/or SO_INCOMING_CPU sk
> -> selected sk will have more than 8
>
> Using the old score could trigger more lookups depending on the
> order that sockets are created.
>
> sk -> sk (SO_INCOMING_CPU) -> sk (ESTABLISHED)
> | |
> `-> select the next SO_INCOMING_CPU sk
> |
> `-> select itself (We should save this lookup)
>
> Fixes: efc6b6f6c311 ("udp: Improve load balancing for SO_REUSEPORT.")
> Signed-off-by: Lorenz Bauer <lmb@xxxxxxxxxxxxx>
Reviewed-by: Kuniyuki Iwashima <kuniyu@xxxxxxxxxx>
1 minor nit below.
> ---
> net/ipv4/udp.c | 20 +++++++++++++++-----
> net/ipv6/udp.c | 19 ++++++++++++++-----
> 2 files changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index fd3dae081f3a..5ef478d2c408 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -450,14 +450,24 @@ static struct sock *udp4_lib_lookup2(struct net *net,
> score = compute_score(sk, net, saddr, sport,
> daddr, hnum, dif, sdif);
> if (score > badness) {
> - result = lookup_reuseport(net, sk, skb,
> - saddr, sport, daddr, hnum);
> + badness = score;
> + result = lookup_reuseport(net, sk, skb, saddr, sport, daddr, hnum);
> + if (!result) {
> + result = sk;
> + continue;
> + }
> +
> /* Fall back to scoring if group has connections */
> - if (result && !reuseport_has_conns(sk))
> + if (!reuseport_has_conns(sk))
> return result;
>
> - result = result ? : sk;
> - badness = score;
> + /* Reuseport logic returned an error, keep original score. */
> + if (IS_ERR(result))
> + continue;
> +
> + badness = compute_score(result, net, saddr, sport,
> + daddr, hnum, dif, sdif);
> +
Unnecessary blank line here.
> }
> }
> return result;
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index e5a337e6b970..8b3cb1d7da7c 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -193,14 +193,23 @@ static struct sock *udp6_lib_lookup2(struct net *net,
> score = compute_score(sk, net, saddr, sport,
> daddr, hnum, dif, sdif);
> if (score > badness) {
> - result = lookup_reuseport(net, sk, skb,
> - saddr, sport, daddr, hnum);
> + badness = score;
> + result = lookup_reuseport(net, sk, skb, saddr, sport, daddr, hnum);
> + if (!result) {
> + result = sk;
> + continue;
> + }
> +
> /* Fall back to scoring if group has connections */
> - if (result && !reuseport_has_conns(sk))
> + if (!reuseport_has_conns(sk))
> return result;
>
> - result = result ? : sk;
> - badness = score;
> + /* Reuseport logic returned an error, keep original score. */
> + if (IS_ERR(result))
> + continue;
> +
> + badness = compute_score(sk, net, saddr, sport,
> + daddr, hnum, dif, sdif);
> }
> }
> return result;
>
> --
> 2.40.1
