From patchwork Mon Apr 27 02:00:38 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?b?5aec56uL5Lic?= X-Patchwork-Id: 1277287 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.138; helo=whitealder.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=openvswitch.org Received: from whitealder.osuosl.org (smtp1.osuosl.org [140.211.166.138]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 499Sfz4fBqz9sSK for ; Mon, 27 Apr 2020 12:00:57 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id 04F5C86117; Mon, 27 Apr 2020 02:00:54 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from whitealder.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 0jPw1g3lQY8k; Mon, 27 Apr 2020 02:00:52 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by whitealder.osuosl.org (Postfix) with ESMTP id 3A2D986094; Mon, 27 Apr 2020 02:00:52 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 1E68CC089E; Mon, 27 Apr 2020 02:00:52 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137]) by lists.linuxfoundation.org (Postfix) with ESMTP id 3C1A3C0172 for ; Mon, 27 Apr 2020 02:00:50 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id 31D6D856E8 for ; Mon, 27 Apr 2020 02:00:50 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from fraxinus.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id KMwoi3yqsVRk for ; Mon, 27 Apr 2020 02:00:47 +0000 (UTC) X-Greylist: from auto-whitelisted by SQLgrey-1.7.6 Received: from smtp4.jd.com (smtp4.jd.com [59.151.64.77]) by fraxinus.osuosl.org (Postfix) with ESMTPS id 5EF63855FE for ; Mon, 27 Apr 2020 02:00:46 +0000 (UTC) Received: from JDCLOUDMAIL58.360buyAD.local (172.31.68.160) by JDCLOUDMAIL59.360buyAD.local (172.31.68.161) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1847.3; Mon, 27 Apr 2020 10:00:38 +0800 Received: from JDCLOUDMAIL58.360buyAD.local ([fe80::49f4:99d7:3aa7:96c]) by JDCLOUDMAIL58.360buyAD.local ([fe80::49f4:99d7:3aa7:96c%3]) with mapi id 15.01.1847.007; Mon, 27 Apr 2020 10:00:38 +0800 To: William Tu Thread-Topic: [ovs-dev] [PATCH] netdev-linux: remove sum of vport stats and kernel netdev stats Thread-Index: AdYZMEPZE011FfHYSZCebEMSv/OxAwBmsv0AAFqnUGA= Date: Mon, 27 Apr 2020 02:00:38 +0000 Message-ID: <172e1abc63cc4481ab1ee0ae82051448@jd.com> References: <43374bda3e77484e8f3158e7844624ea@jd.com> <20200425143050.GC125003@gmail.com> In-Reply-To: <20200425143050.GC125003@gmail.com> Accept-Language: zh-CN, en-US Content-Language: zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [172.31.12.238] MIME-Version: 1.0 Cc: "dev@openvswitch.org" Subject: [ovs-dev] =?utf-8?q?=E7=AD=94=E5=A4=8D=3A__=5BPATCH=5D_netdev-linux?= =?utf-8?q?=3A_remove_sum_of_vport_stats_and_kernel_netdev_stats?= X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: =?utf-8?b?5aec56uL5Lic?= via dev From: =?utf-8?b?5aec56uL5Lic?= Reply-To: =?utf-8?b?5aec56uL5Lic?= Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" Hi William, Thanks for your comments. Agree with you, those counters such as collisions and multicast should be kept as current implementation, since they are don't provided by vport stats. I also suggest to remove rx/tx bytes and packets, rx/tx error and drops as well, only count physical or hardware stats in, because it is confusing when checking with netdev_stats_from_ovs_vport_stats, if netdev_stats_from_ovs_vport_stats has provided them, why they are copied again. What do you think about change as below? > + > ovs_mutex_unlock(&netdev->mutex); > > return error; > -- > 1.8.3.1 > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index ff045cb..b851236 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -2208,18 +2208,6 @@ netdev_linux_get_stats(const struct netdev *netdev_, /* stats not available from OVS then use netdev stats. */ *stats = dev_stats; } else { - /* Use kernel netdev's packet and byte counts since vport's counters - * do not reflect packet counts on the wire when GSO, TSO or GRO are - * enabled. */ - stats->rx_packets = dev_stats.rx_packets; - stats->rx_bytes = dev_stats.rx_bytes; - stats->tx_packets = dev_stats.tx_packets; - stats->tx_bytes = dev_stats.tx_bytes; - - stats->rx_errors += dev_stats.rx_errors; - stats->tx_errors += dev_stats.tx_errors; - stats->rx_dropped += dev_stats.rx_dropped; - stats->tx_dropped += dev_stats.tx_dropped; stats->multicast += dev_stats.multicast; stats->collisions += dev_stats.collisions; stats->rx_length_errors += dev_stats.rx_length_errors; BR, Lidong -----邮件原件----- 发件人: William Tu 发送时间: 2020年4月25日 22:31 收件人: 姜立东 抄送: dev@openvswitch.org 主题: Re: [ovs-dev] [PATCH] netdev-linux: remove sum of vport stats and kernel netdev stats On Thu, Apr 23, 2020 at 05:35:14AM +0000, 姜立东 via dev wrote: > From df9ff3b67f11e467928ca0873031d81b87f3d0c5 Mon Sep 17 00:00:00 2001 > From: Jiang Lidong > Date: Thu, 23 Apr 2020 11:07:28 +0800 > Subject: [PATCH] netdev-linux: remove sum of vport stats and kernel netdev stats > When using kernel veth as OVS interface, doubled drop counter > value is shown when veth drops packets due to traffic overrun. > > In netdev_linux_get_stats, it reads both vport stats and kernel > netdev stats, in case vport stats retrieve failure. If both of > them success, error counters are added to include errors from > different layers. But implementation of ovs_vport_get_stats in > kernel data path has included kernel netdev stats by calling > dev_get_stats. When drop or other error counters is not zero, > its value is doubled by netdev_linux_get_stats. > > In this change, adding kernel netdev stats into vport stats > is removed, since vport stats includes all information of > kernel netdev stats. > > Signed-off-by: Jiang Lidong > --- > lib/netdev-linux.c | 27 +-------------------------- > 1 file changed, 1 insertion(+), 26 deletions(-) > > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index > ff045cb..6d139d0 100644 > --- a/lib/netdev-linux.c > +++ b/lib/netdev-linux.c > @@ -2207,33 +2207,8 @@ netdev_linux_get_stats(const struct netdev *netdev_, > } else if (netdev->vport_stats_error) { > /* stats not available from OVS then use netdev stats. */ > *stats = dev_stats; > - } else { > - /* Use kernel netdev's packet and byte counts since vport's counters > - * do not reflect packet counts on the wire when GSO, TSO or GRO are > - * enabled. */ > - stats->rx_packets = dev_stats.rx_packets; > - stats->rx_bytes = dev_stats.rx_bytes; > - stats->tx_packets = dev_stats.tx_packets; > - stats->tx_bytes = dev_stats.tx_bytes; > - > - stats->rx_errors += dev_stats.rx_errors; > - stats->tx_errors += dev_stats.tx_errors; > - stats->rx_dropped += dev_stats.rx_dropped; > - stats->tx_dropped += dev_stats.tx_dropped; Thanks for reporting the issue, looking at get_stats_via_vport get_stats_via_vport__ netdev_stats_from_ovs_vport_stats // only set the ovs_vport_stats, which has 8 fields above But with your patch it will remove all the reset of stats below > - stats->multicast += dev_stats.multicast; > - stats->collisions += dev_stats.collisions; > - stats->rx_length_errors += dev_stats.rx_length_errors; > - stats->rx_over_errors += dev_stats.rx_over_errors; > - stats->rx_crc_errors += dev_stats.rx_crc_errors; > - stats->rx_frame_errors += dev_stats.rx_frame_errors; > - stats->rx_fifo_errors += dev_stats.rx_fifo_errors; > - stats->rx_missed_errors += dev_stats.rx_missed_errors; > - stats->tx_aborted_errors += dev_stats.tx_aborted_errors; > - stats->tx_carrier_errors += dev_stats.tx_carrier_errors; > - stats->tx_fifo_errors += dev_stats.tx_fifo_errors; > - stats->tx_heartbeat_errors += dev_stats.tx_heartbeat_errors; > - stats->tx_window_errors += dev_stats.tx_window_errors; > } How about we just over write the error and drop counts? diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index c6e46f1885aa..c169c52611c4 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -2220,10 +2220,11 @@ netdev_linux_get_stats(const struct netdev *netdev_, stats->tx_packets = dev_stats.tx_packets; stats->tx_bytes = dev_stats.tx_bytes; - stats->rx_errors += dev_stats.rx_errors; - stats->tx_errors += dev_stats.tx_errors; - stats->rx_dropped += dev_stats.rx_dropped; - stats->tx_dropped += dev_stats.tx_dropped; + stats->rx_errors = dev_stats.rx_errors; + stats->tx_errors = dev_stats.tx_errors; + stats->rx_dropped = dev_stats.rx_dropped; + stats->tx_dropped = dev_stats.tx_dropped; + stats->multicast += dev_stats.multicast; stats->collisions += dev_stats.collisions; stats->rx_length_errors += dev_stats.rx_length_errors;