From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id A46A9A04BA; Wed, 7 Oct 2020 10:27:47 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id A488A1B3C8; Wed, 7 Oct 2020 10:27:45 +0200 (CEST) Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by dpdk.org (Postfix) with ESMTP id BEE314C9D for ; Wed, 7 Oct 2020 10:27:42 +0200 (CEST) Received: by mail-wr1-f66.google.com with SMTP id t9so1088689wrq.11 for ; Wed, 07 Oct 2020 01:27:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=R1GCLKRJTeJBXk8x/brp9wk1rbsfdc9MX7Q99nmWEdQ=; b=BM+siZwf0ncYVvHgsNxxnuIqGi3afy+N0vwKSnBatb8ZIcAnpCrU4WaLfU/vqF+5H1 wVLD7z1FqRaT1dURrG3CsQu24OAb+nkn8pPVmFpIraEUoxPd6kElN1ZUoIgtTBEwDPCZ Xi9GKccLq86IpxNHZqJVAlqo9HvpIWUnNMkSoe/lRzpKoaRyeXSUVadRpueYQWLo20mT ZPtj3V1Cbefk1ifmGKAm66kP/FU1QOqjEFJhiSyH2Bpswl0TWJXZy7k1ZZN/GR9Ad/jB y7ELWU91PLqsxUcmuwEZU4u65Rv1ci9cFAYutnya+U6Gkoynl0317YAQk+Oy0pV1ufyt Vqdg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=R1GCLKRJTeJBXk8x/brp9wk1rbsfdc9MX7Q99nmWEdQ=; b=PVE9z2cwr5AOWnvbaPwNr4u99ne5zYvfLJplBQGni7VVFCB55rlmEQqGXvct6VGPUI i4etLO54gGOHDfPgf4Ep89V8bxDrGa69es6FjkcRwy98rScmd/qISxBmmijWDuioDMh0 HCPVDquWvSJwoNrtIinkH4pyHTM5eHhY23tlrvgpA4HiB0BGerOCuMjGF3+GHg1kYc+j tTleQGbZ43c9irm0Wxo7EI0pS2fEiqZ36zAAaJEvSWbM1aqejW706JaCDQXIUiCSZ3Cr wjX0At8eVgUkCteg4R3fEcAk5+s2UTEed7Kj4uipHZpC9SEuoMBPPQm/7GmL2M/coYhY TsJQ== X-Gm-Message-State: AOAM530lAkJPEYk/oKFBca0BiNJk/OWO0ZK015is2e5Rpv+BMgTV9Vd7 0WkwH7Odd1xBj6XhpHD/zonWVA== X-Google-Smtp-Source: ABdhPJxceCzztjsuayEQzKQD/E30JrTWFT723zJWONRNXllfHsjV+WSOja7Vv01wl3FuI+B9SXIIXQ== X-Received: by 2002:adf:db4d:: with SMTP id f13mr2165237wrj.155.1602059261411; Wed, 07 Oct 2020 01:27:41 -0700 (PDT) Received: from 6wind.com (2a01cb0c0005a600345636f7e65ed1a0.ipv6.abo.wanadoo.fr. [2a01:cb0c:5:a600:3456:36f7:e65e:d1a0]) by smtp.gmail.com with ESMTPSA id q18sm1747570wre.78.2020.10.07.01.27.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Oct 2020 01:27:39 -0700 (PDT) Date: Wed, 7 Oct 2020 10:27:39 +0200 From: Olivier Matz To: Honnappa Nagarahalli Cc: dev@dpdk.org, konstantin.ananyev@intel.com, david.marchand@redhat.com, nd@arm.com Message-ID: <20201007082739.GL21395@platinum> References: <20200224203931.21256-1-honnappa.nagarahalli@arm.com> <20201006132905.46205-1-honnappa.nagarahalli@arm.com> <20201006132905.46205-2-honnappa.nagarahalli@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201006132905.46205-2-honnappa.nagarahalli@arm.com> User-Agent: Mutt/1.10.1 (2018-07-13) Subject: Re: [dpdk-dev] [RFC v2 1/1] lib/ring: add scatter gather APIs X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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" Hi Honnappa, >From a quick walkthrough, I have some questions/comments, please see below. On Tue, Oct 06, 2020 at 08:29:05AM -0500, Honnappa Nagarahalli wrote: > Add scatter gather APIs to avoid intermediate memcpy. Use cases > that involve copying large amount of data to/from the ring > can benefit from these APIs. > > Signed-off-by: Honnappa Nagarahalli > --- > lib/librte_ring/meson.build | 3 +- > lib/librte_ring/rte_ring_elem.h | 1 + > lib/librte_ring/rte_ring_peek_sg.h | 552 +++++++++++++++++++++++++++++ > 3 files changed, 555 insertions(+), 1 deletion(-) > create mode 100644 lib/librte_ring/rte_ring_peek_sg.h > > diff --git a/lib/librte_ring/meson.build b/lib/librte_ring/meson.build > index 31c0b4649..377694713 100644 > --- a/lib/librte_ring/meson.build > +++ b/lib/librte_ring/meson.build > @@ -12,4 +12,5 @@ headers = files('rte_ring.h', > 'rte_ring_peek.h', > 'rte_ring_peek_c11_mem.h', > 'rte_ring_rts.h', > - 'rte_ring_rts_c11_mem.h') > + 'rte_ring_rts_c11_mem.h', > + 'rte_ring_peek_sg.h') > diff --git a/lib/librte_ring/rte_ring_elem.h b/lib/librte_ring/rte_ring_elem.h > index 938b398fc..7d3933f15 100644 > --- a/lib/librte_ring/rte_ring_elem.h > +++ b/lib/librte_ring/rte_ring_elem.h > @@ -1079,6 +1079,7 @@ rte_ring_dequeue_burst_elem(struct rte_ring *r, void *obj_table, > > #ifdef ALLOW_EXPERIMENTAL_API > #include > +#include > #endif > > #include > diff --git a/lib/librte_ring/rte_ring_peek_sg.h b/lib/librte_ring/rte_ring_peek_sg.h > new file mode 100644 > index 000000000..97d5764a6 > --- /dev/null > +++ b/lib/librte_ring/rte_ring_peek_sg.h > @@ -0,0 +1,552 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * > + * Copyright (c) 2020 Arm > + * Copyright (c) 2007-2009 Kip Macy kmacy@freebsd.org > + * All rights reserved. > + * Derived from FreeBSD's bufring.h > + * Used as BSD-3 Licensed with permission from Kip Macy. > + */ > + > +#ifndef _RTE_RING_PEEK_SG_H_ > +#define _RTE_RING_PEEK_SG_H_ > + > +/** > + * @file > + * @b EXPERIMENTAL: this API may change without prior notice > + * It is not recommended to include this file directly. > + * Please include instead. > + * > + * Ring Peek Scatter Gather APIs I am not fully convinced by the API name. To me, "scatter/gather" is associated to iovecs, like for instance in [1]. The wikipedia definition [2] may be closer though. [1] https://www.gnu.org/software/libc/manual/html_node/Scatter_002dGather.html [2] https://en.wikipedia.org/wiki/Gather-scatter_(vector_addressing) What about "zero-copy"? Also, the "peek" term looks also a bit confusing to me, but it existed before your patch. I understand it for dequeue, but not for enqueue. Or, what about replacing the existing experimental peek API by this one? They look quite similar to me. > + * Introduction of rte_ring with scatter gather serialized producer/consumer > + * (HTS sync mode) makes it possible to split public enqueue/dequeue API > + * into 3 phases: > + * - enqueue/dequeue start > + * - copy data to/from the ring > + * - enqueue/dequeue finish > + * Along with the advantages of the peek APIs, these APIs provide the ability > + * to avoid copying of the data to temporary area. > + * > + * Note that right now this new API is available only for two sync modes: > + * 1) Single Producer/Single Consumer (RTE_RING_SYNC_ST) > + * 2) Serialized Producer/Serialized Consumer (RTE_RING_SYNC_MT_HTS). > + * It is a user responsibility to create/init ring with appropriate sync > + * modes selected. > + * > + * Example usage: > + * // read 1 elem from the ring: Comment should be "prepare enqueuing 32 objects" > + * n = rte_ring_enqueue_sg_bulk_start(ring, 32, &sgd, NULL); > + * if (n != 0) { > + * //Copy objects in the ring > + * memcpy (sgd->ptr1, obj, sgd->n1 * sizeof(uintptr_t)); > + * if (n != sgd->n1) > + * //Second memcpy because of wrapround > + * n2 = n - sgd->n1; > + * memcpy (sgd->ptr2, obj[n2], n2 * sizeof(uintptr_t)); Missing { } > + * rte_ring_dequeue_sg_finish(ring, n); Should be enqueue > + * } > + * > + * Note that between _start_ and _finish_ none other thread can proceed > + * with enqueue(/dequeue) operation till _finish_ completes. > + */ > + > +#ifdef __cplusplus > +extern "C" { > +#endif > + > +#include > + > +/* Rock that needs to be passed between reserve and commit APIs */ > +struct rte_ring_sg_data { > + /* Pointer to the first space in the ring */ > + void **ptr1; > + /* Pointer to the second space in the ring if there is wrap-around */ > + void **ptr2; > + /* Number of elements in the first pointer. If this is equal to > + * the number of elements requested, then ptr2 is NULL. > + * Otherwise, subtracting n1 from number of elements requested > + * will give the number of elements available at ptr2. > + */ > + unsigned int n1; > +}; Would it be possible to simply return the offset instead of this structure? The wrap could be managed by a __rte_ring_enqueue_elems() function. I mean something like this: uint32_t start; n = rte_ring_enqueue_sg_bulk_start(ring, 32, &start, NULL); if (n != 0) { /* Copy objects in the ring. */ __rte_ring_enqueue_elems(ring, start, obj, sizeof(uintptr_t), n); rte_ring_enqueue_sg_finish(ring, n); } It would require to slightly change __rte_ring_enqueue_elems() to support to be called with prod_head >= size, and wrap in that case. Regards, Olivier