| From d084e1834f09ab9b876470add92a3870d5636f58 Mon Sep 17 00:00:00 2001 |
| From: Willem de Bruijn <willemb@google.com> |
| Date: Fri, 30 Sep 2011 10:38:28 +0000 |
| Subject: make PACKET_STATISTICS getsockopt report consistently between ring and non-ring |
| |
| |
| From: Willem de Bruijn <willemb@google.com> |
| |
| [ Upstream commit 7091fbd82cd5686444ffe9935ed6a8190101fe9d ] |
| |
| This is a minor change. |
| |
| Up until kernel 2.6.32, getsockopt(fd, SOL_PACKET, PACKET_STATISTICS, |
| ...) would return total and dropped packets since its last invocation. The |
| introduction of socket queue overflow reporting [1] changed drop |
| rate calculation in the normal packet socket path, but not when using a |
| packet ring. As a result, the getsockopt now returns different statistics |
| depending on the reception method used. With a ring, it still returns the |
| count since the last call, as counts are incremented in tpacket_rcv and |
| reset in getsockopt. Without a ring, it returns 0 if no drops occurred |
| since the last getsockopt and the total drops over the lifespan of |
| the socket otherwise. The culprit is this line in packet_rcv, executed |
| on a drop: |
| |
| drop_n_acct: |
| po->stats.tp_drops = atomic_inc_return(&sk->sk_drops); |
| |
| As it shows, the new drop number it taken from the socket drop counter, |
| which is not reset at getsockopt. I put together a small example |
| that demonstrates the issue [2]. It runs for 10 seconds and overflows |
| the queue/ring on every odd second. The reported drop rates are: |
| ring: 16, 0, 16, 0, 16, ... |
| non-ring: 0, 15, 0, 30, 0, 46, 0, 60, 0 , 74. |
| |
| Note how the even ring counts monotonically increase. Because the |
| getsockopt adds tp_drops to tp_packets, total counts are similarly |
| reported cumulatively. Long story short, reinstating the original code, as |
| the below patch does, fixes the issue at the cost of additional per-packet |
| cycles. Another solution that does not introduce per-packet overhead |
| is be to keep the current data path, record the value of sk_drops at |
| getsockopt() at call N in a new field in struct packetsock and subtract |
| that when reporting at call N+1. I'll be happy to code that, instead, |
| it's just more messy. |
| |
| [1] http://patchwork.ozlabs.org/patch/35665/ |
| [2] http://kernel.googlecode.com/files/test-packetsock-getstatistics.c |
| |
| Signed-off-by: Willem de Bruijn <willemb@google.com> |
| Signed-off-by: David S. Miller <davem@davemloft.net> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de> |
| --- |
| net/packet/af_packet.c | 5 ++++- |
| 1 file changed, 4 insertions(+), 1 deletion(-) |
| |
| --- a/net/packet/af_packet.c |
| +++ b/net/packet/af_packet.c |
| @@ -654,7 +654,10 @@ static int packet_rcv(struct sk_buff *sk |
| return 0; |
| |
| drop_n_acct: |
| - po->stats.tp_drops = atomic_inc_return(&sk->sk_drops); |
| + spin_lock(&sk->sk_receive_queue.lock); |
| + po->stats.tp_drops++; |
| + atomic_inc(&sk->sk_drops); |
| + spin_unlock(&sk->sk_receive_queue.lock); |
| |
| drop_n_restore: |
| if (skb_head != skb->data && skb_shared(skb)) { |