Re: [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 8/29/2017 3:28 AM, Wei Zhao wrote:
> 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@intel.com>
> 
> ---
> 
> Changes in v2:
> 
>  fix patch log check warning.
> ---
>  app/test-pmd/config.c             |  6 ++--
>  drivers/net/i40e/i40e_ethdev_vf.c | 64 ++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 67 insertions(+), 3 deletions(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 3ae3e1c..14131d6 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -203,8 +203,10 @@ nic_stats_display(portid_t port_id)
>  	if (diff_cycles > 0)
>  		diff_cycles = prev_cycles[port_id] - diff_cycles;
>  
> -	diff_pkts_rx = stats.ipackets - prev_pkts_rx[port_id];
> -	diff_pkts_tx = stats.opackets - prev_pkts_tx[port_id];
> +	diff_pkts_rx = (stats.ipackets > prev_pkts_rx[port_id]) ?
> +		(stats.ipackets - prev_pkts_rx[port_id]) : 0;
> +	diff_pkts_tx = (stats.opackets > prev_pkts_tx[port_id]) ?
> +		(stats.opackets - prev_pkts_tx[port_id]) : 0;

I guess this testpmd update is not directly related to this patch, but
to protect testpmd against value overflow? Can this be another patch?

<...>

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

But not having this previously means all VF stats were wrong previously,
not only extended ones, also basic ones. And not not wrong with small
difference, this should give a big difference in the stats.

I am suspicious about this part, because if this is the case, I would
expect this should be detected earlier.

I have not traced the code, but is there any chance that
"eth_stats_offset" has been used by other end of the admin command?

> +
>  	stats->ipackets = pstats->rx_unicast + pstats->rx_multicast +
>  						pstats->rx_broadcast;
>  	stats->opackets = pstats->tx_broadcast + pstats->tx_multicast +
> @@ -1025,7 +1083,7 @@ i40evf_dev_xstats_reset(struct rte_eth_dev *dev)
>  	i40evf_update_stats(dev, &pstats);
>  
>  	/* set stats offset base on current values */
> -	vf->vsi.eth_stats_offset = vf->vsi.eth_stats;
> +	vf->vsi.eth_stats_offset = *pstats;

I can see this is the reason of the defect mentioned in the commit log.
Instead of using newly acquired stats as offset, using old values...

<...>