diff mbox

TCP_DEFER_ACCEPT is missing counter update

Message ID Pine.LNX.4.58.0910171417260.7818@u.domain.uli
State Superseded, archived
Delegated to: David Miller
Headers show

Commit Message

Julian Anastasov Oct. 17, 2009, 11:48 a.m. UTC 
Hello,

On Fri, 16 Oct 2009, Eric Dumazet wrote:

> I wonder if tcp_diag should be extented a bit to reflect fact that the ACK was received from client
> (ie forward the inet_rsk(req)->acked information to idiag_rqueue)

	It is a good idea.

> diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
> index cb73fde..c172bd4 100644
> --- a/net/ipv4/inet_diag.c
> +++ b/net/ipv4/inet_diag.c
> @@ -589,7 +589,7 @@ static int inet_diag_fill_req(struct sk_buff *skb, struct sock *sk,
>  	r->id.idiag_src[0] = ireq->loc_addr;
>  	r->id.idiag_dst[0] = ireq->rmt_addr;
>  	r->idiag_expires = jiffies_to_msecs(tmo);
> - 	r->idiag_rqueue = 0;
> +	r->idiag_rqueue = ireq->acked;
>  	r->idiag_wqueue = 0;
>  	r->idiag_uid = sock_i_uid(sk);
>  	r->idiag_inode = 0;

	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>


	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.

Regards

--
Julian Anastasov <ja@ssi.bg>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Eric Dumazet Oct. 17, 2009, 12:07 p.m. UTC | #1 
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()...
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;