* [PATCH] tcp: account SYN-ACK timeouts & retransmissions @ 2010-01-06 20:10 Octavian Purdila 2010-01-08 1:25 ` David Miller 0 siblings, 1 reply; 9+ messages in thread From: Octavian Purdila @ 2010-01-06 20:10 UTC (permalink / raw) To: David Miller; +Cc: netdev, Octavian Purdila, Maksim Pyatkovskiy Signed-off-by: Maksim Pyatkovskiy <mpyatkovskiy@ixiacom.com> Signed-off-by: Octavian Purdila <opurdila@ixiacom.com> --- net/ipv4/inet_connection_sock.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c index ee16475..6c20043 100644 --- a/net/ipv4/inet_connection_sock.c +++ b/net/ipv4/inet_connection_sock.c @@ -23,6 +23,7 @@ #include <net/route.h> #include <net/tcp_states.h> #include <net/xfrm.h> +#include <net/tcp.h> #ifdef INET_CSK_DEBUG const char inet_csk_timer_bug_msg[] = "inet_csk BUG: unknown timer value\n"; @@ -525,7 +526,9 @@ void inet_csk_reqsk_queue_prune(struct sock *parent, while ((req = *reqp) != NULL) { if (time_after_eq(now, req->expires)) { int expire = 0, resend = 0; + struct net *net = sock_net(parent); + NET_INC_STATS_BH(net, LINUX_MIB_TCPTIMEOUTS); syn_ack_recalc(req, thresh, max_retries, queue->rskq_defer_accept, &expire, &resend); @@ -535,6 +538,7 @@ void inet_csk_reqsk_queue_prune(struct sock *parent, inet_rsk(req)->acked)) { unsigned long timeo; + TCP_INC_STATS_BH(net, TCP_MIB_RETRANSSEGS); if (req->retrans++ == 0) lopt->qlen_young--; timeo = min((timeout << req->retrans), max_rto); -- 1.5.6.5 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] tcp: account SYN-ACK timeouts & retransmissions 2010-01-06 20:10 [PATCH] tcp: account SYN-ACK timeouts & retransmissions Octavian Purdila @ 2010-01-08 1:25 ` David Miller 2010-01-11 22:16 ` Octavian Purdila 0 siblings, 1 reply; 9+ messages in thread From: David Miller @ 2010-01-08 1:25 UTC (permalink / raw) To: opurdila; +Cc: netdev, mpyatkovskiy From: Octavian Purdila <opurdila@ixiacom.com> Date: Wed, 6 Jan 2010 22:10:58 +0200 > Signed-off-by: Maksim Pyatkovskiy <mpyatkovskiy@ixiacom.com> > Signed-off-by: Octavian Purdila <opurdila@ixiacom.com> RETRANSSEGS is meant to count data segments retransmits, not pure control frames. Similarly for TCPTIMEOUTS, it's mean for data retransmit timeouts. If you overload these statistics with other similar events, they become less meaningful. Finally, bumping TCP specific statistics from the generic INET connection oriented socket code is a huge no-no. That code is written to be protocol agnostic. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] tcp: account SYN-ACK timeouts & retransmissions 2010-01-08 1:25 ` David Miller @ 2010-01-11 22:16 ` Octavian Purdila 2010-01-12 0:15 ` David Miller 0 siblings, 1 reply; 9+ messages in thread From: Octavian Purdila @ 2010-01-11 22:16 UTC (permalink / raw) To: David Miller; +Cc: netdev, mpyatkovskiy On Friday 08 January 2010 03:25:07 you wrote: > From: Octavian Purdila <opurdila@ixiacom.com> > Date: Wed, 6 Jan 2010 22:10:58 +0200 > > > Signed-off-by: Maksim Pyatkovskiy <mpyatkovskiy@ixiacom.com> > > Signed-off-by: Octavian Purdila <opurdila@ixiacom.com> > > RETRANSSEGS is meant to count data segments retransmits, not pure > control frames. > > Similarly for TCPTIMEOUTS, it's mean for data retransmit timeouts. > > If you overload these statistics with other similar events, they > become less meaningful. > But we do increment RETRANSSEGS & TCPTIMEOUTS for SYNs. This creates an imbalance in client/server stats which makes things harder to diagnose. > Finally, bumping TCP specific statistics from the generic INET > connection oriented socket code is a huge no-no. That code is > written to be protocol agnostic. > Fair point, I'll fix that up in the next version. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] tcp: account SYN-ACK timeouts & retransmissions 2010-01-11 22:16 ` Octavian Purdila @ 2010-01-12 0:15 ` David Miller 2010-01-12 16:55 ` Octavian Purdila 0 siblings, 1 reply; 9+ messages in thread From: David Miller @ 2010-01-12 0:15 UTC (permalink / raw) To: opurdila; +Cc: netdev, mpyatkovskiy From: Octavian Purdila <opurdila@ixiacom.com> Date: Tue, 12 Jan 2010 00:16:33 +0200 > On Friday 08 January 2010 03:25:07 you wrote: >> From: Octavian Purdila <opurdila@ixiacom.com> >> Date: Wed, 6 Jan 2010 22:10:58 +0200 >> >> > Signed-off-by: Maksim Pyatkovskiy <mpyatkovskiy@ixiacom.com> >> > Signed-off-by: Octavian Purdila <opurdila@ixiacom.com> >> >> RETRANSSEGS is meant to count data segments retransmits, not pure >> control frames. >> >> Similarly for TCPTIMEOUTS, it's mean for data retransmit timeouts. >> >> If you overload these statistics with other similar events, they >> become less meaningful. >> > > But we do increment RETRANSSEGS & TCPTIMEOUTS for SYNs. This creates an > imbalance in client/server stats which makes things harder to diagnose. Interesting. Can you do me a favor and look into the code history and see if we used to account both sides? I wonder if we accidently lost the statistic bumps when the generic inet connection socket layer was added. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] tcp: account SYN-ACK timeouts & retransmissions 2010-01-12 0:15 ` David Miller @ 2010-01-12 16:55 ` Octavian Purdila 2010-01-13 21:07 ` Octavian Purdila 0 siblings, 1 reply; 9+ messages in thread From: Octavian Purdila @ 2010-01-12 16:55 UTC (permalink / raw) To: David Miller; +Cc: netdev, mpyatkovskiy On Tuesday 12 January 2010 02:15:37 you wrote: > From: Octavian Purdila <opurdila@ixiacom.com> > Date: Tue, 12 Jan 2010 00:16:33 +0200 > > > On Friday 08 January 2010 03:25:07 you wrote: > >> From: Octavian Purdila <opurdila@ixiacom.com> > >> Date: Wed, 6 Jan 2010 22:10:58 +0200 > >> > >> > Signed-off-by: Maksim Pyatkovskiy <mpyatkovskiy@ixiacom.com> > >> > Signed-off-by: Octavian Purdila <opurdila@ixiacom.com> > >> > >> RETRANSSEGS is meant to count data segments retransmits, not pure > >> control frames. > >> > >> Similarly for TCPTIMEOUTS, it's mean for data retransmit timeouts. > >> > >> If you overload these statistics with other similar events, they > >> become less meaningful. > > > > But we do increment RETRANSSEGS & TCPTIMEOUTS for SYNs. This creates an > > imbalance in client/server stats which makes things harder to diagnose. > > Interesting. > > Can you do me a favor and look into the code history and see > if we used to account both sides? > Sure. > I wonder if we accidently lost the statistic bumps when the > generic inet connection socket layer was added. > I don't think so because we are seeing this issue in 2.6.7 as well. I'll download the CVS history and drill down further. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] tcp: account SYN-ACK timeouts & retransmissions 2010-01-12 16:55 ` Octavian Purdila @ 2010-01-13 21:07 ` Octavian Purdila 2010-01-14 10:03 ` David Miller 0 siblings, 1 reply; 9+ messages in thread From: Octavian Purdila @ 2010-01-13 21:07 UTC (permalink / raw) To: David Miller; +Cc: netdev, mpyatkovskiy On Tuesday 12 January 2010 18:55:31 you wrote: > On Tuesday 12 January 2010 02:15:37 you wrote: > > From: Octavian Purdila <opurdila@ixiacom.com> > > Date: Tue, 12 Jan 2010 00:16:33 +0200 > > > > > On Friday 08 January 2010 03:25:07 you wrote: > > >> From: Octavian Purdila <opurdila@ixiacom.com> > > >> Date: Wed, 6 Jan 2010 22:10:58 +0200 > > >> > > >> > Signed-off-by: Maksim Pyatkovskiy <mpyatkovskiy@ixiacom.com> > > >> > Signed-off-by: Octavian Purdila <opurdila@ixiacom.com> > > >> > > >> RETRANSSEGS is meant to count data segments retransmits, not pure > > >> control frames. > > >> > > >> Similarly for TCPTIMEOUTS, it's mean for data retransmit timeouts. > > >> > > >> If you overload these statistics with other similar events, they > > >> become less meaningful. > > > > > > But we do increment RETRANSSEGS & TCPTIMEOUTS for SYNs. This creates an > > > imbalance in client/server stats which makes things harder to diagnose. > > > > Interesting. > > > > Can you do me a favor and look into the code history and see > > if we used to account both sides? > > Sure. I think we lost the SynAck accounting with this commit from the netdev-vger- cvs tree, where tcp_syn_recv_timer was introduced: commit 2248761e5cfcb8687563b29c77064185e7604947 Author: davem <davem> Date: Sun Nov 10 21:25:00 1996 +0000 Merge to 2.1.8, IPV6 is here. Also fix a stupid bug in the kernel unaligned trap handler where st %g0, [foo] would fail miserably. Before this, both Syns and SynAcks seems to be accounted for. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] tcp: account SYN-ACK timeouts & retransmissions 2010-01-13 21:07 ` Octavian Purdila @ 2010-01-14 10:03 ` David Miller 2010-01-17 19:27 ` Octavian Purdila 0 siblings, 1 reply; 9+ messages in thread From: David Miller @ 2010-01-14 10:03 UTC (permalink / raw) To: opurdila; +Cc: netdev, mpyatkovskiy From: Octavian Purdila <opurdila@ixiacom.com> Date: Wed, 13 Jan 2010 23:07:59 +0200 > I think we lost the SynAck accounting with this commit from the netdev-vger- > cvs tree, where tcp_syn_recv_timer was introduced: > > commit 2248761e5cfcb8687563b29c77064185e7604947 > Author: davem <davem> > Date: Sun Nov 10 21:25:00 1996 +0000 > > Merge to 2.1.8, IPV6 is here. Also fix > a stupid bug in the kernel unaligned trap handler > where st %g0, [foo] would fail miserably. > > > Before this, both Syns and SynAcks seems to be accounted for. I'm convinced. Please spin your patch with the "doing TCP socket specific stuff in generic inet connection socket code" part fixed. Thanks. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] tcp: account SYN-ACK timeouts & retransmissions 2010-01-14 10:03 ` David Miller @ 2010-01-17 19:27 ` Octavian Purdila 2010-01-18 3:09 ` David Miller 0 siblings, 1 reply; 9+ messages in thread From: Octavian Purdila @ 2010-01-17 19:27 UTC (permalink / raw) To: David Miller; +Cc: netdev, mpyatkovskiy On Thursday 14 January 2010 12:03:22 you wrote: > From: Octavian Purdila <opurdila@ixiacom.com> > Date: Wed, 13 Jan 2010 23:07:59 +0200 > > > I think we lost the SynAck accounting with this commit from the > > netdev-vger- cvs tree, where tcp_syn_recv_timer was introduced: > > > > commit 2248761e5cfcb8687563b29c77064185e7604947 > > Author: davem <davem> > > Date: Sun Nov 10 21:25:00 1996 +0000 > > > > Merge to 2.1.8, IPV6 is here. Also fix > > a stupid bug in the kernel unaligned trap handler > > where st %g0, [foo] would fail miserably. > > > > > > Before this, both Syns and SynAcks seems to be accounted for. > > I'm convinced. > > Please spin your patch with the "doing TCP socket specific > stuff in generic inet connection socket code" part fixed. > Hi David, Here it is. I've based this against net-next since it is more intrusive then the original one. I can rebase it against net-2.6 if you think it can make it there. Also, please let me know if you think I should split it up (maybe is worth having syn_ack_timeout in a separate patch, but to me it looks too small). [net-next PATCH v2] tcp: account SYN-ACK timeouts & retransmissions Currently we don't increment SYN-ACK timeouts & retransmissions although we do increment the same stats for SYN. We seem to have lost the SYN-ACK accounting with the introduction of tcp_syn_recv_timer (commit 2248761e in the netdev-vger-cvs tree). This patch fixes this issue. In the process we also rename the v4/v6 syn/ack retransmit functions for clarity. We also add a new request_socket operations (syn_ack_timeout) so we can keep code in inet_connection_sock.c protocol agnostic. Signed-off-by: Octavian Purdila <opurdila@ixiacom.com> --- include/net/request_sock.h | 2 ++ include/net/tcp.h | 2 ++ net/ipv4/inet_connection_sock.c | 2 ++ net/ipv4/tcp_ipv4.c | 18 ++++++++++-------- net/ipv4/tcp_timer.c | 6 ++++++ net/ipv6/tcp_ipv6.c | 12 ++++++++++-- 6 files changed, 32 insertions(+), 10 deletions(-) diff --git a/include/net/request_sock.h b/include/net/request_sock.h index c9b50eb..99e6e19 100644 --- a/include/net/request_sock.h +++ b/include/net/request_sock.h @@ -45,6 +45,8 @@ struct request_sock_ops { void (*send_reset)(struct sock *sk, struct sk_buff *skb); void (*destructor)(struct request_sock *req); + void (*syn_ack_timeout)(struct sock *sk, + struct request_sock *req); }; /* struct request_sock - mini sock to represent a connection request diff --git a/include/net/tcp.h b/include/net/tcp.h index 788c99f..87d164b 100644 --- a/include/net/tcp.h +++ b/include/net/tcp.h @@ -400,6 +400,8 @@ extern int compat_tcp_setsockopt(struct sock *sk, int level, int optname, char __user *optval, unsigned int optlen); extern void tcp_set_keepalive(struct sock *sk, int val); +extern void tcp_syn_ack_timeout(struct sock *sk, + struct request_sock *req); extern int tcp_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg, size_t len, int nonblock, diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c index ee16475..8da6429 100644 --- a/net/ipv4/inet_connection_sock.c +++ b/net/ipv4/inet_connection_sock.c @@ -529,6 +529,8 @@ void inet_csk_reqsk_queue_prune(struct sock *parent, syn_ack_recalc(req, thresh, max_retries, queue->rskq_defer_accept, &expire, &resend); + if (req->rsk_ops->syn_ack_timeout) + req->rsk_ops->syn_ack_timeout(parent, req); if (!expire && (!resend || !req->rsk_ops->rtx_syn_ack(parent, req, NULL) || diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c index 382f667..356f544 100644 --- a/net/ipv4/tcp_ipv4.c +++ b/net/ipv4/tcp_ipv4.c @@ -742,9 +742,9 @@ static void tcp_v4_reqsk_send_ack(struct sock *sk, struct sk_buff *skb, * This still operates on a request_sock only, not on a big * socket. */ -static int __tcp_v4_send_synack(struct sock *sk, struct dst_entry *dst, - struct request_sock *req, - struct request_values *rvp) +static int tcp_v4_send_synack(struct sock *sk, struct dst_entry *dst, + struct request_sock *req, + struct request_values *rvp) { const struct inet_request_sock *ireq = inet_rsk(req); int err = -1; @@ -775,10 +775,11 @@ static int __tcp_v4_send_synack(struct sock *sk, struct dst_entry *dst, return err; } -static int tcp_v4_send_synack(struct sock *sk, struct request_sock *req, +static int tcp_v4_rtx_synack(struct sock *sk, struct request_sock *req, struct request_values *rvp) { - return __tcp_v4_send_synack(sk, NULL, req, rvp); + TCP_INC_STATS_BH(sock_net(sk), TCP_MIB_RETRANSSEGS); + return tcp_v4_send_synack(sk, NULL, req, rvp); } /* @@ -1192,10 +1193,11 @@ static int tcp_v4_inbound_md5_hash(struct sock *sk, struct sk_buff *skb) struct request_sock_ops tcp_request_sock_ops __read_mostly = { .family = PF_INET, .obj_size = sizeof(struct tcp_request_sock), - .rtx_syn_ack = tcp_v4_send_synack, + .rtx_syn_ack = tcp_v4_rtx_synack, .send_ack = tcp_v4_reqsk_send_ack, .destructor = tcp_v4_reqsk_destructor, .send_reset = tcp_v4_send_reset, + .syn_ack_timeout = tcp_syn_ack_timeout, }; #ifdef CONFIG_TCP_MD5SIG @@ -1373,8 +1375,8 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb) } tcp_rsk(req)->snt_isn = isn; - if (__tcp_v4_send_synack(sk, dst, req, - (struct request_values *)&tmp_ext) || + if (tcp_v4_send_synack(sk, dst, req, + (struct request_values *)&tmp_ext) || want_cookie) goto drop_and_free; diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c index 8816a20..de7d1bf 100644 --- a/net/ipv4/tcp_timer.c +++ b/net/ipv4/tcp_timer.c @@ -474,6 +474,12 @@ static void tcp_synack_timer(struct sock *sk) TCP_TIMEOUT_INIT, TCP_RTO_MAX); } +void tcp_syn_ack_timeout(struct sock *sk, struct request_sock *req) +{ + NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_TCPTIMEOUTS); +} +EXPORT_SYMBOL(tcp_syn_ack_timeout); + void tcp_set_keepalive(struct sock *sk, int val) { if ((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)) diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c index 1c832bf..82f2dea 100644 --- a/net/ipv6/tcp_ipv6.c +++ b/net/ipv6/tcp_ipv6.c @@ -520,6 +520,13 @@ done: return err; } +static int tcp_v6_rtx_synack(struct sock *sk, struct request_sock *req, + struct request_values *rvp) +{ + TCP_INC_STATS_BH(sock_net(sk), TCP_MIB_RETRANSSEGS); + return tcp_v6_send_synack(sk, req, rvp); +} + static inline void syn_flood_warning(struct sk_buff *skb) { #ifdef CONFIG_SYN_COOKIES @@ -890,10 +897,11 @@ static int tcp_v6_inbound_md5_hash (struct sock *sk, struct sk_buff *skb) struct request_sock_ops tcp6_request_sock_ops __read_mostly = { .family = AF_INET6, .obj_size = sizeof(struct tcp6_request_sock), - .rtx_syn_ack = tcp_v6_send_synack, + .rtx_syn_ack = tcp_v6_rtx_synack, .send_ack = tcp_v6_reqsk_send_ack, .destructor = tcp_v6_reqsk_destructor, - .send_reset = tcp_v6_send_reset + .send_reset = tcp_v6_send_reset, + .syn_ack_timeout = tcp_syn_ack_timeout, }; #ifdef CONFIG_TCP_MD5SIG -- 1.5.6.5 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] tcp: account SYN-ACK timeouts & retransmissions 2010-01-17 19:27 ` Octavian Purdila @ 2010-01-18 3:09 ` David Miller 0 siblings, 0 replies; 9+ messages in thread From: David Miller @ 2010-01-18 3:09 UTC (permalink / raw) To: opurdila; +Cc: netdev, mpyatkovskiy From: Octavian Purdila <opurdila@ixiacom.com> Date: Sun, 17 Jan 2010 21:27:03 +0200 > [net-next PATCH v2] tcp: account SYN-ACK timeouts & retransmissions > > Currently we don't increment SYN-ACK timeouts & retransmissions > although we do increment the same stats for SYN. We seem to have lost > the SYN-ACK accounting with the introduction of tcp_syn_recv_timer > (commit 2248761e in the netdev-vger-cvs tree). > > This patch fixes this issue. In the process we also rename the v4/v6 > syn/ack retransmit functions for clarity. We also add a new > request_socket operations (syn_ack_timeout) so we can keep code in > inet_connection_sock.c protocol agnostic. > > Signed-off-by: Octavian Purdila <opurdila@ixiacom.com> Applied, thanks Octavian. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2010-01-18 3:09 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-01-06 20:10 [PATCH] tcp: account SYN-ACK timeouts & retransmissions Octavian Purdila 2010-01-08 1:25 ` David Miller 2010-01-11 22:16 ` Octavian Purdila 2010-01-12 0:15 ` David Miller 2010-01-12 16:55 ` Octavian Purdila 2010-01-13 21:07 ` Octavian Purdila 2010-01-14 10:03 ` David Miller 2010-01-17 19:27 ` Octavian Purdila 2010-01-18 3:09 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).