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 6C80741CEC; Mon, 20 Feb 2023 17:33:57 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5606D4309C; Mon, 20 Feb 2023 17:33:57 +0100 (CET) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mails.dpdk.org (Postfix) with ESMTP id 71BA34309B for ; Mon, 20 Feb 2023 17:33:56 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1676910836; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=fO7mSLrxMeoPqPs6VO+mFEQlv6b4IGhW/xNeIRXqenw=; b=eY7Ahi0yqxzC55zd+7KcEk2kLVIYiwobxUqUaqJ+8SWb6u2uucPJ8HzW1WN8csa0Pr4uGK Hyu+DdbQYx/EkYZqV+qy5TjizD4KOrCYM7VTiG7MHYHB0CpzRWEurGhqZ6O4LAVE3jmIXu 711VCDuwLtEMA3wuTDA8208k+EaB95w= Received: from mail-pl1-f197.google.com (mail-pl1-f197.google.com [209.85.214.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-541-ZLROIqyBNXO1JFADYP2S8A-1; Mon, 20 Feb 2023 11:33:52 -0500 X-MC-Unique: ZLROIqyBNXO1JFADYP2S8A-1 Received: by mail-pl1-f197.google.com with SMTP id z1-20020a170902d54100b00198bc9ba4edso653943plf.21 for ; Mon, 20 Feb 2023 08:33:52 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=fO7mSLrxMeoPqPs6VO+mFEQlv6b4IGhW/xNeIRXqenw=; b=iJuQBoaDdxf3GoE/57KcGqt2G55I0rNT4kDQdvBO/y0AqckK5+2+dOIYWHZ9+OviaQ HxTp41F97k6RdZCrJ4pb7JdtVgYFaY3sYXNxRc2T9BOhsvVnEOFjnhQrgFhs/R5pECpB XNgv2XHJuLIW+F+4hbFBYYc+AYvd6Ex7k/O6HVsHQSqFwbtrzVLYUDpSWKV5Z9xqzU4z RnrkDOaR5yIZF6PhYvCrQ87VBbljUQnJqBBLXDVBkitbV3YhW8epTLHZ+3qlv5um5NV5 uoUeEEjjkuQLZPq6rS5RKBrQMOt95hVcv9/v7NmJV1yAWLhC5SnrsKSLFRoi+GQP7LOC ZUbg== X-Gm-Message-State: AO0yUKU/xmM6ALpihcjfK3NC+47ERfkTnt7BfYMlU/fE5QvB/HFAr5aR qgqwFNqR+g5GZtWimt/kTwtgM4TwBwqc82Icomg3i94qTjmQd1P3gZnqEu87X3dqPoIJk0iP7V5 GejGtXIJugvxaAuLU514BLnU3Vg4= X-Received: by 2002:a63:9255:0:b0:4fc:2058:fa29 with SMTP id s21-20020a639255000000b004fc2058fa29mr241477pgn.1.1676910831751; Mon, 20 Feb 2023 08:33:51 -0800 (PST) X-Google-Smtp-Source: AK7set+s5pq6VLqN9C4Bjfw1eEQBjCP5+D0eU9cCESlHAkR4JR1dVzlf2qSGvzrQnBE5tr9KV+SM7pbd61ofdofiKCg= X-Received: by 2002:a63:9255:0:b0:4fc:2058:fa29 with SMTP id s21-20020a639255000000b004fc2058fa29mr241472pgn.1.1676910831370; Mon, 20 Feb 2023 08:33:51 -0800 (PST) MIME-Version: 1.0 References: <20230124104742.1265439-1-david.marchand@redhat.com> <20230124104742.1265439-7-david.marchand@redhat.com> In-Reply-To: From: David Marchand Date: Mon, 20 Feb 2023 17:33:40 +0100 Message-ID: Subject: Re: [PATCH 6/6] app/testpmd: factorize fwd engine Tx To: Ferruh Yigit Cc: dev@dpdk.org, Aman Singh , Yuying Zhang , Robin Jarry X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" 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 On Tue, Feb 14, 2023 at 7:16 PM Ferruh Yigit wrote: > > On 1/24/2023 10:47 AM, David Marchand wrote: > > Reduce code duplication by introducing a helper that takes care of > > transmitting, retrying if enabled and incrementing tx counter. > > > > Signed-off-by: David Marchand > > <...> > > > diff --git a/app/test-pmd/noisy_vnf.c b/app/test-pmd/noisy_vnf.c > > index 937d5a1d7d..3875590132 100644 > > --- a/app/test-pmd/noisy_vnf.c > > +++ b/app/test-pmd/noisy_vnf.c > > @@ -93,30 +93,6 @@ sim_memory_lookups(struct noisy_config *ncf, uint16_t nb_pkts) > > } > > } > > > > -static uint16_t > > -do_retry(uint16_t nb_rx, uint16_t nb_tx, struct rte_mbuf **pkts, > > - struct fwd_stream *fs) > > -{ > > - uint32_t retry = 0; > > - > > - while (nb_tx < nb_rx && 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[nb_tx], nb_rx - nb_tx); > > - } > > - > > - return nb_tx; > > -} > > - > > -static uint32_t > > -drop_pkts(struct rte_mbuf **pkts, uint16_t nb_rx, uint16_t nb_tx) > > -{ > > - if (nb_tx < nb_rx) > > - rte_pktmbuf_free_bulk(&pkts[nb_tx], nb_rx - nb_tx); > > - > > - return nb_rx - nb_tx; > > -} > > - > > /* > > * Forwarding of packets in noisy VNF mode. Forward packets but perform > > * memory operations first as specified on cmdline. > > @@ -156,38 +132,23 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs) > > > > if (!ncf->do_buffering) { > > sim_memory_lookups(ncf, nb_rx); > > - nb_tx = rte_eth_tx_burst(fs->tx_port, fs->tx_queue, > > - pkts_burst, nb_rx); > > - if (unlikely(nb_tx < nb_rx) && fs->retry_enabled) > > - nb_tx += do_retry(nb_rx, nb_tx, pkts_burst, fs); > > - inc_tx_burst_stats(fs, nb_tx); > > - fs->tx_packets += nb_tx; > > - fs->fwd_dropped += drop_pkts(pkts_burst, nb_rx, nb_tx); > > + nb_tx = common_fwd_stream_transmit(fs, pkts_burst, nb_rx); > > > > 'nb_tx' is not used or necessary in this context, so assignment is not > necassary. > > PS: > In the latest next-net head, there is a 'goto' here instead of return, > but that is becuase of recording cycles, becuase of optimization in this > set (patch 1/6) that needs to turn back to 'return' that is what I did > while applying patch. This part changed a bit after rebasing and I think nb_tx is still needed in this context. Please have a look at v2. > > > kreturn true; > > } > > > > fifo_free = rte_ring_free_count(ncf->f); > > if (fifo_free >= nb_rx) { > > - nb_enqd = rte_ring_enqueue_burst(ncf->f, > > - (void **) pkts_burst, nb_rx, NULL); > > - if (nb_enqd < nb_rx) > > - fs->fwd_dropped += drop_pkts(pkts_burst, > > - nb_rx, nb_enqd); > > - } else { > > - nb_deqd = rte_ring_dequeue_burst(ncf->f, > > - (void **) tmp_pkts, nb_rx, NULL); > > - nb_enqd = rte_ring_enqueue_burst(ncf->f, > > - (void **) pkts_burst, nb_deqd, NULL); > > - if (nb_deqd > 0) { > > - nb_tx = rte_eth_tx_burst(fs->tx_port, > > - fs->tx_queue, tmp_pkts, > > - nb_deqd); > > - if (unlikely(nb_tx < nb_rx) && fs->retry_enabled) > > - nb_tx += do_retry(nb_rx, nb_tx, tmp_pkts, fs); > > - inc_tx_burst_stats(fs, nb_tx); > > - fs->fwd_dropped += drop_pkts(tmp_pkts, nb_deqd, nb_tx); > > + nb_enqd = rte_ring_enqueue_burst(ncf->f, (void **) pkts_burst, nb_rx, NULL); > > + if (nb_enqd < nb_rx) { > > + fs->fwd_dropped += nb_rx - nb_enqd; > > + rte_pktmbuf_free_bulk(&pkts_burst[nb_enqd], nb_rx - nb_enqd); > > Why not keep 'drop_pkts()' for this block, it is easier to read with it. That would be the only case where drop_pkts() is used. I am not convinced about readability but if you feel strongly about it, I don't mind restoring it (in a v3, I'll post v2 unchanged on this matter). > > > } > > + } else { > > + nb_deqd = rte_ring_dequeue_burst(ncf->f, (void **) tmp_pkts, nb_rx, NULL); > > + nb_enqd = rte_ring_enqueue_burst(ncf->f, (void **) pkts_burst, nb_deqd, NULL); > > + if (nb_deqd > 0) > > + nb_tx = common_fwd_stream_transmit(fs, tmp_pkts, nb_deqd); > > 'nb_tx' assignment looks wrong, > function returns 'nb_dropped' not 'nb_tx'. 'nb_tx' used below to detect > if flush needed ('needs_flush'), so 'needs_flush' may be set wrong > becuase dropped packet is used instead number of Tx packets. Indeed, this is wrong, I had caught the issue when preparing v2 after rebasing over Robin series. I had changed common_fwd_stream_transmit() so it returns the number of transmitted packets which is more in line with rte_eth_tx_burst(). This is easier to understand too. > > > } > > > > sim_memory_lookups(ncf, nb_enqd); > > @@ -204,15 +165,9 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs) > > needs_flush = delta_ms >= noisy_tx_sw_buf_flush_time && > > noisy_tx_sw_buf_flush_time > 0 && !nb_tx; > > while (needs_flush && !rte_ring_empty(ncf->f)) { > > - unsigned int sent; > > nb_deqd = rte_ring_dequeue_burst(ncf->f, (void **)tmp_pkts, > > MAX_PKT_BURST, NULL); > > - sent = rte_eth_tx_burst(fs->tx_port, fs->tx_queue, > > - tmp_pkts, nb_deqd); > > - if (unlikely(sent < nb_deqd) && fs->retry_enabled) > > - nb_tx += do_retry(nb_rx, nb_tx, tmp_pkts, fs); > > - inc_tx_burst_stats(fs, nb_tx); > > - fs->fwd_dropped += drop_pkts(tmp_pkts, nb_deqd, sent); > > + nb_tx = common_fwd_stream_transmit(fs, tmp_pkts, nb_deqd); > > similaryly 'nb_tx' assignment can be wrong here, and 'nb_tx' used for > return value to hint if record cycle is required, using number of > dropped packets (what 'common_fwd_stream_transmit()' returns) gives > wrong result. Yes, and there are other issues in this part which I moved to a separate patch for v2. > > > ncf->prev_time = rte_get_timer_cycles(); > > } > > > > diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h > > index e6b28b4748..71ff70f55b 100644 > > --- a/app/test-pmd/testpmd.h > > +++ b/app/test-pmd/testpmd.h > > @@ -870,6 +870,36 @@ common_fwd_stream_receive(struct fwd_stream *fs, struct rte_mbuf **burst, > > return nb_rx; > > } > > > > +/* Returns count of dropped packets. */ > > +static inline uint16_t > > +common_fwd_stream_transmit(struct fwd_stream *fs, struct rte_mbuf **burst, > > + unsigned int count) > > I would use 'nb_pkts' instead of 'count' as variable name since it is > more common, but of course this is subjective. Ok for me. I did the same renaming for the rx helper. > > > +{ > > + uint16_t nb_tx; > > + uint32_t retry; > > + > > + nb_tx = rte_eth_tx_burst(fs->tx_port, fs->tx_queue, burst, count); > > + /* > > + * Retry if necessary > > + */ > > + if (unlikely(nb_tx < count) && fs->retry_enabled) { > > + retry = 0; > > + while (nb_tx < count && retry++ < burst_tx_retry_num) { > > + rte_delay_us(burst_tx_delay_time); > > + nb_tx += rte_eth_tx_burst(fs->tx_port, fs->tx_queue, > > + &burst[nb_tx], count - nb_tx); > > + } > > + } > > + fs->tx_packets += nb_tx; > > + inc_tx_burst_stats(fs, nb_tx); > > + if (unlikely(nb_tx < count)) { > > + fs->fwd_dropped += (count - nb_tx); > > + rte_pktmbuf_free_bulk(&burst[nb_tx], count - nb_tx); > > + } > > + > > + return count - nb_tx; > > Instead of returning number of dropped packets, what about returning > number of packets sent ('nb_tx')? > Intuitively this is what expected from a function named > 'common_fwd_stream_transmit()', and even if it is more optimised to > return number of dropped packet, this may have only a little impact on > the caller code. Ah ah, well, I thought the same after rebasing v1. This change is in v2. > > > And even 'fs->tx_packets' updated withing the function, updating it > externally and explicitly feels me as it clarifies usage more, although > this part is up to you, I mean usage like: > fs->tx_packets += common_fwd_stream_transmit(fs, pkts, nb_pkts); We would have fs->tx_packets managed by fwd engine code, but fwd_dropped by the common helper? I prefer fwd engines do not have to deal with the stats at all. We have a lot of fwd engines, and small issues are often (re-)introduced with new fwd engines. A centralised approach is more robust. Thanks Ferruh. -- David Marchand