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 47B60A0C53; Tue, 10 Aug 2021 09:57:41 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id BAE8240E6E; Tue, 10 Aug 2021 09:57:40 +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 090894068E for ; Tue, 10 Aug 2021 09:57:39 +0200 (CEST) Received: by mail-wm1-f42.google.com with SMTP id 203-20020a1c00d40000b02902e6a4e244e4so250629wma.4 for ; Tue, 10 Aug 2021 00:57:39 -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; bh=/J8J8LsgVk7rcCnN/dIIdEGaH7z8CSuRkLdfUHtw7P0=; b=Z1rfTBBoFeoUOnKMN4fmxMuF4HYhQCuMT1sRqBUihVRwU0LU67Su9fuyU5DgZiWkER JSaoV9fWqNjbwXRaGeVZLOfx8uGbDp/tjEd3sVGXXdeZizQ3HiUtlh/r5fs689sHpdNI cddZ/xq+ZJXd858WLF1zg6NCai2CAKDLTzq+B83pFMjUuy8TtfgTvaZjJkKSU4Ey4z5a QzDX27RgB1dLYuudLKXjSb/bdsT3KaU03KSkLvvBfeVKbLW1JynN3mZvWZOHZRSqK8yE /MeRtK0Gl2bynSqex5e+niiOjVtya3eAHwfjUh3T1axQd/oXB3otnIh+1E3EagX7GZvp 9/pQ== 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; bh=/J8J8LsgVk7rcCnN/dIIdEGaH7z8CSuRkLdfUHtw7P0=; b=NrdNMVRP1hMwUnjyA7d7ypgi0Jz2Zeh3uHsMNUBq18jzVMmg2zS55cJt/JHoq9kENu hxRosE3v5uevUR2wlHHqohYprFDHfdxhIJs8saAhyuIKR16M8/hxLptIWmRESLnA2ntW waKQpWEFl7Tq+wtl/Nu+flQtnkTs7nbh6ioyrytx3AUBg1+59tLpOSDcQsr7JEPNweaC yP0ISDIbavl2My4AZ223ZXCUcv0o4BLUusqRGyCy5JugmHGiVN/gmSAJwCcxNn2O4LsA NqwoLCiE1bHWynu733aQDBS+5MUc4nI50aEPQyuh8eAcAbiKquBHOPV4ZKZ4YRCsD/Ut LTbA== X-Gm-Message-State: AOAM530aqsxxgsE+HHaG5kXjEz9jfKfxmtMeyvWk0S22bwgc41R/Jr7w FFXb/0I8fFzgyAox9swfV9j/fnbKAXDld4zdacq6Vg== X-Google-Smtp-Source: ABdhPJwaJ40zdsqB/G1A/UUfVTTf2Ejkl/RYQZupECcjrzmEqNxrQU7cB+gsctYE/mDYxKTWsQBiYQQHHIErSDBx7Fg= X-Received: by 2002:a1c:7711:: with SMTP id t17mr3162578wmi.77.1628582258624; Tue, 10 Aug 2021 00:57:38 -0700 (PDT) MIME-Version: 1.0 References: <20210809062548.30187-1-wangzhihong.wzh@bytedance.com> <20210809065200.31134-1-wangzhihong.wzh@bytedance.com> In-Reply-To: From: =?UTF-8?B?546L5b+X5a6P?= Date: Tue, 10 Aug 2021 15:57:27 +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" 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" 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. > > > 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". > > 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] = pkt; > > > > - next_flow = (next_flow + 1) % cfg_n_flows; > > + if (++next_udp_dst < cfg_n_udp_dst) > > + continue; > > + next_udp_dst = 0; > > + if (++next_udp_src < cfg_n_udp_src) > > + continue; > > + next_udp_src = 0; > > + if (++next_ip_dst < cfg_n_ip_dst) > > + continue; > > + next_ip_dst = 0; > > + if (++next_ip_src < cfg_n_ip_src) > > + continue; > > + next_ip_src = 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. > > > } > > > > nb_tx = rte_eth_tx_burst(fs->tx_port, fs->tx_queue, pkts_burst, nb_pkt); > > /* > > * Retry if necessary > > */ > > - if (unlikely(nb_tx < nb_rx) && fs->retry_enabled) { > > + if (unlikely(nb_tx < nb_pkt) && fs->retry_enabled) { > > retry = 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 += 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 patch for > this, with proper 'Fixes:' tag etc.. Ok. > > > } > > - fs->tx_packets += nb_tx; > > > > inc_tx_burst_stats(fs, nb_tx); > > - if (unlikely(nb_tx < nb_pkt)) { > > - /* Back out the flow counter. */ > > - next_flow -= (nb_pkt - nb_tx); > > - while (next_flow < 0) > > - next_flow += cfg_n_flows; > > + fs->tx_packets += nb_tx; > > + /* Catch up flow idx by actual sent. */ > > + for (i = 0; i < nb_tx; ++i) { > > + RTE_PER_LCORE(_next_udp_dst) = 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) = 0; > > + RTE_PER_LCORE(_next_udp_src) = 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) = 0; > > + RTE_PER_LCORE(_next_ip_dst) = 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) = 0; > > + RTE_PER_LCORE(_next_ip_src) = 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) = 0; > > + } > > Why per-core variables are not used in forward function, but local variables > (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. 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.