bugfix for testpmd
Subsystems (2)
cover bugfix for testpmd
This patchset contains three bugfixes for testpmd.
Huisong Li (3):
app/testpmd: fix port status of active slave device app/testpmd: fix slave device isn't released app/testpmd: remove unused header file
app/test-pmd/cmdline.c | 2 + app/test-pmd/parameters.c | 3 - app/test-pmd/testpmd.c | 116 ++++++++++++++++++++++++++++++++++---- app/test-pmd/testpmd.h | 4 +- 4 files changed, 109 insertions(+), 16 deletions(-)
Signature · 1 line
2.33.0
PATCH 1/3 app/testpmd: fix port status of active slave device
From: Huisong Li <[email protected]>
Stopping a bond device also stops all active slaves under the bond device. If this port is bond device, we need to modify the port status of all slaves from RTE_PORT_STARTED to RTE_PORT_STOPPED.
--- app/test-pmd/cmdline.c | 1 + app/test-pmd/testpmd.c | 49 +++++++++++++++++++++++++++++++++++++++--- app/test-pmd/testpmd.h | 3 ++- 3 files changed, 49 insertions(+), 4 deletions(-)
app/test-pmd/cmdline.c +1 -0
@@ -6639,6 +6639,7 @@ static void cmd_create_bonded_device_parsed(void *parsed_result,"Failed to enable promiscuous mode for port %u: %s - ignore\n" , port_id, rte_strerror(-ret)); + ports[port_id].bond_flag =1 ; ports[port_id].need_setup =0 ; ports[port_id].port_status =RTE_PORT_STOPPED ; }
app/test-pmd/testpmd.c +46 -3
67 lines not on this page · raw message
app/test-pmd/testpmd.h +2 -1
@@ -237,7 +237,8 @@ struct rte_port {struct rte_eth_txconf tx_conf[RTE_MAX_QUEUES_PER_PORT +1 ];/**< per queue tx configuration */ struct rte_ether_addr *mc_addr_pool;/**< pool of multicast addrs */ uint32_t mc_addr_nb;/**< nb. of addr. in mc_addr_pool */ -uint8_t slave_flag;/**< bonding slave port */ + uint8_t slave_flag : 1, /**< bonding slave port */ + bond_flag : 1; /**< port is bond device */struct port_flow *flow_list;/**< Associated flows. */ struct port_indirect_action *actions_list;/**< Associated indirect actions. */
Signature · 1 line
2.33.0
Re: [dpdk-dev] [PATCH 1/3] app/testpmd: fix port status of active slave device
Quoted 6 lines · Min Hu (Connor)
From: Huisong Li <[email protected]>
Stopping a bond device also stops all active slaves under the bond device. If this port is bond device, we need to modify the port status of all slaves from
RTE_PORT_STARTEDtoRTE_PORT_STOPPED.
Quoted 6 lines
Fixes 0e545d3047fe ("app/testpmd: check stopping port is not in bonding")Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>---
Hi Huisong,
Just wanted to check, if this behaviour can be taken care-off in bonding PMD itself. Or each application will need to in-corporate this change.
Thanks
Quoted 105 lines
app/test-pmd/cmdline.c | 1 + app/test-pmd/testpmd.c | 49 +++++++++++++++++++++++++++++++++++++++--- app/test-pmd/testpmd.h | 3 ++- 3 files changed, 49 insertions(+), 4 deletions(-)app/test-pmd/cmdline.c +1
-0@@ -6639,6 +6639,7 @@ static void cmd_create_bonded_device_parsed(void *parsed_result,"Failed to enable promiscuous mode for port %u: %s - ignore\n" , port_id, rte_strerror(-ret)); + ports[port_id].bond_flag =1 ; ports[port_id].need_setup =0 ; ports[port_id].port_status =RTE_PORT_STOPPED ; }app/test-pmd/testpmd.c +46
-3@@ -65,6 +65,9 @@#ifdef RTE_EXEC_ENV_WINDOWS #include <process.h> #endif +#ifdef RTE_NET_BOND +#include <rte_eth_bond.h> +#endif #include "testpmd.h" @@ -2986,6 +2989,35 @@ start_port(portid_t pid)return 0 ; } +#ifdef RTE_NET_BOND +static void +change_bonding_active_slave_port_status (portid_t bond_pid) +{ +portid_t slave_pids[RTE_MAX_ETHPORTS ]; +struct rte_port *port; +int num_active_slaves; +portid_t slave_pid; +int i; + + num_active_slaves =rte_eth_bond_active_slaves_get (bond_pid, slave_pids, +RTE_MAX_ETHPORTS ); +if (num_active_slaves <0 ) { +fprintf (stderr,"Failed to get slave list for port = %u\n" , + bond_pid); +return ; + } + +for (i =0 ; i < num_active_slaves; i++) { + slave_pid = slave_pids[i]; + port = &ports[slave_pid]; +if (rte_atomic16_cmpset (&(port->port_status), +RTE_PORT_STARTED ,RTE_PORT_STOPPED ) ==0 ) +fprintf (stderr,"Port %u can not be set into stopped\n" , + slave_pid); + } +} +#endif +void stop_port (portid_t pid) {@@ -3042,9 +3074,20 @@ stop_port(portid_t pid)if (port->flow_list)port_flow_flush (pi); -if (eth_dev_stop_mp (pi) !=0 ) -RTE_LOG (ERR ,EAL ,"rte_eth_dev_stop failed for port %u\n" , - pi); +if (is_proc_primary ()) { +#ifdef RTE_NET_BOND +/* +* Stopping a bond device also stops all active slaves +* under the bond device. If this port is bond device, +* we need to modify the port status of all slaves. +*/ +if (port->bond_flag ==1 ) +change_bonding_active_slave_port_status (pi); +#endif +if (rte_eth_dev_stop (pi) !=0 ) +RTE_LOG (ERR ,EAL ,"rte_eth_dev_stop failed for port %u\n" , + pi); + }if (rte_atomic16_cmpset (&(port->port_status),RTE_PORT_HANDLING ,RTE_PORT_STOPPED ) ==0 )app/test-pmd/testpmd.h +2
-1@@ -237,7 +237,8 @@ struct rte_port {struct rte_eth_txconf tx_conf[RTE_MAX_QUEUES_PER_PORT +1 ];/**< per queue tx configuration */ struct rte_ether_addr *mc_addr_pool;/**< pool of multicast addrs */ uint32_t mc_addr_nb;/**< nb. of addr. in mc_addr_pool */ -uint8_t slave_flag;/**< bonding slave port */ + uint8_t slave_flag : 1, /**< bonding slave port */ + bond_flag : 1; /**< port is bond device */struct port_flow *flow_list;/**< Associated flows. */ struct port_indirect_action *actions_list;/**< Associated indirect actions. */
Re: [dpdk-dev] [PATCH 1/3] app/testpmd: fix port status of active slave device
Quoted 14 lines · Aman Deep
Quoted 6 lines · Min Hu (Connor)
From: Huisong Li <[email protected]>
Stopping a bond device also stops all active slaves under the bond device. If this port is bond device, we need to modify the port status of all slaves from
RTE_PORT_STARTEDtoRTE_PORT_STOPPED.Quoted 6 lines
Fixes 0e545d3047fe ("app/testpmd: check stopping port is not in bonding")Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>---
Quoted 6 lines
Hi Huisong,
Just wanted to check, if this behaviour can be taken care-off in bonding PMD itself. Or each application will need to in-corporate this change.
Thanks
If an application maintains each port status, the application is responsible for it.
The bonding PMD driver does not know what needs to be changed in the application.
That's what I think.
Quoted 105 lines
Quoted 96 lines
app/test-pmd/cmdline.c | 1 + app/test-pmd/testpmd.c | 49 +++++++++++++++++++++++++++++++++++++++--- app/test-pmd/testpmd.h | 3 ++- 3 files changed, 49 insertions(+), 4 deletions(-)app/test-pmd/cmdline.c +0
-0@@ -6639,6 +6639,7 @@ static void cmd_create_bonded_device_parsed(void *parsed_result,"Failed to enable promiscuous mode for port %u: %s
- ignore\n",
port_id,rte_strerror(-ret));
+ ports[port_id].bond_flag = 1; ports[port_id].need_setup = 0; ports[port_id].port_status =RTE_PORT_STOPPED; }app/test-pmd/testpmd.c +3
-0@@ -65,6 +65,9 @@ #ifdef RTE_EXEC_ENV_WINDOWS #include <process.h> #endif +#ifdef RTE_NET_BOND +#include <rte_eth_bond.h> +#endif #include "testpmd.h" @@ -2986,6 +2989,35 @@ start_port(portid_t pid) return 0;}
+#ifdefRTE_NET_BONDapp/test-pmd/testpmd.c +10
-0+static void +change_bonding_active_slave_port_status (portid_t bond_pid) +{ + portid_t slave_pids[RTE_MAX_ETHPORTS]; + struct rte_port *port; + int num_active_slaves; + portid_t slave_pid; + int i; + + num_active_slaves = rte_eth_bond_active_slaves_get(bond_pid,
slave_pids,
+RTE_MAX_ETHPORTS);
+ if (num_active_slaves< 0) {
+fprintf(stderr, "Failed to get slave list for port = %u\n", + bond_pid);
+ return;
+ }app/test-pmd/testpmd.c +12
-0+ + for (i = 0; i < num_active_slaves; i++) { + slave_pid = slave_pids[i]; + port = &ports[slave_pid]; + if (rte_atomic16_cmpset(&(port->port_status), + RTE_PORT_STARTED, RTE_PORT_STOPPED) == 0) + fprintf(stderr, "Port %u can not be set into stopped\n", + slave_pid); + } +} +#endif +void
stop_port(portid_t pid)
{app/test-pmd/testpmd.c +14
-2@@ -3042,9 +3074,20 @@ stop_port(portid_t pid) if (port->flow_list) port_flow_flush(pi); - if (eth_dev_stop_mp(pi) != 0) - RTE_LOG(ERR, EAL, "rte_eth_dev_stop failed for port %u\n", - pi); + if (is_proc_primary()) { +#ifdef RTE_NET_BOND + /* + * Stopping a bond device also stops all active slaves + * under the bond device. If this port is bond device, + * we need to modify the port status of all slaves. + */ + if (port->bond_flag == 1) + change_bonding_active_slave_port_status(pi); +#endif + if (rte_eth_dev_stop(pi) != 0) + RTE_LOG(ERR, EAL, "rte_eth_dev_stop failed for port %u\n", + pi); + } if (rte_atomic16_cmpset(&(port->port_status), RTE_PORT_HANDLING, RTE_PORT_STOPPED) == 0)app/test-pmd/testpmd.h +0
-0@@ -237,7 +237,8 @@ struct rte_port {struct
rte_eth_txconftx_conf[RTE_MAX_QUEUES_PER_PORT+1]; /**< per queue tx configuration */Quoted 8 lines
struct rte_ether_addr
*mc_addr_pool; /**< pool of multicast addrs */ uint32_tmc_addr_nb; /**< nb. of addr. inmc_addr_pool*/ - uint8_tslave_flag; /**< bonding slave port */ + uint8_tslave_flag: 1, /**< bonding slave port */ +bond_flag: 1; /**< port is bond device */ struct port_flow*flow_list; /**< Associated flows. */ structport_indirect_action*actions_list;
/**< Associated indirect actions. */.
Singh, Aman Deep
Acked-byRe: [dpdk-dev] [PATCH 1/3] app/testpmd: fix port status of active slave device
Quoted 9 lines · lihuisong (C)
Quoted 7 lines · Aman Deep
Quoted 6 lines · Min Hu (Connor)
From: Huisong Li <[email protected]>
Stopping a bond device also stops all active slaves under the bond device. If this port is bond device, we need to modify the port status of all slaves from
RTE_PORT_STARTEDtoRTE_PORT_STOPPED.
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Fixes 0e545d3047fe ("app/testpmd: check stopping port is not in bonding")Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>---
Looks good to me.
Re: [dpdk-dev] [PATCH 1/3] app/testpmd: fix port status of active slave device
In reply to Min Hu (Connor)Quoted 44 lines · Min Hu (Connor)
From: Huisong Li <[email protected]>
Stopping a bond device also stops all active slaves under the bond device. If this port is bond device, we need to modify the port status of all slaves from
RTE_PORT_STARTEDtoRTE_PORT_STOPPED.Fixes 0e545d3047fe ("app/testpmd: check stopping port is not in bonding")Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>--- app/test-pmd/cmdline.c | 1 + app/test-pmd/testpmd.c | 49 +++++++++++++++++++++++++++++++++++++++--- app/test-pmd/testpmd.h | 3 ++- 3 files changed, 49 insertions(+), 4 deletions(-)app/test-pmd/cmdline.c +1
-0@@ -6639,6 +6639,7 @@ static void cmd_create_bonded_device_parsed(void *parsed_result,"Failed to enable promiscuous mode for port %u: %s - ignore\n" , port_id, rte_strerror(-ret)); + ports[port_id].bond_flag =1 ; ports[port_id].need_setup =0 ; ports[port_id].port_status =RTE_PORT_STOPPED ; }app/test-pmd/testpmd.c +3
-0@@ -65,6 +65,9 @@#ifdef RTE_EXEC_ENV_WINDOWS #include <process.h> #endif +#ifdef RTE_NET_BOND +#include <rte_eth_bond.h> +#endif #include "testpmd.h"
app/test-pmd/testpmd.c start_port(portid_t pid) · line 2994
Quoted 6 lines
app/test-pmd/testpmd.c +3
-0@@ -2986,6 +2989,35 @@ start_port(portid_t pid)return 0 ; } +#ifdef RTE_NET_BOND +static void +change_bonding_active_slave_port_status (portid_t bond_pid)
The function sets the status explicitly to PORT_STOPPED, but function name is more generic, should we update the function name to reflect the functionality?
Quoted 30 lines
app/test-pmd/testpmd.c +26
-0+{ +portid_t slave_pids[RTE_MAX_ETHPORTS ]; +struct rte_port *port; +int num_active_slaves; +portid_t slave_pid; +int i; + + num_active_slaves =rte_eth_bond_active_slaves_get (bond_pid, slave_pids, +RTE_MAX_ETHPORTS ); +if (num_active_slaves <0 ) { +fprintf (stderr,"Failed to get slave list for port = %u\n" , + bond_pid); +return ; + } + +for (i =0 ; i < num_active_slaves; i++) { + slave_pid = slave_pids[i]; + port = &ports[slave_pid]; +if (rte_atomic16_cmpset (&(port->port_status), +RTE_PORT_STARTED ,RTE_PORT_STOPPED ) ==0 ) +fprintf (stderr,"Port %u can not be set into stopped\n" , + slave_pid); + } +} +#endif +void stop_port (portid_t pid) {
app/test-pmd/testpmd.c stop_port(portid_t pid) · line 3047
Quoted 6 lines
app/test-pmd/testpmd.c +0
-3@@ -3042,9 +3074,20 @@ stop_port(portid_t pid)if (port->flow_list)port_flow_flush (pi); -if (eth_dev_stop_mp (pi) !=0 ) -RTE_LOG (ERR ,EAL ,"rte_eth_dev_stop failed for port %u\n" , - pi);
Can you please remove the 'eth_dev_stop_mp()' function in this patch, which is removed in patch 2/3.
app/test-pmd/testpmd.c stop_port(portid_t pid) · line 3089
Quoted 13 lines
app/test-pmd/testpmd.c +13
-0+if (is_proc_primary ()) { +#ifdef RTE_NET_BOND +/* +* Stopping a bond device also stops all active slaves +* under the bond device. If this port is bond device, +* we need to modify the port status of all slaves. +*/ +if (port->bond_flag ==1 ) +change_bonding_active_slave_port_status (pi); +#endif +if (rte_eth_dev_stop (pi) !=0 ) +RTE_LOG (ERR ,EAL ,"rte_eth_dev_stop failed for port %u\n" , + pi);
Should we roll back the slave port status if 'rte_eth_dev_stop(pi)' fails?
Quoted 9 lines
app/test-pmd/testpmd.c +1
-0+ }if (rte_atomic16_cmpset (&(port->port_status),RTE_PORT_HANDLING ,RTE_PORT_STOPPED ) ==0 )app/test-pmd/testpmd.h +0
-0
app/test-pmd/testpmd.h struct rte_port { · line 241
Quoted 6 lines
app/test-pmd/testpmd.h +2
-1@@ -237,7 +237,8 @@ struct rte_port {struct rte_eth_txconf tx_conf[RTE_MAX_QUEUES_PER_PORT +1 ];/**< per queue tx configuration */ struct rte_ether_addr *mc_addr_pool;/**< pool of multicast addrs */ uint32_t mc_addr_nb;/**< nb. of addr. in mc_addr_pool */ -uint8_t slave_flag;/**< bonding slave port */ + uint8_t slave_flag : 1, /**< bonding slave port */ + bond_flag : 1; /**< port is bond device */
Can't we detect if the port is a bonding port without introducing a new variable/state?
Quoted 3 lines
app/test-pmd/testpmd.h +0
-0struct port_flow *flow_list;/**< Associated flows. */ struct port_indirect_action *actions_list;/**< Associated indirect actions. */
lihuisong (C)
Acked-by Chas WilliamsRe: [dpdk-dev] [PATCH 1/3] app/testpmd: fix port status of active slave device
Quoted 45 lines · Ferruh Yigit
Quoted 44 lines · Min Hu (Connor)
From: Huisong Li <[email protected]>
Stopping a bond device also stops all active slaves under the bond device. If this port is bond device, we need to modify the port status of all slaves from
RTE_PORT_STARTEDtoRTE_PORT_STOPPED.Fixes 0e545d3047fe ("app/testpmd: check stopping port is not in bonding")Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>---
app/test-pmd/cmdline.c | 1 + app/test-pmd/testpmd.c | 49 +++++++++++++++++++++++++++++++++++++++--- app/test-pmd/testpmd.h | 3 ++- 3 files changed, 49 insertions(+), 4 deletions(-)app/test-pmd/cmdline.c +0
-0@@ -6639,6 +6639,7 @@ static void cmd_create_bonded_device_parsed(void *parsed_result,"Failed to enable promiscuous mode for port %u: %s
- ignore\n",
port_id,rte_strerror(-ret));
+ ports[port_id].bond_flag = 1; ports[port_id].need_setup = 0; ports[port_id].port_status =RTE_PORT_STOPPED; }app/test-pmd/testpmd.c +3
-0@@ -65,6 +65,9 @@ #ifdef RTE_EXEC_ENV_WINDOWS #include <process.h> #endif +#ifdef RTE_NET_BOND +#include <rte_eth_bond.h> +#endif #include "testpmd.h" @@ -2986,6 +2989,35 @@ start_port(portid_t pid) return 0;
Quoted 8 lines
app/test-pmd/testpmd.c start_port(portid_t pid) · line 2994
Quoted 4 lines
}
+#ifdefRTE_NET_BONDapp/test-pmd/testpmd.c +2
-0+static void +change_bonding_active_slave_port_status (portid_t bond_pid)The function sets the status explicitly to
PORT_STOPPED, but function name is more generic, should we update the function name to reflect the functionality?
ok
Quoted 32 lines
Quoted 32 lines
app/test-pmd/testpmd.c +8
-0+{ + portid_t slave_pids[RTE_MAX_ETHPORTS]; + struct rte_port *port; + int num_active_slaves; + portid_t slave_pid; + int i; + + num_active_slaves = rte_eth_bond_active_slaves_get(bond_pid,
slave_pids,
+RTE_MAX_ETHPORTS);
+ if (num_active_slaves< 0) {
+fprintf(stderr, "Failed to get slave list for port = %u\n", + bond_pid);
+ return;
+ }app/test-pmd/testpmd.c +12
-0+ + for (i = 0; i < num_active_slaves; i++) { + slave_pid = slave_pids[i]; + port = &ports[slave_pid]; + if (rte_atomic16_cmpset(&(port->port_status), + RTE_PORT_STARTED, RTE_PORT_STOPPED) == 0) + fprintf(stderr, "Port %u can not be set into stopped\n", + slave_pid); + } +} +#endif +void
stop_port(portid_t pid)
{app/test-pmd/testpmd.c +0
-0@@ -3042,9 +3074,20 @@ stop_port(portid_t pid)
Quoted 8 lines
app/test-pmd/testpmd.c stop_port(portid_t pid) · line 3047
Quoted 5 lines
app/test-pmd/testpmd.c +0
-2if (port->flow_list) port_flow_flush(pi); - if (eth_dev_stop_mp(pi) != 0) - RTE_LOG(ERR, EAL, "rte_eth_dev_stop failed for port %u\n", - pi);Can you please remove the '
eth_dev_stop_mp()' function in this patch, which is removed in patch 2/3.
ok
Quoted 17 lines
Quoted 5 lines
app/test-pmd/testpmd.c +4
-0+ if (is_proc_primary()) { +#ifdef RTE_NET_BOND + /* +* Stopping a bond device also stops all active slaves app/test-pmd/testpmd.c stop_port(portid_t pid) · line 3089
Quoted 10 lines
app/test-pmd/testpmd.c +9
-0+* under the bond device. If this port is bond device, +* we need to modify the port status of all slaves. +*/ + if (port->bond_flag == 1) + change_bonding_active_slave_port_status(pi); +#endif + if (rte_eth_dev_stop(pi) != 0) + RTE_LOG(ERR, EAL, "rte_eth_dev_stop failed for port %u\n", + pi);Should we roll back the slave port status if '
rte_eth_dev_stop(pi)' fails?
Yes, it is necessary here for slaves to fail to execute dev_stop() in bonding driver.
Btw, in thinking about this, I find a behavior that is not very reasonable. Namely, only active slaves are stopped when a bonding device is stopped. It can cause confusion in port status. For example, applications have to only modify active slaves status to RTE_PORT_STOPPED and non-active slaves status is still RTE_PORT_STARTED.
I think the bonding PMD should stop all slaves when a bonding device is stopped. I checked the modification history about this in the bonding PMD. This behavior is introduced by the following patch.
/*
commit 0911d4ec01839c9149a0df5758d00d9d57a47cea
Author: Radu Nicolau <[email protected]>
Date: Thu Nov 8 15:26:42 2018 +0000
net/bonding: fix crash when stopping mode 4 port
When stopping a bonded port all slaves are deactivated. Attempting to deactivate a slave that was never activated will result in a segfault when mode 4 is used.
*/
The root cause of the problem the above patch mentioned is that in mode 4,
the bonding PMD does not allocate rx/tx rings to non-active slave devices.
The call stack is as follows:
#0 0x0000000000b1250c in rte_ring_dequeue_bulk_elem (available=0x0, n=1, esize=8, obj_table=0xffffffff7c80, r=0x0) at ../dpdk-next-net/lib/ring/rte_ring_elem.h:380 #1 rte_ring_dequeue_elem (esize=8, obj_p=0xffffffff7c80, r=0x0) at ../dpdk-next-net/lib/ring/rte_ring_elem.h:476
#2 rte_ring_dequeue (obj_p=0xffffffff7c80, r=0x0) at ../dpdk-next-net/lib/ring/rte_ring.h:463
#3 bond_mode_8023ad_deactivate_slave (bond_dev=0x4753200 <rte_eth_devices+33024>, slave_id=0) at ../dpdk-next-net/drivers/net/bonding/rte_eth_bond_8023ad.c:1163 #4 0x0000000000b29e10 in deactivate_slave (eth_dev=0x4753200 <rte_eth_devices+33024>, port_id=0) at ../dpdk-next-net/drivers/net/bonding/rte_eth_bond_api.c:117 #5 0x0000000000b44208 in bond_ethdev_stop (eth_dev=0x4753200 <rte_eth_devices+33024>) at ../dpdk-next-net/drivers/net/bonding/rte_eth_bond_pmd.c:2103 #6 0x00000000007966fc in rte_eth_dev_stop (port_id=2) at ../dpdk-next-net/lib/ethdev/rte_ethdev.c:1894
#7 0x000000000055ea60 in eth_dev_stop_mp (port_id=2) at ../dpdk-next-net/app/test-pmd/testpmd.c:613
#8 0x0000000000565230 in stop_port (pid=2) at ../dpdk-next-net/app/test-pmd/testpmd.c:3059
#9 0x00000000004f7614 in cmd_operate_specific_port_parsed (parsed_result=0xffffffff91b0, cl=0x4829250, data=0x0) at ../dpdk-next-net/app/test-pmd/cmdline.c:1261 #10 0x000000000078be24 in cmdline_parse (cl=0x4829250, buf=0x4829298 "port stop 2\n") at ../dpdk-next-net/lib/cmdline/cmdline_parse.c:290 #11 0x0000000000789c34 in cmdline_valid_buffer (rdl=0x4829260, buf=0x4829298 "port stop 2\n", size=13) at ../dpdk-next-net/lib/cmdline/cmdline.c:26 #12 0x000000000078f160 in rdline_char_in (rdl=0x4829260, c=10 '\n') at ../dpdk-next-net/lib/cmdline/cmdline_rdline.c:446
#13 0x000000000078a0c8 in cmdline_in (cl=0x4829250, buf=0xfffffffff2e7 "\n", size=1) at ../dpdk-next-net/lib/cmdline/cmdline.c:148
#14 0x000000000078a3b4 in cmdline_interact (cl=0x4829250) at ../dpdk-next-net/lib/cmdline/cmdline.c:222
#15 0x000000000050bf98 in prompt () at ../dpdk-next-net/app/test-pmd/cmdline.c:18001
#16 0x00000000005687c4 in main (argc=4, argv=0xfffffffff510) at ../dpdk-next-net/app/test-pmd/testpmd.c:4268
For the problem Radu encountered, we only need to ensure that non-active slaves doesn't deactivate.
I plan to add a patch in this patchset to fix this problem. What do you think, Ferruh?
Quoted 10 lines
Quoted 10 lines
app/test-pmd/testpmd.c +1
-0+ } if (rte_atomic16_cmpset(&(port->port_status), RTE_PORT_HANDLING, RTE_PORT_STOPPED) == 0)app/test-pmd/testpmd.h +0
-0@@ -237,7 +237,8 @@ struct rte_port {struct
rte_eth_txconftx_conf[RTE_MAX_QUEUES_PER_PORT+1]; /**< per queue tx configuration */
Quoted 8 lines
Quoted 5 lines
struct rte_ether_addr
*mc_addr_pool; /**< pool of multicast addrs */ uint32_tmc_addr_nb; /**< nb. of addr. inmc_addr_pool*/ - uint8_tslave_flag; /**< bonding slave port */ + uint8_tslave_flag: 1, /**< bonding slave port */ +bond_flag: 1; /**< port is bond device */Can't we detect if the port is a bonding port without introducing a new variable/state?
The bonding device is also an ethdev. I do not find the external API that can be used to detect whether a port is a bonding port.
Quoted 6 lines
Quoted 4 lines
struct port_flow
*flow_list; /**< Associated flows. */ structport_indirect_action*actions_list;
/**< Associated indirect actions. */.
PATCH 2/3 app/testpmd: fix slave device isn't released
In reply to Min Hu (Connor)From: Huisong Li <[email protected]>
Currently, some eth devices are added to bond device, these devices are not released when the quit command is executed in testpmd. This patch adds the release operation for all active slaves under a bond device.
--- app/test-pmd/cmdline.c | 1 + app/test-pmd/testpmd.c | 67 ++++++++++++++++++++++++++++++++++++------ app/test-pmd/testpmd.h | 1 + 3 files changed, 60 insertions(+), 9 deletions(-)
app/test-pmd/cmdline.c +1 -0
@@ -8743,6 +8743,7 @@ static void cmd_quit_parsed(__rte_unused void *parsed_result, __rte_unused void *data) {cmdline_quit (cl); + cl_quit =1 ; } cmdline_parse_token_string_t cmd_quit_quit =
app/test-pmd/testpmd.c +58 -9
91 lines not on this page · raw message
app/test-pmd/testpmd.h +1 -0
@@ -27,6 +27,7 @@#define RTE_PORT_STARTED (uint16_t)1#define RTE_PORT_CLOSED (uint16_t)2#define RTE_PORT_HANDLING (uint16_t)3 +extern uint8_t cl_quit;/* * It is used to allocate the memory for hash key.
Signature · 1 line
2.33.0
Re: [dpdk-dev] [PATCH 2/3] app/testpmd: fix slave device isn't released
Quoted 6 lines · Min Hu (Connor)
From: Huisong Li <[email protected]>
Currently, some eth devices are added to bond device, these devices are not released when the quit command is executed in testpmd. This patch adds the release operation for all active slaves under a bond device.
'close_port()' function traverses all ports, when bonding is close, if it removes the slaves, won't 'close_port()' close slave devices?
If so this prevents, 'cl_quit' global variable.
Quoted 52 lines
Fixes 0e545d3047fe ("app/testpmd: check stopping port is not in bonding")Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>--- app/test-pmd/cmdline.c | 1 + app/test-pmd/testpmd.c | 67 ++++++++++++++++++++++++++++++++++++------ app/test-pmd/testpmd.h | 1 + 3 files changed, 60 insertions(+), 9 deletions(-)app/test-pmd/cmdline.c +1
-0@@ -8743,6 +8743,7 @@ static void cmd_quit_parsed(__rte_unused void *parsed_result, __rte_unused void *data) {cmdline_quit (cl); + cl_quit =1 ; } cmdline_parse_token_string_t cmd_quit_quit =app/test-pmd/testpmd.c +1
-9@@ -221,6 +221,7 @@ unsigned int xstats_display_num; /**< Size of extended statistics to show */* option. Set flag to exit stats period loop after received SIGINT/SIGTERM. */ uint8_t f_quit; +uint8_t cl_quit;/* Quit testpmd from cmdline. */ /* * Max Rx frame size, set by '--max-pkt-len' parameter. @@ -613,15 +614,6 @@ eth_dev_start_mp(uint16_t port_id)return 0 ; } -static int -eth_dev_stop_mp (uint16_t port_id) -{ -if (is_proc_primary ()) -return rte_eth_dev_stop (port_id); - -return 0 ; -} -static void mempool_free_mp (struct rte_mempool *mp) {
app/test-pmd/testpmd.c remove_invalid_ports(void) · line 3120
Quoted 6 lines
app/test-pmd/testpmd.c +3
-0@@ -3123,6 +3115,55 @@ remove_invalid_ports(void) nb_cfg_ports = nb_fwd_ports; } +#ifdef RTE_NET_BOND +static void +handle_bonding_slave_device (portid_t bond_pid)
'handle' in the function is not very specific, it is not clear what this function does.
Quoted 25 lines
app/test-pmd/testpmd.c +25
-0+{ +portid_t slave_pids[RTE_MAX_ETHPORTS ]; +struct rte_port *port; +portid_t slave_pid; +int num_slaves; +int i; + + num_slaves =rte_eth_bond_slaves_get (bond_pid, slave_pids, +RTE_MAX_ETHPORTS ); +if (num_slaves <0 ) { +fprintf (stderr,"Failed to get slave list for port = %u\n" , + bond_pid); +return ; + } + +for (i =0 ; i < num_slaves; i++) { + slave_pid = slave_pids[i]; +/* Before removing a slave device, stop the slave device. */ +if (port_is_started (slave_pid) ==1 ) { +if (rte_eth_dev_stop (slave_pid) !=0 ) +fprintf (stderr,"rte_eth_dev_stop failed for port %u\n" , + slave_pid); + + port = &ports[slave_pid]; + if (rte_atomic16_cmpset(&(port->port_status),
app/test-pmd/testpmd.c remove_invalid_ports(void) · line 3155
Quoted 10 lines
app/test-pmd/testpmd.c +10
-0+ RTE_PORT_STARTED, RTE_PORT_STOPPED) == 0) +fprintf (stderr,"Port %u can not be set into stopped\n" , + slave_pid); + } + +/* +* Remove the slave from a bonded device in case of failing to +* close bond device. +*/ +if (rte_eth_bond_slave_remove (bond_pid, slave_pid) !=0 )
Similar to Aman's comment in previous patch, if a bonding device is removed shouldn't the bonding PMD stop the slaves and removed them, instead of application?
Quoted 41 lines
app/test-pmd/testpmd.c +19
-0+fprintf (stderr,"Failed to remove slave %u from master port = %u\n" , + slave_pid, bond_pid); +clear_port_slave_flag (slave_pid); + +/* Close slave device when testpmd quit or is killed. */ +if (cl_quit ==1 || f_quit ==1 ) + rte_eth_dev_close(slave_pid); + } +} +#endif +void close_port (portid_t pid) {@@ -3161,6 +3202,14 @@ close_port(portid_t pid)if (is_proc_primary ()) {port_flow_flush (pi); +#ifdef RTE_NET_BOND +/* +* If this port is bond device, all slaves under the +* device need to be removed or closed. +*/ +if (port->bond_flag ==1 ) +handle_bonding_slave_device (pi); +#endif port_flex_item_flush (pi);rte_eth_dev_close (pi); }app/test-pmd/testpmd.h +1
-0@@ -27,6 +27,7 @@#define RTE_PORT_STARTED (uint16_t)1#define RTE_PORT_CLOSED (uint16_t)2#define RTE_PORT_HANDLING (uint16_t)3 +extern uint8_t cl_quit;/* * It is used to allocate the memory for hash key.
Re: [dpdk-dev] [PATCH 2/3] app/testpmd: fix slave device isn't released
Quoted 10 lines · Ferruh Yigit
Quoted 6 lines · Min Hu (Connor)
From: Huisong Li <[email protected]>
Currently, some eth devices are added to bond device, these devices are not released when the quit command is executed in testpmd. This patch adds the release operation for all active slaves under a bond device.
'
close_port()' function traverses all ports, when bonding is close, if it removes the slaves, won't 'close_port()' close slave devices?
Yes
Quoted 2 lines
If so this prevents, '
cl_quit' global variable.
The variable is used to ensure that the slave ports are not closed when the bonding port is closed, and are closed when testpmd quit or is killed.
Quoted 52 lines
Quoted 52 lines
Fixes 0e545d3047fe ("app/testpmd: check stopping port is not in bonding")Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>---
app/test-pmd/cmdline.c | 1 + app/test-pmd/testpmd.c | 67 ++++++++++++++++++++++++++++++++++++------ app/test-pmd/testpmd.h | 1 + 3 files changed, 60 insertions(+), 9 deletions(-)app/test-pmd/cmdline.c +1
-0@@ -8743,6 +8743,7 @@ static void cmd_quit_parsed(__rte_unused void *parsed_result, __rte_unused void *data) { cmdline_quit(cl); + cl_quit = 1; }
cmdline_parse_token_string_tcmd_quit_quit=app/test-pmd/testpmd.c +0
-0@@ -221,6 +221,7 @@ unsigned int xstats_display_num; /**< Size ofextended statistics to show */
* option. Set flag to exit stats period loop after received SIGINT/SIGTERM. */
uint8_tf_quit;
+uint8_tcl_quit; /* Quit testpmd from cmdline. */
/*
* Max Rx frame size, set by '--max-pkt-len' parameter.app/test-pmd/testpmd.c +0
-0@@ -613,15 +614,6 @@ eth_dev_start_mp(uint16_t port_id) return 0; }-static int
app/test-pmd/testpmd.c +0
-8-eth_dev_stop_mp(uint16_t port_id) -{ - if (is_proc_primary()) - return rte_eth_dev_stop(port_id); - - return 0; -} -static void
mempool_free_mp(struct rte_mempool *mp)
{app/test-pmd/testpmd.c +0
-0@@ -3123,6 +3115,55 @@ remove_invalid_ports(void)
Quoted 8 lines
app/test-pmd/testpmd.c remove_invalid_ports(void) · line 3120
Quoted 5 lines
app/test-pmd/testpmd.c +0
-0nb_cfg_ports = nb_fwd_ports; }+#ifdef
RTE_NET_BONDapp/test-pmd/testpmd.c +2
-0+static void +handle_bonding_slave_device (portid_t bond_pid)'handle' in the function is not very specific, it is not clear what this function does.
ok
Quoted 33 lines
Quoted 33 lines
app/test-pmd/testpmd.c +21
-0+{ + portid_t slave_pids[RTE_MAX_ETHPORTS]; + struct rte_port *port; + portid_t slave_pid; + int num_slaves; + int i; + + num_slaves = rte_eth_bond_slaves_get(bond_pid, slave_pids, + RTE_MAX_ETHPORTS); + if (num_slaves < 0) { + fprintf(stderr, "Failed to get slave list for port = %u\n", + bond_pid); + return; + } + + for (i = 0; i < num_slaves; i++) { + slave_pid = slave_pids[i]; + /* Before removing a slave device, stop the slave device. */ + if (port_is_started(slave_pid) == 1) { + if (rte_eth_dev_stop(slave_pid) != 0) + fprintf(stderr, "rte_eth_dev_stop failed for port%u\n",
+slave_pid);
+
+ port = &ports[slave_pid];
+ if (rte_atomic16_cmpset(&(port->port_status), + RTE_PORT_STARTED, RTE_PORT_STOPPED)== 0) + fprintf(stderr, "Port %u can not be set into stopped\n",
+slave_pid);
+ }
+
Quoted 8 lines
Quoted 5 lines
+ /*
+ * Remove the slave from a bonded device in case of failing to + * close bond device.
+ */
+ if (rte_eth_bond_slave_remove(bond_pid, slave_pid)!= 0)Similar to Aman's comment in previous patch, if a bonding device is removed shouldn't the bonding PMD stop the slaves and removed them, instead of application?
I agree. I plan to remove them, and move the operations of clearing the slave flag
and closing slave port to after the bonding port is closed.
Quoted 43 lines
Quoted 35 lines
+ fprintf(stderr, "Failed to remove slave %u from master port = %u\n",
+slave_pid,bond_pid);
+clear_port_slave_flag(slave_pid);app/test-pmd/testpmd.c +8
-0+ + /* Close slave device when testpmd quit or is killed. */ + if (cl_quit == 1 || f_quit == 1) + rte_eth_dev_close(slave_pid); + } +} +#endif +void
close_port(portid_t pid)
{app/test-pmd/testpmd.c +8
-0@@ -3161,6 +3202,14 @@ close_port(portid_t pid) if (is_proc_primary()) { port_flow_flush(pi); +#ifdef RTE_NET_BOND + /* + * If this port is bond device, all slaves under the + * device need to be removed or closed. + */ + if (port->bond_flag == 1) + handle_bonding_slave_device(pi); +#endif port_flex_item_flush(pi); rte_eth_dev_close(pi); }app/test-pmd/testpmd.h +0
-0app/test-pmd/testpmd.h line 32
Quoted 6 lines
app/test-pmd/testpmd.h +1
-0@@ -27,6 +27,7 @@ #define RTE_PORT_STARTED (uint16_t)1 #define RTE_PORT_CLOSED (uint16_t)2 #define RTE_PORT_HANDLING (uint16_t)3 +extern uint8_t cl_quit; /** It is used to allocate the memory for hash key. .
PATCH 3/3 app/testpmd: remove unused header file
In reply to Min Hu (Connor)From: Huisong Li <[email protected]>
This patch removes unused "rte_eth_bond.h" header file.
--- app/test-pmd/parameters.c | 3 --- 1 file changed, 3 deletions(-)
app/test-pmd/parameters.c +0 -3
@@ -38,9 +38,6 @@#include <rte_ether.h> #include <rte_ethdev.h> #include <rte_string_fns.h> -#ifdef RTE_NET_BOND -#include <rte_eth_bond.h> -#endif #include <rte_flow.h> #include "testpmd.h"
Signature · 1 line
2.33.0
Re: [dpdk-dev] [PATCH 3/3] app/testpmd: remove unused header file
Quoted 6 lines · Min Hu (Connor)
From: Huisong Li <[email protected]>
This patch removes unused "
rte_eth_bond.h" header file.Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>
Fixes: 2950a769315e ("bond: testpmd support")
Cc: [email protected]Only for this patch, not series (since this is trivial), Applied to dpdk-next-net/main, thanks.
v2 cover bugfix for bonding
In reply to Min Hu (Connor)From: Huisong Li <[email protected]>
Fix some bonding related bug for bonding PMD and testpmd.
v2:
- add two patches in net/bonding.
- change port status of slave device in start_port
- delete removing slave operation when close bond port
Huisong Li (4):
net/bonding: fix non-active slaves aren't stopped net/bonding: fix non-terminable while loop app/testpmd: fix port status of slave device app/testpmd: fix slave device isn't released
app/test-pmd/cmdline.c | 2 + app/test-pmd/testpmd.c | 122 ++++++++++++++++++++++++- app/test-pmd/testpmd.h | 5 +- drivers/net/bonding/rte_eth_bond_pmd.c | 23 +++-- 4 files changed, 137 insertions(+), 15 deletions(-)
Signature · 1 line
2.33.0
PATCH v2 1/4 net/bonding: fix non-active slaves aren't stopped
From: Huisong Li <[email protected]>
When stopping a bonded port, all slaves should be deactivated. But only active slaves are stopped. So fix it and do "deactivae_slave()" for active slaves.
--- drivers/net/bonding/rte_eth_bond_pmd.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)
drivers/net/bonding/rte_eth_bond_pmd.c +11 -9
@@ -2118,18 +2118,20 @@ bond_ethdev_stop(struct rte_eth_dev *eth_dev) internals->link_status_polling_enabled =0 ;for (i =0 ; i < internals->slave_count; i++) {uint16_t slave_id = internals->slaves[i].port_id; + + internals->slaves[i].last_link_status =0 ; + ret =rte_eth_dev_stop (slave_id); +if (ret !=0 ) { +RTE_BOND_LOG (ERR ,"Failed to stop device on port %u" , + slave_id); +return ret; + } + +/* active slaves need to deactivate. */ if (find_slave_by_id (internals->active_slaves, internals->active_slave_count, slave_id) != - internals->active_slave_count) { - internals->slaves[i].last_link_status =0 ; - ret =rte_eth_dev_stop (slave_id); -if (ret !=0 ) { -RTE_BOND_LOG (ERR ,"Failed to stop device on port %u" , - slave_id); -return ret; - } + internals->active_slave_count)deactivate_slave (eth_dev, slave_id); - } }return 0 ;
Signature · 1 line
2.33.0
Re: [PATCH V2 1/4] net/bonding: fix non-active slaves aren't stopped
Quoted 3 lines · Min Hu (Connor)
From: Huisong Li <[email protected]>
When stopping a bonded port, all slaves should be deactivated. But only
s/deactivated/stopped/ ?
Quoted 1 line
active slaves are stopped. So fix it and do "
deactivae_slave()" for active
s/deactivae_slave()/deactivate_slave()/
Quoted 1 line
slaves.
Hi Connor,
When a bonding port is closed, is it clear if all slave ports or active slave ports should be stopped?
Quoted 15 lines
Fixes 0911d4ec0183 ("net/bonding: fix crash when stopping mode 4 port")Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>--- drivers/net/bonding/rte_eth_bond_pmd.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)drivers/net/bonding/rte_eth_bond_pmd.c +0
-0
drivers/net/bonding/rte_eth_bond_pmd.c bond_ethdev_stop(struct rte_eth_dev *eth_dev) · line 2127
Quoted 10 lines
drivers/net/bonding/rte_eth_bond_pmd.c +7
-0@@ -2118,18 +2118,20 @@ bond_ethdev_stop(struct rte_eth_dev *eth_dev) internals->link_status_polling_enabled =0 ;for (i =0 ; i < internals->slave_count; i++) {uint16_t slave_id = internals->slaves[i].port_id; + + internals->slaves[i].last_link_status =0 ; + ret =rte_eth_dev_stop (slave_id); +if (ret !=0 ) { +RTE_BOND_LOG (ERR ,"Failed to stop device on port %u" , + slave_id); +return ret;
Should it return here or try to stop all ports?
What about to record the return status, but keep continue to stop all ports. And return error if any of the stop failed?
drivers/net/bonding/rte_eth_bond_pmd.c bond_ethdev_stop(struct rte_eth_dev *eth_dev) · line 2130
Quoted 3 lines
drivers/net/bonding/rte_eth_bond_pmd.c +3
-0+ } + +/* active slaves need to deactivate. */
" active slaves need to be deactivated. " ?
drivers/net/bonding/rte_eth_bond_pmd.c bond_ethdev_stop(struct rte_eth_dev *eth_dev) · line 2133
Quoted 11 lines
drivers/net/bonding/rte_eth_bond_pmd.c +1
-8if (find_slave_by_id (internals->active_slaves, internals->active_slave_count, slave_id) != - internals->active_slave_count) { - internals->slaves[i].last_link_status =0 ; - ret =rte_eth_dev_stop (slave_id); -if (ret !=0 ) { -RTE_BOND_LOG (ERR ,"Failed to stop device on port %u" , - slave_id); -return ret; - } + internals->active_slave_count)
I think original indentation for this line is better.
Quoted 5 lines
drivers/net/bonding/rte_eth_bond_pmd.c +0
-1deactivate_slave (eth_dev, slave_id); - } }return 0 ;
Re: [PATCH V2 1/4] net/bonding: fix non-active slaves aren't stopped
Hi, Ferruh,
Quoted 6 lines · Ferruh Yigit
Quoted 3 lines · Min Hu (Connor)
From: Huisong Li <[email protected]>
When stopping a bonded port, all slaves should be deactivated. But only
s/deactivated/stopped/ ?
not agreed. deactivated and stopped are different state for slave.
Quoted 5 lines
Quoted 2 lines
active slaves are stopped. So fix it and do "
deactivae_slave()" for actives/deactivae_slave()/deactivate_slave()/
agreed.
Quoted 5 lines
Quoted 1 line
slaves.
Hi Connor,
When a bonding port is closed, is it clear if all slave ports or active slave ports should be stopped?
Yes, I think all the slave ports should be stopped(or try to be stopped).
Quoted 21 lines
Quoted 21 lines
Fixes 0911d4ec0183 ("net/bonding: fix crash when stopping mode 4 port")Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>---
drivers/net/bonding/rte_eth_bond_pmd.c| 20 +++++++++++---------1 file changed, 11 insertions(+), 9 deletions(-)drivers/net/bonding/rte_eth_bond_pmd.c +2
-0@@ -2118,18 +2118,20 @@ bond_ethdev_stop(struct rte_eth_dev *eth_dev) internals->link_status_polling_enabled = 0; for (i = 0; i < internals->slave_count; i++) { uint16_t slave_id = internals->slaves[i].port_id; + + internals->slaves[i].last_link_status = 0;
Quoted 8 lines
drivers/net/bonding/rte_eth_bond_pmd.c bond_ethdev_stop(struct rte_eth_dev *eth_dev) · line 2127
Quoted 5 lines
drivers/net/bonding/rte_eth_bond_pmd.c +5
-0+ ret = rte_eth_dev_stop(slave_id); + if (ret != 0) { + RTE_BOND_LOG(ERR, "Failed to stop device on port %u", + slave_id); + return ret;Should it return here or try to stop all ports?
What about to record the return status, but keep continue to stop all ports. And return error if any of the stop failed?
I think no need to do this. APP only see the bonded device. If bonded
device stop failed, it means it works failed. And the number of "stopped" successfully slave does not make any sense.
Quoted 6 lines
drivers/net/bonding/rte_eth_bond_pmd.c bond_ethdev_stop(struct rte_eth_dev *eth_dev) · line 2130
Quoted 4 lines
drivers/net/bonding/rte_eth_bond_pmd.c +3
-0+ } + + /* active slaves need to deactivate. */" active slaves need to be deactivated. " ?
agreed.
Quoted 13 lines
Quoted 4 lines
drivers/net/bonding/rte_eth_bond_pmd.c +0
-0if (find_slave_by_id(internals->active_slaves,
internals->active_slave_count,slave_id) != -internals->active_slave_count) {Quoted 8 lines
- internals->slaves[i].last_link_status = 0;
- ret =rte_eth_dev_stop(slave_id);
- if (ret != 0) {
- RTE_BOND_LOG(ERR, "Failed to stop device on port %u", -slave_id);
- return ret;
- }
+internals->active_slave_count)
Quoted 2 lines
I think original indentation for this line is better.
agreed.
Quoted 6 lines
Quoted 4 lines
deactivate_slave(eth_dev, slave_id); - }
}
return 0;.
Re: [PATCH V2 1/4] net/bonding: fix non-active slaves aren't stopped
Quoted 11 lines · Min Hu (Connor)
Hi, Ferruh,
Quoted 6 lines · Ferruh Yigit
Quoted 3 lines · Min Hu (Connor)
From: Huisong Li <[email protected]>
When stopping a bonded port, all slaves should be deactivated. But only
s/deactivated/stopped/ ?
not agreed. deactivated and stopped are different state for slave.
Just to clarify the sentences, otherwise I see the 'stopped' and 'deactivated' states are different.
Next sentences complains that not all ports are stopped: "But only active slaves are stopped.", so I thought intention in this sentences to claim that all slaves should be stopped (but it mentions all slaves should be 'deactivated'). As long as you address the disconnection between two sentences, I don't mind the wording.
Quoted 42 lines
Quoted 5 lines
Quoted 2 lines
active slaves are stopped. So fix it and do "
deactivae_slave()" for actives/deactivae_slave()/deactivate_slave()/
agreed.
Quoted 5 lines
Quoted 1 line
slaves.
Hi Connor,
When a bonding port is closed, is it clear if all slave ports or active slave ports should be stopped?
Yes, I think all the slave ports should be stopped(or try to be stopped).
Quoted 21 lines
Quoted 21 lines
Fixes 0911d4ec0183 ("net/bonding: fix crash when stopping mode 4 port")Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>---
drivers/net/bonding/rte_eth_bond_pmd.c| 20 +++++++++++---------1 file changed, 11 insertions(+), 9 deletions(-)drivers/net/bonding/rte_eth_bond_pmd.c +2
-0@@ -2118,18 +2118,20 @@ bond_ethdev_stop(struct rte_eth_dev *eth_dev) internals->link_status_polling_enabled = 0; for (i = 0; i < internals->slave_count; i++) { uint16_t slave_id = internals->slaves[i].port_id; + + internals->slaves[i].last_link_status = 0;Quoted 8 lines
drivers/net/bonding/rte_eth_bond_pmd.c bond_ethdev_stop(struct rte_eth_dev *eth_dev) · line 2127
Quoted 5 lines
drivers/net/bonding/rte_eth_bond_pmd.c +5
-0+ ret = rte_eth_dev_stop(slave_id); + if (ret != 0) { + RTE_BOND_LOG(ERR, "Failed to stop device on port %u", + slave_id); + return ret;Should it return here or try to stop all ports?
What about to record the return status, but keep continue to stop all ports. And return error if any of the stop failed?
Quoted 3 lines
I think no need to do this. APP only see the bonded device. If bonded
device stop failed, it means it works failed. And the number of "stopped" successfully slave does not make any sense.
OK if trying to stop as much as possible 'slave' devices doesn't make sense, we can keep as it is.
Btw, when functions fails at this point, bonding device itself already marked as stopped, right? And some of the slave devices may be stopped already before failure.
I don't know how confusing this is for the user, that stop() function is failed but bonding device state is 'stopped'. I don't know if function should recover at least bonding device status (back to started) on failure, what do you think?
Quoted 29 lines
Quoted 6 lines
drivers/net/bonding/rte_eth_bond_pmd.c bond_ethdev_stop(struct rte_eth_dev *eth_dev) · line 2130
Quoted 4 lines
drivers/net/bonding/rte_eth_bond_pmd.c +3
-0+ } + + /* active slaves need to deactivate. */" active slaves need to be deactivated. " ?
agreed.
Quoted 13 lines
Quoted 4 lines
drivers/net/bonding/rte_eth_bond_pmd.c +0
-0if (find_slave_by_id(internals->active_slaves,
internals->active_slave_count,slave_id) != -internals->active_slave_count) {Quoted 8 lines
- internals->slaves[i].last_link_status = 0;
- ret =rte_eth_dev_stop(slave_id);
- if (ret != 0) {
- RTE_BOND_LOG(ERR, "Failed to stop device on port %u", -slave_id);
- return ret;
- }
+internals->active_slave_count)Quoted 2 lines
I think original indentation for this line is better.
agreed.
Quoted 6 lines
Quoted 4 lines
deactivate_slave(eth_dev, slave_id); - }
}
return 0;.
Re: [PATCH V2 1/4] net/bonding: fix non-active slaves aren't stopped
Hi, Ferruh,
Quoted 9 lines · Ferruh Yigit
Quoted 8 lines · Min Hu (Connor)
Hi, Ferruh,
Quoted 5 lines · Ferruh Yigit
Quoted 3 lines · Min Hu (Connor)
From: Huisong Li <[email protected]>
When stopping a bonded port, all slaves should be deactivated. But only
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
s/deactivated/stopped/ ?
not agreed. deactivated and stopped are different state for slave.
Just to clarify the sentences, otherwise I see the 'stopped' and 'deactivated' states are different.
Next sentences complains that not all ports are stopped: "But only active slaves are stopped.", so I thought intention in this sentences to claim that all slaves should be stopped (but it mentions all slaves should be 'deactivated'). As long as you address the disconnection between two sentences, I don't mind the wording.
Actually, there is something wrong with the wording. Yes, I should take your advice.
Quoted 35 lines
Quoted 35 lines
Quoted 6 lines
Quoted 3 lines
active slaves are stopped. So fix it and do "
deactivae_slave()" for actives/deactivae_slave()/deactivate_slave()/
agreed.
Quoted 5 lines
Quoted 1 line
slaves.
Hi Connor,
When a bonding port is closed, is it clear if all slave ports or active slave ports should be stopped?
Yes, I think all the slave ports should be stopped(or try to be stopped).
Quoted 21 lines
Quoted 21 lines
Fixes 0911d4ec0183 ("net/bonding: fix crash when stopping mode 4 port")Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>---
drivers/net/bonding/rte_eth_bond_pmd.c| 20 +++++++++++---------1 file changed, 11 insertions(+), 9 deletions(-)drivers/net/bonding/rte_eth_bond_pmd.c +2
-0@@ -2118,18 +2118,20 @@ bond_ethdev_stop(struct rte_eth_dev *eth_dev) internals->link_status_polling_enabled = 0; for (i = 0; i < internals->slave_count; i++) { uint16_t slave_id = internals->slaves[i].port_id; + + internals->slaves[i].last_link_status = 0;
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/net/bonding/rte_eth_bond_pmd.c bond_ethdev_stop(struct rte_eth_dev *eth_dev) · line 2127
Quoted 5 lines
drivers/net/bonding/rte_eth_bond_pmd.c +5
-0+ ret = rte_eth_dev_stop(slave_id); + if (ret != 0) { + RTE_BOND_LOG(ERR, "Failed to stop device on port %u", + slave_id); + return ret;Should it return here or try to stop all ports?
What about to record the return status, but keep continue to stop all ports. And return error if any of the stop failed?
Well, I am glad you have found something unreasaonable about 'stop'. Let us see API 'rte_eth_dev_stop'
rte_eth_dev_stop(dev)
{
....
dev->data->dev_started = 0;
ret = (*dev->dev_ops->dev_stop)(dev)
retur ret;
}This is unreasaonable. No matter 'dev_ops->dev_stop' succeed or fail, the state 'dev_started ' will always set to be '0'.
But this does not only influence bonding device but other devices like eth dev or vdev.
This is the bug in rte ethdev level. I will send another patch to fix it.
Quoted 40 lines
Quoted 3 lines
I think no need to do this. APP only see the bonded device. If bonded
device stop failed, it means it works failed. And the number of "stopped" successfully slave does not make any sense.OK if trying to stop as much as possible 'slave' devices doesn't make sense, we can keep as it is.
Btw, when functions fails at this point, bonding device itself already marked as stopped, right? And some of the slave devices may be stopped already before failure.
I don't know how confusing this is for the user, thatstop()function is failed but bonding device state is 'stopped'. I don't know if function should recover at least bonding device status (back to started) on failure, what do you think?Quoted 20 lines
Quoted 6 lines
drivers/net/bonding/rte_eth_bond_pmd.c bond_ethdev_stop(struct rte_eth_dev *eth_dev) · line 2130
Quoted 4 lines
drivers/net/bonding/rte_eth_bond_pmd.c +3
-0+ } + + /* active slaves need to deactivate. */" active slaves need to be deactivated. " ?
agreed.
Quoted 13 lines
Quoted 12 lines
drivers/net/bonding/rte_eth_bond_pmd.c +0
-0if (find_slave_by_id(internals->active_slaves,
internals->active_slave_count,slave_id) != -internals->active_slave_count) {
- internals->slaves[i].last_link_status = 0;
- ret =rte_eth_dev_stop(slave_id);
- if (ret != 0) {
- RTE_BOND_LOG(ERR, "Failed to stop device on port %u", -slave_id);
- return ret;
- }
+internals->active_slave_count)Quoted 9 lines
Quoted 2 lines
I think original indentation for this line is better.
agreed.
Quoted 6 lines
Quoted 4 lines
deactivate_slave(eth_dev, slave_id); - }
}
return 0;.
.
Re: [PATCH V2 1/4] net/bonding: fix non-active slaves aren't stopped
Quoted 78 lines · Min Hu (Connor)
Hi, Ferruh,
Quoted 9 lines · Ferruh Yigit
Quoted 8 lines · Min Hu (Connor)
Hi, Ferruh,
Quoted 5 lines · Ferruh Yigit
Quoted 3 lines · Min Hu (Connor)
From: Huisong Li <[email protected]>
When stopping a bonded port, all slaves should be deactivated. But only
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
s/deactivated/stopped/ ?
not agreed. deactivated and stopped are different state for slave.
Just to clarify the sentences, otherwise I see the 'stopped' and 'deactivated' states are different.
Next sentences complains that not all ports are stopped: "But only active slaves are stopped.", so I thought intention in this sentences to claim that all slaves should be stopped (but it mentions all slaves should be 'deactivated'). As long as you address the disconnection between two sentences, I don't mind the wording.Actually, there is something wrong with the wording. Yes, I should take your advice.
Quoted 35 lines
Quoted 35 lines
Quoted 6 lines
Quoted 3 lines
active slaves are stopped. So fix it and do "
deactivae_slave()" for actives/deactivae_slave()/deactivate_slave()/
agreed.
Quoted 5 lines
Quoted 1 line
slaves.
Hi Connor,
When a bonding port is closed, is it clear if all slave ports or active slave ports should be stopped?
Yes, I think all the slave ports should be stopped(or try to be stopped).
Quoted 21 lines
Quoted 21 lines
Fixes 0911d4ec0183 ("net/bonding: fix crash when stopping mode 4 port")Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>---
drivers/net/bonding/rte_eth_bond_pmd.c| 20 +++++++++++---------1 file changed, 11 insertions(+), 9 deletions(-)drivers/net/bonding/rte_eth_bond_pmd.c +2
-0@@ -2118,18 +2118,20 @@ bond_ethdev_stop(struct rte_eth_dev *eth_dev) internals->link_status_polling_enabled = 0; for (i = 0; i < internals->slave_count; i++) { uint16_t slave_id = internals->slaves[i].port_id; + + internals->slaves[i].last_link_status = 0;Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/net/bonding/rte_eth_bond_pmd.c bond_ethdev_stop(struct rte_eth_dev *eth_dev) · line 2127
Quoted 5 lines
drivers/net/bonding/rte_eth_bond_pmd.c +5
-0+ ret = rte_eth_dev_stop(slave_id); + if (ret != 0) { + RTE_BOND_LOG(ERR, "Failed to stop device on port %u", + slave_id); + return ret;Should it return here or try to stop all ports?
What about to record the return status, but keep continue to stop all ports. And return error if any of the stop failed?Well, I am glad you have found something unreasaonable about 'stop'. Let us see API '
rte_eth_dev_stop'
rte_eth_dev_stop(dev)
{
....
dev->data->dev_started = 0;
ret = (*dev->dev_ops->dev_stop)(dev)
retur ret;
}
This is unreasaonable. No matter 'dev_ops->dev_stop' succeed or fail, the state 'dev_started' will always set to be '0'.
Quoted 5 lines
But this does not only influence bonding device but other devices like eth dev or vdev.
This is the bug in rte ethdev level. I will send another patch to fix it.
Hi Connor,
I agree this is an issue in the API, cc'ed Andrew and Thomas.
I vaguely remember that "dev_started = 0" was required for some dev_ops, but not quite sure, let me check this. At worst we can do as following to be sure:
dev->data->dev_started = 0;
ret = (*dev->dev_ops->dev_stop)(dev)
if (ret)
dev->data->dev_started = 1;Also we need to clarify in the API documentation (.h file), what is the status of the device if 'rte_eth_dev_stop()' returned error.
Btw, would you be OK to separate this ethdev patch from your bonding patch, to not stuck your series because of ethdev one.
Quoted 41 lines
Quoted 41 lines
Quoted 4 lines
I think no need to do this. APP only see the bonded device. If bonded
device stop failed, it means it works failed. And the number of "stopped" successfully slave does not make any sense.OK if trying to stop as much as possible 'slave' devices doesn't make sense, we can keep as it is.
Btw, when functions fails at this point, bonding device itself already marked as stopped, right? And some of the slave devices may be stopped already before failure.
I don't know how confusing this is for the user, thatstop()function is failed but bonding device state is 'stopped'. I don't know if function should recover at least bonding device status (back to started) on failure, what do you think?Quoted 20 lines
Quoted 6 lines
drivers/net/bonding/rte_eth_bond_pmd.c bond_ethdev_stop(struct rte_eth_dev *eth_dev) · line 2130
Quoted 4 lines
drivers/net/bonding/rte_eth_bond_pmd.c +3
-0+ } + + /* active slaves need to deactivate. */" active slaves need to be deactivated. " ?
agreed.
Quoted 13 lines
Quoted 12 lines
drivers/net/bonding/rte_eth_bond_pmd.c +0
-0if (find_slave_by_id(internals->active_slaves,
internals->active_slave_count,slave_id) != -internals->active_slave_count) {
- internals->slaves[i].last_link_status = 0;
- ret =rte_eth_dev_stop(slave_id);
- if (ret != 0) {
- RTE_BOND_LOG(ERR, "Failed to stop device on port %u", -slave_id);
- return ret;
- }
+internals->active_slave_count)Quoted 9 lines
Quoted 2 lines
I think original indentation for this line is better.
agreed.
Quoted 6 lines
Quoted 4 lines
deactivate_slave(eth_dev, slave_id); - }
}
return 0;.
.
Re: [PATCH V2 1/4] net/bonding: fix non-active slaves aren't stopped
Hi, Ferruh,
Quoted 97 lines · Ferruh Yigit
Quoted 78 lines · Min Hu (Connor)
Hi, Ferruh,
Quoted 9 lines · Ferruh Yigit
Quoted 8 lines · Min Hu (Connor)
Hi, Ferruh,
Quoted 5 lines · Ferruh Yigit
Quoted 3 lines · Min Hu (Connor)
From: Huisong Li <[email protected]>
When stopping a bonded port, all slaves should be deactivated. But only
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
s/deactivated/stopped/ ?
not agreed. deactivated and stopped are different state for slave.
Just to clarify the sentences, otherwise I see the 'stopped' and 'deactivated' states are different.
Next sentences complains that not all ports are stopped: "But only active slaves are stopped.", so I thought intention in this sentences to claim that all slaves should be stopped (but it mentions all slaves should be 'deactivated'). As long as you address the disconnection between two sentences, I don't mind the wording.Actually, there is something wrong with the wording. Yes, I should take your advice.
Quoted 35 lines
Quoted 35 lines
Quoted 6 lines
Quoted 3 lines
active slaves are stopped. So fix it and do "
deactivae_slave()" for actives/deactivae_slave()/deactivate_slave()/
agreed.
Quoted 5 lines
Quoted 1 line
slaves.
Hi Connor,
When a bonding port is closed, is it clear if all slave ports or active slave ports should be stopped?
Yes, I think all the slave ports should be stopped(or try to be stopped).
Quoted 21 lines
Quoted 21 lines
Fixes 0911d4ec0183 ("net/bonding: fix crash when stopping mode 4 port")Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>---
drivers/net/bonding/rte_eth_bond_pmd.c| 20 +++++++++++---------1 file changed, 11 insertions(+), 9 deletions(-)drivers/net/bonding/rte_eth_bond_pmd.c +2
-0@@ -2118,18 +2118,20 @@ bond_ethdev_stop(struct rte_eth_dev *eth_dev) internals->link_status_polling_enabled = 0; for (i = 0; i < internals->slave_count; i++) { uint16_t slave_id = internals->slaves[i].port_id; + + internals->slaves[i].last_link_status = 0;Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/net/bonding/rte_eth_bond_pmd.c bond_ethdev_stop(struct rte_eth_dev *eth_dev) · line 2127
Quoted 5 lines
drivers/net/bonding/rte_eth_bond_pmd.c +5
-0+ ret = rte_eth_dev_stop(slave_id); + if (ret != 0) { + RTE_BOND_LOG(ERR, "Failed to stop device on port %u", + slave_id); + return ret;Should it return here or try to stop all ports?
What about to record the return status, but keep continue to stop all ports. And return error if any of the stop failed?Well, I am glad you have found something unreasaonable about 'stop'. Let us see API '
rte_eth_dev_stop'
rte_eth_dev_stop(dev)
{
....
dev->data->dev_started = 0;
ret = (*dev->dev_ops->dev_stop)(dev)
retur ret;
}
This is unreasaonable. No matter 'dev_ops->dev_stop' succeed or fail, the state 'dev_started' will always set to be '0'.Quoted 5 lines
But this does not only influence bonding device but other devices like eth dev or vdev.
This is the bug in rte ethdev level. I will send another patch to fix it.Hi Connor,
I agree this is an issue in the API, cc'ed Andrew and Thomas.
I vaguely remember that "
dev_started= 0" was required for somedev_ops, but not quite sure, let me check this. At worst we can do as following to be sure:
dev->data->dev_started= 0;
ret = (*dev->dev_ops->dev_stop)(dev)
if (ret)
dev->data->dev_started= 1;
Quoted 4 lines
Also we need to clarify in the API documentation (.h file), what is the status of the device if '
rte_eth_dev_stop()' returned error.Btw, would you be OK to separate this ethdev patch from your bonding patch, to not stuck your series because of ethdev one.
Yes, this patch can be abandoned from this set.
Quoted 46 lines
Quoted 36 lines
Quoted 36 lines
Quoted 6 lines
I think no need to do this. APP only see the bonded device. If bonded
device stop failed, it means it works failed. And the number of "stopped" successfully slave does not make any sense.OK if trying to stop as much as possible 'slave' devices doesn't make sense, we can keep as it is.
Btw, when functions fails at this point, bonding device itself already marked as stopped, right? And some of the slave devices may be stopped already before failure.
I don't know how confusing this is for the user, thatstop()function is failed but bonding device state is 'stopped'. I don't know if function should recover at least bonding device status (back to started) on failure, what do you think?Quoted 24 lines
Quoted 6 lines
drivers/net/bonding/rte_eth_bond_pmd.c bond_ethdev_stop(struct rte_eth_dev *eth_dev) · line 2130
Quoted 4 lines
drivers/net/bonding/rte_eth_bond_pmd.c +3
-0+ } + + /* active slaves need to deactivate. */" active slaves need to be deactivated. " ?
agreed.
Quoted 14 lines
Quoted 5 lines
drivers/net/bonding/rte_eth_bond_pmd.c +0
-0if (find_slave_by_id(internals->active_slaves,
internals->active_slave_count,slave_id) != -internals->active_slave_count) { - internals->slaves[i].last_link_status = 0;Quoted 8 lines
- ret =
rte_eth_dev_stop(slave_id);
- if (ret != 0) {
- RTE_BOND_LOG(ERR, "Failed to stop device on port %u",
-slave_id);
- return ret;
- }
+internals->active_slave_count)Quoted 2 lines
I think original indentation for this line is better.
agreed.
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
deactivate_slave(eth_dev, slave_id); - }
}
return 0;.
.
.
PATCH v2 2/4 net/bonding: fix non-terminable while loop
In reply to Min Hu (Connor)From: Huisong Li <[email protected]>
All slaves will be stopped and removed when closing a bonded port. But the while loop can not stop if both rte_eth_dev_stop and rte_eth_bond_slave_remove fail to run.
--- drivers/net/bonding/rte_eth_bond_pmd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/net/bonding/rte_eth_bond_pmd.c +2 -1
@@ -2149,13 +2149,14 @@ bond_ethdev_close(struct rte_eth_dev *dev)return 0 ;RTE_BOND_LOG (INFO ,"Closing bonded device %s" , dev->device->name); -while (internals->slave_count != skipped) { +while (skipped < internals->slave_count) {uint16_t port_id = internals->slaves[skipped].port_id;if (rte_eth_dev_stop (port_id) !=0 ) {RTE_BOND_LOG (ERR ,"Failed to stop device on port %u" , port_id); skipped++; +continue ; }if (rte_eth_bond_slave_remove (bond_port_id, port_id) !=0 ) {
Signature · 1 line
2.33.0
Re: [PATCH V2 2/4] net/bonding: fix non-terminable while loop
Quoted 6 lines · Min Hu (Connor)
From: Huisong Li <[email protected]>
All slaves will be stopped and removed when closing a bonded port. But the while loop can not stop if both
rte_eth_dev_stopandrte_eth_bond_slave_removefail to run.
Agree that this is a defect introduced in below commit. Thanks for the fix.
Quoted 14 lines
Fixes fb0379bc5db3 ("net/bonding: check stop call status")Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>--- drivers/net/bonding/rte_eth_bond_pmd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/net/bonding/rte_eth_bond_pmd.c +0
-0
drivers/net/bonding/rte_eth_bond_pmd.c bond_ethdev_close(struct rte_eth_dev *dev) · line 2152
Quoted 5 lines
drivers/net/bonding/rte_eth_bond_pmd.c +1
-1@@ -2149,13 +2149,14 @@ bond_ethdev_close(struct rte_eth_dev *dev)return 0 ;RTE_BOND_LOG (INFO ,"Closing bonded device %s" , dev->device->name); -while (internals->slave_count != skipped) { +while (skipped < internals->slave_count) {
When below fixed with adding 'continue', no need to change the check, right? Although new one is also correct.
drivers/net/bonding/rte_eth_bond_pmd.c bond_ethdev_close(struct rte_eth_dev *dev) · line 2159
Quoted 7 lines
drivers/net/bonding/rte_eth_bond_pmd.c +1
-0uint16_t port_id = internals->slaves[skipped].port_id;if (rte_eth_dev_stop (port_id) !=0 ) {RTE_BOND_LOG (ERR ,"Failed to stop device on port %u" , port_id); skipped++; +continue ;
Can't we remove the slave even if 'stop()' failed? If so I think better to just log the error and keep continue in that case, what do you think?
Quoted 3 lines
drivers/net/bonding/rte_eth_bond_pmd.c +0
-0}if (rte_eth_bond_slave_remove (bond_port_id, port_id) !=0 ) {
Re: [PATCH V2 2/4] net/bonding: fix non-terminable while loop
Hi, Ferruh,
Quoted 9 lines · Ferruh Yigit
Quoted 6 lines · Min Hu (Connor)
From: Huisong Li <[email protected]>
All slaves will be stopped and removed when closing a bonded port. But the while loop can not stop if both
rte_eth_dev_stopandrte_eth_bond_slave_removefail to run.Agree that this is a defect introduced in below commit. Thanks for the fix.
thanks.
Quoted 10 lines
Quoted 10 lines
Fixes fb0379bc5db3 ("net/bonding: check stop call status")Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>---
drivers/net/bonding/rte_eth_bond_pmd.c| 3 ++-1 file changed, 2 insertions(+), 1 deletion(-)
Quoted 11 lines
drivers/net/bonding/rte_eth_bond_pmd.c bond_ethdev_close(struct rte_eth_dev *dev) · line 2151
Quoted 9 lines
drivers/net/bonding/rte_eth_bond_pmd.c +1
-1@@ -2149,13 +2149,14 @@ bond_ethdev_close(struct rte_eth_dev *dev) return 0; RTE_BOND_LOG(INFO, "Closing bonded device %s", dev->device->name); - while (internals->slave_count != skipped) { + while (skipped < internals->slave_count) {When below fixed with adding 'continue', no need to change the check, right? Although new one is also correct.
Agreed.
Quoted 9 lines
drivers/net/bonding/rte_eth_bond_pmd.c bond_ethdev_close(struct rte_eth_dev *dev) · line 2159
Quoted 7 lines
drivers/net/bonding/rte_eth_bond_pmd.c +1
-0uint16_t port_id = internals->slaves[skipped].port_id; if (rte_eth_dev_stop(port_id) != 0) { RTE_BOND_LOG(ERR, "Failed to stop device on port %u", port_id); skipped++; + continue;Can't we remove the slave even if '
stop()' failed? If so I think better to just log the error and keep continue in that case, what do you think?
NO, if slave stop failed, we cannot remove the slave. just see the function stack:rte_eth_bond_slave_remove __eth_bond_slave_remove_lock_free slave_removerte_eth_dev_internal_reset
if (dev->data->dev_started) {
RTE_ETHDEV_LOG(ERR, "Port %u must be stopped to allow reset\n",
dev->data->port_id);
return;
}Quoted 5 lines
drivers/net/bonding/rte_eth_bond_pmd.c bond_ethdev_close(struct rte_eth_dev *dev) · line 2162
Quoted 3 lines
drivers/net/bonding/rte_eth_bond_pmd.c +0
-0} if (rte_eth_bond_slave_remove(bond_port_id, port_id) != 0) {.
Re: [PATCH V2 2/4] net/bonding: fix non-terminable while loop
Quoted 43 lines · Min Hu (Connor)
Hi, Ferruh,
Quoted 9 lines · Ferruh Yigit
Quoted 6 lines · Min Hu (Connor)
From: Huisong Li <[email protected]>
All slaves will be stopped and removed when closing a bonded port. But the while loop can not stop if both
rte_eth_dev_stopandrte_eth_bond_slave_removefail to run.Agree that this is a defect introduced in below commit. Thanks for the fix.
thanks.
Quoted 10 lines
Quoted 10 lines
Fixes fb0379bc5db3 ("net/bonding: check stop call status")Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>---
drivers/net/bonding/rte_eth_bond_pmd.c| 3 ++-1 file changed, 2 insertions(+), 1 deletion(-)Quoted 11 lines
drivers/net/bonding/rte_eth_bond_pmd.c bond_ethdev_close(struct rte_eth_dev *dev) · line 2151
Quoted 9 lines
drivers/net/bonding/rte_eth_bond_pmd.c +1
-1@@ -2149,13 +2149,14 @@ bond_ethdev_close(struct rte_eth_dev *dev) return 0; RTE_BOND_LOG(INFO, "Closing bonded device %s", dev->device->name); - while (internals->slave_count != skipped) { + while (skipped < internals->slave_count) {When below fixed with adding 'continue', no need to change the check, right? Although new one is also correct.
Agreed.
Quoted 8 lines
Quoted 7 lines
drivers/net/bonding/rte_eth_bond_pmd.c +1
-0uint16_t port_id = internals->slaves[skipped].port_id; if (rte_eth_dev_stop(port_id) != 0) { RTE_BOND_LOG(ERR, "Failed to stop device on port %u", port_id); skipped++; + continue;
Quoted 2 lines
Quoted 1 line
Can't we remove the slave even if '
stop()' failed? If so I think better to just log the error and keep continue in that case, what do you think?NO, if slave stop failed, we cannot remove the slave.
Got it, thanks for clarification.
Quoted 16 lines
just see the function stack:
rte_eth_bond_slave_remove__eth_bond_slave_remove_lock_freeslave_removerte_eth_dev_internal_reset
if (dev->data->dev_started) {
RTE_ETHDEV_LOG(ERR, "Port %u must be stopped to allow reset\n",dev->data->port_id);
return;
}Quoted 5 lines
drivers/net/bonding/rte_eth_bond_pmd.c bond_ethdev_close(struct rte_eth_dev *dev) · line 2162
Quoted 3 lines
drivers/net/bonding/rte_eth_bond_pmd.c +0
-0} if (rte_eth_bond_slave_remove(bond_port_id, port_id) != 0) {.
PATCH v2 3/4 app/testpmd: fix port status of slave device
In reply to Min Hu (Connor)From: Huisong Li <[email protected]>
Starting or stopping a bonded port also starts or stops all active slaves under the bonded port. If this port is a bonded device, we need to modify the port status of all slaves.
--- app/test-pmd/cmdline.c | 1 + app/test-pmd/testpmd.c | 74 +++++++++++++++++++++++++++++++++++++++--- app/test-pmd/testpmd.h | 3 +- 3 files changed, 73 insertions(+), 5 deletions(-)
app/test-pmd/cmdline.c +1 -0
@@ -6671,6 +6671,7 @@ static void cmd_create_bonded_device_parsed(void *parsed_result,"Failed to enable promiscuous mode for port %u: %s - ignore\n" , port_id, rte_strerror(-ret)); + ports[port_id].bond_flag =1 ; ports[port_id].need_setup =0 ; ports[port_id].port_status =RTE_PORT_STOPPED ; }
app/test-pmd/testpmd.c +70 -4
95 lines not on this page · raw message
app/test-pmd/testpmd.h +2 -1
@@ -266,7 +266,8 @@ struct rte_port {uint32_t mc_addr_nb;/**< nb. of addr. in mc_addr_pool */ queueid_t queue_nb;/**< nb. of queues for flow rules */ uint32_t queue_sz;/**< size of a queue for flow rules */ -uint8_t slave_flag;/**< bonding slave port */ + uint8_t slave_flag : 1, /**< bonding slave port */ + bond_flag : 1; /**< port is bond device */struct port_template *pattern_templ_list;/**< Pattern templates. */ struct port_template *actions_templ_list;/**< Actions templates. */ struct port_table *table_list;/**< Flow tables. */
Signature · 1 line
2.33.0
PATCH v2 4/4 app/testpmd: fix slave device isn't released
In reply to Min Hu (Connor)From: Huisong Li <[email protected]>
Currently, some eth devices are added to bond device, these devices are not released when the quit command is executed in testpmd. This patch adds the release operation for all active slaves under a bond device.
--- app/test-pmd/cmdline.c | 1 + app/test-pmd/testpmd.c | 48 ++++++++++++++++++++++++++++++++++++++++++ app/test-pmd/testpmd.h | 2 ++ 3 files changed, 51 insertions(+)
app/test-pmd/cmdline.c +1 -0
@@ -8891,6 +8891,7 @@ static void cmd_quit_parsed(__rte_unused void *parsed_result, __rte_unused void *data) {cmdline_quit (cl); + cl_quit =1 ; } cmdline_parse_token_string_t cmd_quit_quit =
app/test-pmd/testpmd.c +48 -0
72 lines not on this page · raw message
app/test-pmd/testpmd.h +2 -0
@@ -32,6 +32,8 @@#define RTE_PORT_CLOSED (uint16_t)2#define RTE_PORT_HANDLING (uint16_t)3 +extern uint8_t cl_quit; +/* * It is used to allocate the memory for hash key. * The hash key size is NIC dependent.
Signature · 1 line
2.33.0
Re: [PATCH V2 4/4] app/testpmd: fix slave device isn't released
Hi, all,
any comments for this patch?
Quoted 122 lines · Min Hu (Connor)
From: Huisong Li <[email protected]>
Currently, some eth devices are added to bond device, these devices are not released when the quit command is executed in testpmd. This patch adds the release operation for all active slaves under a bond device.
Fixes 0e545d3047fe ("app/testpmd: check stopping port is not in bonding")Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>--- app/test-pmd/cmdline.c | 1 + app/test-pmd/testpmd.c | 48 ++++++++++++++++++++++++++++++++++++++++++ app/test-pmd/testpmd.h | 2 ++ 3 files changed, 51 insertions(+)app/test-pmd/cmdline.c +1
-0@@ -8891,6 +8891,7 @@ static void cmd_quit_parsed(__rte_unused void *parsed_result, __rte_unused void *data) {cmdline_quit (cl); + cl_quit =1 ; } cmdline_parse_token_string_t cmd_quit_quit =app/test-pmd/testpmd.c +48
-0@@ -229,6 +229,7 @@ unsigned int xstats_display_num; /**< Size of extended statistics to show */* option. Set flag to exit stats period loop after received SIGINT/SIGTERM. */ uint8_t f_quit; +uint8_t cl_quit;/* Quit testpmd from cmdline. */ /* * Max Rx frame size, set by '--max-pkt-len' parameter. @@ -3167,11 +3168,43 @@ remove_invalid_ports(void) nb_cfg_ports = nb_fwd_ports; } +#ifdef RTE_NET_BOND +static void +clear_bonding_slave_device (portid_t *slave_pids,uint16_t num_slaves) +{ +struct rte_port *port; +portid_t slave_pid; +uint16_t i; + +for (i =0 ; i < num_slaves; i++) { + slave_pid = slave_pids[i]; +if (port_is_started (slave_pid) ==1 ) { +if (rte_eth_dev_stop (slave_pid) !=0 ) +fprintf (stderr,"rte_eth_dev_stop failed for port %u\n" , + slave_pid); + + port = &ports[slave_pid]; + port->port_status =RTE_PORT_STOPPED ; + } + +clear_port_slave_flag (slave_pid); + +/* Close slave device when testpmd quit or is killed. */ +if (cl_quit ==1 || f_quit ==1 ) +rte_eth_dev_close (slave_pid); + } +} +#endif +void close_port (portid_t pid) {portid_t pi;struct rte_port *port; +#ifdef RTE_NET_BOND +portid_t slave_pids[RTE_MAX_ETHPORTS ]; +int num_slaves =0 ; +#endif if (port_id_is_invalid (pid,ENABLED_WARN ))return ;@@ -3205,7 +3238,22 @@ close_port(portid_t pid)if (is_proc_primary ()) {port_flow_flush (pi);port_flex_item_flush (pi); +#ifdef RTE_NET_BOND +if (port->bond_flag ==1 ) + num_slaves =rte_eth_bond_slaves_get (pi, + slave_pids, +RTE_MAX_ETHPORTS ); +#endif rte_eth_dev_close (pi); +#ifdef RTE_NET_BOND +/* +* If this port is bonded device, all slaves under the +* device need to be removed or closed. +*/ +if (port->bond_flag ==1 && num_slaves >0 ) +clear_bonding_slave_device (slave_pids, + num_slaves); +#endif }free_xstats_display_info (pi);app/test-pmd/testpmd.h +2
-0@@ -32,6 +32,8 @@#define RTE_PORT_CLOSED (uint16_t)2#define RTE_PORT_HANDLING (uint16_t)3 +extern uint8_t cl_quit; +/* * It is used to allocate the memory for hash key. * The hash key size is NIC dependent.
Re: [PATCH V2 4/4] app/testpmd: fix slave device isn't released
Hi Connor,
Quoted 65 lines · Min Hu (Connor)
Hi, all,
any comments for this patch?Quoted 61 lines · Min Hu (Connor)
From: Huisong Li <[email protected]>
Currently, some eth devices are added to bond device, these devices are not released when the quit command is executed in testpmd. This patch adds the release operation for all active slaves under a bond device.
Fixes 0e545d3047fe ("app/testpmd: check stopping port is not in bonding")Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>---
app/test-pmd/cmdline.c | 1 + app/test-pmd/testpmd.c | 48 ++++++++++++++++++++++++++++++++++++++++++ app/test-pmd/testpmd.h | 2 ++ 3 files changed, 51 insertions(+)app/test-pmd/cmdline.c +1
-0@@ -8891,6 +8891,7 @@ static void cmd_quit_parsed(__rte_unused void *parsed_result, __rte_unused void *data) { cmdline_quit(cl); + cl_quit = 1; }
cmdline_parse_token_string_tcmd_quit_quit=app/test-pmd/testpmd.c +0
-0@@ -229,6 +229,7 @@ unsigned int xstats_display_num; /**< Size ofextended statistics to show */
* option. Set flag to exit stats period loop after received SIGINT/SIGTERM. */
uint8_tf_quit;
+uint8_tcl_quit; /* Quit testpmd from cmdline. */
/*
* Max Rx frame size, set by '--max-pkt-len' parameter.app/test-pmd/testpmd.c +0
-0@@ -3167,11 +3168,43 @@ remove_invalid_ports(void) nb_cfg_ports = nb_fwd_ports; }+#ifdef
RTE_NET_BONDapp/test-pmd/testpmd.c +12
-0+static void +clear_bonding_slave_device (portid_t *slave_pids,uint16_t num_slaves) +{ + struct rte_port *port; + portid_t slave_pid; + uint16_t i; + + for (i = 0; i < num_slaves; i++) { + slave_pid = slave_pids[i]; + if (port_is_started(slave_pid) == 1) { + if (rte_eth_dev_stop(slave_pid) != 0) + fprintf(stderr, "rte_eth_dev_stop failed for port%u\n",
+slave_pid);
+
+ port = &ports[slave_pid];
Quoted 8 lines
app/test-pmd/testpmd.c remove_invalid_ports(void) · line 3194
Quoted 8 lines
+
port->port_status=RTE_PORT_STOPPED; + }app/test-pmd/testpmd.c +6
-0+ + clear_port_slave_flag(slave_pid); + + /* Close slave device when testpmd quit or is killed. */ + if (cl_quit == 1 || f_quit == 1) + rte_eth_dev_close(slave_pid);
When we close the bond device, shouldn't we close all slave ports also with it. Just to avoid usage of global flag "cl_quit"
Quoted 27 lines
Quoted 27 lines
+ }
app/test-pmd/testpmd.c +3
-0+} +#endif +void
close_port(portid_t pid)
{
portid_tpi;
structrte_port*port;app/test-pmd/testpmd.c +4
-0+#ifdef RTE_NET_BOND + portid_t slave_pids[RTE_MAX_ETHPORTS]; + int num_slaves = 0; +#endif if (
port_id_is_invalid(pid, ENABLED_WARN)) return;app/test-pmd/testpmd.c +7
-0@@ -3205,7 +3238,22 @@ close_port(portid_t pid) if (is_proc_primary()) { port_flow_flush(pi); port_flex_item_flush(pi); +#ifdef RTE_NET_BOND + if (port->bond_flag == 1) + num_slaves = rte_eth_bond_slaves_get(pi, + slave_pids, + RTE_MAX_ETHPORTS); +#endif rte_eth_dev_close(pi); +#ifdef RTE_NET_BOND
Quoted 8 lines
app/test-pmd/testpmd.c close_port(portid_t pid) · line 3256
Quoted 8 lines
app/test-pmd/testpmd.c +8
-0+ /* + * If this port is bonded device, all slaves under the + * device need to be removed or closed. + */ + if (port->bond_flag == 1 && num_slaves > 0) + clear_bonding_slave_device(slave_pids, + num_slaves); +#endif
Can we merge these two #ifdef sections, like move _close() below.
Quoted 15 lines
Quoted 15 lines
app/test-pmd/testpmd.c +0
-0} free_xstats_display_info(pi);app/test-pmd/testpmd.h +1
-0@@ -32,6 +32,8 @@ #define RTE_PORT_CLOSED (uint16_t)2 #define RTE_PORT_HANDLING (uint16_t)3 +extern uint8_t cl_quit; + /** It is used to allocate the memory for hash key. * The hash key size is NIC dependent.
Re: [PATCH V2 0/4] bugfix for bonding
In reply to Min Hu (Connor)Hi, Ferruh,
what do you think of this patch?
Quoted 21 lines · Min Hu (Connor)
From: Huisong Li <[email protected]>
Fix some bonding related bug for bonding PMD and testpmd.
v2:
- add two patches in net/bonding.
- change port status of slave device instart_port
- delete removing slave operation when close bond portHuisong Li (4):
net/bonding: fix non-active slaves aren't stopped net/bonding: fix non-terminable while loop app/testpmd: fix port status of slave device app/testpmd: fix slave device isn't releasedapp/test-pmd/cmdline.c | 2 + app/test-pmd/testpmd.c | 122 ++++++++++++++++++++++++- app/test-pmd/testpmd.h | 5 +- drivers/net/bonding/rte_eth_bond_pmd.c | 23 +++-- 4 files changed, 137 insertions(+), 15 deletions(-)
v3 cover bugfix for bonding
In reply to Min Hu (Connor)Fix some bonding related bug for bonding PMD and testpmd.
Huisong Li (4):
net/bonding: fix non-active slaves aren't stopped net/bonding: fix non-terminable while loop app/testpmd: fix port status of slave device app/testpmd: fix slave device isn't released
Min Hu (Connor) (1):
ethdev: fix dev state when stop
---
v3:
- fix comments
- add a new patch to fix dev state.
v2:
- add two patches in net/bonding.
- change port status of slave device in start_port
- delete removing slave operation when close bond port
app/test-pmd/cmdline.c | 2 + app/test-pmd/testpmd.c | 122 ++++++++++++++++++++++++- app/test-pmd/testpmd.h | 5 +- drivers/net/bonding/rte_eth_bond_pmd.c | 21 +++-- lib/ethdev/rte_ethdev.c | 3 +- 5 files changed, 138 insertions(+), 15 deletions(-)
Signature · 1 line
2.33.0
PATCH v3 1/5 net/bonding: fix non-active slaves aren't stopped
From: Huisong Li <[email protected]>
When stopping a bonded port, all slaves should be stopped. But only active slaves are stopped. So fix it and do "deactivate_slave()" for active slaves.
--- drivers/net/bonding/rte_eth_bond_pmd.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)
drivers/net/bonding/rte_eth_bond_pmd.c +11 -9
@@ -2118,18 +2118,20 @@ bond_ethdev_stop(struct rte_eth_dev *eth_dev) internals->link_status_polling_enabled =0 ;for (i =0 ; i < internals->slave_count; i++) {uint16_t slave_id = internals->slaves[i].port_id; + + internals->slaves[i].last_link_status =0 ; + ret =rte_eth_dev_stop (slave_id); +if (ret !=0 ) { +RTE_BOND_LOG (ERR ,"Failed to stop device on port %u" , + slave_id); +return ret; + } + +/* active slaves need to be deactivated. */ if (find_slave_by_id (internals->active_slaves, internals->active_slave_count, slave_id) != - internals->active_slave_count) { - internals->slaves[i].last_link_status =0 ; - ret =rte_eth_dev_stop (slave_id); -if (ret !=0 ) { -RTE_BOND_LOG (ERR ,"Failed to stop device on port %u" , - slave_id); -return ret; - } + internals->active_slave_count)deactivate_slave (eth_dev, slave_id); - } }return 0 ;
Signature · 1 line
2.33.0
PATCH v3 2/5 net/bonding: fix non-terminable while loop
In reply to Min Hu (Connor)From: Huisong Li <[email protected]>
All slaves will be stopped and removed when closing a bonded port. But the while loop can not stop if both rte_eth_dev_stop and rte_eth_bond_slave_remove fail to run.
--- drivers/net/bonding/rte_eth_bond_pmd.c | 1 + 1 file changed, 1 insertion(+)
drivers/net/bonding/rte_eth_bond_pmd.c +1 -0
@@ -2156,6 +2156,7 @@ bond_ethdev_close(struct rte_eth_dev *dev)RTE_BOND_LOG (ERR ,"Failed to stop device on port %u" , port_id); skipped++; +continue ; }if (rte_eth_bond_slave_remove (bond_port_id, port_id) !=0 ) {
Signature · 1 line
2.33.0
PATCH v3 3/5 app/testpmd: fix port status of slave device
In reply to Min Hu (Connor)From: Huisong Li <[email protected]>
Starting or stopping a bonded port also starts or stops all active slaves under the bonded port. If this port is a bonded device, we need to modify the port status of all slaves.
--- app/test-pmd/cmdline.c | 1 + app/test-pmd/testpmd.c | 74 +++++++++++++++++++++++++++++++++++++++--- app/test-pmd/testpmd.h | 3 +- 3 files changed, 73 insertions(+), 5 deletions(-)
app/test-pmd/cmdline.c +1 -0
@@ -6671,6 +6671,7 @@ static void cmd_create_bonded_device_parsed(void *parsed_result,"Failed to enable promiscuous mode for port %u: %s - ignore\n" , port_id, rte_strerror(-ret)); + ports[port_id].bond_flag =1 ; ports[port_id].need_setup =0 ; ports[port_id].port_status =RTE_PORT_STOPPED ; }
app/test-pmd/testpmd.c +70 -4
95 lines not on this page · raw message
app/test-pmd/testpmd.h +2 -1
@@ -266,7 +266,8 @@ struct rte_port {uint32_t mc_addr_nb;/**< nb. of addr. in mc_addr_pool */ queueid_t queue_nb;/**< nb. of queues for flow rules */ uint32_t queue_sz;/**< size of a queue for flow rules */ -uint8_t slave_flag;/**< bonding slave port */ + uint8_t slave_flag : 1, /**< bonding slave port */ + bond_flag : 1; /**< port is bond device */struct port_template *pattern_templ_list;/**< Pattern templates. */ struct port_template *actions_templ_list;/**< Actions templates. */ struct port_table *table_list;/**< Flow tables. */
Signature · 1 line
2.33.0
Re: [PATCH v3 3/5] app/testpmd: fix port status of slave device
03/05/2022 11:02, Min Hu (Connor) пишет:
Quoted 108 lines
From: Huisong Li <[email protected]>
Starting or stopping a bonded port also starts or stops all active slaves under the bonded port. If this port is a bonded device, we need to modify the port status of all slaves.
Fixes 0e545d3047fe ("app/testpmd: check stopping port is not in bonding")Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>Acked-by Aman Singh <[email protected]>--- app/test-pmd/cmdline.c | 1 + app/test-pmd/testpmd.c | 74 +++++++++++++++++++++++++++++++++++++++--- app/test-pmd/testpmd.h | 3 +- 3 files changed, 73 insertions(+), 5 deletions(-)app/test-pmd/cmdline.c +1
-0@@ -6671,6 +6671,7 @@ static void cmd_create_bonded_device_parsed(void *parsed_result,"Failed to enable promiscuous mode for port %u: %s - ignore\n" , port_id, rte_strerror(-ret)); + ports[port_id].bond_flag =1 ; ports[port_id].need_setup =0 ; ports[port_id].port_status =RTE_PORT_STOPPED ; }app/test-pmd/testpmd.c +51
-2@@ -66,6 +66,9 @@#ifdef RTE_EXEC_ENV_WINDOWS #include <process.h> #endif +#ifdef RTE_NET_BOND +#include <rte_eth_bond.h> +#endif #include "testpmd.h" @@ -597,11 +600,57 @@ eth_dev_configure_mp(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,return 0 ; } +#ifdef RTE_NET_BOND +static int +change_bonding_slave_port_status (portid_t bond_pid,bool is_stop) +{ +portid_t slave_pids[RTE_MAX_ETHPORTS ]; +struct rte_port *port; +int num_slaves; +portid_t slave_pid; +int i; + + num_slaves =rte_eth_bond_slaves_get (bond_pid, slave_pids, +RTE_MAX_ETHPORTS ); +if (num_slaves <0 ) { +fprintf (stderr,"Failed to get slave list for port = %u\n" , + bond_pid); +return num_slaves; + } + +for (i =0 ; i < num_slaves; i++) { + slave_pid = slave_pids[i]; + port = &ports[slave_pid]; + port->port_status = + is_stop ?RTE_PORT_STOPPED :RTE_PORT_STARTED ; + } + +return 0 ; +} +#endif +static int eth_dev_start_mp (uint16_t port_id) { -if (is_proc_primary ()) -return rte_eth_dev_start (port_id); +int ret; + +if (is_proc_primary ()) { + ret =rte_eth_dev_start (port_id); +if (ret !=0 ) +return ret; + +#ifdef RTE_NET_BOND +struct rte_port *port = &ports[port_id]; + +/* +* Starting a bonded port also starts all slaves under the bonded +* device. So if this port is bond device, we need to modify the +* port status of these slaves. +*/ +if (port->bond_flag ==1 ) +return change_bonding_slave_port_status (port_id, false); +#endif + }return 0 ; }@@ -609,8 +658,25 @@ eth_dev_start_mp(uint16_t port_id)static int eth_dev_stop_mp (uint16_t port_id) {
app/test-pmd/testpmd.c eth_dev_start_mp(uint16_t port_id) · line 668
Quoted 10 lines
app/test-pmd/testpmd.c +8
-2-if (is_proc_primary ()) -return rte_eth_dev_stop (port_id); +int ret; + +if (is_proc_primary ()) { + ret =rte_eth_dev_stop (port_id); +if (ret !=0 ) +return ret; + +#ifdef RTE_NET_BOND
Here and in other places - probably no need to pollute the code with all these 'ifdef RTE_NET_BOND'.
I suppose this logic (for checking is bonding API present or not) can be hidden inside change_bonding_slave_port_status() itself.
Quoted 28 lines
app/test-pmd/testpmd.c +11
-0+struct rte_port *port = &ports[port_id]; + +/* +* Stopping a bonded port also stops all slaves under the bonded +* device. So if this port is bond device, we need to modify the +* port status of these slaves. +*/ +if (port->bond_flag ==1 ) +return change_bonding_slave_port_status (port_id, true); +#endif + }return 0 ; }app/test-pmd/testpmd.h +2
-1@@ -266,7 +266,8 @@ struct rte_port {uint32_t mc_addr_nb;/**< nb. of addr. in mc_addr_pool */ queueid_t queue_nb;/**< nb. of queues for flow rules */ uint32_t queue_sz;/**< size of a queue for flow rules */ -uint8_t slave_flag;/**< bonding slave port */ + uint8_t slave_flag : 1, /**< bonding slave port */ + bond_flag : 1; /**< port is bond device */struct port_template *pattern_templ_list;/**< Pattern templates. */ struct port_template *actions_templ_list;/**< Actions templates. */ struct port_table *table_list;/**< Flow tables. */
Re: [PATCH v3 3/5] app/testpmd: fix port status of slave device
Hi, Konstantin,
Quoted 114 lines · Konstantin Ananyev
03/05/2022 11:02, Min Hu (Connor) пишет:
Quoted 113 lines
From: Huisong Li <[email protected]>
Starting or stopping a bonded port also starts or stops all active slaves under the bonded port. If this port is a bonded device, we need to modify the port status of all slaves.
Fixes 0e545d3047fe ("app/testpmd: check stopping port is not in bonding")Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>Acked-by Aman Singh <[email protected]>---
app/test-pmd/cmdline.c | 1 + app/test-pmd/testpmd.c | 74 +++++++++++++++++++++++++++++++++++++++--- app/test-pmd/testpmd.h | 3 +- 3 files changed, 73 insertions(+), 5 deletions(-)app/test-pmd/cmdline.c +1
-0@@ -6671,6 +6671,7 @@ static void cmd_create_bonded_device_parsed(void *parsed_result, "Failed to enable promiscuous mode for port %u: %s - ignore\n", port_id, rte_strerror(-ret)); + ports[port_id].bond_flag = 1; ports[port_id].need_setup = 0; ports[port_id].port_status = RTE_PORT_STOPPED; }app/test-pmd/testpmd.c +32
-0@@ -66,6 +66,9 @@ #ifdef RTE_EXEC_ENV_WINDOWS #include <process.h> #endif +#ifdef RTE_NET_BOND +#include <rte_eth_bond.h> +#endif #include "testpmd.h"@@ -597,11 +600,57 @@ eth_dev_configure_mp(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q, return 0; } +#ifdef RTE_NET_BOND +static int +change_bonding_slave_port_status (portid_t bond_pid,bool is_stop) +{ + portid_t slave_pids[RTE_MAX_ETHPORTS]; + struct rte_port *port; + int num_slaves; + portid_t slave_pid; + int i; + + num_slaves = rte_eth_bond_slaves_get(bond_pid, slave_pids, + RTE_MAX_ETHPORTS); + if (num_slaves < 0) { + fprintf(stderr, "Failed to get slave list for port = %u\n", + bond_pid); + return num_slaves; + } + + for (i = 0; i < num_slaves; i++) { + slave_pid = slave_pids[i]; + port = &ports[slave_pid]; + port->port_status = + is_stop ? RTE_PORT_STOPPED : RTE_PORT_STARTED; + } + + return 0; +} +#endif +static int
eth_dev_start_mp(uint16_t port_id)
{
- if (is_proc_primary())
- returnrte_eth_dev_start(port_id);
+ int ret;app/test-pmd/testpmd.c +11
-0+ + if (is_proc_primary()) { + ret = rte_eth_dev_start(port_id); + if (ret != 0) + return ret; + +#ifdef RTE_NET_BOND + struct rte_port *port = &ports[port_id]; + + /* +* Starting a bonded port also starts all slaves under the bonded
+ * device. So if this port is bond device, we need to modify the + * port status of these slaves.
+ */
+ if (port->bond_flag== 1)
+ returnchange_bonding_slave_port_status(port_id, false); +#endif
+ }
return 0;
}app/test-pmd/testpmd.c +0
-0@@ -609,8 +658,25 @@ eth_dev_start_mp(uint16_t port_id)static int
eth_dev_stop_mp(uint16_t port_id)
{
- if (is_proc_primary())
- returnrte_eth_dev_stop(port_id);
+ int ret;
+
+ if (is_proc_primary()) {
+ ret =rte_eth_dev_stop(port_id);
+ if (ret != 0)
Quoted 9 lines
app/test-pmd/testpmd.c
Quoted 3 lines
+ return ret;
app/test-pmd/testpmd.c +2
-0+ +#ifdef RTE_NET_BOND Here and in other places - probably no need to pollute the code with all these 'ifdef
RTE_NET_BOND'.
I suppose this logic (for checking is bonding API present or not) can be hidden insidechange_bonding_slave_port_status()itself.
I think it does not pollute the code. anyone can tell according to
the flag 'ifdef RTE_NET_BOND'.
if hiddle inside 'change_bonding_slave_port_status', it will be weird.
For example, if the port is not bonding port, It will also invoke 'change_bonding_slave_port_status'. That is unreasonable.
Quoted 30 lines
Quoted 20 lines
+ struct
rte_port*port = &ports[port_id];app/test-pmd/testpmd.c +10
-0+ + /* + * Stopping a bonded port also stops all slaves under the bonded + * device. So if this port is bond device, we need to modify the + * port status of these slaves. + */ + if (port->bond_flag == 1) + return change_bonding_slave_port_status(port_id, true); +#endif + }return 0;
}app/test-pmd/testpmd.h +0
-0@@ -266,7 +266,8 @@ struct rte_port {uint32_t
mc_addr_nb; /**< nb. of addr. inmc_addr_pool*/Quoted 8 lines
queueid_t
queue_nb; /**< nb. of queues for flow rules */ uint32_tqueue_sz; /**< size of a queue for flow rules */ - uint8_tslave_flag; /**< bonding slave port */ + uint8_tslave_flag: 1, /**< bonding slave port */ +bond_flag: 1; /**< port is bond device */ struct port_template*pattern_templ_list; /**< Pattern templates. */ struct port_template*actions_templ_list; /**< Actions templates. */ struct port_table*table_list; /**< Flow tables. */.
Re: [PATCH v3 3/5] app/testpmd: fix port status of slave device
Hi Conor,
Quoted 126 lines · Konstantin Ananyev
Quoted 116 lines
03/05/2022 11:02, Min Hu (Connor) пишет:
Quoted 115 lines
From: Huisong Li <[email protected]>
Starting or stopping a bonded port also starts or stops all active slaves under the bonded port. If this port is a bonded device, we need to modify the port status of all slaves.
Fixes 0e545d3047fe ("app/testpmd: check stopping port is not in bonding")Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>Acked-by Aman Singh <[email protected]>---
app/test-pmd/cmdline.c | 1 + app/test-pmd/testpmd.c | 74 +++++++++++++++++++++++++++++++++++++++--- app/test-pmd/testpmd.h | 3 +- 3 files changed, 73 insertions(+), 5 deletions(-)app/test-pmd/cmdline.c +0
-0@@ -6671,6 +6671,7 @@ static void cmd_create_bonded_device_parsed(void *parsed_result,"Failed to enable promiscuous mode for port %u: %s
- ignore\n",
port_id,rte_strerror(-ret));
+ ports[port_id].bond_flag = 1; ports[port_id].need_setup = 0; ports[port_id].port_status =RTE_PORT_STOPPED; }app/test-pmd/testpmd.c +32
-0@@ -66,6 +66,9 @@ #ifdef RTE_EXEC_ENV_WINDOWS #include <process.h> #endif +#ifdef RTE_NET_BOND +#include <rte_eth_bond.h> +#endif #include "testpmd.h"@@ -597,11 +600,57 @@ eth_dev_configure_mp(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q, return 0; } +#ifdef RTE_NET_BOND +static int +change_bonding_slave_port_status (portid_t bond_pid,bool is_stop) +{ + portid_t slave_pids[RTE_MAX_ETHPORTS]; + struct rte_port *port; + int num_slaves; + portid_t slave_pid; + int i; + + num_slaves = rte_eth_bond_slaves_get(bond_pid, slave_pids, + RTE_MAX_ETHPORTS); + if (num_slaves < 0) { + fprintf(stderr, "Failed to get slave list for port = %u\n", + bond_pid); + return num_slaves; + } + + for (i = 0; i < num_slaves; i++) { + slave_pid = slave_pids[i]; + port = &ports[slave_pid]; + port->port_status = + is_stop ? RTE_PORT_STOPPED : RTE_PORT_STARTED; + } + + return 0; +} +#endif +static int
eth_dev_start_mp(uint16_t port_id)
{
- if (is_proc_primary())
- returnrte_eth_dev_start(port_id);
+ int ret;app/test-pmd/testpmd.c +11
-0+ + if (is_proc_primary()) { + ret = rte_eth_dev_start(port_id); + if (ret != 0) + return ret; + +#ifdef RTE_NET_BOND + struct rte_port *port = &ports[port_id]; + + /* +* Starting a bonded port also starts all slaves under the bonded
+ * device. So if this port is bond device, we need to modify the
+ * port status of these slaves.
+ */
+ if (port->bond_flag== 1)
+ returnchange_bonding_slave_port_status(port_id, false); +#endif
+ }
return 0;
}app/test-pmd/testpmd.c +0
-0@@ -609,8 +658,25 @@ eth_dev_start_mp(uint16_t port_id)static int
eth_dev_stop_mp(uint16_t port_id)
{
- if (is_proc_primary())
- returnrte_eth_dev_stop(port_id);
+ int ret;
+
+ if (is_proc_primary()) {
+ ret =rte_eth_dev_stop(port_id);
+ if (ret != 0)Quoted 9 lines
app/test-pmd/testpmd.c
Quoted 3 lines
+ return ret;
app/test-pmd/testpmd.c +2
-0+ +#ifdef RTE_NET_BOND Here and in other places - probably no need to pollute the code with all these 'ifdef
RTE_NET_BOND'.
I suppose this logic (for checking is bonding API present or not) can be hidden insidechange_bonding_slave_port_status()itself.
Quoted 2 lines
I think it does not pollute the code. anyone can tell according to the flag 'ifdef
RTE_NET_BOND'.
That what I am talking about.
Spreading ifdefed code all around the palce is not a good thing. It makes it harder to read, understand and maintain.
Much more plausible is to hide that logic in one place whenever possible.
Quoted 1 line
if hiddle inside '
change_bonding_slave_port_status', it will be weird.
I don't see why is that.
Let say if bonding is disabled that function can do nothing or even return an error to avoid it's misuse.
Quoted 1 line
For example, if the port is not bonding port, It will also invoke '
change_bonding_slave_port_status'. That is unreasonable.
As I can read the code, hange_bonding_slave_port_status() will be invoked only when port->bond_flag is set. Which implies that port is a bonded one, no?
Quoted 34 lines
Quoted 34 lines
Quoted 24 lines
+ struct
rte_port*port = &ports[port_id];
+
+ /*
+ * Stopping a bonded port also stops all slaves under the bonded
+ * device. So if this port is bond device, we need to modify the
+ * port status of these slaves.
+ */
+ if (port->bond_flag== 1)
+ returnchange_bonding_slave_port_status(port_id, true); +#endif
+ }
return 0;
}app/test-pmd/testpmd.h +0
-0@@ -266,7 +266,8 @@ struct rte_port {uint32_t
mc_addr_nb; /**< nb. of addr. inmc_addr_pool*/Quoted 8 lines
queueid_t
queue_nb; /**< nb. of queues for flow rules */ uint32_tqueue_sz; /**< size of a queue for flow rules */ - uint8_tslave_flag; /**< bonding slave port */ + uint8_tslave_flag: 1, /**< bonding slave port */ +bond_flag: 1; /**< port is bond device */ struct port_template*pattern_templ_list; /**< Pattern templates. */ struct port_template*actions_templ_list; /**< Actions templates. */ struct port_table*table_list; /**< Flow tables. */.
Re: [PATCH v3 3/5] app/testpmd: fix port status of slave device
In reply to Min Hu (Connor)Quoted 128 lines · Min Hu (Connor)
Hi, Konstantin,
Quoted 116 lines · Konstantin Ananyev
03/05/2022 11:02, Min Hu (Connor) пишет:
Quoted 115 lines
From: Huisong Li <[email protected]>
Starting or stopping a bonded port also starts or stops all active slaves under the bonded port. If this port is a bonded device, we need to modify the port status of all slaves.
Fixes 0e545d3047fe ("app/testpmd: check stopping port is not in bonding")Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>Acked-by Aman Singh <[email protected]>---
app/test-pmd/cmdline.c | 1 + app/test-pmd/testpmd.c | 74 +++++++++++++++++++++++++++++++++++++++--- app/test-pmd/testpmd.h | 3 +- 3 files changed, 73 insertions(+), 5 deletions(-)app/test-pmd/cmdline.c +0
-0@@ -6671,6 +6671,7 @@ static void cmd_create_bonded_device_parsed(void *parsed_result,"Failed to enable promiscuous mode for port %u: %s
- ignore\n",
port_id,rte_strerror(-ret));
+ ports[port_id].bond_flag = 1; ports[port_id].need_setup = 0; ports[port_id].port_status =RTE_PORT_STOPPED; }app/test-pmd/testpmd.c +32
-0@@ -66,6 +66,9 @@ #ifdef RTE_EXEC_ENV_WINDOWS #include <process.h> #endif +#ifdef RTE_NET_BOND +#include <rte_eth_bond.h> +#endif #include "testpmd.h"@@ -597,11 +600,57 @@ eth_dev_configure_mp(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q, return 0; } +#ifdef RTE_NET_BOND +static int +change_bonding_slave_port_status (portid_t bond_pid,bool is_stop) +{ + portid_t slave_pids[RTE_MAX_ETHPORTS]; + struct rte_port *port; + int num_slaves; + portid_t slave_pid; + int i; + + num_slaves = rte_eth_bond_slaves_get(bond_pid, slave_pids, + RTE_MAX_ETHPORTS); + if (num_slaves < 0) { + fprintf(stderr, "Failed to get slave list for port = %u\n", + bond_pid); + return num_slaves; + } + + for (i = 0; i < num_slaves; i++) { + slave_pid = slave_pids[i]; + port = &ports[slave_pid]; + port->port_status = + is_stop ? RTE_PORT_STOPPED : RTE_PORT_STARTED; + } + + return 0; +} +#endif +static int
eth_dev_start_mp(uint16_t port_id)
{
- if (is_proc_primary())
- returnrte_eth_dev_start(port_id);
+ int ret;app/test-pmd/testpmd.c +11
-0+ + if (is_proc_primary()) { + ret = rte_eth_dev_start(port_id); + if (ret != 0) + return ret; + +#ifdef RTE_NET_BOND + struct rte_port *port = &ports[port_id]; + + /* +* Starting a bonded port also starts all slaves under the bonded
+ * device. So if this port is bond device, we need to modify the
+ * port status of these slaves.
+ */
+ if (port->bond_flag== 1)
+ returnchange_bonding_slave_port_status(port_id, false); +#endif
+ }
return 0;
}app/test-pmd/testpmd.c +0
-0@@ -609,8 +658,25 @@ eth_dev_start_mp(uint16_t port_id)static int
eth_dev_stop_mp(uint16_t port_id)
{
- if (is_proc_primary())
- returnrte_eth_dev_stop(port_id);
+ int ret;
+
+ if (is_proc_primary()) {
+ ret =rte_eth_dev_stop(port_id);
+ if (ret != 0)Quoted 9 lines
app/test-pmd/testpmd.c
Quoted 3 lines
+ return ret;
app/test-pmd/testpmd.c +2
-0+ +#ifdef RTE_NET_BOND Here and in other places - probably no need to pollute the code with all these 'ifdef
RTE_NET_BOND'.
I suppose this logic (for checking is bonding API present or not) can be hidden insidechange_bonding_slave_port_status()itself.
Quoted 5 lines
I think it does not pollute the code. anyone can tell according to
the flag 'ifdefRTE_NET_BOND'.
if hiddle inside 'change_bonding_slave_port_status', it will be weird.
For example, if the port is not bonding port, It will also invoke 'change_bonding_slave_port_status'. That is unreasonable.
Hi Konstantin,
I also was not happy to have bonding (or any PMD) ifdef in the generic (start()/stop()) functions, but the ifdef blocks updates testpmd (application) level status. So that can't be handled in the PMD and need to be in the application level. Which is enforcing to have same PMD specific code in the testpmd level,
if you have any suggestion to prevent this, I am for it.
I will proceed with first two patch of this set, which fixes bonding PMD issues, I will hold the testpmd ones for more comments.
Thanks,
ferruh
Quoted 33 lines
Quoted 33 lines
Quoted 23 lines
+ struct
rte_port*port = &ports[port_id];
+
+ /*
+ * Stopping a bonded port also stops all slaves under the bonded
+ * device. So if this port is bond device, we need to modify the
+ * port status of these slaves.
+ */
+ if (port->bond_flag== 1)
+ returnchange_bonding_slave_port_status(port_id, true); +#endif
+ }
return 0;
}app/test-pmd/testpmd.h +0
-0@@ -266,7 +266,8 @@ struct rte_port {uint32_t
mc_addr_nb; /**< nb. of addr. inmc_addr_pool*/Quoted 8 lines
queueid_t
queue_nb; /**< nb. of queues for flow rules */ uint32_tqueue_sz; /**< size of a queue for flow rules */ - uint8_tslave_flag; /**< bonding slave port */ + uint8_tslave_flag: 1, /**< bonding slave port */ +bond_flag: 1; /**< port is bond device */ struct port_template*pattern_templ_list; /**< Pattern templates. */ struct port_template*actions_templ_list; /**< Actions templates. */ struct port_table*table_list; /**< Flow tables. */.
Re: [PATCH v3 3/5] app/testpmd: fix port status of slave device
Hi Ferruh,
Quoted 136 lines · Min Hu (Connor)
Quoted 129 lines
Hi, Konstantin,
Quoted 117 lines · Konstantin Ananyev
03/05/2022 11:02, Min Hu (Connor) пишет:
Quoted 116 lines
From: Huisong Li <[email protected]>
Starting or stopping a bonded port also starts or stops all active slaves under the bonded port. If this port is a bonded device, we need to modify the port status of all slaves.
Fixes 0e545d3047fe ("app/testpmd: check stopping port is not in bonding")Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>Acked-by Aman Singh <[email protected]>---
app/test-pmd/cmdline.c| 1 +
app/test-pmd/testpmd.c| 74
+++++++++++++++++++++++++++++++++++++++---
app/test-pmd/testpmd.h| 3 +-3 files changed, 73 insertions(+), 5 deletions(-)app/test-pmd/cmdline.c +0
-0@@ -6671,6 +6671,7 @@ static void cmd_create_bonded_device_parsed(void *parsed_result,"Failed to enable promiscuous mode for port %u: %s
- ignore\n",
port_id,rte_strerror(-ret));
+ ports[port_id].bond_flag = 1; ports[port_id].need_setup = 0; ports[port_id].port_status =RTE_PORT_STOPPED; }app/test-pmd/testpmd.c +32
-0@@ -66,6 +66,9 @@ #ifdef RTE_EXEC_ENV_WINDOWS #include <process.h> #endif +#ifdef RTE_NET_BOND +#include <rte_eth_bond.h> +#endif #include "testpmd.h"@@ -597,11 +600,57 @@ eth_dev_configure_mp(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q, return 0; } +#ifdef RTE_NET_BOND +static int +change_bonding_slave_port_status (portid_t bond_pid,bool is_stop) +{ + portid_t slave_pids[RTE_MAX_ETHPORTS]; + struct rte_port *port; + int num_slaves; + portid_t slave_pid; + int i; + + num_slaves = rte_eth_bond_slaves_get(bond_pid, slave_pids, + RTE_MAX_ETHPORTS); + if (num_slaves < 0) { + fprintf(stderr, "Failed to get slave list for port = %u\n", + bond_pid); + return num_slaves; + } + + for (i = 0; i < num_slaves; i++) { + slave_pid = slave_pids[i]; + port = &ports[slave_pid]; + port->port_status = + is_stop ? RTE_PORT_STOPPED : RTE_PORT_STARTED; + } + + return 0; +} +#endif +static int
eth_dev_start_mp(uint16_t port_id)
{
- if (is_proc_primary())
- returnrte_eth_dev_start(port_id);
+ int ret;app/test-pmd/testpmd.c +11
-0+ + if (is_proc_primary()) { + ret = rte_eth_dev_start(port_id); + if (ret != 0) + return ret; + +#ifdef RTE_NET_BOND + struct rte_port *port = &ports[port_id]; + + /* +* Starting a bonded port also starts all slaves under the bonded
+ * device. So if this port is bond device, we need to modify the
+ * port status of these slaves.
+ */
+ if (port->bond_flag== 1)
+ returnchange_bonding_slave_port_status(port_id, false); +#endif
+ }
return 0;
}app/test-pmd/testpmd.c +0
-0@@ -609,8 +658,25 @@ eth_dev_start_mp(uint16_t port_id)static int
eth_dev_stop_mp(uint16_t port_id)
{
- if (is_proc_primary())
- returnrte_eth_dev_stop(port_id);
+ int ret;
+
+ if (is_proc_primary()) {
+ ret =rte_eth_dev_stop(port_id);
+ if (ret != 0)Quoted 9 lines
app/test-pmd/testpmd.c
Quoted 3 lines
+ return ret;
app/test-pmd/testpmd.c +2
-0+ +#ifdef RTE_NET_BOND Here and in other places - probably no need to pollute the code with all these 'ifdef
RTE_NET_BOND'.
I suppose this logic (for checking is bonding API present or not) can be hidden insidechange_bonding_slave_port_status()itself.Quoted 5 lines
I think it does not pollute the code. anyone can tell according to
the flag 'ifdefRTE_NET_BOND'.
if hiddle inside 'change_bonding_slave_port_status', it will be weird.
For example, if the port is not bonding port, It will also invoke 'change_bonding_slave_port_status'. That is unreasonable.
Quoted 5 lines
Hi Konstantin,
I also was not happy to have bonding (or any PMD) ifdef in the generic (start()/stop()) functions, but the ifdef blocks updates testpmd (application) level status. So that can't be handled in the PMD and need to be in the application level. Which is enforcing to have same PMD specific code in the testpmd level,
if you have any suggestion to prevent this, I am for it.
I am not aking to move it to PMD.
What I am saying that this ifdef logic could be grouped in one place (inside change_bonding_slave_port_status()) instead of spreading it around multiple places.
Quoted 40 lines
I will proceed with first two patch of this set, which fixes bonding PMD issues, I will hold the testpmd ones for more comments.
Thanks,
ferruhQuoted 33 lines
Quoted 33 lines
Quoted 23 lines
+ struct
rte_port*port = &ports[port_id];
+
+ /*
+ * Stopping a bonded port also stops all slaves under the bonded
+ * device. So if this port is bond device, we need to modify the
+ * port status of these slaves.
+ */
+ if (port->bond_flag== 1)
+ returnchange_bonding_slave_port_status(port_id, true); +#endif
+ }
return 0;
}app/test-pmd/testpmd.h +0
-0@@ -266,7 +266,8 @@ struct rte_port {uint32_t
mc_addr_nb; /**< nb. of addr. inmc_addr_pool*/Quoted 8 lines
queueid_t
queue_nb; /**< nb. of queues for flow rules */ uint32_tqueue_sz; /**< size of a queue for flow rules */ - uint8_tslave_flag; /**< bonding slave port */ + uint8_tslave_flag: 1, /**< bonding slave port */ +bond_flag: 1; /**< port is bond device */ struct port_template*pattern_templ_list; /**< Pattern templates. */ struct port_template*actions_templ_list; /**< Actions templates. */ struct port_table*table_list; /**< Flow tables. */.
Re: [PATCH v3 3/5] app/testpmd: fix port status of slave device
Hi, Konstantin,
fixed in v4, thanks.
Quoted 193 lines · Konstantin Ananyev
Hi Ferruh,
Quoted 135 lines · Min Hu (Connor)
Quoted 128 lines
Hi, Konstantin,
Quoted 116 lines · Konstantin Ananyev
03/05/2022 11:02, Min Hu (Connor) пишет:
Quoted 115 lines
From: Huisong Li <[email protected]>
Starting or stopping a bonded port also starts or stops all active slaves under the bonded port. If this port is a bonded device, we need to modify the port status of all slaves.
Fixes 0e545d3047fe ("app/testpmd: check stopping port is not in bonding")Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>Acked-by Aman Singh <[email protected]>---
app/test-pmd/cmdline.c| 1 +
app/test-pmd/testpmd.c| 74
+++++++++++++++++++++++++++++++++++++++---
app/test-pmd/testpmd.h| 3 +-3 files changed, 73 insertions(+), 5 deletions(-)app/test-pmd/cmdline.c +1
-0@@ -6671,6 +6671,7 @@ static voidcmd_create_bonded_device_parsed (void *parsed_result, "Failed to enable promiscuous mode for port %u: %s - ignore\n", port_id, rte_strerror(-ret)); + ports[port_id].bond_flag = 1; ports[port_id].need_setup = 0; ports[port_id].port_status = RTE_PORT_STOPPED; }app/test-pmd/testpmd.c +32
-0@@ -66,6 +66,9 @@ #ifdef RTE_EXEC_ENV_WINDOWS #include <process.h> #endif +#ifdef RTE_NET_BOND +#include <rte_eth_bond.h> +#endif #include "testpmd.h"@@ -597,11 +600,57 @@ eth_dev_configure_mp(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q, return 0; } +#ifdef RTE_NET_BOND +static int +change_bonding_slave_port_status (portid_t bond_pid,bool is_stop) +{ + portid_t slave_pids[RTE_MAX_ETHPORTS]; + struct rte_port *port; + int num_slaves; + portid_t slave_pid; + int i; + + num_slaves = rte_eth_bond_slaves_get(bond_pid, slave_pids, + RTE_MAX_ETHPORTS); + if (num_slaves < 0) { + fprintf(stderr, "Failed to get slave list for port = %u\n", + bond_pid); + return num_slaves; + } + + for (i = 0; i < num_slaves; i++) { + slave_pid = slave_pids[i]; + port = &ports[slave_pid]; + port->port_status = + is_stop ? RTE_PORT_STOPPED : RTE_PORT_STARTED; + } + + return 0; +} +#endif +static int
eth_dev_start_mp(uint16_t port_id)
{
- if (is_proc_primary())
- returnrte_eth_dev_start(port_id);
+ int ret;app/test-pmd/testpmd.c +11
-0+ + if (is_proc_primary()) { + ret = rte_eth_dev_start(port_id); + if (ret != 0) + return ret; + +#ifdef RTE_NET_BOND + struct rte_port *port = &ports[port_id]; + + /* +* Starting a bonded port also starts all slaves under the bonded
+ * device. So if this port is bond device, we need to modify the
+ * port status of these slaves.
+ */
+ if (port->bond_flag== 1)
+ returnchange_bonding_slave_port_status(port_id, false); +#endif
+ }
return 0;
}app/test-pmd/testpmd.c +0
-0@@ -609,8 +658,25 @@ eth_dev_start_mp(uint16_t port_id)static int
eth_dev_stop_mp(uint16_t port_id)
{
- if (is_proc_primary())
- returnrte_eth_dev_stop(port_id);
+ int ret;
+
+ if (is_proc_primary()) {
+ ret =rte_eth_dev_stop(port_id);
+ if (ret != 0)Quoted 9 lines
app/test-pmd/testpmd.c
Quoted 3 lines
+ return ret;
app/test-pmd/testpmd.c +2
-0+ +#ifdef RTE_NET_BOND Here and in other places - probably no need to pollute the code with all these 'ifdef
RTE_NET_BOND'.
I suppose this logic (for checking is bonding API present or not) can be hidden insidechange_bonding_slave_port_status()itself.Quoted 5 lines
I think it does not pollute the code. anyone can tell according to
the flag 'ifdefRTE_NET_BOND'.
if hiddle inside 'change_bonding_slave_port_status', it will be weird.
For example, if the port is not bonding port, It will also invoke 'change_bonding_slave_port_status'. That is unreasonable.Quoted 5 lines
Hi Konstantin,
I also was not happy to have bonding (or any PMD) ifdef in the generic (start()/stop()) functions, but the ifdef blocks updates testpmd (application) level status. So that can't be handled in the PMD and need to be in the application level. Which is enforcing to have same PMD specific code in the testpmd level,
if you have any suggestion to prevent this, I am for it.I am not aking to move it to PMD.
What I am saying that this ifdef logic could be grouped in one place (insidechange_bonding_slave_port_status()) instead of spreading it around multiple places.Quoted 41 lines
I will proceed with first two patch of this set, which fixes bonding PMD issues, I will hold the testpmd ones for more comments.
Thanks,
ferruhQuoted 34 lines
Quoted 34 lines
Quoted 24 lines
+ struct
rte_port*port = &ports[port_id];
+
+ /*
+ * Stopping a bonded port also stops all slaves under the bonded
+ * device. So if this port is bond device, we need to modify the
+ * port status of these slaves.
+ */
+ if (port->bond_flag== 1)
+ returnchange_bonding_slave_port_status(port_id, true); +#endif
+ }
return 0;
}app/test-pmd/testpmd.h +0
-0@@ -266,7 +266,8 @@ struct rte_port {uint32_t
mc_addr_nb; /**< nb. of addr. inmc_addr_pool*/ queueid_tqueue_nb; /**< nb. of queues for flow rules */Quoted 8 lines
uint32_t
queue_sz; /**< size of a queue for flow rules */ - uint8_tslave_flag; /**< bonding slave port */ + uint8_tslave_flag: 1, /**< bonding slave port */
+bond_flag: 1; /**< port is bond device */ struct port_template*pattern_templ_list; /**< Pattern templates. */ struct port_template*actions_templ_list; /**< Actions templates. */ struct port_table*table_list; /**< Flow tables. */.
.
Re: [PATCH v3 3/5] app/testpmd: fix port status of slave device
On 5/11/2022 3:16 AM, Min Hu (Connor) wrote:
<...>
Quoted 33 lines
Quoted 33 lines
Quoted 31 lines
Quoted 20 lines
Quoted 11 lines
Quoted 11 lines
(quoted diff) +0
-0@@ -609,8 +658,25 @@ eth_dev_start_mp(uint16_t port_id)static int
eth_dev_stop_mp(uint16_t port_id)
{
- if (is_proc_primary())
- returnrte_eth_dev_stop(port_id);
+ int ret;
+
+ if (is_proc_primary()) {
+ ret =rte_eth_dev_stop(port_id);
+ if (ret != 0)Quoted 9 lines
Review
Quoted 3 lines
+ return ret;
(quoted diff) +2
-0+ +#ifdef RTE_NET_BONDHere and in other places - probably no need to pollute the code with all these 'ifdef
RTE_NET_BOND'.
I suppose this logic (for checking is bonding API present or not) can be hidden insidechange_bonding_slave_port_status()itself.Quoted 5 lines
I think it does not pollute the code. anyone can tell according to
the flag 'ifdefRTE_NET_BOND'.
if hiddle inside 'change_bonding_slave_port_status', it will be weird.
For example, if the port is not bonding port, It will also invoke 'change_bonding_slave_port_status'. That is unreasonable.Hi Konstantin,
I also was not happy to have bonding (or any PMD) ifdef in the generic (start()/stop()) functions, but the ifdef blocks updates testpmd (application) level status. So that can't be handled in the PMD and need to be in the application level. Which is enforcing to have same PMD specific code in the testpmd level,
if you have any suggestion to prevent this, I am for it.
Quoted 8 lines
Quoted 5 lines
I am not aking to move it to PMD.
What I am saying that this ifdef logic could be grouped in one place (insidechange_bonding_slave_port_status()) instead of spreading it around multiple places.Hi, Konstantin,
fixed in v4, thanks.
Hi Conor,
What do you think to apply same on patch 4/5?
PATCH v4 1/1 app/testpmd: fix port status of slave device
In reply to Min Hu (Connor)From: Huisong Li <[email protected]>
Starting or stopping a bonded port also starts or stops all active slaves under the bonded port. If this port is a bonded device, we need to modify the port status of all slaves.
---
v4:
* set 'ifdef logic' grouped inside change_bonding_slave_port_status.
--- app/test-pmd/cmdline.c | 1 + app/test-pmd/testpmd.c | 73 +++++++++++++++++++++++++++++++++++++++--- app/test-pmd/testpmd.h | 3 +- 3 files changed, 72 insertions(+), 5 deletions(-)
app/test-pmd/cmdline.c +1 -0
@@ -6671,6 +6671,7 @@ static void cmd_create_bonded_device_parsed(void *parsed_result,"Failed to enable promiscuous mode for port %u: %s - ignore\n" , port_id, rte_strerror(-ret)); + ports[port_id].bond_flag =1 ; ports[port_id].need_setup =0 ; ports[port_id].port_status =RTE_PORT_STOPPED ; }
app/test-pmd/testpmd.c +69 -4
94 lines not on this page · raw message
app/test-pmd/testpmd.h +2 -1
@@ -266,7 +266,8 @@ struct rte_port {uint32_t mc_addr_nb;/**< nb. of addr. in mc_addr_pool */ queueid_t queue_nb;/**< nb. of queues for flow rules */ uint32_t queue_sz;/**< size of a queue for flow rules */ -uint8_t slave_flag;/**< bonding slave port */ + uint8_t slave_flag : 1, /**< bonding slave port */ + bond_flag : 1; /**< port is bond device */struct port_template *pattern_templ_list;/**< Pattern templates. */ struct port_template *actions_templ_list;/**< Actions templates. */ struct port_table *table_list;/**< Flow tables. */
Signature · 1 line
2.33.0
Re: [PATCH v4] app/testpmd: fix port status of slave device
Quoted 140 lines
From: Huisong Li <[email protected]>
Starting or stopping a bonded port also starts or stops all active slaves under the bonded port. If this port is a bonded device, we need to modify the port status of all slaves.
Fixes 0e545d3047fe ("app/testpmd: check stopping port is not in bonding")Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>Acked-by Aman Singh <[email protected]>---
v4:
* set 'ifdef logic' grouped insidechange_bonding_slave_port_status.--- app/test-pmd/cmdline.c | 1 + app/test-pmd/testpmd.c | 73 +++++++++++++++++++++++++++++++++++++++--- app/test-pmd/testpmd.h | 3 +- 3 files changed, 72 insertions(+), 5 deletions(-)app/test-pmd/cmdline.c +1
-0@@ -6671,6 +6671,7 @@ static void cmd_create_bonded_device_parsed(void *parsed_result,"Failed to enable promiscuous mode for port %u: %s - ignore\n" , port_id, rte_strerror(-ret)); + ports[port_id].bond_flag =1 ; ports[port_id].need_setup =0 ; ports[port_id].port_status =RTE_PORT_STOPPED ; }app/test-pmd/testpmd.c +69
-4@@ -66,6 +66,9 @@#ifdef RTE_EXEC_ENV_WINDOWS #include <process.h> #endif +#ifdef RTE_NET_BOND +#include <rte_eth_bond.h> +#endif #include "testpmd.h" @@ -597,11 +600,58 @@ eth_dev_configure_mp(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,return 0 ; } +static int +change_bonding_slave_port_status (portid_t bond_pid,bool is_stop) +{ +#ifdef RTE_NET_BOND + +portid_t slave_pids[RTE_MAX_ETHPORTS ]; +struct rte_port *port; +int num_slaves; +portid_t slave_pid; +int i; + + num_slaves =rte_eth_bond_slaves_get (bond_pid, slave_pids, +RTE_MAX_ETHPORTS ); +if (num_slaves <0 ) { +fprintf (stderr,"Failed to get slave list for port = %u\n" , + bond_pid); +return num_slaves; + } + +for (i =0 ; i < num_slaves; i++) { + slave_pid = slave_pids[i]; + port = &ports[slave_pid]; + port->port_status = + is_stop ?RTE_PORT_STOPPED :RTE_PORT_STARTED ; + } +#else +RTE_SET_USED (bond_pid); +RTE_SET_USED (is_stop); +#endif +return 0 ; +} +static int eth_dev_start_mp (uint16_t port_id) { -if (is_proc_primary ()) -return rte_eth_dev_start (port_id); +int ret; + +if (is_proc_primary ()) { + ret =rte_eth_dev_start (port_id); +if (ret !=0 ) +return ret; + +struct rte_port *port = &ports[port_id]; + +/* +* Starting a bonded port also starts all slaves under the bonded +* device. So if this port is bond device, we need to modify the +* port status of these slaves. +*/ +if (port->bond_flag ==1 ) +return change_bonding_slave_port_status (port_id, false); + }return 0 ; }@@ -609,8 +659,23 @@ eth_dev_start_mp(uint16_t port_id)static int eth_dev_stop_mp (uint16_t port_id) { -if (is_proc_primary ()) -return rte_eth_dev_stop (port_id); +int ret; + +if (is_proc_primary ()) { + ret =rte_eth_dev_stop (port_id); +if (ret !=0 ) +return ret; + +struct rte_port *port = &ports[port_id]; + +/* +* Stopping a bonded port also stops all slaves under the bonded +* device. So if this port is bond device, we need to modify the +* port status of these slaves. +*/ +if (port->bond_flag ==1 ) +return change_bonding_slave_port_status (port_id, true); + }return 0 ; }app/test-pmd/testpmd.h +0
-0
app/test-pmd/testpmd.h struct rte_port { · line 273
Quoted 9 lines
app/test-pmd/testpmd.h +2
-1@@ -266,7 +266,8 @@ struct rte_port {uint32_t mc_addr_nb;/**< nb. of addr. in mc_addr_pool */ queueid_t queue_nb;/**< nb. of queues for flow rules */ uint32_t queue_sz;/**< size of a queue for flow rules */ -uint8_t slave_flag;/**< bonding slave port */ + uint8_t slave_flag : 1, /**< bonding slave port */ + bond_flag : 1; /**< port is bond device */struct port_template *pattern_templ_list;/**< Pattern templates. */ struct port_template *actions_templ_list;/**< Actions templates. */ struct port_table *table_list;/**< Flow tables. */
Andrew Rybchenko
AppliedRe: [PATCH v4] app/testpmd: fix port status of slave device
Quoted 7 lines · Konstantin Ananyev
Quoted 7 lines
From: Huisong Li <[email protected]>
Starting or stopping a bonded port also starts or stops all active slaves under the bonded port. If this port is a bonded device, we need to modify the port status of all slaves.
Quoted 8 lines
Quoted 6 lines
Fixes 0e545d3047fe ("app/testpmd: check stopping port is not in bonding")Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>Acked-by Aman Singh <[email protected]>
Applied, thanks.
PATCH v3 4/5 app/testpmd: fix slave device isn't released
In reply to Min Hu (Connor)From: Huisong Li <[email protected]>
Currently, some eth devices are added to bond device, these devices are not released when the quit command is executed in testpmd. This patch adds the release operation for all active slaves under a bond device.
--- app/test-pmd/cmdline.c | 1 + app/test-pmd/testpmd.c | 48 ++++++++++++++++++++++++++++++++++++++++++ app/test-pmd/testpmd.h | 2 ++ 3 files changed, 51 insertions(+)
app/test-pmd/cmdline.c +1 -0
@@ -8891,6 +8891,7 @@ static void cmd_quit_parsed(__rte_unused void *parsed_result, __rte_unused void *data) {cmdline_quit (cl); + cl_quit =1 ; } cmdline_parse_token_string_t cmd_quit_quit =
app/test-pmd/testpmd.c +48 -0
72 lines not on this page · raw message
app/test-pmd/testpmd.h +2 -0
@@ -32,6 +32,8 @@#define RTE_PORT_CLOSED (uint16_t)2#define RTE_PORT_HANDLING (uint16_t)3 +extern uint8_t cl_quit; +/* * It is used to allocate the memory for hash key. * The hash key size is NIC dependent.
Signature · 1 line
2.33.0
Re: [PATCH v3 4/5] app/testpmd: fix slave device isn't released
Quoted 11 lines · Min Hu (Connor)
From: Huisong Li<[email protected]>
Currently, some eth devices are added to bond device, these devices are not released when the quit command is executed in testpmd. This patch adds the release operation for all active slaves under a bond device.
Fixes 0e545d3047fe ("app/testpmd: check stopping port is not in bonding")Signed-off-by Huisong Li<[email protected]>Signed-off-by Min Hu (Connor)<[email protected]>
Hi Connor,
Can you please do the similar #ifdef grouping done in 3/5 patch?
Re: [PATCH v3 4/5] app/testpmd: fix slave device isn't released
Hi Ferruh
"[PATCH v4] app/testpmd: fix slave device isn't released" has been sent out, help to review.
Thanks,
Dongdong
Quoted 21 lines · Ferruh Yigit
Quoted 8 lines · Min Hu (Connor)
From: Huisong Li<[email protected]>
Currently, some eth devices are added to bond device, these devices are not
released when the quit command is executed in testpmd. This patch adds the
release operation for all active slaves under a bond device.Quoted 6 lines
Fixes 0e545d3047fe ("app/testpmd: check stopping port is not inbonding")
Cc:[email protected]Signed-off-by Huisong Li<[email protected]>Signed-off-by Min Hu (Connor)<[email protected]>Hi Connor,
Can you please do the similar #ifdef grouping done in 3/5 patch? .
PATCH v4 1/1 app/testpmd: fix slave device isn't released
In reply to Min Hu (Connor)From: Huisong Li <[email protected]>
Currently, some eth devices are added to bond device, these devices are not released when the quit command is executed in testpmd. This patch adds the release operation for all active slaves under a bond device.
--- app/test-pmd/cmdline.c | 1 + app/test-pmd/testpmd.c | 46 ++++++++++++++++++++++++++++++++++++++++++ app/test-pmd/testpmd.h | 2 ++ 3 files changed, 49 insertions(+)
app/test-pmd/cmdline.c +1 -0
@@ -8915,6 +8915,7 @@ static void cmd_quit_parsed(__rte_unused void *parsed_result, __rte_unused void *data) {cmdline_quit (cl); + cl_quit =1 ; } static cmdline_parse_token_string_t cmd_quit_quit =
app/test-pmd/testpmd.c +46 -0
70 lines not on this page · raw message
app/test-pmd/testpmd.h +2 -0
@@ -38,6 +38,8 @@#define RTE_PORT_CLOSED (uint16_t)2#define RTE_PORT_HANDLING (uint16_t)3 +extern uint8_t cl_quit; +/* * It is used to allocate the memory for hash key. * The hash key size is NIC dependent.
Signature · 1 line
2.33.0
Re: [PATCH v4] app/testpmd: fix slave device isn't released
Quoted 65 lines · Dongdong Liu
From: Huisong Li <[email protected]>
Currently, some eth devices are added to bond device, these devices are not released when the quit command is executed in testpmd. This patch adds the release operation for all active slaves under a bond device.
Fixes 0e545d3047fe ("app/testpmd: check stopping port is not in bonding")Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>Signed-off-by Dongdong Liu <[email protected]>--- app/test-pmd/cmdline.c | 1 + app/test-pmd/testpmd.c | 46 ++++++++++++++++++++++++++++++++++++++++++ app/test-pmd/testpmd.h | 2 ++ 3 files changed, 49 insertions(+)app/test-pmd/cmdline.c +1
-0@@ -8915,6 +8915,7 @@ static void cmd_quit_parsed(__rte_unused void *parsed_result, __rte_unused void *data) {cmdline_quit (cl); + cl_quit =1 ; } static cmdline_parse_token_string_t cmd_quit_quit =app/test-pmd/testpmd.c +20
-0@@ -229,6 +229,7 @@ unsigned int xstats_display_num; /**< Size of extended statistics to show */* option. Set flag to exit stats period loop after received SIGINT/SIGTERM. */ uint8_t f_quit; +uint8_t cl_quit;/* Quit testpmd from cmdline. */ /* * Max Rx frame size, set by '--max-pkt-len' parameter. @@ -3201,11 +3202,44 @@ remove_invalid_ports(void) nb_cfg_ports = nb_fwd_ports; } +static void +clear_bonding_slave_device (portid_t *slave_pids,uint16_t num_slaves) +{ +#ifdef RTE_NET_BOND +struct rte_port *port; +portid_t slave_pid; +uint16_t i; + +for (i =0 ; i < num_slaves; i++) { + slave_pid = slave_pids[i]; +if (port_is_started (slave_pid) ==1 ) { +if (rte_eth_dev_stop (slave_pid) !=0 ) +fprintf (stderr,"rte_eth_dev_stop failed for port %u\n" , + slave_pid); + + port = &ports[slave_pid]; + port->port_status =RTE_PORT_STOPPED ; + } +
app/test-pmd/testpmd.c remove_invalid_ports(void) · line 3233
Quoted 10 lines
app/test-pmd/testpmd.c +10
-0+clear_port_slave_flag (slave_pid); + +/* Close slave device when testpmd quit or is killed. */ +if (cl_quit ==1 || f_quit ==1 ) + rte_eth_dev_close(slave_pid); + } +#else +RTE_SET_USED (slave_pids); +RTE_SET_USED (num_slaves); +#endif
Do we need this #ifdef, everything is already available in compile time.
Quoted 45 lines
app/test-pmd/testpmd.c +16
-0+} +void close_port (portid_t pid) {portid_t pi;struct rte_port *port; +portid_t slave_pids[RTE_MAX_ETHPORTS ]; +int num_slaves =0 ;if (port_id_is_invalid (pid,ENABLED_WARN ))return ;@@ -3240,7 +3274,19 @@ close_port(portid_t pid)port_flow_flush (pi);port_flex_item_flush (pi);port_action_handle_flush (pi); +#ifdef RTE_NET_BOND +if (port->bond_flag ==1 ) + num_slaves =rte_eth_bond_slaves_get (pi, + slave_pids,RTE_MAX_ETHPORTS ); +#endif rte_eth_dev_close (pi); +/* +* If this port is bonded device, all slaves under the +* device need to be removed or closed. +*/ +if (port->bond_flag ==1 && num_slaves >0 ) +clear_bonding_slave_device (slave_pids, + num_slaves); }free_xstats_display_info (pi);app/test-pmd/testpmd.h +2
-0@@ -38,6 +38,8 @@#define RTE_PORT_CLOSED (uint16_t)2#define RTE_PORT_HANDLING (uint16_t)3 +extern uint8_t cl_quit; +/* * It is used to allocate the memory for hash key. * The hash key size is NIC dependent.
Re: [PATCH v4] app/testpmd: fix slave device isn't released
Hi Ferruh
Many thanks for your review.
Quoted 73 lines · Ferruh Yigit
Quoted 72 lines · Dongdong Liu
From: Huisong Li <[email protected]>
Currently, some eth devices are added to bond device, these devices are not
released when the quit command is executed in testpmd. This patch adds the
release operation for all active slaves under a bond device.Fixes 0e545d3047fe ("app/testpmd: check stopping port is not inbonding")
Cc: [email protected]Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>Signed-off-by Dongdong Liu <[email protected]>--- app/test-pmd/cmdline.c | 1 + app/test-pmd/testpmd.c | 46 ++++++++++++++++++++++++++++++++++++++++++ app/test-pmd/testpmd.h | 2 ++ 3 files changed, 49 insertions(+)app/test-pmd/cmdline.c +1
-0@@ -8915,6 +8915,7 @@ static void cmd_quit_parsed(__rte_unused void *parsed_result, __rte_unused void *data) {cmdline_quit (cl); + cl_quit =1 ; } static cmdline_parse_token_string_t cmd_quit_quit =app/test-pmd/testpmd.c +0
-0@@ -229,6 +229,7 @@ unsigned int xstats_display_num; /**< Size ofextended statistics to show */
* option. Set flag to exit stats period loop after received
SIGINT/SIGTERM.
*/
uint8_tf_quit;app/test-pmd/testpmd.c +23
-0+uint8_t cl_quit;/* Quit testpmd from cmdline. */ /* * Max Rx frame size, set by '--max-pkt-len' parameter. @@ -3201,11 +3202,44 @@ remove_invalid_ports(void) nb_cfg_ports = nb_fwd_ports; } +static void +clear_bonding_slave_device (portid_t *slave_pids,uint16_t num_slaves) +{ +#ifdef RTE_NET_BOND +struct rte_port *port; +portid_t slave_pid; +uint16_t i; + +for (i =0 ; i < num_slaves; i++) { + slave_pid = slave_pids[i]; +if (port_is_started (slave_pid) ==1 ) { +if (rte_eth_dev_stop (slave_pid) !=0 ) +fprintf (stderr,"rte_eth_dev_stop failed for port %u\n" , + slave_pid); + + port = &ports[slave_pid]; + port->port_status =RTE_PORT_STOPPED ; + } + +clear_port_slave_flag (slave_pid); + +/* Close slave device when testpmd quit or is killed. */ +if (cl_quit ==1 || f_quit ==1 )
Quoted 8 lines
app/test-pmd/testpmd.c remove_invalid_ports(void) · line 3233
Quoted 6 lines
app/test-pmd/testpmd.c +6
-0+ rte_eth_dev_close(slave_pid); + } +#else +RTE_SET_USED (slave_pids); +RTE_SET_USED (num_slaves); +#endif Do we need this #ifdef, everything is already available in compile time.
Although it can be compiled ok without this #ifdef, I think we still need this #ifdef as this is bond related implementation.
This patch does the similar #ifdef grouping done in 3/5 patch.
Thanks,
Dongdong
Quoted 46 lines
Quoted 36 lines
app/test-pmd/testpmd.c +16
-0+} +void close_port (portid_t pid) {portid_t pi;struct rte_port *port; +portid_t slave_pids[RTE_MAX_ETHPORTS ]; +int num_slaves =0 ;if (port_id_is_invalid (pid,ENABLED_WARN ))return ;@@ -3240,7 +3274,19 @@ close_port(portid_t pid)port_flow_flush (pi);port_flex_item_flush (pi);port_action_handle_flush (pi); +#ifdef RTE_NET_BOND +if (port->bond_flag ==1 ) + num_slaves =rte_eth_bond_slaves_get (pi, + slave_pids,RTE_MAX_ETHPORTS ); +#endif rte_eth_dev_close (pi); +/* +* If this port is bonded device, all slaves under the +* device need to be removed or closed. +*/ +if (port->bond_flag ==1 && num_slaves >0 ) +clear_bonding_slave_device (slave_pids, + num_slaves); }free_xstats_display_info (pi);app/test-pmd/testpmd.h +0
-0app/test-pmd/testpmd.h line 44
Quoted 7 lines
app/test-pmd/testpmd.h +1
-0@@ -38,6 +38,8 @@#define RTE_PORT_CLOSED (uint16_t)2#define RTE_PORT_HANDLING (uint16_t)3 +extern uint8_t cl_quit; +/* * It is used to allocate the memory for hash key. * The hash key size is NIC dependent. .
Re: [PATCH v4] app/testpmd: fix slave device isn't released
Quoted 87 lines · Dongdong Liu
Hi Ferruh
Many thanks for your review.
Quoted 74 lines · Ferruh Yigit
Quoted 73 lines · Dongdong Liu
From: Huisong Li <[email protected]>
Currently, some eth devices are added to bond device, these devices are not
released when the quit command is executed in testpmd. This patch adds the
release operation for all active slaves under a bond device.Fixes 0e545d3047fe ("app/testpmd: check stopping port is not inbonding")
Cc: [email protected]Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>Signed-off-by Dongdong Liu <[email protected]>---
app/test-pmd/cmdline.c | 1 + app/test-pmd/testpmd.c | 46 ++++++++++++++++++++++++++++++++++++++++++ app/test-pmd/testpmd.h | 2 ++ 3 files changed, 49 insertions(+)app/test-pmd/cmdline.c +1
-0@@ -8915,6 +8915,7 @@ static void cmd_quit_parsed(__rte_unused void *parsed_result, __rte_unused void *data) { cmdline_quit(cl); + cl_quit = 1; }static
cmdline_parse_token_string_tcmd_quit_quit=app/test-pmd/testpmd.c +0
-0@@ -229,6 +229,7 @@ unsigned int xstats_display_num; /**< Size ofextended statistics to show */
* option. Set flag to exit stats period loop after received SIGINT/SIGTERM.
*/
uint8_tf_quit;
+uint8_tcl_quit; /* Quit testpmd from cmdline. */ /*
* Max Rx frame size, set by '--max-pkt-len' parameter.app/test-pmd/testpmd.c +0
-0@@ -3201,11 +3202,44 @@ remove_invalid_ports(void) nb_cfg_ports = nb_fwd_ports; }+static void
app/test-pmd/testpmd.c +12
-0+clear_bonding_slave_device(portid_t *slave_pids, uint16_t num_slaves) +{ +#ifdef RTE_NET_BOND + struct rte_port *port; + portid_t slave_pid; + uint16_t i; + + for (i = 0; i < num_slaves; i++) { + slave_pid = slave_pids[i]; + if (port_is_started(slave_pid) == 1) { + if (rte_eth_dev_stop(slave_pid) != 0) + fprintf(stderr, "rte_eth_dev_stop failed for port%u\n",
+slave_pid);app/test-pmd/testpmd.c +9
-0+ + port = &ports[slave_pid]; + port->port_status = RTE_PORT_STOPPED; + } + + clear_port_slave_flag(slave_pid); + + /* Close slave device when testpmd quit or is killed. */ + if (cl_quit == 1 || f_quit == 1)Quoted 8 lines
app/test-pmd/testpmd.c remove_invalid_ports(void) · line 3233
Quoted 6 lines
+
rte_eth_dev_close(slave_pid);
+ }app/test-pmd/testpmd.c +4
-0+#else + RTE_SET_USED(slave_pids); + RTE_SET_USED(num_slaves); +#endif Do we need this #ifdef, everything is already available in compile time.
Quoted 4 lines
Although it can be compiled ok without this #ifdef, I think we still need this #ifdef as this is bond related implementation.
Code will compile and function as expected even if bonding PMD is disable, right? If so why #ifdef is needed.
Quoted 50 lines
This patch does the similar #ifdef grouping done in 3/5 patch.
Thanks,
DongdongQuoted 46 lines
Quoted 36 lines
app/test-pmd/testpmd.c +2
-0+} +void
close_port(portid_t pid)
{
portid_tpi;
structrte_port*port;
+portid_tslave_pids[RTE_MAX_ETHPORTS]; + intnum_slaves= 0;
if (port_id_is_invalid(pid, ENABLED_WARN)) return;app/test-pmd/testpmd.c +12
-0@@ -3240,7 +3274,19 @@ close_port(portid_t pid) port_flow_flush(pi); port_flex_item_flush(pi); port_action_handle_flush(pi); +#ifdef RTE_NET_BOND + if (port->bond_flag == 1) + num_slaves = rte_eth_bond_slaves_get(pi, + slave_pids, RTE_MAX_ETHPORTS); +#endif rte_eth_dev_close(pi); + /* + * If this port is bonded device, all slaves under the + * device need to be removed or closed. + */ + if (port->bond_flag == 1 && num_slaves > 0) + clear_bonding_slave_device(slave_pids, + num_slaves); } free_xstats_display_info(pi);app/test-pmd/testpmd.h +0
-0app/test-pmd/testpmd.h line 44
Quoted 7 lines
app/test-pmd/testpmd.h +1
-0@@ -38,6 +38,8 @@ #define RTE_PORT_CLOSED (uint16_t)2 #define RTE_PORT_HANDLING (uint16_t)3 +extern uint8_t cl_quit; + /** It is used to allocate the memory for hash key. * The hash key size is NIC dependent. .
Re: [PATCH v4] app/testpmd: fix slave device isn't released
Hi Ferruh
Many thanks for your review.
Quoted 89 lines · Ferruh Yigit
Quoted 88 lines · Dongdong Liu
Hi Ferruh
Many thanks for your review.
Quoted 83 lines · Ferruh Yigit
Quoted 70 lines · Dongdong Liu
From: Huisong Li <[email protected]>
Currently, some eth devices are added to bond device, these devices are not
released when the quit command is executed in testpmd. This patch adds the
release operation for all active slaves under a bond device.Fixes 0e545d3047fe ("app/testpmd: check stopping port is not inbonding")
Cc: [email protected]Signed-off-by Huisong Li <[email protected]>Signed-off-by Min Hu (Connor) <[email protected]>Signed-off-by Dongdong Liu <[email protected]>--- app/test-pmd/cmdline.c | 1 + app/test-pmd/testpmd.c | 46 ++++++++++++++++++++++++++++++++++++++++++ app/test-pmd/testpmd.h | 2 ++ 3 files changed, 49 insertions(+)app/test-pmd/cmdline.c +1
-0@@ -8915,6 +8915,7 @@ static void cmd_quit_parsed(__rte_unused void *parsed_result, __rte_unused void *data) {cmdline_quit (cl); + cl_quit =1 ; } static cmdline_parse_token_string_t cmd_quit_quit =app/test-pmd/testpmd.c +0
-0@@ -229,6 +229,7 @@ unsigned int xstats_display_num; /**< Size ofextended statistics to show */
* option. Set flag to exit stats period loop after received
SIGINT/SIGTERM.
*/
uint8_tf_quit;app/test-pmd/testpmd.c +19
-0+uint8_t cl_quit;/* Quit testpmd from cmdline. */ /* * Max Rx frame size, set by '--max-pkt-len' parameter. @@ -3201,11 +3202,44 @@ remove_invalid_ports(void) nb_cfg_ports = nb_fwd_ports; } +static void +clear_bonding_slave_device (portid_t *slave_pids,uint16_t num_slaves) +{ +#ifdef RTE_NET_BOND +struct rte_port *port; +portid_t slave_pid; +uint16_t i; + +for (i =0 ; i < num_slaves; i++) { + slave_pid = slave_pids[i]; +if (port_is_started (slave_pid) ==1 ) { +if (rte_eth_dev_stop (slave_pid) !=0 ) + fprintf(stderr, "rte_eth_dev_stop failed for port %u\n", + slave_pid); + + port = &ports[slave_pid]; + port->port_status =RTE_PORT_STOPPED ; + } +app/test-pmd/testpmd.c remove_invalid_ports(void) · line 3233
Quoted 10 lines
app/test-pmd/testpmd.c +10
-0+clear_port_slave_flag (slave_pid); + +/* Close slave device when testpmd quit or is killed. */ +if (cl_quit ==1 || f_quit ==1 ) + rte_eth_dev_close(slave_pid); + } +#else +RTE_SET_USED (slave_pids); +RTE_SET_USED (num_slaves); +#endif Do we need this #ifdef, everything is already available in compile time.
Quoted 7 lines
Quoted 4 lines
Although it can be compiled ok without this #ifdef, I think we still need this #ifdef as this is bond related implementation.
Code will compile and function as expected even if bonding PMD is disable, right? If so why #ifdef is needed.
Yes, will delete.
Thanks,
Dongdong
Quoted 54 lines
Quoted 51 lines
This patch does the similar #ifdef grouping done in 3/5 patch.
Thanks,
DongdongQuoted 46 lines
Quoted 36 lines
app/test-pmd/testpmd.c +16
-0+} +void close_port (portid_t pid) {portid_t pi;struct rte_port *port; +portid_t slave_pids[RTE_MAX_ETHPORTS ]; +int num_slaves =0 ;if (port_id_is_invalid (pid,ENABLED_WARN ))return ;@@ -3240,7 +3274,19 @@ close_port(portid_t pid)port_flow_flush (pi);port_flex_item_flush (pi);port_action_handle_flush (pi); +#ifdef RTE_NET_BOND +if (port->bond_flag ==1 ) + num_slaves =rte_eth_bond_slaves_get (pi, + slave_pids,RTE_MAX_ETHPORTS ); +#endif rte_eth_dev_close (pi); +/* +* If this port is bonded device, all slaves under the +* device need to be removed or closed. +*/ +if (port->bond_flag ==1 && num_slaves >0 ) +clear_bonding_slave_device (slave_pids, + num_slaves); }free_xstats_display_info (pi);app/test-pmd/testpmd.h +0
-0app/test-pmd/testpmd.h line 44
Quoted 7 lines
app/test-pmd/testpmd.h +1
-0@@ -38,6 +38,8 @@#define RTE_PORT_CLOSED (uint16_t)2#define RTE_PORT_HANDLING (uint16_t)3 +extern uint8_t cl_quit; +/* * It is used to allocate the memory for hash key. * The hash key size is NIC dependent. .
.
PATCH v5 1/1 app/testpmd: fix slave device isn't released
In reply to Min Hu (Connor)From: Huisong Li <[email protected]>
Currently, some eth devices are added to bond device, these devices are not released when the quit command is executed in testpmd. This patch adds the release operation for all active slaves under a bond device.
---v4->v5:
- Delete the no needed #ifdef
--- app/test-pmd/cmdline.c | 1 + app/test-pmd/testpmd.c | 41 +++++++++++++++++++++++++++++++++++++++++ app/test-pmd/testpmd.h | 2 ++ 3 files changed, 44 insertions(+)
app/test-pmd/cmdline.c +1 -0
@@ -8915,6 +8915,7 @@ static void cmd_quit_parsed(__rte_unused void *parsed_result, __rte_unused void *data) {cmdline_quit (cl); + cl_quit =1 ; } static cmdline_parse_token_string_t cmd_quit_quit =
app/test-pmd/testpmd.c +41 -0
65 lines not on this page · raw message
app/test-pmd/testpmd.h +2 -0
@@ -38,6 +38,8 @@#define RTE_PORT_CLOSED (uint16_t)2#define RTE_PORT_HANDLING (uint16_t)3 +extern uint8_t cl_quit; +/* * It is used to allocate the memory for hash key. * The hash key size is NIC dependent.
Signature · 1 line
2.33.0
Re: [PATCH v5] app/testpmd: fix slave device isn't released
Quoted 6 lines · Dongdong Liu
From: Huisong Li<[email protected]>
Currently, some eth devices are added to bond device, these devices are not released when the quit command is executed in testpmd. This patch adds the release operation for all active slaves under a bond device.
Quoted 6 lines
Fixes 0e545d3047fe ("app/testpmd: check stopping port is not in bonding")Signed-off-by Huisong Li<[email protected]>Signed-off-by Min Hu (Connor)<[email protected]>Signed-off-by Dongdong Liu<[email protected]>
Applied to dpdk-next-net/main, thanks.
PATCH v3 5/5 ethdev: fix dev state when stop
In reply to Min Hu (Connor)Currently, 'dev_started' is always set to be 0 when dev stop, whether it succeeded or failed. This is unreasonable and this patch fixed it.
--- lib/ethdev/rte_ethdev.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
lib/ethdev/rte_ethdev.c +2 -1
@@ -1533,8 +1533,9 @@ rte_eth_dev_stop(uint16_t port_id)/* point fast-path functions to dummy ones */ eth_dev_fp_ops_reset (rte_eth_fp_ops + port_id); - dev->data->dev_started =0 ; ret = (*dev->dev_ops->dev_stop)(dev); +if (ret ==0 ) + dev->data->dev_started =0 ;rte_ethdev_trace_stop (port_id, ret);return ret;
Signature · 1 line
2.33.0
Re: [PATCH v3 5/5] ethdev: fix dev state when stop
Quoted 16 lines · Min Hu (Connor)
Currently, '
dev_started' is always set to be 0 when dev stop, whether it succeeded or failed. This is unreasonable and this patch fixed it.Fixes 62024eb82756 ("ethdev: change stop operation callback to return int")Signed-off-by Min Hu (Connor) <[email protected]>--- lib/ethdev/rte_ethdev.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)lib/ethdev/rte_ethdev.c +0
-0
lib/ethdev/rte_ethdev.c rte_eth_dev_stop(uint16_t port_id) · line 1541
Quoted 10 lines
lib/ethdev/rte_ethdev.c +2
-1@@ -1533,8 +1533,9 @@ rte_eth_dev_stop(uint16_t port_id)/* point fast-path functions to dummy ones */ eth_dev_fp_ops_reset (rte_eth_fp_ops + port_id); - dev->data->dev_started =0 ; ret = (*dev->dev_ops->dev_stop)(dev); +if (ret ==0 ) + dev->data->dev_started =0 ;rte_ethdev_trace_stop (port_id, ret);return ret;
Change looks good to me, I checked for possible unexpected side effect but I did not see any.
@Andrew, @Thomas, if you also don't see/remember any issue related change, I will push it soon.
Thomas Monjalon
Acked-byRe: [PATCH v3 5/5] ethdev: fix dev state when stop
25/05/2022 19:44, Ferruh Yigit:
Quoted 26 lines · Min Hu (Connor)
Quoted 25 lines
Currently, '
dev_started' is always set to be 0 when dev stop, whether it succeeded or failed. This is unreasonable and this patch fixed it.Fixes 62024eb82756 ("ethdev: change stop operation callback to return int")Signed-off-by Min Hu (Connor) <[email protected]>--- lib/ethdev/rte_ethdev.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)lib/ethdev/rte_ethdev.c +2
-1@@ -1533,8 +1533,9 @@ rte_eth_dev_stop(uint16_t port_id)/* point fast-path functions to dummy ones */ eth_dev_fp_ops_reset (rte_eth_fp_ops + port_id); - dev->data->dev_started =0 ; ret = (*dev->dev_ops->dev_stop)(dev); +if (ret ==0 ) + dev->data->dev_started =0 ;rte_ethdev_trace_stop (port_id, ret);
Quoted 7 lines
lib/ethdev/rte_ethdev.c rte_eth_dev_stop(uint16_t port_id) · line 1541
Quoted 1 line
lib/ethdev/rte_ethdev.c +0
-0return ret;Change looks good to me, I checked for possible unexpected side effect but I did not see any.
@Andrew, @Thomas, if you also don't see/remember any issue related change, I will push it soon.
Re: [PATCH v3 5/5] ethdev: fix dev state when stop
Quoted 34 lines · Thomas Monjalon
[CAUTION: External Email]
25/05/2022 19:44, Ferruh Yigit:
Quoted 31 lines · Min Hu (Connor)
Quoted 16 lines
Currently, '
dev_started' is always set to be 0 when dev stop, whether it succeeded or failed. This is unreasonable and this patch fixed it.Fixes 62024eb82756 ("ethdev: change stop operation callback to return int")Signed-off-by Min Hu (Connor) <[email protected]>--- lib/ethdev/rte_ethdev.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)lib/ethdev/rte_ethdev.c +0
-0lib/ethdev/rte_ethdev.c rte_eth_dev_stop(uint16_t port_id) · line 1541
Quoted 10 lines
lib/ethdev/rte_ethdev.c +2
-1@@ -1533,8 +1533,9 @@ rte_eth_dev_stop(uint16_t port_id)/* point fast-path functions to dummy ones */ eth_dev_fp_ops_reset (rte_eth_fp_ops + port_id); - dev->data->dev_started =0 ; ret = (*dev->dev_ops->dev_stop)(dev); +if (ret ==0 ) + dev->data->dev_started =0 ;rte_ethdev_trace_stop (port_id, ret);return ret;Change looks good to me, I checked for possible unexpected side effect but I did not see any.
Quoted 5 lines
Quoted 2 lines
@Andrew, @Thomas, if you also don't see/remember any issue related change, I will push it soon.
Acked-by Thomas Monjalon <[email protected]>
Applied to dpdk-next-net/main, thanks.
Re: [PATCH v3 0/5] bugfix for bonding
In reply to Min Hu (Connor)Quoted 16 lines · Min Hu (Connor)
Fix some bonding related bug for bonding PMD and testpmd.
Huisong Li (4):
net/bonding: fix non-active slaves aren't stopped net/bonding: fix non-terminable while loop app/testpmd: fix port status of slave device app/testpmd: fix slave device isn't releasedMin Hu (Connor) (1):
ethdev: fix dev state when stop---
v3:
- fix comments
- add a new patch to fix dev state.
Quoted 5 lines
v2:
- add two patches in net/bonding.
- change port status of slave device instart_port
- delete removing slave operation when close bond port
For 1/5 & 2/5,
Applied to dpdk-next-net/main, thanks.