net/netvsc: report correct stats values

PATCH In review [email protected] 4 participants 16 messages Mar 24, 2022 – May 10, 2022 Network drivers

lore

Share & tools

lore

[email protected]

author

2022-03-24 17:45 UTC #0 lore raw

PATCH v2 1/1 net/netvsc: report correct stats values

From: Long Li <[email protected]>

The netvsc should add to the values from the VF and report the sum.

Fixes 4e9c73e96e ("net/netvsc: add Hyper-V network device")

Cc [email protected]

Signed-off-by Long Li <[email protected]>

---
 drivers/net/netvsc/hn_ethdev.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

drivers/net/netvsc/hn_ethdev.c +5 -5

index 0a357d3645..a6202d898b 100644
@@ -804,8 +804,8 @@ static int hn_dev_stats_get(struct rte_eth_dev *dev,
 		stats->oerrors += txq->stats.errors;
 
 		if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
-			stats->q_opackets[i] = txq->stats.packets;
-			stats->q_obytes[i] = txq->stats.bytes;
+			stats->q_opackets[i] += txq->stats.packets;
+			stats->q_obytes[i] += txq->stats.bytes;
 		}
 	}

@@ -821,12 +821,12 @@ static int hn_dev_stats_get(struct rte_eth_dev *dev,
 		stats->imissed += rxq->stats.ring_full;
 
 		if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
-			stats->q_ipackets[i] = rxq->stats.packets;
-			stats->q_ibytes[i] = rxq->stats.bytes;
+			stats->q_ipackets[i] += rxq->stats.packets;
+			stats->q_ibytes[i] += rxq->stats.bytes;
 		}
 	}
 
-	stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed;
+	stats->rx_nombuf += dev->data->rx_mbuf_alloc_failed;
 	return 0;
 }

Signature · 1 line

2.32.0

Ferruh Yigit

2022-04-26 21:56 UTC #1 lore raw

Quoted 4 lines · On 3/24/2022 5:45 PM, [email protected] wrote:

From: Long Li <[email protected]>
The netvsc should add to the values from the VF and report the sum.

Per port stats already accumulated, like:
'stats->opackets += txq->stats.packets;'

Quoted 12 lines

Fixes 4e9c73e96e ("net/netvsc: add Hyper-V network device")
Cc [email protected]
Signed-off-by Long Li <[email protected]>
---
  drivers/net/netvsc/hn_ethdev.c | 10 +++++-----
  1 file changed, 5 insertions(+), 5 deletions(-)
drivers/net/netvsc/hn_ethdev.c +0 -0
index 0a357d3645..a6202d898b 100644

drivers/net/netvsc/hn_ethdev.c static int hn_dev_stats_get(struct rte_eth_dev *dev, · line 808

Quoted 7 lines

drivers/net/netvsc/hn_ethdev.c +2 -2

@@ -804,8 +804,8 @@ static int hn_dev_stats_get(struct rte_eth_dev *dev,
  		stats->oerrors += txq->stats.errors;
 
  		if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
-			stats->q_opackets[i] = txq->stats.packets;
-			stats->q_obytes[i] = txq->stats.bytes;
+			stats->q_opackets[i] += txq->stats.packets;
+			stats->q_obytes[i] += txq->stats.bytes;

This is per queue stats, 'stats->q_opackets[i]', in next iteration of the loop, 'i' will be increased and 'txq' will be updated, so as far as I can see the above change has no affect.

Quoted 6 lines

drivers/net/netvsc/hn_ethdev.c +0 -0

  		}
  	}

@@ -821,12 +821,12 @@ static int hn_dev_stats_get(struct rte_eth_dev *dev,
  		stats->imissed += rxq->stats.ring_full;

drivers/net/netvsc/hn_ethdev.c static int hn_dev_stats_get(struct rte_eth_dev *dev, · line 829

Quoted 10 lines

drivers/net/netvsc/hn_ethdev.c +3 -3

  		if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
-			stats->q_ipackets[i] = rxq->stats.packets;
-			stats->q_ibytes[i] = rxq->stats.bytes;
+			stats->q_ipackets[i] += rxq->stats.packets;
+			stats->q_ibytes[i] += rxq->stats.bytes;
  		}
  	}
 
-	stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed;
+	stats->rx_nombuf += dev->data->rx_mbuf_alloc_failed;

Why '+='? Is 'dev->data->rx_mbuf_alloc_failed' reset somewhere between two consecutive stats get call?

Anyway, above line has no affect, since the 'stats->rx_nombuf' is overwritten by 'rte_eth_stats_get()'. So above line can be removed.

Quoted 3 lines

drivers/net/netvsc/hn_ethdev.c +0 -0
  	return 0;
  }
 

Stephen Hemminger

2022-04-26 22:45 UTC #2 lore raw

Quoted 9 lines · On Tue, 26 Apr 2022 22:56:14 +0100 Ferruh Yigit <[email protected]> wrote:

drivers/net/netvsc/hn_ethdev.c static int hn_dev_stats_get(struct rte_eth_dev *dev, · line 808
Quoted 5 lines
  		if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
drivers/net/netvsc/hn_ethdev.c +2 -2
-			stats->q_opackets[i] = txq->stats.packets;
-			stats->q_obytes[i] = txq->stats.bytes;
+			stats->q_opackets[i] += txq->stats.packets;
+			stats->q_obytes[i] += txq->stats.bytes;  
This is per queue stats, 'stats->q_opackets[i]', in next iteration of the loop, 'i' will be increased and 'txq' will be updated, so as far as I can see the above change has no affect.

Agree, that is why it was just assignment originally.

Long Li

2022-05-03 18:18 UTC #3 lore raw

Quoted 15 lines

Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 9 lines · On Tue, 26 Apr 2022 22:56:14 +0100 Ferruh Yigit <[email protected]> wrote:
drivers/net/netvsc/hn_ethdev.c static int hn_dev_stats_get(struct rte_eth_dev *dev, · line 808
Quoted 5 lines
  		if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
drivers/net/netvsc/hn_ethdev.c +2 -2
-			stats->q_opackets[i] = txq->stats.packets;
-			stats->q_obytes[i] = txq->stats.bytes;
+			stats->q_opackets[i] += txq->stats.packets;
+			stats->q_obytes[i] += txq->stats.bytes;
This is per queue stats, 'stats->q_opackets[i]', in next iteration of the loop, 'i' will be increased and 'txq' will be updated, so as far as I can see the above change has no affect.

Quoted 1 line

Agree, that is why it was just assignment originally.

The condition here is a little different. NETVSC is a master device with another PMD running as a slave. When reporting stats values, it needs to add the values from the slave PMD. The original code just overwrites the values from its slave PMD.

Ferruh Yigit

2022-05-03 19:04 UTC #4 lore raw

Quoted 15 lines · Long Li

Quoted 15 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 9 lines · On Tue, 26 Apr 2022 22:56:14 +0100 Ferruh Yigit <[email protected]> wrote:
drivers/net/netvsc/hn_ethdev.c static int hn_dev_stats_get(struct rte_eth_dev *dev, · line 808
Quoted 5 lines
   		if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
drivers/net/netvsc/hn_ethdev.c +2 -2
-			stats->q_opackets[i] = txq->stats.packets;
-			stats->q_obytes[i] = txq->stats.bytes;
+			stats->q_opackets[i] += txq->stats.packets;
+			stats->q_obytes[i] += txq->stats.bytes;
This is per queue stats, 'stats->q_opackets[i]', in next iteration of the loop, 'i' will be increased and 'txq' will be updated, so as far as I can see the above change has no affect.

Quoted 3 lines

Quoted 1 line
Agree, that is why it was just assignment originally.
The condition here is a little different. NETVSC is a master device with another PMD running as a slave. When reporting stats values, it needs to add the values from the slave PMD. The original code just overwrites the values from its slave PMD.

Where the initial values are coming from, 'hn_vf_stats_get()'?

If 'hn_vf_stats_get()' fills the stats, what are the values kept in 'txq->stats.*' in above updated loop?

Long Li

2022-05-03 19:14 UTC #5 lore raw

Quoted 21 lines

Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 15 lines · Long Li
Quoted 15 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 9 lines · On Tue, 26 Apr 2022 22:56:14 +0100 Ferruh Yigit <[email protected]> wrote:
drivers/net/netvsc/hn_ethdev.c static int hn_dev_stats_get(struct rte_eth_dev *dev, · line 808
Quoted 5 lines
   		if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
drivers/net/netvsc/hn_ethdev.c +2 -2
-			stats->q_opackets[i] = txq->stats.packets;
-			stats->q_obytes[i] = txq->stats.bytes;
+			stats->q_opackets[i] += txq->stats.packets;
+			stats->q_obytes[i] += txq->stats.bytes;
This is per queue stats, 'stats->q_opackets[i]', in next iteration of the loop, 'i' will be increased and 'txq' will be updated, so as far as I can see the above change has no affect.
Quoted 3 lines
Quoted 1 line
Agree, that is why it was just assignment originally.
The condition here is a little different. NETVSC is a master device with

Quoted 8 lines

another PMD running as a slave. When reporting stats values, it needs to add the values from the slave PMD. The original code just overwrites the values from its slave PMD.
Where the initial values are coming from, 'hn_vf_stats_get()'?
If 'hn_vf_stats_get()' fills the stats, what are the values kept in 'txq->stats.*' in above updated loop?

Yes, hn_vf_stats_get() fills in the stats from the slave PMD. txq->stats values are from the master PMD. Those values are different and accounted separated from the values from the slave PMD.

Ferruh Yigit

2022-05-03 19:55 UTC #6 lore raw

Quoted 25 lines · Long Li

Quoted 25 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 15 lines · Long Li
Quoted 15 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 9 lines · On Tue, 26 Apr 2022 22:56:14 +0100 Ferruh Yigit <[email protected]> wrote:
drivers/net/netvsc/hn_ethdev.c static int hn_dev_stats_get(struct rte_eth_dev *dev, · line 808
Quoted 5 lines
    		if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
drivers/net/netvsc/hn_ethdev.c +2 -2
-			stats->q_opackets[i] = txq->stats.packets;
-			stats->q_obytes[i] = txq->stats.bytes;
+			stats->q_opackets[i] += txq->stats.packets;
+			stats->q_obytes[i] += txq->stats.bytes;
This is per queue stats, 'stats->q_opackets[i]', in next iteration of the loop, 'i' will be increased and 'txq' will be updated, so as far as I can see the above change has no affect.
Quoted 3 lines
Quoted 1 line
Agree, that is why it was just assignment originally.
The condition here is a little different. NETVSC is a master device with
another PMD running as a slave. When reporting stats values, it needs to add the values from the slave PMD. The original code just overwrites the values from its slave PMD.

Quoted 6 lines

Quoted 4 lines
Where the initial values are coming from, 'hn_vf_stats_get()'?
If 'hn_vf_stats_get()' fills the stats, what are the values kept in 'txq->stats.*' in above updated loop?
Yes, hn_vf_stats_get() fills in the stats from the slave PMD. txq->stats values are from the master PMD. Those values are different and accounted separated from the values from the slave PMD.

I see, since this is a little different than what most of the PMDs do, can you please put a little more info to the commit log? Or perhaps can add some comments to the code.

And still 'stats->rx_nombuf' change is not required right? If so can you remove it in the next version?

Long Li

2022-05-03 20:48 UTC #7 lore raw

Quoted 35 lines

Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 32 lines · Long Li
Quoted 22 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 15 lines · Long Li
Quoted 15 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 9 lines · On Tue, 26 Apr 2022 22:56:14 +0100 Ferruh Yigit <[email protected]> wrote:
Review
Quoted 5 lines
    		if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
(quoted diff) +2 -2
-			stats->q_opackets[i] = txq->stats.packets;
-			stats->q_obytes[i] = txq->stats.bytes;
+			stats->q_opackets[i] += txq->stats.packets;
+			stats->q_obytes[i] += txq->stats.bytes;
This is per queue stats, 'stats->q_opackets[i]', in next iteration of the loop, 'i' will be increased and 'txq' will be updated, so as far as I can see the above change has no affect.
Quoted 4 lines
Quoted 1 line
Agree, that is why it was just assignment originally.
The condition here is a little different. NETVSC is a master device with
Quoted 7 lines
another PMD running as a slave. When reporting stats values, it needs to add the values from the slave PMD. The original code just overwrites the values from its slave PMD.
Where the initial values are coming from, 'hn_vf_stats_get()'?
If 'hn_vf_stats_get()' fills the stats, what are the values kept in 'txq-
stats.*'
Quoted 1 line
in above updated loop?

Quoted 7 lines

Quoted 1 line
Yes, hn_vf_stats_get() fills in the stats from the slave PMD. txq->stats
values are from the master PMD. Those values are different and accounted separated from the values from the slave PMD.
I see, since this is a little different than what most of the PMDs do, can you please put a little more info to the commit log? Or perhaps can add some comments to the code.

Ok, will do.

Quoted 3 lines

And still 'stats->rx_nombuf' change is not required right? If so can you remove it in the next version?

It is still needed. NETVSC unconditionally calls the slave PMD to receive packets, even if it can't allocate a mbuf to receive a synthetic packet itself. The accounting of rx_nombuf is valid because the synthetic packets (to NETVSC) and VF packets (to slave PMD) are routed separately from Hyper-V.

Ferruh Yigit

2022-05-04 12:33 UTC #8 lore raw

Quoted 43 lines · Long Li

Quoted 35 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 32 lines · Long Li
Quoted 22 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 15 lines · Long Li
Quoted 15 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 9 lines · On Tue, 26 Apr 2022 22:56:14 +0100 Ferruh Yigit <[email protected]> wrote:
Review
Quoted 5 lines
     		if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
(quoted diff) +2 -2
-			stats->q_opackets[i] = txq->stats.packets;
-			stats->q_obytes[i] = txq->stats.bytes;
+			stats->q_opackets[i] += txq->stats.packets;
+			stats->q_obytes[i] += txq->stats.bytes;
This is per queue stats, 'stats->q_opackets[i]', in next iteration of the loop, 'i' will be increased and 'txq' will be updated, so as far as I can see the above change has no affect.
Quoted 4 lines
Quoted 1 line
Agree, that is why it was just assignment originally.
The condition here is a little different. NETVSC is a master device with
Quoted 7 lines
another PMD running as a slave. When reporting stats values, it needs to add the values from the slave PMD. The original code just overwrites the values from its slave PMD.
Where the initial values are coming from, 'hn_vf_stats_get()'?
If 'hn_vf_stats_get()' fills the stats, what are the values kept in 'txq-
stats.*'
Quoted 1 line
in above updated loop?
Quoted 7 lines
Quoted 1 line
Yes, hn_vf_stats_get() fills in the stats from the slave PMD. txq->stats
values are from the master PMD. Those values are different and accounted separated from the values from the slave PMD.
I see, since this is a little different than what most of the PMDs do, can you please put a little more info to the commit log? Or perhaps can add some comments to the code.

Quoted 7 lines

Ok, will do.
Quoted 3 lines
And still 'stats->rx_nombuf' change is not required right? If so can you remove it in the next version?
It is still needed. NETVSC unconditionally calls the slave PMD to receive packets, even if it can't allocate a mbuf to receive a synthetic packet itself. The accounting of rx_nombuf is valid because the synthetic packets (to NETVSC) and VF packets (to slave PMD) are routed separately from Hyper-V.

I am not referring to the "+=" update, my comment was because 'stats->rx_nombuf' is overwritten in 'rte_eth_stats_get()' [1]. Is it still required?

[1]
https://git.dpdk.org/dpdk/tree/lib/ethdev/rte_ethdev.c?h=v22.03#n2518

Long Li

2022-05-04 18:38 UTC #9 lore raw

Quoted 54 lines

Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 51 lines · Long Li
Quoted 36 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 33 lines · Long Li
Quoted 22 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 15 lines · Long Li
Quoted 15 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 9 lines · On Tue, 26 Apr 2022 22:56:14 +0100 Ferruh Yigit <[email protected]> wrote:
Review
Quoted 5 lines
     		if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
(quoted diff) +2 -2
-			stats->q_opackets[i] = txq->stats.packets;
-			stats->q_obytes[i] = txq->stats.bytes;
+			stats->q_opackets[i] += txq->stats.packets;
+			stats->q_obytes[i] += txq->stats.bytes;
This is per queue stats, 'stats->q_opackets[i]', in next iteration of the loop, 'i' will be increased and 'txq' will be updated, so as far as I can see the above change has no affect.
Quoted 4 lines
Quoted 1 line
Agree, that is why it was just assignment originally.
The condition here is a little different. NETVSC is a master device with
Quoted 8 lines
another PMD running as a slave. When reporting stats values, it needs to add the values from the slave PMD. The original code just overwrites the values from its slave PMD.
Where the initial values are coming from, 'hn_vf_stats_get()'?
If 'hn_vf_stats_get()' fills the stats, what are the values kept in 'txq-
stats.*'
Quoted 1 line
in above updated loop?
Quoted 8 lines
Quoted 2 lines
Yes, hn_vf_stats_get() fills in the stats from the slave PMD. txq->stats
values are from the master PMD. Those values are different and accounted separated from the values from the slave PMD.
I see, since this is a little different than what most of the PMDs do, can you please put a little more info to the commit log? Or perhaps can add some comments to the code.
Ok, will do.
Quoted 3 lines
And still 'stats->rx_nombuf' change is not required right? If so can you remove it in the next version?

Quoted 8 lines

Quoted 1 line
It is still needed. NETVSC unconditionally calls the slave PMD to receive
packets, even if it can't allocate a mbuf to receive a synthetic packet itself. The accounting of rx_nombuf is valid because the synthetic packets (to NETVSC) and VF packets (to slave PMD) are routed separately from Hyper-V.
I am not referring to the "+=" update, my comment was because 'stats-
Quoted 1 line
rx_nombuf' is overwritten in 'rte_eth_stats_get()' [1].
Is it still required?

Yes, it is still needed. NETVSC calls the rte_eth_stats_get() on its slave PMD first, and stats->rx_nombuf is updated (overwritten) for its slave PMD. Afte that, it needs to add to its own dev->data->rx_mbuf_alloc_failed back to stats->rx_nombuf.

Quoted 10 lines

[1]
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.dpdk
.org%2Fdpdk%2Ftree%2Flib%2Fethdev%2Frte_ethdev.c%3Fh%3Dv22.03%23n25 18&data=05%7C01%7Clongli%40microsoft.com%7Cea473df2344c460d575 d08da2dca3e53%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63787 2643902917430%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQ IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sd ata=FZO%2B%2BnWtLGstHHIZ2aXsDUKNI%2Fi9tbj6jONhp174qKw%3D&res erved=0

Ferruh Yigit

2022-05-05 16:29 UTC #10 lore raw

Quoted 59 lines · Long Li

Quoted 59 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 45 lines · Long Li
Quoted 36 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 33 lines · Long Li
Quoted 22 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 15 lines · Long Li
Quoted 15 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 9 lines · On Tue, 26 Apr 2022 22:56:14 +0100 Ferruh Yigit <[email protected]> wrote:
Review
Quoted 5 lines
      		if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
(quoted diff) +2 -2
-			stats->q_opackets[i] = txq->stats.packets;
-			stats->q_obytes[i] = txq->stats.bytes;
+			stats->q_opackets[i] += txq->stats.packets;
+			stats->q_obytes[i] += txq->stats.bytes;
This is per queue stats, 'stats->q_opackets[i]', in next iteration of the loop, 'i' will be increased and 'txq' will be updated, so as far as I can see the above change has no affect.
Quoted 4 lines
Quoted 1 line
Agree, that is why it was just assignment originally.
The condition here is a little different. NETVSC is a master device with
Quoted 8 lines
another PMD running as a slave. When reporting stats values, it needs to add the values from the slave PMD. The original code just overwrites the values from its slave PMD.
Where the initial values are coming from, 'hn_vf_stats_get()'?
If 'hn_vf_stats_get()' fills the stats, what are the values kept in 'txq-
stats.*'
Quoted 1 line
in above updated loop?
Quoted 8 lines
Quoted 2 lines
Yes, hn_vf_stats_get() fills in the stats from the slave PMD. txq->stats
values are from the master PMD. Those values are different and accounted separated from the values from the slave PMD.
I see, since this is a little different than what most of the PMDs do, can you please put a little more info to the commit log? Or perhaps can add some comments to the code.
Quoted 7 lines
Ok, will do.
Quoted 3 lines
And still 'stats->rx_nombuf' change is not required right? If so can you remove it in the next version?
It is still needed. NETVSC unconditionally calls the slave PMD to receive
packets, even if it can't allocate a mbuf to receive a synthetic packet itself. The accounting of rx_nombuf is valid because the synthetic packets (to NETVSC) and VF packets (to slave PMD) are routed separately from Hyper-V.

Quoted 6 lines

Quoted 3 lines
I am not referring to the "+=" update, my comment was because 'stats-
Quoted 1 line
rx_nombuf' is overwritten in 'rte_eth_stats_get()' [1].
Is it still required?
Yes, it is still needed. NETVSC calls the rte_eth_stats_get() on its slave PMD first, and stats->rx_nombuf is updated (overwritten) for its slave PMD. Afte that, it needs to add to its own dev->data->rx_mbuf_alloc_failed back to stats->rx_nombuf.

But its own stat also will be overwritten (not in PMD function, but in ethdev layer). 'stats->rx_nombuf' assignment in the PMD seems has no effect and can be removed.

I can't see how it is needed, can you please put a call stack to describe?

Quoted 10 lines

Quoted 10 lines
[1]
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.dpdk
.org%2Fdpdk%2Ftree%2Flib%2Fethdev%2Frte_ethdev.c%3Fh%3Dv22.03%23n25 18&data=05%7C01%7Clongli%40microsoft.com%7Cea473df2344c460d575 d08da2dca3e53%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63787 2643902917430%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQ IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sd ata=FZO%2B%2BnWtLGstHHIZ2aXsDUKNI%2Fi9tbj6jONhp174qKw%3D&res erved=0

Stephen Hemminger

2022-05-05 16:40 UTC #11 lore raw

Quoted 67 lines · On Thu, 5 May 2022 17:28:38 +0100 Ferruh Yigit <[email protected]> wrote:

Quoted 59 lines · Long Li
Quoted 59 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 45 lines · Long Li
Quoted 36 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 33 lines · Long Li
Quoted 22 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 15 lines · Long Li
Quoted 15 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 9 lines · On Tue, 26 Apr 2022 22:56:14 +0100 Ferruh Yigit <[email protected]> wrote:
Review
Quoted 5 lines
      		if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
(quoted diff) +2 -2
-			stats->q_opackets[i] = txq->stats.packets;
-			stats->q_obytes[i] = txq->stats.bytes;
+			stats->q_opackets[i] += txq->stats.packets;
+			stats->q_obytes[i] += txq->stats.bytes;  
This is per queue stats, 'stats->q_opackets[i]', in next iteration of the loop, 'i' will be increased and 'txq' will be updated, so as far as I can see the above change has no affect.
Quoted 4 lines
Quoted 1 line
Agree, that is why it was just assignment originally.
The condition here is a little different. NETVSC is a master device with
Quoted 8 lines
another PMD running as a slave. When reporting stats values, it needs to add the values from the slave PMD. The original code just overwrites the values from its slave PMD.
Where the initial values are coming from, 'hn_vf_stats_get()'?
If 'hn_vf_stats_get()' fills the stats, what are the values kept in 'txq-
stats.*'
Quoted 1 line
in above updated loop?
Quoted 8 lines
Quoted 2 lines
Yes, hn_vf_stats_get() fills in the stats from the slave PMD. txq->stats
values are from the master PMD. Those values are different and accounted separated from the values from the slave PMD.
I see, since this is a little different than what most of the PMDs do, can you please put a little more info to the commit log? Or perhaps can add some comments to the code.
Quoted 7 lines
Ok, will do.
Quoted 3 lines
And still 'stats->rx_nombuf' change is not required right? If so can you remove it in the next version?
It is still needed. NETVSC unconditionally calls the slave PMD to receive
packets, even if it can't allocate a mbuf to receive a synthetic packet itself. The accounting of rx_nombuf is valid because the synthetic packets (to NETVSC) and VF packets (to slave PMD) are routed separately from Hyper-V.
Quoted 6 lines
Quoted 3 lines
I am not referring to the "+=" update, my comment was because 'stats-
Quoted 1 line
rx_nombuf' is overwritten in 'rte_eth_stats_get()' [1].
Is it still required?
Yes, it is still needed. NETVSC calls the rte_eth_stats_get() on its slave PMD first, and stats->rx_nombuf is updated (overwritten) for its slave PMD. Afte that, it needs to add to its own dev->data->rx_mbuf_alloc_failed back to stats->rx_nombuf.

Quoted 6 lines

But its own stat also will be overwritten (not in PMD function, but in ethdev layer).
'stats->rx_nombuf' assignment in the PMD seems has no effect and can be removed.
I can't see how it is needed, can you please put a call stack to describe?

This here:

int
rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)

{
	struct rte_eth_dev *dev;

	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
	dev = &rte_eth_devices[port_id];

	if (stats == NULL) {
		RTE_ETHDEV_LOG(ERR, "Cannot get ethdev port %u stats to NULL\n",
			port_id);
		return -EINVAL;
	}

	memset(stats, 0, sizeof(*stats));

	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->stats_get, -ENOTSUP);
	stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed;
	return eth_err(port_id, (*dev->dev_ops->stats_get)(dev, stats));
}

Will fill in rx_nombuf from the current rx_mbuf_alloc_failed. But it happens before the PMD specific stats function.

For the case of nested device like netvsc PMD that value needs to be summed.

What happens with VF would be:

Call of rte_eth_stats_get on the netvsc PMD port.

    stats->rx_nombuf comes from rx_mbuf_alloc_failed of the vmbus part

Netvsc PMD stats get calls rte_eth_stats_get on the VF Nested rte_eth_stats_get will set stats->rx_nombuf to the value from VF
the previous value is overwritten.

At end of Netvsc PMD stats get it should be adding the value of any allocation failures in the VMBUS part.

Summary:

rte_eth_stats_get:      stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed;  // original
rte_eth_stats_get(vf):  stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed;  // VF alloc failed
hn_dev_stats_get:       stats->rx_nombuf += dev->data->rx_mbuf_alloc_failed; // Vmbus alloc failed

Ferruh Yigit

2022-05-05 16:57 UTC #12 lore raw

Quoted 101 lines · Stephen Hemminger

Quoted 67 lines · On Thu, 5 May 2022 17:28:38 +0100 Ferruh Yigit <[email protected]> wrote:
Quoted 59 lines · Long Li
Quoted 59 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 45 lines · Long Li
Quoted 36 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 33 lines · Long Li
Quoted 22 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 15 lines · Long Li
Quoted 15 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 9 lines · On Tue, 26 Apr 2022 22:56:14 +0100 Ferruh Yigit <[email protected]> wrote:
Review
Quoted 5 lines
       		if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
(quoted diff) +2 -2
-			stats->q_opackets[i] = txq->stats.packets;
-			stats->q_obytes[i] = txq->stats.bytes;
+			stats->q_opackets[i] += txq->stats.packets;
+			stats->q_obytes[i] += txq->stats.bytes;
This is per queue stats, 'stats->q_opackets[i]', in next iteration of the loop, 'i' will be increased and 'txq' will be updated, so as far as I can see the above change has no affect.
Quoted 4 lines
Quoted 1 line
Agree, that is why it was just assignment originally.
The condition here is a little different. NETVSC is a master device with
Quoted 8 lines
another PMD running as a slave. When reporting stats values, it needs to add the values from the slave PMD. The original code just overwrites the values from its slave PMD.
Where the initial values are coming from, 'hn_vf_stats_get()'?
If 'hn_vf_stats_get()' fills the stats, what are the values kept in 'txq-
stats.*'
Quoted 1 line
in above updated loop?
Quoted 8 lines
Quoted 2 lines
Yes, hn_vf_stats_get() fills in the stats from the slave PMD. txq->stats
values are from the master PMD. Those values are different and accounted separated from the values from the slave PMD.
I see, since this is a little different than what most of the PMDs do, can you please put a little more info to the commit log? Or perhaps can add some comments to the code.
Quoted 7 lines
Ok, will do.
Quoted 3 lines
And still 'stats->rx_nombuf' change is not required right? If so can you remove it in the next version?
It is still needed. NETVSC unconditionally calls the slave PMD to receive
packets, even if it can't allocate a mbuf to receive a synthetic packet itself. The accounting of rx_nombuf is valid because the synthetic packets (to NETVSC) and VF packets (to slave PMD) are routed separately from Hyper-V.
Quoted 6 lines
Quoted 3 lines
I am not referring to the "+=" update, my comment was because 'stats-
Quoted 1 line
rx_nombuf' is overwritten in 'rte_eth_stats_get()' [1].
Is it still required?
Yes, it is still needed. NETVSC calls the rte_eth_stats_get() on its slave PMD first, and stats->rx_nombuf is updated (overwritten) for its slave PMD. Afte that, it needs to add to its own dev->data->rx_mbuf_alloc_failed back to stats->rx_nombuf.
Quoted 6 lines
But its own stat also will be overwritten (not in PMD function, but in ethdev layer).
'stats->rx_nombuf' assignment in the PMD seems has no effect and can be removed.
I can't see how it is needed, can you please put a call stack to describe?
This here:
int
rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
{
	struct rte_eth_dev *dev;

	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
	dev = &rte_eth_devices[port_id];

	if (stats == NULL) {
		RTE_ETHDEV_LOG(ERR, "Cannot get ethdev port %u stats to NULL\n",
			port_id);
		return -EINVAL;
	}

	memset(stats, 0, sizeof(*stats));

	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->stats_get, -ENOTSUP);
	stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed;
	return eth_err(port_id, (*dev->dev_ops->stats_get)(dev, stats));
}

Quoted 3 lines

Will fill in rx_nombuf from the current rx_mbuf_alloc_failed. But it happens before the PMD specific stats function.

I keep seeing the ethdev assignment as *after* the dev_ops, but it is not [1], so code is OK as it is.

[1]
It seems assignment was after but it is fixed on the way:
Commit 53ecfa24fbcd ("ethdev: fix overwriting driver-specific stats")

Long Li

2022-05-10 05:33 UTC #13 lore raw

Quoted 112 lines

Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 16 lines · Stephen Hemminger
Quoted 13 lines · On Thu, 5 May 2022 17:28:38 +0100 Ferruh Yigit <[email protected]> wrote:
Quoted 12 lines · Long Li
Quoted 12 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 9 lines · Long Li
Quoted 9 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 6 lines · Long Li
Quoted 6 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 3 lines · Long Li
Quoted 3 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 50 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines · On Tue, 26 Apr 2022 22:56:14 +0100 Ferruh Yigit <[email protected]> wrote:
Quoted 4 lines
if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
- stats->q_opackets[i] = txq->stats.packets;
- stats->q_obytes[i] = txq->stats.bytes;
+ stats->q_opackets[i] += txq-
stats.packets;
Quoted 42 lines
Quoted 42 lines
Quoted 42 lines
Quoted 31 lines
Quoted 22 lines
Quoted 22 lines
Quoted 14 lines
Quoted 10 lines
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
+ stats->q_obytes[i] += txq->stats.bytes;
This is per queue stats, 'stats->q_opackets[i]', in next iteration of the loop, 'i' will be increased and 'txq' will be updated, so as far as I can see the above change has no affect.
Agree, that is why it was just assignment originally.
The condition here is a little different. NETVSC is a master device with
another PMD running as a slave. When reporting stats values, it needs to add the values from the slave PMD. The original code just overwrites the values from its slave PMD.
Quoted 5 lines
Where the initial values are coming from, 'hn_vf_stats_get()'?
If 'hn_vf_stats_get()' fills the stats, what are the values kept in
'txq-
stats.*'
Quoted 1 line
in above updated loop?
Quoted 8 lines
Quoted 2 lines
Yes, hn_vf_stats_get() fills in the stats from the slave PMD. txq->stats
values are from the master PMD. Those values are different and accounted separated from the values from the slave PMD.
I see, since this is a little different than what most of the PMDs do, can you please put a little more info to the commit log? Or perhaps can add some comments to the code.
Quoted 8 lines
Ok, will do.
Quoted 3 lines
And still 'stats->rx_nombuf' change is not required right? If so can you remove it in the next version?
It is still needed. NETVSC unconditionally calls the slave PMD to receive
packets, even if it can't allocate a mbuf to receive a synthetic packet itself. The accounting of rx_nombuf is valid because the synthetic packets (to NETVSC) and VF packets (to slave PMD) are routed
separately from Hyper-V.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
I am not referring to the "+=" update, my comment was because 'stats-
Quoted 1 line
rx_nombuf' is overwritten in 'rte_eth_stats_get()' [1].
Is it still required?
Yes, it is still needed. NETVSC calls the rte_eth_stats_get() on its slave PMD
first, and stats->rx_nombuf is updated (overwritten) for its slave PMD. Afte that, it needs to add to its own dev->data->rx_mbuf_alloc_failed back to stats-
Quoted 14 lines
rx_nombuf.
Quoted 8 lines
Quoted 1 line
But its own stat also will be overwritten (not in PMD function, but in ethdev layer).
'stats->rx_nombuf' assignment in the PMD seems has no effect and can be removed.
I can't see how it is needed, can you please put a call stack to describe?
This here:
int
Quoted 8 lines
rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats) {
	struct rte_eth_dev *dev;

	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
	dev = &rte_eth_devices[port_id];

	if (stats == NULL) {
		RTE_ETHDEV_LOG(ERR, "Cannot get ethdev port %u stats to
NULL\n",
Quoted 10 lines
			port_id);
		return -EINVAL;
	}

	memset(stats, 0, sizeof(*stats));

	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->stats_get, -ENOTSUP);
	stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed;
	return eth_err(port_id, (*dev->dev_ops->stats_get)(dev, stats)); }

Quoted 6 lines

Quoted 3 lines
Will fill in rx_nombuf from the current rx_mbuf_alloc_failed. But it happens before the PMD specific stats function.
I keep seeing the ethdev assignment as *after* the dev_ops, but it is not [1], so code is OK as it is.

Hi Ferruh,

Do you still want me to send a v3, or this patch is good as it is?

Long

Quoted 5 lines

[1]
It seems assignment was after but it is fixed on the way:
Commit 53ecfa24fbcd ("ethdev: fix overwriting driver-specific stats")

Ferruh Yigit

2022-05-10 11:29 UTC #14 lore raw

Quoted 116 lines · Long Li

Quoted 116 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 16 lines · Stephen Hemminger
Quoted 13 lines · On Thu, 5 May 2022 17:28:38 +0100 Ferruh Yigit <[email protected]> wrote:
Quoted 12 lines · Long Li
Quoted 12 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 9 lines · Long Li
Quoted 9 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 6 lines · Long Li
Quoted 6 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 3 lines · Long Li
Quoted 3 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 50 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines · On Tue, 26 Apr 2022 22:56:14 +0100 Ferruh Yigit <[email protected]> wrote:
if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
- stats->q_opackets[i] = txq->stats.packets;
- stats->q_obytes[i] = txq->stats.bytes;
+ stats->q_opackets[i] += txq-
stats.packets;
Quoted 42 lines
Quoted 42 lines
Quoted 42 lines
Quoted 31 lines
Quoted 22 lines
Quoted 22 lines
Quoted 14 lines
Quoted 10 lines
Quoted 7 lines
Quoted 5 lines
+ stats->q_obytes[i] += txq->stats.bytes;
This is per queue stats, 'stats->q_opackets[i]', in next
iteration of the loop, 'i' will be increased and 'txq' will
be updated, so as far as I can see the above change has no affect.
Agree, that is why it was just assignment originally.
The condition here is a little different. NETVSC is a master device with
another PMD running as a slave. When reporting stats values, it needs to add the values from the slave PMD. The original code just overwrites the values from its slave PMD.
Quoted 5 lines
Where the initial values are coming from, 'hn_vf_stats_get()'?
If 'hn_vf_stats_get()' fills the stats, what are the values kept in
'txq-
stats.*'
Quoted 1 line
in above updated loop?
Quoted 8 lines
Quoted 2 lines
Yes, hn_vf_stats_get() fills in the stats from the slave PMD. txq->stats
values are from the master PMD. Those values are different and accounted separated from the values from the slave PMD.
I see, since this is a little different than what most of the PMDs do, can you please put a little more info to the commit log? Or perhaps can add some comments to the code.
Quoted 8 lines
Ok, will do.
Quoted 3 lines
And still 'stats->rx_nombuf' change is not required right? If so can you remove it in the next version?
It is still needed. NETVSC unconditionally calls the slave PMD to receive
packets, even if it can't allocate a mbuf to receive a synthetic packet itself. The accounting of rx_nombuf is valid because the synthetic packets (to NETVSC) and VF packets (to slave PMD) are routed
separately from Hyper-V.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
I am not referring to the "+=" update, my comment was because 'stats-
Quoted 1 line
rx_nombuf' is overwritten in 'rte_eth_stats_get()' [1].
Is it still required?
Yes, it is still needed. NETVSC calls the rte_eth_stats_get() on its slave PMD
first, and stats->rx_nombuf is updated (overwritten) for its slave PMD. Afte that, it needs to add to its own dev->data->rx_mbuf_alloc_failed back to stats-
Quoted 14 lines
rx_nombuf.
Quoted 8 lines
Quoted 1 line
But its own stat also will be overwritten (not in PMD function, but in ethdev layer).
'stats->rx_nombuf' assignment in the PMD seems has no effect and can be removed.
I can't see how it is needed, can you please put a call stack to describe?
This here:
int
Quoted 8 lines
rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats) {
	struct rte_eth_dev *dev;

	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
	dev = &rte_eth_devices[port_id];

	if (stats == NULL) {
		RTE_ETHDEV_LOG(ERR, "Cannot get ethdev port %u stats to
NULL\n",
Quoted 13 lines
			port_id);
		return -EINVAL;
	}

	memset(stats, 0, sizeof(*stats));

	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->stats_get, -ENOTSUP);
	stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed;
	return eth_err(port_id, (*dev->dev_ops->stats_get)(dev, stats)); }
Will fill in rx_nombuf from the current rx_mbuf_alloc_failed. But it happens before the PMD specific stats function.

Quoted 7 lines

Quoted 2 lines
I keep seeing the ethdev assignment as *after* the dev_ops, but it is not [1], so code is OK as it is.
Hi Ferruh,
Do you still want me to send a v3, or this patch is good as it is?

Yes can you please send a v3 with some more description in the commit log on the special case for the PMD, and perhaps some comments in the code.

Thanks,
ferruh

Quoted 7 lines

Long
Quoted 5 lines
[1]
It seems assignment was after but it is fixed on the way:
Commit 53ecfa24fbcd ("ethdev: fix overwriting driver-specific stats")

Long Li

2022-05-10 18:03 UTC #15 lore raw

Quoted 130 lines

Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 20 lines · Long Li
Quoted 20 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 17 lines · Stephen Hemminger
Quoted 14 lines · On Thu, 5 May 2022 17:28:38 +0100 Ferruh Yigit <[email protected]> wrote:
Quoted 13 lines · Long Li
Quoted 13 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 10 lines · Long Li
Quoted 10 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 7 lines · Long Li
Quoted 7 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats values
Quoted 3 lines · Long Li
Quoted 3 lines
Subject: Re: [Patch v2] net/netvsc: report correct stats
values
Quoted 14 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
On Tue, 26 Apr 2022 22:56:14 +0100 Ferruh Yigit
<[email protected]> wrote:
if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
- stats->q_opackets[i] = txq-
stats.packets;
Quoted 8 lines
Quoted 8 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
- stats->q_obytes[i] = txq->stats.bytes;
+ stats->q_opackets[i] += txq-
stats.packets;
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
+ stats->q_obytes[i] += txq->stats.bytes;
This is per queue stats, 'stats->q_opackets[i]', in next
iteration of the loop, 'i' will be increased and 'txq'
will be updated, so as far as I can see the above change has no
affect.
Quoted 92 lines
Quoted 83 lines
Quoted 32 lines
Quoted 32 lines
Quoted 32 lines
Quoted 32 lines
Quoted 32 lines
Quoted 17 lines
Quoted 17 lines
Quoted 9 lines
Quoted 5 lines
Quoted 2 lines
Agree, that is why it was just assignment originally.
The condition here is a little different. NETVSC is a master device with
another PMD running as a slave. When reporting stats values, it needs to add the values from the slave PMD. The original code just overwrites the values from its slave PMD.
Quoted 5 lines
Where the initial values are coming from, 'hn_vf_stats_get()'?
If 'hn_vf_stats_get()' fills the stats, what are the values kept in
'txq-
stats.*'
Quoted 1 line
in above updated loop?
Quoted 8 lines
Quoted 2 lines
Yes, hn_vf_stats_get() fills in the stats from the slave PMD. txq->stats
values are from the master PMD. Those values are different and accounted separated from the values from the slave PMD.
I see, since this is a little different than what most of the PMDs do, can you please put a little more info to the commit log? Or perhaps can add some comments to the code.
Ok, will do.
Quoted 3 lines
And still 'stats->rx_nombuf' change is not required right? If so can you remove it in the next version?
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines
It is still needed. NETVSC unconditionally calls the slave PMD to receive
packets, even if it can't allocate a mbuf to receive a synthetic packet itself. The accounting of rx_nombuf is valid because the synthetic packets (to NETVSC) and VF packets (to slave PMD) are routed
separately from Hyper-V.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
I am not referring to the "+=" update, my comment was because 'stats-
Quoted 1 line
rx_nombuf' is overwritten in 'rte_eth_stats_get()' [1].
Is it still required?
Yes, it is still needed. NETVSC calls the rte_eth_stats_get() on its slave PMD
first, and stats->rx_nombuf is updated (overwritten) for its slave PMD. Afte that, it needs to add to its own dev->data->rx_mbuf_alloc_failed back to stats-
Quoted 14 lines
rx_nombuf.
Quoted 8 lines
Quoted 1 line
But its own stat also will be overwritten (not in PMD function, but in ethdev layer).
'stats->rx_nombuf' assignment in the PMD seems has no effect and can be removed.
I can't see how it is needed, can you please put a call stack to describe?
This here:
int
Quoted 8 lines
rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats) {
	struct rte_eth_dev *dev;

	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
	dev = &rte_eth_devices[port_id];

	if (stats == NULL) {
		RTE_ETHDEV_LOG(ERR, "Cannot get ethdev port %u stats to
NULL\n",
Quoted 10 lines
			port_id);
		return -EINVAL;
	}

	memset(stats, 0, sizeof(*stats));

	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->stats_get, -ENOTSUP);
	stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed;
	return eth_err(port_id, (*dev->dev_ops->stats_get)(dev, stats)); }
Quoted 6 lines
Quoted 3 lines
Will fill in rx_nombuf from the current rx_mbuf_alloc_failed. But it happens before the PMD specific stats function.
I keep seeing the ethdev assignment as *after* the dev_ops, but it is not [1], so code is OK as it is.
Hi Ferruh,

Quoted 8 lines

Quoted 2 lines
Do you still want me to send a v3, or this patch is good as it is?
Yes can you please send a v3 with some more description in the commit log on the special case for the PMD, and perhaps some comments in the code.
Thanks,
Ferruh

Yes, will send out shortly.