Re: TCP_DEFER_ACCEPT is missing counter update

[Eric Dumazet <eric.dumazet@gmail.com>]

Julian Anastasov a écrit :
> 
> 	I tested both patches. It seems the current algorithm to
> convert seconds to retransmissions does not match well the TCP
> SYN-ACK timer and sometimes can convert the seconds to
> retransmissions which are 1 above the expected. For example,
> you set 9 seconds (expecting 2 retrans) but you get 3 retrans,
> visible with TCP_SYNCNT=1.
> 
> 	Also, it is limited to period of 32 retransmissions.
> 
> 	The following patch changes the TCP_DEFER_ACCEPT
> period calculation to match TCP SYN-ACK retransmissions and to
> help those folks who select the seconds with TCP SYN-ACK
> timing in mind. It also allows the retransmission threshold
> to be up to 255.
> 
> Signed-off-by: Julian Anastasov <ja@ssi.bg>
> 
> diff -urp v2.6.31/linux/net/ipv4/tcp.c linux/net/ipv4/tcp.c
> --- v2.6.31/linux/net/ipv4/tcp.c	2009-09-11 10:27:17.000000000 +0300
> +++ linux/net/ipv4/tcp.c	2009-10-17 12:34:38.000000000 +0300
> @@ -2165,13 +2165,20 @@ static int do_tcp_setsockopt(struct sock
>  	case TCP_DEFER_ACCEPT:
>  		icsk->icsk_accept_queue.rskq_defer_accept = 0;
>  		if (val > 0) {
> +			int timeout = TCP_TIMEOUT_INIT / HZ;
> +			int period = timeout;
> +
>  			/* Translate value in seconds to number of
>  			 * retransmits */
> -			while (icsk->icsk_accept_queue.rskq_defer_accept < 32 &&
> -			       val > ((TCP_TIMEOUT_INIT / HZ) <<
> -				       icsk->icsk_accept_queue.rskq_defer_accept))
> +			icsk->icsk_accept_queue.rskq_defer_accept = 1;
> +			while (icsk->icsk_accept_queue.rskq_defer_accept < 255 &&
> +			       val > period) {
>  				icsk->icsk_accept_queue.rskq_defer_accept++;
> -			icsk->icsk_accept_queue.rskq_defer_accept++;
> +				timeout <<= 1;
> +				if (timeout > TCP_RTO_MAX / HZ)
> +					timeout = TCP_RTO_MAX / HZ;
> +				period += timeout;
> +			}
>  		}
>  		break;
>  
> 
> 	FYI, the old algorithm selects the following retransmissions
> for the configured seconds:
> 
> defer_accept=1 retrans for 1-3 secs
> defer_accept=2 retrans for 4-6 secs
> defer_accept=3 retrans for 7-12 secs
> defer_accept=4 retrans for 13-24 secs
> defer_accept=5 retrans for 25-48 secs
> defer_accept=6 retrans for 49-96 secs
> defer_accept=7 retrans for 97-192 secs
> defer_accept=8 retrans for 193-384 secs
> 
> 	While the new algorithm is as follows:
> 
> defer_accept=1 retrans for 1-3 secs
> defer_accept=2 retrans for 4-9 secs
> defer_accept=3 retrans for 10-21 secs
> defer_accept=4 retrans for 22-45 secs
> defer_accept=5 retrans for 46-93 secs
> defer_accept=6 retrans for 94-189 secs
> defer_accept=7 retrans for 190-309 secs
> defer_accept=8 retrans for 310-429 secs
> 
> 	Comments? Next step is to post the 3 patches separately
> for final review and applying.
> 

I really like this, but please define helper functions out of do_tcp_setsockopt()

/* Translate value in seconds to number of SYN-ACK retransmits */
static u8 secs_to_retrans(int seconds)
{
	u8 res = 0;

	if (seconds > 0) {
		int timeout = TCP_TIMEOUT_INIT / HZ;
		int period = timeout;

		res = 1;
		while (res < 255 && seconds > period) {
			res++;
			timeout <<= 1;
			if (timeout > TCP_RTO_MAX / HZ)
				timeout = TCP_RTO_MAX / HZ;
			period += timeout;
		}
	}
	return res;
}

You also need the reverse function for getsockopt()...