Re: [dpdk-dev] [PATCH] mempool: optimize copy in cache get

[Olivier Matz <olivier.matz@6wind.com>]

Hi,

On Mon, Jul 01, 2019 at 02:21:41PM +0000, Wang, Xiao W wrote:
> Hi,
> 
> > -----Original Message-----
> > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > Sent: Monday, July 1, 2019 9:11 PM
> > To: Andrew Rybchenko <arybchenko@solarflare.com>
> > Cc: Wang, Xiao W <xiao.w.wang@intel.com>; dev@dpdk.org
> > Subject: Re: [PATCH] mempool: optimize copy in cache get
> > 
> > Hi,
> > 
> > On Tue, May 21, 2019 at 12:34:55PM +0300, Andrew Rybchenko wrote:
> > > On 5/21/19 12:03 PM, Xiao Wang wrote:
> > > > Use rte_memcpy to improve the pointer array copy. This optimization
> > method
> > > > has already been applied to __mempool_generic_put() [1], this patch
> > applies
> > > > it to __mempool_generic_get(). Slight performance gain can be observed
> > in
> > > > testpmd txonly test.
> > > >
> > > > [1] 863bfb47449 ("mempool: optimize copy in cache")
> > > >
> > > > Signed-off-by: Xiao Wang <xiao.w.wang@intel.com>
> > > > ---
> > > >   lib/librte_mempool/rte_mempool.h | 7 +------
> > > >   1 file changed, 1 insertion(+), 6 deletions(-)
> > > >
> > > > diff --git a/lib/librte_mempool/rte_mempool.h
> > b/lib/librte_mempool/rte_mempool.h
> > > > index 8053f7a04..975da8d22 100644
> > > > --- a/lib/librte_mempool/rte_mempool.h
> > > > +++ b/lib/librte_mempool/rte_mempool.h
> > > > @@ -1344,15 +1344,11 @@ __mempool_generic_get(struct
> > rte_mempool *mp, void **obj_table,
> > > >   		      unsigned int n, struct rte_mempool_cache *cache)
> > > >   {
> > > >   	int ret;
> > > > -	uint32_t index, len;
> > > > -	void **cache_objs;
> > > >   	/* No cache provided or cannot be satisfied from cache */
> > > >   	if (unlikely(cache == NULL || n >= cache->size))
> > > >   		goto ring_dequeue;
> > > > -	cache_objs = cache->objs;
> > > > -
> > > >   	/* Can this be satisfied from the cache? */
> > > >   	if (cache->len < n) {
> > > >   		/* No. Backfill the cache first, and then fill from it */
> > > > @@ -1375,8 +1371,7 @@ __mempool_generic_get(struct rte_mempool
> > *mp, void **obj_table,
> > > >   	}
> > > >   	/* Now fill in the response ... */
> > > > -	for (index = 0, len = cache->len - 1; index < n; ++index, len--,
> > obj_table++)
> > > > -		*obj_table = cache_objs[len];
> > > > +	rte_memcpy(obj_table, &cache->objs[cache->len - n], sizeof(void *) *
> > n);
> > > >   	cache->len -= n;
> > >
> > > I think the idea of the loop above is to get objects in reverse order to
> > > order
> > > to reuse cache top objects (put last) first. It should improve cache hit
> > > etc.
> > > So, performance effect of the patch could be very different on various CPUs
> > > (with different cache sizes) and various work-loads.
> > >
> > > So, I doubt that it is a step in right direction.
> > 
> > For reference, this was already discussed 3 years ago:
> > 
> > https://mails.dpdk.org/archives/dev/2016-May/039873.html
> > https://mails.dpdk.org/archives/dev/2016-June/040029.html
> > 
> > I'm still not convinced that reversing object addresses (as it's done
> > today) is really important. But Andrew is probably right, the impact of
> > this kind of patch probably varies depending on many factors. More
> > performance numbers on real-life use-cases would help to decide what to
> > do.
> > 
> > Regards,
> > Olivier
> 
> I agree, and thanks for the reference link. So theoretically neither way can be
> a definite best choice, it depends on various real-life factors. I'm thinking about
> how to let app developer be aware of this so that they themselves could make
> the choice. Or it's not worth doing due to small perf gain?

I don't think it's worth doing a dynamic selection for this.

On the other hand, having performance numbers for different
use-cases/archs is welcome. From a pure cpu cycles perspective, the
rte_memcpy() should be faster.

Regards,
Olivier