[v4] lib: set/get max memzone segments

  Currently, the max memzones count constat (RTE_MAX_MEMZONE) 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 the max
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.

Signed-off-by: Ophir Munk <ophirmu@nvidia.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 app/test/test_func_reentrancy.c     |  2 +-
 app/test/test_malloc_perf.c         |  2 +-
 app/test/test_memzone.c             | 42 ++++++++++++++++++++++++-------------
 config/rte_config.h                 |  1 -
 drivers/net/qede/base/bcm_osal.c    | 38 ++++++++++++++++++++++++++-------
 drivers/net/qede/base/bcm_osal.h    |  3 +++
 drivers/net/qede/qede_main.c        |  7 +++++++
 lib/eal/common/eal_common_memzone.c | 38 ++++++++++++++++++++++++++++++---
 lib/eal/common/eal_memcfg.h         |  2 ++
 lib/eal/include/rte_memzone.h       | 30 ++++++++++++++++++++++++++
 lib/eal/version.map                 |  4 ++++
 11 files changed, 141 insertions(+), 28 deletions(-)
  

On 5/24/2023 11:25 PM, Ophir Munk wrote:
> Currently, the max memzones count constat (RTE_MAX_MEMZONE) 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 the max
> 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.
> 
> Signed-off-by: Ophir Munk <ophirmu@nvidia.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> ---


> +
> +int
> +rte_memzone_max_set(size_t max)
> +{
> +	struct rte_mem_config *mcfg;
> +
> +	if (eal_get_internal_configuration()->init_complete > 0)
> +		return -1;
> +
> +	mcfg = rte_eal_get_configuration()->mem_config;
> +	if (!mcfg)
> +		return -1;
> +
> +	mcfg->max_memzone = max;
> +
> +	return 0;
> +}

Would this even work? AFAIR mem_config is only available some time 
during EAL init, not before (mem_config pointer will be NULL at that point).

I suggest the following flow:

set():

if init_complete => return -1
else => set local static value

get():

if init_complete => return memzones.count
else => return local static value (set to our default)

That way we don't actually need the memconfig, and multiprocess will 
work because memzones.count is shared between primary and secondary anyway.
  

On Thu, May 25, 2023 at 12:26 AM Ophir Munk <ophirmu@nvidia.com> wrote:
>
> Currently, the max memzones count constat (RTE_MAX_MEMZONE) 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 the max
> 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.
>
> Signed-off-by: Ophir Munk <ophirmu@nvidia.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>

qede maintainers, can you double check the change on your driver please?
Thanks.
  

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: 26 May 2023 15:25
> To: Devendra Singh Rawat <dsinghrawat@marvell.com>; Alok Prasad <palok@marvell.com>
> Cc: dev@dpdk.org; Bruce Richardson <bruce.richardson@intel.com>; Ophir Munk <ophirmu@nvidia.com>; Matan Azrad
> <matan@nvidia.com>; Thomas Monjalon <thomas@monjalon.net>; Lior Margalit <lmargalit@nvidia.com>
> Subject: [EXT] Re: [PATCH v4] lib: set/get max memzone segments
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Thu, May 25, 2023 at 12:26 AM Ophir Munk <ophirmu@nvidia.com> wrote:
> >
> > Currently, the max memzones count constat (RTE_MAX_MEMZONE) 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 the max
> > 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.
> >
> > Signed-off-by: Ophir Munk <ophirmu@nvidia.com>
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> 
> qede maintainers, can you double check the change on your driver please?
> Thanks.
> 
> 
> --
> David Marchand

Acked-by: Alok Prasad <palok@marvell.com>
  

> -----Original Message-----
> Subject: Re: [PATCH v4] lib: set/get max memzone segments
> 
> On 5/24/2023 11:25 PM, Ophir Munk wrote:
> > Currently, the max memzones count constat (RTE_MAX_MEMZONE) 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 the max
> > 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.
> >
> > Signed-off-by: Ophir Munk <ophirmu@nvidia.com>
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > ---
> 
> > +
> > +int
> > +rte_memzone_max_set(size_t max)
> > +{
> > +	struct rte_mem_config *mcfg;
> > +
> > +	if (eal_get_internal_configuration()->init_complete > 0)
> > +		return -1;
> > +
> > +	mcfg = rte_eal_get_configuration()->mem_config;
> > +	if (!mcfg)
> > +		return -1;
> > +
> > +	mcfg->max_memzone = max;
> > +
> > +	return 0;
> > +}
> 
> Would this even work? 

Yes. It's working. 
I successfully ran the following test:

int max = rte_memzone_max_get(); 
printf("Max memzone before eal_init: %d\n", max);      // Printing the default max value of 2560
rte_memzone_max_set(1000);
max = rte_memzone_max_get();
printf("Max memzone before eal_init after set to 1000: %d\n", max);    // Printing the new max value of 1000
rte_eal_init(argc, argv); 
rte_memzone_max_set(2000); // Here we fail with -1 since we set after eal init
max = rte_memzone_max_get();
printf("Max memzone after eal_init and after set to 2000: %d\n", max);   // Printing the correct max value of 1000

> AFAIR mem_config is only available some time during
> EAL init, not before (mem_config pointer will be NULL at that point).
> 

Please note that DPDK supports early memory config (lib/eal/common/eal_common_config.c):

/* early configuration structure, when memory config is not mmapped */             
static struct rte_mem_config early_mem_config;  

So max memzone is saved in the early memory and later this memory becomes mapped (shared).

Having said that - I think the current patch is correct.
Please confirm.

> I suggest the following flow:
> 
> set():
> 
> if init_complete => return -1

Also if mem_config is NULL => return -1

> else => set local static value
> 

> get():
> 
> if init_complete => return memzones.count else => return local static value (set
> to our default)
> 
> That way we don't actually need the memconfig, and multiprocess will work
> because memzones.count is shared between primary and secondary anyway.
> 
> --
> Thanks,
> Anatoly
  

25/05/2023 00:25, Ophir Munk:
> --- a/config/rte_config.h
> +++ b/config/rte_config.h
> -#define RTE_MAX_MEMZONE 2560

Good to be able to remove this compilation-time configuration.


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


> -		"%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.

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

> +
> +	mcfg = rte_eal_get_configuration()->mem_config;
> +	if (!mcfg)

Better to use "== NULL" for pointers.

> +		return -1;

Do we need an error log as well?

> +
> +	mcfg->max_memzone = max;
> +
> +	return 0;
> +}
> +
> +size_t
> +rte_memzone_max_get(void)
> +{
> +	struct rte_mem_config *mcfg;
> +
> +	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.
 
> +		return DEFAULT_MAX_MEMZONE;
> +
> +	return mcfg->max_memzone;
> +}
> diff --git a/lib/eal/common/eal_memcfg.h b/lib/eal/common/eal_memcfg.h
> index ea013a9..183bb25 100644
> --- 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."

[...]
> +/**
> + * @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".

> + *
> + * This function can only be called prior to rte_eal_init().
> + *
> + * @param max
> + *   Maximum number of memzones
> + * @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
> + *
> + * Get the maximum number of memzones.
> + *
> + * @note: The maximum value will not change after calling rte_eal_init().
> + *
> + * @return
> + *   Maximum number of memzones
> + */
> +__rte_experimental
> +size_t rte_memzone_max_get(void);

Good, thank you.
  

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