> > > > > diff --git a/lib/mempool/rte_mempool.h
> > b/lib/mempool/rte_mempool.h
> > > > > index 5a81e53a9f..5fc7cdb79c 100644
> > > > > --- a/lib/mempool/rte_mempool.h
> > > > > +++ b/lib/mempool/rte_mempool.h
> > > > > @@ -105,8 +105,7 @@ struct __rte_cache_aligned rte_mempool_cache
> > {
> > > > > } stats; /**< Statistics */
> > > > > #endif
> > > > > /** Cache objects */
> > > > > - alignas(RTE_CACHE_LINE_SIZE) void
> > > > > *objs[RTE_MEMPOOL_CACHE_MAX_SIZE];
> > > > > - RTE_CACHE_GUARD;
> > > > > + alignas(RTE_CACHE_LINE_SIZE) void *objs[];
> > > >
> > > > LGTM to me in general.
> > > > Again, just as a thought:
> > > > Would it be plausible to have a pointer per lcore instead?
> > > > i.e.:
> > > > alignas(RTE_CACHE_LINE_SIZE) void *objs[RTE_MAX_LCORE];
> > > > That way we can avoid multiply op in rte_mempool_default_cache(),
> > > > also it will allow us to allocate lcore cache on demand (only for
> > > > enabled lcores).
> > > > As a downside - sizeof(rte_mempool_cache) will grow by
> > > > (RTE_MAX_LCORE - 1) * sizeof(uintptr_t), but that's probably not a
> > big
> > > > deal.
> > >
> > > The multiply op is not expensive.
> > >
> > > This patch has:
> > > struct rte_mempool_cache *cache =
> > > (struct rte_mempool_cache *)
> > > RTE_PTR_ADD(
> > > mp->local_cache,
> > > lcore_id * (size_t)mp->sizeof_cache_per_lcore);
> > >
> > > Which compiles to something like:
> > > ptr = LOAD(mp + offsetof(cache_ptr));
> > > off = LOAD(mp + offsetof(sizeof_cache_per_lcore));
> > > off *= lcore_id;
> > > ptr += off;
> > >
> > >
> > > Yes, having an array of pointers to the caches in the mempool would
> > be a good
> > > alternative:
> > >
> > > struct __rte_cache_aligned rte_mempool {
> > > - struct rte_mempool_cache *local_cache; /**< Per-lcore local cache
> > */
> > >
> > > [...]
> > > struct rte_mempool_debug_stats stats[RTE_MAX_LCORE + 1];
> > > #endif
> > > + /** Per-lcore local cache */
> > > + struct rte_mempool_cache *local_cache[RTE_MAX_LCORE];
> > > };
> >
> > Yes, you right, off-course it has to be that way.
> >
> > >
> > >
> > > Lookup for that would become simple:
> > > struct rte_mempool_cache *cache =
> > > mp->local_cache[lcore_id];
> > >
> > > Which compiles to something like:
> > > off = lcore_id * sizeof(void*); // Shift operation
> > > off += offsetof(cache_ptr_array);
> > > ptr = LOAD(mp + off);
> > >
> > > Yes, I suppose that would be faster.
> > > I'll take a stab at it.
>
> I tried it, but the mempool perf test yields similar results on our build
> server (a
> virtual machine).
>
> There are also a couple of disadvantages:
> 1. The table has a larger memory (and cache) footprint than the solution with
> the
> base pointer and multiplier. We knew that, and consider it insignificant.
> 2. If the mempool cache misses, the backend must be involved, and this needs
> to
> load mp->ops_index. With the base/multiplier variant, ops_index is in the same
> cache line as the local_cache pointer, and thus already hot in the cache.
> With the
> lcore_id indexed local_cache pointer array, ops_index is not hot in the cache.
> Obviously, we want to optimize for mempool cache hits, so this should be
> considered insignificant; I only mention it for completeness.
>
> I think we should stick with the patch series as is.
Ok, thanks for trying.
Overall the series looks good to me, so:
Series-Acked-by: Konstantin Ananyev <[email protected]>
>
> If you want to play around with the lcore_id indexed local_cache pointer
> array, a
> patch is provided at the bottom of this email.
If I'll have time, I will run both versions on our boxes, though no hard
promises.
> > >
> > > Caches for all lcores must be allocated at mempool creation. Or we
> > would need
> > > special handling to allocate/free the relevant per-lcore cache for
> > all registered
> > > mempools whenever a new lcore is registered for use at runtime, e.g.
> > by
> > > rte_thread_register()/unregister().
> >
> > My initial thought was:
> > we can allocate caches for all lcores that are already enabled at
> > mempool_init time.
> > If later, such core will be disabled - that's probably not a big deal,
> > we can simply left the cache for it
> > hanging around unused until mempool will be destroyed.
> > For lcores that will be enabled after mempool_init() we can use lazy
> > allocation (at first access to it).
> > But after another thought - that's probably not such good idea, as in
> > principle rte_malloc() here can fail.
> >
> > > >
> > > > > };
> > > > >
> > > > > /**
> > > > > @@ -265,6 +264,7 @@ struct __rte_cache_aligned rte_mempool {
> > > > > int32_t ops_index;
> > > > >
> > > > > struct rte_mempool_cache *local_cache; /**< Per-lcore local
> > cache
> > > > */
> > > > > + uint32_t sizeof_cache_per_lcore; /**< Multiplier for
> > indexing
> > > > into the
> > > > > local cache. */
> > > > >
> > > > > uint32_t populated_size; /**< Number of populated
> > > > objects. */
> > > > > struct rte_mempool_objhdr_list elt_list; /**< List of
> > objects in
> > > > pool */
> > > > > @@ -1050,8 +1050,7 @@ rte_mempool_free(struct rte_mempool *mp);
> > > > > * @param cache_size
> > > > > * If cache_size is non-zero, the rte_mempool library will try
> > to
> > > > > * limit the accesses to the common lockless pool, by
> > maintaining
> > > > a
> > > > > - * per-lcore object cache. This argument must be lower or
> > equal to
> > > > > - * RTE_MEMPOOL_CACHE_MAX_SIZE and n.
> > > > > + * per-lcore object cache. This argument must be lower or
> > equal to
> > > > n.
> > > > > * The access to the per-lcore table is of course
> > > > > * faster than the multi-producer/consumer pool. The cache can
> > be
> > > > > * disabled if the cache_size argument is set to 0; it can be
> > > > useful to
> > > > > @@ -1371,15 +1370,17 @@ rte_mempool_cache_create(uint32_t size,
> > int
> > > > > socket_id)
> > > > > static __rte_always_inline struct rte_mempool_cache *
> > > > > rte_mempool_default_cache(struct rte_mempool *mp, unsigned
> > lcore_id)
> > > > > {
> > > > > - if (unlikely(mp->cache_size == 0))
> > > > > + if (unlikely(mp->local_cache == NULL))
> > > > > return NULL;
> > > > >
> > > > > if (unlikely(lcore_id == LCORE_ID_ANY))
> > > > > return NULL;
> > > > >
> > > > > - rte_mempool_trace_default_cache(mp, lcore_id,
> > > > > - &mp->local_cache[lcore_id]);
> > > > > - return &mp->local_cache[lcore_id];
> > > > > + struct rte_mempool_cache *cache = (struct rte_mempool_cache
> > > > > *)RTE_PTR_ADD(mp->local_cache,
> > > > > + lcore_id * (size_t)mp->sizeof_cache_per_lcore);
> > > > > + rte_mempool_trace_default_cache(mp, lcore_id, cache);
> > > > > + __rte_assume(cache != NULL);
> > > > > + return cache;
> > > > > }
> > > > >
> > > > > /**
> > > > > --
> > > > > 2.43.0
>
> diff --git a/doc/guides/rel_notes/release_26_11.rst
> b/doc/guides/rel_notes/release_26_11.rst
> index 64f6802e58..42fc5d2ad2 100644
> --- a/doc/guides/rel_notes/release_26_11.rst
> +++ b/doc/guides/rel_notes/release_26_11.rst
> @@ -105,7 +105,7 @@ API Changes
> as it is often used for the ``cache_size`` parameter when creating
> mempools.
>
> * mempool: Updated the ``rte_mempool`` structure as follows:
> - - Added the ``sizeof_cache_per_lcore`` field, for indexing into the
> per-lcore
> local cache.
> + - Changed the ``local_cache`` field to array of pointers to per-lcore
> local cache,
> for improved lookup performance.
>
> * mempool: Updated the ``rte_mempool_cache`` structure as follows:
> - Removed the deprecated and obsolete ``flushthresh`` field.
> diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
> index 069aabaa70..556ecfe150 100644
> --- a/lib/mempool/rte_mempool.c
> +++ b/lib/mempool/rte_mempool.c
> @@ -759,7 +759,7 @@ mempool_cache_init(struct rte_mempool_cache
> *cache, uint32_t size)
> /*
> * Create and initialize a cache for objects that are retrieved from and
> * returned to an underlying mempool. This structure is identical to the
> - * local_cache entry pointed to by the mempool structure.
> + * local_cache[lcore_id] pointed to by the mempool structure.
> */
> RTE_EXPORT_SYMBOL(rte_mempool_cache_create)
> struct rte_mempool_cache *
> @@ -930,6 +930,7 @@ rte_mempool_create_empty(const char *name,
> unsigned n, unsigned elt_size,
> mp->elt_size = objsz.elt_size;
> mp->header_size = objsz.header_size;
> mp->trailer_size = objsz.trailer_size;
> + /* Size of default caches, zero means disabled. */
> mp->cache_size = cache_size;
> mp->private_data_size = private_data_size;
> STAILQ_INIT(&mp->elt_list);
> @@ -953,17 +954,16 @@ rte_mempool_create_empty(const char *name,
> unsigned n, unsigned elt_size,
> goto exit_unlock;
> }
>
> - /* local_cache pointer is only set if per-lcore local cache is
> present */
> + /* local_cache pointers are only set if per-lcore local cache is
> present */
> if (cache_size != 0) {
> - mp->local_cache = (struct rte_mempool_cache *)
> - RTE_PTR_ADD(mp, sizeof(struct rte_mempool) +
> private_data_size);
> - mp->sizeof_cache_per_lcore = sizeof_cache_per_lcore;
> -
> /* Init all default caches. */
> - struct rte_mempool_cache *cache = mp->local_cache;
> + struct rte_mempool_cache *cache = (struct rte_mempool_cache *)
> + RTE_PTR_ADD(mp, sizeof(struct rte_mempool) +
> private_data_size);
> for (unsigned int lcore_id = 0; lcore_id < RTE_MAX_LCORE;
> - lcore_id++, cache = RTE_PTR_ADD(cache,
> sizeof_cache_per_lcore))
> + lcore_id++, cache = RTE_PTR_ADD(cache,
> sizeof_cache_per_lcore)) {
> + mp->local_cache[lcore_id] = cache;
> mempool_cache_init(cache, cache_size);
> + }
> }
>
> te->data = mp;
> @@ -1026,17 +1026,16 @@ RTE_EXPORT_SYMBOL(rte_mempool_avail_count)
> unsigned int
> rte_mempool_avail_count(const struct rte_mempool *mp)
> {
> - unsigned int count;
> + unsigned count;
> + unsigned lcore_id;
>
> count = rte_mempool_ops_get_count(mp);
>
> - if (mp->local_cache == NULL)
> + if (mp->cache_size == 0)
> return count;
>
> - const struct rte_mempool_cache *cache = mp->local_cache;
> - for (unsigned int lcore_id = 0; lcore_id < RTE_MAX_LCORE;
> - lcore_id++, cache = RTE_PTR_ADD(cache, mp-
> >sizeof_cache_per_lcore))
> - count += cache->len;
> + for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++)
> + count += mp->local_cache[lcore_id]->len;
>
> /*
> * due to race condition (access to len is not locked), the
> @@ -1064,11 +1063,11 @@ rte_mempool_stats_reset(struct rte_mempool
> *mp)
>
> #ifdef RTE_LIBRTE_MEMPOOL_STATS
> memset(&mp->stats, 0, sizeof(mp->stats));
> - if (mp->local_cache != NULL) {
> - struct rte_mempool_cache *cache = mp->local_cache;
> - for (unsigned int lcore_id = 0; lcore_id < RTE_MAX_LCORE;
> - lcore_id++, cache = RTE_PTR_ADD(cache, mp-
> >sizeof_cache_per_lcore))
> - memset(&cache->stats, 0, sizeof(cache->stats));
> +- if (mp->cache_size != 0) {
> +- for (unsigned int lcore_id = 0; lcore_id < RTE_MAX_LCORE;
> lcore_id++) {
> +- memset(&mp->local_cache[lcore_id]->stats, 0,
> +-
> sizeof(mp->local_cache[lcore_id]->stats));
> +- }
> }
>
> RTE_MEMPOOL_LOG(DEBUG, "<%s>@%p: statistics reset", mp->name, mp);
> @@ -1082,18 +1081,18 @@ rte_mempool_stats_reset(struct rte_mempool
> *mp)
> static unsigned
> rte_mempool_dump_cache(FILE *f, const struct rte_mempool *mp)
> {
> - unsigned int count = 0;
> + unsigned lcore_id;
> + unsigned count = 0;
> + unsigned cache_count;
>
> fprintf(f, " internal cache infos (hide zero value items):\n");
> fprintf(f, " cache_size=%"PRIu32"\n", mp->cache_size);
>
> - if (mp->local_cache == NULL)
> + if (mp->cache_size == 0)
> return count;
>
> - const struct rte_mempool_cache *cache = mp->local_cache;
> - for (unsigned int lcore_id = 0; lcore_id < RTE_MAX_LCORE;
> - lcore_id++, cache = RTE_PTR_ADD(cache, mp-
> >sizeof_cache_per_lcore)) {
> - unsigned int cache_count = cache->len;
> + for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
> + cache_count = mp->local_cache[lcore_id]->len;
> if (cache_count == 0)
> continue;
> fprintf(f, " cache_count[%u]=%"PRIu32"\n",
> @@ -1234,12 +1233,14 @@ mempool_audit_cookies(struct rte_mempool *mp)
> static void
> mempool_audit_cache(const struct rte_mempool *mp)
> {
> - if (mp->local_cache == NULL)
> + unsigned lcore_id;
> +
> + if (mp->cache_size == 0)
> return;
>
> - const struct rte_mempool_cache *cache = mp->local_cache;
> - for (unsigned int lcore_id = 0; lcore_id < RTE_MAX_LCORE;
> - lcore_id++, cache = RTE_PTR_ADD(cache, mp-
> >sizeof_cache_per_lcore)) {
> + for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
> + const struct rte_mempool_cache *cache;
> + cache = mp->local_cache[lcore_id];
> if (cache->size > mp->size) {
> RTE_MEMPOOL_LOG(CRIT, "badness on cache[%u] size",
> lcore_id);
> rte_panic("MEMPOOL: invalid cache[%u] size\n",
> lcore_id);
> @@ -1333,15 +1334,13 @@ rte_mempool_dump(FILE *f, struct rte_mempool
> *mp)
> sum.get_success_blks += mp->stats[lcore_id].get_success_blks;
> sum.get_fail_blks += mp->stats[lcore_id].get_fail_blks;
> }
> - if (mp->local_cache != NULL) {
> + if (mp->cache_size != 0) {
> /* Add the statistics stored in the mempool caches. */
> - const struct rte_mempool_cache *cache = mp->local_cache;
> - for (lcore_id = 0; lcore_id < RTE_MAX_LCORE;
> - lcore_id++, cache = RTE_PTR_ADD(cache, mp-
> >sizeof_cache_per_lcore)) {
> - sum.put_bulk += cache->stats.put_bulk;
> - sum.put_objs += cache->stats.put_objs;
> - sum.get_success_bulk += cache->stats.get_success_bulk;
> - sum.get_success_objs += cache->stats.get_success_objs;
> + for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
> + sum.put_bulk +=
> mp->local_cache[lcore_id]->stats.put_bulk;
> + sum.put_objs +=
> mp->local_cache[lcore_id]->stats.put_objs;
> + sum.get_success_bulk += mp->local_cache[lcore_id]-
> >stats.get_success_bulk;
> + sum.get_success_objs += mp->local_cache[lcore_id]-
> >stats.get_success_objs;
> }
> }
> fprintf(f, " stats:\n");
> @@ -1638,11 +1637,10 @@ mempool_info_cb(struct rte_mempool *mp, void
> *arg)
> mp->populated_size);
>
> cache_count = 0;
> - if (mp->local_cache != NULL) {
> - const struct rte_mempool_cache *cache = mp->local_cache;
> - for (unsigned int lcore_id = 0; lcore_id < RTE_MAX_LCORE;
> - lcore_id++, cache = RTE_PTR_ADD(cache, mp-
> >sizeof_cache_per_lcore))
> - cache_count += cache->len;
> + if (mp->cache_size > 0) {
> + int lcore_id;
> + for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++)
> + cache_count += mp->local_cache[lcore_id]->len;
> }
> rte_tel_data_add_dict_uint(info->d, "total_cache_count", cache_count);
> common_count = rte_mempool_ops_get_count(mp);
> diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
> index 5fc7cdb79c..cc255063f5 100644
> --- a/lib/mempool/rte_mempool.h
> +++ b/lib/mempool/rte_mempool.h
> @@ -263,14 +263,18 @@ struct __rte_cache_aligned rte_mempool {
> */
> int32_t ops_index;
>
> - struct rte_mempool_cache *local_cache; /**< Per-lcore local cache */
> - uint32_t sizeof_cache_per_lcore; /**< Multiplier for indexing into
> the local
> cache. */
> -
> uint32_t populated_size; /**< Number of populated objects. */
> struct rte_mempool_objhdr_list elt_list; /**< List of objects in pool
> */
> uint32_t nb_mem_chunks; /**< Number of memory chunks */
> struct rte_mempool_memhdr_list mem_list; /**< List of memory chunks */
>
> + /**
> + * Pointers to per-lcore local cache.
> + * Note: Cache line aligned for higher cache hit rate when only using
> the first
> few lcores.
> + */
> + alignas(RTE_CACHE_LINE_SIZE)
> + struct rte_mempool_cache *local_cache[RTE_MAX_LCORE];
> +
> #ifdef RTE_LIBRTE_MEMPOOL_STATS
> /** Per-lcore statistics.
> *
> @@ -1370,17 +1374,12 @@ rte_mempool_cache_create(uint32_t size, int
> socket_id)
> static __rte_always_inline struct rte_mempool_cache *
> rte_mempool_default_cache(struct rte_mempool *mp, unsigned lcore_id)
> {
> - if (unlikely(mp->local_cache == NULL))
> - return NULL;
> -
> if (unlikely(lcore_id == LCORE_ID_ANY))
> return NULL;
>
> - struct rte_mempool_cache *cache = (struct rte_mempool_cache
> *)RTE_PTR_ADD(mp->local_cache,
> - lcore_id * (size_t)mp->sizeof_cache_per_lcore);
> - rte_mempool_trace_default_cache(mp, lcore_id, cache);
> - __rte_assume(cache != NULL);
> - return cache;
> + rte_mempool_trace_default_cache(mp, lcore_id,
> + mp->local_cache[lcore_id]);
> + return mp->local_cache[lcore_id];
> }
>
> /**
> @@ -1449,8 +1448,8 @@ rte_mempool_do_generic_put(struct rte_mempool
> *mp, void * const *obj_table,
> * are more hot, from the upper half of the cache.
> */
> __rte_assume(cache->len > cache->size / 2);
> - rte_mempool_ops_enqueue_bulk(mp, &cache->objs[0], cache->size
> /
> 2);
> - rte_memcpy(&cache->objs[0], &cache->objs[cache->size / 2],
> + rte_mempool_ops_enqueue_bulk(mp, cache->objs, cache->size /
> 2);
> + rte_memcpy(cache->objs, &cache->objs[cache->size / 2],
> sizeof(void *) * (cache->len - cache->size /
> 2));
> cache_objs = &cache->objs[cache->len - cache->size / 2];
> cache->len = cache->len - cache->size / 2 + n;
