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 59329A0547; Sat, 10 Apr 2021 04:53:57 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id DCC70141162; Sat, 10 Apr 2021 04:53:56 +0200 (CEST) Received: from szxga05-in.huawei.com (szxga05-in.huawei.com [45.249.212.191]) by mails.dpdk.org (Postfix) with ESMTP id C5A1614115F for ; Sat, 10 Apr 2021 04:53:55 +0200 (CEST) Received: from DGGEMS411-HUB.china.huawei.com (unknown [172.30.72.60]) by szxga05-in.huawei.com (SkyGuard) with ESMTP id 4FHKJ72GKjzPnsn; Sat, 10 Apr 2021 10:51:03 +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; Sat, 10 Apr 2021 10:53:47 +0800 To: Ferruh Yigit , "Li, Xiaoyun" 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> <7538897f-9251-3c82-adbd-d3d995ed3c95@intel.com> From: oulijun Message-ID: Date: Sat, 10 Apr 2021 10:53:47 +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: <7538897f-9251-3c82-adbd-d3d995ed3c95@intel.com> Content-Type: text/plain; charset="utf-8"; 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/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 >>>> 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. >>> >> 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 >>> >>> . >>> > > . >