RE: [RFC] mempool: zero-copy cache put bulk

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli@arm.com]
> Sent: Sunday, 6 November 2022 00.11
> 
> + Akshitha, she is working on similar patch
> 
> Few comments inline
> 
> > From: Morten Brørup <mb@smartsharesystems.com>
> > 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=1052
> >
> >
> > 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.
> 
> 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 = rte_mempool_default_cache(rxq->mp, rte_lcore_id())".

[1] https://elixir.bootlin.com/dpdk/v22.11-rc2/source/drivers/net/i40e/i40e_rxtx_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.

> 
> 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@smartserver.smartshare.dk/T/#u


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

> 
> >
> > /**
> >  * 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 == NULL)
> >         cache = rte_mempool_default_cache(mp, rte_lcore_id());
> >     if (cache == NULL) {
> >         rte_errno = 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 = 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 <= cache->flushthresh) {
> >         cache_objs = &cache->objs[cache->len];
> >         cache->len += n;
> >     } else {
> >         cache_objs = &cache->objs[0];
> >         rte_mempool_ops_enqueue_bulk(mp, cache_objs, cache->len);
> >         cache->len = 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ørup
> >
>