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.
Overall DCB support in the testpmd seems missing some corner cases and requires
more update/fix.
+
/**
* 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
.
