Skip to contentSkip to conversation

tcp: RACK loss recovery bug fixes

PATCH In review Yuchung Cheng 2 participants 6 messages Dec 7, 2017 – Dec 8, 2017

Subsystems (3)
Networking Network drivers TCP
lore
Share & tools
lore

Yuchung Cheng

author
#0 lore raw

cover tcp: RACK loss recovery bug fixes

This patch set has four minor bug fixes in TCP RACK loss recovery.

Yuchung Cheng (4):
tcp: correctly test congestion state in RACK
tcp: always evaluate losses in RACK upon undo
tcp: fix off-by-one bug in RACK
tcp: evaluate packet losses upon RTT change

 net/ipv4/tcp_input.c    |  1 +
 net/ipv4/tcp_recovery.c | 28 +++++++++++++---------------
 2 files changed, 14 insertions(+), 15 deletions(-)
Signature · 1 line
2.15.1.424.g9478a66081-goog
author
#1 lore raw

PATCH 1/4 tcp: correctly test congestion state in RACK

RACK does not test the loss recovery state correctly to compute the reordering window. It assumes if lost_out is zero then TCP is not in loss recovery. But it can be zero during recovery before calling tcp_rack_detect_loss(): when an ACK acknowledges all packets marked lost before receiving this ACK, but has not yet to discover new ones by tcp_rack_detect_loss(). The fix is to simply test the congestion state directly.

Signed-off-by Yuchung Cheng <[email protected]>
Reviewed-by Neal Cardwell <[email protected]>
Reviewed-by Priyaranjan Jha <[email protected]>
Reviewed-by Eric Dumazet <[email protected]>
---
 net/ipv4/tcp_recovery.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
net/ipv4/tcp_recovery.c +2 -1
index d3ea89020c69..3143664902e9 100644
@@ -55,7 +55,8 @@ static void tcp_rack_detect_loss(struct sock *sk, u32 *reo_timeout)
 	 * to queuing or delayed ACKs.
 	 */
 	reo_wnd = 1000;
-	if ((tp->rack.reord || !tp->lost_out) && min_rtt != ~0U) {
+	if ((tp->rack.reord || inet_csk(sk)->icsk_ca_state < TCP_CA_Recovery) &&
+	    min_rtt != ~0U) {
 		reo_wnd = max((min_rtt >> 2) * tp->rack.reo_wnd_steps, reo_wnd);
 		reo_wnd = min(reo_wnd, tp->srtt_us >> 3);
 	}
Signature · 1 line
2.15.1.424.g9478a66081-goog
author
#2 lore raw

PATCH 2/4 tcp: always evaluate losses in RACK upon undo

In reply to Yuchung Cheng

When sender detects spurious retransmission, all packets marked lost are remarked to be in-flight. However some may be considered lost based on its timestamps in RACK. This patch forces RACK to re-evaluate, which may be skipped previously if the ACK does not advance RACK timestamp.

Signed-off-by Yuchung Cheng <[email protected]>
Reviewed-by Neal Cardwell <[email protected]>
Reviewed-by Priyaranjan Jha <[email protected]>
Reviewed-by Eric Dumazet <[email protected]>
---
 net/ipv4/tcp_input.c | 1 +
 1 file changed, 1 insertion(+)
net/ipv4/tcp_input.c +1 -0
index 734cfc8ff76e..5a240f9d208b 100644
@@ -2326,6 +2326,7 @@ static void tcp_undo_cwnd_reduction(struct sock *sk, bool unmark_loss)
 	}
 	tp->snd_cwnd_stamp = tcp_jiffies32;
 	tp->undo_marker = 0;
+	tp->rack.advanced = 1; /* Force RACK to re-exam losses */
 }
 
 static inline bool tcp_may_undo(const struct tcp_sock *tp)
Signature · 1 line
2.15.1.424.g9478a66081-goog
author
#3 lore raw

PATCH 3/4 tcp: fix off-by-one bug in RACK

In reply to Yuchung Cheng

RACK should mark a packet lost when remaining wait time is zero.

Signed-off-by Yuchung Cheng <[email protected]>
Reviewed-by Neal Cardwell <[email protected]>
Reviewed-by Priyaranjan Jha <[email protected]>
Reviewed-by Eric Dumazet <[email protected]>
---
 net/ipv4/tcp_recovery.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
net/ipv4/tcp_recovery.c +3 -3
index 3143664902e9..0c182303e62e 100644
@@ -80,12 +80,12 @@ static void tcp_rack_detect_loss(struct sock *sk, u32 *reo_timeout)
 		 */
 		remaining = tp->rack.rtt_us + reo_wnd -
 			    tcp_stamp_us_delta(tp->tcp_mstamp, skb->skb_mstamp);
-		if (remaining < 0) {
+		if (remaining <= 0) {
 			tcp_rack_mark_skb_lost(sk, skb);
 			list_del_init(&skb->tcp_tsorted_anchor);
 		} else {
-			/* Record maximum wait time (+1 to avoid 0) */
-			*reo_timeout = max_t(u32, *reo_timeout, 1 + remaining);
+			/* Record maximum wait time */
+			*reo_timeout = max_t(u32, *reo_timeout, remaining);
 		}
 	}
 }
Signature · 1 line
2.15.1.424.g9478a66081-goog
author
#4 lore raw

PATCH 4/4 tcp: evaluate packet losses upon RTT change

In reply to Yuchung Cheng

RACK skips an ACK unless it advances the most recently delivered TX timestamp (rack.mstamp). Since RACK also uses the most recent RTT to decide if a packet is lost, RACK should still run the loss detection whenever the most recent RTT changes. For example, an ACK that does not advance the timestamp but triggers the cwnd undo due to reordering, would then use the most recent (higher) RTT measurement to detect further losses.

Signed-off-by Yuchung Cheng <[email protected]>
Reviewed-by Neal Cardwell <[email protected]>
Reviewed-by Priyaranjan Jha <[email protected]>
Reviewed-by Eric Dumazet <[email protected]>
---
 net/ipv4/tcp_recovery.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)
net/ipv4/tcp_recovery.c +8 -11
index 0c182303e62e..3a81720ac0c4 100644
@@ -117,13 +117,8 @@ void tcp_rack_advance(struct tcp_sock *tp, u8 sacked, u32 end_seq,
 {
 	u32 rtt_us;
 
-	if (tp->rack.mstamp &&
-	    !tcp_rack_sent_after(xmit_time, tp->rack.mstamp,
-				 end_seq, tp->rack.end_seq))
-		return;
-
 	rtt_us = tcp_stamp_us_delta(tp->tcp_mstamp, xmit_time);
-	if (sacked & TCPCB_RETRANS) {
+	if (rtt_us < tcp_min_rtt(tp) && (sacked & TCPCB_RETRANS)) {
 		/* If the sacked packet was retransmitted, it's ambiguous
 		 * whether the retransmission or the original (or the prior
 		 * retransmission) was sacked.

@@ -134,13 +129,15 @@ void tcp_rack_advance(struct tcp_sock *tp, u8 sacked, u32 end_seq,
 		 * so it's at least one RTT (i.e., retransmission is at least
 		 * an RTT later).
 		 */
-		if (rtt_us < tcp_min_rtt(tp))
-			return;
+		return;
 	}
-	tp->rack.rtt_us = rtt_us;
-	tp->rack.mstamp = xmit_time;
-	tp->rack.end_seq = end_seq;
 	tp->rack.advanced = 1;
+	tp->rack.rtt_us = rtt_us;
+	if (tcp_rack_sent_after(xmit_time, tp->rack.mstamp,
+				end_seq, tp->rack.end_seq)) {
+		tp->rack.mstamp = xmit_time;
+		tp->rack.end_seq = end_seq;
+	}
 }
 
 /* We have waited long enough to accommodate reordering. Mark the expired
Signature · 1 line
2.15.1.424.g9478a66081-goog