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
