[PATCH net 1/1] net: tcp: Don't increase the TCP_MIB_OUTRSTS when fail to transmit RST

[PATCH net 1/1] net: tcp: Don't increase the TCP_MIB_OUTRSTS when fail to transmit RST

[gfree.wind]

From: Gao Feng <fgao@ikuai8.com>

When fail to transmit RST, don't increase TCP_MIB_OUTRSTS in func
tcp_send_active_reset like the case that it only increases
LINUX_MIB_TCPABORTFAILED when fail to alloc skb.

Signed-off-by: Gao Feng <fgao@ikuai8.com>
---
 net/ipv4/tcp_output.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 22548b5..274802f 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -3014,8 +3014,8 @@ void tcp_send_active_reset(struct sock *sk, gfp_t priority)
 	/* Send it off. */
 	if (tcp_transmit_skb(sk, skb, 0, priority))
 		NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPABORTFAILED);
-
-	TCP_INC_STATS(sock_net(sk), TCP_MIB_OUTRSTS);
+	else
+		TCP_INC_STATS(sock_net(sk), TCP_MIB_OUTRSTS);
 }
 
 /* Send a crossed SYN-ACK during socket establishment.
-- 
1.9.1

Re: [PATCH net 1/1] net: tcp: Don't increase the TCP_MIB_OUTRSTS when fail to transmit RST

[Neal Cardwell]

On Thu, Apr 6, 2017 at 9:35 AM,  <gfree.wind@foxmail.com> wrote:
> From: Gao Feng <fgao@ikuai8.com>
>
> When fail to transmit RST, don't increase TCP_MIB_OUTRSTS in func
> tcp_send_active_reset like the case that it only increases
> LINUX_MIB_TCPABORTFAILED when fail to alloc skb.
>
> Signed-off-by: Gao Feng <fgao@ikuai8.com>
> ---

I would be concerned that this is a change in the semantics of
TCP_MIB_OUTRSTS that might break user-space monitoring tools that rely
on the current semantics. Counting attempted RSTs could be an
important signal to monitor, and it could be quite bad if that signal
is lost or hidden because the machine is so overloaded that the
transmission of the RSTs fails.

Also it would seem to muddy the semantics a bit, since both
tcp_v4_send_reset() and tcp_v6_send_response() currently increment
TCP_MIB_OUTRSTS without regard to whether the transmit actually
succeeded or not.

neal

RE: [PATCH net 1/1] net: tcp: Don't increase the TCP_MIB_OUTRSTS when fail to transmit RST

[Gao Feng]

Hi Neal,

> -----Original Message-----
> From: Neal Cardwell [mailto:ncardwell@google.com]
> Sent: Thursday, April 6, 2017 10:01 PM
> To: Gao Feng <gfree.wind@foxmail.com>
> Cc: David Miller <davem@davemloft.net>; Alexey Kuznetsov
> <kuznet@ms2.inr.ac.ru>; James Morris <jmorris@namei.org>; Patrick McHardy
> <kaber@trash.net>; Netdev <netdev@vger.kernel.org>; Gao Feng
> <fgao@ikuai8.com>
> Subject: Re: [PATCH net 1/1] net: tcp: Don't increase the TCP_MIB_OUTRSTS
> when fail to transmit RST
> 
> On Thu, Apr 6, 2017 at 9:35 AM,  <gfree.wind@foxmail.com> wrote:
> > From: Gao Feng <fgao@ikuai8.com>
> >
> > When fail to transmit RST, don't increase TCP_MIB_OUTRSTS in func
> > tcp_send_active_reset like the case that it only increases
> > LINUX_MIB_TCPABORTFAILED when fail to alloc skb.
> >
> > Signed-off-by: Gao Feng <fgao@ikuai8.com>
> > ---
> 
> I would be concerned that this is a change in the semantics of
> TCP_MIB_OUTRSTS that might break user-space monitoring tools that rely on
> the current semantics. Counting attempted RSTs could be an important signal
> to monitor, and it could be quite bad if that signal is lost or hidden because the
> machine is so overloaded that the transmission of the RSTs fails.
> 
> Also it would seem to muddy the semantics a bit, since both
> tcp_v4_send_reset() and tcp_v6_send_response() currently increment
> TCP_MIB_OUTRSTS without regard to whether the transmit actually succeeded
> or not.
> 
> neal
If so, we should increase the TCP_MIB_OUTRSTS too when fail to alloc skb.
When machine is overloaded and mem is exhausted, it may fail to alloc skb.

Regards
Feng

Re: [PATCH net 1/1] net: tcp: Don't increase the TCP_MIB_OUTRSTS when fail to transmit RST

[Neal Cardwell]

On Thu, Apr 6, 2017 at 10:05 AM, Gao Feng <gfree.wind@foxmail.com> wrote:
> If so, we should increase the TCP_MIB_OUTRSTS too when fail to alloc skb.
> When machine is overloaded and mem is exhausted, it may fail to alloc skb.

Moving the increment of TCP_MIB_OUTRSTS to the top of
tcp_send_active_reset() sounds fine to me.

neal

RE: [PATCH net 1/1] net: tcp: Don't increase the TCP_MIB_OUTRSTS when fail to transmit RST

[Gao Feng]

Hi Neal

> -----Original Message-----
> 
> On Thu, Apr 6, 2017 at 10:05 AM, Gao Feng <gfree.wind@foxmail.com> wrote:
> > If so, we should increase the TCP_MIB_OUTRSTS too when fail to alloc skb.
> > When machine is overloaded and mem is exhausted, it may fail to alloc skb.
> 
> Moving the increment of TCP_MIB_OUTRSTS to the top of
> tcp_send_active_reset() sounds fine to me.
> 
> neal
I sent the v2 patch, and didn't change the tcp_v4_send_reset and tcp_v6_send_response.
Because I think they are only used during connecting. The rst count is not as important as 
tcp_send_active_reset.

Regards
Feng



.

Re: [PATCH net 1/1] net: tcp: Don't increase the TCP_MIB_OUTRSTS when fail to transmit RST

[Eric Dumazet]

On Thu, 2017-04-06 at 10:08 -0400, Neal Cardwell wrote:
> On Thu, Apr 6, 2017 at 10:05 AM, Gao Feng <gfree.wind@foxmail.com> wrote:
> > If so, we should increase the TCP_MIB_OUTRSTS too when fail to alloc skb.
> > When machine is overloaded and mem is exhausted, it may fail to alloc skb.
> 
> Moving the increment of TCP_MIB_OUTRSTS to the top of
> tcp_send_active_reset() sounds fine to me.


Yes.

Keep in mind that whatever hard we try to send the packet, something
might drop it later without TCP stack knowing it.

So it is not really useful to test the immediate actions that are under
our control.