| Message ID | 1427093798-23078-1-git-send-email-changchun.ouyang@intel.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 78A32379E; Mon, 23 Mar 2015 07:56:48 +0100 (CET) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 041D23796 for <dev@dpdk.org>; Mon, 23 Mar 2015 07:56:46 +0100 (CET) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga101.fm.intel.com with ESMTP; 22 Mar 2015 23:56:46 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.11,450,1422950400"; d="scan'208";a="702654928" Received: from shvmail01.sh.intel.com ([10.239.29.42]) by orsmga002.jf.intel.com with ESMTP; 22 Mar 2015 23:56:45 -0700 Received: from shecgisg004.sh.intel.com (shecgisg004.sh.intel.com [10.239.29.89]) by shvmail01.sh.intel.com with ESMTP id t2N6uhIK007764; Mon, 23 Mar 2015 14:56:43 +0800 Received: from shecgisg004.sh.intel.com (localhost [127.0.0.1]) by shecgisg004.sh.intel.com (8.13.6/8.13.6/SuSE Linux 0.8) with ESMTP id t2N6uekR023111; Mon, 23 Mar 2015 14:56:42 +0800 Received: (from couyang@localhost) by shecgisg004.sh.intel.com (8.13.6/8.13.6/Submit) id t2N6uenK023107; Mon, 23 Mar 2015 14:56:40 +0800 From: Ouyang Changchun <changchun.ouyang@intel.com> To: dev@dpdk.org Date: Mon, 23 Mar 2015 14:56:38 +0800 Message-Id: <1427093798-23078-1-git-send-email-changchun.ouyang@intel.com> X-Mailer: git-send-email 1.7.12.2 Subject: [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 <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Ouyang Changchun
March 23, 2015, 6:56 a.m. UTC
It need clear/reset the stats information before count in all queues data.
Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
---
lib/librte_pmd_virtio/virtio_ethdev.c | 8 ++++++++
1 file changed, 8 insertions(+)
Comments
Hello, Hello, On Mon, Mar 23, 2015 at 7:56 AM, Ouyang Changchun < changchun.ouyang@intel.com> wrote: > It need clear/reset the stats information before count in all queues data. > > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com> > --- > 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 = 0; > + stats->obytes = 0; > + stats->oerrors = 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.
On 3/23/2015 3:20 PM, David Marchand wrote: > Hello, > > Hello, > > On Mon, Mar 23, 2015 at 7:56 AM, Ouyang Changchun > <changchun.ouyang@intel.com <mailto:changchun.ouyang@intel.com>> wrote: > > It need clear/reset the stats information before count in all > queues data. > > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com > <mailto:changchun.ouyang@intel.com>> > --- > 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 = 0; > + stats->obytes = 0; > + stats->oerrors = 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; 2. we don't rely on the assumption of caller always zero'd the stats, and still can return correct value; thanks Changchun
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 > > <changchun.ouyang@intel.com <mailto:changchun.ouyang@intel.com>> wrote: > > > > It need clear/reset the stats information before count in all > > queues data. > > > > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com > > <mailto:changchun.ouyang@intel.com>> > > --- > > 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 = 0; > > + stats->obytes = 0; > > + stats->oerrors = 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; If there are some useless reset in other drivers, they should be removed. > 2. we don't rely on the assumption of caller always zero'd the stats, > and still can return correct value; The caller is ethdev and it is zero'ing the structure before the call: http://dpdk.org/browse/dpdk/commit/?id=02331c16ec0
I agree with Thomas. If some other drivers have memset they should also be fixed
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 > > 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 > > > <changchun.ouyang@intel.com <mailto:changchun.ouyang@intel.com>> > wrote: > > > > > > It need clear/reset the stats information before count in all > > > queues data. > > > > > > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com > > > <mailto:changchun.ouyang@intel.com>> > > > --- > > > 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 = 0; > > > + stats->obytes = 0; > > > + stats->oerrors = 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; > > If there are some useless reset in other drivers, they should be removed. > > > 2. we don't rely on the assumption of caller always zero'd the stats, > > and still can return correct value; > > The caller is ethdev and it is zero'ing the structure before the call: > http://dpdk.org/browse/dpdk/commit/?id=02331c16ec0 > I have seen this patch is rejected in patch-work, But I'd like continue to discuss it here. 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 assumption like here, caller must zero'd some memory? It could work, but I think the better way is every function should make sure it has correct behavior even without any assumption. On the other hand, I even think it is worthy to revert this patch, if you guys think the duplicated zero'd are not so nice: http://dpdk.org/browse/dpdk/commit/?id=02331c16ec0 the background is that some guys want to implement another function(mostly like API in ethdev) which will call virtio_dev_stats_get directly, and NOT zero'd the stats before the calling, and then get the incorrect stats info as stats use '+=' to calculate the value and nowhere zero'd it before do the '+='. So my opinion is we had better zero'd in virtio_dev_stats_get, instead of in rtedev, While other pmd use '=' rather than '+=', So they don't need explicitly zero'd the stats, but has similar effect. Any thoughts? Thanks Changchun
2015-03-27 14:26, Ouyang, Changchun: > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > > 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: > > > > --- 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 = 0; > > > > + stats->obytes = 0; > > > > + stats->oerrors = 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; > > > > If there are some useless reset in other drivers, they should be removed. > > > > > 2. we don't rely on the assumption of caller always zero'd the stats, > > > and still can return correct value; > > > > The caller is ethdev and it is zero'ing the structure before the call: > > http://dpdk.org/browse/dpdk/commit/?id=02331c16ec0 > > > > I have seen this patch is rejected in patch-work, > But I'd like continue to discuss it here. > > 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 assumption like here, caller must zero'd some memory? > It could work, but I think the better way is every function should make sure it has correct behavior even without any assumption. Assumptions are everywhere, we call it API ;) > On the other hand, I even think it is worthy to revert this patch, if you guys think the duplicated zero'd are not so nice: > http://dpdk.org/browse/dpdk/commit/?id=02331c16ec0 > > the background is that > some guys want to implement another function(mostly like API in ethdev) which will call virtio_dev_stats_get directly, > and NOT zero'd the stats before the calling, and then get the incorrect stats info as stats use '+=' to calculate the value and nowhere zero'd it before > do the '+='. You should think about zero'ing prerequisite as driver API of stats_get implementation. If you want to call it in another place, you just have to respect this API and do the zero'ing. This kind of API must be clear to show where are the responsibilities and make maintenance easier. > So my opinion is we had better zero'd in virtio_dev_stats_get, instead of in rtedev, > While other pmd use '=' rather than '+=', > So they don't need explicitly zero'd the stats, but has similar effect. > > Any thoughts? > > Thanks > Changchun
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 = 0; + stats->obytes = 0; + stats->oerrors = 0; + for (i = 0; i < dev->data->nb_tx_queues; i++) { const struct virtqueue *txvq = dev->data->tx_queues[i]; if (txvq == NULL) @@ -587,6 +591,10 @@ virtio_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats) } } + stats->ipackets = 0; + stats->ibytes = 0; + stats->ierrors = 0; + for (i = 0; i < dev->data->nb_rx_queues; i++) { const struct virtqueue *rxvq = dev->data->rx_queues[i]; if (rxvq == NULL)