netdev - Re: [PATCH/RFC] [v2] TCP: use non-delayed ACK for congestion control RTT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 19 Dec 2007 13:08:35 +0200 (EET)
From:	"Ilpo Järvinen" <ilpo.jarvinen@...sinki.fi>
To:	Gavin McCullagh <Gavin.McCullagh@...m.ie>
cc:	Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH/RFC] [v2] TCP: use non-delayed ACK for congestion	control
 RTT

On Tue, 18 Dec 2007, Gavin McCullagh wrote:

> The last attempt didn't take account of the situation where a timestamp
> wasn't available and tcp_clean_rtx_queue() has to feed both the RTO and the
> congestion avoidance.  This updated patch stores both RTTs, making the
> delayed one available for the RTO and the other (ca_seq_rtt) available for
> congestion control.
> 
> 
> Signed-off-by: Gavin McCullagh <gavin.mccullagh@...m.ie> 
> 
> 
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 889c893..6fb7989 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -2686,13 +2687,15 @@ static int tcp_clean_rtx_queue(struct sock *sk, s32 *seq_rtt_p,
>  				if (sacked & TCPCB_SACKED_RETRANS)
>  					tp->retrans_out -= packets_acked;
>  				flag |= FLAG_RETRANS_DATA_ACKED;
> +				ca_seq_rtt = -1;
>  				seq_rtt = -1;
>  				if ((flag & FLAG_DATA_ACKED) ||
>  				    (packets_acked > 1))
>  					flag |= FLAG_NONHEAD_RETRANS_ACKED;
>  			} else {
> +				ca_seq_rtt = now - scb->when;

Isn't it also much better this way in a case where ACK losses happened,
taking the longest RTT in that case is clearly questionable as it
may over-estimate considerably.

However, another thing to consider is the possibility of this value being 
used in "timeout-like" fashion in ca modules (I haven't read enough ca 
modules code to know if any of them does that), on contrary to 
determinating just rtt or packet's delay in which case this change seems 
appropriate (most modules do the latter). Therefore, if timeout-like 
module exists one should also add TCP_CONG_RTT_STAMP_LONGEST for that 
particular module and keep using seq_rtt for it like previously and use 
ca_seq_rtt only for others.

>  				if (seq_rtt < 0) {
> -					seq_rtt = now - scb->when;
> +					seq_rtt = ca_seq_rtt;
>  					if (fully_acked)
>  						last_ackt = skb->tstamp;
>  				}
> @@ -2709,8 +2712,9 @@ static int tcp_clean_rtx_queue(struct sock *sk, s32 *seq_rtt_p,
>  			    !before(end_seq, tp->snd_up))
>  				tp->urg_mode = 0;
>  		} else {
> +			ca_seq_rtt = now - scb->when;
>  			if (seq_rtt < 0) {
> -				seq_rtt = now - scb->when;
> +				seq_rtt = ca_seq_rtt;
>  				if (fully_acked)
>  					last_ackt = skb->tstamp;
>  			}

This part doesn't exists anymore in development tree. Please base this 
patch (and anything in future) you intend to get included to mainline
onto net-2.6.25 unless there's a very good reason to not do so or 
whatever 2.6.xx is the correct net development tree at that time (if
one exists). Thanks.

-- 
 i.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html