[PATCH net-2.6.25 0/2]: TCP minor

[PATCH net-2.6.25 0/2]: TCP minor

[Ilpo Järvinen]

Hi Dave,

Here are two TCP patches. The first one removes not so useful
parameter (my avoid GSO skb fragment RFC patch that I'll send
separately right after I get this out depends it). The second
applies cleanly only after the other fack_count fix patch.

--
 i.

[PATCH 1/2] [TCP]: Push fack_count calculation deeper into functions

[Ilpo Järvinen]

This shouldn't have a significant impact because the call to
tcp_sacktag_one is typically made just once per ACK (this
doesn't hold if there were some ACK losses in between or the
receiver didn't generate all ACKs that it should have, but
those are not very likely events to occur).

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

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 75a16b3..977c68c 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1214,11 +1214,15 @@ static int tcp_match_skb_to_sack(struct sock *sk, struct sk_buff *skb,
 }
 
 static int tcp_sacktag_one(struct sk_buff *skb, struct sock *sk,
-			   int *reord, int dup_sack, int fack_count)
+			   int *reord, int dup_sack)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
 	u8 sacked = TCP_SKB_CB(skb)->sacked;
 	int flag = 0;
+	int fack_count;
+	
+	fack_count = TCP_SKB_CB(skb)->fack_count -
+			TCP_SKB_CB(tcp_write_queue_head(sk))->fack_count;
 
 	/* Account D-SACK for retransmitted packet. */
 	if (dup_sack && (sacked & TCPCB_RETRANS)) {
@@ -1315,13 +1319,9 @@ static struct sk_buff *tcp_sacktag_walk(struct sk_buff *skb, struct sock *sk,
 					int queue)
 {
 	struct sk_buff *next;
-	unsigned int fack_count_base;
-
-	fack_count_base = TCP_SKB_CB(tcp_write_queue_head(sk))->fack_count;
 
 	tcp_for_write_queue_from_safe(skb, next, sk, queue) {
 		int in_sack = 0;
-		unsigned int fack_count;
 
 		if (skb == tcp_send_head(sk))
 			break;
@@ -1334,10 +1334,8 @@ static struct sk_buff *tcp_sacktag_walk(struct sk_buff *skb, struct sock *sk,
 		if (unlikely(in_sack < 0))
 			break;
 
-		if (in_sack) {
-			fack_count = TCP_SKB_CB(skb)->fack_count - fack_count_base;
-			*flag |= tcp_sacktag_one(skb, sk, reord, dup_sack, fack_count);
-		}
+		if (in_sack)
+			*flag |= tcp_sacktag_one(skb, sk, reord, dup_sack);
 	}
 	return skb;
 }
-- 
1.5.0.6

[PATCH 2/2] [TCP]: Include __tcp_reset_fack_counts to non-__ version

[Ilpo Järvinen]

This makes flow more obvious in case of short-circuit and
removes need for prev double pointer & fc recount per queue
switch.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
---
 include/net/tcp.h |   54 ++++++++++++++++++----------------------------------
 1 files changed, 19 insertions(+), 35 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 2d9949e..5e6c433 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1467,38 +1467,6 @@ static inline void __tcp_add_write_queue_head(struct sock *sk, struct sk_buff *s
 	tcp_rb_insert(skb, &tcp_sk(sk)->write_queue_rb);
 }
 
-static inline struct sk_buff *__tcp_reset_fack_counts(struct sock *sk,
-						      struct sk_buff *skb,
-						      struct sk_buff **prev,
-						      int queue)
-{
-	unsigned int fc = 0;
-
-	if (*prev != NULL)
-		fc = TCP_SKB_CB(*prev)->fack_count + tcp_skb_pcount(*prev);
-
-	BUG_ON((*prev != NULL) && !tcp_skb_adjacent(sk, *prev, skb));
-
-	tcp_for_write_queue_from(skb, sk, queue) {
-		if ((*prev != NULL) && !tcp_skb_adjacent(sk, *prev, skb))
-			break;
-
-		/* Terminate whole search? */
-		if (!before(TCP_SKB_CB(skb)->seq, tcp_sk(sk)->snd_nxt) ||
-		    TCP_SKB_CB(skb)->fack_count == fc) {
-			*prev = NULL;
-			return NULL;
-		}
-
-		TCP_SKB_CB(skb)->fack_count = fc;
-		fc += tcp_skb_pcount(skb);
-
-		*prev = skb;
-	}
-
-	return skb;
-}
-
 /* An insert into the middle of the write queue causes the fack
  * counts in subsequent packets to become invalid, fix them up.
  *
@@ -1509,6 +1477,7 @@ static inline void tcp_reset_fack_counts(struct sock *sk, struct sk_buff *inskb)
 	struct sk_buff *prev;
 	struct sk_buff *skb[2] = {NULL, NULL};
 	int queue;
+	unsigned int fc = 0;
 
 	if (!before(TCP_SKB_CB(inskb)->seq, tcp_sk(sk)->snd_nxt))
 		return;
@@ -1531,6 +1500,9 @@ static inline void tcp_reset_fack_counts(struct sock *sk, struct sk_buff *inskb)
 		skb[otherq] = prev->next;
 	}
 
+	if (prev != NULL)
+		fc = TCP_SKB_CB(prev)->fack_count + tcp_skb_pcount(prev);
+
 	while (skb[queue] != (struct sk_buff *)__tcp_list_select(sk, queue)) {
 		/* Lazy find for the other queue */
 		if (skb[queue] == NULL) {
@@ -1540,9 +1512,21 @@ static inline void tcp_reset_fack_counts(struct sock *sk, struct sk_buff *inskb)
 				break;
 		}
 
-		skb[queue] = __tcp_reset_fack_counts(sk, skb[queue], &prev, queue);
-		if (skb[queue] == NULL)
-			break;
+		BUG_ON((prev != NULL) && !tcp_skb_adjacent(sk, prev, skb[queue]));
+
+		tcp_for_write_queue_from(skb[queue], sk, queue) {
+			if ((prev != NULL) && !tcp_skb_adjacent(sk, prev, skb[queue]))
+				break;
+
+			if (!before(TCP_SKB_CB(skb[queue])->seq, tcp_sk(sk)->snd_nxt) ||
+			    TCP_SKB_CB(skb[queue])->fack_count == fc)
+				return;
+
+			TCP_SKB_CB(skb[queue])->fack_count = fc;
+			fc += tcp_skb_pcount(skb[queue]);
+
+			prev = skb[queue];
+		}
 
 		queue ^= TCP_WQ_SACKED;
 	}
-- 
1.5.0.6

Re: [PATCH 1/2] [TCP]: Push fack_count calculation deeper into functions

[David Miller]

From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Tue, 11 Dec 2007 13:50:38 +0200

> This shouldn't have a significant impact because the call to
> tcp_sacktag_one is typically made just once per ACK (this
> doesn't hold if there were some ACK losses in between or the
> receiver didn't generate all ACKs that it should have, but
> those are not very likely events to occur).
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>

Applied.

Note that the LRO code will generate stretch ACKs for
in-order normal ACKs, it is something to keep in mind
about this but seems irrelevant for the code you are
touching.

Re: [PATCH 2/2] [TCP]: Include __tcp_reset_fack_counts to non-__ version

[David Miller]

From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Tue, 11 Dec 2007 13:50:39 +0200

> This makes flow more obvious in case of short-circuit and
> removes need for prev double pointer & fc recount per queue
> switch.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>

Looks good.  References to pointers are always ugly and result
in hard to understand code and subtle bugs.

Applied.

Re: [PATCH 2/2] [TCP]: Include __tcp_reset_fack_counts to non-__ version

[Christoph Hellwig]

On Tue, Dec 11, 2007 at 01:50:39PM +0200, Ilpo J?rvinen wrote:
> +		BUG_ON((prev != NULL) && !tcp_skb_adjacent(sk, prev, skb[queue]));
> +
> +		tcp_for_write_queue_from(skb[queue], sk, queue) {
> +			if ((prev != NULL) && !tcp_skb_adjacent(sk, prev, skb[queue]))
> +				break;
> +
> +			if (!before(TCP_SKB_CB(skb[queue])->seq, tcp_sk(sk)->snd_nxt) ||
> +			    TCP_SKB_CB(skb[queue])->fack_count == fc)
> +				return;

There's quite a few overflows of the normal 80 char limit here.  Because
you're current style is a little on the verbose side that's trivially
fixable, though:

		BUG_ON(prev && !tcp_skb_adjacent(sk, prev, skb[queue]));

		tcp_for_write_queue_from(skb[queue], sk, queue) {
			if (prev && !tcp_skb_adjacent(sk, prev, skb[queue]))
				break;

			if (!before(TCP_SKB_CB(skb[queue])->seq,
					       tcp_sk(sk)->snd_nxt) ||
			    TCP_SKB_CB(skb[queue])->fack_count == fc)
				return;

Re: [PATCH 2/2] [TCP]: Include __tcp_reset_fack_counts to non-__ version

[Ilpo Järvinen]

On Tue, 11 Dec 2007, Christoph Hellwig wrote:

> On Tue, Dec 11, 2007 at 01:50:39PM +0200, Ilpo J?rvinen wrote:
> > +		BUG_ON((prev != NULL) && !tcp_skb_adjacent(sk, prev, skb[queue]));
> > +
> > +		tcp_for_write_queue_from(skb[queue], sk, queue) {
> > +			if ((prev != NULL) && !tcp_skb_adjacent(sk, prev, skb[queue]))
> > +				break;
> > +
> > +			if (!before(TCP_SKB_CB(skb[queue])->seq, tcp_sk(sk)->snd_nxt) ||
> > +			    TCP_SKB_CB(skb[queue])->fack_count == fc)
> > +				return;
> 
> There's quite a few overflows of the normal 80 char limit here.  Because
> you're current style is a little on the verbose side that's trivially
> fixable, though:

This part got removed when part of TCP code got removed during net-2.6.25 
rebase...

Thanks anyway for the reminder, I'll try to be more careful during code 
moves in future but I'll probably continue to allow expections in cases 
where the offenders only consist of closing parenthesis, block opening 
brace and termination semicolon (like it was in one of these lines as 
well).

-- 
 i.