Re: [dpdk-dev] [PATCH v4 3/8] ethdev: expose extended error stats

["Tahhan, Maryam" <maryam.tahhan@intel.com>]

> 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 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@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, &eth_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