| Message ID | 1360673668.10638.14.camel@ubuntu-vm-makita |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, 2013-02-12 at 21:54 +0900, Toshiaki Makita wrote: > When tcp_fin_timeout is greater than 60, /proc/net/tcp shows > FIN_WAIT2 keepalive timer as expires in (tcp_fin_timeout - 60) > sec. This is confusing because the timer is not for keepalive > and the socket needs another timewait timer to disappear. > > Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> > --- > net/ipv4/tcp_ipv4.c | 23 +++++++++++++++-------- > net/ipv6/tcp_ipv6.c | 23 +++++++++++++++-------- > 2 files changed, 30 insertions(+), 16 deletions(-) > > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c > index 629937d..32bde0e 100644 > --- a/net/ipv4/tcp_ipv4.c > +++ b/net/ipv4/tcp_ipv4.c > @@ -2656,17 +2656,24 @@ static void get_tcp4_sock(struct sock *sk, struct seq_file *f, int i, int *len) > int rx_queue; > > if (icsk->icsk_pending == ICSK_TIME_RETRANS) { > - timer_active = 1; > - timer_expires = icsk->icsk_timeout; > + timer_active = 1; > + timer_expires = icsk->icsk_timeout; > } else if (icsk->icsk_pending == ICSK_TIME_PROBE0) { > - timer_active = 4; > - timer_expires = icsk->icsk_timeout; > + timer_active = 4; > + timer_expires = icsk->icsk_timeout; > } else if (timer_pending(&sk->sk_timer)) { > - timer_active = 2; > - timer_expires = sk->sk_timer.expires; > + if (sk->sk_state == TCP_FIN_WAIT2 && sock_flag(sk, SOCK_DEAD)) { > + timer_active = 3; > + timer_expires = sk->sk_timer.expires + > + (tp->linger2 >= 0 ? > + TCP_TIMEWAIT_LEN : 0); > + } else { > + timer_active = 2; > + timer_expires = sk->sk_timer.expires; > + } > } else { > - timer_active = 0; > - timer_expires = jiffies; > + timer_active = 0; > + timer_expires = jiffies; > } > I find this patch confusing : 1) Please don't change the indentation for a bug fix 2) You add a new 'active=3' field, that some user space reading /proc/net/tcp wont expect. So the changelog is not matching the changes. -- 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
On Tue, 2013-02-12 at 06:57 -0800, Eric Dumazet wrote: > I find this patch confusing : > > 1) Please don't change the indentation for a bug fix > > 2) You add a new 'active=3' field, that some user space > reading /proc/net/tcp wont expect. > > So the changelog is not matching the changes. Also, net/ipv4/inet_diag.c was not changed -- 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
On Tue, 2013-02-12 at 07:13 -0800, Eric Dumazet wrote: > On Tue, 2013-02-12 at 06:57 -0800, Eric Dumazet wrote: > > > I find this patch confusing : > > > > 1) Please don't change the indentation for a bug fix OK, I will separate changes of indentation to another patch. > > > > 2) You add a new 'active=3' field, that some user space > > reading /proc/net/tcp wont expect. I think, at least, it's harmless and beneficial for netstat. Even without this change, a keepalive timer of an orphaned FIN_WAIT2 socket will eventually turn to a timewait timer, and by default, or when tcp_fin_timeout is 60, there will be only timewait timers for orphaned FIN_WAIT2. If the socket is not closed and SO_KEEPALIVE is set, a keepalive timer of FIN_WAIT2 will also be shown in /proc/net/tcp, whose state isn't equal to above one. I don't know any application that will be damaged with this change, and I think the possibility that this change affects a bad influence to userspace is low. Please advise. > > > > So the changelog is not matching the changes. > > Also, net/ipv4/inet_diag.c was not changed > thank you for pointing out my mistake. I will correct it, too. Toshiaki Makita -- 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 --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c index 629937d..32bde0e 100644 --- a/net/ipv4/tcp_ipv4.c +++ b/net/ipv4/tcp_ipv4.c @@ -2656,17 +2656,24 @@ static void get_tcp4_sock(struct sock *sk, struct seq_file *f, int i, int *len) int rx_queue; if (icsk->icsk_pending == ICSK_TIME_RETRANS) { - timer_active = 1; - timer_expires = icsk->icsk_timeout; + timer_active = 1; + timer_expires = icsk->icsk_timeout; } else if (icsk->icsk_pending == ICSK_TIME_PROBE0) { - timer_active = 4; - timer_expires = icsk->icsk_timeout; + timer_active = 4; + timer_expires = icsk->icsk_timeout; } else if (timer_pending(&sk->sk_timer)) { - timer_active = 2; - timer_expires = sk->sk_timer.expires; + if (sk->sk_state == TCP_FIN_WAIT2 && sock_flag(sk, SOCK_DEAD)) { + timer_active = 3; + timer_expires = sk->sk_timer.expires + + (tp->linger2 >= 0 ? + TCP_TIMEWAIT_LEN : 0); + } else { + timer_active = 2; + timer_expires = sk->sk_timer.expires; + } } else { - timer_active = 0; - timer_expires = jiffies; + timer_active = 0; + timer_expires = jiffies; } if (sk->sk_state == TCP_LISTEN) diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c index 93825dd..3af4f9d 100644 --- a/net/ipv6/tcp_ipv6.c +++ b/net/ipv6/tcp_ipv6.c @@ -1795,17 +1795,24 @@ static void get_tcp6_sock(struct seq_file *seq, struct sock *sp, int i) srcp = ntohs(inet->inet_sport); if (icsk->icsk_pending == ICSK_TIME_RETRANS) { - timer_active = 1; - timer_expires = icsk->icsk_timeout; + timer_active = 1; + timer_expires = icsk->icsk_timeout; } else if (icsk->icsk_pending == ICSK_TIME_PROBE0) { - timer_active = 4; - timer_expires = icsk->icsk_timeout; + timer_active = 4; + timer_expires = icsk->icsk_timeout; } else if (timer_pending(&sp->sk_timer)) { - timer_active = 2; - timer_expires = sp->sk_timer.expires; + if (sp->sk_state == TCP_FIN_WAIT2 && sock_flag(sp, SOCK_DEAD)) { + timer_active = 3; + timer_expires = sp->sk_timer.expires + + (tp->linger2 >= 0 ? + TCP_TIMEWAIT_LEN : 0); + } else { + timer_active = 2; + timer_expires = sp->sk_timer.expires; + } } else { - timer_active = 0; - timer_expires = jiffies; + timer_active = 0; + timer_expires = jiffies; } seq_printf(seq,
When tcp_fin_timeout is greater than 60, /proc/net/tcp shows FIN_WAIT2 keepalive timer as expires in (tcp_fin_timeout - 60) sec. This is confusing because the timer is not for keepalive and the socket needs another timewait timer to disappear. Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> --- net/ipv4/tcp_ipv4.c | 23 +++++++++++++++-------- net/ipv6/tcp_ipv6.c | 23 +++++++++++++++-------- 2 files changed, 30 insertions(+), 16 deletions(-)