On 3/23/2021 2:17 PM, oulijun wrote:
在 2021/3/23 16:55, Li, Xiaoyun 写道:
Hi
-----Original Message----- From: Lijun Ou oulijun@huawei.com Sent: Friday, March 5, 2021 18:22 To: Yigit, Ferruh ferruh.yigit@intel.com Cc: Li, Xiaoyun xiaoyun.li@intel.com; dev@dpdk.org; linuxarm@openeuler.org Subject: [PATCH 1/3] app/testpmd: fix forwarding configuration when DCB test
From: Huisong Li lihuisong@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: stable@dpdk.org
Signed-off-by: Huisong Li lihuisong@huawei.com Signed-off-by: Lijun Ou oulijun@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
.