Re: [PATCH V3] lib: set/get max memzone segments

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

Hi,

On 5/3/2023 8:26 AM, Ophir Munk wrote:
> In current DPDK the RTE_MAX_MEMZONE definition is unconditionally hard
> coded as 2560.  For applications requiring different values of this
> parameter – it is more convenient to set the max value via an rte API -
> rather than changing the dpdk source code per application.  In many
> organizations, the possibility to compile a private DPDK library for a
> particular application does not exist at all.  With this option there is
> no need to recompile DPDK and it allows using an in-box packaged DPDK.
> An example usage for updating the RTE_MAX_MEMZONE would be of an
> application that uses the DPDK mempool library which is based on DPDK
> memzone library.  The application may need to create a number of
> steering tables, each of which will require its own mempool allocation.
> This commit is not about how to optimize the application usage of
> mempool nor about how to improve the mempool implementation based on
> memzone.  It is about how to make the max memzone definition - run-time
> customized.
> This commit adds an API which must be called before rte_eal_init():
> rte_memzone_max_set(int max).  If not called, the default memzone
> (RTE_MAX_MEMZONE) is used.  There is also an API to query the effective
> max memzone: rte_memzone_max_get().

Commit message could use a little rewording and shortening. Suggested:

---

Currently, RTE_MAX_MEMZONE constant is used to decide how many memzones 
a DPDK application can have. This value could technically be changed by 
manually editing `rte_config.h` before compilation, but if DPDK is 
already compiled, that option is not useful. There are certain use cases 
that would benefit from making this value configurable.

This commit addresses the issue by adding a new API to set maximum 
number of memzones before EAL initialization (while using the old 
constant as default value), as well as an API to get current maximum 
number of memzones.

---

What do you think?


>   /* Array of memzone pointers */
> -static const struct rte_memzone *ecore_mz_mapping[RTE_MAX_MEMZONE];
> +static const struct rte_memzone **ecore_mz_mapping;
>   /* Counter to track current memzone allocated */
>   static uint16_t ecore_mz_count;
>   
> +int ecore_mz_mapping_alloc(void)
> +{
> +	ecore_mz_mapping = rte_zmalloc("ecore_mz_map",
> +		rte_memzone_max_get() * sizeof(struct rte_memzone *), 0);

Doesn't this need to check if it's already allocated? Does it need any 
special secondary process handling?

> +
> +	if (!ecore_mz_mapping)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +void ecore_mz_mapping_free(void)
> +{
> +	rte_free(ecore_mz_mapping);

Shouldn't this at least set the pointer to NULL to avoid double-free?

> +#define RTE_DEFAULT_MAX_MEMZONE 2560
> +
> +static size_t memzone_max = RTE_DEFAULT_MAX_MEMZONE;
> +
>   static inline const struct rte_memzone *
>   memzone_lookup_thread_unsafe(const char *name)
>   {
> @@ -81,8 +85,9 @@ memzone_reserve_aligned_thread_unsafe(const char *name, size_t len,
>   	/* no more room in config */
>   	if (arr->count >= arr->len) {
>   		RTE_LOG(ERR, EAL,
> -		"%s(): Number of requested memzone segments exceeds RTE_MAX_MEMZONE\n",
> -			__func__);
> +		"%s(): Number of requested memzone segments exceeds max "
> +		"memzone segments (%d >= %d)\n",

I think the "segments" terminology can be dropped, it is a holdover from 
the times when memzones were not allocated by malloc. The message can 
just say "Number of requested memzones exceeds maximum number of memzones".

> +			__func__, arr->count, arr->len);
>   		rte_errno = ENOSPC;
>   		return NULL;
>   	}
> @@ -396,7 +401,7 @@ rte_eal_memzone_init(void)
>   
>   	if (rte_eal_process_type() == RTE_PROC_PRIMARY &&
>   			rte_fbarray_init(&mcfg->memzones, "memzone",
> -			RTE_MAX_MEMZONE, sizeof(struct rte_memzone))) {
> +			rte_memzone_max_get(), sizeof(struct rte_memzone))) {
>   		RTE_LOG(ERR, EAL, "Cannot allocate memzone list\n");
>   		ret = -1;
>   	} else if (rte_eal_process_type() == RTE_PROC_SECONDARY &&
> @@ -430,3 +435,20 @@ void rte_memzone_walk(void (*func)(const struct rte_memzone *, void *),
>   	}
>   	rte_rwlock_read_unlock(&mcfg->mlock);
>   }
> +
> +int
> +rte_memzone_max_set(size_t max)
> +{
> +	/* Setting max memzone must occur befaore calling rte_eal_init() */
> +	if (eal_get_internal_configuration()->init_complete > 0)
> +		return -1;
> +
> +	memzone_max = max;
> +	return 0;
> +}
> +
> +size_t
> +rte_memzone_max_get(void)
> +{
> +	return memzone_max;
> +}

It seems that this is a local (static) value, which means it is not 
shared between processes, and thus could potentially mismatch between 
two different processes. While this _technically_ would not be a problem 
because secondary process init will not actually use this value, but the 
API will still return incorrect information.

I suggest updating/syncing this value somewhere in 
`eal_mcfg_update_internal()/eal_mcfg_update_from_internal()`, and adding 
this value to mem config. An alternative to that would be to just return 
`mem_config->memzones.count` (instead of the value itself), and return 
-1 (or zero?) if init hasn't yet completed.

-- 
Thanks,
Anatoly