Messages in this thread | | | From | Menglong Dong <> | Date | Tue, 8 Aug 2023 09:58:16 +0800 | Subject | Re: [PATCH net-next v2 1/3] net: tcp: send zero-window ACK when no memory |
| |
On Mon, Aug 7, 2023 at 10:05 PM Eric Dumazet <edumazet@google.com> wrote: > > On Mon, Aug 7, 2023 at 3:47 PM <menglong8.dong@gmail.com> wrote: > > > > From: Menglong Dong <imagedong@tencent.com> > > > > For now, skb will be dropped when no memory, which makes client keep > > retrans util timeout and it's not friendly to the users. > > > > In this patch, we reply an ACK with zero-window in this case to update > > the snd_wnd of the sender to 0. Therefore, the sender won't timeout the > > connection and will probe the zero-window with the retransmits. > > > > Signed-off-by: Menglong Dong <imagedong@tencent.com> > > --- > > v2: > > - send 0 rwin ACK for the receive queue empty case when necessary > > - send the ACK immediately by using the ICSK_ACK_NOW flag > > --- > > include/net/inet_connection_sock.h | 3 ++- > > net/ipv4/tcp_input.c | 14 +++++++++++--- > > net/ipv4/tcp_output.c | 14 +++++++++++--- > > 3 files changed, 24 insertions(+), 7 deletions(-) > > > > diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h > > index c2b15f7e5516..be3c858a2ebb 100644 > > --- a/include/net/inet_connection_sock.h > > +++ b/include/net/inet_connection_sock.h > > @@ -164,7 +164,8 @@ enum inet_csk_ack_state_t { > > ICSK_ACK_TIMER = 2, > > ICSK_ACK_PUSHED = 4, > > ICSK_ACK_PUSHED2 = 8, > > - ICSK_ACK_NOW = 16 /* Send the next ACK immediately (once) */ > > + ICSK_ACK_NOW = 16, /* Send the next ACK immediately (once) */ > > + ICSK_ACK_NOMEM = 32, > > }; > > > > void inet_csk_init_xmit_timers(struct sock *sk, > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > > index 8e96ebe373d7..aae485d0a3b6 100644 > > --- a/net/ipv4/tcp_input.c > > +++ b/net/ipv4/tcp_input.c > > @@ -5059,12 +5059,20 @@ static void tcp_data_queue(struct sock *sk, struct sk_buff *skb) > > > > /* Ok. In sequence. In window. */ > > queue_and_out: > > - if (skb_queue_len(&sk->sk_receive_queue) == 0) > > - sk_forced_mem_schedule(sk, skb->truesize); > > - else if (tcp_try_rmem_schedule(sk, skb, skb->truesize)) { > > + if (skb_queue_len(&sk->sk_receive_queue) == 0) { > > + if (tcp_try_rmem_schedule(sk, skb, skb->truesize)) { > > + sk_forced_mem_schedule(sk, skb->truesize); > > I think we want sk->sk_data_ready() here, to let applications drain the queue, > regardless of sk->sk_rcvlowat value. > > > > + inet_csk(sk)->icsk_ack.pending |= > > + (ICSK_ACK_NOMEM | ICSK_ACK_NOW); > > + inet_csk_schedule_ack(sk); > > + } > > + } else if (tcp_try_rmem_schedule(sk, skb, skb->truesize)) { > > reason = SKB_DROP_REASON_PROTO_MEM; > > NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPRCVQDROP); > > sk->sk_data_ready(sk); > > We also want to keep this sk->sk_data_ready(sk) call. > > > + inet_csk(sk)->icsk_ack.pending |= > > + (ICSK_ACK_NOMEM | ICSK_ACK_NOW); > > + inet_csk_schedule_ack(sk); > > goto drop; > > } > > This would suggest a different code refactoring, to avoid code duplication. > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > index 57c8af1859c16eba5e952a23ea959b628006f9c1..dde6c44f2c1e33dcf60c23b49cd99f270874ca96 > 100644 > --- a/net/ipv4/tcp_input.c > +++ b/net/ipv4/tcp_input.c > @@ -5050,13 +5050,17 @@ static void tcp_data_queue(struct sock *sk, > struct sk_buff *skb) > > /* Ok. In sequence. In window. */ > queue_and_out: > - if (skb_queue_len(&sk->sk_receive_queue) == 0) > - sk_forced_mem_schedule(sk, skb->truesize); > - else if (tcp_try_rmem_schedule(sk, skb, skb->truesize)) { > - reason = SKB_DROP_REASON_PROTO_MEM; > - NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPRCVQDROP); > + if (tcp_try_rmem_schedule(sk, skb, skb->truesize)) { > + /* TODO: maybe ratelimit these WIN 0 ACK ? */ > + inet_csk(sk)->icsk_ack.pending |= > CSK_ACK_NOMEM | ICSK_ACK_NOW; > + inet_csk_schedule_ack(sk); > sk->sk_data_ready(sk); > - goto drop; > + if (skb_queue_len(&sk->sk_receive_queue)) { > + reason = SKB_DROP_REASON_PROTO_MEM; > + NET_INC_STATS(sock_net(sk), > LINUX_MIB_TCPRCVQDROP); > + goto drop; > + } > + sk_forced_mem_schedule(sk, skb->truesize); > } > > eaten = tcp_queue_rcv(sk, skb, &fragstolen);
Looks much better, thank you!
| |