Re: [PATCH v8] mempool cache: add zero-copy get and put functions

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 2/13/23 13:25, Morten Brørup wrote:
>> From: Olivier Matz [mailto:olivier.matz@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. :-)

I've suggested to use minus here to ensure that we handle
extremely big 'n' value here correctly (which would result in
addition overflow).

> 
> [...]
> 
>>>>>> +/**
>>>>>> + * @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.

No strong opinion, but I'd wait for description improvements.
It is very important to have good description from the very
beginning.
I'll try to find time this week to help, but can't promise.
May be it is already late...

> 
> [...]
> 
>>> 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?

Since 'cache' is mandatory here (it cannot be NULL), I agree
that it is 'mempool cache' API, not 'mempool API'.

> 
>>
>> - 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".

I have no strong opinion here. Yes, "bulk" is nice for
consistency, but IMHO not strictly required since it
makes function name longer and there is no value
in single-object version of these functions.

> 
>>
>> 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.)

prepare -> reserve?

However, in the case of get we really do get. When function
returns corresponding objects are fully got from mempool.
Yes, in the case of put we need to copy object pointers
into provided space. However, we don't need to call any
mempool (cache) API to commit it. Plus API naming symmetry.
So, I'd *not* add prepare/reserve to function names.

> 
>>
>>>
>>> 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.
>