[dpdk-dev,v3,2/7] ixgbe: add functions to get and reset xstats

  Implement ixgbe_dev_xstats_reset and ixgbe_dev_xstats_get.

Signed-off-by: Maryam Tahhan <maryam.tahhan@intel.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 85 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)
  

Hi Maryam,

On 06/26/2015 02:59 PM, Maryam Tahhan wrote:
> Implement ixgbe_dev_xstats_reset and ixgbe_dev_xstats_get.
> 
> Signed-off-by: Maryam Tahhan <maryam.tahhan@intel.com>
> ---
>  drivers/net/ixgbe/ixgbe_ethdev.c | 85 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 85 insertions(+)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 927021f..0f62bcb 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -131,7 +131,10 @@ static int ixgbe_dev_link_update(struct rte_eth_dev *dev,
>  				int wait_to_complete);
>  static void ixgbe_dev_stats_get(struct rte_eth_dev *dev,
>  				struct rte_eth_stats *stats);
> +static int ixgbe_dev_xstats_get(struct rte_eth_dev *dev,
> +				struct rte_eth_xstats *xstats, unsigned n);
>  static void ixgbe_dev_stats_reset(struct rte_eth_dev *dev);
> +static void ixgbe_dev_xstats_reset(struct rte_eth_dev *dev);
>  static int ixgbe_dev_queue_stats_mapping_set(struct rte_eth_dev *eth_dev,
>  					     uint16_t queue_id,
>  					     uint8_t stat_idx,
> @@ -334,7 +337,9 @@ static const struct eth_dev_ops ixgbe_eth_dev_ops = {
>  	.allmulticast_disable = ixgbe_dev_allmulticast_disable,
>  	.link_update          = ixgbe_dev_link_update,
>  	.stats_get            = ixgbe_dev_stats_get,
> +	.xstats_get           = ixgbe_dev_xstats_get,
>  	.stats_reset          = ixgbe_dev_stats_reset,
> +	.xstats_reset         = ixgbe_dev_xstats_reset,
>  	.queue_stats_mapping_set = ixgbe_dev_queue_stats_mapping_set,
>  	.dev_infos_get        = ixgbe_dev_info_get,
>  	.mtu_set              = ixgbe_dev_mtu_set,
> @@ -414,6 +419,33 @@ static const struct eth_dev_ops ixgbevf_eth_dev_ops = {
>  	.set_mc_addr_list     = ixgbe_dev_set_mc_addr_list,
>  };
>  
> +/* store statistics names and its offset in stats structure  */ struct
> +rte_ixgbe_xstats_name_off {
> +	char name[RTE_ETH_XSTATS_NAME_SIZE];
> +	unsigned offset;
> +};
> +
> +static const struct rte_ixgbe_xstats_name_off rte_ixgbe_stats_strings[] = {
> +	{"rx_illegal_byte_err", offsetof(struct ixgbe_hw_stats, errbc)},
> +	{"rx_len_err", offsetof(struct ixgbe_hw_stats, rlec)},
> +	{"rx_undersize_count", offsetof(struct ixgbe_hw_stats, ruc)},
> +	{"rx_oversize_count", offsetof(struct ixgbe_hw_stats, roc)},
> +	{"rx_fragment_count", offsetof(struct ixgbe_hw_stats, rfc)},
> +	{"rx_jabber_count", offsetof(struct ixgbe_hw_stats, rjc)},
> +	{"l3_l4_xsum_error", offsetof(struct ixgbe_hw_stats, xec)},
> +	{"mac_local_fault", offsetof(struct ixgbe_hw_stats, mlfc)},
> +	{"mac_remote_fault", offsetof(struct ixgbe_hw_stats, mrfc)},
> +	{"mac_short_pkt_discard", offsetof(struct ixgbe_hw_stats, mspdc)},
> +	{"fccrc_error", offsetof(struct ixgbe_hw_stats, fccrc)},
> +	{"fcoe_drop", offsetof(struct ixgbe_hw_stats, fcoerpdc)},
> +	{"fc_last_error", offsetof(struct ixgbe_hw_stats, fclast)},
> +	{"rx_broadcast_packets", offsetof(struct ixgbe_hw_stats, bprc)},
> +	{"mgmt_pkts_dropped", offsetof(struct ixgbe_hw_stats, mngpdc)},
> +};
> +
> +#define RTE_NB_XSTATS (sizeof(rte_ixgbe_stats_strings) /	\
> +		sizeof(rte_ixgbe_stats_strings[0]))
> +

Maybe RTE_NB_XSTATS should be renamed in IXGBE_NB_XSTATS?


>  /**
>   * Atomically reads the link status information from global
>   * structure rte_eth_dev.
> @@ -1982,6 +2014,59 @@ ixgbe_dev_stats_reset(struct rte_eth_dev *dev)
>  	memset(stats, 0, sizeof(*stats));
>  }
>  
> +static int
> +ixgbe_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstats *xstats,
> +					 unsigned n)
> +{
> +	struct ixgbe_hw *hw =
> +			IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +	struct ixgbe_hw_stats *hw_stats =
> +			IXGBE_DEV_PRIVATE_TO_STATS(dev->data->dev_private);
> +	uint64_t total_missed_rx, total_qbrc, total_qprc, total_qprdc;
> +	uint64_t rxnfgpc, txdgpc;
> +	unsigned i, count = RTE_NB_XSTATS;
> +
> +	if (n == 0)
> +		return count;

I think it does not exactly matches the API described in rte_ethdev.h:

 * @return
 *   - positive value lower or equal to n: success. The return value
 *     is the number of entries filled in the stats table.
 *   - positive value higher than n: error, the given statistics table
 *     is too small. The return value corresponds to the size that should
 *     be given to succeed. The entries in the table are not valid and
 *     shall not be used by the caller.
 *   - negative value on error (invalid port id)

I think it should be:

  if (n < count)
    return count


> +
> +	total_missed_rx = 0;
> +	total_qbrc = 0;
> +	total_qprc = 0;
> +	total_qprdc = 0;
> +	rxnfgpc = 0;
> +	txdgpc = 0;
> +	count = 0;
> +
> +	ixgbe_read_stats_registers(hw, hw_stats, &total_missed_rx, &total_qbrc,
> +							   &total_qprc, &rxnfgpc, &txdgpc, &total_qprdc);
> +
> +	if (!xstats)
> +		return 0;

this cannot happen except if n == 0.
This condition is already tested above, and "count" should be returned.

> +
> +	/* Error stats */
> +	for (i = 0; i < RTE_NB_XSTATS; i++) {
> +		snprintf(xstats[count].name, sizeof(xstats[count].name),
> +				"%s", rte_ixgbe_stats_strings[i].name);
> +		xstats[count++].value = *(uint64_t *)(((char *)hw_stats) +
> +							rte_ixgbe_stats_strings[i].offset);
> +	}
> +
> +	return count;
> +}

Shouldn't it be xstats[i] instead of xstats[count] ?

Does it work when using "show port in test-pmd"?

> +
> +static void
> +ixgbe_dev_xstats_reset(struct rte_eth_dev *dev)
> +{
> +	struct ixgbe_hw_stats *stats =
> +			IXGBE_DEV_PRIVATE_TO_STATS(dev->data->dev_private);
> +
> +	/* HW registers are cleared on read */
> +	ixgbe_dev_xstats_get(dev, NULL, RTE_NB_XSTATS);
> +
> +	/* Reset software totals */
> +	memset(stats, 0, sizeof(*stats));
> +}
> +
>  static void
>  ixgbevf_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
>  {
>
  

On 07/03/2015 03:16 PM, Olivier MATZ wrote:
> Hi Maryam,
> 
> On 06/26/2015 02:59 PM, Maryam Tahhan wrote:
>> Implement ixgbe_dev_xstats_reset and ixgbe_dev_xstats_get.
>>
>> Signed-off-by: Maryam Tahhan <maryam.tahhan@intel.com>
>> ---
>>  drivers/net/ixgbe/ixgbe_ethdev.c | 85 ++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 85 insertions(+)
>>
>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
>> index 927021f..0f62bcb 100644
>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>> @@ -131,7 +131,10 @@ static int ixgbe_dev_link_update(struct rte_eth_dev *dev,
>>  				int wait_to_complete);
>>  static void ixgbe_dev_stats_get(struct rte_eth_dev *dev,
>>  				struct rte_eth_stats *stats);
>> +static int ixgbe_dev_xstats_get(struct rte_eth_dev *dev,
>> +				struct rte_eth_xstats *xstats, unsigned n);
>>  static void ixgbe_dev_stats_reset(struct rte_eth_dev *dev);
>> +static void ixgbe_dev_xstats_reset(struct rte_eth_dev *dev);
>>  static int ixgbe_dev_queue_stats_mapping_set(struct rte_eth_dev *eth_dev,
>>  					     uint16_t queue_id,
>>  					     uint8_t stat_idx,
>> @@ -334,7 +337,9 @@ static const struct eth_dev_ops ixgbe_eth_dev_ops = {
>>  	.allmulticast_disable = ixgbe_dev_allmulticast_disable,
>>  	.link_update          = ixgbe_dev_link_update,
>>  	.stats_get            = ixgbe_dev_stats_get,
>> +	.xstats_get           = ixgbe_dev_xstats_get,
>>  	.stats_reset          = ixgbe_dev_stats_reset,
>> +	.xstats_reset         = ixgbe_dev_xstats_reset,
>>  	.queue_stats_mapping_set = ixgbe_dev_queue_stats_mapping_set,
>>  	.dev_infos_get        = ixgbe_dev_info_get,
>>  	.mtu_set              = ixgbe_dev_mtu_set,
>> @@ -414,6 +419,33 @@ static const struct eth_dev_ops ixgbevf_eth_dev_ops = {
>>  	.set_mc_addr_list     = ixgbe_dev_set_mc_addr_list,
>>  };
>>  
>> +/* store statistics names and its offset in stats structure  */ struct
>> +rte_ixgbe_xstats_name_off {
>> +	char name[RTE_ETH_XSTATS_NAME_SIZE];
>> +	unsigned offset;
>> +};
>> +
>> +static const struct rte_ixgbe_xstats_name_off rte_ixgbe_stats_strings[] = {
>> +	{"rx_illegal_byte_err", offsetof(struct ixgbe_hw_stats, errbc)},
>> +	{"rx_len_err", offsetof(struct ixgbe_hw_stats, rlec)},
>> +	{"rx_undersize_count", offsetof(struct ixgbe_hw_stats, ruc)},
>> +	{"rx_oversize_count", offsetof(struct ixgbe_hw_stats, roc)},
>> +	{"rx_fragment_count", offsetof(struct ixgbe_hw_stats, rfc)},
>> +	{"rx_jabber_count", offsetof(struct ixgbe_hw_stats, rjc)},
>> +	{"l3_l4_xsum_error", offsetof(struct ixgbe_hw_stats, xec)},
>> +	{"mac_local_fault", offsetof(struct ixgbe_hw_stats, mlfc)},
>> +	{"mac_remote_fault", offsetof(struct ixgbe_hw_stats, mrfc)},
>> +	{"mac_short_pkt_discard", offsetof(struct ixgbe_hw_stats, mspdc)},
>> +	{"fccrc_error", offsetof(struct ixgbe_hw_stats, fccrc)},
>> +	{"fcoe_drop", offsetof(struct ixgbe_hw_stats, fcoerpdc)},
>> +	{"fc_last_error", offsetof(struct ixgbe_hw_stats, fclast)},
>> +	{"rx_broadcast_packets", offsetof(struct ixgbe_hw_stats, bprc)},
>> +	{"mgmt_pkts_dropped", offsetof(struct ixgbe_hw_stats, mngpdc)},
>> +};
>> +
>> +#define RTE_NB_XSTATS (sizeof(rte_ixgbe_stats_strings) /	\
>> +		sizeof(rte_ixgbe_stats_strings[0]))
>> +
> 
> Maybe RTE_NB_XSTATS should be renamed in IXGBE_NB_XSTATS?
> 
> 
>>  /**
>>   * Atomically reads the link status information from global
>>   * structure rte_eth_dev.
>> @@ -1982,6 +2014,59 @@ ixgbe_dev_stats_reset(struct rte_eth_dev *dev)
>>  	memset(stats, 0, sizeof(*stats));
>>  }
>>  
>> +static int
>> +ixgbe_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstats *xstats,
>> +					 unsigned n)
>> +{
>> +	struct ixgbe_hw *hw =
>> +			IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>> +	struct ixgbe_hw_stats *hw_stats =
>> +			IXGBE_DEV_PRIVATE_TO_STATS(dev->data->dev_private);
>> +	uint64_t total_missed_rx, total_qbrc, total_qprc, total_qprdc;
>> +	uint64_t rxnfgpc, txdgpc;
>> +	unsigned i, count = RTE_NB_XSTATS;
>> +
>> +	if (n == 0)
>> +		return count;
> 
> I think it does not exactly matches the API described in rte_ethdev.h:
> 
>  * @return
>  *   - positive value lower or equal to n: success. The return value
>  *     is the number of entries filled in the stats table.
>  *   - positive value higher than n: error, the given statistics table
>  *     is too small. The return value corresponds to the size that should
>  *     be given to succeed. The entries in the table are not valid and
>  *     shall not be used by the caller.
>  *   - negative value on error (invalid port id)
> 
> I think it should be:
> 
>   if (n < count)
>     return count
> 
> 
>> +
>> +	total_missed_rx = 0;
>> +	total_qbrc = 0;
>> +	total_qprc = 0;
>> +	total_qprdc = 0;
>> +	rxnfgpc = 0;
>> +	txdgpc = 0;
>> +	count = 0;
>> +
>> +	ixgbe_read_stats_registers(hw, hw_stats, &total_missed_rx, &total_qbrc,
>> +							   &total_qprc, &rxnfgpc, &txdgpc, &total_qprdc);
>> +
>> +	if (!xstats)
>> +		return 0;
> 
> this cannot happen except if n == 0.
> This condition is already tested above, and "count" should be returned.
> 
>> +
>> +	/* Error stats */
>> +	for (i = 0; i < RTE_NB_XSTATS; i++) {
>> +		snprintf(xstats[count].name, sizeof(xstats[count].name),
>> +				"%s", rte_ixgbe_stats_strings[i].name);
>> +		xstats[count++].value = *(uint64_t *)(((char *)hw_stats) +
>> +							rte_ixgbe_stats_strings[i].offset);
>> +	}
>> +
>> +	return count;
>> +}
> 
> Shouldn't it be xstats[i] instead of xstats[count] ?
> 
> Does it work when using "show port in test-pmd"?

ok I missed the 'count = 0' above.
So why using count instead of i ?

Also, I think it would be clearer to use the constant IXGBE_NB_XSTATS
instead of count at the beginning of the function.


> 
>> +
>> +static void
>> +ixgbe_dev_xstats_reset(struct rte_eth_dev *dev)
>> +{
>> +	struct ixgbe_hw_stats *stats =
>> +			IXGBE_DEV_PRIVATE_TO_STATS(dev->data->dev_private);
>> +
>> +	/* HW registers are cleared on read */
>> +	ixgbe_dev_xstats_get(dev, NULL, RTE_NB_XSTATS);
>> +
>> +	/* Reset software totals */
>> +	memset(stats, 0, sizeof(*stats));
>> +}
>> +
>>  static void
>>  ixgbevf_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
>>  {
>>
  

Best Regards, 
Maryam

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Friday, July 3, 2015 2:16 PM
> To: Tahhan, Maryam; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 2/7] ixgbe: add functions to get and reset
> xstats
> 
> Hi Maryam,
> 
> On 06/26/2015 02:59 PM, Maryam Tahhan wrote:
> > Implement ixgbe_dev_xstats_reset and ixgbe_dev_xstats_get.
> >
> > Signed-off-by: Maryam Tahhan <maryam.tahhan@intel.com>
> > ---
> >  drivers/net/ixgbe/ixgbe_ethdev.c | 85
> > ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 85 insertions(+)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > index 927021f..0f62bcb 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > @@ -131,7 +131,10 @@ static int ixgbe_dev_link_update(struct
> rte_eth_dev *dev,
> >  				int wait_to_complete);
> >  static void ixgbe_dev_stats_get(struct rte_eth_dev *dev,
> >  				struct rte_eth_stats *stats);
> > +static int ixgbe_dev_xstats_get(struct rte_eth_dev *dev,
> > +				struct rte_eth_xstats *xstats, unsigned n);
> >  static void ixgbe_dev_stats_reset(struct rte_eth_dev *dev);
> > +static void ixgbe_dev_xstats_reset(struct rte_eth_dev *dev);
> >  static int ixgbe_dev_queue_stats_mapping_set(struct rte_eth_dev
> *eth_dev,
> >  					     uint16_t queue_id,
> >  					     uint8_t stat_idx,
> > @@ -334,7 +337,9 @@ static const struct eth_dev_ops ixgbe_eth_dev_ops
> = {
> >  	.allmulticast_disable = ixgbe_dev_allmulticast_disable,
> >  	.link_update          = ixgbe_dev_link_update,
> >  	.stats_get            = ixgbe_dev_stats_get,
> > +	.xstats_get           = ixgbe_dev_xstats_get,
> >  	.stats_reset          = ixgbe_dev_stats_reset,
> > +	.xstats_reset         = ixgbe_dev_xstats_reset,
> >  	.queue_stats_mapping_set = ixgbe_dev_queue_stats_mapping_set,
> >  	.dev_infos_get        = ixgbe_dev_info_get,
> >  	.mtu_set              = ixgbe_dev_mtu_set,
> > @@ -414,6 +419,33 @@ static const struct eth_dev_ops
> ixgbevf_eth_dev_ops = {
> >  	.set_mc_addr_list     = ixgbe_dev_set_mc_addr_list,
> >  };
> >
> > +/* store statistics names and its offset in stats structure  */
> > +struct rte_ixgbe_xstats_name_off {
> > +	char name[RTE_ETH_XSTATS_NAME_SIZE];
> > +	unsigned offset;
> > +};
> > +
> > +static const struct rte_ixgbe_xstats_name_off rte_ixgbe_stats_strings[] = {
> > +	{"rx_illegal_byte_err", offsetof(struct ixgbe_hw_stats, errbc)},
> > +	{"rx_len_err", offsetof(struct ixgbe_hw_stats, rlec)},
> > +	{"rx_undersize_count", offsetof(struct ixgbe_hw_stats, ruc)},
> > +	{"rx_oversize_count", offsetof(struct ixgbe_hw_stats, roc)},
> > +	{"rx_fragment_count", offsetof(struct ixgbe_hw_stats, rfc)},
> > +	{"rx_jabber_count", offsetof(struct ixgbe_hw_stats, rjc)},
> > +	{"l3_l4_xsum_error", offsetof(struct ixgbe_hw_stats, xec)},
> > +	{"mac_local_fault", offsetof(struct ixgbe_hw_stats, mlfc)},
> > +	{"mac_remote_fault", offsetof(struct ixgbe_hw_stats, mrfc)},
> > +	{"mac_short_pkt_discard", offsetof(struct ixgbe_hw_stats, mspdc)},
> > +	{"fccrc_error", offsetof(struct ixgbe_hw_stats, fccrc)},
> > +	{"fcoe_drop", offsetof(struct ixgbe_hw_stats, fcoerpdc)},
> > +	{"fc_last_error", offsetof(struct ixgbe_hw_stats, fclast)},
> > +	{"rx_broadcast_packets", offsetof(struct ixgbe_hw_stats, bprc)},
> > +	{"mgmt_pkts_dropped", offsetof(struct ixgbe_hw_stats, mngpdc)}, };
> > +
> > +#define RTE_NB_XSTATS (sizeof(rte_ixgbe_stats_strings) /	\
> > +		sizeof(rte_ixgbe_stats_strings[0]))
> > +
> 
> Maybe RTE_NB_XSTATS should be renamed in IXGBE_NB_XSTATS?
> 
> 
> >  /**
> >   * Atomically reads the link status information from global
> >   * structure rte_eth_dev.
> > @@ -1982,6 +2014,59 @@ ixgbe_dev_stats_reset(struct rte_eth_dev
> *dev)
> >  	memset(stats, 0, sizeof(*stats));
> >  }
> >
> > +static int
> > +ixgbe_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstats
> *xstats,
> > +					 unsigned n)
> > +{
> > +	struct ixgbe_hw *hw =
> > +			IXGBE_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> > +	struct ixgbe_hw_stats *hw_stats =
> > +			IXGBE_DEV_PRIVATE_TO_STATS(dev->data-
> >dev_private);
> > +	uint64_t total_missed_rx, total_qbrc, total_qprc, total_qprdc;
> > +	uint64_t rxnfgpc, txdgpc;
> > +	unsigned i, count = RTE_NB_XSTATS;
> > +
> > +	if (n == 0)
> > +		return count;
> 
> I think it does not exactly matches the API described in rte_ethdev.h:
> 
>  * @return
>  *   - positive value lower or equal to n: success. The return value
>  *     is the number of entries filled in the stats table.
>  *   - positive value higher than n: error, the given statistics table
>  *     is too small. The return value corresponds to the size that should
>  *     be given to succeed. The entries in the table are not valid and
>  *     shall not be used by the caller.
>  *   - negative value on error (invalid port id)
> 
> I think it should be:
> 
>   if (n < count)
>     return count
> 

I was using 0 on the first call just to return the size of the extended stats structure that needs to be allocated by the application. It's passed as 0 from ethdev_get_stats to retrieve the size. But I will update it.
> 
> > +
> > +	total_missed_rx = 0;
> > +	total_qbrc = 0;
> > +	total_qprc = 0;
> > +	total_qprdc = 0;
> > +	rxnfgpc = 0;
> > +	txdgpc = 0;
> > +	count = 0;
> > +
> > +	ixgbe_read_stats_registers(hw, hw_stats, &total_missed_rx,
> &total_qbrc,
> > +							   &total_qprc,
> &rxnfgpc, &txdgpc, &total_qprdc);
> > +
> > +	if (!xstats)
> > +		return 0;
> 


This is the mechanism used to reset stats for normal stats and xstats. The reset functions call the stats function with the nullified stats structure. So I think this is ok to leave in, I will add a comment about it being part of the reset functionality

> this cannot happen except if n == 0.
> This condition is already tested above, and "count" should be returned.
> 
> > +
> > +	/* Error stats */
> > +	for (i = 0; i < RTE_NB_XSTATS; i++) {
> > +		snprintf(xstats[count].name, sizeof(xstats[count].name),
> > +				"%s", rte_ixgbe_stats_strings[i].name);
> > +		xstats[count++].value = *(uint64_t *)(((char *)hw_stats) +
> > +
> 	rte_ixgbe_stats_strings[i].offset);
> > +	}
> > +
> > +	return count;
> > +}
> 
> Shouldn't it be xstats[i] instead of xstats[count] ?

No, i is just used to cycle through the stats string structure but we use the count for xstats because that is the index that is passed in from  rte_eth_xstats_get(). Remember we are filling out the xstats structure with generic stats at the ethdev level first and then passing the xstats structure to the driver for the rest of it to be filled out.
> 
> Does it work when using "show port in test-pmd"?


Yes, I tested it before patch submission and it works.

> 
> > +
> > +static void
> > +ixgbe_dev_xstats_reset(struct rte_eth_dev *dev) {
> > +	struct ixgbe_hw_stats *stats =
> > +			IXGBE_DEV_PRIVATE_TO_STATS(dev->data-
> >dev_private);
> > +
> > +	/* HW registers are cleared on read */
> > +	ixgbe_dev_xstats_get(dev, NULL, RTE_NB_XSTATS);
> > +
> > +	/* Reset software totals */
> > +	memset(stats, 0, sizeof(*stats));
> > +}
> > +
> >  static void
> >  ixgbevf_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats
> > *stats)  {
> >
  

<snip>
> >
> >> +
> >> +	total_missed_rx = 0;
> >> +	total_qbrc = 0;
> >> +	total_qprc = 0;
> >> +	total_qprdc = 0;
> >> +	rxnfgpc = 0;
> >> +	txdgpc = 0;
> >> +	count = 0;
> >> +
> >> +	ixgbe_read_stats_registers(hw, hw_stats, &total_missed_rx,
> &total_qbrc,
> >> +							   &total_qprc,
> &rxnfgpc, &txdgpc, &total_qprdc);
> >> +
> >> +	if (!xstats)
> >> +		return 0;
> >
> > this cannot happen except if n == 0.
> > This condition is already tested above, and "count" should be returned.
> >
> >> +
> >> +	/* Error stats */
> >> +	for (i = 0; i < RTE_NB_XSTATS; i++) {
> >> +		snprintf(xstats[count].name, sizeof(xstats[count].name),
> >> +				"%s", rte_ixgbe_stats_strings[i].name);
> >> +		xstats[count++].value = *(uint64_t *)(((char *)hw_stats) +
> >> +
> 	rte_ixgbe_stats_strings[i].offset);
> >> +	}
> >> +
> >> +	return count;
> >> +}
> >
> > Shouldn't it be xstats[i] instead of xstats[count] ?
> >
> > Does it work when using "show port in test-pmd"?
> 
> ok I missed the 'count = 0' above.
> So why using count instead of i ?
> 
> Also, I think it would be clearer to use the constant IXGBE_NB_XSTATS instead
> of count at the beginning of the function.
> 
> 

Hi Olivier

Actually, count should not be 0, it should be n, which is the passed in index from rte_eth_xstats_get()...

Because we fill out the generic stats first in rte_eth_xstats_get() then we call ixgbe_dev_xstats_get to fill out the rest.

I will fix this up
> >
> >> +
> >> +static void
> >> +ixgbe_dev_xstats_reset(struct rte_eth_dev *dev) {
> >> +	struct ixgbe_hw_stats *stats =
> >> +			IXGBE_DEV_PRIVATE_TO_STATS(dev->data-
> >dev_private);
> >> +
> >> +	/* HW registers are cleared on read */
> >> +	ixgbe_dev_xstats_get(dev, NULL, RTE_NB_XSTATS);
> >> +
> >> +	/* Reset software totals */
> >> +	memset(stats, 0, sizeof(*stats));
> >> +}
> >> +
> >>  static void
> >>  ixgbevf_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats
> >> *stats)  {
> >>
  

> 
> 
> <snip>
> > >
> > >> +
> > >> +	total_missed_rx = 0;
> > >> +	total_qbrc = 0;
> > >> +	total_qprc = 0;
> > >> +	total_qprdc = 0;
> > >> +	rxnfgpc = 0;
> > >> +	txdgpc = 0;
> > >> +	count = 0;
> > >> +
> > >> +	ixgbe_read_stats_registers(hw, hw_stats, &total_missed_rx,
> > &total_qbrc,
> > >> +							   &total_qprc,
> > &rxnfgpc, &txdgpc, &total_qprdc);
> > >> +
> > >> +	if (!xstats)
> > >> +		return 0;
> > >
> > > this cannot happen except if n == 0.
> > > This condition is already tested above, and "count" should be returned.
> > >
> > >> +
> > >> +	/* Error stats */
> > >> +	for (i = 0; i < RTE_NB_XSTATS; i++) {
> > >> +		snprintf(xstats[count].name, sizeof(xstats[count].name),
> > >> +				"%s", rte_ixgbe_stats_strings[i].name);
> > >> +		xstats[count++].value = *(uint64_t *)(((char *)hw_stats) +
> > >> +
> > 	rte_ixgbe_stats_strings[i].offset);
> > >> +	}
> > >> +
> > >> +	return count;
> > >> +}
> > >
> > > Shouldn't it be xstats[i] instead of xstats[count] ?
> > >
> > > Does it work when using "show port in test-pmd"?
> >
> > ok I missed the 'count = 0' above.
> > So why using count instead of i ?
> >
> > Also, I think it would be clearer to use the constant IXGBE_NB_XSTATS
> > instead of count at the beginning of the function.
> >
> >
> 
> Hi Olivier
> 
> Actually, count should not be 0, it should be n, which is the passed in index
> from rte_eth_xstats_get()...
> 
> Because we fill out the generic stats first in rte_eth_xstats_get() then we call
> ixgbe_dev_xstats_get to fill out the rest.
> 
> I will fix this up
Hi Olivier, 

I confused this change with a subsequent patch in the patch set... so yes for this patch you are correct we can just use i... and leave count as 0. 

in a subsequent patch I modify count to start at n, which is a passed in index from in rte_eth_xstats_get()...

so I will change count for i in the loop here. 


> > >
> > >> +
> > >> +static void
> > >> +ixgbe_dev_xstats_reset(struct rte_eth_dev *dev) {
> > >> +	struct ixgbe_hw_stats *stats =
> > >> +			IXGBE_DEV_PRIVATE_TO_STATS(dev->data-
> > >dev_private);
> > >> +
> > >> +	/* HW registers are cleared on read */
> > >> +	ixgbe_dev_xstats_get(dev, NULL, RTE_NB_XSTATS);
> > >> +
> > >> +	/* Reset software totals */
> > >> +	memset(stats, 0, sizeof(*stats)); }
> > >> +
> > >>  static void
> > >>  ixgbevf_dev_stats_get(struct rte_eth_dev *dev, struct
> > >> rte_eth_stats
> > >> *stats)  {
> > >>