Re: [2.6.20.21 review 12/35] TCP: Fix TCP handling of SACK inbidirectional flows.

[Ilpo Järvinen]

On Sat, 13 Oct 2007, Willy Tarreau wrote:

> It's possible that new SACK blocks that should trigger new LOST
> markings arrive with new data (which previously made is_dupack
> false). In addition, I think this fixes a case where we get
> a cumulative ACK with enough SACK blocks to trigger the fast
> recovery (is_dupack would be false there too).
> 
> I'm not completely pleased with this solution because readability
> of the code is somewhat questionable as 'is_dupack' in SACK case
> is no longer about dupacks only but would mean something like
> 'lost_marker_work_todo' too... But because of Eifel stuff done
> in CA_Recovery, the FLAG_DATA_SACKED check cannot be placed to
> the if statement which seems attractive solution. Nevertheless,
> I didn't like adding another variable just for that either... :-)
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxx>
> Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx>
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxx>
> ---
>  net/ipv4/tcp_input.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> Index: 2.6/net/ipv4/tcp_input.c
> ===================================================================
> --- 2.6.orig/net/ipv4/tcp_input.c
> +++ 2.6/net/ipv4/tcp_input.c
> @@ -1951,7 +1951,10 @@ tcp_fastretrans_alert(struct sock *sk, u
>  {
>  	struct inet_connection_sock *icsk = inet_csk(sk);
>  	struct tcp_sock *tp = tcp_sk(sk);
> -	int is_dupack = (tp->snd_una == prior_snd_una && !(flag&FLAG_NOT_DUP));
> +	int is_dupack = (tp->snd_una == prior_snd_una &&
> +			 (!(flag&FLAG_NOT_DUP) ||
> +			  ((flag&FLAG_DATA_SACKED) &&
> +			   (tp->fackets_out > tp->reordering))));
>  
>  	/* Some technical things:
>  	 * 1. Reno does not count dupacks (sacked_out) automatically. */

FYI,

This ended up being a non complete fix. Day after these two patches 
(11-12) I submitted two other patches to complete this fix series (got 
bitten by release-early-release-often, fixed day-after-submission 
thoughts in those two later patches). For some reason these two keep 
floating around as separate ones from those two later ones.

To make things even more complicated, eb7bdad82e8 (see stable-2.6.22) 
could have been split more logically to do_lost addition and 
FLAG_SND_UNA_ADVANCED parts (but that didn't occur to me back then).

All of them listed here (from stable-2.6.22 since one of them is
reduced from mainline version):

6d742fb6e2b8913457e1282e1be77d6f4e45af00 Fix TCP DSACK cwnd handling
eb7bdad82e8af48e1ed1b650268dc85ca7e9ff39 Handle snd_una in tcp_cwnd_down()
8385cffd22359ad561a173accefeb354bd606ce4 TCP: Fix TCP handling of SACK in 
783366ad4b212cde069c50903494eb6a6b83958c TCP: Fix TCP rate-halving on 


-- 
 i.