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

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

<snip>

> 
> > 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().
I am wondering if the PMD should be aware of the cache or not. For ex: in the case of pipeline mode, the RX and TX side of the PMD are running on different cores.
However, since the rte_mempool_cache_flush API is provided, may be that decision is already done? Interestingly, rte_mempool_cache_flush is called by just a single PMD.

So, the question is, should we allow zero-copy only for per-core cache or for other cases as well.

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

> 
> 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.
Just wondering about the advantage of this. I do not think PMD's have much of a choice other than calling 'rte_mempool_ops_enqueue_bulk'

> 
> >
> > 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());
Any reason we need this? If we are expecting the PMD to store the pointer to cache and a NULL is passed, it would mean it is a mempool with no per-core cache?
We could also leave the NULL check to the PMD.

> > >     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);
These are new stats. Do these break ABI compatibility (though these are under DEBUG flag)?

> > >
> > >     return cache_objs;
> > > }
> > >
> > >
> > > Med venlig hilsen / Kind regards,
> > > -Morten Brørup
> > >
> >