From: "Zhao1, Wei" <wei.zhao1@intel.com>
To: "Wu, Jingjing" <jingjing.wu@intel.com>, "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v3 1/3] net/i40e: fix clear xstats bug in vf port
Date: Tue, 19 Sep 2017 03:29:52 +0000 [thread overview]
Message-ID: <A2573D2ACFCADC41BB3BE09C6DE313CA07C6595D@PGSMSX103.gar.corp.intel.com> (raw)
In-Reply-To: <9BB6961774997848B5B42BEC655768F810E76F10@SHSMSX103.ccr.corp.intel.com>
Hi, jinging
> -----Original Message-----
> From: Wu, Jingjing
> Sent: Tuesday, September 19, 2017 10:59 AM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: Zhao1, Wei <wei.zhao1@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v3 1/3] net/i40e: fix clear xstats bug in vf port
>
>
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wei Zhao
> > Sent: Monday, September 18, 2017 2:18 PM
> > To: dev@dpdk.org
> > Cc: Zhao1, Wei <wei.zhao1@intel.com>
> > Subject: [dpdk-dev] [PATCH v3 1/3] net/i40e: fix clear xstats bug in
> > vf port
> >
> > 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.
> >
> > ---
> >
> > changes in v3:
> >
> > remove nic_stats_display protect to a new patch
> > ---
> > drivers/net/i40e/i40e_ethdev_vf.c | 64
> > ++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 63 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/i40e/i40e_ethdev_vf.c
> > b/drivers/net/i40e/i40e_ethdev_vf.c
> > index 38c3adc..806ff9e 100644
> > --- a/drivers/net/i40e/i40e_ethdev_vf.c
> > +++ b/drivers/net/i40e/i40e_ethdev_vf.c
> > @@ -888,16 +888,74 @@ i40evf_update_stats(struct rte_eth_dev *dev,
> > struct i40e_eth_stats **pstats)
> > return 0;
> > }
> >
> > +static void
> > +i40evf_stat_update_48(uint64_t *offset,
> > + uint64_t *stat)
> > +{
> > + if (*stat >= *offset)
> > + *stat = *stat - *offset;
> > + else
> > + *stat = (uint64_t)((*stat +
> > + ((uint64_t)1 << I40E_48_BIT_WIDTH)) - *offset);
> > +
> > + *stat &= I40E_48_BIT_MASK;
> > +}
> > +
> > +static void
> > +i40evf_stat_update_32(uint64_t *offset,
> > + uint64_t *stat)
> > +{
> > + if (*stat >= *offset)
> > + *stat = (uint64_t)(*stat - *offset);
> > + else
> > + *stat = (uint64_t)((*stat +
> > + ((uint64_t)1 << I40E_32_BIT_WIDTH)) - *offset); }
>
> The type of count is 64 bits. Is that correct to use 1 << I40E_32_BIT_WIDTH?
No, this is because the register data is 32 bit width, although we store it use 64 bit in memory.
You can see another function i40e_update_vsi_stats().
>
> > +
> > +static void
> > +i40evf_update_vsi_stats(struct i40e_vsi *vsi,
> > + struct i40e_eth_stats *nes)
> > +{
> > + struct i40e_eth_stats *oes = &vsi->eth_stats_offset;
> > +
> > + i40evf_stat_update_48(&oes->rx_bytes,
> > + &nes->rx_bytes);
> > + i40evf_stat_update_48(&oes->rx_unicast,
> > + &nes->rx_unicast);
> > + i40evf_stat_update_48(&oes->rx_multicast,
> > + &nes->rx_multicast);
> > + i40evf_stat_update_48(&oes->rx_broadcast,
> > + &nes->rx_broadcast);
> > + i40evf_stat_update_32(&oes->rx_discards,
> > + &nes->rx_discards);
> > + i40evf_stat_update_32(&oes->rx_unknown_protocol,
> > + &nes->rx_unknown_protocol);
> > + i40evf_stat_update_48(&oes->tx_bytes,
> > + &nes->tx_bytes);
> > + i40evf_stat_update_48(&oes->tx_unicast,
> > + &nes->tx_unicast);
> > + i40evf_stat_update_48(&oes->tx_multicast,
> > + &nes->tx_multicast);
> > + i40evf_stat_update_48(&oes->tx_broadcast,
> > + &nes->tx_broadcast);
> > + i40evf_stat_update_32(&oes->tx_errors, &nes->tx_errors);
> > + i40evf_stat_update_32(&oes->tx_discards, &nes->tx_discards); }
> > +
> > 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);
> > +
>
> It looks like, with this change, the static gotten by user the incensement from
> the last query?
> If so, I don't think it is our expected.
>
This change only reset after clear command, so the data increase after the reset command.
> The names of functions are similar. Could you help to refine the code?
> For example, merge i40evf_dev_stats_get and i40evf_get_statistics to be
> one function.
> Rename i40evf_update_stats like i40evf_query_stats, and chang
> i40evf_update_vsi_stats To be i40evf_update_stats? I think it would be
> clearer, what do you think?
Ok, I wiil change the name in later version
>
> Thanks
> Jingjing
next prev parent reply other threads:[~2017-09-19 3:29 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-29 2:28 [dpdk-dev] [PATCH v2 1/2] " Wei Zhao
2017-08-31 16:53 ` Ferruh Yigit
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 [this message]
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
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=A2573D2ACFCADC41BB3BE09C6DE313CA07C6595D@PGSMSX103.gar.corp.intel.com \
--to=wei.zhao1@intel.com \
--cc=dev@dpdk.org \
--cc=jingjing.wu@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).