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 CD6E241C87; Mon, 13 Feb 2023 11:25:04 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 625D5410D1; Mon, 13 Feb 2023 11:25:04 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id DA34840ED8 for ; Mon, 13 Feb 2023 11:25:02 +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: [PATCH v8] mempool cache: add zero-copy get and put functions Date: Mon, 13 Feb 2023 11:25:00 +0100 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D87734@smartserver.smartshare.dk> In-Reply-To: X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH v8] mempool cache: add zero-copy get and put functions Thread-Index: Adk/jr14ONk7Z3MSQ/6UDEE45G5t6gAAe82A References: <98CBD80474FA8B44BF855DF32C47DC35D87488@smartserver.smartshare.dk> <20230209145833.129986-1-mb@smartsharesystems.com> <98CBD80474FA8B44BF855DF32C47DC35D87732@smartserver.smartshare.dk> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Olivier Matz" Cc: , "Kamalakshitha Aligeri" , , , , "nd" , , "Honnappa Nagarahalli" 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: Olivier Matz [mailto:olivier.matz@6wind.com] > Sent: Monday, 13 February 2023 10.37 >=20 > Hello, >=20 > Thank you for this work, and sorry for the late feedback too. Better late than never. And it's a core library, so important to get it = right! >=20 > On Mon, Feb 13, 2023 at 04:29:51AM +0000, Honnappa Nagarahalli wrote: > > > > > > > > > +/** > > > > > + * @internal used by rte_mempool_cache_zc_put_bulk() and > > > > > rte_mempool_do_generic_put(). > > > > > + * > > > > > + * Zero-copy put objects in a mempool cache backed by the > specified > > > > > mempool. > > > > > + * > > > > > + * @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 if the request itself is too big for the cache, = i.e. > > > > > + * exceeds the cache flush threshold. > > > > > + */ > > > > > +static __rte_always_inline void ** > > > > > +__rte_mempool_cache_zc_put_bulk(struct rte_mempool_cache > *cache, > > > > > + struct rte_mempool *mp, > > > > > + unsigned int n) > > > > > +{ > > > > > + void **cache_objs; > > > > > + > > > > > + RTE_ASSERT(cache !=3D NULL); > > > > > + RTE_ASSERT(mp !=3D NULL); > > > > > + > > > > > + if (n <=3D cache->flushthresh - cache->len) { >=20 > The previous code was doing this test instead: >=20 > if (cache->len + n <=3D cache->flushthresh) >=20 > I know there is an invariant asserting that cache->len <=3D cache- > >threshold, > so there is no real issue, but I'll tend to say that it is a good > practise > to avoid substractions on unsigned values to avoid the risk of > wrapping. >=20 > I also think the previous test was a bit more readable. I agree with you, but I didn't object to Andrew's recommendation of = changing it to this, so I did. I will change it back. Konstantin, I hope you don't mind. :-) [...] > > > > > +/** > > > > > + * @warning > > > > > + * @b EXPERIMENTAL: This API may change, or be removed, > without > > > > prior > > > > > notice. > > > > > + * > > > > > + * Zero-copy put objects in a mempool cache backed by the > specified > > > > > mempool. >=20 > I think we should document the differences and advantage of using this > function over the standard version, explaining which copy is avoided, > why it is faster, ... >=20 > Also, we should say that once this function is called, the user has > to copy the objects to the cache. >=20 I agree, the function descriptions could be more verbose. If we want to get this feature into DPDK now, we can postpone the = descriptions improvements to a later patch. [...] > > Earlier there was a discussion on the API name. > > IMO, we should keep the API names similar to those in ring library. > This would provide consistency across the libraries. > > There were some concerns expressed in PMD having to call 2 APIs. I = do > not think changing to 2 APIs will have any perf impact. >=20 > I'm not really convinced by the API names too. Again, sorry, I know > this > comment arrives after the battle. >=20 > Your proposal is: >=20 > /* Zero-copy put objects in a mempool cache backed by the specified > mempool. */ > rte_mempool_cache_zc_put_bulk(cache, mp, n) >=20 > /* Zero-copy get objects from a mempool cache backed by the specified > mempool. */ > rte_mempool_cache_zc_get_bulk(cache, mp, n) >=20 > Here are some observations: >=20 > - This was said in the discussion previously, but the functions do not > really get or put objects in the cache. Instead, they prepare the > cache (filling it or flushing it if needed) and update its length so > that the user can do the effective copy. Can be fixed by improving function descriptions. >=20 > - The "_cache" is superfluous for me: these functions do not deal more > with the cache than the non zero-copy version I have been thinking of these as "mempool cache" APIs. I don't mind getting rid of "_cache" in their names, if we agree that = they are "mempool" functions, instead of "mempool cache" functions. >=20 > - The order of the parameters is (cache, mp, n) while the other > functions > that take a mempool and a cache as parameters have the mp first (see > _generic versions). The order of the parameters was due to considering these as "mempool = cache" functions, so I followed the convention for an existing "mempool = cache" function: rte_mempool_cache_flush(struct rte_mempool_cache *cache, struct rte_mempool *mp); If we instead consider them as simple "mempool" functions, I agree with = you about the parameter ordering. So, what does the community think... Are these "mempool cache" = functions, or just "mempool" functions? >=20 > - The "_bulk" is indeed present on other functions, but not all (the > generic > version does not have it), I'm not sure it is absolutely required The mempool library offers both single-object and bulk functions, so the = function names must include "_bulk". >=20 > What do you think about these API below? >=20 > rte_mempool_prepare_zc_put(mp, n, cache) > rte_mempool_prepare_zc_get(mp, n, cache) I initially used "prepare" in the names, but since we don't have = accompanying "commit" functions, I decided against "prepare" to avoid = confusion. (Any SQL developer will probably agree with me on this.) >=20 > > > > Also, what is the use case for the 'rewind' API? >=20 > +1 >=20 > I have the same feeling that rewind() is not required now. It can be > added later if we find a use-case. >=20 > In case we want to keep it, I think we need to better specify in the > API > comments in which unique conditions the function can be called > (i.e. after a call to rte_mempool_prepare_zc_put() with the same = number > of objects, given no other operations were done on the mempool in > between). A call outside of these conditions has an undefined = behavior. Please refer to my answer to Honnappa on this topic.