Skip to contentSkip to conversation

app/testpmd: fix flow rules list after port stop

PATCH In review Gregory Etelson 5 participants 7 messages Aug 11, 2020 – Jan 6, 2021

lore
Share & tools
lore

Gregory Etelson

author
#0 lore raw

PATCH 1/1 app/testpmd: fix flow rules list after port stop

According to current RTE API, port flow rules must not be kept after port stop.

Testpmd did not flush port flow rules after `port stop' command was called. As the result, after the port was restarted, it showed bogus flow rules.

Fixes ce8d561418d4 ("app/testpmd: add port configuration settings")
Cc [email protected]
Signed-off-by Gregory Etelson <[email protected]>
Acked-by Ori Kam <[email protected]>
---
 app/test-pmd/testpmd.c | 3 +++
 1 file changed, 3 insertions(+)
app/test-pmd/testpmd.c +3 -0
index 7842c3b781..4ba5c41c6e 100644
@@ -2627,6 +2627,9 @@ stop_port(portid_t pid)
 						RTE_PORT_HANDLING) == 0)
 			continue;
 
+		if (port->flow_list)
+			port_flow_flush(pi);
+
 		rte_eth_dev_stop(pi);
 
 		if (rte_atomic16_cmpset(&(port->port_status),
Signature · 1 line
2.25.1
Acked-by on v1 1/1
#1 lore raw
Quoted 8 lines · Gregory Etelson

According to current RTE API, port flow rules must not be kept after port stop.

Testpmd did not flush port flow rules after `port stop' command was called. As the result, after the port was restarted, it showed bogus flow rules.

Quoted 6 lines
Fixes ce8d561418d4 ("app/testpmd: add port configuration settings")
Cc [email protected]
Signed-off-by Gregory Etelson <[email protected]>
Acked-by Ori Kam <[email protected]>
Acked-by Ajit Khaparde <[email protected]>
Quoted 21 lines
---
 app/test-pmd/testpmd.c | 3 +++
 1 file changed, 3 insertions(+)
app/test-pmd/testpmd.c +3 -1
index 7842c3b781..4ba5c41c6e 100644
@@ -2627,6 +2627,9 @@ stop_port(portid_t pid)
                                                RTE_PORT_HANDLING) == 0)
                        continue;
 
+               if (port->flow_list)
+                       port_flow_flush(pi);
+
                rte_eth_dev_stop(pi);
 
                if (rte_atomic16_cmpset(&(port->port_status),
--

2.25.1

#2 lore raw

24/11/2020 15:42, Ajit Khaparde:

Quoted 9 lines · Gregory Etelson
Quoted 8 lines

According to current RTE API, port flow rules must not be kept after port stop.

Testpmd did not flush port flow rules after `port stop' command was called. As the result, after the port was restarted, it showed bogus flow rules.

Quoted 8 lines
Quoted 6 lines
Fixes ce8d561418d4 ("app/testpmd: add port configuration settings")
Cc [email protected]
Signed-off-by Gregory Etelson <[email protected]>
Acked-by Ori Kam <[email protected]>
Acked-by Ajit Khaparde <[email protected]>

The rte_flow doc is now updated to make clear that flow rules are not kept after port stop.
This testpmd patch should make clear that the need of flushing is only for testpmd objects.
The change is postponed to DPDK 21.02.

author
#3 lore raw

PATCH v2 1/1 app/testpmd: fix testpmd flows left before port stop.

In reply to Gregory Etelson

According to RTE flow user guide, PMD will not keep flow rules after port stop. Application resources that refer to flow rules become obsolete after port stop and must not be used.
Testpmd maintains linked list of active flows for each port. Entries in that list are allocated dynamically and must be explicitly released to prevent memory leak.
The patch releases testpmd port flow_list that holds remaining flows before port is stopped.

Cc [email protected]
Signed-off-by Gregory Etelson <[email protected]>
---
 app/test-pmd/testpmd.c | 3 +++
 1 file changed, 3 insertions(+)
app/test-pmd/testpmd.c +3 -0
index 33fc0fddf5..0bb192b2f5 100644
@@ -2806,6 +2806,9 @@ stop_port(portid_t pid)
 			}
 		}
 
+		if (port->flow_list)
+			port_flow_flush(pi);
+
 		if (rte_eth_dev_stop(pi) != 0)
 			RTE_LOG(ERR, EAL, "rte_eth_dev_stop failed for port %u\n",
 				pi);
Signature · 1 line
2.29.2
#4 lore raw

Re: [dpdk-dev] [PATCH v2] app/testpmd: fix testpmd flows left before port stop.

Quoted 22 lines · Gregory Etelson

According to RTE flow user guide, PMD will not keep flow rules after port stop. Application resources that refer to flow rules become obsolete after port stop and must not be used.
Testpmd maintains linked list of active flows for each port. Entries in that list are allocated dynamically and must be explicitly released to prevent memory leak.
The patch releases testpmd port flow_list that holds remaining flows before port is stopped.

Cc [email protected]
Signed-off-by Gregory Etelson <[email protected]>
---
 app/test-pmd/testpmd.c | 3 +++
 1 file changed, 3 insertions(+)
app/test-pmd/testpmd.c +0 -0
index 33fc0fddf5..0bb192b2f5 100644

app/test-pmd/testpmd.c stop_port(portid_t pid) · line 2814

Quoted 9 lines
app/test-pmd/testpmd.c +3 -0
@@ -2806,6 +2806,9 @@ stop_port(portid_t pid)
 			}
 		}
 
+		if (port->flow_list)
+			port_flow_flush(pi);
+
 		if (rte_eth_dev_stop(pi) != 0)
 			RTE_LOG(ERR, EAL, "rte_eth_dev_stop failed for port %u\n",
 				pi);

port_flow_flush() does rte_flow_flush() which is not strictly required. Description sounds like we should cleanup testpmd lists only.

author
#5 lore raw

Re: [dpdk-dev] [PATCH v2] app/testpmd: fix testpmd flows left before port stop.

Hello Andrew,

Quoted 31 lines · Gregory Etelson
Quoted 21 lines

According to RTE flow user guide, PMD will not keep flow rules after port stop. Application resources that refer to flow rules become obsolete after port stop and must not be used.
Testpmd maintains linked list of active flows for each port. Entries in that list are allocated dynamically and must be explicitly released to prevent memory leak.
The patch releases testpmd port flow_list that holds remaining flows before port is stopped.

Cc [email protected]
Signed-off-by Gregory Etelson <[email protected]>
---
 app/test-pmd/testpmd.c | 3 +++
 1 file changed, 3 insertions(+)
app/test-pmd/testpmd.c index renamed from app/test-pmd/testpmd.c +0 -0

33fc0fddf5..0bb192b2f5 100644

app/test-pmd/testpmd.c stop_port(portid_t pid) · line 2813

Quoted 8 lines
app/test-pmd/testpmd.c +3 -0
@@ -2806,6 +2806,9 @@ stop_port(portid_t pid)
                      }
              }
 
+             if (port->flow_list)
+                     port_flow_flush(pi);
+
              if (rte_eth_dev_stop(pi) != 0)
                      RTE_LOG(ERR, EAL, "rte_eth_dev_stop failed for

port %u\n",

Quoted 5 lines

app/test-pmd/testpmd.c stop_port(portid_t pid) · line 2814

Quoted 2 lines
app/test-pmd/testpmd.c +0 -0
                              pi);

port_flow_flush() does rte_flow_flush() which is not strictly required. Description sounds like we should cleanup testpmd lists only.

You are right, call to rte_flow_flush() is not required, if testpmd calls port_flow_flush() as part of port stop sequence, because PMD will remove flows in that scenario. port_flow_flush() has a general implementation. It destroys all flows without port state consideration - current or future. In this form, port_flow_flush() can be called from any testpmd scenario that needs flows destruction.

Regards,
Gregory

Acked-by Ajit Khaparde Acked-by Ori Kam on v2 1/1 Applied
#6 lore raw

Re: [dpdk-dev] [dpdk-stable] [PATCH v2] app/testpmd: fix testpmd flows left before port stop.

In reply to Gregory Etelson
Quoted 9 lines · Gregory Etelson

According to RTE flow user guide, PMD will not keep flow rules after port stop. Application resources that refer to flow rules become obsolete after port stop and must not be used.
Testpmd maintains linked list of active flows for each port. Entries in that list are allocated dynamically and must be explicitly released to prevent memory leak.
The patch releases testpmd port flow_list that holds remaining flows before port is stopped.

Quoted 3 lines
Cc [email protected]
Signed-off-by Gregory Etelson <[email protected]>

Carrying acks from previous version:

Acked-by Ajit Khaparde <[email protected]>
Acked-by Ori Kam <[email protected]>

Applied to dpdk-next-net/main, thanks.