From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 3E4A41AEF3 for ; Thu, 21 Sep 2017 23:00:53 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 21 Sep 2017 14:00:52 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,426,1500966000"; d="scan'208";a="902700345" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.252.27.101]) ([10.252.27.101]) by FMSMGA003.fm.intel.com with ESMTP; 21 Sep 2017 14:00:51 -0700 From: Ferruh Yigit To: "Zhao1, Wei" , "dev@dpdk.org" Cc: "Wu, Jingjing" References: <20170829022806.68101-1-wei.zhao1@intel.com> <9e09b6bb-1df3-590f-0f9c-9d94606985c5@intel.com> <8c4a5650-946b-0112-358f-346b74d42aba@intel.com> <0df9305d-409c-e137-9a3a-349292c63ecb@intel.com> Message-ID: Date: Thu, 21 Sep 2017 22:00:51 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <0df9305d-409c-e137-9a3a-349292c63ecb@intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 21 Sep 2017 21:00:54 -0000 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 ; dev@dpdk.org >>> Cc: Wu, Jingjing >>> 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 ; 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 >>>>>> >>>>>> --- >>>>>> >>>>>> 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. > > 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 :) > > > 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] <----- > > <...> >