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 B4533A0C56; Thu, 12 Aug 2021 11:33:05 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2FDA84014D; Thu, 12 Aug 2021 11:33:05 +0200 (CEST) Received: from mail-pj1-f54.google.com (mail-pj1-f54.google.com [209.85.216.54]) by mails.dpdk.org (Postfix) with ESMTP id C1AB640042 for ; Thu, 12 Aug 2021 11:33:04 +0200 (CEST) Received: by mail-pj1-f54.google.com with SMTP id s22-20020a17090a1c16b0290177caeba067so14337692pjs.0 for ; Thu, 12 Aug 2021 02:33:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bytedance-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=EyiSxx4yo/6qX9z/ckTzfMVF4+0vNx7nCSNcdZ+SNYA=; b=g9b0NDm7fa5G+8K2hLDZU/OiUPhoGlJ68BmfGkZ1wtk1VMBdqDVW7Hbf8X/tymQhgC w9cHyBLQeYvYpP4LmZ04MzRohFTFtLpuBwlXCj+ip0Xr8ZgfwBD6CecHLLXcSD4sGTXD QQYb9iHmW5sx/SWGhGanClxBWkY2yBOFijf8TPeEsVpHI60iMsS7lBHHjqU4vS185CbK vNVZJpeRpAORWoX9rxkmk37OfvjkHet9Kntd9Y40ywg0EuqiiJjiukuZWM7oLsxTFblT cP1eKY4xaMXImb8zqTmH/jKsog/WUSMiVf1ORrEgjXDJH04kFxJV5dBsBUBJ0R1EISz8 NHqQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=EyiSxx4yo/6qX9z/ckTzfMVF4+0vNx7nCSNcdZ+SNYA=; b=Dx14U910Na3KCaNEL3VZjb0fdcNo0Gvq8KZSAnNhecooemN8HDY6JjaDs8T3h/MROd iS0k8s2kJNxSGQnwUDfZq7xSYK0IZtgvoF0GdU6hptiu9SfwCSyBKCHArNXPprWIKJO1 p/OXu4nejKO4hh4ko9Oc+qf1on3uR6bQrapy++8Gwk2Cl7fKbr/JsNYvkK8dnwTlHuX4 Sc8hpV9WDxFhJ+REqiLwgpHbVooq5+tjoRqKvnyC/bqPuDME4uIYXCyhXI4J54JCpM8l 19inuru3dDbFVSft2YOFbmeqxBvug/3RTN6Le68ItSR125E5M7+Jmp2Vsmjl6r/chKrv 5H/w== X-Gm-Message-State: AOAM530d9qYNz5B4EeBOdyk/4gyXndPt3d/BeYRtRqA+5GOTlRTFlEyz /8poBCHjmZBjNr0Kbm/wjoGPlg8Zvn+zXB83YmCKtA== X-Google-Smtp-Source: ABdhPJxLHOhCIIJb147FUlZYOLhWiq+V2C+nGd2+JvYO80JnfLHBEsa9ZgaWjXQ+1QBs2K8nUU6hLppRhayqmTmP6os= X-Received: by 2002:a17:90a:c7cc:: with SMTP id gf12mr3430594pjb.152.1628760783724; Thu, 12 Aug 2021 02:33:03 -0700 (PDT) MIME-Version: 1.0 References: <20210809062548.30187-1-wangzhihong.wzh@bytedance.com> <20210809065200.31134-1-wangzhihong.wzh@bytedance.com> <09afb669-2152-6770-5f89-bebc36bad6e9@intel.com> In-Reply-To: From: =?UTF-8?B?546L5b+X5a6P?= Date: Thu, 12 Aug 2021 17:32:52 +0800 Message-ID: To: Ferruh Yigit Cc: dev@dpdk.org, xiaoyun.li@intel.com, "Singh, Aman Deep" , Igor Russkikh , Cyril Chemparathy Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [dpdk-dev] [External] Re: [PATCH v2] app/testpmd: flowgen support ip and udp fields 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 Wed, Aug 11, 2021 at 6:31 PM Ferruh Yigit wrote= : > > On 8/11/2021 3:48 AM, =E7=8E=8B=E5=BF=97=E5=AE=8F wrote: > > On Tue, Aug 10, 2021 at 5:12 PM Ferruh Yigit w= rote: > >> > >> On 8/10/2021 8:57 AM, =E7=8E=8B=E5=BF=97=E5=AE=8F wrote: > >>> Thanks for the review Ferruh :) > >>> > >>> On Mon, Aug 9, 2021 at 11:18 PM Ferruh Yigit = wrote: > >>>> > >>>> On 8/9/2021 7:52 AM, Zhihong Wang wrote: > >>>>> This patch aims to: > >>>>> 1. Add flexibility by supporting IP & UDP src/dst fields > >>>> > >>>> What is the reason/"use case" of this flexibility? > >>> > >>> The purpose is to emulate pkt generator behaviors. > >>> > >> > >> 'flowgen' forwarding is already to emulate pkt generator, but it was o= nly > >> changing destination IP. > >> > >> What additional benefit does changing udp ports of the packets brings?= What is > >> your usecase for this change? > > > > Pkt generators like pktgen/trex/ixia/spirent can change various fields > > including ip/udp src/dst. > > > > But testpmd is not packet generator, it has very simple 'flowgen' forward= ing > engine, I would like to understand motivation to make it more complex. I agree this *simplicity* point. In fact my sole intention is to make flowgen useable for multi-core test. I'll keep the original setup in the next patch. > > > Keeping the cfg_n_* while setting cfg_n_ip_dst =3D 1024 and others =3D = 1 > > makes the default behavior exactly unchanged. Do you think it makes > > sense? > > > >> > >>>> > >>>>> 2. Improve multi-core performance by using per-core vars> > >>>> > >>>> On multi core this also has syncronization problem, OK to make it pe= r-core. Do > >>>> you have any observed performance difference, if so how much is it? > >>> > >>> Huge difference, one example: 8 core flowgen -> rxonly results: 43 > >>> Mpps (per-core) vs. 9.3 Mpps (shared), of course the numbers "varies > >>> depending on system configuration". > >>> > >> > >> Thanks for clarification. > >> > >>>> > >>>> And can you please separate this to its own patch? This can be befor= e ip/udp update. > >>> > >>> Will do. > >>> > >>>> > >>>>> v2: fix assigning ip header cksum > >>>>> > >>>> > >>>> +1 to update, can you please make it as seperate patch? > >>> > >>> Sure. > >>> > >>>> > >>>> So overall this can be a patchset with 4 patches: > >>>> 1- Fix retry logic (nb_rx -> nb_pkt) > >>>> 2- Use 'rte_ipv4_cksum()' API (instead of static 'ip_sum()') > >>>> 3- User per-core varible (for 'next_flow') > >>>> 4- Support ip/udp src/dst variaty of packets > >>>> > >>> > >>> Great summary. Thanks a lot. > >>> > >>>>> Signed-off-by: Zhihong Wang > >>>>> --- > >>>>> app/test-pmd/flowgen.c | 137 +++++++++++++++++++++++++++++++------= ------------ > >>>>> 1 file changed, 86 insertions(+), 51 deletions(-) > >>>>> > >>>> > >>>> <...> > >>>> > >>>>> @@ -185,30 +193,57 @@ pkt_burst_flow_gen(struct fwd_stream *fs) > >>>>> } > >>>>> pkts_burst[nb_pkt] =3D pkt; > >>>>> > >>>>> - next_flow =3D (next_flow + 1) % cfg_n_flows; > >>>>> + if (++next_udp_dst < cfg_n_udp_dst) > >>>>> + continue; > >>>>> + next_udp_dst =3D 0; > >>>>> + if (++next_udp_src < cfg_n_udp_src) > >>>>> + continue; > >>>>> + next_udp_src =3D 0; > >>>>> + if (++next_ip_dst < cfg_n_ip_dst) > >>>>> + continue; > >>>>> + next_ip_dst =3D 0; > >>>>> + if (++next_ip_src < cfg_n_ip_src) > >>>>> + continue; > >>>>> + next_ip_src =3D 0; > >>>> > >>>> What is the logic here, can you please clarifiy the packet generatio= n logic both > >>>> in a comment here and in the commit log? > >>> > >>> It's round-robin field by field. Will add the comments. > >>> > >> > >> Thanks. If the receiving end is doing RSS based on IP address, dst add= ress will > >> change in every 100 packets and src will change in every 10000 packets= . This is > >> a slight behavior change. > >> > >> When it was only dst ip, it was simple to just increment it, not sure = about it > >> in this case. I wonder if we should set all randomly for each packet. = I don't > >> know what is the better logic here, we can discuss it more in the next= version. > > > > A more sophisticated pkt generator provides various options among > > "step-by-step" / "random" / etc. > > > > But supporting multiple fields naturally brings this implicitly. It > > won't be a problem as it can be configured by setting the cfg_n_* as > > we discussed above. > > > > I think rte_rand() is a good option, anyway this can be tweaked easily > > once the framework becomes shaped. > > > > Can be done, but do we really want to add more packet generator capabilit= y to > testpmd? > > >> > >>>> > >>>>> } > >>>>> > >>>>> nb_tx =3D rte_eth_tx_burst(fs->tx_port, fs->tx_queue, pkts_bu= rst, nb_pkt); > >>>>> /* > >>>>> * Retry if necessary > >>>>> */ > >>>>> - if (unlikely(nb_tx < nb_rx) && fs->retry_enabled) { > >>>>> + if (unlikely(nb_tx < nb_pkt) && fs->retry_enabled) { > >>>>> retry =3D 0; > >>>>> - while (nb_tx < nb_rx && retry++ < burst_tx_retry_num)= { > >>>>> + while (nb_tx < nb_pkt && retry++ < burst_tx_retry_num= ) { > >>>>> rte_delay_us(burst_tx_delay_time); > >>>>> nb_tx +=3D rte_eth_tx_burst(fs->tx_port, fs->= tx_queue, > >>>>> - &pkts_burst[nb_tx], nb_rx - n= b_tx); > >>>>> + &pkts_burst[nb_tx], nb_pkt - = nb_tx); > >>>>> } > >>>> > >>>> +1 to this fix, thanks for it. But can you please make a seperate pa= tch for > >>>> this, with proper 'Fixes:' tag etc.. > >>> > >>> Ok. > >>> > >>>> > >>>>> } > >>>>> - fs->tx_packets +=3D nb_tx; > >>>>> > >>>>> inc_tx_burst_stats(fs, nb_tx); > >>>>> - if (unlikely(nb_tx < nb_pkt)) { > >>>>> - /* Back out the flow counter. */ > >>>>> - next_flow -=3D (nb_pkt - nb_tx); > >>>>> - while (next_flow < 0) > >>>>> - next_flow +=3D cfg_n_flows; > >>>>> + fs->tx_packets +=3D nb_tx; > >>>>> + /* Catch up flow idx by actual sent. */ > >>>>> + for (i =3D 0; i < nb_tx; ++i) { > >>>>> + RTE_PER_LCORE(_next_udp_dst) =3D RTE_PER_LCORE(_next_= udp_dst) + 1; > >>>>> + if (RTE_PER_LCORE(_next_udp_dst) < cfg_n_udp_dst) > >>>>> + continue; > >>>>> + RTE_PER_LCORE(_next_udp_dst) =3D 0; > >>>>> + RTE_PER_LCORE(_next_udp_src) =3D RTE_PER_LCORE(_next_= udp_src) + 1; > >>>>> + if (RTE_PER_LCORE(_next_udp_src) < cfg_n_udp_src) > >>>>> + continue; > >>>>> + RTE_PER_LCORE(_next_udp_src) =3D 0; > >>>>> + RTE_PER_LCORE(_next_ip_dst) =3D RTE_PER_LCORE(_next_i= p_dst) + 1; > >>>>> + if (RTE_PER_LCORE(_next_ip_dst) < cfg_n_ip_dst) > >>>>> + continue; > >>>>> + RTE_PER_LCORE(_next_ip_dst) =3D 0; > >>>>> + RTE_PER_LCORE(_next_ip_src) =3D RTE_PER_LCORE(_next_i= p_src) + 1; > >>>>> + if (RTE_PER_LCORE(_next_ip_src) < cfg_n_ip_src) > >>>>> + continue; > >>>>> + RTE_PER_LCORE(_next_ip_src) =3D 0; > >>>>> + } > >>>> > >>>> Why per-core variables are not used in forward function, but local v= ariables > >>>> (like 'next_ip_src' etc..) used? Is it for the performance, if so wh= at is the > >>>> impact? > >>>> > >>>> And why not directly assign from local variables to per-core variabl= es, but have > >>>> above catch up loop? > >>>> > >>>> > >>> > >>> Local vars are for generating pkts, global ones catch up finally when > >>> nb_tx is clear. > >> > >> Why you are not using global ones to generate packets? This removes th= e need for > >> catch up? > > > > When there are multiple fields, back out the overran index caused by > > dropped packets is not that straightforward -- It's the "carry" issue > > in adding. > > > >> > >>> So flow indexes only increase by actual sent pkt number. > >>> It serves the same purpose of the original "/* backout the flow count= er */". > >>> My math isn't good enough to make it look more intelligent though. > >>> > >> > >> Maybe I am missing something, for this case why not just assign back f= rom locals > >> to globals? > > > > As above. > > > > However, this can be simplified if we discard the "back out" > > mechanism: generate 32 pkts and send 20 of them while the rest 12 are > > dropped, the difference is that is the idx gonna start from 21 or 33 > > next time? > > > > I am not sure point of "back out", I think we can remove it unless there = is no > objection, so receiving end can recognize failed packets. >