* [PATCH v3 1/5] net/bonding: fix non-active slaves aren't stopped
[not found] ` <20220503100217.46203-1-humin29@huawei.com>
@ 2022-05-03 10:02 ` Min Hu (Connor)
2022-05-03 10:02 ` [PATCH v3 2/5] net/bonding: fix non-terminable while loop Min Hu (Connor)
` (3 subsequent siblings)
4 siblings, 0 replies; 42+ messages in thread
From: Min Hu (Connor) @ 2022-05-03 10:02 UTC (permalink / raw)
To: dev; +Cc: Huisong Li, stable, Min Hu, Chas Williams, Radu Nicolau
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
^ permalink raw reply [flat|nested] 42+ messages in thread* [PATCH v3 2/5] net/bonding: fix non-terminable while loop
[not found] ` <20220503100217.46203-1-humin29@huawei.com>
2022-05-03 10:02 ` [PATCH v3 1/5] net/bonding: fix non-active slaves aren't stopped Min Hu (Connor)
@ 2022-05-03 10:02 ` Min Hu (Connor)
2022-05-03 10:02 ` [PATCH v3 3/5] app/testpmd: fix port status of slave device Min Hu (Connor)
` (2 subsequent siblings)
4 siblings, 0 replies; 42+ messages in thread
From: Min Hu (Connor) @ 2022-05-03 10:02 UTC (permalink / raw)
To: dev
Cc: Huisong Li, stable, Min Hu, Chas Williams, Andrew Rybchenko,
Ivan Ilchenko
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
^ permalink raw reply [flat|nested] 42+ messages in thread* [PATCH v3 3/5] app/testpmd: fix port status of slave device
[not found] ` <20220503100217.46203-1-humin29@huawei.com>
2022-05-03 10:02 ` [PATCH v3 1/5] net/bonding: fix non-active slaves aren't stopped Min Hu (Connor)
2022-05-03 10:02 ` [PATCH v3 2/5] net/bonding: fix non-terminable while loop Min Hu (Connor)
@ 2022-05-03 10:02 ` Min Hu (Connor)
2022-05-03 23:39 ` Konstantin Ananyev
2022-05-11 2:14 ` [PATCH v4] " Min Hu (Connor)
2022-05-03 10:02 ` [PATCH v3 4/5] app/testpmd: fix slave device isn't released Min Hu (Connor)
2022-05-03 10:02 ` [PATCH v3 5/5] ethdev: fix dev state when stop Min Hu (Connor)
4 siblings, 2 replies; 42+ messages in thread
From: Min Hu (Connor) @ 2022-05-03 10:02 UTC (permalink / raw)
To: dev
Cc: Huisong Li, stable, Min Hu, Aman Singh, Xiaoyun Li, Yuying Zhang,
Pablo de Lara, Bernard Iremonger
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
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v3 3/5] app/testpmd: fix port status of slave device
2022-05-03 10:02 ` [PATCH v3 3/5] app/testpmd: fix port status of slave device Min Hu (Connor)
@ 2022-05-03 23:39 ` Konstantin Ananyev
2022-05-06 8:16 ` Min Hu (Connor)
2022-05-11 2:14 ` [PATCH v4] " Min Hu (Connor)
1 sibling, 1 reply; 42+ messages in thread
From: Konstantin Ananyev @ 2022-05-03 23:39 UTC (permalink / raw)
To: Min Hu (Connor), dev
Cc: Huisong Li, stable, Aman Singh, Xiaoyun Li, Yuying Zhang,
Pablo de Lara, Bernard Iremonger
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. */
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v3 3/5] app/testpmd: fix port status of slave device
2022-05-03 23:39 ` Konstantin Ananyev
@ 2022-05-06 8:16 ` Min Hu (Connor)
2022-05-08 11:28 ` Konstantin Ananyev
2022-05-10 16:34 ` Ferruh Yigit
0 siblings, 2 replies; 42+ messages in thread
From: Min Hu (Connor) @ 2022-05-06 8:16 UTC (permalink / raw)
To: Konstantin Ananyev, dev
Cc: Huisong Li, stable, Aman Singh, Xiaoyun Li, Yuying Zhang,
Pablo de Lara, Bernard Iremonger
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. */
>
> .
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v3 3/5] app/testpmd: fix port status of slave device
2022-05-06 8:16 ` Min Hu (Connor)
@ 2022-05-08 11:28 ` Konstantin Ananyev
2022-05-10 16:34 ` Ferruh Yigit
1 sibling, 0 replies; 42+ messages in thread
From: Konstantin Ananyev @ 2022-05-08 11:28 UTC (permalink / raw)
To: Min Hu (Connor), dev
Cc: Huisong Li, stable, Aman Singh, Xiaoyun Li, Yuying Zhang,
Pablo de Lara, Bernard Iremonger
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. */
>>
>> .
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v3 3/5] app/testpmd: fix port status of slave device
2022-05-06 8:16 ` Min Hu (Connor)
2022-05-08 11:28 ` Konstantin Ananyev
@ 2022-05-10 16:34 ` Ferruh Yigit
2022-05-10 21:48 ` Konstantin Ananyev
1 sibling, 1 reply; 42+ messages in thread
From: Ferruh Yigit @ 2022-05-10 16:34 UTC (permalink / raw)
To: Min Hu (Connor), Konstantin Ananyev, dev
Cc: Huisong Li, stable, Aman Singh, Xiaoyun Li, Yuying Zhang,
Pablo de Lara, Bernard Iremonger
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. */
>>
>> .
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v3 3/5] app/testpmd: fix port status of slave device
2022-05-10 16:34 ` Ferruh Yigit
@ 2022-05-10 21:48 ` Konstantin Ananyev
2022-05-11 2:16 ` Min Hu (Connor)
0 siblings, 1 reply; 42+ messages in thread
From: Konstantin Ananyev @ 2022-05-10 21:48 UTC (permalink / raw)
To: Ferruh Yigit, Min Hu (Connor), dev
Cc: Huisong Li, stable, Aman Singh, Xiaoyun Li, Yuying Zhang,
Pablo de Lara, Bernard Iremonger
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. */
>>>
>>> .
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v3 3/5] app/testpmd: fix port status of slave device
2022-05-10 21:48 ` Konstantin Ananyev
@ 2022-05-11 2:16 ` Min Hu (Connor)
2022-05-11 10:05 ` Ferruh Yigit
0 siblings, 1 reply; 42+ messages in thread
From: Min Hu (Connor) @ 2022-05-11 2:16 UTC (permalink / raw)
To: Konstantin Ananyev, Ferruh Yigit, dev
Cc: Huisong Li, stable, Aman Singh, Xiaoyun Li, Yuying Zhang,
Pablo de Lara, Bernard Iremonger
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. */
>>>>
>>>> .
>>
>
> .
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v3 3/5] app/testpmd: fix port status of slave device
2022-05-11 2:16 ` Min Hu (Connor)
@ 2022-05-11 10:05 ` Ferruh Yigit
0 siblings, 0 replies; 42+ messages in thread
From: Ferruh Yigit @ 2022-05-11 10:05 UTC (permalink / raw)
To: Min Hu (Connor), Konstantin Ananyev, dev
Cc: Huisong Li, stable, Aman Singh, Xiaoyun Li, Yuying Zhang,
Pablo de Lara, Bernard Iremonger
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?
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v4] app/testpmd: fix port status of slave device
2022-05-03 10:02 ` [PATCH v3 3/5] app/testpmd: fix port status of slave device Min Hu (Connor)
2022-05-03 23:39 ` Konstantin Ananyev
@ 2022-05-11 2:14 ` Min Hu (Connor)
2022-05-11 22:08 ` Konstantin Ananyev
1 sibling, 1 reply; 42+ messages in thread
From: Min Hu (Connor) @ 2022-05-11 2:14 UTC (permalink / raw)
To: dev
Cc: Huisong Li, stable, Min Hu, Aman Singh, Xiaoyun Li, Yuying Zhang,
Pablo de Lara, Bernard Iremonger
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
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v4] app/testpmd: fix port status of slave device
2022-05-11 2:14 ` [PATCH v4] " Min Hu (Connor)
@ 2022-05-11 22:08 ` Konstantin Ananyev
2022-05-19 7:15 ` Andrew Rybchenko
0 siblings, 1 reply; 42+ messages in thread
From: Konstantin Ananyev @ 2022-05-11 22:08 UTC (permalink / raw)
To: Min Hu (Connor), dev
Cc: Huisong Li, stable, Aman Singh, Xiaoyun Li, Yuying Zhang,
Pablo de Lara, Bernard Iremonger
> 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>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v4] app/testpmd: fix port status of slave device
2022-05-11 22:08 ` Konstantin Ananyev
@ 2022-05-19 7:15 ` Andrew Rybchenko
0 siblings, 0 replies; 42+ messages in thread
From: Andrew Rybchenko @ 2022-05-19 7:15 UTC (permalink / raw)
To: Konstantin Ananyev, Min Hu (Connor), dev
Cc: Huisong Li, stable, Aman Singh, Xiaoyun Li, Yuying Zhang,
Pablo de Lara, Bernard Iremonger
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.
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v3 4/5] app/testpmd: fix slave device isn't released
[not found] ` <20220503100217.46203-1-humin29@huawei.com>
` (2 preceding siblings ...)
2022-05-03 10:02 ` [PATCH v3 3/5] app/testpmd: fix port status of slave device Min Hu (Connor)
@ 2022-05-03 10:02 ` Min Hu (Connor)
2022-06-01 17:54 ` Ferruh Yigit
` (2 more replies)
2022-05-03 10:02 ` [PATCH v3 5/5] ethdev: fix dev state when stop Min Hu (Connor)
4 siblings, 3 replies; 42+ messages in thread
From: Min Hu (Connor) @ 2022-05-03 10:02 UTC (permalink / raw)
To: dev
Cc: Huisong Li, stable, Min Hu, Xiaoyun Li, Aman Singh, Yuying Zhang,
Bernard Iremonger, Pablo de Lara
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
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v3 4/5] app/testpmd: fix slave device isn't released
2022-05-03 10:02 ` [PATCH v3 4/5] app/testpmd: fix slave device isn't released Min Hu (Connor)
@ 2022-06-01 17:54 ` Ferruh Yigit
2022-06-07 8:15 ` Dongdong Liu
2022-06-07 8:10 ` [PATCH v4] " Dongdong Liu
2022-06-09 11:49 ` [PATCH v5] " Dongdong Liu
2 siblings, 1 reply; 42+ messages in thread
From: Ferruh Yigit @ 2022-06-01 17:54 UTC (permalink / raw)
To: Min Hu (Connor), dev
Cc: Huisong Li, stable, Xiaoyun Li, Aman Singh, Yuying Zhang,
Bernard Iremonger, Pablo de Lara, Andrew Rybchenko,
Konstantin Ananyev
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?
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v3 4/5] app/testpmd: fix slave device isn't released
2022-06-01 17:54 ` Ferruh Yigit
@ 2022-06-07 8:15 ` Dongdong Liu
0 siblings, 0 replies; 42+ messages in thread
From: Dongdong Liu @ 2022-06-07 8:15 UTC (permalink / raw)
To: Ferruh Yigit, Min Hu (Connor), dev
Cc: Huisong Li, stable, Xiaoyun Li, Aman Singh, Yuying Zhang,
Bernard Iremonger, Pablo de Lara, Andrew Rybchenko,
Konstantin Ananyev
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?
> .
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v4] app/testpmd: fix slave device isn't released
2022-05-03 10:02 ` [PATCH v3 4/5] app/testpmd: fix slave device isn't released Min Hu (Connor)
2022-06-01 17:54 ` Ferruh Yigit
@ 2022-06-07 8:10 ` Dongdong Liu
2022-06-07 14:31 ` Ferruh Yigit
2022-06-09 11:49 ` [PATCH v5] " Dongdong Liu
2 siblings, 1 reply; 42+ messages in thread
From: Dongdong Liu @ 2022-06-07 8:10 UTC (permalink / raw)
To: dev
Cc: stable, xiaoyun.li, aman.deep.singh, yuying.zhang,
bernard.iremonger, pablo.de.lara.guarch, ferruh.yigit,
Huisong Li, Min Hu, 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
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v4] app/testpmd: fix slave device isn't released
2022-06-07 8:10 ` [PATCH v4] " Dongdong Liu
@ 2022-06-07 14:31 ` Ferruh Yigit
2022-06-09 7:50 ` Dongdong Liu
0 siblings, 1 reply; 42+ messages in thread
From: Ferruh Yigit @ 2022-06-07 14:31 UTC (permalink / raw)
To: Dongdong Liu, dev
Cc: stable, xiaoyun.li, aman.deep.singh, yuying.zhang,
bernard.iremonger, pablo.de.lara.guarch, Huisong Li, Min Hu
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.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v4] app/testpmd: fix slave device isn't released
2022-06-07 14:31 ` Ferruh Yigit
@ 2022-06-09 7:50 ` Dongdong Liu
2022-06-09 8:50 ` Ferruh Yigit
0 siblings, 1 reply; 42+ messages in thread
From: Dongdong Liu @ 2022-06-09 7:50 UTC (permalink / raw)
To: Ferruh Yigit, dev
Cc: stable, xiaoyun.li, aman.deep.singh, yuying.zhang,
bernard.iremonger, pablo.de.lara.guarch, Huisong Li, Min Hu
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.
>
> .
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v4] app/testpmd: fix slave device isn't released
2022-06-09 7:50 ` Dongdong Liu
@ 2022-06-09 8:50 ` Ferruh Yigit
2022-06-09 11:20 ` Dongdong Liu
0 siblings, 1 reply; 42+ messages in thread
From: Ferruh Yigit @ 2022-06-09 8:50 UTC (permalink / raw)
To: Dongdong Liu, dev
Cc: stable, xiaoyun.li, aman.deep.singh, yuying.zhang,
bernard.iremonger, pablo.de.lara.guarch, Huisong Li, Min Hu
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.
>>
>> .
>>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v4] app/testpmd: fix slave device isn't released
2022-06-09 8:50 ` Ferruh Yigit
@ 2022-06-09 11:20 ` Dongdong Liu
0 siblings, 0 replies; 42+ messages in thread
From: Dongdong Liu @ 2022-06-09 11:20 UTC (permalink / raw)
To: Ferruh Yigit, dev
Cc: stable, xiaoyun.li, aman.deep.singh, yuying.zhang,
bernard.iremonger, pablo.de.lara.guarch, Huisong Li, Min Hu
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.
>>>
>>> .
>>>
>
> .
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v5] app/testpmd: fix slave device isn't released
2022-05-03 10:02 ` [PATCH v3 4/5] app/testpmd: fix slave device isn't released Min Hu (Connor)
2022-06-01 17:54 ` Ferruh Yigit
2022-06-07 8:10 ` [PATCH v4] " Dongdong Liu
@ 2022-06-09 11:49 ` Dongdong Liu
2022-06-10 8:10 ` Ferruh Yigit
2 siblings, 1 reply; 42+ messages in thread
From: Dongdong Liu @ 2022-06-09 11:49 UTC (permalink / raw)
To: dev
Cc: stable, xiaoyun.li, aman.deep.singh, yuying.zhang,
bernard.iremonger, pablo.de.lara.guarch, ferruh.yigit,
Huisong Li, Min Hu, 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
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v5] app/testpmd: fix slave device isn't released
2022-06-09 11:49 ` [PATCH v5] " Dongdong Liu
@ 2022-06-10 8:10 ` Ferruh Yigit
0 siblings, 0 replies; 42+ messages in thread
From: Ferruh Yigit @ 2022-06-10 8:10 UTC (permalink / raw)
To: Dongdong Liu, dev
Cc: stable, xiaoyun.li, aman.deep.singh, yuying.zhang,
bernard.iremonger, pablo.de.lara.guarch, Huisong Li, Min Hu
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.
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v3 5/5] ethdev: fix dev state when stop
[not found] ` <20220503100217.46203-1-humin29@huawei.com>
` (3 preceding siblings ...)
2022-05-03 10:02 ` [PATCH v3 4/5] app/testpmd: fix slave device isn't released Min Hu (Connor)
@ 2022-05-03 10:02 ` Min Hu (Connor)
2022-05-25 17:44 ` Ferruh Yigit
4 siblings, 1 reply; 42+ messages in thread
From: Min Hu (Connor) @ 2022-05-03 10:02 UTC (permalink / raw)
To: dev
Cc: Min Hu (Connor),
stable, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko,
Ivan Ilchenko
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
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v3 5/5] ethdev: fix dev state when stop
2022-05-03 10:02 ` [PATCH v3 5/5] ethdev: fix dev state when stop Min Hu (Connor)
@ 2022-05-25 17:44 ` Ferruh Yigit
2022-05-26 10:21 ` Thomas Monjalon
0 siblings, 1 reply; 42+ messages in thread
From: Ferruh Yigit @ 2022-05-25 17:44 UTC (permalink / raw)
To: Min Hu (Connor), Thomas Monjalon, Andrew Rybchenko
Cc: stable, Ivan Ilchenko, dev
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.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v3 5/5] ethdev: fix dev state when stop
2022-05-25 17:44 ` Ferruh Yigit
@ 2022-05-26 10:21 ` Thomas Monjalon
2022-05-30 12:04 ` Ferruh Yigit
0 siblings, 1 reply; 42+ messages in thread
From: Thomas Monjalon @ 2022-05-26 10:21 UTC (permalink / raw)
To: Min Hu (Connor), Andrew Rybchenko, Ferruh Yigit
Cc: stable, Ivan Ilchenko, dev
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>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v3 5/5] ethdev: fix dev state when stop
2022-05-26 10:21 ` Thomas Monjalon
@ 2022-05-30 12:04 ` Ferruh Yigit
0 siblings, 0 replies; 42+ messages in thread
From: Ferruh Yigit @ 2022-05-30 12:04 UTC (permalink / raw)
To: Thomas Monjalon, Min Hu (Connor), Andrew Rybchenko
Cc: stable, Ivan Ilchenko, dev
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.
^ permalink raw reply [flat|nested] 42+ messages in thread