app/testpmd: fix statistics in multiple process

PATCH In review Min Hu (Connor) 3 participants 5 messages Sep 14, 2021 – Oct 27, 2021

Share & tools

Min Hu (Connor)

author

2021-09-14 03:14 UTC #0 lore raw

PATCH 1/1 app/testpmd: fix statistics in multiple process

Currently, 'clear port stats all' in secondary will clear stats in PMD, and also update stats which APP stores in 'ports[]' array in secondary process, note that, that in primary process remains unchanged. So, when 'show fwd stats all' in primary process, stats in PMD may be less than stats which APP stores in 'ports[]' array(the stats is different with that in secondary). So forward statistics(stats in PMD minus stats which APP stores in 'ports[]' array) will be wrong. Like this:
---------------------- Forward statistics for port 0 --------------
RX-packets: 18446744073703120168 RX-dropped: 18446744073704076766
RX-total: 18446744073697645318
TX-packets: 18446744073703122216 TX-dropped: 0
TX-total: 18446744073703122216
--------------------------------------------------------------------
Stats in PMD are shared between multiple processes, but stats which APP stores have their own copies in multiple processes. And this will result in bugs.

This patch will fix it by creating shared memory to store last stats for multiple and secondary process in testpmd.

Fixes 184de26c78d0 ("app/testpmd: support multi-process")

Signed-off-by Min Hu (Connor) <[email protected]>

---
 app/test-pmd/config.c  |  4 +--
 app/test-pmd/testpmd.c | 63 ++++++++++++++++++++++++++++++++++++------
 app/test-pmd/testpmd.h |  2 +-
 3 files changed, 58 insertions(+), 11 deletions(-)

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

index f5765b34f7..31fee009c4 100644
@@ -265,7 +265,7 @@ nic_stats_clear(portid_t port_id)
 		return;
 	}
 
-	ret = rte_eth_stats_get(port_id, &ports[port_id].stats);
+	ret = rte_eth_stats_get(port_id, ports[port_id].stats);
 	if (ret != 0) {
 		if (ret < 0)
 			ret = -ret;

@@ -358,7 +358,7 @@ nic_xstats_clear(portid_t port_id)
 		return;
 	}
 
-	ret = rte_eth_stats_get(port_id, &ports[port_id].stats);
+	ret = rte_eth_stats_get(port_id, ports[port_id].stats);
 	if (ret != 0) {
 		if (ret < 0)
 			ret = -ret;

app/test-pmd/testpmd.c +55 -8

105 lines not on this page · raw message

app/test-pmd/testpmd.h +1 -1

index 5863b2f43f..a48a302343 100644
@@ -202,7 +202,7 @@ struct rte_port {
 	struct rte_eth_dev_info dev_info;   /**< PCI info + driver name */
 	struct rte_eth_conf     dev_conf;   /**< Port configuration. */
 	struct rte_ether_addr       eth_addr;   /**< Port ethernet address */
-	struct rte_eth_stats    stats;      /**< Last port statistics */
+	struct rte_eth_stats    *stats;      /**< Last port statistics */
 	unsigned int            socket_id;  /**< For NUMA support */
 	uint16_t		parse_tunnel:1; /**< Parse internal headers */
 	uint16_t                tso_segsz;  /**< Segmentation offload MSS for non-tunneled packets. */

Signature · 1 line

2.33.0

Li, Xiaoyun

2021-09-16 05:18 UTC #1 lore raw

Quoted 22 lines

-----Original Message-----
From: Min Hu (Connor) <[email protected]>
Sent: Tuesday, September 14, 2021 11:13
To: [email protected]
Cc: Yigit, Ferruh <[email protected]>; Li, Xiaoyun <[email protected]>
Subject: [PATCH] app/testpmd: fix statistics in multiple process
Currently, 'clear port stats all' in secondary will clear stats in PMD, and also update stats which APP stores in 'ports[]' array in secondary process, note that, that in primary process remains unchanged. So, when 'show fwd stats all' in primary process, stats in PMD may be less than stats which APP stores in 'ports[]' array(the stats is different with that in secondary). So forward statistics(stats in PMD minus stats which APP stores in 'ports[]' array) will be wrong. Like this:
---------------------- Forward statistics for port 0 --------------
RX-packets: 18446744073703120168 RX-dropped: 18446744073704076766
RX-total: 18446744073697645318
TX-packets: 18446744073703122216 TX-dropped: 0
TX-total: 18446744073703122216
--------------------------------------------------------------------
Stats in PMD are shared between multiple processes, but stats which APP stores have their own copies in multiple processes. And this will result in bugs.

Quoted 2 lines

This patch will fix it by creating shared memory to store last stats for multiple and secondary process in testpmd.

Why not just limit "clear port stats " behavior to only primary process? Is there any particular reason second process has to do clear stats?

Stats is quite complicate struct. I feel like it will have race condition issue if you allow two processes to clear it. Only allow getting for multiple processes makes more sense.

Also your patch has issues even if this idea is accepted by others. Please see below.

Quoted 10 lines

Fixes 184de26c78d0 ("app/testpmd: support multi-process")
Signed-off-by Min Hu (Connor) <[email protected]>
---
 app/test-pmd/config.c  |  4 +--
 app/test-pmd/testpmd.c | 63 ++++++++++++++++++++++++++++++++++++------
 app/test-pmd/testpmd.h |  2 +-
 3 files changed, 58 insertions(+), 11 deletions(-)

<snip>

app/test-pmd/testpmd.c init_port_config(void) · line 3618

Quoted 6 lines

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

@@ -3572,6 +3614,7 @@ init_port_config(void)
 			port->dev_conf.intr_conf.lsc = 1;
 		if (rmv_interrupt && (*port->dev_info.dev_flags &
 RTE_ETH_DEV_INTR_RMV))
 			port->dev_conf.intr_conf.rmv = 1;
+		port_stats_init(pid);

Init_port_config is called in several places. Why init stats as 0 for port->stats here?
You did so. Then if user use cmd like "port config 0 rxq 2" which calls init_port_config, port->stats is cleared while device stats isn't. Then the fwd stats you got will be wrong.

Please read code carefully. Port->stats will be reset as device stats when start_fwd. Not reset as 0.

Port->stats should be only cleared when you do clear_nic_stats. Otherwise, it doesn't make sense as "last port stats".

Quoted 30 lines

app/test-pmd/testpmd.c +4 -0

 	}
 }

@@ -3882,6 +3925,10 @@ main(int argc, char** argv)
 		rte_exit(EXIT_FAILURE, "Cannot init EAL: %s\n",
 			 rte_strerror(rte_errno));
 
+	ret = eth_stats_zone_reserve();
+	if (ret != 0)
+		rte_exit(EXIT_FAILURE, "Allocate memzone ethdev stats failed");
+
 	ret = register_eth_event_callback();
 	if (ret != 0)
 		rte_exit(EXIT_FAILURE, "Cannot register for ethdev events");

app/test-pmd/testpmd.h index renamed from app/test-pmd/testpmd.h +0 -0

5863b2f43f..a48a302343 100644

app/test-pmd/testpmd.h +1 -1

@@ -202,7 +202,7 @@ struct rte_port {
 	struct rte_eth_dev_info dev_info;   /**< PCI info + driver name */
 	struct rte_eth_conf     dev_conf;   /**< Port configuration. */
 	struct rte_ether_addr       eth_addr;   /**< Port ethernet address */
-	struct rte_eth_stats    stats;      /**< Last port statistics */
+	struct rte_eth_stats    *stats;      /**< Last port statistics */
 	unsigned int            socket_id;  /**< For NUMA support */
 	uint16_t		parse_tunnel:1; /**< Parse internal headers */
 	uint16_t                tso_segsz;  /**< Segmentation offload MSS for non-

tunneled packets. */
--
2.33.0

Min Hu (Connor)

author

2021-09-17 03:34 UTC #2 lore raw

Hi, Xiaoyun,

Quoted 24 lines · Xiaoyun

Hi
Quoted 22 lines
-----Original Message-----
From: Min Hu (Connor) <[email protected]>
Sent: Tuesday, September 14, 2021 11:13
To: [email protected]
Cc: Yigit, Ferruh <[email protected]>; Li, Xiaoyun <[email protected]>
Subject: [PATCH] app/testpmd: fix statistics in multiple process
Currently, 'clear port stats all' in secondary will clear stats in PMD, and also update stats which APP stores in 'ports[]' array in secondary process, note that, that in primary process remains unchanged. So, when 'show fwd stats all' in primary process, stats in PMD may be less than stats which APP stores in 'ports[]' array(the stats is different with that in secondary). So forward statistics(stats in PMD minus stats which APP stores in 'ports[]' array) will be wrong. Like this:
---------------------- Forward statistics for port 0 --------------
RX-packets: 18446744073703120168 RX-dropped: 18446744073704076766
RX-total: 18446744073697645318
TX-packets: 18446744073703122216 TX-dropped: 0
TX-total: 18446744073703122216
--------------------------------------------------------------------
Stats in PMD are shared between multiple processes, but stats which APP stores have their own copies in multiple processes. And this will result in bugs.

Quoted 5 lines

Quoted 2 lines
This patch will fix it by creating shared memory to store last stats for multiple and secondary process in testpmd.
Why not just limit "clear port stats " behavior to only primary process? Is there any particular reason second process has to do clear stats?

While, I have no idea if some particular reason exists second process has to do clear stats. I just want to implement this function.

Quoted 4 lines

Stats is quite complicate struct. I feel like it will have race condition issue if you allow two processes to clear it.
Only allow getting for multiple processes makes more sense.

Yes, I agree with you, maybe I can add constrains in testpmd guide doc. BTW, what abot any others' opinion ?

Quoted 25 lines

Also your patch has issues even if this idea is accepted by others. Please see below.
Quoted 10 lines
Fixes 184de26c78d0 ("app/testpmd: support multi-process")
Signed-off-by Min Hu (Connor) <[email protected]>
---
  app/test-pmd/config.c  |  4 +--
  app/test-pmd/testpmd.c | 63 ++++++++++++++++++++++++++++++++++++------
  app/test-pmd/testpmd.h |  2 +-
  3 files changed, 58 insertions(+), 11 deletions(-)
<snip>
app/test-pmd/testpmd.c init_port_config(void) · line 3618
Quoted 6 lines
app/test-pmd/testpmd.c +1 -0
@@ -3572,6 +3614,7 @@ init_port_config(void)
  			port->dev_conf.intr_conf.lsc = 1;
  		if (rmv_interrupt && (*port->dev_info.dev_flags &
 RTE_ETH_DEV_INTR_RMV))
  			port->dev_conf.intr_conf.rmv = 1;
+		port_stats_init(pid);
Init_port_config is called in several places. Why init stats as 0 for port->stats here?
You did so. Then if user use cmd like "port config 0 rxq 2" which calls init_port_config, port->stats is cleared while device stats isn't. Then the fwd stats you got will be wrong.

Quoted 4 lines

Please read code carefully. Port->stats will be reset as device stats when start_fwd. Not reset as 0.
Port->stats should be only cleared when you do clear_nic_stats. Otherwise, it doesn't make sense as "last port stats".

I know your meaning.
Actually, My purpose for 'port_stats_init' is only called for all port
at init stage. While 'init_port config' confusd me, May I can put the
'port_stats_init' in 'init_config', this may can work out.

Thanks.

Quoted 33 lines

Quoted 27 lines

app/test-pmd/testpmd.c +4 -0

  	}
  }

@@ -3882,6 +3925,10 @@ main(int argc, char** argv)
  		rte_exit(EXIT_FAILURE, "Cannot init EAL: %s\n",
  			 rte_strerror(rte_errno));
 
+	ret = eth_stats_zone_reserve();
+	if (ret != 0)
+		rte_exit(EXIT_FAILURE, "Allocate memzone ethdev stats failed");
+
  	ret = register_eth_event_callback();
  	if (ret != 0)
  		rte_exit(EXIT_FAILURE, "Cannot register for ethdev events");

app/test-pmd/testpmd.h index renamed from app/test-pmd/testpmd.h +0 -0

5863b2f43f..a48a302343 100644

app/test-pmd/testpmd.h +1 -1

@@ -202,7 +202,7 @@ struct rte_port {
  	struct rte_eth_dev_info dev_info;   /**< PCI info + driver name */
  	struct rte_eth_conf     dev_conf;   /**< Port configuration. */
  	struct rte_ether_addr       eth_addr;   /**< Port ethernet address */
-	struct rte_eth_stats    stats;      /**< Last port statistics */
+	struct rte_eth_stats    *stats;      /**< Last port statistics */
  	unsigned int            socket_id;  /**< For NUMA support */
  	uint16_t		parse_tunnel:1; /**< Parse internal headers */
  	uint16_t                tso_segsz;  /**< Segmentation offload MSS for non-

Quoted 3 lines

tunneled packets. */
--
2.33.0

Ferruh Yigit

2021-10-26 16:22 UTC #3 lore raw

Quoted 32 lines · Min Hu (Connor)

Hi, Xiaoyun,
Quoted 24 lines · Xiaoyun
Hi
Quoted 22 lines
-----Original Message-----
From: Min Hu (Connor) <[email protected]>
Sent: Tuesday, September 14, 2021 11:13
To: [email protected]
Cc: Yigit, Ferruh <[email protected]>; Li, Xiaoyun <[email protected]>
Subject: [PATCH] app/testpmd: fix statistics in multiple process
Currently, 'clear port stats all' in secondary will clear stats in PMD, and also update stats which APP stores in 'ports[]' array in secondary process, note that, that in primary process remains unchanged. So, when 'show fwd stats all' in primary process, stats in PMD may be less than stats which APP stores in 'ports[]' array(the stats is different with that in secondary). So forward statistics(stats in PMD minus stats which APP stores in 'ports[]' array) will be wrong. Like this:
---------------------- Forward statistics for port 0 --------------
RX-packets: 18446744073703120168 RX-dropped: 18446744073704076766
RX-total: 18446744073697645318
TX-packets: 18446744073703122216 TX-dropped: 0
TX-total: 18446744073703122216
--------------------------------------------------------------------
Stats in PMD are shared between multiple processes, but stats which APP stores have their own copies in multiple processes. And this will result in bugs.
Quoted 5 lines
Quoted 2 lines
This patch will fix it by creating shared memory to store last stats for multiple and secondary process in testpmd.
Why not just limit "clear port stats " behavior to only primary process? Is there any particular reason second process has to do clear stats?

Quoted 9 lines

While, I have no idea if some particular reason exists second process has to do clear stats. I just want to implement this function.
Quoted 4 lines
Stats is quite complicate struct. I feel like it will have race condition issue if you allow two processes to clear it.
Only allow getting for multiple processes makes more sense.
Yes, I agree with you, maybe I can add constrains in testpmd guide doc. BTW, what abot any others' opinion ?

I agree with Xiaoyun on possible race conditions, and to restrict the secondary to read the stats but not clear it.

Quoted 70 lines

Quoted 25 lines
Also your patch has issues even if this idea is accepted by others. Please see below.
Quoted 10 lines
Fixes 184de26c78d0 ("app/testpmd: support multi-process")
Signed-off-by Min Hu (Connor) <[email protected]>
---
app/test-pmd/config.c | 4 +--
app/test-pmd/testpmd.c | 63 ++++++++++++++++++++++++++++++++++++------ app/test-pmd/testpmd.h | 2 +-
  3 files changed, 58 insertions(+), 11 deletions(-)
<snip>
Quoted 6 lines
app/test-pmd/testpmd.c +0 -0
@@ -3572,6 +3614,7 @@ init_port_config(void)
               port->dev_conf.intr_conf.lsc = 1;
if (rmv_interrupt && (*port->dev_info.dev_flags & RTE_ETH_DEV_INTR_RMV)) port->dev_conf.intr_conf.rmv = 1;
+ port_stats_init(pid);
Init_port_config is called in several places. Why init stats as 0 for port->stats here?
You did so. Then if user use cmd like "port config 0 rxq 2" which calls init_port_config, port->stats is cleared while device stats isn't. Then the fwd stats you got will be wrong.
Quoted 4 lines
Please read code carefully. Port->stats will be reset as device stats when start_fwd. Not reset as 0.
Port->stats should be only cleared when you do clear_nic_stats. Otherwise, it doesn't make sense as "last port stats".
I know your meaning.
Actually, My purpose for 'port_stats_init' is only called for all port
at init stage. While 'init_port config' confusd me, May I can put the
'port_stats_init' in 'init_config', this may can work out.
Thanks.
Quoted 33 lines
Quoted 20 lines
}
}
app/test-pmd/testpmd.c +4 -0
@@ -3882,6 +3925,10 @@ main(int argc, char** argv)
           rte_exit(EXIT_FAILURE, "Cannot init EAL: %s\n",
                rte_strerror(rte_errno));
 
+    ret = eth_stats_zone_reserve();
+    if (ret != 0)
+        rte_exit(EXIT_FAILURE, "Allocate memzone ethdev stats failed");
+
       ret = register_eth_event_callback();
       if (ret != 0)
           rte_exit(EXIT_FAILURE, "Cannot register for ethdev events");
app/test-pmd/testpmd.h index renamed from app/test-pmd/testpmd.h +0 -0
5863b2f43f..a48a302343 100644
app/test-pmd/testpmd.h +0 -0
@@ -202,7 +202,7 @@ struct rte_port {
struct rte_eth_dev_info dev_info; /**< PCI info + driver name */
Quoted 10 lines
struct rte_eth_conf dev_conf; /**< Port configuration. */ struct rte_ether_addr eth_addr; /**< Port ethernet address */ - struct rte_eth_stats stats; /**< Last port statistics */ + struct rte_eth_stats *stats; /**< Last port statistics */ unsigned int socket_id; /**< For NUMA support */ uint16_t parse_tunnel:1; /**< Parse internal headers */ uint16_t tso_segsz; /**< Segmentation offload MSS for non- tunneled packets. */
Signature · 1 line
2.33.0
.

Min Hu (Connor)

author

2021-10-27 00:09 UTC #4 lore raw

Quoted 38 lines · Ferruh Yigit

Quoted 37 lines · Min Hu (Connor)
Hi, Xiaoyun,
Quoted 24 lines · Xiaoyun
Hi
Quoted 22 lines
-----Original Message-----
From: Min Hu (Connor) <[email protected]>
Sent: Tuesday, September 14, 2021 11:13
To: [email protected]
Cc: Yigit, Ferruh <[email protected]>; Li, Xiaoyun <[email protected]>
Subject: [PATCH] app/testpmd: fix statistics in multiple process
Currently, 'clear port stats all' in secondary will clear stats in PMD, and also update stats which APP stores in 'ports[]' array in secondary process, note that, that in primary process remains unchanged. So, when 'show fwd stats all' in primary process, stats in PMD may be less than stats which APP stores in 'ports[]' array(the stats is different with that in secondary). So forward statistics(stats in PMD minus stats which APP stores in 'ports[]' array) will be wrong. Like this:
---------------------- Forward statistics for port 0 --------------
RX-packets: 18446744073703120168 RX-dropped: 18446744073704076766
RX-total: 18446744073697645318
TX-packets: 18446744073703122216 TX-dropped: 0
TX-total: 18446744073703122216
--------------------------------------------------------------------
Stats in PMD are shared between multiple processes, but stats which APP stores have their own copies in multiple processes. And this will result in bugs.
Quoted 5 lines
Quoted 2 lines
This patch will fix it by creating shared memory to store last stats for multiple and secondary process in testpmd.
Why not just limit "clear port stats " behavior to only primary process? Is there any particular reason second process has to do clear stats?
While, I have no idea if some particular reason exists second process has to do clear stats. I just want to implement this function.
Quoted 2 lines
Stats is quite complicate struct. I feel like it will have race condition issue if you allow two processes to clear it.

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
Only allow getting for multiple processes makes more sense.
Yes, I agree with you, maybe I can add constrains in testpmd guide doc. BTW, what abot any others' opinion ?
I agree with Xiaoyun on possible race conditions, and to restrict the secondary to read the stats but not clear it.

Got it, this patch can be abandoned, thanks.

Quoted 74 lines

Quoted 28 lines
Quoted 28 lines
Also your patch has issues even if this idea is accepted by others. Please see below.
Quoted 11 lines
Fixes 184de26c78d0 ("app/testpmd: support multi-process")
Signed-off-by Min Hu (Connor) <[email protected]>
---
app/test-pmd/config.c | 4 +--
app/test-pmd/testpmd.c | 63
++++++++++++++++++++++++++++++++++++------
app/test-pmd/testpmd.h | 2 +-
  3 files changed, 58 insertions(+), 11 deletions(-)
<snip>
Quoted 6 lines
app/test-pmd/testpmd.c +0 -0
@@ -3572,6 +3614,7 @@ init_port_config(void)
               port->dev_conf.intr_conf.lsc = 1;
if (rmv_interrupt && (*port->dev_info.dev_flags & RTE_ETH_DEV_INTR_RMV)) port->dev_conf.intr_conf.rmv = 1;
+ port_stats_init(pid);
Init_port_config is called in several places. Why init stats as 0 for port->stats here?
You did so. Then if user use cmd like "port config 0 rxq 2" which calls init_port_config, port->stats is cleared while device stats isn't. Then the fwd stats you got will be wrong.
Please read code carefully. Port->stats will be reset as device stats when start_fwd. Not reset as 0.
Quoted 44 lines
Quoted 2 lines
Port->stats should be only cleared when you do clear_nic_stats. Otherwise, it doesn't make sense as "last port stats".
I know your meaning.
Actually, My purpose for 'port_stats_init' is only called for all port at init stage. While 'init_port config' confusd me, May I can put the 'port_stats_init' in 'init_config', this may can work out.
Thanks.
Quoted 34 lines
Quoted 21 lines
}
}
app/test-pmd/testpmd.c +4 -0
@@ -3882,6 +3925,10 @@ main(int argc, char** argv)
           rte_exit(EXIT_FAILURE, "Cannot init EAL: %s\n",
                rte_strerror(rte_errno));
 
+    ret = eth_stats_zone_reserve();
+    if (ret != 0)
+        rte_exit(EXIT_FAILURE, "Allocate memzone ethdev stats 
 failed");
+
       ret = register_eth_event_callback();
       if (ret != 0)
rte_exit(EXIT_FAILURE, "Cannot register for ethdev events");
app/test-pmd/testpmd.h index renamed from app/test-pmd/testpmd.h +0 -0
5863b2f43f..a48a302343 100644
app/test-pmd/testpmd.h +0 -0
@@ -202,7 +202,7 @@ struct rte_port {
struct rte_eth_dev_info dev_info; /**< PCI info + driver name */
Quoted 10 lines
struct rte_eth_conf dev_conf; /**< Port configuration. */ struct rte_ether_addr eth_addr; /**< Port ethernet address */ - struct rte_eth_stats stats; /**< Last port statistics */ + struct rte_eth_stats *stats; /**< Last port statistics */ unsigned int socket_id; /**< For NUMA support */ uint16_t parse_tunnel:1; /**< Parse internal headers */ uint16_t tso_segsz; /**< Segmentation offload MSS for non- tunneled packets. */
Signature · 1 line
2.33.0
.
.