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 9ECCFA0547; Sat, 10 Apr 2021 03:12:31 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 86E8514113A; Sat, 10 Apr 2021 03:12:31 +0200 (CEST) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by mails.dpdk.org (Postfix) with ESMTP id 1C077141136 for ; Sat, 10 Apr 2021 03:12:29 +0200 (CEST) IronPort-SDR: HIVzMgOS8DtnHTMC6wWzEEIyZOdVnfiePDh3ZyYYoEtwzsP+eftYiGhjVHqr9bDTMhLcQovU+D /MSr23lzKQAA== X-IronPort-AV: E=McAfee;i="6000,8403,9949"; a="180999238" X-IronPort-AV: E=Sophos;i="5.82,210,1613462400"; d="scan'208";a="180999238" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Apr 2021 18:12:29 -0700 IronPort-SDR: qC065uocm4CXgWj0sQgun1VIgIjbZQdkcXyUIynaKLkFjN1fD/Lun9408JuyYni4yjxH9RGkqp FWe05wivkw6g== X-IronPort-AV: E=Sophos;i="5.82,210,1613462400"; d="scan'208";a="422960774" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.215.191]) ([10.213.215.191]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Apr 2021 18:12:27 -0700 To: oulijun , "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> From: Ferruh Yigit X-User: ferruhy Message-ID: <7538897f-9251-3c82-adbd-d3d995ed3c95@intel.com> Date: Sat, 10 Apr 2021 02:12:23 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit 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" 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. 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 >> >> . >>