From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from proxy.6wind.com (host.76.145.23.62.rev.coltfrance.com [62.23.145.76]) by dpdk.org (Postfix) with ESMTP id 50F50C3EC for ; Wed, 11 May 2016 11:56:15 +0200 (CEST) Received: from [10.16.0.195] (unknown [10.16.0.195]) by proxy.6wind.com (Postfix) with ESMTP id 7A7B723CD6; Wed, 11 May 2016 11:54:37 +0200 (CEST) To: Lazaros Koromilas , dev@dpdk.org References: <1459784610-14008-1-git-send-email-l@nofutznetworks.com> From: Olivier MATZ Message-ID: <573301B0.5000205@6wind.com> Date: Wed, 11 May 2016 11:56:00 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.6.0 MIME-Version: 1.0 In-Reply-To: <1459784610-14008-1-git-send-email-l@nofutznetworks.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v2 1/2] mempool: allow for user-owned mempool caches X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 11 May 2016 09:56:15 -0000 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