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 A638EA034C; Fri, 21 Jan 2022 08:36:26 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 32C174272D; Fri, 21 Jan 2022 08:36:26 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 27A8140042 for ; Fri, 21 Jan 2022 08:36:25 +0100 (CET) Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [PATCH v2 1/1] mempool: implement index-based per core cache X-MimeOLE: Produced By Microsoft Exchange V6.5 Date: Fri, 21 Jan 2022 08:36:23 +0100 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D86E18@smartserver.smartshare.dk> In-Reply-To: X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH v2 1/1] mempool: implement index-based per core cache Thread-Index: AQHYCD+SYdKHeDO5vkio0T5c8Zbcg6xrnQAAgACjtfCAANnXQA== References: <20211224225923.806498-1-dharmik.thakkar@arm.com> <20220113053630.886638-1-dharmik.thakkar@arm.com> <20220113053630.886638-2-dharmik.thakkar@arm.com> <98CBD80474FA8B44BF855DF32C47DC35D86E14@smartserver.smartshare.dk> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Honnappa Nagarahalli" , "Dharmik Thakkar" , "Olivier Matz" , "Andrew Rybchenko" , "Ray Kinsella" Cc: , "nd" , "Ruifeng Wang" , "Beilei Xing" , "nd" 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 +Ray Kinsella, ABI Policy maintainer > From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli@arm.com] > Sent: Friday, 21 January 2022 07.01 >=20 > > > > +CC Beilei as i40e maintainer > > > > > From: Dharmik Thakkar [mailto:dharmik.thakkar@arm.com] > > > Sent: Thursday, 13 January 2022 06.37 > > > > > > Current mempool per core cache implementation stores pointers to > mbufs > > > On 64b architectures, each pointer consumes 8B This patch replaces > it > > > with index-based implementation, where in each buffer is addressed > by > > > (pool base address + index) It reduces the amount of memory/cache > > > required for per core cache > > > > > > L3Fwd performance testing reveals minor improvements in the cache > > > performance (L1 and L2 misses reduced by 0.60%) with no change in > > > throughput > > > > > > Suggested-by: Honnappa Nagarahalli > > > Signed-off-by: Dharmik Thakkar > > > Reviewed-by: Ruifeng Wang > > > --- > > > lib/mempool/rte_mempool.h | 150 > +++++++++++++++++++++++++- > > > lib/mempool/rte_mempool_ops_default.c | 7 ++ > > > 2 files changed, 156 insertions(+), 1 deletion(-) > > > > > > diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h > > > index 1e7a3c15273c..f2403fbc97a7 100644 > > > --- a/lib/mempool/rte_mempool.h > > > +++ b/lib/mempool/rte_mempool.h > > > @@ -50,6 +50,10 @@ > > > #include > > > #include > > > > > > +#ifdef RTE_MEMPOOL_INDEX_BASED_LCORE_CACHE > > > +#include > > > +#endif > > > + > > > #include "rte_mempool_trace_fp.h" > > > > > > #ifdef __cplusplus > > > @@ -239,6 +243,9 @@ struct rte_mempool { > > > int32_t ops_index; > > > > > > struct rte_mempool_cache *local_cache; /**< Per-lcore local = cache > */ > > > +#ifdef RTE_MEMPOOL_INDEX_BASED_LCORE_CACHE > > > + void *pool_base_value; /**< Base value to calculate indices */ > > > +#endif > > > > > > uint32_t populated_size; /**< Number of populated > > > objects. */ > > > struct rte_mempool_objhdr_list elt_list; /**< List of objects in > > > pool */ @@ -1314,7 +1321,22 @@ rte_mempool_cache_flush(struct > > > rte_mempool_cache *cache, > > > if (cache =3D=3D NULL || cache->len =3D=3D 0) > > > return; > > > rte_mempool_trace_cache_flush(cache, mp); > > > + > > > +#ifdef RTE_MEMPOOL_INDEX_BASED_LCORE_CACHE > > > + unsigned int i; > > > + unsigned int cache_len =3D cache->len; > > > + void *obj_table[RTE_MEMPOOL_CACHE_MAX_SIZE * 3]; > > > + void *base_value =3D mp->pool_base_value; > > > + uint32_t *cache_objs =3D (uint32_t *) cache->objs; > > > > Hi Dharmik and Honnappa, > > > > The essence of this patch is based on recasting the type of the objs > field in the > > rte_mempool_cache structure from an array of pointers to an array of > > uint32_t. > > > > However, this effectively breaks the ABI, because the > rte_mempool_cache > > structure is public and part of the API. > The patch does not change the public structure, the new member is = under > compile time flag, not sure how it breaks the ABI. >=20 > > > > Some drivers [1] even bypass the mempool API and access the > > rte_mempool_cache structure directly, assuming that the objs array = in > the > > cache is an array of pointers. So you cannot recast the fields in = the > > rte_mempool_cache structure the way this patch requires. > IMO, those drivers are at fault. The mempool cache structure is public > only because the APIs are inline. We should still maintain modularity > and not use the members of structures belonging to another library > directly. A similar effort involving rte_ring was not accepted = sometime > back [1] >=20 > [1] > = http://inbox.dpdk.org/dev/DBAPR08MB5814907968595EE56F5E20A798390@DBAPR0 > 8MB5814.eurprd08.prod.outlook.com/ >=20 > > > > Although I do consider bypassing an API's accessor functions > "spaghetti > > code", this driver's behavior is formally acceptable as long as the > > rte_mempool_cache structure is not marked as internal. > > > > I really liked your idea of using indexes instead of pointers, so = I'm > very sorry to > > shoot it down. :-( > > > > [1]: E.g. the Intel i40e PMD, > > > = http://code.dpdk.org/dpdk/latest/source/drivers/net/i40e/i40e_rxtx_vec_ > avx > > 512.c#L25 > It is possible to throw an error when this feature is enabled in this > file. Alternatively, this PMD could implement the code for index based > mempool. >=20 I agree with both your points, Honnappa. The ABI remains intact, and only changes when this feature is enabled at = compile time. In addition to your suggestions, I propose that the patch modifies the = objs type in the mempool cache structure itself, instead of type casting = it through an access variable. This should throw an error when compiling = an application that accesses it as a pointer array instead of a uint32_t = array - like the affected Intel PMDs. The updated objs field in the mempool cache structure should have the = same size when compiled as the original objs field, so this feature = doesn't change anything else in the ABI, only the type of the mempool = cache objects. Also, the description of the feature should stress that applications = accessing the cache objects directly will fail miserably.