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

[Tahhan, Maryam]

Best Regards, 
Maryam

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz at 6wind.com]
> Sent: Friday, July 3, 2015 2:16 PM
> To: Tahhan, Maryam; dev at 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 at 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)  {
> >