[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: Friday, September 22, 2017 5:01 AM
> 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/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.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
> >>> 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] <-----
> >
> > <...>
> >