[PATCH net-next 1/1] tcp: Prevent needless syn-ack rexmt during TWHS

[PATCH net-next 1/1] tcp: Prevent needless syn-ack rexmt during TWHS

[Vijay Subramanian]

If server socket is slow to accept() connections, request_socks can represent
connections for which the three-way handshake is already done.  From client's
point of view, the connection is in ESTABLISHED state but on server side, socket
is not in accept_queue or ESTABLISHED state.  When the syn-ack timer expires,
because of the order in which tests are performed, server can retransmit the
synack repeatedly. Following patch prevents the server from retransmitting the
synack needlessly (and prevents client from replying with ack).  This reduces
traffic when server is slow to accept() connections.

If the server socket has received the third ack during connection establishment,
this is remembered in inet_rsk(req)->acked.  The request_sock will expire in
around 30 seconds and will be dropped if it does not move into accept_queue.

With help from Eric Dumazet.

Reported-by: Elliot Hughes <enh@google.com>
Cc: Eric Dumazet <edumazet@google.com>
Signed-off-by: Vijay Subramanian <subramanian.vijay@gmail.com>
---
Ignoring "WARNING: line over 80 characters" in the interest of readability.

Eric,
What about your earlier patch that modified sk_acceptq_is_full()?

 net/ipv4/inet_connection_sock.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index d34ce29..4e8e52e 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -598,9 +598,8 @@ void inet_csk_reqsk_queue_prune(struct sock *parent,
 					       &expire, &resend);
 				req->rsk_ops->syn_ack_timeout(parent, req);
 				if (!expire &&
-				    (!resend ||
-				     !req->rsk_ops->rtx_syn_ack(parent, req, NULL) ||
-				     inet_rsk(req)->acked)) {
+				    (!resend || inet_rsk(req)->acked ||
+				     !req->rsk_ops->rtx_syn_ack(parent, req, NULL))) {
 					unsigned long timeo;
 
 					if (req->retrans++ == 0)
-- 
1.7.0.4

Re: [PATCH net-next 1/1] tcp: Prevent needless syn-ack rexmt during TWHS

[Neal Cardwell]

On Thu, Oct 25, 2012 at 8:45 PM, Vijay Subramanian
<subramanian.vijay@gmail.com> wrote:
> If server socket is slow to accept() connections, request_socks can represent
> connections for which the three-way handshake is already done.  From client's
> point of view, the connection is in ESTABLISHED state but on server side, socket
> is not in accept_queue or ESTABLISHED state.  When the syn-ack timer expires,
> because of the order in which tests are performed, server can retransmit the
> synack repeatedly. Following patch prevents the server from retransmitting the
> synack needlessly (and prevents client from replying with ack).  This reduces
> traffic when server is slow to accept() connections.
>
> If the server socket has received the third ack during connection establishment,
> this is remembered in inet_rsk(req)->acked.  The request_sock will expire in
> around 30 seconds and will be dropped if it does not move into accept_queue.
>
> With help from Eric Dumazet.
>
> Reported-by: Elliot Hughes <enh@google.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Vijay Subramanian <subramanian.vijay@gmail.com>
> ---

Acked-by: Neal Cardwell <ncardwell@google.com>
Tested-by: Neal Cardwell <ncardwell@google.com>

Nice. Verified the bug and the fix.

neal

Re: [PATCH net-next 1/1] tcp: Prevent needless syn-ack rexmt during TWHS

[Eric Dumazet]

On Thu, 2012-10-25 at 17:45 -0700, Vijay Subramanian wrote:
> If server socket is slow to accept() connections, request_socks can represent
> connections for which the three-way handshake is already done.  From client's
> point of view, the connection is in ESTABLISHED state but on server side, socket
> is not in accept_queue or ESTABLISHED state.  When the syn-ack timer expires,
> because of the order in which tests are performed, server can retransmit the
> synack repeatedly. Following patch prevents the server from retransmitting the
> synack needlessly (and prevents client from replying with ack).  This reduces
> traffic when server is slow to accept() connections.
> 
> If the server socket has received the third ack during connection establishment,
> this is remembered in inet_rsk(req)->acked.  The request_sock will expire in
> around 30 seconds and will be dropped if it does not move into accept_queue.
> 
> With help from Eric Dumazet.
> 
> Reported-by: Elliot Hughes <enh@google.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Vijay Subramanian <subramanian.vijay@gmail.com>
> ---
> Ignoring "WARNING: line over 80 characters" in the interest of readability.
> 
> Eric,
> What about your earlier patch that modified sk_acceptq_is_full()?

Acked-by: Eric Dumazet <edumazet@google.com>

Please note Elliott Hughes takes two t, and by the way didnt reported
this particular problem. but the fact that client got the SYNACK message
while socket was not yet available to the server side.

About earlier patch, this would break :

tcp_abort_on_overflow - BOOLEAN
        If listening service is too slow to accept new connections,
        reset them. Default state is FALSE. It means that if overflow
        occurred due to a burst, connection will recover. Enable this
        option _only_ if you are really sure that listening daemon
        cannot be tuned to accept connections faster. Enabling this
        option can harm clients of your server.

And even if tcp_abort_on_overflow is TRUE, earlier patch would break
some ability to have a momentary burst of SYN packets above the listen()
backlog.

Re: [PATCH net-next 1/1] tcp: Prevent needless syn-ack rexmt during TWHS

[Eric Dumazet]

On Fri, 2012-10-26 at 07:06 +0200, Eric Dumazet wrote:

> About earlier patch, this would break :
> 
> tcp_abort_on_overflow - BOOLEAN
...
> And even if tcp_abort_on_overflow is TRUE, earlier patch would break
> some ability to have a momentary burst of SYN packets above the listen()
> backlog.

So instead of sending SYNACK immediately, we could try to defer it a
bit.

But before adding yet another trick, we should evaluate if this is
really needed.

The timer wheel we currently use on the listener socket limits us
because of the granularity of the timer (one single timer for all
requests)