[dpdk-dev] [PATCH 0/3] bugfix for testpmd

[dpdk-dev] [PATCH 0/3] bugfix for testpmd

[Min Hu (Connor)]

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(-)

-- 
2.33.0

[dpdk-dev] [PATCH 1/3] app/testpmd: fix port status of active slave device

[Min Hu (Connor)]

From: Huisong Li <lihuisong@huawei.com>

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.

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 | 49 +++++++++++++++++++++++++++++++++++++++---
 app/test-pmd/testpmd.h |  3 ++-
 3 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 722f4fb9d9..5bfb4b509b 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -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;
 	}
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index af0e79fe6d..d6b9ebc4dd 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -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)
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index e3995d24ab..ad3b4f875c 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -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. */
-- 
2.33.0

[dpdk-dev] [PATCH 2/3] app/testpmd: fix slave device isn't released

[Min Hu (Connor)]

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(-)

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)
+{
+	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);
 		}
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.
-- 
2.33.0

[dpdk-dev] [PATCH 3/3] app/testpmd: remove unused header file

[Min Hu (Connor)]

From: Huisong Li <lihuisong@huawei.com>

This patch removes unused "rte_eth_bond.h" header file.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 app/test-pmd/parameters.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index ab8e8f7e69..1dc83200a8 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -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"
-- 
2.33.0

Re: [dpdk-dev] [PATCH 3/3] app/testpmd: remove unused header file

[Ferruh Yigit]

On 10/25/2021 7:39 AM, Min Hu (Connor) wrote:
> From: Huisong Li <lihuisong@huawei.com>
> 
> This patch removes unused "rte_eth_bond.h" header file.
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>

     Fixes: 2950a769315e ("bond: testpmd support")
     Cc: stable@dpdk.org

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>


Only for this patch, not series (since this is trivial),
Applied to dpdk-next-net/main, thanks.

Re: [dpdk-dev] [PATCH 1/3] app/testpmd: fix port status of active slave device

[Singh, Aman Deep]

On 10/25/2021 12:09 PM, Min Hu (Connor) wrote:
> From: Huisong Li <lihuisong@huawei.com>
>
> 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.
>
> 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>
> ---

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

>   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(-)
>
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 722f4fb9d9..5bfb4b509b 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -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;
>   	}
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index af0e79fe6d..d6b9ebc4dd 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -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)
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index e3995d24ab..ad3b4f875c 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -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

[lihuisong (C)]

在 2021/11/15 21:01, Singh, Aman Deep 写道:
> On 10/25/2021 12:09 PM, Min Hu (Connor) wrote:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> 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.
>>
>> 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>
>> ---
>
> 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.

>
>>   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(-)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>> index 722f4fb9d9..5bfb4b509b 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -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;
>>       }
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>> index af0e79fe6d..d6b9ebc4dd 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -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)
>> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
>> index e3995d24ab..ad3b4f875c 100644
>> --- a/app/test-pmd/testpmd.h
>> +++ b/app/test-pmd/testpmd.h
>> @@ -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

[Singh, Aman Deep]

[-- Attachment #1: Type: text/plain, Size: 718 bytes --]


On 11/16/2021 6:50 AM, lihuisong (C) wrote:
>
> 在 2021/11/15 21:01, Singh, Aman Deep 写道:
>> On 10/25/2021 12:09 PM, Min Hu (Connor) wrote:
>>> From: Huisong Li <lihuisong@huawei.com>
>>>
>>> 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.
>>>
>>> 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>
>>> ---
>>
Looks good to me.

Acked-by: Aman Singh <aman.deep.singh@intel.com> <snip>


[-- Attachment #2: Type: text/html, Size: 1964 bytes --]

Re: [dpdk-dev] [PATCH 1/3] app/testpmd: fix port status of active slave device

[Ferruh Yigit]

On 10/25/2021 7:39 AM, Min Hu (Connor) wrote:
> From: Huisong Li <lihuisong@huawei.com>
> 
> 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.
> 
> 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 | 49 +++++++++++++++++++++++++++++++++++++++---
>   app/test-pmd/testpmd.h |  3 ++-
>   3 files changed, 49 insertions(+), 4 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 722f4fb9d9..5bfb4b509b 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -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;
>   	}
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index af0e79fe6d..d6b9ebc4dd 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -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)

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?

> +{
> +	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);

Can you please remove the 'eth_dev_stop_mp()' function in this patch,
which is removed in patch 2/3.

> +		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?

> +		}
>   
>   		if (rte_atomic16_cmpset(&(port->port_status),
>   			RTE_PORT_HANDLING, RTE_PORT_STOPPED) == 0)
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index e3995d24ab..ad3b4f875c 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -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?

>   	struct port_flow        *flow_list; /**< Associated flows. */
>   	struct port_indirect_action *actions_list;
>   	/**< Associated indirect actions. */

Re: [dpdk-dev] [PATCH 2/3] app/testpmd: fix slave device isn't released

[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?

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.

Re: [dpdk-dev] [PATCH 2/3] app/testpmd: fix slave device isn't released

[lihuisong (C)]

在 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.
>
> .

Re: [dpdk-dev] [PATCH 1/3] app/testpmd: fix port status of active slave device

[lihuisong (C)]

在 2022/2/4 20:07, Ferruh Yigit 写道:
> On 10/25/2021 7:39 AM, Min Hu (Connor) wrote:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> 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.
>>
>> 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 | 49 +++++++++++++++++++++++++++++++++++++++---
>>   app/test-pmd/testpmd.h |  3 ++-
>>   3 files changed, 49 insertions(+), 4 deletions(-)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>> index 722f4fb9d9..5bfb4b509b 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -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;
>>       }
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>> index af0e79fe6d..d6b9ebc4dd 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -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)
>
> 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
>
>> +{
>> +    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);
>
> Can you please remove the 'eth_dev_stop_mp()' function in this patch,
> which is removed in patch 2/3.
ok
>
>> +        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?
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 <radu.nicolau@intel.com>
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.

     Fixes: 7486331308f6 ("net/bonding: stop and deactivate slaves on stop")
     Cc: stable@dpdk.org

     Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
     Acked-by: Chas Williams <chas3@att.com>
*/

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?
>
>> +        }
>>             if (rte_atomic16_cmpset(&(port->port_status),
>>               RTE_PORT_HANDLING, RTE_PORT_STOPPED) == 0)
>> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
>> index e3995d24ab..ad3b4f875c 100644
>> --- a/app/test-pmd/testpmd.h
>> +++ b/app/test-pmd/testpmd.h
>> @@ -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?
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.
>
>>       struct port_flow        *flow_list; /**< Associated flows. */
>>       struct port_indirect_action *actions_list;
>>       /**< Associated indirect actions. */
>
> .

[PATCH V2 0/4] bugfix for bonding

[Min Hu (Connor)]

From: Huisong Li <lihuisong@huawei.com>

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(-)

-- 
2.33.0

[PATCH V2 1/4] net/bonding: fix non-active slaves aren't stopped

[Min Hu (Connor)]

From: Huisong Li <lihuisong@huawei.com>

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.

Fixes: 0911d4ec0183 ("net/bonding: fix crash when stopping mode 4 port")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index b305b6a35b..469dc71170 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -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;
-- 
2.33.0

[PATCH V2 2/4] net/bonding: fix non-terminable while loop

[Min Hu (Connor)]

From: Huisong Li <lihuisong@huawei.com>

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.

Fixes: fb0379bc5db3 ("net/bonding: check stop call status")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 469dc71170..00d4deda44 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -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) {
-- 
2.33.0

[PATCH V2 3/4] app/testpmd: fix port status of slave device

[Min Hu (Connor)]

From: Huisong Li <lihuisong@huawei.com>

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")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
Acked-by: Aman Singh <aman.deep.singh@intel.com>
---
 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(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 7ab0575e64..f0efcf09f0 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -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;
 	}
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index fe2ce19f99..dc90600787 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -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)
 {
-	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
+		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;
 }
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 31f766c965..67f253b30e 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -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. */
-- 
2.33.0

[PATCH V2 4/4] app/testpmd: fix slave device isn't released

[Min Hu (Connor)]

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 | 48 ++++++++++++++++++++++++++++++++++++++++++
 app/test-pmd/testpmd.h |  2 ++
 3 files changed, 51 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index f0efcf09f0..cd3873e1e0 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -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 =
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index dc90600787..22700e073d 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -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);
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 67f253b30e..5af9455012 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -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.
-- 
2.33.0

Re: [PATCH V2 0/4] bugfix for bonding

[Min Hu (Connor)]

Hi, Ferruh,
	what do you think of this patch?

在 2022/3/24 11:00, Min Hu (Connor) 写道:
> From: Huisong Li <lihuisong@huawei.com>
> 
> 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(-)
> 

Re: [PATCH V2 1/4] net/bonding: fix non-active slaves aren't stopped

[Ferruh Yigit]

On 3/24/2022 3:00 AM, Min Hu (Connor) wrote:
> From: Huisong Li <lihuisong@huawei.com>
> 
> When stopping a bonded port, all slaves should be deactivated. But only

s/deactivated/stopped/ ?

> active slaves are stopped. So fix it and do "deactivae_slave()" for active

s/deactivae_slave()/deactivate_slave()/

> slaves.

Hi Connor,

When a bonding port is closed, is it clear if all slave ports or active 
slave ports should be stopped?

> 
> Fixes: 0911d4ec0183 ("net/bonding: fix crash when stopping mode 4 port")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>   drivers/net/bonding/rte_eth_bond_pmd.c | 20 +++++++++++---------
>   1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index b305b6a35b..469dc71170 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -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?

> +		}
> +
> +		/* active slaves need to deactivate. */

" 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)

I think original indentation for this line is better.

>   			deactivate_slave(eth_dev, slave_id);
> -		}
>   	}
>   
>   	return 0;

Re: [PATCH V2 2/4] net/bonding: fix non-terminable while loop

[Ferruh Yigit]

On 3/24/2022 3:00 AM, Min Hu (Connor) wrote:
> From: Huisong Li <lihuisong@huawei.com>
> 
> 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.
> 

Agree that this is a defect introduced in below commit. Thanks for the fix.

> Fixes: fb0379bc5db3 ("net/bonding: check stop call status")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>   drivers/net/bonding/rte_eth_bond_pmd.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index 469dc71170..00d4deda44 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -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.

>   		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;

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?

>   		}
>   
>   		if (rte_eth_bond_slave_remove(bond_port_id, port_id) != 0) {

Re: [PATCH V2 1/4] net/bonding: fix non-active slaves aren't stopped

[Min Hu (Connor)]

Hi, Ferruh,

在 2022/4/27 2:19, Ferruh Yigit 写道:
> On 3/24/2022 3:00 AM, Min Hu (Connor) wrote:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> 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.

> 
>> active slaves are stopped. So fix it and do "deactivae_slave()" for 
>> active
> 
> s/deactivae_slave()/deactivate_slave()/
> 
agreed.

>> 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).
> 
>>
>> Fixes: 0911d4ec0183 ("net/bonding: fix crash when stopping mode 4 port")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>>   drivers/net/bonding/rte_eth_bond_pmd.c | 20 +++++++++++---------
>>   1 file changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c 
>> b/drivers/net/bonding/rte_eth_bond_pmd.c
>> index b305b6a35b..469dc71170 100644
>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>> @@ -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?
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.

> 
>> +        }
>> +
>> +        /* active slaves need to deactivate. */
> 
> " active slaves need to be deactivated. " ?
agreed.
> 
>>           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)
> 
> I think original indentation for this line is better.
> 
agreed.
>>               deactivate_slave(eth_dev, slave_id);
>> -        }
>>       }
>>       return 0;
> 
> .

Re: [PATCH V2 2/4] net/bonding: fix non-terminable while loop

[Min Hu (Connor)]

Hi, Ferruh,

在 2022/4/27 2:19, Ferruh Yigit 写道:
> On 3/24/2022 3:00 AM, Min Hu (Connor) wrote:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> 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.
>>
> 
> Agree that this is a defect introduced in below commit. Thanks for the fix.
thanks.
> 
>> Fixes: fb0379bc5db3 ("net/bonding: check stop call status")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>>   drivers/net/bonding/rte_eth_bond_pmd.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c 
>> b/drivers/net/bonding/rte_eth_bond_pmd.c
>> index 469dc71170..00d4deda44 100644
>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>> @@ -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.
> 
>>           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;
> 
> 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_remove
rte_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;
}

> 
>>           }
>>           if (rte_eth_bond_slave_remove(bond_port_id, port_id) != 0) {
> 
> .

Re: [PATCH V2 1/4] net/bonding: fix non-active slaves aren't stopped

[Ferruh Yigit]

On 4/29/2022 7:45 AM, Min Hu (Connor) wrote:
> Hi, Ferruh,
> 
> 在 2022/4/27 2:19, Ferruh Yigit 写道:
>> On 3/24/2022 3:00 AM, Min Hu (Connor) wrote:
>>> From: Huisong Li <lihuisong@huawei.com>
>>>
>>> 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.

>>
>>> active slaves are stopped. So fix it and do "deactivae_slave()" for 
>>> active
>>
>> s/deactivae_slave()/deactivate_slave()/
>>
> agreed.
> 
>>> 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).
>>
>>>
>>> Fixes: 0911d4ec0183 ("net/bonding: fix crash when stopping mode 4 port")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>> ---
>>>   drivers/net/bonding/rte_eth_bond_pmd.c | 20 +++++++++++---------
>>>   1 file changed, 11 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c 
>>> b/drivers/net/bonding/rte_eth_bond_pmd.c
>>> index b305b6a35b..469dc71170 100644
>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>> @@ -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?
> 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?

>>
>>> +        }
>>> +
>>> +        /* active slaves need to deactivate. */
>>
>> " active slaves need to be deactivated. " ?
> agreed.
>>
>>>           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)
>>
>> I think original indentation for this line is better.
>>
> agreed.
>>>               deactivate_slave(eth_dev, slave_id);
>>> -        }
>>>       }
>>>       return 0;
>>
>> .

Re: [PATCH V2 2/4] net/bonding: fix non-terminable while loop

[Ferruh Yigit]

On 4/29/2022 7:52 AM, Min Hu (Connor) wrote:
> Hi, Ferruh,
> 
> 在 2022/4/27 2:19, Ferruh Yigit 写道:
>> On 3/24/2022 3:00 AM, Min Hu (Connor) wrote:
>>> From: Huisong Li <lihuisong@huawei.com>
>>>
>>> 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.
>>>
>>
>> Agree that this is a defect introduced in below commit. Thanks for the 
>> fix.
> thanks.
>>
>>> Fixes: fb0379bc5db3 ("net/bonding: check stop call status")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>> ---
>>>   drivers/net/bonding/rte_eth_bond_pmd.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c 
>>> b/drivers/net/bonding/rte_eth_bond_pmd.c
>>> index 469dc71170..00d4deda44 100644
>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>> @@ -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.
>>
>>>           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;
>>
>> 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.

> just see the function stack:
> rte_eth_bond_slave_remove
> __eth_bond_slave_remove_lock_free
> slave_remove
> rte_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;
> }
> 
>>
>>>           }
>>>           if (rte_eth_bond_slave_remove(bond_port_id, port_id) != 0) {
>>
>> .

Re: [PATCH V2 1/4] net/bonding: fix non-active slaves aren't stopped

[Min Hu (Connor)]

Hi, Ferruh,

在 2022/4/29 21:31, Ferruh Yigit 写道:
> On 4/29/2022 7:45 AM, Min Hu (Connor) wrote:
>> Hi, Ferruh,
>>
>> 在 2022/4/27 2:19, Ferruh Yigit 写道:
>>> On 3/24/2022 3:00 AM, Min Hu (Connor) wrote:
>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>
>>>> 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.
Actually, there is something wrong with the wording.
Yes, I should take your advice.

> 
>>>
>>>> active slaves are stopped. So fix it and do "deactivae_slave()" for 
>>>> active
>>>
>>> s/deactivae_slave()/deactivate_slave()/
>>>
>> agreed.
>>
>>>> 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).
>>>
>>>>
>>>> Fixes: 0911d4ec0183 ("net/bonding: fix crash when stopping mode 4 
>>>> port")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>> ---
>>>>   drivers/net/bonding/rte_eth_bond_pmd.c | 20 +++++++++++---------
>>>>   1 file changed, 11 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c 
>>>> b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>> index b305b6a35b..469dc71170 100644
>>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>> @@ -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?
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.


>> 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?
> 
>>>
>>>> +        }
>>>> +
>>>> +        /* active slaves need to deactivate. */
>>>
>>> " active slaves need to be deactivated. " ?
>> agreed.
>>>
>>>>           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)
>>>
>>> I think original indentation for this line is better.
>>>
>> agreed.
>>>>               deactivate_slave(eth_dev, slave_id);
>>>> -        }
>>>>       }
>>>>       return 0;
>>>
>>> .
> 
> .

[PATCH v3 0/5] bugfix for bonding

[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(-)

-- 
2.33.0

[PATCH v3 1/5] net/bonding: fix non-active slaves aren't stopped

[Min Hu (Connor)]

From: Huisong Li <lihuisong@huawei.com>

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.

Fixes: 0911d4ec0183 ("net/bonding: fix crash when stopping mode 4 port")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index b305b6a35b..92caf24f4b 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -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;
-- 
2.33.0

[PATCH v3 2/5] net/bonding: fix non-terminable while loop

[Min Hu (Connor)]

From: Huisong Li <lihuisong@huawei.com>

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.

Fixes: fb0379bc5db3 ("net/bonding: check stop call status")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 92caf24f4b..8f48ba7d23 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -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) {
-- 
2.33.0

[PATCH v3 3/5] app/testpmd: fix port status of slave device

[Min Hu (Connor)]

From: Huisong Li <lihuisong@huawei.com>

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")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
Acked-by: Aman Singh <aman.deep.singh@intel.com>
---
 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(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 6ffea8e21a..d9fc7a88bd 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -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;
 	}
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index fe2ce19f99..dc90600787 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -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)
 {
-	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
+		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;
 }
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 31f766c965..67f253b30e 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -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. */
-- 
2.33.0

[PATCH v3 4/5] app/testpmd: fix slave device isn't released

[Min Hu (Connor)]

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 | 48 ++++++++++++++++++++++++++++++++++++++++++
 app/test-pmd/testpmd.h |  2 ++
 3 files changed, 51 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index d9fc7a88bd..f3c1d9ab79 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -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 =
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index dc90600787..22700e073d 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -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);
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 67f253b30e..5af9455012 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -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.
-- 
2.33.0

[PATCH v3 5/5] ethdev: fix dev state when stop

[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")
Cc: stable@dpdk.org

Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 29a3d80466..e0011372aa 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -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;
-- 
2.33.0

Re: [PATCH V2 1/4] net/bonding: fix non-active slaves aren't stopped

[Ferruh Yigit]

On 5/3/2022 7:54 AM, Min Hu (Connor) wrote:
> Hi, Ferruh,
> 
> 在 2022/4/29 21:31, Ferruh Yigit 写道:
>> On 4/29/2022 7:45 AM, Min Hu (Connor) wrote:
>>> Hi, Ferruh,
>>>
>>> 在 2022/4/27 2:19, Ferruh Yigit 写道:
>>>> On 3/24/2022 3:00 AM, Min Hu (Connor) wrote:
>>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>>
>>>>> 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.
> Actually, there is something wrong with the wording.
> Yes, I should take your advice.
> 
>>
>>>>
>>>>> active slaves are stopped. So fix it and do "deactivae_slave()" for 
>>>>> active
>>>>
>>>> s/deactivae_slave()/deactivate_slave()/
>>>>
>>> agreed.
>>>
>>>>> 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).
>>>>
>>>>>
>>>>> Fixes: 0911d4ec0183 ("net/bonding: fix crash when stopping mode 4 
>>>>> port")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>>> ---
>>>>>   drivers/net/bonding/rte_eth_bond_pmd.c | 20 +++++++++++---------
>>>>>   1 file changed, 11 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c 
>>>>> b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>> index b305b6a35b..469dc71170 100644
>>>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>> @@ -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?
> 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.
> 

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.


> 
>>> 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?
>>
>>>>
>>>>> +        }
>>>>> +
>>>>> +        /* active slaves need to deactivate. */
>>>>
>>>> " active slaves need to be deactivated. " ?
>>> agreed.
>>>>
>>>>>           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)
>>>>
>>>> I think original indentation for this line is better.
>>>>
>>> agreed.
>>>>>               deactivate_slave(eth_dev, slave_id);
>>>>> -        }
>>>>>       }
>>>>>       return 0;
>>>>
>>>> .
>>
>> .

Re: [PATCH v3 3/5] app/testpmd: fix port status of slave device

[Konstantin Ananyev]

03/05/2022 11:02, Min Hu (Connor) пишет:
> From: Huisong Li <lihuisong@huawei.com>
> 
> 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")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> Acked-by: Aman Singh <aman.deep.singh@intel.com>
> ---
>   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(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 6ffea8e21a..d9fc7a88bd 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -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;
>   	}
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index fe2ce19f99..dc90600787 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -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)
>   {
> -	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.


> +		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;
>   }
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index 31f766c965..67f253b30e 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -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 V2 1/4] net/bonding: fix non-active slaves aren't stopped

[Min Hu (Connor)]

Hi, Ferruh,

在 2022/5/4 3:04, Ferruh Yigit 写道:
> On 5/3/2022 7:54 AM, Min Hu (Connor) wrote:
>> Hi, Ferruh,
>>
>> 在 2022/4/29 21:31, Ferruh Yigit 写道:
>>> On 4/29/2022 7:45 AM, Min Hu (Connor) wrote:
>>>> Hi, Ferruh,
>>>>
>>>> 在 2022/4/27 2:19, Ferruh Yigit 写道:
>>>>> On 3/24/2022 3:00 AM, Min Hu (Connor) wrote:
>>>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>>>
>>>>>> 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.
>> Actually, there is something wrong with the wording.
>> Yes, I should take your advice.
>>
>>>
>>>>>
>>>>>> active slaves are stopped. So fix it and do "deactivae_slave()" 
>>>>>> for active
>>>>>
>>>>> s/deactivae_slave()/deactivate_slave()/
>>>>>
>>>> agreed.
>>>>
>>>>>> 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).
>>>>>
>>>>>>
>>>>>> Fixes: 0911d4ec0183 ("net/bonding: fix crash when stopping mode 4 
>>>>>> port")
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>>>> ---
>>>>>>   drivers/net/bonding/rte_eth_bond_pmd.c | 20 +++++++++++---------
>>>>>>   1 file changed, 11 insertions(+), 9 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c 
>>>>>> b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>> index b305b6a35b..469dc71170 100644
>>>>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>> @@ -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?
>> 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.
>>
> 
> 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.
Yes, this patch can be abandoned from this set.

> 
> 
>>
>>>> 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?
>>>
>>>>>
>>>>>> +        }
>>>>>> +
>>>>>> +        /* active slaves need to deactivate. */
>>>>>
>>>>> " active slaves need to be deactivated. " ?
>>>> agreed.
>>>>>
>>>>>>           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)
>>>>>
>>>>> I think original indentation for this line is better.
>>>>>
>>>> agreed.
>>>>>>               deactivate_slave(eth_dev, slave_id);
>>>>>> -        }
>>>>>>       }
>>>>>>       return 0;
>>>>>
>>>>> .
>>>
>>> .
> 
> .

Re: [PATCH v3 3/5] app/testpmd: fix port status of slave device

[Min Hu (Connor)]

Hi, Konstantin,

在 2022/5/4 7:39, Konstantin Ananyev 写道:
> 03/05/2022 11:02, Min Hu (Connor) пишет:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> 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")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> Acked-by: Aman Singh <aman.deep.singh@intel.com>
>> ---
>>   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(-)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>> index 6ffea8e21a..d9fc7a88bd 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -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;
>>       }
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>> index fe2ce19f99..dc90600787 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -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)
>>   {
>> -    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.
> 
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.


> 
>> +        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;
>>   }
>> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
>> index 31f766c965..67f253b30e 100644
>> --- a/app/test-pmd/testpmd.h
>> +++ b/app/test-pmd/testpmd.h
>> @@ -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

[Konstantin Ananyev]

Hi Conor,

> 在 2022/5/4 7:39, Konstantin Ananyev 写道:
>> 03/05/2022 11:02, Min Hu (Connor) пишет:
>>> From: Huisong Li <lihuisong@huawei.com>
>>>
>>> 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")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>> Acked-by: Aman Singh <aman.deep.singh@intel.com>
>>> ---
>>>   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(-)
>>>
>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>>> index 6ffea8e21a..d9fc7a88bd 100644
>>> --- a/app/test-pmd/cmdline.c
>>> +++ b/app/test-pmd/cmdline.c
>>> @@ -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;
>>>       }
>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>>> index fe2ce19f99..dc90600787 100644
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -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)
>>>   {
>>> -    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.
>>
> 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.


> 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.

> 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?

> 
> 
>>
>>> +        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;
>>>   }
>>> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
>>> index 31f766c965..67f253b30e 100644
>>> --- a/app/test-pmd/testpmd.h
>>> +++ b/app/test-pmd/testpmd.h
>>> @@ -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

[Ferruh Yigit]

On 5/6/2022 9:16 AM, Min Hu (Connor) wrote:
> Hi, Konstantin,
> 
> 在 2022/5/4 7:39, Konstantin Ananyev 写道:
>> 03/05/2022 11:02, Min Hu (Connor) пишет:
>>> From: Huisong Li <lihuisong@huawei.com>
>>>
>>> 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")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>> Acked-by: Aman Singh <aman.deep.singh@intel.com>
>>> ---
>>>   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(-)
>>>
>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>>> index 6ffea8e21a..d9fc7a88bd 100644
>>> --- a/app/test-pmd/cmdline.c
>>> +++ b/app/test-pmd/cmdline.c
>>> @@ -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;
>>>       }
>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>>> index fe2ce19f99..dc90600787 100644
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -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)
>>>   {
>>> -    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.
>>
> 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.
> 

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

> 
>>
>>> +        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;
>>>   }
>>> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
>>> index 31f766c965..67f253b30e 100644
>>> --- a/app/test-pmd/testpmd.h
>>> +++ b/app/test-pmd/testpmd.h
>>> @@ -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

[Konstantin Ananyev]

Hi Ferruh,

> On 5/6/2022 9:16 AM, Min Hu (Connor) wrote:
>> Hi, Konstantin,
>>
>> 在 2022/5/4 7:39, Konstantin Ananyev 写道:
>>> 03/05/2022 11:02, Min Hu (Connor) пишет:
>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>
>>>> 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")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>> Acked-by: Aman Singh <aman.deep.singh@intel.com>
>>>> ---
>>>>   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(-)
>>>>
>>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>>>> index 6ffea8e21a..d9fc7a88bd 100644
>>>> --- a/app/test-pmd/cmdline.c
>>>> +++ b/app/test-pmd/cmdline.c
>>>> @@ -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;
>>>>       }
>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>>>> index fe2ce19f99..dc90600787 100644
>>>> --- a/app/test-pmd/testpmd.c
>>>> +++ b/app/test-pmd/testpmd.c
>>>> @@ -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)
>>>>   {
>>>> -    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.
>>>
>> 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.
>>
> 
> 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.

> 
> 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
> 
>>
>>>
>>>> +        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;
>>>>   }
>>>> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
>>>> index 31f766c965..67f253b30e 100644
>>>> --- a/app/test-pmd/testpmd.h
>>>> +++ b/app/test-pmd/testpmd.h
>>>> @@ -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. */
>>>
>>> .
> 

[PATCH v4] app/testpmd: fix port status of slave device

[Min Hu (Connor)]

From: Huisong Li <lihuisong@huawei.com>

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")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
Acked-by: Aman Singh <aman.deep.singh@intel.com>
---
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(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 6ffea8e21a..d9fc7a88bd 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -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;
 	}
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index fe2ce19f99..51fa344bb2 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -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;
 }
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 31f766c965..67f253b30e 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -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. */
-- 
2.33.0

Re: [PATCH v3 3/5] app/testpmd: fix port status of slave device

[Min Hu (Connor)]

Hi, Konstantin,
	fixed in v4, thanks.

在 2022/5/11 5:48, Konstantin Ananyev 写道:
> 
> Hi Ferruh,
> 
>> On 5/6/2022 9:16 AM, Min Hu (Connor) wrote:
>>> Hi, Konstantin,
>>>
>>> 在 2022/5/4 7:39, Konstantin Ananyev 写道:
>>>> 03/05/2022 11:02, Min Hu (Connor) пишет:
>>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>>
>>>>> 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")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>>> Acked-by: Aman Singh <aman.deep.singh@intel.com>
>>>>> ---
>>>>>   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(-)
>>>>>
>>>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>>>>> index 6ffea8e21a..d9fc7a88bd 100644
>>>>> --- a/app/test-pmd/cmdline.c
>>>>> +++ b/app/test-pmd/cmdline.c
>>>>> @@ -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;
>>>>>       }
>>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>>>>> index fe2ce19f99..dc90600787 100644
>>>>> --- a/app/test-pmd/testpmd.c
>>>>> +++ b/app/test-pmd/testpmd.c
>>>>> @@ -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)
>>>>>   {
>>>>> -    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.
>>>>
>>> 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.
>>>
>>
>> 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.
> 
>>
>> 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
>>
>>>
>>>>
>>>>> +        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;
>>>>>   }
>>>>> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
>>>>> index 31f766c965..67f253b30e 100644
>>>>> --- a/app/test-pmd/testpmd.h
>>>>> +++ b/app/test-pmd/testpmd.h
>>>>> @@ -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

[Ferruh Yigit]

On 5/11/2022 3:16 AM, Min Hu (Connor) wrote:

<...>
>>>>>> @@ -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())
>>>>>> -        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.
>>>>>
>>>> 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.
>>>>
>>>
>>> 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.
>>
 >
 > Hi, Konstantin,
 >      fixed in v4, thanks.

Hi Conor,

What do you think to apply same on patch 4/5?

Re: [PATCH v3 0/5] bugfix for bonding

[Ferruh Yigit]

On 5/3/2022 11:02 AM, Min Hu (Connor) wrote:
> 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
> 

For 1/5 & 2/5,
Applied to dpdk-next-net/main, thanks.

Re: [PATCH v4] app/testpmd: fix port status of slave device

[Konstantin Ananyev]

> From: Huisong Li <lihuisong@huawei.com>
> 
> 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")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> Acked-by: Aman Singh <aman.deep.singh@intel.com>
> ---
> 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(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 6ffea8e21a..d9fc7a88bd 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -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;
>   	}
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index fe2ce19f99..51fa344bb2 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -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;
>   }
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index 31f766c965..67f253b30e 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -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. */

Acked-by: Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>

Re: [PATCH v4] app/testpmd: fix port status of slave device

[Andrew Rybchenko]

On 5/12/22 01:08, Konstantin Ananyev wrote:
> 
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> 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")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> Acked-by: Aman Singh <aman.deep.singh@intel.com>
> 
> Acked-by: Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>

Applied, thanks.

Re: [PATCH v3 5/5] ethdev: fix dev state when stop

[Ferruh Yigit]

On 5/3/2022 11:02 AM, Min Hu (Connor) wrote:
> 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")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>   lib/ethdev/rte_ethdev.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 29a3d80466..e0011372aa 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -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.

Re: [PATCH v3 5/5] ethdev: fix dev state when stop

[Thomas Monjalon]

25/05/2022 19:44, Ferruh Yigit:
> On 5/3/2022 11:02 AM, Min Hu (Connor) wrote:
> > 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")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> > ---
> >   lib/ethdev/rte_ethdev.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> > index 29a3d80466..e0011372aa 100644
> > --- a/lib/ethdev/rte_ethdev.c
> > +++ b/lib/ethdev/rte_ethdev.c
> > @@ -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.

Acked-by: Thomas Monjalon <thomas@monjalon.net>

Re: [PATCH V2 4/4] app/testpmd: fix slave device isn't released

[Min Hu (Connor)]

Hi, all,
	any comments for this patch?

在 2022/3/24 11:00, Min Hu (Connor) 写道:
> 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 | 48 ++++++++++++++++++++++++++++++++++++++++++
>   app/test-pmd/testpmd.h |  2 ++
>   3 files changed, 51 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index f0efcf09f0..cd3873e1e0 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -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 =
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index dc90600787..22700e073d 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -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);
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index 67f253b30e..5af9455012 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -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

[Singh, Aman Deep]

Hi Connor,

On 5/30/2022 11:31 AM, Min Hu (Connor) wrote:
> Hi, all,
>     any comments for this patch?
>
> 在 2022/3/24 11:00, Min Hu (Connor) 写道:
>> 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 | 48 ++++++++++++++++++++++++++++++++++++++++++
>>   app/test-pmd/testpmd.h |  2 ++
>>   3 files changed, 51 insertions(+)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>> index f0efcf09f0..cd3873e1e0 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -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 =
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>> index dc90600787..22700e073d 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -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);
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"
>> +    }
>> +}
>> +#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
Can we merge these two #ifdef sections, like move _close() below.
>>           }
>>             free_xstats_display_info(pi);
>> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
>> index 67f253b30e..5af9455012 100644
>> --- a/app/test-pmd/testpmd.h
>> +++ b/app/test-pmd/testpmd.h
>> @@ -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 v3 5/5] ethdev: fix dev state when stop

[Ferruh Yigit]

On 5/26/2022 11:21 AM, Thomas Monjalon wrote:
> [CAUTION: External Email]
> 
> 25/05/2022 19:44, Ferruh Yigit:
>> On 5/3/2022 11:02 AM, Min Hu (Connor) wrote:
>>> 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")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>> ---
>>>    lib/ethdev/rte_ethdev.c | 3 ++-
>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>>> index 29a3d80466..e0011372aa 100644
>>> --- a/lib/ethdev/rte_ethdev.c
>>> +++ b/lib/ethdev/rte_ethdev.c
>>> @@ -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.
> 
> Acked-by: Thomas Monjalon <thomas@monjalon.net>
> 

Acked-by: Ferruh Yigit <ferruh.yigit@xilinx.com>

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

Re: [PATCH v3 4/5] app/testpmd: fix slave device isn't released

[Ferruh Yigit]

On 5/3/2022 11:02 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.
> 
> 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>

Hi Connor,

Can you please do the similar #ifdef grouping done in 3/5 patch?

[PATCH v4] app/testpmd: fix slave device isn't released

[Dongdong Liu]

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>
Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
---
 app/test-pmd/cmdline.c |  1 +
 app/test-pmd/testpmd.c | 46 ++++++++++++++++++++++++++++++++++++++++++
 app/test-pmd/testpmd.h |  2 ++
 3 files changed, 49 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index fdd0cada3b..6c58b02650 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -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 =
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index fc1b64b60d..2b16551a26 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -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;
+		}
+
+		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
+}
+
 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);
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 6693813dda..b44d5dd5ac 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -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.
-- 
2.33.0

Re: [PATCH v3 4/5] app/testpmd: fix slave device isn't released

[Dongdong Liu]

Hi Ferruh

"[PATCH v4] app/testpmd: fix slave device isn't released" has been sent 
out, help to review.

Thanks,
Dongdong
On 2022/6/2 1:54, Ferruh Yigit wrote:
> On 5/3/2022 11:02 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.
>>
>> 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>
>
> Hi Connor,
>
> Can you please do the similar #ifdef grouping done in 3/5 patch?
> .
>

Re: [PATCH v4] app/testpmd: fix slave device isn't released

[Ferruh Yigit]

On 6/7/2022 9:10 AM, Dongdong Liu 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.
> 
> 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>
> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
> ---
>   app/test-pmd/cmdline.c |  1 +
>   app/test-pmd/testpmd.c | 46 ++++++++++++++++++++++++++++++++++++++++++
>   app/test-pmd/testpmd.h |  2 ++
>   3 files changed, 49 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index fdd0cada3b..6c58b02650 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -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 =
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index fc1b64b60d..2b16551a26 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -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;
> +		}
> +
> +		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.

> +}
> +
>   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);
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index 6693813dda..b44d5dd5ac 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -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

[Dongdong Liu]

Hi Ferruh

Many thanks for your review.
On 2022/6/7 22:31, Ferruh Yigit wrote:
> On 6/7/2022 9:10 AM, Dongdong Liu 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.
>>
>> 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>
>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>> ---
>>   app/test-pmd/cmdline.c |  1 +
>>   app/test-pmd/testpmd.c | 46 ++++++++++++++++++++++++++++++++++++++++++
>>   app/test-pmd/testpmd.h |  2 ++
>>   3 files changed, 49 insertions(+)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>> index fdd0cada3b..6c58b02650 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -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 =
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>> index fc1b64b60d..2b16551a26 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -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;
>> +        }
>> +
>> +        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.

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
>
>> +}
>> +
>>   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);
>> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
>> index 6693813dda..b44d5dd5ac 100644
>> --- a/app/test-pmd/testpmd.h
>> +++ b/app/test-pmd/testpmd.h
>> @@ -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

[Ferruh Yigit]

On 6/9/2022 8:50 AM, Dongdong Liu wrote:
> Hi Ferruh
> 
> Many thanks for your review.
> On 2022/6/7 22:31, Ferruh Yigit wrote:
>> On 6/7/2022 9:10 AM, Dongdong Liu 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.
>>>
>>> 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>
>>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>>> ---
>>>   app/test-pmd/cmdline.c |  1 +
>>>   app/test-pmd/testpmd.c | 46 ++++++++++++++++++++++++++++++++++++++++++
>>>   app/test-pmd/testpmd.h |  2 ++
>>>   3 files changed, 49 insertions(+)
>>>
>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>>> index fdd0cada3b..6c58b02650 100644
>>> --- a/app/test-pmd/cmdline.c
>>> +++ b/app/test-pmd/cmdline.c
>>> @@ -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 =
>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>>> index fc1b64b60d..2b16551a26 100644
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -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;
>>> +        }
>>> +
>>> +        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.
> 
> 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.

> This patch does the similar #ifdef grouping done in 3/5 patch.
> 
> Thanks,
> Dongdong
>>
>>> +}
>>> +
>>>   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);
>>> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
>>> index 6693813dda..b44d5dd5ac 100644
>>> --- a/app/test-pmd/testpmd.h
>>> +++ b/app/test-pmd/testpmd.h
>>> @@ -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

[Dongdong Liu]

Hi Ferruh

Many thanks for your review.
On 2022/6/9 16:50, Ferruh Yigit wrote:
> On 6/9/2022 8:50 AM, Dongdong Liu wrote:
>> Hi Ferruh
>>
>> Many thanks for your review.
>> On 2022/6/7 22:31, Ferruh Yigit wrote:
>>> On 6/7/2022 9:10 AM, Dongdong Liu 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.
>>>>
>>>> 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>
>>>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>>>> ---
>>>>   app/test-pmd/cmdline.c |  1 +
>>>>   app/test-pmd/testpmd.c | 46
>>>> ++++++++++++++++++++++++++++++++++++++++++
>>>>   app/test-pmd/testpmd.h |  2 ++
>>>>   3 files changed, 49 insertions(+)
>>>>
>>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>>>> index fdd0cada3b..6c58b02650 100644
>>>> --- a/app/test-pmd/cmdline.c
>>>> +++ b/app/test-pmd/cmdline.c
>>>> @@ -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 =
>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>>>> index fc1b64b60d..2b16551a26 100644
>>>> --- a/app/test-pmd/testpmd.c
>>>> +++ b/app/test-pmd/testpmd.c
>>>> @@ -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;
>>>> +        }
>>>> +
>>>> +        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.
>>
>> 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
>
>> This patch does the similar #ifdef grouping done in 3/5 patch.
>>
>> Thanks,
>> Dongdong
>>>
>>>> +}
>>>> +
>>>>   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);
>>>> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
>>>> index 6693813dda..b44d5dd5ac 100644
>>>> --- a/app/test-pmd/testpmd.h
>>>> +++ b/app/test-pmd/testpmd.h
>>>> @@ -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] app/testpmd: fix slave device isn't released

[Dongdong Liu]

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>
Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
---
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(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index fdd0cada3b..6c58b02650 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -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 =
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index fc1b64b60d..8bcb488d33 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -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,39 @@ remove_invalid_ports(void)
 	nb_cfg_ports = nb_fwd_ports;
 }
 
+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);
+	}
+}
+
 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 +3269,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);
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 6693813dda..b44d5dd5ac 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -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.
-- 
2.33.0

Re: [PATCH v5] app/testpmd: fix slave device isn't released

[Ferruh Yigit]

On 6/9/2022 12:49 PM, Dongdong Liu 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.
> 
> 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>
> Signed-off-by: Dongdong Liu<liudongdong3@huawei.com>

Acked-by: Ferruh Yigit <ferruh.yigit@xilinx.com>

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