Re: [PATCH] RFC2988bis + taking RTT sample from 3WHS for the passive open side

+netdev (add missing cc)

On Wed, Jun 8, 2011 at 9:30 AM, Eric Dumazet [off-list ref] wrote:

Le mardi 07 juin 2011 à 22:00 -0700, H.K. Jerry Chu a écrit :

quoted

From: Jerry Chu <redacted>

This patch lowers the default initRTO from 3secs to 1sec per
RFC2988bis. It falls back to 3secs if the SYN or SYN-ACK packet
has been retransmitted, AND the TCP timestamp option is not on.

It also adds support to take RTT sample during 3WHS on the passive
open side, just like its active open counterpart, and uses it, if
valid, to seed the initRTO for the data transmission phase.

The patch also resets ssthresh to its initial default at the
beginning of the data transmission phase, and reduces cwnd to 1 if
there has been MORE THAN ONE retransmission during 3WHS per RFC5681.

Signed-off-by: H.K. Jerry Chu <redacted>
---

...

quoted

@@ -1854,7 +1859,7 @@ static int tcp_v4_init_sock(struct sock *sk)

       * algorithms that we must have the following bandaid to talk
       * efficiently to them.  -DaveM
       */
-     tp->snd_cwnd = 2;
+     tp->snd_cwnd = TCP_INIT_CWND;

Hmm, are you sure this belongs to this patch ?

quoted

      /* See draft-stevens-tcpca-spec-01 for discussion of the
       * initialization of these values.

diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 80b1f80..d2fe4e0 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c

@@ -486,7 +486,7 @@ struct sock *tcp_create_openreq_child(struct sock *sk, struct request_sock *req,

               * algorithms that we must have the following bandaid to talk
               * efficiently to them.  -DaveM
               */
-             newtp->snd_cwnd = 2;
+             newtp->snd_cwnd = TCP_INIT_CWND;

same here ?

It's part of the cwnd initialization stuff, which I included some fix
per RFC5681 (in
tcp_init_metrics()). The field is currently not being used anyway
until data-in-SYN is
supported. I fixed it here just because TCP_INIT_CWND is the right value.

quoted

              newtp->snd_cwnd_cnt = 0;
              newtp->bytes_acked = 0;

@@ -720,6 +720,10 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,

              NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_TCPDEFERACCEPTDROP);
              return NULL;
      }
+     if (tmp_opt.saw_tstamp && tmp_opt.rcv_tsecr)
+             tcp_rsk(req)->snt_synack = tmp_opt.rcv_tsecr;
+     else if (req->retrans) /* don't take RTT sample if retrans && ~TS */
+             tcp_rsk(req)->snt_synack = 0;

Could you clarify why this is done in this order ?

What else is more appropriate? It needs to decide if we've got a valid
RTT sample.
If TS is on and rcv_tsec != 0 then we've got a valid RTT sample. Otherwise it
does not take sample if there has been retrans per Karn's algorithm.

Thanks,

Jerry

Thanks !