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 0036DA0A02; Sat, 17 Apr 2021 08:12:58 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E00FB161E5E; Sat, 17 Apr 2021 08:12:58 +0200 (CEST) Received: from szxga04-in.huawei.com (szxga04-in.huawei.com [45.249.212.190]) by mails.dpdk.org (Postfix) with ESMTP id 9E29140143 for ; Sat, 17 Apr 2021 08:12:56 +0200 (CEST) Received: from DGGEMS409-HUB.china.huawei.com (unknown [172.30.72.58]) by szxga04-in.huawei.com (SkyGuard) with ESMTP id 4FMjNQ40r4zpYmm; Sat, 17 Apr 2021 14:09:58 +0800 (CST) Received: from [10.67.103.128] (10.67.103.128) by DGGEMS409-HUB.china.huawei.com (10.3.19.209) with Microsoft SMTP Server id 14.3.498.0; Sat, 17 Apr 2021 14:12:52 +0800 To: Ferruh Yigit , , Xiaoyun Li CC: Anatoly Burakov , Andrew Rybchenko , Thomas Monjalon References: <1614906276-34293-1-git-send-email-oulijun@huawei.com> <1618537978-35479-1-git-send-email-humin29@huawei.com> From: "Min Hu (Connor)" Message-ID: <3372d919-a756-0f16-f7c3-afc699d44e94@huawei.com> Date: Sat, 17 Apr 2021 14:12:52 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.3.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.128] X-CFilter-Loop: Reflected Subject: Re: [dpdk-dev] [PATCH v9] app/testpmd: support multi-process 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" Hi, Ferrruh, All is fixed in v10, please check it out, thanks. 在 2021/4/17 0:30, Ferruh Yigit 写道: > On 4/16/2021 2:52 AM, Min Hu (Connor) wrote: >> This patch adds multi-process support for testpmd. >> The test cmd example as follows: >> the primary cmd: >> ./dpdk-testpmd -a xxx --proc-type=auto -l 0-1 -- -i \ >> --rxq=4 --txq=4 --num-procs=2 --proc-id=0 >> >> the secondary cmd: >> ./dpdk-testpmd -a xxx --proc-type=auto -l 2-3 -- -i \ >> --rxq=4 --txq=4 --num-procs=2 --proc-id=1 >> > > Hi Connor, > > Thanks for the update. +Anatoly as multi-process maintainer, and ethdev > maintainers. > >> Signed-off-by: Min Hu (Connor) >> Signed-off-by: Lijun Ou >> Acked-by: Xiaoyun Li >> Acked-by: Ajit Khaparde >> --- >> v9: >> * Updated release notes and rst doc. >> * Deleted deprecated codes. >> * move macro and variable. >> >> v8: >> * Added warning info about queue numbers and process numbers. >> >> v7: >> * Fixed compiling error for unexpected unindent. >> >> v6: >> * Add rte flow description for multiple process. >> >> v5: >> * Fixed run_app.rst for multiple process description. >> * Fix compiling error. >> >> v4: >> * Fixed minimum vlaue of Rxq or Txq in doc. >> >> v3: >> * Fixed compiling error using gcc10.0. >> >> v2: >> * Added document for this patch. >> --- >>   app/test-pmd/cmdline.c                 |   6 ++ >>   app/test-pmd/config.c                  |  14 ++++- >>   app/test-pmd/parameters.c              |  12 ++++ >>   app/test-pmd/testpmd.c                 | 108 >> +++++++++++++++++++++++---------- >>   app/test-pmd/testpmd.h                 |   3 + >>   doc/guides/rel_notes/release_21_05.rst |   1 + >>   doc/guides/testpmd_app_ug/run_app.rst  |  86 ++++++++++++++++++++++++++ >>   7 files changed, 196 insertions(+), 34 deletions(-) >> >> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c >> index 5bf1497..e465824 100644 >> --- a/app/test-pmd/cmdline.c >> +++ b/app/test-pmd/cmdline.c >> @@ -5354,6 +5354,12 @@ cmd_set_flush_rx_parsed(void *parsed_result, >>           __rte_unused void *data) >>   { >>       struct cmd_set_flush_rx *res = parsed_result; >> + >> +    if (num_procs > 1 && (strcmp(res->mode, "on") == 0)) { >> +        printf("multi-process doesn't support to flush rx queues.\n"); >> +        return; >> +    } >> + >>       no_flush_rx = (uint8_t)((strcmp(res->mode, "on") == 0) ? 0 : 1); >>   } >> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c >> index 40b2b29..c982c87 100644 >> --- a/app/test-pmd/config.c >> +++ b/app/test-pmd/config.c >> @@ -2860,6 +2860,8 @@ rss_fwd_config_setup(void) >>       queueid_t  rxq; >>       queueid_t  nb_q; >>       streamid_t  sm_id; >> +    int start; >> +    int end; >>       nb_q = nb_rxq; >>       if (nb_q > nb_txq) >> @@ -2877,7 +2879,15 @@ rss_fwd_config_setup(void) >>       init_fwd_streams(); >>       setup_fwd_config_of_each_lcore(&cur_fwd_config); >> -    rxp = 0; rxq = 0; >> + >> +    if (proc_id > 0 && nb_q % num_procs) >> +        printf("Warning! queue numbers should be multiple of " >> +            "processes, or packet loss will happen.\n"); >> + >> +    start = proc_id * nb_q / num_procs; >> +    end = start + nb_q / num_procs; >> +    rxp = 0; >> +    rxq = start; > > Can you put some comment above, on what/why is done, something similar > to you already put into the documentation? > >>       for (sm_id = 0; sm_id < cur_fwd_config.nb_fwd_streams; sm_id++) { >>           struct fwd_stream *fs; >> @@ -2894,6 +2904,8 @@ rss_fwd_config_setup(void) >>               continue; >>           rxp = 0; >>           rxq++; >> +        if (rxq >= end) >> +            rxq = start; >>       } >>   } >> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c >> index f3954c1..d86cc21 100644 >> --- a/app/test-pmd/parameters.c >> +++ b/app/test-pmd/parameters.c >> @@ -508,6 +508,9 @@ parse_link_speed(int n) >>   void >>   launch_args_parse(int argc, char** argv) >>   { >> +#define PARAM_PROC_ID "proc-id" >> +#define PARAM_NUM_PROCS "num-procs" >> + >>       int n, opt; >>       char **argvopt; >>       int opt_idx; >> @@ -625,6 +628,8 @@ launch_args_parse(int argc, char** argv) >>           { "rx-mq-mode",                 1, 0, 0 }, >>           { "record-core-cycles",         0, 0, 0 }, >>           { "record-burst-stats",         0, 0, 0 }, >> +        { PARAM_NUM_PROCS,              1, 0, 0 }, >> +        { PARAM_PROC_ID,                1, 0, 0 }, >>           { 0, 0, 0, 0 }, >>       }; >> @@ -1391,6 +1396,13 @@ launch_args_parse(int argc, char** argv) >>                   record_core_cycles = 1; >>               if (!strcmp(lgopts[opt_idx].name, "record-burst-stats")) >>                   record_burst_stats = 1; >> + > > To be consistent for rest, this empty line can be removed. > >> +            if (strncmp(lgopts[opt_idx].name, >> +                    PARAM_NUM_PROCS, 9) == 0) >> +                num_procs = atoi(optarg); >> +            if (strncmp(lgopts[opt_idx].name, >> +                    PARAM_PROC_ID, 7) == 0) >> +                proc_id = atoi(optarg); >>               break; >>           case 'h': >>               usage(argv[0]); >> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c >> index 96d2e0f..01d0d82 100644 >> --- a/app/test-pmd/testpmd.c >> +++ b/app/test-pmd/testpmd.c >> @@ -518,6 +518,16 @@ enum rte_eth_rx_mq_mode rx_mq_mode = >> ETH_MQ_RX_VMDQ_DCB_RSS; >>    */ >>   uint32_t eth_link_speed; >> +/* >> + * Id of the current process. >> + */ > > Can you please add more info, like this being related to the > primary/secondary process support and used to configure the queues to be > polled. > >> +int proc_id; >> + >> +/* >> + * Number of processes. >> + */ >> +unsigned int num_procs = 1; >> + > > Ditto. > > <...> > >> @@ -2511,21 +2537,28 @@ start_port(portid_t pid) >>                   return -1; >>               } >>               /* configure port */ >> -            diag = rte_eth_dev_configure(pi, nb_rxq + nb_hairpinq, >> +            if (rte_eal_process_type() == RTE_PROC_PRIMARY) { >> +                diag = rte_eth_dev_configure(pi, >> +                             nb_rxq + nb_hairpinq, >>                                nb_txq + nb_hairpinq, >>                                &(port->dev_conf)); >> -            if (diag != 0) { >> -                if (rte_atomic16_cmpset(&(port->port_status), >> -                RTE_PORT_HANDLING, RTE_PORT_STOPPED) == 0) >> -                    printf("Port %d can not be set back " >> -                            "to stopped\n", pi); >> -                printf("Fail to configure port %d\n", pi); >> -                /* try to reconfigure port next time */ >> -                port->need_reconfig = 1; >> -                return -1; >> +                if (diag != 0) { >> +                    if (rte_atomic16_cmpset( >> +                            &(port->port_status), >> +                            RTE_PORT_HANDLING, >> +                            RTE_PORT_STOPPED) == 0) >> +                        printf("Port %d can not be set " >> +                               "back to stopped\n", pi); >> +                    printf("Fail to configure port %d\n", >> +                        pi); >> +                    /* try to reconfigure port next time */ >> +                    port->need_reconfig = 1; >> +                    return -1; >> +                } > > Just an idea, > I am a little worried about the complexity this new process type checks > are added to the multiple locations. What do you think hiding process > type checks behind new functions, like: > eth_dev_configure_mp(...) { >   if (rte_eal_process_type() == RTE_PROC_PRIMARY) { >     return rte_eth_dev_configure(..) >   } >   return 0; > } > > and replace it with 'rte_eth_dev_configure()': > >  -diag = rte_eth_dev_configure(...) >  +diag = eth_dev_configure_mp(...) > > Do you think does this help reducing the complexity added by the > multi-process support? > >>               } >>           } >> -        if (port->need_reconfig_queues > 0) { >> +        if (port->need_reconfig_queues > 0 && >> +            rte_eal_process_type() == RTE_PROC_PRIMARY) { > > According our coding convention, we don't allign with paranthesis but > put double tabs. > > And what about creating an inline function for primary process check, > like is_proc_primary(), that may help. > >>               port->need_reconfig_queues = 0; >>               /* setup tx queues */ >>               for (qi = 0; qi < nb_txq; qi++) { >> @@ -2626,17 +2659,20 @@ start_port(portid_t pid) >>           cnt_pi++; >>           /* start port */ >> -        diag = rte_eth_dev_start(pi); >> -        if (diag < 0) { >> -            printf("Fail to start port %d: %s\n", pi, >> -                   rte_strerror(-diag)); >> +        if (rte_eal_process_type() == RTE_PROC_PRIMARY) { >> +            diag = rte_eth_dev_start(pi); >> +            if (diag < 0) { >> +                printf("Fail to start port %d: %s\n", pi, >> +                       rte_strerror(-diag)); >> -            /* Fail to setup rx queue, return */ >> -            if (rte_atomic16_cmpset(&(port->port_status), >> +                /* Fail to setup rx queue, return */ >> +                if (rte_atomic16_cmpset(&(port->port_status), >>                   RTE_PORT_HANDLING, RTE_PORT_STOPPED) == 0) > > Syntax is wrong here after change, but if you go with above idea to hide > process type check within functions, you won't need to change these > lines at all. > > <...> > >> @@ -3885,8 +3925,10 @@ main(int argc, char** argv) >>           } >>       } >> -    if (!no_device_start && start_port(RTE_PORT_ALL) != 0) >> +    if (!no_device_start && start_port(RTE_PORT_ALL) != 0) { >> +        pmd_test_exit(); > > Why need to add 'pmd_test_exit()' for the multi-process support, if > there is a special case, please add comment for it. > > > .