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 59443A3295 for ; Wed, 23 Oct 2019 11:48:43 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 177951BF78; Wed, 23 Oct 2019 11:48:42 +0200 (CEST) Received: from proxy.6wind.com (host.76.145.23.62.rev.coltfrance.com [62.23.145.76]) by dpdk.org (Postfix) with ESMTP id 2FD271BF76 for ; Wed, 23 Oct 2019 11:48:40 +0200 (CEST) Received: from glumotte.dev.6wind.com (unknown [10.16.0.195]) by proxy.6wind.com (Postfix) with ESMTP id EFC1633567F; Wed, 23 Oct 2019 11:48:39 +0200 (CEST) Date: Wed, 23 Oct 2019 11:48:39 +0200 From: Olivier Matz To: Honnappa Nagarahalli Cc: sthemmin@microsoft.com, jerinj@marvell.com, bruce.richardson@intel.com, david.marchand@redhat.com, pbhagavatula@marvell.com, konstantin.ananyev@intel.com, drc@linux.vnet.ibm.com, hemant.agrawal@nxp.com, dev@dpdk.org, dharmik.thakkar@arm.com, ruifeng.wang@arm.com, gavin.hu@arm.com Message-ID: <20191023094839.GB25286@glumotte.dev.6wind.com> References: <20190906190510.11146-1-honnappa.nagarahalli@arm.com> <20191021002300.26497-1-honnappa.nagarahalli@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20191021002300.26497-1-honnappa.nagarahalli@arm.com> User-Agent: Mutt/1.10.1 (2018-07-13) Subject: Re: [dpdk-dev] [RFC v6 0/6] lib/ring: APIs to support custom element size 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, On Sun, Oct 20, 2019 at 07:22:54PM -0500, Honnappa Nagarahalli wrote: > The current rte_ring hard-codes the type of the ring element to 'void *', > hence the size of the element is hard-coded to 32b/64b. Since the ring > element type is not an input to rte_ring APIs, it results in couple > of issues: > > 1) If an application requires to store an element which is not 64b, it > needs to write its own ring APIs similar to rte_event_ring APIs. This > creates additional burden on the programmers, who end up making > work-arounds and often waste memory. > 2) If there are multiple libraries that store elements of the same > type, currently they would have to write their own rte_ring APIs. This > results in code duplication. > > This patch adds new APIs to support configurable ring element size. > The APIs support custom element sizes by allowing to define the ring > element to be a multiple of 32b. > > The aim is to achieve same performance as the existing ring > implementation. The patch adds same performance tests that are run > for existing APIs. This allows for performance comparison. > > I also tested with memcpy. x86 shows significant improvements on bulk > and burst tests. On the Arm platform, I used, there is a drop of > 4% to 6% in few tests. May be this is something that we can explore > later. > > Note that this version skips changes to other libraries as I would > like to get an agreement on the implementation from the community. > They will be added once there is agreement on the rte_ring changes. > > v6 > - Labelled as RFC to indicate the better status > - Added unit tests to test the rte_ring_xxx_elem APIs > - Corrected 'macro based partial memcpy' (5/6) patch > - Added Konstantin's method after correction (6/6) > - Check Patch shows significant warnings and errors mainly due > copying code from existing test cases. None of them are harmful. > I will fix them once we have an agreement. > > v5 > - Use memcpy for chunks of 32B (Konstantin). > - Both 'ring_perf_autotest' and 'ring_perf_elem_autotest' are available > to compare the results easily. > - Copying without memcpy is also available in 1/3, if anyone wants to > experiment on their platform. > - Added other platform owners to test on their respective platforms. > > v4 > - Few fixes after more performance testing > > v3 > - Removed macro-fest and used inline functions > (Stephen, Bruce) > > v2 > - Change Event Ring implementation to use ring templates > (Jerin, Pavan) > > Honnappa Nagarahalli (6): > test/ring: use division for cycle count calculation > lib/ring: apis to support configurable element size > test/ring: add functional tests for configurable element size ring > test/ring: add perf tests for configurable element size ring > lib/ring: copy ring elements using memcpy partially > lib/ring: improved copy function to copy ring elements > > app/test/Makefile | 2 + > app/test/meson.build | 2 + > app/test/test_ring_elem.c | 859 +++++++++++++++++++++++++++ > app/test/test_ring_perf.c | 22 +- > app/test/test_ring_perf_elem.c | 419 +++++++++++++ > lib/librte_ring/Makefile | 3 +- > lib/librte_ring/meson.build | 4 + > lib/librte_ring/rte_ring.c | 34 +- > lib/librte_ring/rte_ring.h | 1 + > lib/librte_ring/rte_ring_elem.h | 818 +++++++++++++++++++++++++ > lib/librte_ring/rte_ring_version.map | 2 + > 11 files changed, 2147 insertions(+), 19 deletions(-) > create mode 100644 app/test/test_ring_elem.c > create mode 100644 app/test/test_ring_perf_elem.c > create mode 100644 lib/librte_ring/rte_ring_elem.h Sorry, I come a day after the fair. I have only few comments on the shape (I'll reply to individual patches). On the substance, it looks good to me. I also feel this version is much better than the template-based versions. Thanks Olivier