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

[Zhao1, Wei]

Hi，Ferruh

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Thursday, September 14, 2017 9:31 PM
> To: Zhao1, Wei <wei.zhao1 at intel.com>; dev at dpdk.org
> Cc: Wu, Jingjing <jingjing.wu at intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf
> port
> 
> On 9/1/2017 3:30 AM, Zhao1, Wei wrote:
> > Hi,  Ferruh
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Friday, September 1, 2017 12:54 AM
> >> To: Zhao1, Wei <wei.zhao1 at intel.com>; dev at dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug
> >> in vf port
> >>
> >> 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 at 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?
> >
> > Nonono, this code change is directly related to this patch, if we do
> > not do this code change, the diff_pkts_rx and diff_pkts_tx statistic data will
> be wrong  when the first time after clear xstats command.
> 
> If this testpmd code is only wrong for i40e vf after this patch, perhaps
> something else is wrong? Perhaps we should update i40e vf stats.
> 
> OR, if this code is already wrong, lets move it to its own patch.
> 

A new patch will be commit later.

> >
> >>
> >> <...>
> >>
> >>>  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?
> >
> > To be frankly speaking, this bug is firstly discovered by a big user.
> > This bug only appear after use CLI "clear port xstats 0". So it is not easy to
> detect this bug.
> > After using this fix patch ,the big user who report this issue has feed back it
> work well now.
> > The root cause is not so complicated, when the pf which admin this vf
> > is in kernel state, DPDK can not Give pf the info to clear and update
> > offset command, so vf can only keep record the offset data in DPDK VF
> port locally.
> 
> Please help me understand this.
> 
> 1- The problem you are fixing only seen with Linux PF, with DPDK PF you
> don't see the problem, correct? If so this should be part of commit log.
> 
> 2- As I checked the Linux driver code, it does same thing with DPDK:
> a) in PF side, read from registers
> b) removed vsi->eth_stats_offsets from read values
> c) set vsi->eth_stats
> So vsi->eth_stats should be valid, can you please elaborate the issue with
> Linux PF.
> 
> 3- This patch introduces i40evf_update_vsi_stats(), which removes
> vsi->eth_stats_offset from stats received from PF.
> But for DPDK PF case, the stats received from PF are already removes
> vsi->eth_stats_offset, won't this will be a duplicate, and give wrong
> values for the DPDK PF case ?
> 
> 4- Is VF stats registers, reset on read? I mean the received stats values via
> i40evf_update_stats() are values from previous read, or cumulative values?
> 

This patch only fix vf port clear xstats error, because pf has no such problem.
To understand this patch , you can compare the difference between pf and vf 
Code when pocess clear xstats command. You will find pf has a record scheme when 
Receive clear xstats command. What vf did is the same as pf.

> >
> >
> >>
> >>> +
> >>>  	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...
> 
> After some more digging, "vf->vsi.eth_stats" and "*pstats" should be same,
> i40evf_update_stats() both updates the "vf->vsi.eth_stats" and returns its
> pointer [1], so why this update needed.
> 
> [1]
> i40e_pf_host_process_cmd_get_stats() {
> ...
> 	i40e_pf_host_send_msg_to_vf(vf, VIRTCHNL_OP_GET_STATS,
> 		I40E_SUCCESS,
> 		(uint8_t *)&vf->vsi->eth_stats,
> 		sizeof(vf->vsi->eth_stats));
> ...
> 
> 
> >>
> >> <...>