[PATCH 0/3] Fix issues with SACK processing

[PATCH 0/3] Fix issues with SACK processing

[Baruch Even]

These patches are intended to fix the issues I've raised in a former
email in addition to the sorting code.

I still was not able to runtime test these patches, they were only
compile tested.

Baruch

[PATCH 1/3] Advance fast path pointer for first block only

[Baruch Even]

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 <baruch@ev-en.org>

---

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

Index: 2.6-rc6/net/ipv4/tcp_input.c
===================================================================
--- 2.6-rc6.orig/net/ipv4/tcp_input.c	2007-01-27 14:53:27.000000000 +0200
+++ 2.6-rc6/net/ipv4/tcp_input.c	2007-01-27 15:59:22.000000000 +0200
@@ -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.

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

[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 <baruch@ev-en.org>

Index: 2.6-rc6/net/ipv4/tcp_input.c
===================================================================
--- 2.6-rc6.orig/net/ipv4/tcp_input.c	2007-01-27 15:06:30.000000000 +0200
+++ 2.6-rc6/net/ipv4/tcp_input.c	2007-01-27 15:59:15.000000000 +0200
@@ -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)

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

[David Miller]

From: Baruch Even <baruch@ev-en.org>
Date: Sat, 27 Jan 2007 18:49:49 +0200

> Since the SACK receive cache doesn't need the data to be in host
> order we also remove the ntohl in the checking loop.
 ...
> -	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;
 ...
>  		}
>  		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?

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

[Baruch Even]

* David Miller <davem@davemloft.net> [070128 06:06]:
> From: Baruch Even <baruch@ev-en.org>
> Date: Sat, 27 Jan 2007 18:49:49 +0200
> 
> > Since the SACK receive cache doesn't need the data to be in host
> > order we also remove the ntohl in the checking loop.
>  ...
> > -	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;
>  ...
> >  		}
> >  		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

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

[Herbert Xu]

Baruch Even <baruch@ev-en.org> wrote:
>> 
>> > Since the SACK receive cache doesn't need the data to be in host
>> > order we also remove the ntohl in the checking loop.
>>  ...
>> > -   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;
> 
> 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 :)
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Mon, 29 Jan 2007 16:06:07 +1100

> Baruch Even <baruch@ev-en.org> wrote:
> >> 
> >> > Since the SACK receive cache doesn't need the data to be in host
> >> > order we also remove the ntohl in the checking loop.
> >>  ...
> >> > -   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;
> > 
> > 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 :-)

[PATCH 3/3] Check num sacks in SACK fast path

[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 <baruch@ev-en.org>

Index: 2.6-rc6/include/linux/tcp.h
===================================================================
--- 2.6-rc6.orig/include/linux/tcp.h	2007-01-27 15:06:02.000000000 +0200
+++ 2.6-rc6/include/linux/tcp.h	2007-01-27 15:19:04.000000000 +0200
@@ -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;
Index: 2.6-rc6/net/ipv4/tcp_input.c
===================================================================
--- 2.6-rc6.orig/net/ipv4/tcp_input.c	2007-01-27 15:18:30.000000000 +0200
+++ 2.6-rc6/net/ipv4/tcp_input.c	2007-01-27 15:30:09.000000000 +0200
@@ -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;

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

[David Miller]

From: Baruch Even <baruch@ev-en.org>
Date: Mon, 29 Jan 2007 09:13:44 +0200

> +	for (i = 0; i < num_sacks; i++) {
> +		__be32 start_seq = sp[i].start_seq;
> +		__be32 end_seq = sp[i].end_seq;

This is not sufficient, you have to also fix up the type
of the recv_sack_cache[] array entries in struct tcp_sock
to be struct tcp_sack_block_wire.

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

[David Miller]

From: Baruch Even <baruch@ev-en.org>
Date: Fri, 02 Feb 2007 16:41:21 +0200

> 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 <baruch@ev-en.org>

Thanks for fixing up the endianness annotations, applied.