On Sat, Oct 08, 2022 at 10:56:06PM +0200, Thomas Monjalon wrote: > 07/10/2022 12:44, Andrew Rybchenko: > > From: Morten Brørup <m...@smartsharesystems.com> > > > > A flush threshold for the mempool cache was introduced in DPDK version > > 1.3, but rte_mempool_do_generic_get() was not completely updated back > > then, and some inefficiencies were introduced. > > > > Fix the following in rte_mempool_do_generic_get(): > > > > 1. The code that initially screens the cache request was not updated > > with the change in DPDK version 1.3. > > The initial screening compared the request length to the cache size, > > which was correct before, but became irrelevant with the introduction of > > the flush threshold. E.g. the cache can hold up to flushthresh objects, > > which is more than its size, so some requests were not served from the > > cache, even though they could be. > > The initial screening has now been corrected to match the initial > > screening in rte_mempool_do_generic_put(), which verifies that a cache > > is present, and that the length of the request does not overflow the > > memory allocated for the cache. > > > > This bug caused a major performance degradation in scenarios where the > > application burst length is the same as the cache size. In such cases, > > the objects were not ever fetched from the mempool cache, regardless if > > they could have been. > > This scenario occurs e.g. if an application has configured a mempool > > with a size matching the application's burst size. > > > > 2. The function is a helper for rte_mempool_generic_get(), so it must > > behave according to the description of that function. > > Specifically, objects must first be returned from the cache, > > subsequently from the backend. > > After the change in DPDK version 1.3, this was not the behavior when > > the request was partially satisfied from the cache; instead, the objects > > from the backend were returned ahead of the objects from the cache. > > This bug degraded application performance on CPUs with a small L1 cache, > > which benefit from having the hot objects first in the returned array. > > (This is probably also the reason why the function returns the objects > > in reverse order, which it still does.) > > Now, all code paths first return objects from the cache, subsequently > > from the backend. > > > > The function was not behaving as described (by the function using it) > > and expected by applications using it. This in itself is also a bug. > > > > 3. If the cache could not be backfilled, the function would attempt > > to get all the requested objects from the backend (instead of only the > > number of requested objects minus the objects available in the backend), > > and the function would fail if that failed. > > Now, the first part of the request is always satisfied from the cache, > > and if the subsequent backfilling of the cache from the backend fails, > > only the remaining requested objects are retrieved from the backend. > > > > The function would fail despite there are enough objects in the cache > > plus the common pool. > > > > 4. The code flow for satisfying the request from the cache was slightly > > inefficient: > > The likely code path where the objects are simply served from the cache > > was treated as unlikely. Now it is treated as likely. > > > > Signed-off-by: Morten Brørup <m...@smartsharesystems.com> > > Signed-off-by: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru> > > Reviewed-by: Morten Brørup <m...@smartsharesystems.com> > > Applied, thanks.
Better late than never: I reviewed this patch after it has been pushed, and it looks good to me. Thanks, Olivier
