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 BBCE9A0543; Sun, 6 Nov 2022 07:58:02 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5282040691; Sun, 6 Nov 2022 07:58:02 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 4E1214003C for ; Sun, 6 Nov 2022 07:58:01 +0100 (CET) X-MimeOLE: Produced By Microsoft Exchange V6.5 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: [RFC] mempool: zero-copy cache put bulk Date: Sun, 6 Nov 2022 07:57:58 +0100 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D8748A@smartserver.smartshare.dk> In-Reply-To: X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [RFC] mempool: zero-copy cache put bulk Thread-Index: AdjxHCA/ROrKGJlPRZqF8NgKVJugtwAS1RvwABAiw0A= References: <98CBD80474FA8B44BF855DF32C47DC35D87489@smartserver.smartshare.dk> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Honnappa Nagarahalli" , , , , "Kamalakshitha Aligeri" Cc: "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 > From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli@arm.com] > Sent: Sunday, 6 November 2022 00.11 >=20 > + Akshitha, she is working on similar patch >=20 > Few comments inline >=20 > > From: Morten Br=F8rup > > Sent: Saturday, November 5, 2022 8:40 AM > > > > Zero-copy access to the mempool cache is beneficial for PMD > performance, > > and must be provided by the mempool library to fix [Bug 1052] = without > a > > performance regression. > > > > [Bug 1052]: https://bugs.dpdk.org/show_bug.cgi?id=3D1052 > > > > > > This RFC offers a conceptual zero-copy put function, where the > application > > promises to store some objects, and in return gets an address where > to store > > them. > > > > I would like some early feedback. > > > > Notes: > > * Allowing the 'cache' parameter to be NULL, and getting it from the > > mempool instead, was inspired by rte_mempool_cache_flush(). > I am not sure why the 'cache' parameter is required for this API. This > API should take the mem pool as the parameter. >=20 > We have based our API on 'rte_mempool_do_generic_put' and removed the > 'cache' parameter. I thoroughly considered omitting the 'cache' parameter, but included it = for two reasons: 1. The function is a "mempool cache" function (i.e. primarily working on = the mempool cache), not a "mempool" function. So it is appropriate to have a pointer directly to the structure it is = working on. Following this through, I also made 'cache' the first = parameter and 'mp' the second, like in rte_mempool_cache_flush(). 2. In most cases, the function only accesses the mempool structure in = order to get the cache pointer. Skipping this step improves performance. And since the cache is created along with the mempool itself (and thus = never changes for a mempool), it would be safe for the PMD to store the = 'cache' pointer along with the 'mp' pointer in the PMD's queue = structure. E.g. in the i40e PMD the i40e_rx_queue structure could include a "struct = rte_mempool_cache *cache" field, which could be used i40e_rxq_rearm() = [1] instead of "cache =3D rte_mempool_default_cache(rxq->mp, = rte_lcore_id())". [1] = https://elixir.bootlin.com/dpdk/v22.11-rc2/source/drivers/net/i40e/i40e_r= xtx_vec_avx512.c#L31 > This new API, on success, returns the pointer to > memory where the objects are copied. On failure it returns NULL and = the > caller has to call 'rte_mempool_ops_enqueue_bulk'. Alternatively, the > new API could do this as well and PMD does not need to do anything if > it gets a NULL pointer. Yes, we agree about these two details: 1. The function should return a pointer, not an integer. It would be a waste to use a another CPU register to convey a = success/error integer value, when the success/failure information is = just as easily conveyed by the pointer return value (non-NULL/NULL), and = rte_errno for various error values in the unlikely cases. 2. The function should leave it up to the PMD what to do if direct = access to the cache is unavailable. >=20 > We should think about providing similar API on the RX side to keep it > symmetric. I sent an RFC for that too: http://inbox.dpdk.org/dev/98CBD80474FA8B44BF855DF32C47DC35D87488@smartser= ver.smartshare.dk/T/#u >=20 > > * Asserting that the 'mp' parameter is not NULL is not done by other > > functions, so I omitted it here too. > > > > NB: Please ignore formatting. Also, this code has not even been > compile > > tested. > We are little bit ahead, tested the changes with i40e PF PMD, wrote > unit test cases, going through internal review, will send out RFC on > Monday Sounds good. Looking forward to review. >=20 > > > > /** > > * Promise to put objects in a mempool via zero-copy access to a > user-owned > > mempool cache. > > * > > * @param cache > > * A pointer to the mempool cache. > > * @param mp > > * A pointer to the mempool. > > * @param n > > * The number of objects to be put in the mempool cache. > > * @return > > * The pointer to where to put the objects in the mempool cache. > > * NULL on error > > * with rte_errno set appropriately. > > */ > > static __rte_always_inline void * > > rte_mempool_cache_put_bulk_promise(struct rte_mempool_cache *cache, > > struct rte_mempool *mp, > > unsigned int n) > > { > > void **cache_objs; > > > > if (cache =3D=3D NULL) > > cache =3D rte_mempool_default_cache(mp, rte_lcore_id()); > > if (cache =3D=3D NULL) { > > rte_errno =3D EINVAL; > > return NULL; > > } > > > > rte_mempool_trace_cache_put_bulk_promise(cache, mp, n); > > > > /* The request itself is too big for the cache */ > > if (unlikely(n > cache->flushthresh)) { > > rte_errno =3D EINVAL; > > return NULL; > > } > > > > /* > > * The cache follows the following algorithm: > > * 1. If the objects cannot be added to the cache without > crossing > > * the flush threshold, flush the cache to the backend. > > * 2. Add the objects to the cache. > > */ > > > > if (cache->len + n <=3D cache->flushthresh) { > > cache_objs =3D &cache->objs[cache->len]; > > cache->len +=3D n; > > } else { > > cache_objs =3D &cache->objs[0]; > > rte_mempool_ops_enqueue_bulk(mp, cache_objs, cache->len); > > cache->len =3D n; > > } > > > > RTE_MEMPOOL_CACHE_STAT_ADD(cache, put_bulk, 1); > > RTE_MEMPOOL_CACHE_STAT_ADD(cache, put_objs, n); > > > > return cache_objs; > > } > > > > > > Med venlig hilsen / Kind regards, > > -Morten Br=F8rup > > >=20