netdev - Re: [PATCH net-next] tcp: remove SOCK_QUEUE

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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CACSApvY-5AOmkOmkevBSbo01p78KVMxWKifXB=D_=J9+aemxRA@mail.gmail.com>
Date:   Mon, 14 Sep 2020 09:45:57 -0400
From:   Soheil Hassas Yeganeh <soheil@...gle.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Eric Dumazet <eric.dumazet@...il.com>
Subject: Re: [PATCH net-next] tcp: remove SOCK_QUEUE_SHRUNK

On Mon, Sep 14, 2020 at 6:20 AM Eric Dumazet <edumazet@...gle.com> wrote:
>
> SOCK_QUEUE_SHRUNK is currently used by TCP as a temporary state
> that remembers if some room has been made in the rtx queue
> by an incoming ACK packet.
>
> This is later used from tcp_check_space() before
> considering to send EPOLLOUT.
>
> Problem is: If we receive SACK packets, and no packet
> is removed from RTX queue, we can send fresh packets, thus
> moving them from write queue to rtx queue and eventually
> empty the write queue.
>
> This stall can happen if TCP_NOTSENT_LOWAT is used.
>
> With this fix, we no longer risk stalling sends while holes
> are repaired, and we can fully use socket sndbuf.
>
> This also removes a cache line dirtying for typical RPC
> workloads.
>
> Fixes: c9bee3b7fdec ("tcp: TCP_NOTSENT_LOWAT socket option")
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> Cc: Soheil Hassas Yeganeh <soheil@...gle.com>

Acked-by: Soheil Hassas Yeganeh <soheil@...gle.com>

Thank you for fixing this bug!


> ---
>  include/net/sock.h    |  2 --
>  net/ipv4/tcp_input.c  | 23 +++++++----------------
>  net/ipv4/tcp_output.c |  1 -
>  3 files changed, 7 insertions(+), 19 deletions(-)
>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 7dd3051551fbad3f432c969e16c04ff7f63bbe26..eaa5cac5e8368bf1c18e221fffa321d692579bad 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -845,7 +845,6 @@ enum sock_flags {
>         SOCK_RCVTSTAMP, /* %SO_TIMESTAMP setting */
>         SOCK_RCVTSTAMPNS, /* %SO_TIMESTAMPNS setting */
>         SOCK_LOCALROUTE, /* route locally only, %SO_DONTROUTE setting */
> -       SOCK_QUEUE_SHRUNK, /* write queue has been shrunk recently */
>         SOCK_MEMALLOC, /* VM depends on this socket for swapping */
>         SOCK_TIMESTAMPING_RX_SOFTWARE,  /* %SOF_TIMESTAMPING_RX_SOFTWARE */
>         SOCK_FASYNC, /* fasync() active */
> @@ -1526,7 +1525,6 @@ static inline void sk_mem_uncharge(struct sock *sk, int size)
>  DECLARE_STATIC_KEY_FALSE(tcp_tx_skb_cache_key);
>  static inline void sk_wmem_free_skb(struct sock *sk, struct sk_buff *skb)
>  {
> -       sock_set_flag(sk, SOCK_QUEUE_SHRUNK);
>         sk_wmem_queued_add(sk, -skb->truesize);
>         sk_mem_uncharge(sk, skb->truesize);
>         if (static_branch_unlikely(&tcp_tx_skb_cache_key) &&
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 3658ad84f0c6252658dd5174eae31dcde4b34942..50834e7f958eec178ea144e8d438c98f2cff9014 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -5332,12 +5332,6 @@ static bool tcp_should_expand_sndbuf(const struct sock *sk)
>         return true;
>  }
>
> -/* When incoming ACK allowed to free some skb from write_queue,
> - * we remember this event in flag SOCK_QUEUE_SHRUNK and wake up socket
> - * on the exit from tcp input handler.
> - *
> - * PROBLEM: sndbuf expansion does not work well with largesend.
> - */
>  static void tcp_new_space(struct sock *sk)
>  {
>         struct tcp_sock *tp = tcp_sk(sk);
> @@ -5352,16 +5346,13 @@ static void tcp_new_space(struct sock *sk)
>
>  static void tcp_check_space(struct sock *sk)
>  {
> -       if (sock_flag(sk, SOCK_QUEUE_SHRUNK)) {
> -               sock_reset_flag(sk, SOCK_QUEUE_SHRUNK);
> -               /* pairs with tcp_poll() */
> -               smp_mb();
> -               if (sk->sk_socket &&
> -                   test_bit(SOCK_NOSPACE, &sk->sk_socket->flags)) {
> -                       tcp_new_space(sk);
> -                       if (!test_bit(SOCK_NOSPACE, &sk->sk_socket->flags))
> -                               tcp_chrono_stop(sk, TCP_CHRONO_SNDBUF_LIMITED);
> -               }
> +       /* pairs with tcp_poll() */
> +       smp_mb();
> +       if (sk->sk_socket &&
> +           test_bit(SOCK_NOSPACE, &sk->sk_socket->flags)) {
> +               tcp_new_space(sk);
> +               if (!test_bit(SOCK_NOSPACE, &sk->sk_socket->flags))
> +                       tcp_chrono_stop(sk, TCP_CHRONO_SNDBUF_LIMITED);
>         }
>  }
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index ab79d36ed07fc6918035ab3f9ebdb3ce6b7767c8..386978dcd318d84af486d0d1a5bb1786f4a493cf 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -1682,7 +1682,6 @@ int tcp_trim_head(struct sock *sk, struct sk_buff *skb, u32 len)
>                 skb->truesize      -= delta_truesize;
>                 sk_wmem_queued_add(sk, -delta_truesize);
>                 sk_mem_uncharge(sk, delta_truesize);
> -               sock_set_flag(sk, SOCK_QUEUE_SHRUNK);
>         }
>
>         /* Any change of skb->len requires recalculation of tso factor. */
> --
> 2.28.0.618.gf4bc123cb7-goog
>