From patchwork Sat Apr 18 08:03:10 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Eric Dumazet X-Patchwork-Id: 26154 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@bilbo.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from ozlabs.org (ozlabs.org [203.10.76.45]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "mx.ozlabs.org", Issuer "CA Cert Signing Authority" (verified OK)) by bilbo.ozlabs.org (Postfix) with ESMTPS id 14EC0B7067 for ; Sat, 18 Apr 2009 18:03:26 +1000 (EST) Received: by ozlabs.org (Postfix) id 06AFEDE19E; Sat, 18 Apr 2009 18:03:26 +1000 (EST) Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by ozlabs.org (Postfix) with ESMTP id 9E9D6DE17C for ; Sat, 18 Apr 2009 18:03:25 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752814AbZDRIDV (ORCPT ); Sat, 18 Apr 2009 04:03:21 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752796AbZDRIDU (ORCPT ); Sat, 18 Apr 2009 04:03:20 -0400 Received: from gw1.cosmosbay.com ([212.99.114.194]:52916 "EHLO gw1.cosmosbay.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751690AbZDRIDT convert rfc822-to-8bit (ORCPT ); Sat, 18 Apr 2009 04:03:19 -0400 Received: from [127.0.0.1] (localhost [127.0.0.1]) by gw1.cosmosbay.com (8.13.7/8.13.7) with ESMTP id n3I83BMr010629; Sat, 18 Apr 2009 10:03:11 +0200 Message-ID: <49E9893E.90300@cosmosbay.com> Date: Sat, 18 Apr 2009 10:03:10 +0200 From: Eric Dumazet User-Agent: Thunderbird 2.0.0.21 (Windows/20090302) MIME-Version: 1.0 To: David Miller CC: netdev@vger.kernel.org Subject: Re: [PATCH] loopback: packet drops accounting References: <49E78DE5.10104@hartkopp.net> <20090417.013853.04495551.davem@davemloft.net> <49E84459.1060507@cosmosbay.com> <20090417.015914.09817926.davem@davemloft.net> <49E84B99.1080502@cosmosbay.com> In-Reply-To: <49E84B99.1080502@cosmosbay.com> X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-1.6 (gw1.cosmosbay.com [0.0.0.0]); Sat, 18 Apr 2009 10:03:11 +0200 (CEST) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Eric Dumazet a écrit : > David Miller a écrit : >> From: Eric Dumazet >> Date: Fri, 17 Apr 2009 10:56:57 +0200 >> >>> We can in some situations drop packets in netif_rx() >>> >>> loopback driver does not report these (unlikely) drops to its stats, >>> and incorrectly change packets/bytes counts. >>> >>> After this patch applied, "ifconfig lo" can reports these drops as in : >>> >>> # ifconfig lo >>> lo Link encap:Local Loopback >>> inet addr:127.0.0.1 Mask:255.0.0.0 >>> UP LOOPBACK RUNNING MTU:16436 Metric:1 >>> RX packets:692562900 errors:0 dropped:0 overruns:0 frame:0 >>> TX packets:692562900 errors:3228 dropped:3228 overruns:0 carrier:0 >>> collisions:0 txqueuelen:0 >>> RX bytes:2865674174 (2.6 GiB) TX bytes:2865674174 (2.6 GiB) >>> >>> I chose to reflect those errors only in tx_dropped/tx_errors, and not mirror >>> these errors in rx_dropped/rx_errors. >>> >>> Signed-off-by: Eric Dumazet >> Well, logically the receive is what failed, not the transmit. >> >> I think it's therefore misleading to count it as a TX drop. >> >> Do you feel strongly about this? > Hi David You were right :) Considering that loopbak_xmit() calls eth_type_trans(skb, dev) and this function already starts the RX handling of the packet (skb_pull(...)) So, trying to make loopback_xmit() returns NETDEV_TX_BUSY is wrong too, or too complex, because we would have to undo all changes that might happened during failed xmit. This would be hard to maintain and over-engineering at least, given that these drops are very unlikely. So I resubmit my initial patch, changing the errors to be reported on the rx stats. Thanks [PATCH] loopback: packet drops accounting We can in some situations drop packets in netif_rx() loopback driver does not report these (unlikely) drops to its stats, and incorrectly change packets/bytes counts. After this patch applied, "ifconfig lo" can reports these drops as in : # ifconfig lo lo Link encap:Local Loopback inet addr:127.0.0.1 Mask:255.0.0.0 UP LOOPBACK RUNNING MTU:16436 Metric:1 RX packets:692562900 errors:3228 dropped:3228 overruns:0 frame:0 TX packets:692562900 errors:0 dropped:0 overruns:0 carrier:0 collisions:0 txqueuelen:0 RX bytes:2865674174 (2.6 GiB) TX bytes:2865674174 (2.6 GiB) I initialy chose to reflect those errors only in tx_dropped/tx_errors, but David convinced me that it was really RX errors, as loopback_xmit() really starts a RX process. (calling eth_type_trans() for example, that itself pulls the ethernet header) These errors are accounted in rx_dropped/rx_errors. Signed-off-by: Eric Dumazet --- drivers/net/loopback.c | 21 +++++++++++++++------ 1 files changed, 15 insertions(+), 6 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c index b7d438a..a036296 100644 --- a/drivers/net/loopback.c +++ b/drivers/net/loopback.c @@ -62,6 +62,7 @@ struct pcpu_lstats { unsigned long packets; unsigned long bytes; + unsigned long drops; }; /* @@ -71,18 +72,22 @@ struct pcpu_lstats { static int loopback_xmit(struct sk_buff *skb, struct net_device *dev) { struct pcpu_lstats *pcpu_lstats, *lb_stats; + int len; skb_orphan(skb); - skb->protocol = eth_type_trans(skb,dev); + skb->protocol = eth_type_trans(skb, dev); /* it's OK to use per_cpu_ptr() because BHs are off */ pcpu_lstats = dev->ml_priv; lb_stats = per_cpu_ptr(pcpu_lstats, smp_processor_id()); - lb_stats->bytes += skb->len; - lb_stats->packets++; - netif_rx(skb); + len = skb->len; + if (likely(netif_rx(skb) == NET_RX_SUCCESS)) { + lb_stats->bytes += len; + lb_stats->packets++; + } else + lb_stats->drops++; return 0; } @@ -93,6 +98,7 @@ static struct net_device_stats *loopback_get_stats(struct net_device *dev) struct net_device_stats *stats = &dev->stats; unsigned long bytes = 0; unsigned long packets = 0; + unsigned long drops = 0; int i; pcpu_lstats = dev->ml_priv; @@ -102,11 +108,14 @@ static struct net_device_stats *loopback_get_stats(struct net_device *dev) lb_stats = per_cpu_ptr(pcpu_lstats, i); bytes += lb_stats->bytes; packets += lb_stats->packets; + drops += lb_stats->drops; } stats->rx_packets = packets; stats->tx_packets = packets; - stats->rx_bytes = bytes; - stats->tx_bytes = bytes; + stats->rx_dropped = drops; + stats->rx_errors = drops; + stats->rx_bytes = bytes; + stats->tx_bytes = bytes; return stats; }