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

[Ophir Munk <ophirmu@nvidia.com>]

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

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, 4 May 2023 10:27
> To: Ophir Munk <ophirmu@nvidia.com>
> Cc: dev@dpdk.org; Bruce Richardson <bruce.richardson@intel.com>; Devendra
> Singh Rawat <dsinghrawat@marvell.com>; Alok Prasad <palok@marvell.com>;
> Ophir Munk <ophirmu@mellanox.com>; Matan Azrad <matan@nvidia.com>;
> NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>; Lior
> Margalit <lmargalit@nvidia.com>
> Subject: Re: [PATCH V3] lib: set/get max memzone segments
> 
> Hello Ophir,
> 
> Good thing someone is looking into this.
> Thanks.
> 
> I have a few comments.
> 
> 
> This commitlog is a bit compact.
> Separating it with some empty lines would help digest it.
> 
> 
> 
> The code was using a rather short name "RTE_MAX_MEMZONE".
> But I prefer we spell this as "max memzones count" (or a better wording), in
> the descriptions/comments.
> 
> 

Ack. Commit message updated.

> > 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
> 
> Afaics, this prototype got unaligned with the patch content, as a size_t is now
> taken as input.
> You can simply mention rte_memzone_max_set().
> 
> 

Ack

> > (RTE_MAX_MEMZONE) is used.  There is also an API to query the
> > effective
> 
> After the patch RTE_MAX_MEMZONE does not exist anymore.
> 

Ack

> 
> > max memzone: rte_memzone_max_get().
> >
> > Signed-off-by: Ophir Munk <ophirmu@nvidia.com>
> 
> 
> A global comment on the patch:
> 
> rte_calloc provides what you want in all cases below: an array of objects.
> I prefer rte_calloc(count, sizeof elem) to rte_zmalloc(count * sizeof elem).
> 
> This also avoids a temporary variable to compute the total size of such an
> array.
> 

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);
> 
> I think we should allocate only for the first call and we are missing some
> refcount.
> 
> 

Ack. This allocation occurs as part of qed_probe. I added a ref count.

> > +
> > +       if (!ecore_mz_mapping)
> > +               return -ENOMEM;
> > +
> > +       return 0;
> > +}
> > +
> > +void ecore_mz_mapping_free(void)
> > +{
> > +       rte_free(ecore_mz_mapping);
> 
> Won't we release this array while another qed device is still in use?
> 
> 

Handled with the ref count.

> >
> > +#define RTE_DEFAULT_MAX_MEMZONE 2560
> 
> No need for a RTE_ prefix for a local macro and ...
> 

Ack

> 
> > +
> > +static size_t memzone_max = RTE_DEFAULT_MAX_MEMZONE;
> 
> ... in the end, I see no need for the RTE_DEFAULT_MAX_MEMZONE macro at
> all, it is only used as the default init value, here.
> 
> 

I prefer leaving the macro as it explains the value context.

> > +
> >  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",
> 
> Won't we always display this log for the case when arr->count == arr->len ?
> It is then pointless to display both arr->count and arr->len (which should be
> formatted as %u).
> 
> I would simply log:
> RTE_LOG(ERR, EAL,
>         "%s(): Number of requested memzone segments exceeds maximum
> %u\n",
>         __func__, arr->len);
> 

Ack

> 
> > +{
> > +       /* Setting max memzone must occur befaore calling
> > +rte_eal_init() */
> 
> before*
> 

Ack. Thanks.

> This comment would be better placed in the API description than in the
> implementation.
> 

Ack

> 
> > +       if (eal_get_internal_configuration()->init_complete > 0)
> > +               return -1;
> > +
> > diff --git a/lib/eal/include/rte_memzone.h
> > b/lib/eal/include/rte_memzone.h index 5302caa..3ff7c73 100644
> > --- a/lib/eal/include/rte_memzone.h
> > +++ b/lib/eal/include/rte_memzone.h
> > @@ -305,6 +305,26 @@ void rte_memzone_dump(FILE *f);  void
> > rte_memzone_walk(void (*func)(const struct rte_memzone *, void *arg),
> >                       void *arg);
> >
> > +/**
> 
>  * @warning
>  * @b EXPERIMENTAL: this API may change without prior notice
> 

Ack

> > + * Set max memzone value
> > + *
> > + * @param max
> > + *   Value of max memzone allocations
> 
> I'd rather describe as:
> "Maximum number of memzones"
> 
> Please also mention that this function can only be called prior to rte_eal_init().
> 
> 

Ack

> > + * @return
> > + *  0 on success, -1 otherwise
> > + */
> > +__rte_experimental
> > +int rte_memzone_max_set(size_t max);
> > +
> > +/**
> 
>  * @warning
>  * @b EXPERIMENTAL: this API may change without prior notice
> 

Ack

> > + * Get max memzone value
> 
> Get the maximum number of memzones.
> 
> And we can remind the developer, here, that this value won't change after
> rte_eal_init.
> 
> 

Ack

> >  };
> >
> >  INTERNAL {
> > --
> > 2.8.4
> >
> 
> 
> --
> David Marchand