| Message ID | 20200116052511.8557-1-honnappa.nagarahalli@arm.com (mailing list archive) |
|---|---|
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id E6CF0A0352; Thu, 16 Jan 2020 06:25:24 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 442CB1BFE3; Thu, 16 Jan 2020 06:25:24 +0100 (CET) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by dpdk.org (Postfix) with ESMTP id B8B9C1BFE2 for <dev@dpdk.org>; Thu, 16 Jan 2020 06:25:22 +0100 (CET) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 137DB328; Wed, 15 Jan 2020 21:25:22 -0800 (PST) Received: from qc2400f-1.austin.arm.com (qc2400f-1.austin.arm.com [10.118.14.48]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id EED843F534; Wed, 15 Jan 2020 21:25:21 -0800 (PST) From: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> To: olivier.matz@6wind.com, sthemmin@microsoft.com, jerinj@marvell.com, bruce.richardson@intel.com, david.marchand@redhat.com, pbhagavatula@marvell.com, konstantin.ananyev@intel.com, yipeng1.wang@intel.com, honnappa.nagarahalli@arm.com Cc: dev@dpdk.org, dharmik.thakkar@arm.com, ruifeng.wang@arm.com, gavin.hu@arm.com, nd@arm.com Date: Wed, 15 Jan 2020 23:25:05 -0600 Message-Id: <20200116052511.8557-1-honnappa.nagarahalli@arm.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20190906190510.11146-1-honnappa.nagarahalli@arm.com> References: <20190906190510.11146-1-honnappa.nagarahalli@arm.com> Subject: [dpdk-dev] [PATCH v9 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 <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
| Series |
lib/ring: APIs to support custom element size
|
|
Message
Honnappa Nagarahalli
Jan. 16, 2020, 5:25 a.m. UTC
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. v9 - Split 'test_ring_burst_bulk_tests' test case into 4 smaller functions to address clang compilation time issue. - Addressed compilation failure in Intel CI in the hash changes. v8 - Changed the 128b copy elements inline function to use 'memcpy' to generate unaligned load/store instructions for x86. Generic copy function results in performance drop. (Konstantin) - Changed the API type #defines to be more clear (Konstantin) - Removed the code duplication in performance tests (Konstantin) - Fixed memory leak, changed test macros to inline functions (Konstantin) - Changed functional tests to test for 20B ring element. Fixed a bug in 32b element copy code for enqueue/dequeue(ring size needs to be normalized for 32b). - Squashed the functional and performance tests in their respective single commits. v7 - Merged the test cases to test both legacy APIs and rte_ring_xxx_elem APIs without code duplication (Konstantin, Olivier) - Performance test cases are merged as well (Konstantin, Olivier) - Macros to copy elements are converted into inline functions (Olivier) - Added back the changes to hash and event libraries 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 rte_ring_xxx_elem APIs test/ring: modify perf test cases to use rte_ring_xxx_elem APIs lib/hash: use ring with 32b element size to save memory lib/eventdev: use custom element size ring for event rings app/test/test_ring.c | 1342 +++++++++++++------------- app/test/test_ring.h | 187 ++++ app/test/test_ring_perf.c | 452 +++++---- lib/librte_eventdev/rte_event_ring.c | 147 +-- lib/librte_eventdev/rte_event_ring.h | 45 +- lib/librte_hash/rte_cuckoo_hash.c | 94 +- lib/librte_hash/rte_cuckoo_hash.h | 2 +- lib/librte_ring/Makefile | 3 +- lib/librte_ring/meson.build | 4 + lib/librte_ring/rte_ring.c | 41 +- lib/librte_ring/rte_ring.h | 1 + lib/librte_ring/rte_ring_elem.h | 1003 +++++++++++++++++++ lib/librte_ring/rte_ring_version.map | 2 + 13 files changed, 2242 insertions(+), 1081 deletions(-) create mode 100644 app/test/test_ring.h create mode 100644 lib/librte_ring/rte_ring_elem.h
Comments
I see that the none of the CIs (except Travis) have run on this patch. Intel CI has reported a compilation error and I fixed it in this version. Does anyone know if/when the CI will run on the patches? Thanks, Honnappa > -----Original Message----- > From: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> > Sent: Wednesday, January 15, 2020 11:25 PM > To: olivier.matz@6wind.com; sthemmin@microsoft.com; jerinj@marvell.com; > bruce.richardson@intel.com; david.marchand@redhat.com; > pbhagavatula@marvell.com; konstantin.ananyev@intel.com; > yipeng1.wang@intel.com; Honnappa Nagarahalli > <Honnappa.Nagarahalli@arm.com> > Cc: dev@dpdk.org; Dharmik Thakkar <Dharmik.Thakkar@arm.com>; Ruifeng > Wang <Ruifeng.Wang@arm.com>; Gavin Hu <Gavin.Hu@arm.com>; nd > <nd@arm.com> > Subject: [PATCH v9 0/6] lib/ring: APIs to support custom element size > > 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. > > v9 > - Split 'test_ring_burst_bulk_tests' test case into 4 smaller > functions to address clang compilation time issue. > - Addressed compilation failure in Intel CI in the hash changes. > > v8 > - Changed the 128b copy elements inline function to use 'memcpy' > to generate unaligned load/store instructions for x86. Generic > copy function results in performance drop. (Konstantin) > - Changed the API type #defines to be more clear (Konstantin) > - Removed the code duplication in performance tests (Konstantin) > - Fixed memory leak, changed test macros to inline functions (Konstantin) > - Changed functional tests to test for 20B ring element. Fixed > a bug in 32b element copy code for enqueue/dequeue(ring size > needs to be normalized for 32b). > - Squashed the functional and performance tests in their > respective single commits. > > v7 > - Merged the test cases to test both legacy APIs and > rte_ring_xxx_elem APIs without code duplication (Konstantin, Olivier) > - Performance test cases are merged as well (Konstantin, Olivier) > - Macros to copy elements are converted into inline functions (Olivier) > - Added back the changes to hash and event libraries > > 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 rte_ring_xxx_elem APIs > test/ring: modify perf test cases to use rte_ring_xxx_elem APIs > lib/hash: use ring with 32b element size to save memory > lib/eventdev: use custom element size ring for event rings > > app/test/test_ring.c | 1342 +++++++++++++------------- > app/test/test_ring.h | 187 ++++ > app/test/test_ring_perf.c | 452 +++++---- > lib/librte_eventdev/rte_event_ring.c | 147 +-- > lib/librte_eventdev/rte_event_ring.h | 45 +- > lib/librte_hash/rte_cuckoo_hash.c | 94 +- > lib/librte_hash/rte_cuckoo_hash.h | 2 +- > lib/librte_ring/Makefile | 3 +- > lib/librte_ring/meson.build | 4 + > lib/librte_ring/rte_ring.c | 41 +- > lib/librte_ring/rte_ring.h | 1 + > lib/librte_ring/rte_ring_elem.h | 1003 +++++++++++++++++++ > lib/librte_ring/rte_ring_version.map | 2 + > 13 files changed, 2242 insertions(+), 1081 deletions(-) create mode 100644 > app/test/test_ring.h create mode 100644 lib/librte_ring/rte_ring_elem.h > > -- > 2.17.1
On Thu, Jan 16, 2020 at 5:36 PM Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> wrote: > > I see that the none of the CIs (except Travis) have run on this patch. Intel CI has reported a compilation error and I fixed it in this version. Does anyone know if/when the CI will run on the patches? - Pushed the series https://patchwork.dpdk.org/project/dpdk/list/?series=8147 to a branch of mine for checks. Travis reports: " [2155/2156] Compiling C object 'app/te...st@@dpdk-test@exe/test_ring_perf.c.o'. No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself. Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#build-times-out-because-no-output-was-received The build has been terminated " I see you discussed this already with Aaron, did I miss something? - Besides, I have no ack/review from the hash and eventdev maintainers.
On Fri, Jan 17, 2020 at 5:44 PM David Marchand <david.marchand@redhat.com> wrote: > > On Thu, Jan 16, 2020 at 5:36 PM Honnappa Nagarahalli > <Honnappa.Nagarahalli@arm.com> wrote: > > > > I see that the none of the CIs (except Travis) have run on this patch. Intel CI has reported a compilation error and I fixed it in this version. Does anyone know if/when the CI will run on the patches? > > - Pushed the series > https://patchwork.dpdk.org/project/dpdk/list/?series=8147 to a branch > of mine for checks. > Travis reports: > " > [2155/2156] Compiling C object 'app/te...st@@dpdk-test@exe/test_ring_perf.c.o'. > > No output has been received in the last 10m0s, this potentially > indicates a stalled build or something wrong with the build itself. > > Check the details on how to adjust your build configuration on: > https://docs.travis-ci.com/user/common-build-problems/#build-times-out-because-no-output-was-received > > The build has been terminated > " > > I see you discussed this already with Aaron, did I miss something? > > > - Besides, I have no ack/review from the hash and eventdev maintainers. + Bruce, Harry, Mattias Even though event ring added in the eventdev common code, it's been used only by SW eventdev drivers. So adding evendev ring author and SW driver maintainers for review. > > > -- > David Marchand >
<snip> > > On Thu, Jan 16, 2020 at 5:36 PM Honnappa Nagarahalli > <Honnappa.Nagarahalli@arm.com> wrote: > > > > I see that the none of the CIs (except Travis) have run on this patch. Intel CI > has reported a compilation error and I fixed it in this version. Does anyone > know if/when the CI will run on the patches? > > - Pushed the series > https://patchwork.dpdk.org/project/dpdk/list/?series=8147 to a branch of > mine for checks. > Travis reports: > " > [2155/2156] Compiling C object 'app/te...st@@dpdk- > test@exe/test_ring_perf.c.o'. > > No output has been received in the last 10m0s, this potentially indicates a > stalled build or something wrong with the build itself. > > Check the details on how to adjust your build configuration on: > https://docs.travis-ci.com/user/common-build-problems/#build-times-out- > because-no-output-was-received > > The build has been terminated > " > > I see you discussed this already with Aaron, did I miss something? Aaron has tested it with clang-8 and said it does not show the issue. I am not sure if he tested this on Travis CI. I have tested it on clang-9 and it does not show any issues. I have modified the test cases to take much lesser time for Clang-7. Aaron, please let me know if you want to upgrade Travis CI? > > > - Besides, I have no ack/review from the hash and eventdev maintainers. Yipeng, Jerin can you please review your parts? > > > -- > David Marchand
<snip> > > > > > On Thu, Jan 16, 2020 at 5:36 PM Honnappa Nagarahalli > > <Honnappa.Nagarahalli@arm.com> wrote: > > > > > > I see that the none of the CIs (except Travis) have run on this > > > patch. Intel CI > > has reported a compilation error and I fixed it in this version. Does > > anyone know if/when the CI will run on the patches? > > > > - Pushed the series > > https://patchwork.dpdk.org/project/dpdk/list/?series=8147 to a branch > > of mine for checks. > > Travis reports: > > " > > [2155/2156] Compiling C object 'app/te...st@@dpdk- > > test@exe/test_ring_perf.c.o'. > > > > No output has been received in the last 10m0s, this potentially > > indicates a stalled build or something wrong with the build itself. > > > > Check the details on how to adjust your build configuration on: > > https://docs.travis-ci.com/user/common-build-problems/#build-times-out > > - > > because-no-output-was-received > > > > The build has been terminated > > " > > > > I see you discussed this already with Aaron, did I miss something? > Aaron has tested it with clang-8 and said it does not show the issue. I am not > sure if he tested this on Travis CI. > I have tested it on clang-9 and it does not show any issues. I have modified > the test cases to take much lesser time for Clang-7. > > Aaron, please let me know if you want to upgrade Travis CI? BTW, intel compilation CI is passing now with v9 including clang. > > > > > > - Besides, I have no ack/review from the hash and eventdev maintainers. > Yipeng, Jerin can you please review your parts? > > > > > > > -- > > David Marchand >
On Fri, Jan 17, 2020 at 3:29 PM Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> wrote: > > - Pushed the series > > https://patchwork.dpdk.org/project/dpdk/list/?series=8147 to a branch of > > mine for checks. > > Travis reports: > > " > > [2155/2156] Compiling C object 'app/te...st@@dpdk- > > test@exe/test_ring_perf.c.o'. > > > > No output has been received in the last 10m0s, this potentially indicates a > > stalled build or something wrong with the build itself. > > > > Check the details on how to adjust your build configuration on: > > https://docs.travis-ci.com/user/common-build-problems/#build-times-out- > > because-no-output-was-received > > > > The build has been terminated > > " > > > > I see you discussed this already with Aaron, did I miss something? > Aaron has tested it with clang-8 and said it does not show the issue. I am not sure if he tested this on Travis CI. > I have tested it on clang-9 and it does not show any issues. I have modified the test cases to take much lesser time for Clang-7. The problem is seen with clang 7 in Ubuntu 16.04. https://travis-ci.com/david-marchand/dpdk/jobs/276790838 > > Aaron, please let me know if you want to upgrade Travis CI? Do you mean upgrading Travis to hide this issue?
<snip> > On Fri, Jan 17, 2020 at 3:29 PM Honnappa Nagarahalli > <Honnappa.Nagarahalli@arm.com> wrote: > > > - Pushed the series > > > https://patchwork.dpdk.org/project/dpdk/list/?series=8147 to a > > > branch of mine for checks. > > > Travis reports: > > > " > > > [2155/2156] Compiling C object 'app/te...st@@dpdk- > > > test@exe/test_ring_perf.c.o'. > > > > > > No output has been received in the last 10m0s, this potentially > > > indicates a stalled build or something wrong with the build itself. > > > > > > Check the details on how to adjust your build configuration on: > > > https://docs.travis-ci.com/user/common-build-problems/#build-times-o > > > ut- > > > because-no-output-was-received > > > > > > The build has been terminated > > > " > > > > > > I see you discussed this already with Aaron, did I miss something? > > Aaron has tested it with clang-8 and said it does not show the issue. I am not > sure if he tested this on Travis CI. > > I have tested it on clang-9 and it does not show any issues. I have modified > the test cases to take much lesser time for Clang-7. > > The problem is seen with clang 7 in Ubuntu 16.04. > https://travis-ci.com/david-marchand/dpdk/jobs/276790838 Agree. I have split the test cases into multiple functions to address clang 7 compile time. But it does not suffice for Travis CI. > > > > > Aaron, please let me know if you want to upgrade Travis CI? > > Do you mean upgrading Travis to hide this issue? Yes, upgrading to use clang-8 or clang-9. > > > -- > David Marchand
On 2020-01-17 14:34, Jerin Jacob wrote: > On Fri, Jan 17, 2020 at 5:44 PM David Marchand > <david.marchand@redhat.com> wrote: >> On Thu, Jan 16, 2020 at 5:36 PM Honnappa Nagarahalli >> <Honnappa.Nagarahalli@arm.com> wrote: >>> I see that the none of the CIs (except Travis) have run on this patch. Intel CI has reported a compilation error and I fixed it in this version. Does anyone know if/when the CI will run on the patches? >> - Pushed the series >> https://patchwork.dpdk.org/project/dpdk/list/?series=8147 to a branch >> of mine for checks. >> Travis reports: >> " >> [2155/2156] Compiling C object 'app/te...st@@dpdk-test@exe/test_ring_perf.c.o'. >> >> No output has been received in the last 10m0s, this potentially >> indicates a stalled build or something wrong with the build itself. >> >> Check the details on how to adjust your build configuration on: >> https://docs.travis-ci.com/user/common-build-problems/#build-times-out-because-no-output-was-received >> >> The build has been terminated >> " >> >> I see you discussed this already with Aaron, did I miss something? >> >> >> - Besides, I have no ack/review from the hash and eventdev maintainers. > + Bruce, Harry, Mattias > > Even though event ring added in the eventdev common code, it's been > used only by SW eventdev drivers. > So adding evendev ring author and SW driver maintainers for review. I endorse this change, although I hadn't have time to review the code. DSW throughput increases ~5% on x86_64 after applying this patchset, so the goal of maintaining legacy performance seems to have been met (and exceeded). I will look into using these new custom-sized rings for the DSW control rings (which uses regular DPDK void-pointer rings today).
On Wed, Jan 15, 2020 at 11:25:05PM -0600, 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. > This is a nice improvement to the ring library, thanks! It looks globally good to me, few comments as reply to individual patches. Olivier