Re: [dpdk-dev] [PATCH 1/3] app/testpmd: fix forwarding configuration when DCB test

[oulijun]

在 2021/4/10 9:12, Ferruh Yigit 写道:
On 3/23/2021 2:17 PM, oulijun wrote:


在 2021/3/23 16:55, Li, Xiaoyun 写道:
Hi

-----Original Message-----
From: Lijun Ou <ouli...@huawei.com>
Sent: Friday, March 5, 2021 18:22
To: Yigit, Ferruh <ferruh.yi...@intel.com>
Cc: Li, Xiaoyun <xiaoyun...@intel.com>; dev@dpdk.org;
linux...@openeuler.org
Subject: [PATCH 1/3] app/testpmd: fix forwarding configuration when 
DCB test

From: Huisong Li <lihuis...@huawei.com>

The commit message is too long and redundant. Please simplify it.

Currently, 'nb_fwd_lcores' value are both adjusted based on 
'nb_fwd_streams' in
rss/simple/icmp_echo_fwd_config_setup. But the operation is lack for
dcb_fwd_config_setup, which may lead to a bad events where multiple 
polling
threads operate on the same queue. As a result, various possible 
exceptions will
occur. The procedure is as follows:

startup testpmd app as follows:
testpmd> port stop all
testpmd> set nbcore 8
testpmd> port config 0 dcb vt off 4 pfc on port config all rxq 16 port
testpmd> config all txq 16 port start all set fwd mac
Logical Core 1 (socket 0) forwards packets on 4 streams:
RX P=0/Q=0 (socket 0) -> TX P=0/Q=0 (socket 0) 
peer=02:00:00:00:00:00 RX
P=0/Q=1 (socket 0) -> TX P=0/Q=1 (socket 0) peer=02:00:00:00:00:00 RX
P=0/Q=2 (socket 0) -> TX P=0/Q=2 (socket 0) peer=02:00:00:00:00:00 RX
P=0/Q=3 (socket 0) -> TX P=0/Q=3 (socket 0) peer=02:00:00:00:00:00 
Logical
Core 2 (socket 0) forwards packets on 4 streams:
RX P=0/Q=4 (socket 0) -> TX P=0/Q=4 (socket 0) 
peer=02:00:00:00:00:00 RX
P=0/Q=5 (socket 0) -> TX P=0/Q=5 (socket 0) peer=02:00:00:00:00:00 RX
P=0/Q=6 (socket 0) -> TX P=0/Q=6 (socket 0) peer=02:00:00:00:00:00 RX
P=0/Q=7 (socket 0) -> TX P=0/Q=7 (socket 0) peer=02:00:00:00:00:00 
Logical
Core 3 (socket 0) forwards packets on 4 streams:
RX P=0/Q=8 (socket 0) -> TX P=0/Q=8 (socket 0) 
peer=02:00:00:00:00:00 RX
P=0/Q=9 (socket 0) -> TX P=0/Q=9 (socket 0) peer=02:00:00:00:00:00 RX
P=0/Q=10 (socket 0) -> TX P=0/Q=10 (socket 0) peer=02:00:00:00:00:00 RX
P=0/Q=11 (socket 0) -> TX P=0/Q=11 (socket 0) peer=02:00:00:00:00:00 
Logical
Core 4 (socket 0) forwards packets on 4 streams:
RX P=0/Q=12 (socket 0) -> TX P=0/Q=12 (socket 0) 
peer=02:00:00:00:00:00 RX
P=0/Q=13 (socket 0) -> TX P=0/Q=13 (socket 0) peer=02:00:00:00:00:00 RX
P=0/Q=14 (socket 0) -> TX P=0/Q=14 (socket 0) peer=02:00:00:00:00:00 RX
P=0/Q=15 (socket 0) -> TX P=0/Q=15 (socket 0) peer=02:00:00:00:00:00 
Logical
Core 5 (socket 0) forwards packets on 1 streams:
RX P=0/Q=4 (socket 0) -> TX P=0/Q=4 (socket 0) 
peer=02:00:00:00:00:00 Logical
Core 6 (socket 0) forwards packets on 1 streams:
RX P=0/Q=5 (socket 0) -> TX P=0/Q=5 (socket 0) 
peer=02:00:00:00:00:00 Logical
Core 7 (socket 0) forwards packets on 1 streams:
RX P=0/Q=6 (socket 0) -> TX P=0/Q=6 (socket 0) 
peer=02:00:00:00:00:00 Logical
Core 8 (socket 0) forwards packets on 1 streams:
RX P=0/Q=7 (socket 0) -> TX P=0/Q=7 (socket 0) peer=02:00:00:00:00:00

Notice: number of rxq and txq in running cmdline are greater than 
used nb_tc.

For the DCB forwarding test, each core is assigned on each traffic 
class and each
core is assigned a multi-stream. Therefore, 'nb_fwd_lcores'
value can be adjusted based on 'total_tc_num' in all forwarding ports.

In addition, after operation of port stop/port start, forwarding 
configuration is
changed by rss_fwd_config_setup. In "start_port"
function, dcb_test is set to 1 based on dcb_config. So it also 
should be cleared
when dcb_config is 0.

Fixes: 900550de04a7 ("app/testpmd: add dcb support")
Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
Cc: sta...@dpdk.org

Signed-off-by: Huisong Li <lihuis...@huawei.com>
Signed-off-by: Lijun Ou <ouli...@huawei.com>
---
  app/test-pmd/config.c  | 19 +++++++++++++++++++  
app/test-pmd/testpmd.c |
12 +++++++-----
  2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
576d5ac..c89f8cd 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2864,6 +2864,21 @@ rss_fwd_config_setup(void)
      }
  }

+static uint16_t
+get_fwd_port_total_tc_num(void)
+{
+    struct rte_eth_dcb_info dcb_info;
+    uint16_t total_tc_num = 0;
+    unsigned int i;
+
+    for (i = 0; i < nb_fwd_ports; i++) {
+        (void)rte_eth_dev_get_dcb_info(fwd_ports_ids[i], &dcb_info);
+        total_tc_num += dcb_info.nb_tcs;
+    }
+
+    return total_tc_num;
+}

It's only 3 lines. Is it really necessary to make it into a function 
which is not called by anyone else? A comment and the loop are enough.

To calculate the total number of TCs on all forwarding ports, we have 
to call rte_eth_dev_get_dcb_info().  However,  rte_eth_dev_get_dcb_info()
has been called twice by dcb_fwd_config_setup() to setup dcb 
forwarding configuration. The dcb_fwd_config_setup() might be more 
readable
if encapsulated, I think.  In addition, variables in "cur_fwd_config" 
are initialized more centrally. What do you think?

+1 to have the function, agree it makes more readable.

But for the PMDs that doesn't support '.get_dcb_info' it will return 0, 
causing 'nb_fwd_lcores' to be zero, I think this should be prevented.
Either can add a zero check here, or do the check during "port config # 
dcb ..." and prevent setting 'dcb_config' at first place for those pmds. 
I think second option is better, what do you think.

Yes, the second one is the source.
Overall DCB support in the testpmd seems missing some corner cases and 
requires more update/fix.

In testpmd, it seems that the code has not been updated for a long time. 
I don't know how other dcb-enabled drivers are tested and how they 
should be used.
Do they not apply this method to test the DCB?
+
  /**
   * For the DCB forwarding test, each core is assigned on each 
traffic class.
   *
@@ -2883,12 +2898,16 @@ dcb_fwd_config_setup(void)
      lcoreid_t  lc_id;
      uint16_t nb_rx_queue, nb_tx_queue;
      uint16_t i, j, k, sm_id = 0;
+    uint16_t total_tc_num = 0;
      uint8_t tc = 0;

      cur_fwd_config.nb_fwd_lcores = (lcoreid_t) nb_fwd_lcores;
      cur_fwd_config.nb_fwd_ports = nb_fwd_ports;
      cur_fwd_config.nb_fwd_streams =
          (streamid_t) (nb_rxq * cur_fwd_config.nb_fwd_ports);
+    total_tc_num = get_fwd_port_total_tc_num();
+    if (cur_fwd_config.nb_fwd_lcores > total_tc_num)
+        cur_fwd_config.nb_fwd_lcores = total_tc_num;

      /* reinitialize forwarding streams */
      init_fwd_streams();
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
1a57324..7eb810b 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2707,14 +2707,16 @@ stop_port(portid_t pid)
      portid_t peer_pl[RTE_MAX_ETHPORTS];
      int peer_pi;

-    if (dcb_test) {
-        dcb_test = 0;
-        dcb_config = 0;
-    }
-
      if (port_id_is_invalid(pid, ENABLED_WARN))
          return;

+    /*
+     * In "start_port" function, dcb_test is set to 1 based on 
dcb_config.
+     * So it should be cleared when dcb_config is 0.
+     */
+    if (dcb_config == 0)
+        dcb_test = 0;
+

I don't understand why are you changing this.
dcb_test will only be set when dcb_config is 1 when starting ports. 
And both dcb_test and dcb_config will be cleared when stopping ports.
So dcb will only affect when you set port dcb and then start port and 
when stop port dcb will be cleared.

So what's the problem of original codes?

Your change will cause issues that there's no place to set dcb_config 
as 0. If you config dcb, then it'll be always dcb mode unless restart 
the whole testpmd.

      printf("Stopping ports...\n");

      RTE_ETH_FOREACH_DEV(pi) {
--
2.7.4

.


.