Hi, Ferruh > -----Original Message----- > From: Yigit, Ferruh > Sent: Friday, September 22, 2017 5:01 AM > 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/21/2017 7:16 PM, Ferruh Yigit wrote: > > On 9/21/2017 4:11 AM, Zhao1, Wei wrote: > >> 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 > >>> vsi->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. > > > > Hi Wei, > > > > This is not helping much :) Please bare with me and let me try again. > > > > As far as I understand you are baselining stats in VF. > > > > 1) Is this problem only seen with Linux PF? > > > > 2) If this is seen only in Linux PF, is it because of [1]? > > > > 3) DPDK PF already sends the baselined stats, won't this cause > > duplicated baselining for DPDK PF case and give wrong values. > > > > 4) Why below [2] is required, is it because of [1]? > > > > > > [1] > > vf->vsi type is "struct i40e_vsi", and this structure is not binary > > compatible between Linux and DPDK, so you can't use this struct to > > communicate between Linux PF and DPDK VF. > > > > If this is the case, can updating DPDK "struct i40e_vsi" be long term > > fix to this problem? > > Aha!, I got it, this is nothing related to being binary compatible etc.. > > I was thinking PF and VF sharing the vsi information, but no they can't, they > are in two different context. So there is no way VF can update > vsi->offset in PF (unless there is an aq for it), so the baselining done > in PF is useless since PF vsi->offset is always zero. And you need to baseline > again in VF. > > So answer to my questions, this is problem for both PFs and double > baselining is not problem because PF one has no effect. > Please correct me if I am wrong :)
Congratulation to you, you have got the final answer! > > > > > > > Thanks, > > ferruh > > > > <...> > >>>>>> -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; > > > > [2] <----- > > > > <...> > >