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 <[email protected]>

---

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

app/test-pmd/config.c +0 -0

index 3ae3e1c..14131d6 100644

-----Original Message-----
From: Yigit, Ferruh
Sent: Friday, September 1, 2017 12:54 AM
To: Zhao1, Wei <[email protected]>; [email protected]
Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

Quoted 27 lines · 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 <[email protected]>

---

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

app/test-pmd/config.c index renamed from app/test-pmd/config.c +0 -0

3ae3e1c..14131d6 100644

app/test-pmd/config.c +0 -0

@@ -203,8 +203,10 @@ nic_stats_display(portid_t port_id)
 	if (diff_cycles > 0)
 		diff_cycles = prev_cycles[port_id] - diff_cycles;
 

app/test-pmd/config.c nic_stats_display(portid_t port_id) · line 209

Quoted 6 lines

app/test-pmd/config.c +4 -2

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

<...>

Quoted 7 lines

static int
i40evf_get_statistics(struct rte_eth_dev *dev, struct rte_eth_stats
*stats) {

app/test-pmd/config.c +1 -0

 	int ret;
 	struct i40e_eth_stats *pstats = NULL;
+	struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data-

dev_private);

app/test-pmd/config.c

Quoted 7 lines

app/test-pmd/config.c +2 -0

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

app/test-pmd/config.c

Quoted 11 lines

app/test-pmd/config.c +2 -2

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

<...>

-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Zhao1, Wei
Sent: Friday, September 1, 2017 10:30 AM
To: Yigit, Ferruh <[email protected]>; [email protected]
Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

Hi, Ferruh

Quoted 35 lines

-----Original Message-----
From: Yigit, Ferruh
Sent: Friday, September 1, 2017 12:54 AM
To: Zhao1, Wei <[email protected]>; [email protected]
Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

Quoted 27 lines · 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 <[email protected]>

---

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

app/test-pmd/config.c index renamed from app/test-pmd/config.c +0 -0

3ae3e1c..14131d6 100644

app/test-pmd/config.c +0 -0

@@ -203,8 +203,10 @@ nic_stats_display(portid_t port_id)
 	if (diff_cycles > 0)
 		diff_cycles = prev_cycles[port_id] - diff_cycles;
 

Quoted 9 lines

app/test-pmd/config.c nic_stats_display(portid_t port_id) · line 209

Quoted 6 lines

app/test-pmd/config.c +4 -2

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

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.

-----Original Message-----
From: Wu, Jingjing
Sent: Saturday, September 9, 2017 11:16 AM
To: Zhao1, Wei <[email protected]>; Yigit, Ferruh <[email protected]>; [email protected]
Subject: RE: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

Quoted 54 lines

-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Zhao1, Wei
Sent: Friday, September 1, 2017 10:30 AM
To: Yigit, Ferruh <[email protected]>; [email protected]
Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

Hi, Ferruh

Quoted 44 lines

-----Original Message-----
From: Yigit, Ferruh
Sent: Friday, September 1, 2017 12:54 AM
To: Zhao1, Wei <[email protected]>; [email protected]
Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

Quoted 24 lines · 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 <[email protected]>

---

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

app/test-pmd/config.c index renamed from app/test-pmd/config.c +0 -0

3ae3e1c..14131d6 100644

app/test-pmd/config.c nic_stats_display(portid_t port_id) · line 209

Quoted 9 lines

app/test-pmd/config.c +4 -2

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

Quoted 4 lines

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.

<......>

Quoted 9 lines

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

Hi, Ferruh

Quoted 35 lines

-----Original Message-----
From: Yigit, Ferruh
Sent: Friday, September 1, 2017 12:54 AM
To: Zhao1, Wei <[email protected]>; [email protected]
Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

Quoted 27 lines · 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 <[email protected]>

---

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

app/test-pmd/config.c index renamed from app/test-pmd/config.c +0 -0

3ae3e1c..14131d6 100644

app/test-pmd/config.c +0 -0

@@ -203,8 +203,10 @@ nic_stats_display(portid_t port_id)
 	if (diff_cycles > 0)
 		diff_cycles = prev_cycles[port_id] - diff_cycles;
 

Quoted 9 lines

app/test-pmd/config.c nic_stats_display(portid_t port_id) · line 209

Quoted 6 lines

app/test-pmd/config.c +4 -2

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

Quoted 23 lines

<...>

Quoted 7 lines

static int
i40evf_get_statistics(struct rte_eth_dev *dev, struct rte_eth_stats
*stats) {

app/test-pmd/config.c +1 -0

 	int ret;
 	struct i40e_eth_stats *pstats = NULL;
+	struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data-

dev_private);

app/test-pmd/config.c

Quoted 7 lines

app/test-pmd/config.c +2 -0

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

Quoted 5 lines

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.

Quoted 10 lines

Quoted 10 lines

app/test-pmd/config.c +1 -1

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

Quoted 6 lines

app/test-pmd/config.c

Quoted 3 lines

app/test-pmd/config.c +1 -1

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

Quoted 2 lines

<...>

-----Original Message-----
From: Yigit, Ferruh
Sent: Thursday, September 14, 2017 9:31 PM
To: Zhao1, Wei <[email protected]>; [email protected]
Cc: Wu, Jingjing <[email protected]>
Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

Quoted 47 lines · Wei

Hi, Ferruh

Quoted 44 lines

-----Original Message-----
From: Yigit, Ferruh
Sent: Friday, September 1, 2017 12:54 AM
To: Zhao1, Wei <[email protected]>; [email protected]
Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

Quoted 24 lines · 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 <[email protected]>

---

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

app/test-pmd/config.c index renamed from app/test-pmd/config.c +0 -0

3ae3e1c..14131d6 100644

app/test-pmd/config.c nic_stats_display(portid_t port_id) · line 209

Quoted 9 lines

app/test-pmd/config.c +4 -2

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

Quoted 2 lines

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.

Quoted 23 lines

Quoted 23 lines

<...>

Quoted 7 lines

static int
i40evf_get_statistics(struct rte_eth_dev *dev, struct rte_eth_stats
*stats) {

app/test-pmd/config.c +1 -0

 	int ret;
 	struct i40e_eth_stats *pstats = NULL;
+	struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data-

dev_private);

app/test-pmd/config.c

Quoted 7 lines

app/test-pmd/config.c +2 -0

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

Quoted 4 lines

Quoted 4 lines

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"

Quoted 4 lines

Quoted 1 line

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.

Quoted 1 line

After using this fix patch ,the big user who report this issue has feed back it

work well now.

Quoted 3 lines

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?

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

app/test-pmd/config.c +1 -1

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

Quoted 6 lines

Quoted 6 lines

app/test-pmd/config.c

Quoted 3 lines

app/test-pmd/config.c +1 -1

 	/* 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));

...

Quoted 2 lines

Quoted 2 lines

<...>

Hi，Ferruh

Quoted 56 lines

-----Original Message-----
From: Yigit, Ferruh
Sent: Thursday, September 14, 2017 9:31 PM
To: Zhao1, Wei <[email protected]>; [email protected]
Cc: Wu, Jingjing <[email protected]>
Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

Quoted 47 lines · Wei

Hi, Ferruh

Quoted 44 lines

-----Original Message-----
From: Yigit, Ferruh
Sent: Friday, September 1, 2017 12:54 AM
To: Zhao1, Wei <[email protected]>; [email protected]
Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

Quoted 24 lines · 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 <[email protected]>

---

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

app/test-pmd/config.c index renamed from app/test-pmd/config.c +0 -0

3ae3e1c..14131d6 100644

app/test-pmd/config.c nic_stats_display(portid_t port_id) · line 209

Quoted 9 lines

app/test-pmd/config.c +4 -2

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

Quoted 9 lines

Quoted 2 lines

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.

Quoted 58 lines

Quoted 23 lines

Quoted 23 lines

<...>

Quoted 7 lines

static int
i40evf_get_statistics(struct rte_eth_dev *dev, struct rte_eth_stats
*stats) {

app/test-pmd/config.c +1 -0

 	int ret;
 	struct i40e_eth_stats *pstats = NULL;
+	struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data-

dev_private);

app/test-pmd/config.c

Quoted 7 lines

app/test-pmd/config.c +2 -0

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

Quoted 4 lines

Quoted 4 lines

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"

Quoted 4 lines

Quoted 1 line

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.

Quoted 1 line

After using this fix patch ,the big user who report this issue has feed back it

work well now.

Quoted 3 lines

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 ?

Quoted 3 lines

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.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

app/test-pmd/config.c

Quoted 6 lines

-1025,7 +1083,7 @@ i40evf_dev_xstats_reset(struct rte_eth_dev *dev)

app/test-pmd/config.c +1 -1

 	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;

Quoted 128 lines · Wei

Hi，Ferruh

Quoted 56 lines

-----Original Message-----
From: Yigit, Ferruh
Sent: Thursday, September 14, 2017 9:31 PM
To: Zhao1, Wei <[email protected]>; [email protected]
Cc: Wu, Jingjing <[email protected]>
Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

Quoted 47 lines · Wei

Hi, Ferruh

Quoted 44 lines

-----Original Message-----
From: Yigit, Ferruh
Sent: Friday, September 1, 2017 12:54 AM
To: Zhao1, Wei <[email protected]>; [email protected]
Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

Quoted 24 lines · 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 <[email protected]>

---

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

app/test-pmd/config.c index renamed from app/test-pmd/config.c +0 -0

3ae3e1c..14131d6 100644

app/test-pmd/config.c nic_stats_display(portid_t port_id) · line 209

Quoted 9 lines

app/test-pmd/config.c +4 -2

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

Quoted 9 lines

Quoted 2 lines

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.

Quoted 58 lines

Quoted 23 lines

Quoted 23 lines

<...>

Quoted 7 lines

static int
i40evf_get_statistics(struct rte_eth_dev *dev, struct rte_eth_stats
*stats) {

app/test-pmd/config.c +1 -0

 	int ret;
 	struct i40e_eth_stats *pstats = NULL;
+	struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data-

dev_private);

app/test-pmd/config.c

Quoted 7 lines

app/test-pmd/config.c +2 -0

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

Quoted 4 lines

Quoted 4 lines

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"

Quoted 4 lines

Quoted 1 line

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.

Quoted 1 line

After using this fix patch ,the big user who report this issue has feed back it

work well now.

Quoted 3 lines

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 ?

Quoted 8 lines

Quoted 3 lines

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

<...>

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

app/test-pmd/config.c

Quoted 6 lines

-1025,7 +1083,7 @@ i40evf_dev_xstats_reset(struct rte_eth_dev *dev)

app/test-pmd/config.c +1 -1

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

<...>

-----Original Message-----
From: Yigit, Ferruh
Sent: Friday, September 22, 2017 5:01 AM
To: Zhao1, Wei <[email protected]>; [email protected]
Cc: Wu, Jingjing <[email protected]>
Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

Quoted 49 lines · Ferruh Yigit

Quoted 48 lines · Wei

Hi，Ferruh

Quoted 46 lines

-----Original Message-----
From: Yigit, Ferruh
Sent: Thursday, September 14, 2017 9:31 PM
To: Zhao1, Wei <[email protected]>; [email protected]
Cc: Wu, Jingjing <[email protected]>
Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

Quoted 37 lines · Wei

Hi, Ferruh

Quoted 35 lines

-----Original Message-----
From: Yigit, Ferruh
Sent: Friday, September 1, 2017 12:54 AM
To: Zhao1, Wei <[email protected]>; [email protected]
Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

Quoted 27 lines · 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 <[email protected]>

---

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

app/test-pmd/config.c index renamed from app/test-pmd/config.c +0 -0

3ae3e1c..14131d6 100644

app/test-pmd/config.c +0 -0

@@ -203,8 +203,10 @@ nic_stats_display(portid_t port_id)
 	if (diff_cycles > 0)
 		diff_cycles = prev_cycles[port_id] - diff_cycles;
 

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

app/test-pmd/config.c nic_stats_display(portid_t port_id) · line 209

Quoted 6 lines

app/test-pmd/config.c +4 -2

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

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

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.

Quoted 10 lines

Quoted 10 lines

Quoted 3 lines

OR, if this code is already wrong, lets move it to its own patch.

A new patch will be commit later.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

<...>

Quoted 20 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

app/test-pmd/config.c

Quoted 7 lines

static int
i40evf_get_statistics(struct rte_eth_dev *dev, struct
rte_eth_stats
*stats) {

app/test-pmd/config.c +1 -0

 	int ret;
 	struct i40e_eth_stats *pstats = NULL;
+	struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev-

data-

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

app/test-pmd/config.c

Quoted 8 lines

dev_private);

app/test-pmd/config.c +2 -0

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

Quoted 19 lines

Quoted 19 lines

Quoted 19 lines

Quoted 5 lines

Quoted 5 lines

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"

Quoted 5 lines

Quoted 1 line

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.

Quoted 2 lines

After using this fix patch ,the big user who report this issue has feed back it

work well now.

Quoted 4 lines

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

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

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.

Quoted 15 lines

Quoted 15 lines

Quoted 15 lines

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 ?

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

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?

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

This patch only fix vf port clear xstats error, because pf has no such

problem.

Quoted 3 lines

Quoted 3 lines

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.

Quoted 17 lines

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

Quoted 7 lines

[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 :)

Quoted 8 lines

Thanks,
ferruh

<...>

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

-1025,7 +1083,7 @@ i40evf_dev_xstats_reset(struct rte_eth_dev

*dev)

Quoted 10 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

app/test-pmd/config.c

Quoted 5 lines

app/test-pmd/config.c +1 -1

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

<...>

-----Original Message-----
From: Yigit, Ferruh
Sent: Friday, September 22, 2017 2:16 AM
To: Zhao1, Wei <[email protected]>; [email protected]
Cc: Wu, Jingjing <[email protected]>
Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

Quoted 57 lines · Wei

Hi，Ferruh

Quoted 55 lines

-----Original Message-----
From: Yigit, Ferruh
Sent: Thursday, September 14, 2017 9:31 PM
To: Zhao1, Wei <[email protected]>; [email protected]
Cc: Wu, Jingjing <[email protected]>
Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

Quoted 46 lines · Wei

Hi, Ferruh

Quoted 44 lines

-----Original Message-----
From: Yigit, Ferruh
Sent: Friday, September 1, 2017 12:54 AM
To: Zhao1, Wei <[email protected]>; [email protected]
Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port

Quoted 24 lines · 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 <[email protected]>

---

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

app/test-pmd/config.c index renamed from app/test-pmd/config.c +0 -0

3ae3e1c..14131d6 100644

app/test-pmd/config.c nic_stats_display(portid_t port_id) · line 209

Quoted 9 lines

app/test-pmd/config.c +4 -2

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

Quoted 18 lines

Quoted 11 lines

Quoted 4 lines

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.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

<...>

Quoted 20 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

app/test-pmd/config.c

Quoted 7 lines

static int
i40evf_get_statistics(struct rte_eth_dev *dev, struct
rte_eth_stats
*stats) {

app/test-pmd/config.c +1 -0

 	int ret;
 	struct i40e_eth_stats *pstats = NULL;
+	struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev-

data-

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

app/test-pmd/config.c

Quoted 8 lines

dev_private);

app/test-pmd/config.c +2 -0

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

Quoted 39 lines

Quoted 39 lines

Quoted 5 lines

Quoted 5 lines

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"

Quoted 5 lines

Quoted 1 line

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.

Quoted 2 lines

After using this fix patch ,the big user who report this issue has feed back it

work well now.

Quoted 4 lines

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 ?

Quoted 3 lines

Quoted 3 lines

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?

Quoted 3 lines

Quoted 1 line

This patch only fix vf port clear xstats error, because pf has no such

problem.

Quoted 3 lines

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

<...>

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

-1025,7 +1083,7 @@ i40evf_dev_xstats_reset(struct rte_eth_dev

*dev)

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

app/test-pmd/config.c

Quoted 5 lines

app/test-pmd/config.c +1 -1

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

<...>

-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Wei Zhao
Sent: Monday, September 18, 2017 2:18 PM
To: [email protected]
Cc: Zhao1, Wei <[email protected]>
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 <[email protected]>

---

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

drivers/net/i40e/i40e_ethdev_vf.c +12 -0

index 38c3adc..806ff9e 100644
@@ -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;
+}

drivers/net/i40e/i40e_ethdev_vf.c +30 -0

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

-----Original Message-----
From: Wu, Jingjing
Sent: Tuesday, September 19, 2017 10:59 AM
To: Zhao1, Wei <[email protected]>; [email protected]
Cc: Zhao1, Wei <[email protected]>
Subject: RE: [dpdk-dev] [PATCH v3 1/3] net/i40e: fix clear xstats bug in vf port

Quoted 58 lines

-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Wei Zhao
Sent: Monday, September 18, 2017 2:18 PM
To: [email protected]
Cc: Zhao1, Wei <[email protected]>
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 <[email protected]>

---

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

drivers/net/i40e/i40e_ethdev_vf.c +16 -0

index 38c3adc..806ff9e 100644
@@ -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)

drivers/net/i40e/i40e_ethdev_vf.c i40evf_update_stats(struct rte_eth_dev *dev, struct i40e_eth_stats **pstats)

Quoted 6 lines

drivers/net/i40e/i40e_ethdev_vf.c +6 -0

+{
+	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?

Quoted 23 lines

drivers/net/i40e/i40e_ethdev_vf.c +22 -0

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

drivers/net/i40e/i40e_ethdev_vf.c i40evf_update_stats(struct rte_eth_dev *dev, struct i40e_eth_stats **pstats) · line 946

Quoted 10 lines

drivers/net/i40e/i40e_ethdev_vf.c +8 -0

+			    &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)

{

Quoted 10 lines

drivers/net/i40e/i40e_ethdev_vf.c +2 -0

 	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;
 

drivers/net/i40e/i40e_ethdev_vf.c i40evf_update_stats(struct rte_eth_dev *dev, · line 957

Quoted 2 lines

drivers/net/i40e/i40e_ethdev_vf.c +2 -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

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 <[email protected]>

Quoted 8 lines · 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 <[email protected]>

Series Reviewed-by: Ferruh Yigit <[email protected]>