Hi,Ferruh > -----Original Message----- > From: Yigit, Ferruh > Sent: Thursday, September 14, 2017 9:31 PM > To: Zhao1, Wei <wei.zh...@intel.com>; dev@dpdk.org > Cc: Wu, Jingjing <jingjing...@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.zh...@intel.com>; dev@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.zh...@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)); > ... > > > >> > >> <...>