netdev - Re: [PATCH] tcp: avoid unnecessary loop if even ports are used up

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+kCZ7_hUzitt9rjdUkJxWa+pMXU+3321Yw59n=8+NNpT5YDUg@mail.gmail.com>
Date:   Mon, 22 Feb 2021 09:11:26 +0800
From:   Honglei Wang <redsky110@...il.com>
To:     Jeremy Sowden <jeremy@...zel.net>
Cc:     David Miller <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>, netdev@...r.kernel.org
Subject: Re: [PATCH] tcp: avoid unnecessary loop if even ports are used up

On Sun, Feb 21, 2021 at 6:06 AM Jeremy Sowden <jeremy@...zel.net> wrote:
>
> On 2021-02-20, at 13:44:02 -0800, David Miller wrote:
> > From: Honglei Wang <redsky110@...il.com>
> > Date: Sat, 20 Feb 2021 19:03:56 +0800
> >
> > > 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.
> > >
> > > 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;
> > >     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)
> >
> > The first time this executes, won't last_port_is_odd be uninitialized?
>
> It's declared static, so it will be zero-initialized.
>
Yep, it'll be initialized as zero by default. I tried to initialize it
explicitly to false for the code logic, but the checkpatch.pl yielded
that it's incorrect to initialize static bool to false.
(Sorry for the last response which I forgot send with plain text mode..)

Thanks,
Honglei
> J.