Only advance the SACK fast-path pointer for the first block, the fast-path assumes that only the first block advances next time so we should not move the skb for the next sack blocks.

Signed-off-by Baruch Even <[email protected]>

---

I'm not sure about the fack_count part, this patch changes the value that is maintained in this case.

net/ipv4/tcp_input.c +7 -2

Index: 2.6-rc6/net/ipv4/tcp_input.c
===================================================================
@@ -1048,8 +1048,13 @@
 			int in_sack, pcount;
 			u8 sacked;
 
-			tp->fastpath_skb_hint = skb;
-			tp->fastpath_cnt_hint = fack_count;
+			if (i == 0) {
+				/* Only advance the hint for the first SACK
+				 * block, the hint is for quickly handling the
+				 * advancing of the first SACK blocks only. */
+				tp->fastpath_skb_hint = skb;
+				tp->fastpath_cnt_hint = fack_count;
+			}
 
 			/* The retransmission queue is always in order, so
 			 * we can short-circuit the walk early.

Baruch Even

author

2007-01-27 16:49 UTC #2 lore raw

PATCH 2/3 Seperate DSACK from SACK fast path

In reply to Baruch Even

Move DSACK code outside the SACK fast-path checking code. If the DSACK determined that the information was too old we stayed with a partial cache copied. Most likely this matters very little since the next packet will not be DSACK and we will find it in the cache. but it's still not good form and there is little reason to couple the two checks.

Since the SACK receive cache doesn't need the data to be in host order we also remove the ntohl in the checking loop.

Signed-off-by Baruch Even <[email protected]>

net/ipv4/tcp_input.c +31 ~~-35~~

Index: 2.6-rc6/net/ipv4/tcp_input.c
===================================================================
@@ -948,16 +948,43 @@
 		tp->fackets_out = 0;
 	prior_fackets = tp->fackets_out;
 
+	/* Check for D-SACK. */
+	if (before(ntohl(sp[0].start_seq), TCP_SKB_CB(ack_skb)->ack_seq)) {
+		dup_sack = 1;
+		tp->rx_opt.sack_ok |= 4;
+		NET_INC_STATS_BH(LINUX_MIB_TCPDSACKRECV);
+	} else if (num_sacks > 1 &&
+			!after(ntohl(sp[0].end_seq), ntohl(sp[1].end_seq)) &&
+			!before(ntohl(sp[0].start_seq), ntohl(sp[1].start_seq))) {
+		dup_sack = 1;
+		tp->rx_opt.sack_ok |= 4;
+		NET_INC_STATS_BH(LINUX_MIB_TCPDSACKOFORECV);
+	}
+
+	/* D-SACK for already forgotten data...
+	 * Do dumb counting. */
+	if (dup_sack &&
+			!after(ntohl(sp[0].end_seq), prior_snd_una) &&
+			after(ntohl(sp[0].end_seq), tp->undo_marker))
+		tp->undo_retrans--;
+
+	/* Eliminate too old ACKs, but take into
+	 * account more or less fresh ones, they can
+	 * contain valid SACK info.
+	 */
+	if (before(TCP_SKB_CB(ack_skb)->ack_seq, prior_snd_una - tp->max_window))
+		return 0;
+
 	/* SACK fastpath:
 	 * if the only SACK change is the increase of the end_seq of
 	 * the first block then only apply that SACK block
 	 * and use retrans queue hinting otherwise slowpath */
 	flag = 1;
-	for (i = 0; i< num_sacks; i++) {
-		__u32 start_seq = ntohl(sp[i].start_seq);
-		__u32 end_seq =	 ntohl(sp[i].end_seq);
+	for (i = 0; i < num_sacks; i++) {
+		__u32 start_seq = sp[i].start_seq;
+		__u32 end_seq = sp[i].end_seq;
 
-		if (i == 0){
+		if (i == 0) {
 			if (tp->recv_sack_cache[i].start_seq != start_seq)
 				flag = 0;
 		} else {

@@ -967,37 +994,6 @@
 		}
 		tp->recv_sack_cache[i].start_seq = start_seq;
 		tp->recv_sack_cache[i].end_seq = end_seq;
-
-		/* Check for D-SACK. */
-		if (i == 0) {
-			u32 ack = TCP_SKB_CB(ack_skb)->ack_seq;
-
-			if (before(start_seq, ack)) {
-				dup_sack = 1;
-				tp->rx_opt.sack_ok |= 4;
-				NET_INC_STATS_BH(LINUX_MIB_TCPDSACKRECV);
-			} else if (num_sacks > 1 &&
-				   !after(end_seq, ntohl(sp[1].end_seq)) &&
-				   !before(start_seq, ntohl(sp[1].start_seq))) {
-				dup_sack = 1;
-				tp->rx_opt.sack_ok |= 4;
-				NET_INC_STATS_BH(LINUX_MIB_TCPDSACKOFORECV);
-			}
-
-			/* D-SACK for already forgotten data...
-			 * Do dumb counting. */
-			if (dup_sack &&
-			    !after(end_seq, prior_snd_una) &&
-			    after(end_seq, tp->undo_marker))
-				tp->undo_retrans--;
-
-			/* Eliminate too old ACKs, but take into
-			 * account more or less fresh ones, they can
-			 * contain valid SACK info.
-			 */
-			if (before(ack, prior_snd_una - tp->max_window))
-				return 0;
-		}
 	}
 
 	if (flag)

David Miller

maintainer

2007-01-28 04:06 UTC #3 lore raw

Re: [PATCH 2/3] Seperate DSACK from SACK fast path

From: Baruch Even <[email protected]>
Date: Sat, 27 Jan 2007 18:49:49 +0200

Quoted 2 lines

Since the SACK receive cache doesn't need the data to be in host order we also remove the ntohl in the checking loop.

...

Quoted 6 lines

net/ipv4/tcp_input.c +3 -3

-	for (i = 0; i< num_sacks; i++) {
-		__u32 start_seq = ntohl(sp[i].start_seq);
-		__u32 end_seq =	 ntohl(sp[i].end_seq);
+	for (i = 0; i < num_sacks; i++) {
+		__u32 start_seq = sp[i].start_seq;
+		__u32 end_seq = sp[i].end_seq;

...

net/ipv4/tcp_input.c line 996

Quoted 3 lines

net/ipv4/tcp_input.c +0 -0

 		}
 		tp->recv_sack_cache[i].start_seq = start_seq;
 		tp->recv_sack_cache[i].end_seq = end_seq;

Ok, and now the sack cache and the real sack blocks are stored in net-endian and this works out because we only make direct equality comparisons with the recv_sack_cache[] entry values?

Baruch Even

author

2007-01-28 05:27 UTC #4 lore raw

Re: [PATCH 2/3] Seperate DSACK from SACK fast path

Quoted 13 lines · David Miller

From: Baruch Even <[email protected]>
Date: Sat, 27 Jan 2007 18:49:49 +0200
Quoted 2 lines
Since the SACK receive cache doesn't need the data to be in host order we also remove the ntohl in the checking loop.
...
Quoted 6 lines
net/ipv4/tcp_input.c +3 -3
-	for (i = 0; i< num_sacks; i++) {
-		__u32 start_seq = ntohl(sp[i].start_seq);
-		__u32 end_seq =	 ntohl(sp[i].end_seq);
+	for (i = 0; i < num_sacks; i++) {
+		__u32 start_seq = sp[i].start_seq;
+		__u32 end_seq = sp[i].end_seq;
...

Quoted 8 lines

net/ipv4/tcp_input.c line 996
Quoted 3 lines
net/ipv4/tcp_input.c +0 -0
 		}
 		tp->recv_sack_cache[i].start_seq = start_seq;
 		tp->recv_sack_cache[i].end_seq = end_seq;
Ok, and now the sack cache and the real sack blocks are stored in net-endian and this works out because we only make direct equality comparisons with the recv_sack_cache[] entry values?

Yes. The only comparison we do with recv_sack_cache entries is != and that works for net-endian just fine.

The only reason recv_sack_cache was in host-order before that was that start_seq and end_seq were used to do more before/after comparisons for DSACK.

Baruch

Herbert Xu

2007-01-29 05:06 UTC #5 lore raw

Re: [PATCH 2/3] Seperate DSACK from SACK fast path

Quoted 11 lines · Baruch Even

Quoted 10 lines
Quoted 3 lines
Since the SACK receive cache doesn't need the data to be in host order we also remove the ntohl in the checking loop.
...
net/ipv4/tcp_input.c line 985
Quoted 6 lines
net/ipv4/tcp_input.c +3 -3
-   for (i = 0; i< num_sacks; i++) {
-           __u32 start_seq = ntohl(sp[i].start_seq);
-           __u32 end_seq =  ntohl(sp[i].end_seq);
+   for (i = 0; i < num_sacks; i++) {
+           __u32 start_seq = sp[i].start_seq;
+           __u32 end_seq = sp[i].end_seq;

Quoted 2 lines

Yes. The only comparison we do with recv_sack_cache entries is != and that works for net-endian just fine.

In that case you need to use __be32 before Al Viro starts coming after you :)

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

David Miller

maintainer

2007-01-29 05:28 UTC #6 lore raw

Re: [PATCH 2/3] Seperate DSACK from SACK fast path

From: Herbert Xu <[email protected]>
Date: Mon, 29 Jan 2007 16:06:07 +1100

Quoted 12 lines

Quoted 11 lines · Baruch Even
Quoted 10 lines
Quoted 3 lines
Since the SACK receive cache doesn't need the data to be in host order we also remove the ntohl in the checking loop.
...
Quoted 6 lines
net/ipv4/tcp_input.c +3 -3
-   for (i = 0; i< num_sacks; i++) {
-           __u32 start_seq = ntohl(sp[i].start_seq);
-           __u32 end_seq =  ntohl(sp[i].end_seq);
+   for (i = 0; i < num_sacks; i++) {
+           __u32 start_seq = sp[i].start_seq;
+           __u32 end_seq = sp[i].end_seq;

Quoted 5 lines

Quoted 2 lines
Yes. The only comparison we do with recv_sack_cache entries is != and that works for net-endian just fine.
In that case you need to use __be32 before Al Viro starts coming after you :)

Good catch, Baruch please fix this up :-)

Baruch Even

author

2007-01-27 16:50 UTC #7 lore raw

PATCH 3/3 Check num sacks in SACK fast path

In reply to Baruch Even

When we check for SACK fast path make sure that we also have the same number of SACK blocks in the cache and in the new SACK data. This prevents us from mistakenly taking the cache data if the old data in the SACK cache is the same as the data in the SACK block.

Signed-off-by Baruch Even <[email protected]>

include/linux/tcp.h +1 -0

Index: 2.6-rc6/include/linux/tcp.h
===================================================================
@@ -317,6 +317,7 @@
 	struct tcp_sack_block selective_acks[4]; /* The SACKS themselves*/
 
 	struct tcp_sack_block recv_sack_cache[4];
+	u32     recv_sack_cache_size;
 
 	/* from STCP, retrans queue hinting */
 	struct sk_buff* lost_skb_hint;

net/ipv4/tcp_input.c +2 -1

Index: 2.6-rc6/net/ipv4/tcp_input.c
===================================================================
@@ -979,7 +979,8 @@
 	 * if the only SACK change is the increase of the end_seq of
 	 * the first block then only apply that SACK block
 	 * and use retrans queue hinting otherwise slowpath */
-	flag = 1;
+	flag = num_sacks == tp->recv_sack_cache_size;
+	tp->recv_sack_cache_size = num_sacks;
 	for (i = 0; i < num_sacks; i++) {
 		__u32 start_seq = sp[i].start_seq;
 		__u32 end_seq = sp[i].end_seq;