[dpdk-dev] [PATCH v3 1/3] net/i40e: fix clear xstats bug in vf port

[Wu, Jingjing]

> -----Original Message-----
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Wei Zhao
> Sent: Monday, September 18, 2017 2:18 PM
> To: dev at dpdk.org
> Cc: Zhao1, Wei <wei.zhao1 at intel.com>
> Subject: [dpdk-dev] [PATCH v3 1/3] net/i40e: fix clear xstats bug in vf port
> 
> There is a bug in vf clear xstats command, it do not record the statics data in
> offset struct member.So, vf need to keep record of xstats data from pf and
> update the statics according to offset.
> 
> Fixes: da61cd0849766 ("i40evf: add extended stats")
> 
> Signed-off-by: Wei Zhao <wei.zhao1 at intel.com>
> 
> ---
> 
> Changes in v2:
> 
>  fix patch log check warning.
> 
> ---
> 
> changes in v3:
> 
>  remove nic_stats_display protect to a new patch
> ---
>  drivers/net/i40e/i40e_ethdev_vf.c | 64
> ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev_vf.c
> b/drivers/net/i40e/i40e_ethdev_vf.c
> index 38c3adc..806ff9e 100644
> --- a/drivers/net/i40e/i40e_ethdev_vf.c
> +++ b/drivers/net/i40e/i40e_ethdev_vf.c
> @@ -888,16 +888,74 @@ i40evf_update_stats(struct rte_eth_dev *dev, struct
> i40e_eth_stats **pstats)
>  	return 0;
>  }
> 
> +static void
> +i40evf_stat_update_48(uint64_t *offset,
> +		   uint64_t *stat)
> +{
> +	if (*stat >= *offset)
> +		*stat = *stat - *offset;
> +	else
> +		*stat = (uint64_t)((*stat +
> +			((uint64_t)1 << I40E_48_BIT_WIDTH)) - *offset);
> +
> +	*stat &= I40E_48_BIT_MASK;
> +}
> +
> +static void
> +i40evf_stat_update_32(uint64_t *offset,
> +		   uint64_t *stat)
> +{
> +	if (*stat >= *offset)
> +		*stat = (uint64_t)(*stat - *offset);
> +	else
> +		*stat = (uint64_t)((*stat +
> +			((uint64_t)1 << I40E_32_BIT_WIDTH)) - *offset); }

The type of count is 64 bits. Is that correct to use 1 << I40E_32_BIT_WIDTH?

> +
> +static void
> +i40evf_update_vsi_stats(struct i40e_vsi *vsi,
> +					struct i40e_eth_stats *nes)
> +{
> +	struct i40e_eth_stats *oes = &vsi->eth_stats_offset;
> +
> +	i40evf_stat_update_48(&oes->rx_bytes,
> +			    &nes->rx_bytes);
> +	i40evf_stat_update_48(&oes->rx_unicast,
> +			    &nes->rx_unicast);
> +	i40evf_stat_update_48(&oes->rx_multicast,
> +			    &nes->rx_multicast);
> +	i40evf_stat_update_48(&oes->rx_broadcast,
> +			    &nes->rx_broadcast);
> +	i40evf_stat_update_32(&oes->rx_discards,
> +				&nes->rx_discards);
> +	i40evf_stat_update_32(&oes->rx_unknown_protocol,
> +			    &nes->rx_unknown_protocol);
> +	i40evf_stat_update_48(&oes->tx_bytes,
> +			    &nes->tx_bytes);
> +	i40evf_stat_update_48(&oes->tx_unicast,
> +			    &nes->tx_unicast);
> +	i40evf_stat_update_48(&oes->tx_multicast,
> +			    &nes->tx_multicast);
> +	i40evf_stat_update_48(&oes->tx_broadcast,
> +			    &nes->tx_broadcast);
> +	i40evf_stat_update_32(&oes->tx_errors, &nes->tx_errors);
> +	i40evf_stat_update_32(&oes->tx_discards, &nes->tx_discards); }
> +
>  static int
>  i40evf_get_statistics(struct rte_eth_dev *dev, struct rte_eth_stats *stats)  {
>  	int ret;
>  	struct i40e_eth_stats *pstats = NULL;
> +	struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data-
> >dev_private);
> +	struct i40e_vsi *vsi = &vf->vsi;
> 
>  	ret = i40evf_update_stats(dev, &pstats);
>  	if (ret != 0)
>  		return 0;
> 
> +	i40evf_update_vsi_stats(vsi, pstats);
> +

It looks like, with this change, the static gotten by user the incensement from the last query?
If so, I don't think it is our expected.

The names of functions are similar. Could you help to refine the code?
For example,  merge i40evf_dev_stats_get and i40evf_get_statistics to be one function.
Rename i40evf_update_stats like i40evf_query_stats, and chang i40evf_update_vsi_stats
To be i40evf_update_stats? I think it would be clearer, what do you think?

Thanks
Jingjing