[PATCH net 1/2] tcp: only undo on partial ACKs in CA_Loss

[PATCH net 1/2] tcp: only undo on partial ACKs in CA_Loss

[Yuchung Cheng]

Undo based on TCP timestamps should only happen on ACKs that advance
SND_UNA, according to the Eifel algorithm in RFC 3522:

Section 3.2:

  (4) If the value of the Timestamp Echo Reply field of the
      acceptable ACK's Timestamps option is smaller than the
      value of RetransmitTS, then proceed to step (5),

Section Terminology:
   We use the term 'acceptable ACK' as defined in [RFC793].  That is an
   ACK that acknowledges previously unacknowledged data.

This is because upon receiving an out-of-order packet, the receiver
returns the last timestamp that advances RCV_NXT, not the current
timestamp of the packet in the DUPACK. Without checking the flag,
the DUPACK will cause tcp_packet_delayed() to return true and
tcp_try_undo_loss() will revert cwnd reduction.

Note that we check the condition in CA_Recovery already by only
calling tcp_try_undo_partial() if FLAG_SND_UNA_ADVANCED is set or
tcp_try_undo_recovery() if snd_una crosses high_seq.

Signed-off-by: Yuchung Cheng <ycheng@google.com>
Signed-off-by: Neal Cardwell <ncardwell@google.com>
---
 net/ipv4/tcp_input.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index bc790ea..9faf775 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -2698,11 +2698,16 @@ static void tcp_process_loss(struct sock *sk, int flag, bool is_dupack)
 	struct tcp_sock *tp = tcp_sk(sk);
 	bool recovered = !before(tp->snd_una, tp->high_seq);
 
+	if ((flag & FLAG_SND_UNA_ADVANCED) &&
+	    tcp_try_undo_loss(sk, false))
+		return;
+
 	if (tp->frto) { /* F-RTO RFC5682 sec 3.1 (sack enhanced version). */
 		/* Step 3.b. A timeout is spurious if not all data are
 		 * lost, i.e., never-retransmitted data are (s)acked.
 		 */
-		if (tcp_try_undo_loss(sk, flag & FLAG_ORIG_SACK_ACKED))
+		if ((flag & FLAG_ORIG_SACK_ACKED) &&
+		    tcp_try_undo_loss(sk, true))
 			return;
 
 		if (after(tp->snd_nxt, tp->high_seq) &&
@@ -2732,8 +2737,6 @@ static void tcp_process_loss(struct sock *sk, int flag, bool is_dupack)
 		else if (flag & FLAG_SND_UNA_ADVANCED)
 			tcp_reset_reno_sack(tp);
 	}
-	if (tcp_try_undo_loss(sk, false))
-		return;
 	tcp_xmit_retransmit_queue(sk);
 }
 
-- 
2.2.0.rc0.207.ga3a616c

[PATCH net 2/2] tcp: don't over-send F-RTO probes

[Yuchung Cheng]

After sending the new data packets to probe (step 2), F-RTO may
incorrectly send more probes if the next ACK advances SND_UNA and
does not sack new packet. However F-RTO RFC 5682 probes at most
once. This bug may cause sender to always send new data instead of
repairing holes, inducing longer HoL blocking on the receiver for
the application.

Signed-off-by: Yuchung Cheng <ycheng@google.com>
Signed-off-by: Neal Cardwell <ncardwell@google.com>
---
 net/ipv4/tcp_input.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 9faf775..243d674 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -2710,9 +2710,9 @@ static void tcp_process_loss(struct sock *sk, int flag, bool is_dupack)
 		    tcp_try_undo_loss(sk, true))
 			return;
 
-		if (after(tp->snd_nxt, tp->high_seq) &&
-		    (flag & FLAG_DATA_SACKED || is_dupack)) {
-			tp->frto = 0; /* Loss was real: 2nd part of step 3.a */
+		if (after(tp->snd_nxt, tp->high_seq)) {
+			if (flag & FLAG_DATA_SACKED || is_dupack)
+				tp->frto = 0; /* Step 3.a. loss was real */
 		} else if (flag & FLAG_SND_UNA_ADVANCED && !recovered) {
 			tp->high_seq = tp->snd_nxt;
 			__tcp_push_pending_frames(sk, tcp_current_mss(sk),
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH net 1/2] tcp: only undo on partial ACKs in CA_Loss

[David Miller]

From: Yuchung Cheng <ycheng@google.com>
Date: Mon, 18 May 2015 12:31:44 -0700

> Undo based on TCP timestamps should only happen on ACKs that advance
> SND_UNA, according to the Eifel algorithm in RFC 3522:
> 
> Section 3.2:
> 
>   (4) If the value of the Timestamp Echo Reply field of the
>       acceptable ACK's Timestamps option is smaller than the
>       value of RetransmitTS, then proceed to step (5),
> 
> Section Terminology:
>    We use the term 'acceptable ACK' as defined in [RFC793].  That is an
>    ACK that acknowledges previously unacknowledged data.
> 
> This is because upon receiving an out-of-order packet, the receiver
> returns the last timestamp that advances RCV_NXT, not the current
> timestamp of the packet in the DUPACK. Without checking the flag,
> the DUPACK will cause tcp_packet_delayed() to return true and
> tcp_try_undo_loss() will revert cwnd reduction.
> 
> Note that we check the condition in CA_Recovery already by only
> calling tcp_try_undo_partial() if FLAG_SND_UNA_ADVANCED is set or
> tcp_try_undo_recovery() if snd_una crosses high_seq.
> 
> Signed-off-by: Yuchung Cheng <ycheng@google.com>
> Signed-off-by: Neal Cardwell <ncardwell@google.com>

Applied.

Re: [PATCH net 2/2] tcp: don't over-send F-RTO probes

[David Miller]

From: Yuchung Cheng <ycheng@google.com>
Date: Mon, 18 May 2015 12:31:45 -0700

> After sending the new data packets to probe (step 2), F-RTO may
> incorrectly send more probes if the next ACK advances SND_UNA and
> does not sack new packet. However F-RTO RFC 5682 probes at most
> once. This bug may cause sender to always send new data instead of
> repairing holes, inducing longer HoL blocking on the receiver for
> the application.
> 
> Signed-off-by: Yuchung Cheng <ycheng@google.com>
> Signed-off-by: Neal Cardwell <ncardwell@google.com>

Applied.