> From: Olivier Matz [mailto:olivier.m...@6wind.com]
> Sent: Monday, 13 February 2023 10.37
>
> Hello,
>
> 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!
>
> On Mon, Feb 13, 2023 at 04:29:51AM +0000, Honnappa Nagarahalli wrote:
> > <snip>
> >
> > > > > +/**
> > > > > + * @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 != NULL);
> > > > > + RTE_ASSERT(mp != NULL);
> > > > > +
> > > > > + if (n <= cache->flushthresh - cache->len) {
>
> The previous code was doing this test instead:
>
> if (cache->len + n <= cache->flushthresh)
>
> I know there is an invariant asserting that cache->len <= 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.
>
> 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.
>
> 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, ...
>
> Also, we should say that once this function is called, the user has
> to copy the objects to the cache.
>
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.
>
> I'm not really convinced by the API names too. Again, sorry, I know
> this
> comment arrives after the battle.
>
> Your proposal is:
>
> /* Zero-copy put objects in a mempool cache backed by the specified
> mempool. */
> rte_mempool_cache_zc_put_bulk(cache, mp, n)
>
> /* Zero-copy get objects from a mempool cache backed by the specified
> mempool. */
> rte_mempool_cache_zc_get_bulk(cache, mp, n)
>
> Here are some observations:
>
> - 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.
>
> - 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.
>
> - 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?
>
> - 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".
>
> What do you think about these API below?
>
> 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.)
>
> >
> > Also, what is the use case for the 'rewind' API?
>
> +1
>
> I have the same feeling that rewind() is not required now. It can be
> added later if we find a use-case.
>
> 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.
