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.march...@redhat.com>
> Sent: Thursday, 4 May 2023 10:27
> To: Ophir Munk <ophi...@nvidia.com>
> Cc: dev@dpdk.org; Bruce Richardson <bruce.richard...@intel.com>; Devendra
> Singh Rawat <dsinghra...@marvell.com>; Alok Prasad <pa...@marvell.com>;
> Ophir Munk <ophi...@mellanox.com>; Matan Azrad <ma...@nvidia.com>;
> NBU-Contact-Thomas Monjalon (EXTERNAL) <tho...@monjalon.net>; Lior
> Margalit <lmarga...@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 <ophi...@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
