[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 12 Dec 2019 17:54:10 -0500
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>,
Neal Cardwell <ncardwell@...gle.com>,
Jason Baron <jbaron@...mai.com>,
Eric Dumazet <eric.dumazet@...il.com>
Subject: Re: [PATCH net 2/3] tcp: refine tcp_write_queue_empty() implementation
On Thu, Dec 12, 2019 at 3:55 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
> Due to how tcp_sendmsg() is implemented, we can have an empty
> skb at the tail of the write queue.
>
> Most [1] tcp_write_queue_empty() callers want to know if there is
> anything to send (payload and/or FIN)
>
> Instead of checking if the sk_write_queue is empty, we need
> to test if tp->write_seq == tp->snd_nxt
>
> [1] tcp_send_fin() was the only caller that expected to
> see if an skb was in the write queue, I have changed the code
> to reuse the tcp_write_queue_tail() result.
>
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> Cc: Neal Cardwell <ncardwell@...gle.com>
Acked-by: Soheil Hassas Yeganeh <soheil@...gle.com>
Nice catch!
> ---
> include/net/tcp.h | 11 ++++++++++-
> net/ipv4/tcp_output.c | 5 +++--
> 2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 86b9a8766648824c0f122f6c01f55d59bd0d7d72..e460ea7f767ba627972a63a974cae80357808366 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1766,9 +1766,18 @@ static inline bool tcp_skb_is_last(const struct sock *sk,
> return skb_queue_is_last(&sk->sk_write_queue, skb);
> }
>
> +/**
> + * tcp_write_queue_empty - test if any payload (or FIN) is available in write queue
> + * @sk: socket
> + *
> + * Since the write queue can have a temporary empty skb in it,
> + * we must not use "return skb_queue_empty(&sk->sk_write_queue)"
> + */
> static inline bool tcp_write_queue_empty(const struct sock *sk)
> {
> - return skb_queue_empty(&sk->sk_write_queue);
> + const struct tcp_sock *tp = tcp_sk(sk);
> +
> + return tp->write_seq == tp->snd_nxt;
> }
>
> static inline bool tcp_rtx_queue_empty(const struct sock *sk)
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 57f434a8e41ffd6bc584cb4d9e87703491a378c1..36902d08473ec7e45a654234b407217ee6c65fb1 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -3129,7 +3129,7 @@ void sk_forced_mem_schedule(struct sock *sk, int size)
> */
> void tcp_send_fin(struct sock *sk)
> {
> - struct sk_buff *skb, *tskb = tcp_write_queue_tail(sk);
> + struct sk_buff *skb, *tskb, *tail = tcp_write_queue_tail(sk);
> struct tcp_sock *tp = tcp_sk(sk);
>
> /* Optimization, tack on the FIN if we have one skb in write queue and
> @@ -3137,6 +3137,7 @@ void tcp_send_fin(struct sock *sk)
> * Note: in the latter case, FIN packet will be sent after a timeout,
> * as TCP stack thinks it has already been transmitted.
> */
> + tskb = tail;
> if (!tskb && tcp_under_memory_pressure(sk))
> tskb = skb_rb_last(&sk->tcp_rtx_queue);
>
> @@ -3144,7 +3145,7 @@ void tcp_send_fin(struct sock *sk)
> TCP_SKB_CB(tskb)->tcp_flags |= TCPHDR_FIN;
> TCP_SKB_CB(tskb)->end_seq++;
> tp->write_seq++;
> - if (tcp_write_queue_empty(sk)) {
> + if (!tail) {
> /* This means tskb was already sent.
> * Pretend we included the FIN on previous transmit.
> * We need to set tp->snd_nxt to the value it would have
> --
> 2.24.1.735.g03f4e72817-goog
>
Powered by blists - more mailing lists