[dpdk-dev,v3,1/3] net/i40e: fix clear xstats bug in vf port
Checks
Commit Message
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(-)
Comments
> -----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?
> +
> +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.
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?
Thanks
Jingjing
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
@@ -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);
+}
+
+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);
+
stats->ipackets = pstats->rx_unicast + pstats->rx_multicast +
pstats->rx_broadcast;
stats->opackets = pstats->tx_broadcast + pstats->tx_multicast +
@@ -920,7 +978,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;
}
static int i40evf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
@@ -944,6 +1002,8 @@ static int i40evf_dev_xstats_get(struct rte_eth_dev *dev,
int ret;
unsigned i;
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;
if (n < I40EVF_NB_XSTATS)
return I40EVF_NB_XSTATS;
@@ -955,6 +1015,8 @@ static int i40evf_dev_xstats_get(struct rte_eth_dev *dev,
if (!xstats)
return 0;
+ i40evf_update_vsi_stats(vsi, pstats);
+
/* loop over xstats array and values from pstats */
for (i = 0; i < I40EVF_NB_XSTATS; i++) {
xstats[i].id = i;