From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 16FB5DE0 for ; Thu, 31 Aug 2017 18:53:58 +0200 (CEST) Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 31 Aug 2017 09:53:56 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,454,1498546800"; d="scan'208";a="144175738" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.57]) ([10.237.220.57]) by orsmga005.jf.intel.com with ESMTP; 31 Aug 2017 09:53:52 -0700 To: Wei Zhao , dev@dpdk.org References: <20170829022806.68101-1-wei.zhao1@intel.com> From: Ferruh Yigit Message-ID: <9e09b6bb-1df3-590f-0f9c-9d94606985c5@intel.com> Date: Thu, 31 Aug 2017 17:53: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: <20170829022806.68101-1-wei.zhao1@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, 31 Aug 2017 16:53:59 -0000 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? <...> > 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? > + > 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... <...>