[PATCH -next] net: tcp: add mib counters to track zero window transitions

[PATCH -next] net: tcp: add mib counters to track zero window transitions

[Florian Westphal]

Three counters are added:
- one to track when we went from non-zero to zero window
- one to track the reverse
- one counter incremented when we want to announce zero window.

The latter is added because it can show cases where we want to close the
window but can't because we would shrink window.

Suggested-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 Eric, is this what you had in mind?

 I re-ran my 'slow-sender-with-reader-that-does-not-drain-socket'
 scenario and, as expected, only TCPWANTZEROWINDOW increases.

 Thanks,
 Florian

 include/uapi/linux/snmp.h |  3 +++
 net/ipv4/proc.c           |  3 +++
 net/ipv4/tcp_output.c     | 13 ++++++++++++-
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/snmp.h b/include/uapi/linux/snmp.h
index bbaba22..6404eed 100644
--- a/include/uapi/linux/snmp.h
+++ b/include/uapi/linux/snmp.h
@@ -259,6 +259,9 @@ enum
 	LINUX_MIB_TCPSPURIOUS_RTX_HOSTQUEUES, /* TCPSpuriousRtxHostQueues */
 	LINUX_MIB_BUSYPOLLRXPACKETS,		/* BusyPollRxPackets */
 	LINUX_MIB_TCPAUTOCORKING,		/* TCPAutoCorking */
+	LINUX_MIB_TCPFROMZEROWINDOWADV,		/* TCPFromZeroWindowAdv */
+	LINUX_MIB_TCPTOZEROWINDOWADV,		/* TCPToZeroWindowAdv */
+	LINUX_MIB_TCPWANTZEROWINDOW,		/* TCPWantZeroWindow */
 	__LINUX_MIB_MAX
 };
 
diff --git a/net/ipv4/proc.c b/net/ipv4/proc.c
index a6c8a80..542d414 100644
--- a/net/ipv4/proc.c
+++ b/net/ipv4/proc.c
@@ -280,6 +280,9 @@ static const struct snmp_mib snmp4_net_list[] = {
 	SNMP_MIB_ITEM("TCPSpuriousRtxHostQueues", LINUX_MIB_TCPSPURIOUS_RTX_HOSTQUEUES),
 	SNMP_MIB_ITEM("BusyPollRxPackets", LINUX_MIB_BUSYPOLLRXPACKETS),
 	SNMP_MIB_ITEM("TCPAutoCorking", LINUX_MIB_TCPAUTOCORKING),
+	SNMP_MIB_ITEM("TCPFromZeroWindowAdv", LINUX_MIB_TCPFROMZEROWINDOWADV),
+	SNMP_MIB_ITEM("TCPToZeroWindowAdv", LINUX_MIB_TCPTOZEROWINDOWADV),
+	SNMP_MIB_ITEM("TCPWantZeroWindow", LINUX_MIB_TCPWANTZEROWINDOW),
 	SNMP_MIB_SENTINEL
 };
 
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 48414fc..e8d6f14 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -269,6 +269,7 @@ EXPORT_SYMBOL(tcp_select_initial_window);
 static u16 tcp_select_window(struct sock *sk)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
+	u32 old_win = tp->rcv_wnd;
 	u32 cur_win = tcp_receive_window(tp);
 	u32 new_win = __tcp_select_window(sk);
 
@@ -281,6 +282,9 @@ static u16 tcp_select_window(struct sock *sk)
 		 *
 		 * Relax Will Robinson.
 		 */
+		if (new_win == 0)
+			NET_INC_STATS_BH(sock_net(sk),
+					 LINUX_MIB_TCPWANTZEROWINDOW);
 		new_win = ALIGN(cur_win, 1 << tp->rx_opt.rcv_wscale);
 	}
 	tp->rcv_wnd = new_win;
@@ -298,8 +302,15 @@ static u16 tcp_select_window(struct sock *sk)
 	new_win >>= tp->rx_opt.rcv_wscale;
 
 	/* If we advertise zero window, disable fast path. */
-	if (new_win == 0)
+	if (new_win == 0) {
 		tp->pred_flags = 0;
+		if (old_win)
+			NET_INC_STATS_BH(sock_net(sk),
+					 LINUX_MIB_TCPTOZEROWINDOWADV);
+	} else if (old_win == 0) {
+		NET_INC_STATS_BH(sock_net(sk),
+				 LINUX_MIB_TCPFROMZEROWINDOWADV);
+	}
 
 	return new_win;
 }
-- 
1.8.1.5

Re: [PATCH -next] net: tcp: add mib counters to track zero window transitions

[David Miller]

From: Florian Westphal <fw@strlen.de>
Date: Mon, 17 Feb 2014 22:57:48 +0100

> Three counters are added:
> - one to track when we went from non-zero to zero window
> - one to track the reverse
> - one counter incremented when we want to announce zero window.
> 
> The latter is added because it can show cases where we want to close the
> window but can't because we would shrink window.
> 
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  Eric, is this what you had in mind?
> 
>  I re-ran my 'slow-sender-with-reader-that-does-not-drain-socket'
>  scenario and, as expected, only TCPWANTZEROWINDOW increases.

Eric, ping?

Re: [PATCH -next] net: tcp: add mib counters to track zero window transitions

[Eric Dumazet]

On Wed, 2014-02-19 at 13:17 -0500, David Miller wrote:
> From: Florian Westphal <fw@strlen.de>
> Date: Mon, 17 Feb 2014 22:57:48 +0100
> 
> > Three counters are added:
> > - one to track when we went from non-zero to zero window
> > - one to track the reverse
> > - one counter incremented when we want to announce zero window.
> > 
> > The latter is added because it can show cases where we want to close the
> > window but can't because we would shrink window.
> > 
> > Suggested-by: Eric Dumazet <edumazet@google.com>
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> >  Eric, is this what you had in mind?
> > 
> >  I re-ran my 'slow-sender-with-reader-that-does-not-drain-socket'
> >  scenario and, as expected, only TCPWANTZEROWINDOW increases.
> 
> Eric, ping?

Thanks, I missed this patch. Let me think about it, thanks !

Re: [PATCH -next] net: tcp: add mib counters to track zero window transitions

[Hannes Frederic Sowa]

On Wed, Feb 19, 2014 at 10:49:47AM -0800, Eric Dumazet wrote:
> On Wed, 2014-02-19 at 13:17 -0500, David Miller wrote:
> > From: Florian Westphal <fw@strlen.de>
> > Date: Mon, 17 Feb 2014 22:57:48 +0100
> > 
> > > Three counters are added:
> > > - one to track when we went from non-zero to zero window
> > > - one to track the reverse
> > > - one counter incremented when we want to announce zero window.
> > > 
> > > The latter is added because it can show cases where we want to close the
> > > window but can't because we would shrink window.
> > > 
> > > Suggested-by: Eric Dumazet <edumazet@google.com>
> > > Signed-off-by: Florian Westphal <fw@strlen.de>
> > > ---
> > >  Eric, is this what you had in mind?
> > > 
> > >  I re-ran my 'slow-sender-with-reader-that-does-not-drain-socket'
> > >  scenario and, as expected, only TCPWANTZEROWINDOW increases.
> > 
> > Eric, ping?
> 
> Thanks, I missed this patch. Let me think about it, thanks !

We need NET_INC_STATS instead of NET_INC_STATS_BH, no?

Greetings,

  Hannes

Re: [PATCH -next] net: tcp: add mib counters to track zero window transitions

[Hannes Frederic Sowa]

On Wed, Feb 19, 2014 at 07:59:49PM +0100, Hannes Frederic Sowa wrote:
> On Wed, Feb 19, 2014 at 10:49:47AM -0800, Eric Dumazet wrote:
> > On Wed, 2014-02-19 at 13:17 -0500, David Miller wrote:
> > > From: Florian Westphal <fw@strlen.de>
> > > Date: Mon, 17 Feb 2014 22:57:48 +0100
> > > 
> > > > Three counters are added:
> > > > - one to track when we went from non-zero to zero window
> > > > - one to track the reverse
> > > > - one counter incremented when we want to announce zero window.
> > > > 
> > > > The latter is added because it can show cases where we want to close the
> > > > window but can't because we would shrink window.
> > > > 
> > > > Suggested-by: Eric Dumazet <edumazet@google.com>
> > > > Signed-off-by: Florian Westphal <fw@strlen.de>
> > > > ---
> > > >  Eric, is this what you had in mind?
> > > > 
> > > >  I re-ran my 'slow-sender-with-reader-that-does-not-drain-socket'
> > > >  scenario and, as expected, only TCPWANTZEROWINDOW increases.
> > > 
> > > Eric, ping?
> > 
> > Thanks, I missed this patch. Let me think about it, thanks !
> 
> We need NET_INC_STATS instead of NET_INC_STATS_BH, no?

Ok, not strictly needed but just to keep the style, as tcp_select_window can
be called from both, bh and process context.

Greetings,

  Hannes

Re: [PATCH -next] net: tcp: add mib counters to track zero window transitions

[Florian Westphal]

Hannes Frederic Sowa <hannes@stressinduktion.org> wrote:
> On Wed, Feb 19, 2014 at 10:49:47AM -0800, Eric Dumazet wrote:
> > On Wed, 2014-02-19 at 13:17 -0500, David Miller wrote:
> > > From: Florian Westphal <fw@strlen.de>
> > > Date: Mon, 17 Feb 2014 22:57:48 +0100
> > > 
> > > > Three counters are added:
> > > > - one to track when we went from non-zero to zero window
> > > > - one to track the reverse
> > > > - one counter incremented when we want to announce zero window.
> > > > 
> > > > The latter is added because it can show cases where we want to close the
> > > > window but can't because we would shrink window.
> > > > 
> > > > Suggested-by: Eric Dumazet <edumazet@google.com>
> > > > Signed-off-by: Florian Westphal <fw@strlen.de>
> > > > ---
> > > >  Eric, is this what you had in mind?
> > > > 
> > > >  I re-ran my 'slow-sender-with-reader-that-does-not-drain-socket'
> > > >  scenario and, as expected, only TCPWANTZEROWINDOW increases.
> > > 
> > > Eric, ping?
> > 
> > Thanks, I missed this patch. Let me think about it, thanks !
> 
> We need NET_INC_STATS instead of NET_INC_STATS_BH, no?

sure, I can send v2 but I'll wait for (n)ack from Eric first
before considering it.

tcp_transmit_skb (caller of tcp_select_window) also uses _BH
which is why it ended up in tcp_select_window.

Re: [PATCH -next] net: tcp: add mib counters to track zero window transitions

[Eric Dumazet]

On Wed, 2014-02-19 at 20:18 +0100, Florian Westphal wrote:

> sure, I can send v2 but I'll wait for (n)ack from Eric first
> before considering it.
> 
> tcp_transmit_skb (caller of tcp_select_window) also uses _BH
> which is why it ended up in tcp_select_window.

Yes, it seems the NET_INC_STATS_BH() in tcp_transmit_skb() should really
be a NET_INC_STATS(), even if it currently doesn't matter, even on 32bit
arches.

Re: [PATCH -next] net: tcp: add mib counters to track zero window transitions

[Hannes Frederic Sowa]

On Wed, Feb 19, 2014 at 08:18:31PM +0100, Florian Westphal wrote:
> Hannes Frederic Sowa <hannes@stressinduktion.org> wrote:
> > On Wed, Feb 19, 2014 at 10:49:47AM -0800, Eric Dumazet wrote:
> > > On Wed, 2014-02-19 at 13:17 -0500, David Miller wrote:
> > > > From: Florian Westphal <fw@strlen.de>
> > > > Date: Mon, 17 Feb 2014 22:57:48 +0100
> > > > 
> > > > > Three counters are added:
> > > > > - one to track when we went from non-zero to zero window
> > > > > - one to track the reverse
> > > > > - one counter incremented when we want to announce zero window.
> > > > > 
> > > > > The latter is added because it can show cases where we want to close the
> > > > > window but can't because we would shrink window.
> > > > > 
> > > > > Suggested-by: Eric Dumazet <edumazet@google.com>
> > > > > Signed-off-by: Florian Westphal <fw@strlen.de>
> > > > > ---
> > > > >  Eric, is this what you had in mind?
> > > > > 
> > > > >  I re-ran my 'slow-sender-with-reader-that-does-not-drain-socket'
> > > > >  scenario and, as expected, only TCPWANTZEROWINDOW increases.
> > > > 
> > > > Eric, ping?
> > > 
> > > Thanks, I missed this patch. Let me think about it, thanks !
> > 
> > We need NET_INC_STATS instead of NET_INC_STATS_BH, no?
> 
> sure, I can send v2 but I'll wait for (n)ack from Eric first
> before considering it.
> 
> tcp_transmit_skb (caller of tcp_select_window) also uses _BH
> which is why it ended up in tcp_select_window.

NET_STATS only use 32 bit counter and thus need not be protected with a
seqlock on 32 bit platforms. As such, it does not matter, but e.g. the IP
counter are prone to deadlocks if used with wrong postfix because of 64 bit
counter thus protected by seqlock.

A pitty that the _STATS_BH postfixes have the opposite meaning of the bottom
_bh postfixes.

Basically always safe is _STATS and if we are sure we can omit the bh disable
call because we only call the function from bh, we can use _STATS_BH calls.

Greetings,

  Hannnes

Re: [PATCH -next] net: tcp: add mib counters to track zero window transitions

[Hannes Frederic Sowa]

Sorry...

On Wed, Feb 19, 2014 at 08:30:38PM +0100, Hannes Frederic Sowa wrote:
> NET_STATS only use 32 bit counter and thus need not be protected with a
> seqlock on 32 bit platforms. As such, it does not matter, but e.g. the IP
> counter are prone to deadlocks if used with wrong postfix because of 64 bit
> counter thus protected by seqlock.
> 
> A pitty that the _STATS_BH postfixes have the opposite meaning of the bottom

	s/bottom$/locking/

> _bh postfixes.
> 
> Basically always safe is _STATS and if we are sure we can omit the bh disable
> call because we only call the function from bh, we can use _STATS_BH calls.

Should read mails again before sending. ;)

Re: [PATCH -next] net: tcp: add mib counters to track zero window transitions

[Eric Dumazet]

On Wed, 2014-02-19 at 20:30 +0100, Hannes Frederic Sowa wrote:

> NET_STATS only use 32 bit counter and thus need not be protected with a
> seqlock on 32 bit platforms. As such, it does not matter, but e.g. the IP
> counter are prone to deadlocks if used with wrong postfix because of 64 bit
> counter thus protected by seqlock.
> 
> A pitty that the _STATS_BH postfixes have the opposite meaning of the bottom
> _bh postfixes.
> 
> Basically always safe is _STATS and if we are sure we can omit the bh disable
> call because we only call the function from bh, we can use _STATS_BH calls.

Using __this_cpu_inc() is not safe in preemptable contexts.

Sure, x86 doesn't care, but other arches might.

Fortunately LINUX_MIB_TCPSPURIOUS_RTX_HOSTQUEUES can be incremented
only for a retransmit, and they should always happen from BH.

Better avoid the confusion for this ultra rare case, I'll send a patch.

[PATCH] net: tcp: use NET_INC_STATS()

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

While LINUX_MIB_TCPSPURIOUS_RTX_HOSTQUEUES can only be incremented
in tcp_transmit_skb() from softirq (incoming message or timer
activation), it is better to use NET_INC_STATS() instead of
NET_INC_STATS_BH() as tcp_transmit_skb() can be called from process
context.

This will avoid copy/paste confusion when/if we want to add
other SNMP counters in tcp_transmit_skb()

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
Cc: Florian Westphal <fw@strlen.de>
---
 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 09805817627b..d718482fd11c 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -864,8 +864,8 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
 
 		if (unlikely(skb->fclone == SKB_FCLONE_ORIG &&
 			     fclone->fclone == SKB_FCLONE_CLONE))
-			NET_INC_STATS_BH(sock_net(sk),
-					 LINUX_MIB_TCPSPURIOUS_RTX_HOSTQUEUES);
+			NET_INC_STATS(sock_net(sk),
+				      LINUX_MIB_TCPSPURIOUS_RTX_HOSTQUEUES);
 
 		if (unlikely(skb_cloned(skb)))
 			skb = pskb_copy(skb, gfp_mask);

Re: [PATCH] net: tcp: use NET_INC_STATS()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 25 Feb 2014 04:31:03 -0800

> From: Eric Dumazet <edumazet@google.com>
> 
> While LINUX_MIB_TCPSPURIOUS_RTX_HOSTQUEUES can only be incremented
> in tcp_transmit_skb() from softirq (incoming message or timer
> activation), it is better to use NET_INC_STATS() instead of
> NET_INC_STATS_BH() as tcp_transmit_skb() can be called from process
> context.
> 
> This will avoid copy/paste confusion when/if we want to add
> other SNMP counters in tcp_transmit_skb()
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied, thanks Eric.

Re: [PATCH -next] net: tcp: add mib counters to track zero window transitions

[David Miller]

From: Florian Westphal <fw@strlen.de>
Date: Mon, 17 Feb 2014 22:57:48 +0100

> Three counters are added:
> - one to track when we went from non-zero to zero window
> - one to track the reverse
> - one counter incremented when we want to announce zero window.
> 
> The latter is added because it can show cases where we want to close the
> window but can't because we would shrink window.
> 
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  Eric, is this what you had in mind?
> 
>  I re-ran my 'slow-sender-with-reader-that-does-not-drain-socket'
>  scenario and, as expected, only TCPWANTZEROWINDOW increases.

What is happening with this patch?  The discussion just died out.

Re: [PATCH -next] net: tcp: add mib counters to track zero window transitions

[Florian Westphal]

David Miller <davem@davemloft.net> wrote:
> From: Florian Westphal <fw@strlen.de>
> Date: Mon, 17 Feb 2014 22:57:48 +0100
> 
> > Three counters are added:
> > - one to track when we went from non-zero to zero window
> > - one to track the reverse
> > - one counter incremented when we want to announce zero window.
> > 
> > The latter is added because it can show cases where we want to close the
> > window but can't because we would shrink window.
> > 
> > Suggested-by: Eric Dumazet <edumazet@google.com>
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> >  Eric, is this what you had in mind?
> > 
> >  I re-ran my 'slow-sender-with-reader-that-does-not-drain-socket'
> >  scenario and, as expected, only TCPWANTZEROWINDOW increases.
> 
> What is happening with this patch?  The discussion just died out.

I guess Eric is busy with TCP usec timer resolution changes.
You can mark the patch as defered if you want; I can re-send it for
3.16 if needed.

Re: [PATCH -next] net: tcp: add mib counters to track zero window transitions

[Eric Dumazet]

On Tue, 2014-02-25 at 09:23 +0100, Florian Westphal wrote:
> David Miller <davem@davemloft.net> wrote:
 
> > What is happening with this patch?  The discussion just died out.
> 
> I guess Eric is busy with TCP usec timer resolution changes.
> You can mark the patch as defered if you want; I can re-send it for
> 3.16 if needed.

Well, I was waiting you resend it using Hannes feedback ;)

That is using NET_INC_STATS() instead of NET_INC_STATS_BH()

I am sending the cleanup for LINUX_MIB_TCPSPURIOUS_RTX_HOSTQUEUES
immediately.

Re: [PATCH -next] net: tcp: add mib counters to track zero window transitions

[Florian Westphal]

Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2014-02-25 at 09:23 +0100, Florian Westphal wrote:
> > David Miller <davem@davemloft.net> wrote:
>  
> > > What is happening with this patch?  The discussion just died out.
> > 
> > I guess Eric is busy with TCP usec timer resolution changes.
> > You can mark the patch as defered if you want; I can re-send it for
> > 3.16 if needed.
> 
> Well, I was waiting you resend it using Hannes feedback ;)

Heh.  I was waiting for you to tell me wheter you reject the general
intention of the patch. 	Classic deadlock :)

> I am sending the cleanup for LINUX_MIB_TCPSPURIOUS_RTX_HOSTQUEUES
> immediately.

Thanks Eric.  I'll send v2 of the patch in a few minutes.