[dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

[dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

[Wei Zhao]

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;
 	prev_pkts_rx[port_id] = stats.ipackets;
 	prev_pkts_tx[port_id] = stats.opackets;
 	mpps_rx = diff_cycles > 0 ?
diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index f6d8293..d7a9c03 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -993,16 +993,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 +
@@ -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;
 }
 
 static int i40evf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
@@ -1049,6 +1107,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;
@@ -1060,6 +1120,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;
-- 
2.9.3

Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

[Ferruh Yigit]

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...

<...>

Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

[Zhao1, Wei]

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...
> 
> <...>

Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

[Wu, Jingjing]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zhao1, Wei
> Sent: Friday, September 1, 2017 10:30 AM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port
> 
> 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.
>
Yes, the fix will make the error happen, but this is the fix of the clear xstats issue.
You can create a separate patch for it just to make clearer.

<......>
> 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.
> 
That was because i40evf PMD doesn't support the ops "stats_reset", but "xstats_reset"
Is implemented when xstats are introduced in i40vf PMD.
I think you also need to add the ops "stats_reset", testing can cover this basic case then.

Thanks
Jingjing

Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

[Zhao1, Wei]

Hi, jingjing

> -----Original Message-----
> From: Wu, Jingjing
> Sent: Saturday, September 9, 2017 11:16 AM
> To: Zhao1, Wei <wei.zhao1@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zhao1, Wei
> > Sent: Friday, September 1, 2017 10:30 AM
> > To: Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug
> > in vf port
> >
> > 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.
> >
> Yes, the fix will make the error happen, but this is the fix of the clear xstats
> issue.
> You can create a separate patch for it just to make clearer.

OK, I will create a separate patch for it later.

> 
> <......>
> > 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.
> >
> That was because i40evf PMD doesn't support the ops "stats_reset", but
> "xstats_reset"
> Is implemented when xstats are introduced in i40vf PMD.
> I think you also need to add the ops "stats_reset", testing can cover this basic
> case then.


Yes , I have create a new patch for add support of reset stats in vf port.
http://dpdk.org/dev/patchwork/patch/28046/
> 
> Thanks
> Jingjing

Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

[Ferruh Yigit]

On 9/1/2017 3:30 AM, Zhao1, Wei wrote:
> 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.

If this testpmd code is only wrong for i40e vf after this patch, perhaps
something else is wrong? Perhaps we should update i40e vf stats.

OR, if this code is already wrong, lets move it to its own 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?
> 
> 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.

Please help me understand this.

1- The problem you are fixing only seen with Linux PF, with DPDK PF you
don't see the problem, correct? If so this should be part of commit log.

2- As I checked the Linux driver code, it does same thing with DPDK:
a) in PF side, read from registers
b) removed vsi->eth_stats_offsets from read values
c) set vsi->eth_stats
So vsi->eth_stats should be valid, can you please elaborate the issue
with Linux PF.

3- This patch introduces i40evf_update_vsi_stats(), which removes
vsi->eth_stats_offset from stats received from PF.
But for DPDK PF case, the stats received from PF are already removes
vsi->eth_stats_offset, won't this will be a duplicate, and give wrong
values for the DPDK PF case ?

4- Is VF stats registers, reset on read? I mean the received stats
values via i40evf_update_stats() are values from previous read, or
cumulative values?

> 
> 
>>
>>> +
>>>  	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...

After some more digging, "vf->vsi.eth_stats" and "*pstats" should be
same, i40evf_update_stats() both updates the "vf->vsi.eth_stats" and
returns its pointer [1], so why this update needed.

[1]
i40e_pf_host_process_cmd_get_stats() {
...
	i40e_pf_host_send_msg_to_vf(vf, VIRTCHNL_OP_GET_STATS,
		I40E_SUCCESS,
		(uint8_t *)&vf->vsi->eth_stats,
		sizeof(vf->vsi->eth_stats));
...


>>
>> <...>

[dpdk-dev] [PATCH v3 1/3] net/i40e: fix clear xstats bug in vf port

[Wei Zhao]

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);
+}
+
+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;
-- 
2.9.3

[dpdk-dev] [PATCH v3 2/3] net/i40e: add statistics protect for vf clear xstats

[Wei Zhao]

The diff_pkts_rx and diff_pkts_tx statistic data
will be wrong  when the first time after clear
xstats command if there is no protect.

Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
---
 app/test-pmd/config.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index ca83eef..e8e311c 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;
 	prev_pkts_rx[port_id] = stats.ipackets;
 	prev_pkts_tx[port_id] = stats.opackets;
 	mpps_rx = diff_cycles > 0 ?
-- 
2.9.3

[dpdk-dev] [PATCH v3 3/3] net/i40e: add support of reset stats in vf port

[Wei Zhao]

Add a new support of reset stats statics in vf port.

Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
---
 drivers/net/i40e/i40e_ethdev_vf.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index 806ff9e..13c472f 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -198,6 +198,7 @@ static const struct eth_dev_ops i40evf_eth_dev_ops = {
 	.allmulticast_disable = i40evf_dev_allmulticast_disable,
 	.link_update          = i40evf_dev_link_update,
 	.stats_get            = i40evf_dev_stats_get,
+	.stats_reset          = i40evf_dev_xstats_reset,
 	.xstats_get           = i40evf_dev_xstats_get,
 	.xstats_get_names     = i40evf_dev_xstats_get_names,
 	.xstats_reset         = i40evf_dev_xstats_reset,
-- 
2.9.3

Re: [dpdk-dev] [PATCH v3 1/3] net/i40e: fix clear xstats bug in vf port

[Wu, Jingjing]

> -----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

Re: [dpdk-dev] [PATCH v3 1/3] net/i40e: fix clear xstats bug in vf port

[Zhao1, Wei]

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

Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

[Zhao1, Wei]

Hi，Ferruh

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Thursday, September 14, 2017 9:31 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: Wu, Jingjing <jingjing.wu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf
> port
> 
> On 9/1/2017 3:30 AM, Zhao1, Wei wrote:
> > 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.
> 
> If this testpmd code is only wrong for i40e vf after this patch, perhaps
> something else is wrong? Perhaps we should update i40e vf stats.
> 
> OR, if this code is already wrong, lets move it to its own patch.
> 

A new patch will be commit later.

> >
> >>
> >> <...>
> >>
> >>>  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.
> 
> Please help me understand this.
> 
> 1- The problem you are fixing only seen with Linux PF, with DPDK PF you
> don't see the problem, correct? If so this should be part of commit log.
> 
> 2- As I checked the Linux driver code, it does same thing with DPDK:
> a) in PF side, read from registers
> b) removed vsi->eth_stats_offsets from read values
> c) set vsi->eth_stats
> So vsi->eth_stats should be valid, can you please elaborate the issue with
> Linux PF.
> 
> 3- This patch introduces i40evf_update_vsi_stats(), which removes
> vsi->eth_stats_offset from stats received from PF.
> But for DPDK PF case, the stats received from PF are already removes
> vsi->eth_stats_offset, won't this will be a duplicate, and give wrong
> values for the DPDK PF case ?
> 
> 4- Is VF stats registers, reset on read? I mean the received stats values via
> i40evf_update_stats() are values from previous read, or cumulative values?
> 

This patch only fix vf port clear xstats error, because pf has no such problem.
To understand this patch , you can compare the difference between pf and vf 
Code when pocess clear xstats command. You will find pf has a record scheme when 
Receive clear xstats command. What vf did is the same as pf.

> >
> >
> >>
> >>> +
> >>>  	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...
> 
> After some more digging, "vf->vsi.eth_stats" and "*pstats" should be same,
> i40evf_update_stats() both updates the "vf->vsi.eth_stats" and returns its
> pointer [1], so why this update needed.
> 
> [1]
> i40e_pf_host_process_cmd_get_stats() {
> ...
> 	i40e_pf_host_send_msg_to_vf(vf, VIRTCHNL_OP_GET_STATS,
> 		I40E_SUCCESS,
> 		(uint8_t *)&vf->vsi->eth_stats,
> 		sizeof(vf->vsi->eth_stats));
> ...
> 
> 
> >>
> >> <...>

[dpdk-dev] [PATCH v4 1/4] net/i40e: fix clear xstats bug in vf port

[Wei Zhao]

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>
---
 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);
+}
+
+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;
-- 
2.9.3

[dpdk-dev] [PATCH v4 2/4] net/i40e: add statistics protect for vf clear xstats

[Wei Zhao]

The diff_pkts_rx and diff_pkts_tx statistic data
will be wrong  when the first time after clear
xstats command if there is no protect.

Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
---
 app/test-pmd/config.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index ca83eef..e8e311c 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;
 	prev_pkts_rx[port_id] = stats.ipackets;
 	prev_pkts_tx[port_id] = stats.opackets;
 	mpps_rx = diff_cycles > 0 ?
-- 
2.9.3

[dpdk-dev] [PATCH v4 3/4] net/i40e: add support of reset stats in vf port

[Wei Zhao]

Add a new support of reset stats statics in vf port.

Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
---
 drivers/net/i40e/i40e_ethdev_vf.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index 806ff9e..13c472f 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -198,6 +198,7 @@ static const struct eth_dev_ops i40evf_eth_dev_ops = {
 	.allmulticast_disable = i40evf_dev_allmulticast_disable,
 	.link_update          = i40evf_dev_link_update,
 	.stats_get            = i40evf_dev_stats_get,
+	.stats_reset          = i40evf_dev_xstats_reset,
 	.xstats_get           = i40evf_dev_xstats_get,
 	.xstats_get_names     = i40evf_dev_xstats_get_names,
 	.xstats_reset         = i40evf_dev_xstats_reset,
-- 
2.9.3

[dpdk-dev] [PATCH v4 4/4] net/i40e: merge and rename some function

[Wei Zhao]

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.

Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
---
 drivers/net/i40e/i40e_ethdev_vf.c | 56 +++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 32 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index 13c472f..919f8c7 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -864,7 +864,7 @@ i40evf_del_mac_addr(struct rte_eth_dev *dev, uint32_t index)
 }
 
 static int
-i40evf_update_stats(struct rte_eth_dev *dev, struct i40e_eth_stats **pstats)
+i40evf_query_stats(struct rte_eth_dev *dev, struct i40e_eth_stats **pstats)
 {
 	struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
 	struct virtchnl_queue_select q_stats;
@@ -914,7 +914,7 @@ i40evf_stat_update_32(uint64_t *offset,
 }
 
 static void
-i40evf_update_vsi_stats(struct i40e_vsi *vsi,
+i40evf_update_stats(struct i40e_vsi *vsi,
 					struct i40e_eth_stats *nes)
 {
 	struct i40e_eth_stats *oes = &vsi->eth_stats_offset;
@@ -943,32 +943,6 @@ i40evf_update_vsi_stats(struct i40e_vsi *vsi,
 	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 +
-						pstats->tx_unicast;
-	stats->imissed = pstats->rx_discards;
-	stats->oerrors = pstats->tx_errors + pstats->tx_discards;
-	stats->ibytes = pstats->rx_bytes;
-	stats->obytes = pstats->tx_bytes;
-
-	return 0;
-}
-
 static void
 i40evf_dev_xstats_reset(struct rte_eth_dev *dev)
 {
@@ -976,7 +950,7 @@ i40evf_dev_xstats_reset(struct rte_eth_dev *dev)
 	struct i40e_eth_stats *pstats = NULL;
 
 	/* read stat values to clear hardware registers */
-	i40evf_update_stats(dev, &pstats);
+	i40evf_query_stats(dev, &pstats);
 
 	/* set stats offset base on current values */
 	vf->vsi.eth_stats_offset = *pstats;
@@ -1009,14 +983,14 @@ static int i40evf_dev_xstats_get(struct rte_eth_dev *dev,
 	if (n < I40EVF_NB_XSTATS)
 		return I40EVF_NB_XSTATS;
 
-	ret = i40evf_update_stats(dev, &pstats);
+	ret = i40evf_query_stats(dev, &pstats);
 	if (ret != 0)
 		return 0;
 
 	if (!xstats)
 		return 0;
 
-	i40evf_update_vsi_stats(vsi, pstats);
+	i40evf_update_stats(vsi, pstats);
 
 	/* loop over xstats array and values from pstats */
 	for (i = 0; i < I40EVF_NB_XSTATS; i++) {
@@ -2250,8 +2224,26 @@ i40evf_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 static void
 i40evf_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 {
-	if (i40evf_get_statistics(dev, 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_query_stats(dev, &pstats);
+	if (ret == 0) {
+		i40evf_update_stats(vsi, pstats);
+
+		stats->ipackets = pstats->rx_unicast + pstats->rx_multicast +
+						pstats->rx_broadcast;
+		stats->opackets = pstats->tx_broadcast + pstats->tx_multicast +
+						pstats->tx_unicast;
+		stats->imissed = pstats->rx_discards;
+		stats->oerrors = pstats->tx_errors + pstats->tx_discards;
+		stats->ibytes = pstats->rx_bytes;
+		stats->obytes = pstats->tx_bytes;
+	} else {
 		PMD_DRV_LOG(ERR, "Get statistics failed");
+	}
 }
 
 static void
-- 
2.9.3

Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

[Ferruh Yigit]

On 9/21/2017 4:11 AM, Zhao1, Wei wrote:
> Hi，Ferruh
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Thursday, September 14, 2017 9:31 PM
>> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
>> Cc: Wu, Jingjing <jingjing.wu@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf
>> port
>>
>> On 9/1/2017 3:30 AM, Zhao1, Wei wrote:
>>> 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.
>>
>> If this testpmd code is only wrong for i40e vf after this patch, perhaps
>> something else is wrong? Perhaps we should update i40e vf stats.
>>
>> OR, if this code is already wrong, lets move it to its own patch.
>>
> 
> A new patch will be commit later.
> 
>>>
>>>>
>>>> <...>
>>>>
>>>>>  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.
>>
>> Please help me understand this.
>>
>> 1- The problem you are fixing only seen with Linux PF, with DPDK PF you
>> don't see the problem, correct? If so this should be part of commit log.
>>
>> 2- As I checked the Linux driver code, it does same thing with DPDK:
>> a) in PF side, read from registers
>> b) removed vsi->eth_stats_offsets from read values
>> c) set vsi->eth_stats
>> So vsi->eth_stats should be valid, can you please elaborate the issue with
>> Linux PF.
>>
>> 3- This patch introduces i40evf_update_vsi_stats(), which removes
>> vsi->eth_stats_offset from stats received from PF.
>> But for DPDK PF case, the stats received from PF are already removes
>> vsi->eth_stats_offset, won't this will be a duplicate, and give wrong
>> values for the DPDK PF case ?
>>
>> 4- Is VF stats registers, reset on read? I mean the received stats values via
>> i40evf_update_stats() are values from previous read, or cumulative values?
>>
> 
> This patch only fix vf port clear xstats error, because pf has no such problem.
> To understand this patch , you can compare the difference between pf and vf 
> Code when pocess clear xstats command. You will find pf has a record scheme when 
> Receive clear xstats command. What vf did is the same as pf.

Hi Wei,

This is not helping much :) Please bare with me and let me try again.

As far as I understand you are baselining stats in VF.

1) Is this problem only seen with Linux PF?

2) If this is seen only in Linux PF, is it because of [1]?

3) DPDK PF already sends the baselined stats, won't this cause
duplicated baselining for DPDK PF case and give wrong values.

4) Why below [2] is required, is it because of [1]?


[1]
vf->vsi type is "struct i40e_vsi", and this structure is not binary
compatible between Linux and DPDK, so you can't use this struct to
communicate between Linux PF and DPDK VF.

If this is the case, can updating DPDK "struct i40e_vsi" be long term
fix to this problem?


Thanks,
ferruh

<...>
>>>>> -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;

[2] <-----

<...>

Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

[Ferruh Yigit]

On 9/21/2017 7:16 PM, Ferruh Yigit wrote:
> On 9/21/2017 4:11 AM, Zhao1, Wei wrote:
>> Hi，Ferruh
>>
>>> -----Original Message-----
>>> From: Yigit, Ferruh
>>> Sent: Thursday, September 14, 2017 9:31 PM
>>> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
>>> Cc: Wu, Jingjing <jingjing.wu@intel.com>
>>> Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf
>>> port
>>>
>>> On 9/1/2017 3:30 AM, Zhao1, Wei wrote:
>>>> 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.
>>>
>>> If this testpmd code is only wrong for i40e vf after this patch, perhaps
>>> something else is wrong? Perhaps we should update i40e vf stats.
>>>
>>> OR, if this code is already wrong, lets move it to its own patch.
>>>
>>
>> A new patch will be commit later.
>>
>>>>
>>>>>
>>>>> <...>
>>>>>
>>>>>>  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.
>>>
>>> Please help me understand this.
>>>
>>> 1- The problem you are fixing only seen with Linux PF, with DPDK PF you
>>> don't see the problem, correct? If so this should be part of commit log.
>>>
>>> 2- As I checked the Linux driver code, it does same thing with DPDK:
>>> a) in PF side, read from registers
>>> b) removed vsi->eth_stats_offsets from read values
>>> c) set vsi->eth_stats
>>> So vsi->eth_stats should be valid, can you please elaborate the issue with
>>> Linux PF.
>>>
>>> 3- This patch introduces i40evf_update_vsi_stats(), which removes
>>> vsi->eth_stats_offset from stats received from PF.
>>> But for DPDK PF case, the stats received from PF are already removes
>>> vsi->eth_stats_offset, won't this will be a duplicate, and give wrong
>>> values for the DPDK PF case ?
>>>
>>> 4- Is VF stats registers, reset on read? I mean the received stats values via
>>> i40evf_update_stats() are values from previous read, or cumulative values?
>>>
>>
>> This patch only fix vf port clear xstats error, because pf has no such problem.
>> To understand this patch , you can compare the difference between pf and vf 
>> Code when pocess clear xstats command. You will find pf has a record scheme when 
>> Receive clear xstats command. What vf did is the same as pf.
> 
> Hi Wei,
> 
> This is not helping much :) Please bare with me and let me try again.
> 
> As far as I understand you are baselining stats in VF.
> 
> 1) Is this problem only seen with Linux PF?
> 
> 2) If this is seen only in Linux PF, is it because of [1]?
> 
> 3) DPDK PF already sends the baselined stats, won't this cause
> duplicated baselining for DPDK PF case and give wrong values.
> 
> 4) Why below [2] is required, is it because of [1]?
> 
> 
> [1]
> vf->vsi type is "struct i40e_vsi", and this structure is not binary
> compatible between Linux and DPDK, so you can't use this struct to
> communicate between Linux PF and DPDK VF.
> 
> If this is the case, can updating DPDK "struct i40e_vsi" be long term
> fix to this problem?

Aha!, I got it, this is nothing related to being binary compatible etc..

I was thinking PF and VF sharing the vsi information, but no they can't,
they are in two different context. So there is no way VF can update
vsi->offset in PF (unless there is an aq for it), so the baselining done
in PF is useless since PF vsi->offset is always zero. And you need to
baseline again in VF.

So answer to my questions, this is problem for both PFs and double
baselining is not problem because PF one has no effect.
Please correct me if I am wrong :)

> 
> 
> Thanks,
> ferruh
> 
> <...>
>>>>>> -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;
> 
> [2] <-----
> 
> <...>
> 

Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

[Zhao1, Wei]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Friday, September 22, 2017 2:16 AM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: Wu, Jingjing <jingjing.wu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf
> port
> 
> On 9/21/2017 4:11 AM, Zhao1, Wei wrote:
> > Hi，Ferruh
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Thursday, September 14, 2017 9:31 PM
> >> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> >> Cc: Wu, Jingjing <jingjing.wu@intel.com>
> >> Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug
> >> in vf port
> >>
> >> On 9/1/2017 3:30 AM, Zhao1, Wei wrote:
> >>> 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.
> >>
> >> If this testpmd code is only wrong for i40e vf after this patch,
> >> perhaps something else is wrong? Perhaps we should update i40e vf stats.
> >>
> >> OR, if this code is already wrong, lets move it to its own patch.
> >>
> >
> > A new patch will be commit later.
> >
> >>>
> >>>>
> >>>> <...>
> >>>>
> >>>>>  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.
> >>
> >> Please help me understand this.
> >>
> >> 1- The problem you are fixing only seen with Linux PF, with DPDK PF
> >> you don't see the problem, correct? If so this should be part of commit log.
> >>
> >> 2- As I checked the Linux driver code, it does same thing with DPDK:
> >> a) in PF side, read from registers
> >> b) removed vsi->eth_stats_offsets from read values
> >> c) set vsi->eth_stats
> >> So vsi->eth_stats should be valid, can you please elaborate the issue
> >> with Linux PF.
> >>
> >> 3- This patch introduces i40evf_update_vsi_stats(), which removes
> >> vsi->eth_stats_offset from stats received from PF.
> >> But for DPDK PF case, the stats received from PF are already removes
> >> vsi->eth_stats_offset, won't this will be a duplicate, and give wrong
> >> values for the DPDK PF case ?
> >>
> >> 4- Is VF stats registers, reset on read? I mean the received stats
> >> values via
> >> i40evf_update_stats() are values from previous read, or cumulative
> values?
> >>
> >
> > This patch only fix vf port clear xstats error, because pf has no such
> problem.
> > To understand this patch , you can compare the difference between pf
> > and vf Code when pocess clear xstats command. You will find pf has a
> > record scheme when Receive clear xstats command. What vf did is the
> same as pf.
> 
> Hi Wei,
> 
> This is not helping much :) Please bare with me and let me try again.
> 
> As far as I understand you are baselining stats in VF.
> 
> 1) Is this problem only seen with Linux PF?
> 
> 2) If this is seen only in Linux PF, is it because of [1]?
> 
> 3) DPDK PF already sends the baselined stats, won't this cause duplicated
> baselining for DPDK PF case and give wrong values.
> 
> 4) Why below [2] is required, is it because of [1]?
> 

When host pf is in linux, the vf port can not communicate much with him, include offset after this vf clear command, so vf need to record the baseline.
Customer only report this state will has such problem, but I think if  host pf is in DPDK, vf also has such problem.
So, this patch fix both sate error.


> 
> [1]
> vf->vsi type is "struct i40e_vsi", and this structure is not binary
> compatible between Linux and DPDK, so you can't use this struct to
> communicate between Linux PF and DPDK VF.
> 
> If this is the case, can updating DPDK "struct i40e_vsi" be long term fix to this
> problem?
> 

Of course, I can not. What we did is just "record", not communicate.

> 
> Thanks,
> ferruh
> 
> <...>
> >>>>> -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;
> 
> [2] <-----
> 
> <...>

Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

[Zhao1, Wei]

Hi,  Ferruh

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Friday, September 22, 2017 5:01 AM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: Wu, Jingjing <jingjing.wu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf
> port
> 
> On 9/21/2017 7:16 PM, Ferruh Yigit wrote:
> > On 9/21/2017 4:11 AM, Zhao1, Wei wrote:
> >> Hi，Ferruh
> >>
> >>> -----Original Message-----
> >>> From: Yigit, Ferruh
> >>> Sent: Thursday, September 14, 2017 9:31 PM
> >>> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> >>> Cc: Wu, Jingjing <jingjing.wu@intel.com>
> >>> Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats
> >>> bug in vf port
> >>>
> >>> On 9/1/2017 3:30 AM, Zhao1, Wei wrote:
> >>>> 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.
> >>>
> >>> If this testpmd code is only wrong for i40e vf after this patch,
> >>> perhaps something else is wrong? Perhaps we should update i40e vf
> stats.
> >>>
> >>> OR, if this code is already wrong, lets move it to its own patch.
> >>>
> >>
> >> A new patch will be commit later.
> >>
> >>>>
> >>>>>
> >>>>> <...>
> >>>>>
> >>>>>>  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.
> >>>
> >>> Please help me understand this.
> >>>
> >>> 1- The problem you are fixing only seen with Linux PF, with DPDK PF
> >>> you don't see the problem, correct? If so this should be part of commit
> log.
> >>>
> >>> 2- As I checked the Linux driver code, it does same thing with DPDK:
> >>> a) in PF side, read from registers
> >>> b) removed vsi->eth_stats_offsets from read values
> >>> c) set vsi->eth_stats
> >>> So vsi->eth_stats should be valid, can you please elaborate the
> >>> issue with Linux PF.
> >>>
> >>> 3- This patch introduces i40evf_update_vsi_stats(), which removes
> >>> vsi->eth_stats_offset from stats received from PF.
> >>> But for DPDK PF case, the stats received from PF are already removes
> >>> vsi->eth_stats_offset, won't this will be a duplicate, and give
> >>> vsi->wrong
> >>> values for the DPDK PF case ?
> >>>
> >>> 4- Is VF stats registers, reset on read? I mean the received stats
> >>> values via
> >>> i40evf_update_stats() are values from previous read, or cumulative
> values?
> >>>
> >>
> >> This patch only fix vf port clear xstats error, because pf has no such
> problem.
> >> To understand this patch , you can compare the difference between pf
> >> and vf Code when pocess clear xstats command. You will find pf has a
> >> record scheme when Receive clear xstats command. What vf did is the
> same as pf.
> >
> > Hi Wei,
> >
> > This is not helping much :) Please bare with me and let me try again.
> >
> > As far as I understand you are baselining stats in VF.
> >
> > 1) Is this problem only seen with Linux PF?
> >
> > 2) If this is seen only in Linux PF, is it because of [1]?
> >
> > 3) DPDK PF already sends the baselined stats, won't this cause
> > duplicated baselining for DPDK PF case and give wrong values.
> >
> > 4) Why below [2] is required, is it because of [1]?
> >
> >
> > [1]
> > vf->vsi type is "struct i40e_vsi", and this structure is not binary
> > compatible between Linux and DPDK, so you can't use this struct to
> > communicate between Linux PF and DPDK VF.
> >
> > If this is the case, can updating DPDK "struct i40e_vsi" be long term
> > fix to this problem?
> 
> Aha!, I got it, this is nothing related to being binary compatible etc..
> 
> I was thinking PF and VF sharing the vsi information, but no they can't, they
> are in two different context. So there is no way VF can update
> vsi->offset in PF (unless there is an aq for it), so the baselining done
> in PF is useless since PF vsi->offset is always zero. And you need to baseline
> again in VF.
> 
> So answer to my questions, this is problem for both PFs and double
> baselining is not problem because PF one has no effect.
> Please correct me if I am wrong :)

Congratulation to you, you have got the final answer!


> 
> >
> >
> > Thanks,
> > ferruh
> >
> > <...>
> >>>>>> -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;
> >
> > [2] <-----
> >
> > <...>
> >

Re: [dpdk-dev] [PATCH v4 1/4] net/i40e: fix clear xstats bug in vf port

[Ferruh Yigit]

On 9/21/2017 7:32 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>

Series Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [dpdk-dev] [PATCH v4 1/4] net/i40e: fix clear xstats bug in vf port

[Ferruh Yigit]

On 9/22/2017 6:13 PM, Ferruh Yigit wrote:
> On 9/21/2017 7:32 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>
> 
> Series Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Series applied to dpdk-next-net/master, thanks.

[dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

[Wei Zhao]

From: root <root@dpdk4.bj.intel.com>

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;
 	prev_pkts_rx[port_id] = stats.ipackets;
 	prev_pkts_tx[port_id] = stats.opackets;
 	mpps_rx = diff_cycles > 0 ?
diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index f6d8293..d7a9c03 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -993,16 +993,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 +
@@ -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;
 }
 
 static int i40evf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
@@ -1049,6 +1107,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;
@@ -1060,6 +1120,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;
-- 
2.9.3