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

[Ophir Munk <ophirmu@nvidia.com>]

> > --- a/lib/eal/common/eal_common_memzone.c
> > +++ b/lib/eal/common/eal_common_memzone.c
> > +#define DEFAULT_MAX_MEMZONE 2560
> 
> Maybe add "_COUNT" at the end to make clear it is not about the size of a
> memzone.
> We should add a comment here to explain the meaning of this default:
> used until the "set" function is called.
> 
> 

Ack

> > -		"%s(): Number of requested memzone segments exceeds
> RTE_MAX_MEMZONE\n",
> > -			__func__);
> > +		"%s(): Number of requested memzone segments exceeds "
> > +		"maximum %u\n", __func__, arr->len);
> 
> We should keep "maximum" on the first line to ease "grep" in the code.
> 

Ack

> > +int
> > +rte_memzone_max_set(size_t max)
> > +{
> > +	struct rte_mem_config *mcfg;
> > +
> > +	if (eal_get_internal_configuration()->init_complete > 0)
> > +		return -1;
> 
> An error log would be needed here I think.
> 

Ack

> > +
> > +	mcfg = rte_eal_get_configuration()->mem_config;
> > +	if (!mcfg)
> 
> Better to use "== NULL" for pointers.
> 
> > +		return -1;
> 

Ack

> Do we need an error log as well?
> 

Yes. Log was added.

> > +
> > +	mcfg = rte_eal_get_configuration()->mem_config;
> > +	if (!mcfg || !mcfg->max_memzone)
> 
> Same comment as above: don't use boolean operator for pointer or value.
> 

Ack

> > --- a/lib/eal/common/eal_memcfg.h
> > +++ b/lib/eal/common/eal_memcfg.h
> > @@ -75,6 +75,8 @@ struct rte_mem_config {
> >  	/**< TSC rate */
> >
> >  	uint8_t dma_maskbits; /**< Keeps the more restricted dma mask. */
> > +
> > +	size_t max_memzone; /**< maximum allowed allocated memzones.
> */
> 
> Uppercase for first work, and we may remove "allowed"?
> Suggestion: "Maximum number of allocated memzones."
> 

Ack

> [...]
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Set max memzone value
> 
> Add a dot at the end.
> Instead of "value", we should mention "number" or "count".
> 

Ack