Re: [dpdk-dev] [PATCH v2 1/2] mempool: allow for user-owned mempool caches

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

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