On 2/13/23 13:25, Morten Brørup wrote:
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. :-)
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.
