On Sat, Jun 28, 2014 at 5:16 AM, Neal Cardwell [off-list ref] wrote:

On Wed, Jun 25, 2014 at 10:09 AM, Octavian Purdila
[off-list ref] wrote:

quoted

Commit 016818d07 (tcp: TCP Fast Open Server - take SYNACK RTT after
completing 3WHS) changes the code to only take a snt_synack timestamp
when a SYNACK transmit or retransmit succeeds. This behaviour is later
broken by commit 843f4a55e (tcp: use tcp_v4_send_synack on first
SYN-ACK), as snt_synack is now updated even if tcp_v4_send_synack
fails.

Also, commit 3a19ce0ee (tcp: IPv6 support for fastopen server) misses
the required IPv6 updates for 016818d07.

This patch makes sure that snt_synack is updated only when the SYNACK
trasnmit/retransmit succeeds, for both IPv4 and IPv6.

Cc: Cardwell <ncardwell@google.com>
Cc: Daniel Lee <redacted>
Cc: Yuchung Cheng <redacted>

Signed-off-by: Octavian Purdila <redacted>

quoted

diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 08ae3da..a962455 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c

@@ -497,6 +497,8 @@ static int tcp_v6_send_synack(struct sock *sk, struct dst_entry *dst,
                skb_set_queue_mapping(skb, queue_mapping);
                err = ip6_xmit(sk, skb, fl6, np->opt, np->tclass);
                err = net_xmit_eval(err);
+               if (!tcp_rsk(req)->snt_synack && !err)
+                       tcp_rsk(req)->snt_synack = tcp_time_stamp;
        }

Octavian, thanks for finding this issue!

Hi Neal,

I think we should resolve the inconsistency you have discovered by
converging on the approach in Yuchung's 843f4a55e (tcp: use
tcp_v4_send_synack on first SYN-ACK), which restores the original
behavior of snt_synack from the commit where it was added by Jerry:
9ad7c049f0f79 ("tcp: RFC2988bis + taking RTT sample from 3WHS for the
passive open side") in v3.1. In that commit tcp_v4_conn_request() and
tcp_v6_conn_request() unconditionally store the time at which the
server received the first client SYN in snt_synack:

  tcp_rsk(req)->snt_synack = tcp_time_stamp;

At Google we have found that behavior very useful because it allows
extracting useful performance metrics summarizing how long it took to
establish a passive TCP connection.

In that case perhaps it is better to add a new field (or rename the
existing one if it is not needed anymore) to store the syn arrival
time? I think it is confusing to store the syn arrival time in the
"synack sent time" field.