[PATCH net-2.6 0/4] [TCP]: Bug fixes & minor tweaks

[PATCH net-2.6 0/4] [TCP]: Bug fixes & minor tweaks

[Ilpo Järvinen]

Hi,                                                      
                                                         
Here are some changes that should go to net-2.6. Boot & simple
transfer tested. I'll do future cleanups that are now possible
in a different series to a next kernel version.          
                                                         
--                                                       
 i.                                                      

[PATCH 1/4] [TCP]: Consider GSO while counting reord in sacktag

[Ilpo Järvinen]

Reordering detection fails to take account that the reordered
skb may have pcount larger than 1. In such case the lowest of
them had the largest reordering, the old formula used the
highest of them which is pcount - 1 packets less reordered.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
---
 net/ipv4/tcp_input.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index ca9590f..0f75757 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1403,8 +1403,6 @@ tcp_sacktag_write_queue(struct sock *sk, struct sk_buff *ack_skb, u32 prior_snd_
 			if (in_sack < 0)
 				break;
 
-			fack_count += tcp_skb_pcount(skb);
-
 			sacked = TCP_SKB_CB(skb)->sacked;
 
 			/* Account D-SACK for retransmitted packet. */
@@ -1427,11 +1425,14 @@ tcp_sacktag_write_queue(struct sock *sk, struct sk_buff *ack_skb, u32 prior_snd_
 				}
 
 				/* Nothing to do; acked frame is about to be dropped. */
+				fack_count += tcp_skb_pcount(skb);
 				continue;
 			}
 
-			if (!in_sack)
+			if (!in_sack) {
+				fack_count += tcp_skb_pcount(skb);
 				continue;
+			}
 
 			if (!(sacked&TCPCB_SACKED_ACKED)) {
 				if (sacked & TCPCB_SACKED_RETRANS) {
@@ -1480,6 +1481,7 @@ tcp_sacktag_write_queue(struct sock *sk, struct sk_buff *ack_skb, u32 prior_snd_
 				flag |= FLAG_DATA_SACKED;
 				tp->sacked_out += tcp_skb_pcount(skb);
 
+				fack_count += tcp_skb_pcount(skb);
 				if (fack_count > tp->fackets_out)
 					tp->fackets_out = fack_count;
 
@@ -1490,6 +1492,8 @@ tcp_sacktag_write_queue(struct sock *sk, struct sk_buff *ack_skb, u32 prior_snd_
 			} else {
 				if (dup_sack && (sacked&TCPCB_RETRANS))
 					reord = min(fack_count, reord);
+
+				fack_count += tcp_skb_pcount(skb);
 			}
 
 			/* D-SACK. We can detect redundant retransmission
@@ -1515,7 +1519,7 @@ tcp_sacktag_write_queue(struct sock *sk, struct sk_buff *ack_skb, u32 prior_snd_
 
 	if ((reord < tp->fackets_out) && icsk->icsk_ca_state != TCP_CA_Loss &&
 	    (!tp->frto_highmark || after(tp->snd_una, tp->frto_highmark)))
-		tcp_update_reordering(sk, ((tp->fackets_out + 1) - reord), 0);
+		tcp_update_reordering(sk, tp->fackets_out - reord, 0);
 
 #if FASTRETRANS_DEBUG > 0
 	BUG_TRAP((int)tp->sacked_out >= 0);
-- 
1.5.0.6

[PATCH 2/4] [TCP]: Fix reord detection due to snd_una covered holes

[Ilpo Järvinen]

Fixes subtle bug like the one with fastpath_cnt_hint happening
due to the way the GSO and hints interact. Because hints are not
reset when just a GSOed skb is partially ACKed, there's no
guarantee that the relevant part of the write queue is going to
be processed in sacktag at all (skbs below snd_una) because
fastpath hint can fast forward the entrypoint.

This was also on the way of future reductions in sacktag's skb
processing. Also future cleanups in sacktag can be made after
this (in 2.6.25).

This may make reordering update in tcp_try_undo_partial
redundant but I'm not too sure so I left it there.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
---
 net/ipv4/tcp_input.c |   50 ++++++++++++++++++++++++++++++++------------------
 1 files changed, 32 insertions(+), 18 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 0f75757..9fc9096 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1417,11 +1417,6 @@ tcp_sacktag_write_queue(struct sock *sk, struct sk_buff *ack_skb, u32 prior_snd_
 					if ((dup_sack && in_sack) &&
 					    (sacked&TCPCB_SACKED_ACKED))
 						reord = min(fack_count, reord);
-				} else {
-					/* If it was in a hole, we detected reordering. */
-					if (fack_count < prior_fackets &&
-					    !(sacked&TCPCB_SACKED_ACKED))
-						reord = min(fack_count, reord);
 				}
 
 				/* Nothing to do; acked frame is about to be dropped. */
@@ -2634,7 +2629,8 @@ static u32 tcp_tso_acked(struct sock *sk, struct sk_buff *skb)
  * is before the ack sequence we can discard it as it's confirmed to have
  * arrived at the other end.
  */
-static int tcp_clean_rtx_queue(struct sock *sk, s32 *seq_rtt_p)
+static int tcp_clean_rtx_queue(struct sock *sk, s32 *seq_rtt_p,
+			       int prior_fackets)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
 	const struct inet_connection_sock *icsk = inet_csk(sk);
@@ -2643,6 +2639,8 @@ static int tcp_clean_rtx_queue(struct sock *sk, s32 *seq_rtt_p)
 	int fully_acked = 1;
 	int flag = 0;
 	int prior_packets = tp->packets_out;
+	u32 cnt = 0;
+	u32 reord = tp->packets_out;
 	s32 seq_rtt = -1;
 	ktime_t last_ackt = net_invalid_timestamp();
 
@@ -2683,10 +2681,14 @@ static int tcp_clean_rtx_queue(struct sock *sk, s32 *seq_rtt_p)
 				if ((flag & FLAG_DATA_ACKED) ||
 				    (packets_acked > 1))
 					flag |= FLAG_NONHEAD_RETRANS_ACKED;
-			} else if (seq_rtt < 0) {
-				seq_rtt = now - scb->when;
-				if (fully_acked)
-					last_ackt = skb->tstamp;
+			} else {
+				if (seq_rtt < 0) {
+					seq_rtt = now - scb->when;
+					if (fully_acked)
+						last_ackt = skb->tstamp;
+				}
+				if (!(sacked & TCPCB_SACKED_ACKED))
+					reord = min(cnt, reord);
 			}
 
 			if (sacked & TCPCB_SACKED_ACKED)
@@ -2697,12 +2699,16 @@ static int tcp_clean_rtx_queue(struct sock *sk, s32 *seq_rtt_p)
 			if ((sacked & TCPCB_URG) && tp->urg_mode &&
 			    !before(end_seq, tp->snd_up))
 				tp->urg_mode = 0;
-		} else if (seq_rtt < 0) {
-			seq_rtt = now - scb->when;
-			if (fully_acked)
-				last_ackt = skb->tstamp;
+		} else {
+			if (seq_rtt < 0) {
+				seq_rtt = now - scb->when;
+				if (fully_acked)
+					last_ackt = skb->tstamp;
+			}
+			reord = min(cnt, reord);
 		}
 		tp->packets_out -= packets_acked;
+		cnt += packets_acked;
 
 		/* Initial outgoing SYN's get put onto the write_queue
 		 * just like anything else we transmit.  It is not
@@ -2734,13 +2740,18 @@ static int tcp_clean_rtx_queue(struct sock *sk, s32 *seq_rtt_p)
 		tcp_ack_update_rtt(sk, flag, seq_rtt);
 		tcp_rearm_rto(sk);
 
+		if (tcp_is_reno(tp)) {
+			tcp_remove_reno_sacks(sk, pkts_acked);
+		} else {
+			/* Non-retransmitted hole got filled? That's reordering */
+			if (reord < prior_fackets)
+				tcp_update_reordering(sk, tp->fackets_out - reord, 0);
+		}
+
 		tp->fackets_out -= min(pkts_acked, tp->fackets_out);
 		/* hint's skb might be NULL but we don't need to care */
 		tp->fastpath_cnt_hint -= min_t(u32, pkts_acked,
 					       tp->fastpath_cnt_hint);
-		if (tcp_is_reno(tp))
-			tcp_remove_reno_sacks(sk, pkts_acked);
-
 		if (ca_ops->pkts_acked) {
 			s32 rtt_us = -1;
 
@@ -3023,6 +3034,7 @@ static int tcp_ack(struct sock *sk, struct sk_buff *skb, int flag)
 	u32 ack_seq = TCP_SKB_CB(skb)->seq;
 	u32 ack = TCP_SKB_CB(skb)->ack_seq;
 	u32 prior_in_flight;
+	u32 prior_fackets;
 	s32 seq_rtt;
 	int prior_packets;
 	int frto_cwnd = 0;
@@ -3047,6 +3059,8 @@ static int tcp_ack(struct sock *sk, struct sk_buff *skb, int flag)
 			tp->bytes_acked += min(ack - prior_snd_una, tp->mss_cache);
 	}
 
+	prior_fackets = tp->fackets_out;
+
 	if (!(flag&FLAG_SLOWPATH) && after(ack, prior_snd_una)) {
 		/* Window is constant, pure forward advance.
 		 * No more checks are required.
@@ -3088,7 +3102,7 @@ static int tcp_ack(struct sock *sk, struct sk_buff *skb, int flag)
 	prior_in_flight = tcp_packets_in_flight(tp);
 
 	/* See if we can take anything off of the retransmit queue. */
-	flag |= tcp_clean_rtx_queue(sk, &seq_rtt);
+	flag |= tcp_clean_rtx_queue(sk, &seq_rtt, prior_fackets);
 
 	/* Guarantee sacktag reordering detection against wrap-arounds */
 	if (before(tp->frto_highmark, tp->snd_una))
-- 
1.5.0.6

[PATCH 3/4] [TCP]: Add unlikely() to sacktag out-of-mem in fragment case

[Ilpo Järvinen]

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
---
 net/ipv4/tcp_input.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 9fc9096..84bcdc9 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1400,7 +1400,7 @@ tcp_sacktag_write_queue(struct sock *sk, struct sk_buff *ack_skb, u32 prior_snd_
 			/* DSACK info lost if out-of-mem, try SACK still */
 			if (in_sack <= 0)
 				in_sack = tcp_match_skb_to_sack(sk, skb, start_seq, end_seq);
-			if (in_sack < 0)
+			if (unlikely(in_sack < 0))
 				break;
 
 			sacked = TCP_SKB_CB(skb)->sacked;
-- 
1.5.0.6

[PATCH 4/4] [TCP]: Split SACK FRTO flag clearing (fixes FRTO corner case bug)

[Ilpo Järvinen]

In case we run out of mem when fragmenting, the clearing of
FLAG_ONLY_ORIG_SACKED might get missed which then feeds FRTO
with false information. Move clearing outside skb processing
loop so that it will get executed even if the skb loop
terminates prematurely due to out-of-mem.

Besides, now the core of the loop truly deals with a single
skb only, which also enables creation a more self-contained
of tcp_sacktag_one later on.

In addition, small reorganization of if branches was made.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
---
 net/ipv4/tcp_input.c |   35 +++++++++++++++++------------------
 1 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 84bcdc9..20c9440 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1444,12 +1444,17 @@ tcp_sacktag_write_queue(struct sock *sk, struct sk_buff *ack_skb, u32 prior_snd_
 						tp->retransmit_skb_hint = NULL;
 					}
 				} else {
-					/* New sack for not retransmitted frame,
-					 * which was in hole. It is reordering.
-					 */
-					if (!(sacked & TCPCB_RETRANS) &&
-					    fack_count < prior_fackets)
-						reord = min(fack_count, reord);
+					if (!(sacked & TCPCB_RETRANS)) {
+						/* New sack for not retransmitted frame,
+						 * which was in hole. It is reordering.
+						 */
+						if (fack_count < prior_fackets)
+							reord = min(fack_count, reord);
+
+						/* SACK enhanced F-RTO (RFC4138; Appendix B) */
+						if (!after(TCP_SKB_CB(skb)->end_seq, tp->frto_highmark))
+							flag |= FLAG_ONLY_ORIG_SACKED;
+					}
 
 					if (sacked & TCPCB_LOST) {
 						TCP_SKB_CB(skb)->sacked &= ~TCPCB_LOST;
@@ -1458,18 +1463,6 @@ tcp_sacktag_write_queue(struct sock *sk, struct sk_buff *ack_skb, u32 prior_snd_
 						/* clear lost hint */
 						tp->retransmit_skb_hint = NULL;
 					}
-					/* SACK enhanced F-RTO detection.
-					 * Set flag if and only if non-rexmitted
-					 * segments below frto_highmark are
-					 * SACKed (RFC4138; Appendix B).
-					 * Clearing correct due to in-order walk
-					 */
-					if (after(end_seq, tp->frto_highmark)) {
-						flag &= ~FLAG_ONLY_ORIG_SACKED;
-					} else {
-						if (!(sacked & TCPCB_RETRANS))
-							flag |= FLAG_ONLY_ORIG_SACKED;
-					}
 				}
 
 				TCP_SKB_CB(skb)->sacked |= TCPCB_SACKED_ACKED;
@@ -1503,6 +1496,12 @@ tcp_sacktag_write_queue(struct sock *sk, struct sk_buff *ack_skb, u32 prior_snd_
 				tp->retransmit_skb_hint = NULL;
 			}
 		}
+
+		/* SACK enhanced FRTO (RFC4138, Appendix B): Clearing correct
+		 * due to in-order walk
+		 */
+		if (after(end_seq, tp->frto_highmark))
+			flag &= ~FLAG_ONLY_ORIG_SACKED;
 	}
 
 	if (tp->retrans_out &&
-- 
1.5.0.6

Re: [PATCH 1/4] [TCP]: Consider GSO while counting reord in sacktag

[David Miller]

From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Sat, 10 Nov 2007 12:55:59 +0200

> Reordering detection fails to take account that the reordered
> skb may have pcount larger than 1. In such case the lowest of
> them had the largest reordering, the old formula used the
> highest of them which is pcount - 1 packets less reordered.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>

Applied, thanks.

Re: [PATCH 2/4] [TCP]: Fix reord detection due to snd_una covered holes

[David Miller]

From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Sat, 10 Nov 2007 12:56:00 +0200

> Fixes subtle bug like the one with fastpath_cnt_hint happening
> due to the way the GSO and hints interact. Because hints are not
> reset when just a GSOed skb is partially ACKed, there's no
> guarantee that the relevant part of the write queue is going to
> be processed in sacktag at all (skbs below snd_una) because
> fastpath hint can fast forward the entrypoint.
> 
> This was also on the way of future reductions in sacktag's skb
> processing. Also future cleanups in sacktag can be made after
> this (in 2.6.25).
> 
> This may make reordering update in tcp_try_undo_partial
> redundant but I'm not too sure so I left it there.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>

Thanks for discovering this.

Applied, thanks.

Re: [PATCH 3/4] [TCP]: Add unlikely() to sacktag out-of-mem in fragment case

[David Miller]

From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Sat, 10 Nov 2007 12:56:01 +0200

> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>

Applied.

Re: [PATCH 4/4] [TCP]: Split SACK FRTO flag clearing (fixes FRTO corner case bug)

[David Miller]

From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Sat, 10 Nov 2007 12:56:02 +0200

> In case we run out of mem when fragmenting, the clearing of
> FLAG_ONLY_ORIG_SACKED might get missed which then feeds FRTO
> with false information. Move clearing outside skb processing
> loop so that it will get executed even if the skb loop
> terminates prematurely due to out-of-mem.
> 
> Besides, now the core of the loop truly deals with a single
> skb only, which also enables creation a more self-contained
> of tcp_sacktag_one later on.
> 
> In addition, small reorganization of if branches was made.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>

Applied, thanks!