From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
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 <dev@dpdk.org>; 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 <ferruh.yigit@intel.com>, <dev@dpdk.org>, Xiaoyun Li
<xiaoyun.li@intel.com>
CC: Anatoly Burakov <anatoly.burakov@intel.com>, Andrew Rybchenko
<andrew.rybchenko@oktetlabs.ru>, Thomas Monjalon <thomas@monjalon.net>
References: <1614906276-34293-1-git-send-email-oulijun@huawei.com>
<1618537978-35479-1-git-send-email-humin29@huawei.com>
<b369c67b-33b1-892d-e596-a3b776958382@intel.com>
From: "Min Hu (Connor)" <humin29@huawei.com>
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: <b369c67b-33b1-892d-e596-a3b776958382@intel.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
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) <humin29@huawei.com>
>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>> Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
>> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>> ---
>> 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.
>
>
> .