> From: Olivier MATZ [mailto:olivier.matz at 6wind.com]
> Sent: Wednesday, July 15, 2015 10:19 AM
> To: Tahhan, Maryam; dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 3/8] ethdev: expose extended error stats
>
> Hi Maryam,
>
> The API of the driver-specific function should be the same than the generic
> one, described in rte_ethdev.h.
>
> 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
>
> The driver should not have to worry about the generic stats written by the
> generic layer.
>
> Please see some comments below to fix it.
>
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 <maryam.tahhan at intel.com>
> > ---
> > 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 = 0;
> > rxnfgpc = 0;
> > txdgpc = 0;
> > + count = 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 = 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 = *(uint64_t *)(((char *)hw_stats) +
> > + xstats[count++].value = *(uint64_t *)(((char *)hw_stats) +
> >
> rte_ixgbe_stats_strings[i].offset);
> > }
> >
> > - return count;
> > + return IXGBE_NB_XSTATS;
>
> The modifications in ixgbe driver can be removed: the patch 2/8 already
> provides everything that is required.
>
>
> > }
> >
> > 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 = 0, i, q;
> > + signed xcount = 0;
> > uint64_t val, *stats_ptr;
> >
> > VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> >
> > dev = &rte_eth_devices[port_id];
> >
> > - /* implemented by the driver */
> > - if (dev->dev_ops->xstats_get != NULL)
> > - return (*dev->dev_ops->xstats_get)(dev, xstats, n);
> > -
> > - /* else, return generic statistics */
> > + /* Return generic statistics */
> > count = RTE_NB_STATS;
> > count += dev->data->nb_rx_queues * RTE_NB_RXQ_STATS;
> > count += dev->data->nb_tx_queues * RTE_NB_TXQ_STATS;
> > +
> > + /* implemented by the driver */
> > + if (dev->dev_ops->xstats_get != NULL) {
> > + /* Retrieve the xstats from the driver at the end of the
> > + * xstats struct.
> > + */
> > + xcount = (*dev->dev_ops->xstats_get)(dev, xstats, n);
>
> it should be:
>
> xcount = (*dev->dev_ops->xstats_get)(dev, &xstats[count],
> (n > count) ? n - count : 0);
>
>
> > + if (xcount < 0)
> > + return xcount;
> > + }
> > +
> > if (n < count)
> > - return count;
> > + return count + xcount;
>
> it should be:
>
> if (n < count + xcount)
> return count + xcount;
>
>
> >
> > /* now fill the xstats structure */
> > -
> > count = 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 */
> >
>
> Regards,
> Olivier
