Re: [dpdk-dev] [PATCH v2] mempool: reduce rte_mempool structure size

["Wiles, Keith" <keith.wiles@intel.com>]

>>Hi Keith,
>>
>>Thank you for adding the RTE_NEXT_ABI. I think this is the way
>>described in the process. Your changes will be available in next
>>version (16.4) for people compiling with RTE_NEXT_ABI=y, and in
>>16.7 without option (I'm just surprised that RTE_NEXT_ABI=y in
>>default configs...).
>>
>>I think a deprecation notice should also be added in this commit
>>in doc/guides/rel_notes/deprecation.rst.
>
>Will add the text.
>>
>>Please also find comments below.
>>
>>On 02/09/2016 06:30 PM, Keith Wiles wrote:
>>
>>> diff --git a/config/defconfig_x86_64-native-linuxapp-gcc b/config/defconfig_x86_64-native-linuxapp-gcc
>>> index 60baf5b..02e9ace 100644
>>> --- a/config/defconfig_x86_64-native-linuxapp-gcc
>>> +++ b/config/defconfig_x86_64-native-linuxapp-gcc
>>> @@ -40,3 +40,8 @@ CONFIG_RTE_ARCH_64=y
>>>  
>>>  CONFIG_RTE_TOOLCHAIN="gcc"
>>>  CONFIG_RTE_TOOLCHAIN_GCC=y
>>> +CONFIG_RTE_BUILD_SHARED_LIB=y
>>> +CONFIG_RTE_NEXT_ABI=n
>>> +CONFIG_RTE_EAL_IGB_UIO=n
>>> +CONFIG_RTE_LIBRTE_KNI=n
>>> +CONFIG_RTE_KNI_KMOD=n
>
>Hmm, not sure where this came from, but will remove it.

I think this was from the ABI-Checker I ran and the tool should leave the repo in its original state.

>>
>>I think this should not be part of the patch.
>>
>>> @@ -672,6 +704,24 @@ rte_mempool_count(const struct rte_mempool *mp)
>>>  static unsigned
>>>  rte_mempool_dump_cache(FILE *f, const struct rte_mempool *mp)
>>>  {
>>> +#ifdef RTE_NEXT_ABI
>>> +	unsigned lcore_id;
>>> +	unsigned count = 0;
>>> +	unsigned cache_count;
>>> +
>>> +	fprintf(f, "  cache infos:\n");
>>> +	fprintf(f, "    cache_size=%"PRIu32"\n", mp->cache_size);
>>> +	if (mp->cache_size == 0)
>>> +		return count;
>>> +
>>> +	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
>>> +		cache_count = mp->local_cache[lcore_id].len;
>>> +		fprintf(f, "    cache_count[%u]=%u\n", lcore_id, cache_count);
>>> +		count += cache_count;
>>> +	}
>>> +	fprintf(f, "    total_cache_count=%u\n", count);
>>> +	return count;
>>> +#else
>>>  #if RTE_MEMPOOL_CACHE_MAX_SIZE > 0
>>>  	unsigned lcore_id;
>>>  	unsigned count = 0;
>>
>>I think in this case we could avoid to duplicate the code without
>>beeing unclear by using the proper #ifdefs:
>
>I was struggling with how it should be done. I like to see clear ifdefs and be able to see the complete code for a given case. In these cases I wanted to make it simple to remove the code quickly by just deleting lines instead of editing lines. I will follow your suggestion.
>>
>>#if RTE_MEMPOOL_CACHE_MAX_SIZE > 0 || defined(RTE_NEXT_ABI)
>>	/* common code */
>>#ifdef RTE_NEXT_ABI
>>	if (mp->cache_size == 0)
>>		return count;
>>#endif
>>	/* common code */
>>#else
>>...
>>#endif
>>
>>
>>> @@ -755,6 +806,26 @@ mempool_audit_cookies(const struct rte_mempool *mp)
>>>  #define mempool_audit_cookies(mp) do {} while(0)
>>>  #endif
>>>  
>>> +#ifdef RTE_NEXT_ABI
>>> +/* check cookies before and after objects */
>>> +static void
>>> +mempool_audit_cache(const struct rte_mempool *mp)
>>> +{
>>> +	/* check cache size consistency */
>>> +	unsigned lcore_id;
>>> +
>>> +	if (mp->cache_size == 0)
>>> +		return;
>>> +
>>> +	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
>>> +		if (mp->local_cache[lcore_id].len > mp->cache_flushthresh) {
>>> +			RTE_LOG(CRIT, MEMPOOL, "badness on cache[%u]\n",
>>> +				lcore_id);
>>> +			rte_panic("MEMPOOL: invalid cache len\n");
>>> +		}
>>> +	}
>>> +}
>>> +#else
>>
>>same here
>>
>>> diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
>>> index 6e2390a..fc9b595 100644
>>> --- a/lib/librte_mempool/rte_mempool.h
>>> +++ b/lib/librte_mempool/rte_mempool.h
>>> @@ -95,6 +95,19 @@ struct rte_mempool_debug_stats {
>>>  } __rte_cache_aligned;
>>>  #endif
>>>  
>>> +#ifdef RTE_NEXT_ABI
>>> +/**
>>> + * A structure that stores a per-core object cache.
>>> + */
>>> +struct rte_mempool_cache {
>>> +	unsigned len; /**< Cache len */
>>> +	/*
>>> +	 * Cache is allocated to this size to allow it to overflow in certain
>>> +	 * cases to avoid needless emptying of cache.
>>> +	 */
>>> +	void *objs[RTE_MEMPOOL_CACHE_MAX_SIZE * 3]; /**< Cache objects */
>>> +} __rte_cache_aligned;
>>> +#else
>>
>>same here
>>
>>
>>
>>> @@ -755,19 +793,25 @@ static inline void __attribute__((always_inline))
>>>  __mempool_put_bulk(struct rte_mempool *mp, void * const *obj_table,
>>>  		    unsigned n, int is_mp)
>>>  {
>>> +#ifndef RTE_NEXT_ABI	/* Note: ifndef */
>>>  #if RTE_MEMPOOL_CACHE_MAX_SIZE > 0
>>> +#endif /* RTE_NEXT_ABI */
>>>  	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;
>>> +#ifndef RTE_NEXT_ABI	/* Note: ifndef */
>>>  #endif /* RTE_MEMPOOL_CACHE_MAX_SIZE > 0 */
>>> +#endif /* RTE_NEXT_ABI */
>>
>>this looks strange... I think it does not work properly.
>>Why not
>>#if RTE_MEMPOOL_CACHE_MAX_SIZE > 0 || defined(RTE_NEXT_ABI)
>
>Yes, it is strange :-(
>>
>>>  	/* increment stat now, adding in mempool always success */
>>>  	__MEMPOOL_STAT_ADD(mp, put, n);
>>>  
>>> +#ifndef RTE_NEXT_ABI	/* Note: ifndef */
>>>  #if RTE_MEMPOOL_CACHE_MAX_SIZE > 0
>>> +#endif /* RTE_NEXT_ABI */
>>>  	/* cache is not enabled or single producer or non-EAL thread */
>>>  	if (unlikely(cache_size == 0 || is_mp == 0 ||
>>>  		     lcore_id >= RTE_MAX_LCORE))
>>> @@ -802,7 +846,9 @@ __mempool_put_bulk(struct rte_mempool *mp, void * const *obj_table,
>>>  	return;
>>>  
>>>  ring_enqueue:
>>> +#ifndef RTE_NEXT_ABI	/* Note: ifndef */
>>>  #endif /* RTE_MEMPOOL_CACHE_MAX_SIZE > 0 */
>>> +#endif /* RTE_NEXT_ABI */
>>>  
>>>  	/* push remaining objects in ring */
>>>  #ifdef RTE_LIBRTE_MEMPOOL_DEBUG
>>> @@ -946,7 +992,9 @@ __mempool_get_bulk(struct rte_mempool *mp, void **obj_table,
>>>  		   unsigned n, int is_mc)
>>>  {
>>>  	int ret;
>>> +#ifndef RTE_NEXT_ABI	/* Note: ifndef */
>>>  #if RTE_MEMPOOL_CACHE_MAX_SIZE > 0
>>> +#endif /* RTE_NEXT_ABI */
>>>  	struct rte_mempool_cache *cache;
>>>  	uint32_t index, len;
>>>  	void **cache_objs;
>>> @@ -992,7 +1040,9 @@ __mempool_get_bulk(struct rte_mempool *mp, void **obj_table,
>>>  	return 0;
>>>  
>>>  ring_dequeue:
>>> +#ifndef RTE_NEXT_ABI	/* Note: ifndef */
>>>  #endif /* RTE_MEMPOOL_CACHE_MAX_SIZE > 0 */
>>> +#endif /* RTE_NEXT_ABI */
>>>  
>>>  	/* get remaining objects from ring */
>>>  	if (is_mc)
>>
>>Same in those cases.
>>
>>
>>
>>Regards,
>>Olivier
>>
>
>
>Regards,
>Keith
>
>
>
>
>


Regards,
Keith