From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id B2311A0A02; Tue, 23 Mar 2021 15:18:57 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9CECA4069E; Tue, 23 Mar 2021 15:18:57 +0100 (CET) Received: from szxga05-in.huawei.com (szxga05-in.huawei.com [45.249.212.191]) by mails.dpdk.org (Postfix) with ESMTP id 4CA3E40689 for ; Tue, 23 Mar 2021 15:18:56 +0100 (CET) Received: from DGGEMS411-HUB.china.huawei.com (unknown [172.30.72.59]) by szxga05-in.huawei.com (SkyGuard) with ESMTP id 4F4YMD2SyRzPkCl; Tue, 23 Mar 2021 22:16:24 +0800 (CST) Received: from [10.78.49.194] (10.78.49.194) by DGGEMS411-HUB.china.huawei.com (10.3.19.211) with Microsoft SMTP Server id 14.3.498.0; Tue, 23 Mar 2021 22:18:53 +0800 To: "Li, Xiaoyun" , "Yigit, Ferruh" CC: "dev@dpdk.org" , "linuxarm@openeuler.org" References: <1614939741-63927-1-git-send-email-oulijun@huawei.com> <1614939741-63927-2-git-send-email-oulijun@huawei.com> From: oulijun Message-ID: <2a0bee90-2c74-5f07-aaf0-cba8b94944e8@huawei.com> Date: Tue, 23 Mar 2021 22:18:53 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="gbk"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.78.49.194] X-CFilter-Loop: Reflected Subject: Re: [dpdk-dev] [PATCH 1/3] app/testpmd: fix forwarding configuration when DCB test X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" ÔÚ 2021/3/23 16:55, Li, Xiaoyun дµÀ: > Hi > >> -----Original Message----- >> From: Lijun Ou >> Sent: Friday, March 5, 2021 18:22 >> To: Yigit, Ferruh >> Cc: Li, Xiaoyun ; dev@dpdk.org; >> linuxarm@openeuler.org >> Subject: [PATCH 1/3] app/testpmd: fix forwarding configuration when DCB test >> >> From: Huisong Li >> > 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 >> Signed-off-by: Lijun Ou >> --- >> 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. > >> + >> /** >> * 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. > Yes, I think. The forwarding streams should not be changed from "dcb_fwd_config_setup" to "rss_fwd_config_setup" after dcb info is configured. But, now, the logical codes do it when stopping ports and then starting ports. > 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. > As far as I know, the current testpmd only supports switching from the normal mode to the dcb mode, but does not support the reverse operation. And " dcb_config" is set to 1, and then "dcb_test" is set to 1 after config. >> printf("Stopping ports...\n"); >> >> RTE_ETH_FOREACH_DEV(pi) { >> -- >> 2.7.4 > > . >