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 2CF19A0C41; Fri, 16 Apr 2021 18:30:50 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 00E0540687; Fri, 16 Apr 2021 18:30:50 +0200 (CEST) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by mails.dpdk.org (Postfix) with ESMTP id 0A9F040150 for ; Fri, 16 Apr 2021 18:30:47 +0200 (CEST) IronPort-SDR: 5MLYwmoqeVVa7iU+4VHRGe5hrCfsHLXnnrWcrmef/Mej+e6sB/GHNQYonOk615JjdNQprHVjn7 I4R9GlbzmYWQ== X-IronPort-AV: E=McAfee;i="6200,9189,9956"; a="191874679" X-IronPort-AV: E=Sophos;i="5.82,226,1613462400"; d="scan'208";a="191874679" Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Apr 2021 09:30:46 -0700 IronPort-SDR: 7SoeW5tpvRdMblbwYWHOY7LJTzCBMCOrDdsjY73qmlmhFioGz1sWIFh4zV8ODTxzSwt8DtjY0z Wr44HJjpOvPw== X-IronPort-AV: E=Sophos;i="5.82,226,1613462400"; d="scan'208";a="453392928" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.207.150]) ([10.213.207.150]) by fmsmga002-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Apr 2021 09:30:44 -0700 To: "Min Hu (Connor)" , dev@dpdk.org, Xiaoyun Li References: <1614906276-34293-1-git-send-email-oulijun@huawei.com> <1618537978-35479-1-git-send-email-humin29@huawei.com> From: Ferruh Yigit Cc: Anatoly Burakov , Andrew Rybchenko , Thomas Monjalon X-User: ferruhy Message-ID: Date: Fri, 16 Apr 2021 17:30:40 +0100 MIME-Version: 1.0 In-Reply-To: <1618537978-35479-1-git-send-email-humin29@huawei.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit 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" 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.