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

  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(-)
  

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.

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
  

> 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