2.6.9 failed assertion in tcp_timer.c

Perhaps the MTU is growing upwards? Does this patch help?

Dave, we need to use the skb's MSS in trim_head as otherwise the counters will be screwed up.

Cheers,

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

===== net/ipv4/tcp_output.c 1.68 vs edited =====

@@ -566,8 +566,6 @@
 
 int tcp_trim_head(struct sock *sk, struct sk_buff *skb, u32 len)
 {
-	struct tcp_opt *tp = tcp_sk(sk);
-
 	if (skb_cloned(skb) &&
 	    pskb_expand_head(skb, 0, 0, GFP_ATOMIC))
 		return -ENOMEM;

@@ -590,7 +588,7 @@
 	/* Any change of skb->len requires recalculation of tso
 	 * factor and mss.
 	 */
-	tcp_set_skb_tso_segs(skb, tp->mss_cache_std);
+	tcp_set_skb_tso_segs(skb, tcp_skb_mss(skb));
 
 	return 0;
 }

Excellent catch. Patch applied, thanks Herbert.

This patch gets it a bit further, but still panics (divide by 0?):

divide error: 0000 [#1]
SMP

CPU:    0
EIP:    0060:[<c029e36c>]    Not tainted VLI
EFLAGS: 00010246   (2.6.9) 

EIP is at tcp_set_skb_tso_segs+0x2c/0x50
eax: 000000f3 ebx: d75f5b00 ecx: 00000000 edx: 00000000
esi: 00000001 edi: d6c22460 ebp: d6c22650 esp: c035decc
ds: 007b es: 007b ss: 0068
Process swapper (pid: 0, threadinfo=c035c000 task=c030eac0)

Stack: d75f5b00 c029e858 d75f5b00 d75f5b00 d6c22460 fffffff5 c029f3dd c02971a6 
       00000002 000005b4 d6c22460 d6c22650 d6c224c4 c035df58 c02a148d 00000002 
       00004100 00000080 f7c52260 c0393bec c038bda0 f710f130 00000010 c035df4c 
Call Trace:
 [<c029e858>] tcp_trim_head+0x88/0xc0
 [<c029f3dd>] tcp_retransmit_skb+0x8d/0x320
 [<c02971a6>] tcp_enter_loss+0x76/0x240
 [<c02a148d>] tcp_retransmit_timer+0xfd/0x430
 [<c02a1889>] tcp_write_timer+0xc9/0x100
 [<c02a17c0>] tcp_write_timer+0x0/0x100
 [<c011ee49>] run_timer_softirq+0xd9/0x170
 [<c011b0f6>] __do_softirq+0xb6/0xd0
 [<c011b13d>] do_softirq+0x2d/0x30
 [<c010de35>] smp_apic_timer_interrupt+0x85/0xf0
 [<c010492e>] apic_timer_interrupt+0x1a/0x20
 [<c0101f40>] default_idle+0x0/0x40
 [<c0101f6c>] default_idle+0x2c/0x40
 [<c0101ffb>] cpu_idle+0x3b/0x50
 [<c035ea66>] start_kernel+0x156/0x180
 [<c035e530>] unknown_bootoption+0x0/0x180

Oops, I forgot that this gets called on non-TSO packets as well. So we don't really know whether it went further or not :)

We should only set the TSO size if the packet was TSO to start with.

Cheers,

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

@@ -588,7 +588,8 @@
 	/* Any change of skb->len requires recalculation of tso
 	 * factor and mss.
 	 */
-	tcp_set_skb_tso_segs(skb, tcp_skb_mss(skb));
+	if (tcp_skb_mss(skb))
+		tcp_set_skb_tso_segs(skb, tcp_skb_mss(skb));
 
 	return 0;
 }

Applied.

Actually, I think we've caught your crash now. If that code path is triggering at all, then it'll trigger with TSO packets too. If we get a truly partial ack on a TSO packet, then tcp_tso_acked will not trim it off. So we will fall through to this last-ditch trim call, which doesn't update packets_out.

There are two solutions to this problem. I've taken the simpler approach for now. We simply trim off the partial bits in tcp_tso_acked and live with the fact that the packet counters may differ from what's on the netwrok by one.

Later on we can "fix" this by remembering where the original TSO packet started from, perhaps in skb->h or somewhere. Dave, is this worth it?

Cheers,

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

===== net/ipv4/tcp_input.c 1.81 vs edited =====

@@ -2369,25 +2369,19 @@
 {
 	struct tcp_opt *tp = tcp_sk(sk);
 	struct tcp_skb_cb *scb = TCP_SKB_CB(skb); 
-	__u32 mss = tcp_skb_mss(skb);
-	__u32 snd_una = tp->snd_una;
-	__u32 orig_seq, seq;
-	__u32 packets_acked = 0;
+	__u32 seq = tp->snd_una;
+	__u32 packets_acked;
 	int acked = 0;
 
 	/* If we get here, the whole TSO packet has not been
 	 * acked.
 	 */
-	BUG_ON(!after(scb->end_seq, snd_una));
+	BUG_ON(!after(scb->end_seq, seq));
 
-	seq = orig_seq = scb->seq;
-	while (!after(seq + mss, snd_una)) {
-		packets_acked++;
-		seq += mss;
-	}
-
-	if (tcp_trim_head(sk, skb, (seq - orig_seq)))
+	packets_acked = tcp_skb_pcount(skb);
+	if (tcp_trim_head(sk, skb, seq - scb->seq))
 		return 0;
+	packets_acked -= tcp_skb_pcount(skb);
 
 	if (packets_acked) {
 		__u8 sacked = scb->sacked;

@@ -588,7 +588,7 @@
 	/* Any change of skb->len requires recalculation of tso
 	 * factor and mss.
 	 */
-	if (tcp_skb_mss(skb))
+	if (tcp_skb_pcount(skb) > 1)
 		tcp_set_skb_tso_segs(skb, tcp_skb_mss(skb));
 
 	return 0;

Looks like your most recent 2 patches did the trick. Uptime on the test box is over 4 hours, whereas before I could count on a panic within about an hour. I'll continue running 2.6.9 + your patches and let you know if this changes...

Thanks,
Phil

Thanks for testing Phil.

Herbert I'll ponder over this patch today.

I'm going to apply this patch for now.

I think at this point we can generalize TSO processing a bit better in this code now.