<snip> > > > From: Honnappa Nagarahalli [mailto:honnappa.nagaraha...@arm.com] > > Sent: Sunday, 6 November 2022 00.11 > > > > + Akshitha, she is working on similar patch > > > > Few comments inline > > > > > From: Morten Brørup <m...@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 > > > > >
