Hi Lazaros, Sorry for the late review. Please find some comments, in addition to what Konstantin already said.
On 04/04/2016 05:43 PM, Lazaros Koromilas wrote: > --- a/app/test/test_mempool.c > +++ b/app/test/test_mempool.c > @@ -79,6 +79,7 @@ > > static struct rte_mempool *mp; > static struct rte_mempool *mp_cache, *mp_nocache; > +static int use_external_cache; > > static rte_atomic32_t synchro; > > @@ -107,19 +108,33 @@ test_mempool_basic(void) > char *obj_data; > int ret = 0; > unsigned i, j; > + struct rte_mempool_cache *cache; > + > + if (use_external_cache) > + /* Create a user-owned mempool cache. */ > + cache = rte_mempool_cache_create(RTE_MEMPOOL_CACHE_MAX_SIZE, > + SOCKET_ID_ANY); > + else > + cache = rte_mempool_default_cache(mp, rte_lcore_id()); Shouldn't we return an error if rte_mempool_default_cache() failed? Even if the cache can be NULL for get/put, it would crash on the flush() operation, so it's better to return an error if the cache cannot be allocated. I also think the resource should be freed on error, maybe by doing "goto fail" instead of "return -1" in the subsequent checks. Note that I also reworked this test in my patchset, see: http://dpdk.org/dev/patchwork/patch/12069/ I think the "use_external_cache" parameter should be a parameter instead of a global variable, like I've done for the mempool pointer. > --- a/app/test/test_mempool_perf.c > +++ b/app/test/test_mempool_perf.c > @@ -98,6 +101,8 @@ > > static struct rte_mempool *mp; > static struct rte_mempool *mp_cache, *mp_nocache; > +static int use_external_cache; > +static unsigned external_cache_size = RTE_MEMPOOL_CACHE_MAX_SIZE; > > static rte_atomic32_t synchro; > The same comment (global vs parameter) could apply here, but it would require to rework the full test file... so maybe it's off topic. > @@ -137,6 +142,14 @@ per_lcore_mempool_test(__attribute__((unused)) void *arg) > int ret; > uint64_t start_cycles, end_cycles; > uint64_t time_diff = 0, hz = rte_get_timer_hz(); > + struct rte_mempool_cache *cache; > + > + if (use_external_cache) > + /* Create a user-owned mempool cache. */ > + cache = rte_mempool_cache_create(external_cache_size, > + SOCKET_ID_ANY); > + else > + cache = rte_mempool_default_cache(mp, lcore_id); > > /* n_get_bulk and n_put_bulk must be divisors of n_keep */ > if (((n_keep / n_get_bulk) * n_get_bulk) != n_keep) Same comments than above (check return value != NULL). The cache creation could be moved some lines below to avoid to free the resource on error. > --- a/lib/librte_eal/common/eal_common_log.c > +++ b/lib/librte_eal/common/eal_common_log.c > @@ -125,7 +125,7 @@ rte_log_add_in_history(const char *buf, size_t size) > } > } > else { > - if (rte_mempool_mc_get(log_history_mp, &obj) < 0) > + if (rte_mempool_get(log_history_mp, &obj) < 0) > obj = NULL; > hist_buf = obj; > } After seeing many changes like this, I wonder if it's possible to move these in a separate commit: "mempool: deprecate specific get/put functions" It would remove some noise in the "interesting" part. I suggest the following order: mempool: deprecate specific get/put functions mempool: use bit flags instead of is_mp and is_mc mempool: allow for user-owned mempool caches What do you think? > static inline void __attribute__((always_inline)) > -__mempool_put_bulk(struct rte_mempool *mp, void * const *obj_table, > - unsigned n, int is_mp) > +__mempool_generic_put(struct rte_mempool *mp, void * const *obj_table, > + unsigned n, struct rte_mempool_cache *cache, int is_mp) > { > - struct rte_mempool_cache *cache; > uint32_t index; > void **cache_objs; > - unsigned lcore_id = rte_lcore_id(); > - uint32_t cache_size = mp->cache_size; > - uint32_t flushthresh = mp->cache_flushthresh; > > /* increment stat now, adding in mempool always success */ > __MEMPOOL_STAT_ADD(mp, put, n); > > - /* cache is not enabled or single producer or non-EAL thread */ > - if (unlikely(cache_size == 0 || is_mp == 0 || > - lcore_id >= RTE_MAX_LCORE)) > + /* No cache provided or cache is not enabled or single producer */ > + if (unlikely(cache == NULL || cache->size == 0 || is_mp == 0)) > goto ring_enqueue; Is it possible that cache->size == 0? I suggest to remove that test and ensure that size != 0 at cache creation. > -__mempool_put_bulk(struct rte_mempool *mp, void * const *obj_table, > - unsigned n, int is_mp) > +__mempool_generic_put(struct rte_mempool *mp, void * const *obj_table, > + unsigned n, struct rte_mempool_cache *cache, int is_mp) > -rte_mempool_mp_put_bulk(struct rte_mempool *mp, void * const *obj_table, > - unsigned n) > -rte_mempool_sp_put_bulk(struct rte_mempool *mp, void * const *obj_table, > - unsigned n) > +rte_mempool_generic_put(struct rte_mempool *mp, void * const *obj_table, > + unsigned n, struct rte_mempool_cache *cache, int is_mp) > -rte_mempool_mp_put(struct rte_mempool *mp, void *obj) > -rte_mempool_sp_put(struct rte_mempool *mp, void *obj) > -__mempool_get_bulk(struct rte_mempool *mp, void **obj_table, > - unsigned n, int is_mc) > +__mempool_generic_get(struct rte_mempool *mp, void **obj_table, > + unsigned n, struct rte_mempool_cache *cache, int is_mc) > -rte_mempool_mc_get_bulk(struct rte_mempool *mp, void **obj_table, unsigned n) > -rte_mempool_sc_get_bulk(struct rte_mempool *mp, void **obj_table, unsigned n) > +rte_mempool_generic_get(struct rte_mempool *mp, void **obj_table, > -rte_mempool_mc_get(struct rte_mempool *mp, void **obj_p) > -rte_mempool_sc_get(struct rte_mempool *mp, void **obj_p) I think removing the mp/mc/sp/sc functions (or deprecate them for now, as suggested by Konstantin/Thomas) is a good thing for code readability in rte_mempool.h. Thanks for doing this! Regards, Olivier