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 43EED5A5D for ; Wed, 15 Jul 2015 15:14:28 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga103.fm.intel.com with ESMTP; 15 Jul 2015 06:14:27 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,480,1432623600"; d="scan'208";a="747773579" Received: from irsmsx101.ger.corp.intel.com ([163.33.3.153]) by fmsmga001.fm.intel.com with ESMTP; 15 Jul 2015 06:14:26 -0700 Received: from irsmsx109.ger.corp.intel.com ([169.254.13.200]) by IRSMSX101.ger.corp.intel.com ([169.254.1.201]) with mapi id 14.03.0224.002; Wed, 15 Jul 2015 14:14:25 +0100 From: "Tahhan, Maryam" To: Olivier MATZ , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v4 3/8] ethdev: expose extended error stats Thread-Index: AQHQt0mpOOrfYt3E5UqklBDI793uz53cPzgAgABSOUA= Date: Wed, 15 Jul 2015 13:14:24 +0000 Message-ID: <1A27633A6DA49C4A92FCD5D4312DBF536A466407@IRSMSX109.ger.corp.intel.com> References: <1436118000-132275-1-git-send-email-maryam.tahhan@intel.com> <1436118000-132275-4-git-send-email-maryam.tahhan@intel.com> <55A62582.9090208@6wind.com> In-Reply-To: <55A62582.9090208@6wind.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v4 3/8] ethdev: expose extended error stats 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: Wed, 15 Jul 2015 13:14:29 -0000 > From: Olivier MATZ [mailto:olivier.matz@6wind.com] > Sent: Wednesday, July 15, 2015 10:19 AM > To: Tahhan, Maryam; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v4 3/8] ethdev: expose extended error stat= s >=20 > Hi Maryam, >=20 > The API of the driver-specific function should be the same than the gener= ic > one, described in rte_ethdev.h. >=20 > What I mean is: > - the xstats pointer is the place where the xstats should be written > by the driver > - the n argument is the number of entries in the xstats structure >=20 > The driver should not have to worry about the generic stats written by th= e > generic layer. >=20 > Please see some comments below to fix it. >=20 Hi Olivier I have sent an updated patch set with the fixes recommended below. Thanks for reviewing Maryam > On 07/05/2015 07:39 PM, Maryam Tahhan wrote: > > Extend rte_eth_xstats_get to retrieve additional stats from the device > > driver as well the ethdev generic stats. > > > > Signed-off-by: Maryam Tahhan > > --- > > drivers/net/ixgbe/ixgbe_ethdev.c | 7 ++++--- > > lib/librte_ether/rte_ethdev.c | 25 ++++++++++++++++--------- > > 2 files changed, 20 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c > > b/drivers/net/ixgbe/ixgbe_ethdev.c > > index 7feb03c..5971d41 100644 > > --- a/drivers/net/ixgbe/ixgbe_ethdev.c > > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c > > @@ -2035,6 +2035,7 @@ ixgbe_dev_xstats_get(struct rte_eth_dev *dev, > struct rte_eth_xstats *xstats, > > total_qprdc =3D 0; > > rxnfgpc =3D 0; > > txdgpc =3D 0; > > + count =3D n - IXGBE_NB_XSTATS; > > > > ixgbe_read_stats_registers(hw, hw_stats, &total_missed_rx, > &total_qbrc, > > &total_qprc, > &rxnfgpc, &txdgpc, &total_qprdc); @@ -2047,13 > > +2048,13 @@ ixgbe_dev_xstats_get(struct rte_eth_dev *dev, struct > > rte_eth_xstats *xstats, > > > > /* Extended stats */ > > for (i =3D 0; i < IXGBE_NB_XSTATS; i++) { > > - snprintf(xstats[i].name, sizeof(xstats[i].name), > > + snprintf(xstats[count].name, sizeof(xstats[i].name), > > "%s", rte_ixgbe_stats_strings[i].name); > > - xstats[i].value =3D *(uint64_t *)(((char *)hw_stats) + > > + xstats[count++].value =3D *(uint64_t *)(((char *)hw_stats) + > > > rte_ixgbe_stats_strings[i].offset); > > } > > > > - return count; > > + return IXGBE_NB_XSTATS; >=20 > The modifications in ixgbe driver can be removed: the patch 2/8 already > provides everything that is required. >=20 >=20 > > } > > > > static void > > diff --git a/lib/librte_ether/rte_ethdev.c > > b/lib/librte_ether/rte_ethdev.c index da915db..e392f60 100644 > > --- a/lib/librte_ether/rte_ethdev.c > > +++ b/lib/librte_ether/rte_ethdev.c > > @@ -1681,26 +1681,33 @@ rte_eth_xstats_get(uint8_t port_id, struct > rte_eth_xstats *xstats, > > { > > struct rte_eth_stats eth_stats; > > struct rte_eth_dev *dev; > > - unsigned count, i, q; > > + unsigned count =3D 0, i, q; > > + signed xcount =3D 0; > > uint64_t val, *stats_ptr; > > > > VALID_PORTID_OR_ERR_RET(port_id, -EINVAL); > > > > dev =3D &rte_eth_devices[port_id]; > > > > - /* implemented by the driver */ > > - if (dev->dev_ops->xstats_get !=3D NULL) > > - return (*dev->dev_ops->xstats_get)(dev, xstats, n); > > - > > - /* else, return generic statistics */ > > + /* Return generic statistics */ > > count =3D RTE_NB_STATS; > > count +=3D dev->data->nb_rx_queues * RTE_NB_RXQ_STATS; > > count +=3D dev->data->nb_tx_queues * RTE_NB_TXQ_STATS; > > + > > + /* implemented by the driver */ > > + if (dev->dev_ops->xstats_get !=3D NULL) { > > + /* Retrieve the xstats from the driver at the end of the > > + * xstats struct. > > + */ > > + xcount =3D (*dev->dev_ops->xstats_get)(dev, xstats, n); >=20 > it should be: >=20 > xcount =3D (*dev->dev_ops->xstats_get)(dev, &xstats[count], > (n > count) ? n - count : 0); >=20 >=20 > > + if (xcount < 0) > > + return xcount; > > + } > > + > > if (n < count) > > - return count; > > + return count + xcount; >=20 > it should be: >=20 > if (n < count + xcount) > return count + xcount; >=20 >=20 > > > > /* now fill the xstats structure */ > > - > > count =3D 0; > > rte_eth_stats_get(port_id, ð_stats); > > > > @@ -1742,7 +1749,7 @@ rte_eth_xstats_get(uint8_t port_id, struct > rte_eth_xstats *xstats, > > } > > } > > > > - return count; > > + return count + xcount; > > } > > > > /* reset ethdev extended statistics */ > > >=20 > Regards, > Olivier