From: "Zhao1, Wei" <wei.zhao1@intel.com>
To: "Yigit, Ferruh" <ferruh.yigit@intel.com>, "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port
Date: Fri, 1 Sep 2017 02:30:12 +0000 [thread overview]
Message-ID: <A2573D2ACFCADC41BB3BE09C6DE313CA07C60BA2@PGSMSX103.gar.corp.intel.com> (raw)
In-Reply-To: <9e09b6bb-1df3-590f-0f9c-9d94606985c5@intel.com>
Hi, Ferruh
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Friday, September 1, 2017 12:54 AM
> To: Zhao1, Wei <wei.zhao1@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.zhao1@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.
>
> <...>
>
> > 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.
>
> > +
> > 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...
>
> <...>
next prev parent reply other threads:[~2017-09-01 2:30 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-29 2:28 Wei Zhao
2017-08-31 16:53 ` Ferruh Yigit
2017-09-01 2:30 ` Zhao1, Wei [this message]
2017-09-09 3:15 ` Wu, Jingjing
2017-09-11 1:59 ` Zhao1, Wei
2017-09-14 13:30 ` Ferruh Yigit
2017-09-21 3:11 ` Zhao1, Wei
2017-09-21 18:16 ` Ferruh Yigit
2017-09-21 21:00 ` Ferruh Yigit
2017-09-22 7:51 ` Zhao1, Wei
2017-09-22 2:43 ` Zhao1, Wei
2017-09-18 6:18 ` [dpdk-dev] [PATCH v3 1/3] " Wei Zhao
2017-09-18 6:18 ` [dpdk-dev] [PATCH v3 2/3] net/i40e: add statistics protect for vf clear xstats Wei Zhao
2017-09-18 6:18 ` [dpdk-dev] [PATCH v3 3/3] net/i40e: add support of reset stats in vf port Wei Zhao
2017-09-19 2:58 ` [dpdk-dev] [PATCH v3 1/3] net/i40e: fix clear xstats bug " Wu, Jingjing
2017-09-19 3:29 ` Zhao1, Wei
2017-09-21 6:32 ` [dpdk-dev] [PATCH v4 1/4] " Wei Zhao
2017-09-21 6:32 ` [dpdk-dev] [PATCH v4 2/4] net/i40e: add statistics protect for vf clear xstats Wei Zhao
2017-09-21 6:32 ` [dpdk-dev] [PATCH v4 3/4] net/i40e: add support of reset stats in vf port Wei Zhao
2017-09-21 6:32 ` [dpdk-dev] [PATCH v4 4/4] net/i40e: merge and rename some function Wei Zhao
2017-09-22 17:13 ` [dpdk-dev] [PATCH v4 1/4] net/i40e: fix clear xstats bug in vf port Ferruh Yigit
2017-09-22 17:39 ` Ferruh Yigit
-- strict thread matches above, loose matches on Subject: below --
2017-08-29 2:26 [dpdk-dev] [PATCH v2 1/2] " Wei Zhao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=A2573D2ACFCADC41BB3BE09C6DE313CA07C60BA2@PGSMSX103.gar.corp.intel.com \
--to=wei.zhao1@intel.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).