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 27697A0C4C; Wed, 11 Aug 2021 04:48:14 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9CE6B4067A; Wed, 11 Aug 2021 04:48:13 +0200 (CEST) Received: from mail-wm1-f42.google.com (mail-wm1-f42.google.com [209.85.128.42]) by mails.dpdk.org (Postfix) with ESMTP id C113E40150 for ; Wed, 11 Aug 2021 04:48:12 +0200 (CEST) Received: by mail-wm1-f42.google.com with SMTP id m28-20020a05600c3b1cb02902b5a8c22575so915496wms.0 for ; Tue, 10 Aug 2021 19:48:12 -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=TqdXGKon/cnOhmVTpdgqgeW1hjR/WMVHleeHz7g9Dac=; b=Ov4cbv09gcgEwX306ZBlBLjLxWplOzGrk393dTBkfT1MkYXB8XseeaYwGx8TUvSmTZ q9Ma8TJd5QiOWmqvRw4IcC5lE8CAy5wvXu1IV9++A+uGmm5hOrmUSJvPGUvI7bbv2Ddm GN3sD15c36zqdR3jqgtSKBGiZUyN5QVH5rB64zlPSsQmcIkCEIyBOdJVw0mkMZnlA26R TLhtcsjTzrS2iKMocZZ4C6ZSONh+KK/wj1xVRd8oh7mbbjCwYHJcNGYgWa24Unclns91 hk/M9hfUiMLF6dzsms34T5OasDLTqHAMuGp2X4xIqVFBZ/eW69oAGNxWbyGTGbN+Jxrb nw0A== 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=TqdXGKon/cnOhmVTpdgqgeW1hjR/WMVHleeHz7g9Dac=; b=g1X3Y9X2tCz1d4niqtPPPwSkz5aG1bZSheCN9NbGIiN97UA7XxZmneUSrySn7zZQIE cX7fbG1sJ+GbjFkTPpYvI2unVphRfD2/p8DGjeah865E5VDMWR6nwmV7kP5l80gtu8gv nOf14tQJ8HgL1G1lk7ykzRcKYMU8ibX8NC0M3PV5SrHoLZfREmolHd+7AWK4aNXYR+Eq rjp0/0NiYMKy9bDOs61dzZt0p0VuV4UOiXMDoiKNglRUp8+M/H5bCUpw9NI6dX9TDUrG RUs7mbSYVcIsUZYPV58nqEnH8F3TGGGL40ptWz5Y/aQ9u/NqFfmJOxu9muQ1TxKX6RGU ZuAg== X-Gm-Message-State: AOAM533uWCI9gxNe4HdU1bTqLOSPInB0hBgzP11lYLHS1/97j/jqsi6S Qbmmd8+/uG70Z18IZMZadPHPzXOr0+cTHuaETzjSSQ== X-Google-Smtp-Source: ABdhPJypTsuDAvl/pXgaBkaLVVtGe1CbDbrOvPSRfo3WPOJ8wMeYtO3JXCYgx8DT2YtA78edEHCpj6Q5ynGB6g4ncK4= X-Received: by 2002:a7b:ce8a:: with SMTP id q10mr6829730wmj.155.1628650091940; Tue, 10 Aug 2021 19:48:11 -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: <09afb669-2152-6770-5f89-bebc36bad6e9@intel.com> From: =?UTF-8?B?546L5b+X5a6P?= Date: Wed, 11 Aug 2021 10:48:00 +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 Tue, Aug 10, 2021 at 5:12 PM Ferruh Yigit wrote= : > > 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 w= rote: > >> > >> 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 only > changing destination IP. > > What additional benefit does changing udp ports of the packets brings? Wh= at is > your usecase for this change? Pkt generators like pktgen/trex/ixia/spirent can change various fields including ip/udp src/dst. 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 per-= 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 before = 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 generation = 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 addres= s will > change in every 100 packets and src will change in every 10000 packets. T= his is > a slight behavior change. > > When it was only dst ip, it was simple to just increment it, not sure abo= ut it > in this case. I wonder if we should set all randomly for each packet. I d= on't > know what is the better logic here, we can discuss it more in the next ve= rsion. 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. > > >> > >>> } > >>> > >>> nb_tx =3D rte_eth_tx_burst(fs->tx_port, fs->tx_queue, pkts_burs= t, 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 - nb_= tx); > >>> + &pkts_burst[nb_tx], nb_pkt - nb= _tx); > >>> } > >> > >> +1 to this fix, thanks for it. But can you please make a seperate patc= h 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_ud= p_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_ud= p_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_ip_= 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_ip_= 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 var= iables > >> (like 'next_ip_src' etc..) used? Is it for the performance, if so what= is the > >> impact? > >> > >> And why not directly assign from local variables to per-core variables= , 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 the n= eed 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 counter= */". > > 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 from= 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?