[PATCH net-next] tcp: Fix bug in ofo queue pruning MIB stats

[PATCH net-next] tcp: Fix bug in ofo queue pruning MIB stats

[Vijay Subramanian]

Linux MIB LINUX_MIB_OFOPRUNED is supposed to count the number of packets
dropped from the out-of-order queue due to socket buffer overrun. Instead
of counting the number of skbs freed, it counts the number of calls make to
__skb_queue_purge() which is not what the user (see f.e. netstat) is expecting.
Fix this by  incrementing the counter correctly.

Signed-off-by: Vijay Subramanian <subramanian.vijay@gmail.com>
---
 net/ipv4/tcp_input.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 2877c3e..0e2c21b 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -4833,7 +4833,8 @@ static int tcp_prune_ofo_queue(struct sock *sk)
 	int res = 0;
 
 	if (!skb_queue_empty(&tp->out_of_order_queue)) {
-		NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_OFOPRUNED);
+		NET_ADD_STATS_BH(sock_net(sk), LINUX_MIB_OFOPRUNED,
+				 tp->out_of_order_queue.qlen);
 		__skb_queue_purge(&tp->out_of_order_queue);
 
 		/* Reset SACK state.  A conforming SACK implementation will
-- 
1.7.0.4

Re: [PATCH net-next] tcp: Fix bug in ofo queue pruning MIB stats

[David Miller]

From: Vijay Subramanian <subramanian.vijay@gmail.com>
Date: Wed, 21 Dec 2011 15:50:01 -0800

> Linux MIB LINUX_MIB_OFOPRUNED is supposed to count the number of packets
> dropped from the out-of-order queue due to socket buffer overrun. Instead
> of counting the number of skbs freed, it counts the number of calls make to
> __skb_queue_purge() which is not what the user (see f.e. netstat) is expecting.
> Fix this by  incrementing the counter correctly.
> 
> Signed-off-by: Vijay Subramanian <subramanian.vijay@gmail.com>

I disagree, this is an event, and the counter is counting how many times
we prune the out of order queue, not how many packets we prune from that
queue.

Re: [PATCH net-next] tcp: Fix bug in ofo queue pruning MIB stats

[Vijay Subramanian]

On 21 December 2011 16:33, David Miller <davem@davemloft.net> wrote:
> From: Vijay Subramanian <subramanian.vijay@gmail.com>
> Date: Wed, 21 Dec 2011 15:50:01 -0800
>
>> Linux MIB LINUX_MIB_OFOPRUNED is supposed to count the number of packets
>> dropped from the out-of-order queue due to socket buffer overrun. Instead
>> of counting the number of skbs freed, it counts the number of calls make to
>> __skb_queue_purge() which is not what the user (see f.e. netstat) is expecting.
>> Fix this by  incrementing the counter correctly.
>>
>> Signed-off-by: Vijay Subramanian <subramanian.vijay@gmail.com>
>
> I disagree, this is an event, and the counter is counting how many times
> we prune the out of order queue, not how many packets we prune from that
> queue.

David,
Thank you for the review.

The reason I felt this was a bug was because of what netstat reports.
Here are 2 sample output lines from netstat.

622 packets pruned from receive queue because of socket buffer overrun
   --> (from LINUX_MIB_RCVPRUNED)
7 packets dropped from out-of-order queue because of socket buffer
overrun   --> (from LINUX_MIB_OFOPRUNED)

netstat is interpreting this incorrectly if the mib counter is
supposed to be tracking events.

Also, LINUX_MIB_OFOPRUNED is named similarly to counters that track
dropped packets (e.g.LINUX_MIB_RCVPRUNED ) than counters that track
events such as a function call (e.g LINUX_MIB_PRUNECALLED). I realize
the naming scheme is not a clinching argument  but taken with what
netstat reports, the intent seems to be to track dropped packets.

If the counter is tracking the right thing, is it worth fixing
netstat? The nstat tool in iproute2 tool does not print any
explanatory text in the output, so there is less chance of confusion
there.  Maybe we just use the newer nstat and forget about this?

Thanks for your time.
Vijay

Re: [PATCH net-next] tcp: Fix bug in ofo queue pruning MIB stats

[David Miller]

From: Vijay Subramanian <subramanian.vijay@gmail.com>
Date: Wed, 21 Dec 2011 17:14:05 -0800

> If the counter is tracking the right thing, is it worth fixing
> netstat?

Probably.

The thing is, I can see both intepretations being useful.

If we count packets, we can't tell that OFO pruning happened 4 times
vs. we pruned 4 packets.

But if you want to know the "extent" to which OFO pruning occurs, then
you want the packet count.

Re: [PATCH net-next] tcp: Fix bug in ofo queue pruning MIB stats

[Rick Jones]

On 12/21/2011 07:15 PM, David Miller wrote:
> From: Vijay Subramanian<subramanian.vijay@gmail.com>
> Date: Wed, 21 Dec 2011 17:14:05 -0800
>
>> If the counter is tracking the right thing, is it worth fixing
>> netstat?
>
> Probably.
>
> The thing is, I can see both intepretations being useful.
>
> If we count packets, we can't tell that OFO pruning happened 4 times
> vs. we pruned 4 packets.
>
> But if you want to know the "extent" to which OFO pruning occurs, then
> you want the packet count.

Starting from the premise that the *primary* goal of netstat is to allow 
end users/admins to know about packet loss events and how they correlate 
with retransmissions, were I to have a "vote" it would be "count the 
packets."

rick jones