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

[Ophir Munk <ophirmu@nvidia.com>]

Hi Anatoly,
Thank you for your review. I noticed a review by David Marchand that addresses similar points to yours. In V4 I supply a reply to you both.

> 
> 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?
> 

Ack.

> 
> >   /* 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? 

Ack. Issue is addressed with a ref count.

> Does it need any special secondary process handling?
> 

No. 

> > +
> > +	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?
> 

Ack

> > +#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".
> 

Ack

> > +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.
> 

Static variable removed.
I opted the second alternative, but if init hasn't yet completed the return value is the default (2560) rather than -1 or 0.

> --
> Thanks,
> Anatoly