Message ID | 20190117081847.7791-1-yupeng0921@gmail.com |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | [net-next] remove TWKilled counter | expand |
On Thu, Jan 17, 2019 at 3:43 AM yupeng <yupeng0921@gmail.com> wrote: > diff --git a/include/uapi/linux/snmp.h b/include/uapi/linux/snmp.h > index 86dc24a96c90..fb0a17337bf2 100644 > --- a/include/uapi/linux/snmp.h > +++ b/include/uapi/linux/snmp.h > @@ -178,7 +178,6 @@ enum > LINUX_MIB_ARPFILTER, /* ArpFilter */ > LINUX_MIB_TIMEWAITED, /* TimeWaited */ > LINUX_MIB_TIMEWAITRECYCLED, /* TimeWaitRecycled */ > - LINUX_MIB_TIMEWAITKILLED, /* TimeWaitKilled */ > LINUX_MIB_PAWSACTIVEREJECTED, /* PAWSActiveRejected */ > LINUX_MIB_PAWSESTABREJECTED, /* PAWSEstabRejected */ > LINUX_MIB_DELAYEDACKS, /* DelayedACKs */ This is no way to go, it is breaking user-space. You should at least keep this definition even if this counter is unused.
On Thu, Jan 17, 2019 at 10:07 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Thu, Jan 17, 2019 at 3:43 AM yupeng <yupeng0921@gmail.com> wrote: > > diff --git a/include/uapi/linux/snmp.h b/include/uapi/linux/snmp.h > > index 86dc24a96c90..fb0a17337bf2 100644 > > --- a/include/uapi/linux/snmp.h > > +++ b/include/uapi/linux/snmp.h > > @@ -178,7 +178,6 @@ enum > > LINUX_MIB_ARPFILTER, /* ArpFilter */ > > LINUX_MIB_TIMEWAITED, /* TimeWaited */ > > LINUX_MIB_TIMEWAITRECYCLED, /* TimeWaitRecycled */ > > - LINUX_MIB_TIMEWAITKILLED, /* TimeWaitKilled */ > > LINUX_MIB_PAWSACTIVEREJECTED, /* PAWSActiveRejected */ > > LINUX_MIB_PAWSESTABREJECTED, /* PAWSEstabRejected */ > > LINUX_MIB_DELAYEDACKS, /* DelayedACKs */ > > This is no way to go, it is breaking user-space. > > You should at least keep this definition even if this counter is unused. From the snmp.h commit history, I didn't aware that we should keep the user-space compatibility. For example, the commit 713bafea9292 removed LINUX_MIB_TCPFACKREORDER, and the commit 4f693b55c3d2 inserted LINUX_MIB_TCPBACKLOGCOALESCE to the middle of the linux mib definitions enum. I think these commits would break the user-space compatibility too. Did they use any method to avoid breaking the user-space compatibility? Or if I misunderstand anything, please let me know. Thanks a lot.
Hello, I think the snmp.h didn't try to keep user-space compatibility, so we could delete LINUX_MIB_TIMEWAITKILLED counter. Do I misunderstand anything? On Thu, Jan 17, 2019 at 12:26 PM peng yu <yupeng0921@gmail.com> wrote: > > On Thu, Jan 17, 2019 at 10:07 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > > On Thu, Jan 17, 2019 at 3:43 AM yupeng <yupeng0921@gmail.com> wrote: > > > diff --git a/include/uapi/linux/snmp.h b/include/uapi/linux/snmp.h > > > index 86dc24a96c90..fb0a17337bf2 100644 > > > --- a/include/uapi/linux/snmp.h > > > +++ b/include/uapi/linux/snmp.h > > > @@ -178,7 +178,6 @@ enum > > > LINUX_MIB_ARPFILTER, /* ArpFilter */ > > > LINUX_MIB_TIMEWAITED, /* TimeWaited */ > > > LINUX_MIB_TIMEWAITRECYCLED, /* TimeWaitRecycled */ > > > - LINUX_MIB_TIMEWAITKILLED, /* TimeWaitKilled */ > > > LINUX_MIB_PAWSACTIVEREJECTED, /* PAWSActiveRejected */ > > > LINUX_MIB_PAWSESTABREJECTED, /* PAWSEstabRejected */ > > > LINUX_MIB_DELAYEDACKS, /* DelayedACKs */ > > > > This is no way to go, it is breaking user-space. > > > > You should at least keep this definition even if this counter is unused. > > From the snmp.h commit history, I didn't aware that we should keep the > user-space compatibility. For example, the commit 713bafea9292 removed > LINUX_MIB_TCPFACKREORDER, and the commit 4f693b55c3d2 inserted > LINUX_MIB_TCPBACKLOGCOALESCE to the middle of the linux mib > definitions enum. I think these commits would break the user-space > compatibility too. Did they use any method to avoid breaking the > user-space compatibility? Or if I misunderstand anything, please let > me know. > > Thanks a lot.
On Mon, Jan 21, 2019 at 3:14 PM peng yu <yupeng0921@gmail.com> wrote: > > Hello, I think the snmp.h didn't try to keep user-space compatibility, > so we could delete LINUX_MIB_TIMEWAITKILLED counter. Do I > misunderstand anything? I don't know. Maybe user-space doesn't need these definitions at all, as like nstat can figure out the values from /proc/net/netstat. It looks like we either could just kill snmp.h or we have to keep it for backward compatibility.
Hi, David and Cong I don't quite understand where this patch to go. This patch wants to delete the LINUX_MIB_TIMEWAITKILLED counter. Moving snmp.h out of the uapi directory is a difference thing. Do we really need to do it? On Tue, Jan 22, 2019 at 10:58 PM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Mon, Jan 21, 2019 at 3:14 PM peng yu <yupeng0921@gmail.com> wrote: > > > > Hello, I think the snmp.h didn't try to keep user-space compatibility, > > so we could delete LINUX_MIB_TIMEWAITKILLED counter. Do I > > misunderstand anything? > > I don't know. Maybe user-space doesn't need these definitions at all, > as like nstat can figure out the values from /proc/net/netstat. It looks > like we either could just kill snmp.h or we have to keep it for backward > compatibility.
diff --git a/include/net/inet_timewait_sock.h b/include/net/inet_timewait_sock.h index 78775038f011..b042c0baa2ca 100644 --- a/include/net/inet_timewait_sock.h +++ b/include/net/inet_timewait_sock.h @@ -69,8 +69,7 @@ struct inet_timewait_sock { /* these three are in inet_sock */ __be16 tw_sport; /* And these are ours. */ - unsigned int tw_kill : 1, - tw_transparent : 1, + unsigned int tw_transparent : 1, tw_flowlabel : 20, tw_pad : 2, /* 2 bits hole */ tw_tos : 8; diff --git a/include/uapi/linux/snmp.h b/include/uapi/linux/snmp.h index 86dc24a96c90..fb0a17337bf2 100644 --- a/include/uapi/linux/snmp.h +++ b/include/uapi/linux/snmp.h @@ -178,7 +178,6 @@ enum LINUX_MIB_ARPFILTER, /* ArpFilter */ LINUX_MIB_TIMEWAITED, /* TimeWaited */ LINUX_MIB_TIMEWAITRECYCLED, /* TimeWaitRecycled */ - LINUX_MIB_TIMEWAITKILLED, /* TimeWaitKilled */ LINUX_MIB_PAWSACTIVEREJECTED, /* PAWSActiveRejected */ LINUX_MIB_PAWSESTABREJECTED, /* PAWSEstabRejected */ LINUX_MIB_DELAYEDACKS, /* DelayedACKs */ diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c index 88c5069b5d20..f0c4fe21909d 100644 --- a/net/ipv4/inet_timewait_sock.c +++ b/net/ipv4/inet_timewait_sock.c @@ -144,10 +144,7 @@ static void tw_timer_handler(struct timer_list *t) { struct inet_timewait_sock *tw = from_timer(tw, t, tw_timer); - if (tw->tw_kill) - __NET_INC_STATS(twsk_net(tw), LINUX_MIB_TIMEWAITKILLED); - else - __NET_INC_STATS(twsk_net(tw), LINUX_MIB_TIMEWAITED); + __NET_INC_STATS(twsk_net(tw), LINUX_MIB_TIMEWAITED); inet_twsk_kill(tw); } @@ -218,32 +215,6 @@ EXPORT_SYMBOL(inet_twsk_deschedule_put); void __inet_twsk_schedule(struct inet_timewait_sock *tw, int timeo, bool rearm) { - /* timeout := RTO * 3.5 - * - * 3.5 = 1+2+0.5 to wait for two retransmits. - * - * RATIONALE: if FIN arrived and we entered TIME-WAIT state, - * our ACK acking that FIN can be lost. If N subsequent retransmitted - * FINs (or previous seqments) are lost (probability of such event - * is p^(N+1), where p is probability to lose single packet and - * time to detect the loss is about RTO*(2^N - 1) with exponential - * backoff). Normal timewait length is calculated so, that we - * waited at least for one retransmitted FIN (maximal RTO is 120sec). - * [ BTW Linux. following BSD, violates this requirement waiting - * only for 60sec, we should wait at least for 240 secs. - * Well, 240 consumes too much of resources 8) - * ] - * This interval is not reduced to catch old duplicate and - * responces to our wandering segments living for two MSLs. - * However, if we use PAWS to detect - * old duplicates, we can reduce the interval to bounds required - * by RTO, rather than MSL. So, if peer understands PAWS, we - * kill tw bucket after 3.5*RTO (it is important that this number - * is greater than TS tick!) and detect old duplicates with help - * of PAWS. - */ - - tw->tw_kill = timeo <= 4*HZ; if (!rearm) { BUG_ON(mod_timer(&tw->tw_timer, jiffies + timeo)); atomic_inc(&tw->tw_dr->tw_count); diff --git a/net/ipv4/proc.c b/net/ipv4/proc.c index c3610b37bb4c..20a29736d07e 100644 --- a/net/ipv4/proc.c +++ b/net/ipv4/proc.c @@ -186,7 +186,6 @@ static const struct snmp_mib snmp4_net_list[] = { SNMP_MIB_ITEM("ArpFilter", LINUX_MIB_ARPFILTER), SNMP_MIB_ITEM("TW", LINUX_MIB_TIMEWAITED), SNMP_MIB_ITEM("TWRecycled", LINUX_MIB_TIMEWAITRECYCLED), - SNMP_MIB_ITEM("TWKilled", LINUX_MIB_TIMEWAITKILLED), SNMP_MIB_ITEM("PAWSActive", LINUX_MIB_PAWSACTIVEREJECTED), SNMP_MIB_ITEM("PAWSEstab", LINUX_MIB_PAWSESTABREJECTED), SNMP_MIB_ITEM("DelayedACKs", LINUX_MIB_DELAYEDACKS),
The most possible way to trigger TWKilled is using tcp_tw_recycle, but tcp_tw_recycle was removed. The TWKilled could still be triggered if below conditions are satisfied: 1 the tw_timer expires in fin_wait_2 status 2 the timeout value of tw_timer is changed to less than 4 seconds (via net.ipv4.tcp_fin_timeout or TCP_LINGER2 option) But the 4 seconds threshold makes no sense in this scenario. So remove this counter. Signed-off-by: yupeng <yupeng0921@gmail.com> --- include/net/inet_timewait_sock.h | 3 +-- include/uapi/linux/snmp.h | 1 - net/ipv4/inet_timewait_sock.c | 31 +------------------------------ net/ipv4/proc.c | 1 - 4 files changed, 2 insertions(+), 34 deletions(-)