Date:   Mon, 22 Feb 2021 08:47:35 +0100
From:   Eric Dumazet <edumazet@...gle.com>
To:     Honglei Wang <redsky110@...il.com>
Cc:     David Miller <davem@...emloft.net>, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH] tcp: avoid unnecessary loop if even ports are used up

On Sat, Feb 20, 2021 at 12:04 PM Honglei Wang <redsky110@...il.com> wrote:
>
> We are getting port for connect() from even ports firstly now. This
> makes bind() users have more available slots at odd part. But there is a
> problem here when the even ports are used up. This happens when there
> is a flood of short life cycle connections. In this scenario, it starts
> getting ports from the odd part, but each requirement has to walk all of
> the even port and the related hash buckets (it probably gets nothing
> before the workload pressure's gone) before go to the odd part. This
> makes the code path __inet_hash_connect()->__inet_check_established()
> and the locks there hot.
>
> This patch tries to improve the strategy so we can go faster when the
> even part is used up. It'll record the last gotten port was odd or even,
> if it's an odd one, it means there is no available even port for us and
> we probably can't get an even port this time, neither. So we just walk
> 1/16 of the whole even ports. If we can get one in this way, it probably
> means there are more available even part, we'll go back to the old
> strategy and walk all of them when next connect() comes. If still can't
> get even port in the 1/16 part, we just go to the odd part directly and
> avoid doing unnecessary loop.


Your patch trades correctness for speed.

Sorry, but adding yet another static (and thus shared) variable
assuming only one process
on the physical host attempts a series of connect() is a non starter for me.

Just scanning 1/8 of even ports to decide if none of them is available
is potentially going to
not see 7/16 of potential free 4-tuple, and an application needing
28,000 4-tuple with SRCIP,DSTIP,DSTPORT being fixed
might not be able to run anymore.

If you do not care about bind() being able to find a free port, I
would suggest you add
a sysctl to simply relax the even/odd strategy that Google has been using
to avoid all these port exhaustion bugs we had in the past.
(Although now we use one netns per job, jobs are now isolated and only
can hurt themselves)



>
>
> Signed-off-by: Honglei Wang <redsky110@...il.com>
> ---
>  net/ipv4/inet_hashtables.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> index 45fb450b4522..c95bf5cf9323 100644
> --- a/net/ipv4/inet_hashtables.c
> +++ b/net/ipv4/inet_hashtables.c
> @@ -721,9 +721,10 @@ int __inet_hash_connect(struct inet_timewait_death_row *death_row,
>         struct net *net = sock_net(sk);
>         struct inet_bind_bucket *tb;
>         u32 remaining, offset;
> -       int ret, i, low, high;
> +       int ret, i, low, high, span;
>         static u32 hint;


This is an old tree, current kernels do not have this 'static u32 hint' anymore.


>
>         int l3mdev;
> +       static bool last_port_is_odd;
>
>         if (port) {
>                 head = &hinfo->bhash[inet_bhashfn(net, port,
> @@ -756,8 +757,19 @@ int __inet_hash_connect(struct inet_timewait_death_row *death_row,
>          */
>         offset &= ~1U;
>  other_parity_scan:
> +       /* If the last available port is odd, it means
> +        * we walked all of the even ports, but got
> +        * nothing last time. It's telling us the even
> +        * part is busy to get available port. In this
> +        * case, we can go a bit faster.
> +        */
> +       if (last_port_is_odd && !(offset & 1) && remaining > 32)
> +               span = 32;
> +       else
> +               span = 2;
> +
>         port = low + offset;
> -       for (i = 0; i < remaining; i += 2, port += 2) {
> +       for (i = 0; i < remaining; i += span, port += span) {
>                 if (unlikely(port >= high))
>                         port -= remaining;
>                 if (inet_is_local_reserved_port(net, port))
> @@ -806,6 +818,11 @@ int __inet_hash_connect(struct inet_timewait_death_row *death_row,
>  ok:
>         hint += i + 2;
>
> +       if (offset & 1)
> +               last_port_is_odd = true;
> +       else
> +               last_port_is_odd = false;
> +
>         /* Head lock still held and bh's disabled */
>         inet_bind_hash(sk, tb, port);
>         if (sk_unhashed(sk)) {
> --
> 2.14.1
>

Powered by blists - more mailing lists