[PATCH] reno sacked_out count fix

[PATCH] reno sacked_out count fix

[Angelo P. Castellani]

[-- Attachment #1: Type: text/plain, Size: 611 bytes --]

Using NewReno, if a sk_buff is timed out and is accounted as lost_out,
it should also be removed from the sacked_out.

This is necessary because recovery using NewReno fast retransmit could
take up to a lot RTTs and the sk_buff RTO can expire without actually
being really lost.

left_out = sacked_out + lost_out
in_flight = packets_out - left_out + retrans_out

Using NewReno without this patch, on very large network losses,
left_out becames bigger than packets_out + retrans_out (!!).

For this reason unsigned integer in_flight overflows to 2^32 - something.

Regards,
Angelo P. Castellani

[-- Attachment #2: reno-fix.diff --]
[-- Type: text/x-patch, Size: 548 bytes --]

diff -urd ../linux-2.6.16-orig/net/ipv4/tcp_input.c ./net/ipv4/tcp_input.c
--- ../linux-2.6.16-orig/net/ipv4/tcp_input.c	2006-05-15 15:42:39.000000000 +0200
+++ ./net/ipv4/tcp_input.c	2006-05-16 11:18:21.000000000 +0200
@@ -1676,6 +1676,8 @@
 			if (!(TCP_SKB_CB(skb)->sacked&TCPCB_TAGBITS)) {
 				TCP_SKB_CB(skb)->sacked |= TCPCB_LOST;
 				tp->lost_out += tcp_skb_pcount(skb);
+				if (IsReno(tp)) 
+					tcp_remove_reno_sacks(sk, tp, tcp_skb_pcount(skb) + 1);
 
 				/* clear xmit_retrans hint */
 				if (tp->retransmit_skb_hint &&

Re: [PATCH] reno sacked_out count fix

[David S. Miller]

From: "Angelo P. Castellani" <angelo.castellani+lkml@gmail.com>
Date: Tue, 16 May 2006 11:24:00 +0200

> Using NewReno, if a sk_buff is timed out and is accounted as lost_out,
> it should also be removed from the sacked_out.
> 
> This is necessary because recovery using NewReno fast retransmit could
> take up to a lot RTTs and the sk_buff RTO can expire without actually
> being really lost.
> 
> left_out = sacked_out + lost_out
> in_flight = packets_out - left_out + retrans_out
> 
> Using NewReno without this patch, on very large network losses,
> left_out becames bigger than packets_out + retrans_out (!!).
> 
> For this reason unsigned integer in_flight overflows to 2^32 - something.

Thanks for your analysis and fix, I will apply it.

This pseudo-SACK for Reno scheme could probably use some changes.  For
example, one idea Herbert Xu had was to maintain the pseudo-SACKs in
the actual TAG bits of the TCP_SKB_CB() as if we saw real SACKs.  But
I am concerned that since we have to be careful not to trust those
pseudo-SACK bits too much, it could actually make the code more
complicated not less complicated.