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

Sun, 08 May 2022 04:28:19 -0700 


Hi Conor,


在 2022/5/4 7:39, Konstantin Ananyev 写道:

03/05/2022 11:02, Min Hu (Connor) пишет:

From: Huisong Li <lihuis...@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: sta...@dpdk.org

Signed-off-by: Huisong Li <lihuis...@huawei.com>
Signed-off-by: Min Hu (Connor) <humi...@huawei.com>
Acked-by: Aman Singh <aman.deep.si...@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. */


.
























					Reply via email to