From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id A8D5B5A73 for ; Fri, 27 Mar 2015 15:26:11 +0100 (CET) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga103.fm.intel.com with ESMTP; 27 Mar 2015 07:26:09 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.11,479,1422950400"; d="scan'208";a="705109577" Received: from pgsmsx103.gar.corp.intel.com ([10.221.44.82]) by orsmga002.jf.intel.com with ESMTP; 27 Mar 2015 07:26:03 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by PGSMSX103.gar.corp.intel.com (10.221.44.82) with Microsoft SMTP Server (TLS) id 14.3.224.2; Fri, 27 Mar 2015 22:26:02 +0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.198]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.108]) with mapi id 14.03.0224.002; Fri, 27 Mar 2015 22:26:01 +0800 From: "Ouyang, Changchun" To: Thomas Monjalon , Stephen Hemminger Thread-Topic: [dpdk-dev] [PATCH] virtio: Fix stats issue Thread-Index: AQHQZTaVUkrATAewH0mYHzrDXOz+WZ0pI0OAgACby4D//5xwAIAHBuDw Date: Fri, 27 Mar 2015 14:26:01 +0000 Message-ID: References: <1427093798-23078-1-git-send-email-changchun.ouyang@intel.com> <550FD107.8060109@intel.com> <11509543.rOgWUoKvXr@xps13> In-Reply-To: <11509543.rOgWUoKvXr@xps13> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH] virtio: Fix stats issue X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 27 Mar 2015 14:26:12 -0000 Hi Stephen, Thomas, David, > -----Original Message----- > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > Sent: Monday, March 23, 2015 6:42 PM > To: Ouyang, Changchun > Cc: dev@dpdk.org; David Marchand > Subject: Re: [dpdk-dev] [PATCH] virtio: Fix stats issue >=20 > 2015-03-23 16:38, Ouyang, Changchun: > > On 3/23/2015 3:20 PM, David Marchand wrote: > > > On Mon, Mar 23, 2015 at 7:56 AM, Ouyang Changchun > > > > > wrote: > > > > > > It need clear/reset the stats information before count in all > > > queues data. > > > > > > Signed-off-by: Changchun Ouyang > > > > > > --- > > > lib/librte_pmd_virtio/virtio_ethdev.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c > > > b/lib/librte_pmd_virtio/virtio_ethdev.c > > > index 603be2d..e4cb55e 100644 > > > --- a/lib/librte_pmd_virtio/virtio_ethdev.c > > > +++ b/lib/librte_pmd_virtio/virtio_ethdev.c > > > @@ -572,6 +572,10 @@ virtio_dev_stats_get(struct rte_eth_dev *dev= , > > > struct rte_eth_stats *stats) > > > { > > > unsigned i; > > > > > > + stats->opackets =3D 0; > > > + stats->obytes =3D 0; > > > + stats->oerrors =3D 0; > > > > > > > > > stats are supposed to be zero'd in generic rte_ethdev.c before this > > > pmd function is called, so this patch seems useless to me. > > > Can you give some context ? > > > > > > Same comment for the i* part. > > > > > 2 reasons: > > 1. this change could keep the stats_get has consistent behavior with > > the one in other drivers; >=20 > If there are some useless reset in other drivers, they should be removed. >=20 > > 2. we don't rely on the assumption of caller always zero'd the stats, > > and still can return correct value; >=20 > The caller is ethdev and it is zero'ing the structure before the call: > http://dpdk.org/browse/dpdk/commit/?id=3D02331c16ec0 >=20 I have seen this patch is rejected in patch-work, But I'd like continue to discuss it here. =20 Do you guys see any big issues if we initialize 0 here in this function? Well, I don't see any big issue here, just once again zero'd the stats info= before get the actual value, it doesn't affect the rx and tx perf. do you guys think it is better way to make a function behave under some ass= umption like here, caller must zero'd some memory? It could work, but I think the better way is every function should make sur= e it has correct behavior even without any assumption. On the other hand, I even think it is worthy to revert this patch, if you g= uys think the duplicated zero'd are not so nice: http://dpdk.org/browse/dpdk/commit/?id=3D02331c16ec0 the background is that=20 some guys want to implement another function(mostly like API in ethdev) whi= ch will call virtio_dev_stats_get directly, and NOT zero'd the stats before the calling, and then get the incorrect sta= ts info as stats use '+=3D' to calculate the value and nowhere zero'd it be= fore do the '+=3D'. =20 So my opinion is we had better zero'd in virtio_dev_stats_get, instead of i= n rtedev, While other pmd use '=3D' rather than '+=3D', So they don't need explicitly zero'd the stats, but has similar effect. Any thoughts? Thanks Changchun