[2/3] app/testpmd: fix slave device isn't released
Checks
Commit Message
From: Huisong Li <lihuisong@huawei.com>
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")
Cc: stable@dpdk.org
Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
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(-)
Comments
On 10/25/2021 7:39 AM, Min Hu (Connor) wrote:
> From: Huisong Li <lihuisong@huawei.com>
>
> 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.
> Fixes: 0e545d3047fe ("app/testpmd: check stopping port is not in bonding")
> Cc: stable@dpdk.org
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
> 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(-)
>
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 5bfb4b509b..98236a8be3 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -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 =
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index d6b9ebc4dd..fea9744bd6 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -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)
> {
> @@ -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.
> +{
> + 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);
> + }
> +
> + /*
> + * 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?
> + 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);
> }
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index ad3b4f875c..216fc41432 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -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.
在 2022/2/4 20:14, Ferruh Yigit 写道:
> On 10/25/2021 7:39 AM, Min Hu (Connor) wrote:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> 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
>
> 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.
>
>> Fixes: 0e545d3047fe ("app/testpmd: check stopping port is not in
>> bonding")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>> 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(-)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>> index 5bfb4b509b..98236a8be3 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -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 =
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>> index d6b9ebc4dd..fea9744bd6 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -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)
>> {
>> @@ -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.
ok
>
>> +{
>> + 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);
>> + }
>> +
>> + /*
>> + * 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.
>
>> + 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);
>> }
>> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
>> index ad3b4f875c..216fc41432 100644
>> --- a/app/test-pmd/testpmd.h
>> +++ b/app/test-pmd/testpmd.h
>> @@ -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.
>
> .
@@ -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 =
@@ -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)
{
@@ -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)
+{
+ 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);
+ }
+
+ /*
+ * 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)
+ 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);
}
@@ -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.