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 18197A2EDC for ; Mon, 9 Sep 2019 15:04:26 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 109B91B203; Mon, 9 Sep 2019 15:04:25 +0200 (CEST) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 309F82BB5 for ; Mon, 9 Sep 2019 15:04:23 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 713EF30832DA; Mon, 9 Sep 2019 13:04:22 +0000 (UTC) Received: from dhcp-25.97.bos.redhat.com (unknown [10.18.25.35]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 5EB1560461; Mon, 9 Sep 2019 13:04:21 +0000 (UTC) From: Aaron Conole To: Honnappa Nagarahalli Cc: olivier.matz@6wind.com, yipeng1.wang@intel.com, sameh.gobriel@intel.com, bruce.richardson@intel.com, pablo.de.lara.guarch@intel.com, dev@dpdk.org, pbhagavatula@marvell.com, jerinj@marvell.com References: <20190828144614.25284-1-honnappa.nagarahalli@arm.com> <20190906190510.11146-1-honnappa.nagarahalli@arm.com> Date: Mon, 09 Sep 2019 09:04:20 -0400 In-Reply-To: <20190906190510.11146-1-honnappa.nagarahalli@arm.com> (Honnappa Nagarahalli's message of "Fri, 6 Sep 2019 14:05:04 -0500") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.44]); Mon, 09 Sep 2019 13:04:22 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH v2 0/6] lib/ring: templates 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" Honnappa Nagarahalli writes: > 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 consists of several parts: > 1) New APIs to support configurable ring element size > These will help reduce code duplication in the templates. I think these > can be made internal (do not expose to DPDK applications, but expose to > DPDK libraries), feedback needed. > > 2) rte_ring templates > The templates provide an easy way to add new APIs for different ring > element types/sizes which can be used by multiple libraries. These > also allow for creating APIs to store elements of custom types > (for ex: a structure) > > The template needs 4 parameters: > a) RTE_RING_TMPLT_API_SUFFIX - This is used as a suffix to the > rte_ring APIs. > For ex: if RTE_RING_TMPLT_API_SUFFIX is '32b', the API name will be > rte_ring_create_32b > b) RTE_RING_TMPLT_ELEM_SIZE - Size of the ring element in bytes. > For ex: sizeof(uint32_t) > c) RTE_RING_TMPLT_ELEM_TYPE - Type of the ring element. > For ex: uint32_t. If a common ring library does not use a standard > data type, it should create its own type by defining a structure > with standard data type. For ex: for an elment size of 96b, one > could define a structure > > struct s_96b { > uint32_t a[3]; > } > The common library can use this structure to define > RTE_RING_TMPLT_ELEM_TYPE. > > The application using this common ring library should define its > element type as a union with the above structure. > > union app_element_type { > struct s_96b v; > struct app_element { > uint16_t a; > uint16_t b; > uint32_t c; > uint32_t d; > } > } > d) RTE_RING_TMPLT_EXPERIMENTAL - Indicates if the new APIs being defined > are experimental. Should be set to empty to remove the experimental > tag. > > The ring library consists of some APIs that are defined as inline > functions and some APIs that are non-inline functions. The non-inline > functions are in rte_ring_template.c. However, this file needs to be > included in other .c files. Any feedback on how to handle this is > appreciated. > > Note that the templates help create the APIs that are dependent on the > element size (for ex: rte_ring_create, enqueue/dequeue etc). Other APIs > that do NOT depend on the element size do not need to be part of the > template (for ex: rte_ring_dump, rte_ring_count, rte_ring_free_count > etc). > > 3) APIs for 32b ring element size > This uses the templates to create APIs to enqueue/dequeue elements of > size 32b. > > 4) rte_hash libray is changed to use 32b ring APIs > The 32b APIs are used in rte_hash library to store the free slot index > and free bucket index. > > 5) Event Dev changed to use ring templates > Event Dev defines its own 128b ring APIs using the templates. This helps > in keeping the 'struct rte_event' as is. If Event Dev has to use generic > 128b ring APIs, it requires 'struct rte_event' to change to > 'union rte_event' to include a generic data type such as '__int128_t'. > This breaks the API compatibility and results in large number of > changes. > With this change, the event rings are stored on rte_ring's tailq. > Event Dev specific ring list is NOT available. IMO, this does not have > any impact to the user. > > This patch results in following checkpatch issue: > WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned' > > However, this patch is following the rules in the existing code. Please > let me know if this needs to be fixed. > > v2 > - Change Event Ring implementation to use ring templates > (Jerin, Pavan) Since you'll likely be spinning a v3 (to switch off the macroization), this series seems to have some unit test failures: 24/82 DPDK:fast-tests / event_ring_autotest FAIL 0.12 s (exit status 255 or signal 127 SIGinvalid) --- command --- DPDK_TEST='event_ring_autotest' /home/travis/build/ovsrobot/dpdk/build/app/test/dpdk-test -l 0-1 --file-prefix=event_ring_autotest --- stdout --- EAL: Probing VFIO support... APP: HPET is not enabled, using TSC as default timer RTE>>event_ring_autotest RING: Requested number of elements is invalid, must be power of 2, and do not exceed the limit 2147483647 Test detected odd count Test detected NULL ring lookup RING: Requested number of elements is invalid, must be power of 2, and do not exceed the limit 2147483647 RING: Requested number of elements is invalid, must be power of 2, and do not exceed the limit 2147483647 Error, status after enqueue is unexpected Test Failed RTE>> --- stderr --- EAL: Detected 2 lcore(s) EAL: Detected 1 NUMA nodes EAL: Multi-process socket /var/run/dpdk/event_ring_autotest/mp_socket EAL: Selected IOVA mode 'PA' EAL: No available hugepages reported in hugepages-1048576kB ------- Please double check. Seems to only happen with clang/llvm. > Honnappa Nagarahalli (6): > lib/ring: apis to support configurable element size > lib/ring: add template to support different element sizes > tools/checkpatch: relax constraints on __rte_experimental > lib/ring: add ring APIs to support 32b ring elements > lib/hash: use ring with 32b element size to save memory > lib/eventdev: use ring templates for event rings > > devtools/checkpatches.sh | 11 +- > lib/librte_eventdev/Makefile | 2 + > lib/librte_eventdev/meson.build | 2 + > lib/librte_eventdev/rte_event_ring.c | 146 +--------- > lib/librte_eventdev/rte_event_ring.h | 41 +-- > lib/librte_eventdev/rte_event_ring_128b.c | 19 ++ > lib/librte_eventdev/rte_event_ring_128b.h | 44 +++ > lib/librte_hash/rte_cuckoo_hash.c | 55 ++-- > lib/librte_hash/rte_cuckoo_hash.h | 2 +- > lib/librte_ring/Makefile | 9 +- > lib/librte_ring/meson.build | 11 +- > lib/librte_ring/rte_ring.c | 34 ++- > lib/librte_ring/rte_ring.h | 72 +++++ > lib/librte_ring/rte_ring_32.c | 19 ++ > lib/librte_ring/rte_ring_32.h | 36 +++ > lib/librte_ring/rte_ring_template.c | 46 +++ > lib/librte_ring/rte_ring_template.h | 330 ++++++++++++++++++++++ > lib/librte_ring/rte_ring_version.map | 4 + > 18 files changed, 660 insertions(+), 223 deletions(-) > create mode 100644 lib/librte_eventdev/rte_event_ring_128b.c > create mode 100644 lib/librte_eventdev/rte_event_ring_128b.h > create mode 100644 lib/librte_ring/rte_ring_32.c > create mode 100644 lib/librte_ring/rte_ring_32.h > create mode 100644 lib/librte_ring/rte_ring_template.c > create mode 100644 lib/librte_ring/rte_ring_template.h