From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Wei Zhao <wei.zhao1@intel.com>, dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port
Date: Thu, 31 Aug 2017 17:53:51 +0100 [thread overview]
Message-ID: <9e09b6bb-1df3-590f-0f9c-9d94606985c5@intel.com> (raw)
In-Reply-To: <20170829022806.68101-1-wei.zhao1@intel.com>
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?
<...>
> 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...
<...>
next prev parent reply other threads:[~2017-08-31 16:53 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 [this message]
2017-09-01 2:30 ` Zhao1, Wei
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=9e09b6bb-1df3-590f-0f9c-9d94606985c5@intel.com \
--to=ferruh.yigit@intel.com \
--cc=dev@dpdk.org \
--cc=wei.zhao1@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).