From: "Burakov, Anatoly" <anatoly.burakov@intel.com>
To: Ophir Munk <ophirmu@nvidia.com>, <dev@dpdk.org>,
Bruce Richardson <bruce.richardson@intel.com>,
Devendra Singh Rawat <dsinghrawat@marvell.com>,
Alok Prasad <palok@marvell.com>
Cc: Ophir Munk <ophirmu@mellanox.com>, Matan Azrad <matan@nvidia.com>,
"Thomas Monjalon" <thomas@monjalon.net>,
Lior Margalit <lmargalit@nvidia.com>
Subject: Re: [PATCH V3] lib: set/get max memzone segments
Date: Thu, 18 May 2023 16:54:27 +0100 [thread overview]
Message-ID: <84723e01-f3f1-203c-55e3-bc73da9ff75c@intel.com> (raw)
In-Reply-To: <20230503072641.474600-1-ophirmu@nvidia.com>
Hi,
On 5/3/2023 8:26 AM, Ophir Munk wrote:
> In current DPDK the RTE_MAX_MEMZONE definition is unconditionally hard
> coded as 2560. For applications requiring different values of this
> parameter – it is more convenient to set the max value via an rte API -
> rather than changing the dpdk source code per application. In many
> organizations, the possibility to compile a private DPDK library for a
> particular application does not exist at all. With this option there is
> no need to recompile DPDK and it allows using an in-box packaged DPDK.
> An example usage for updating the RTE_MAX_MEMZONE would be of an
> application that uses the DPDK mempool library which is based on DPDK
> memzone library. The application may need to create a number of
> steering tables, each of which will require its own mempool allocation.
> This commit is not about how to optimize the application usage of
> mempool nor about how to improve the mempool implementation based on
> memzone. It is about how to make the max memzone definition - run-time
> customized.
> 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
> (RTE_MAX_MEMZONE) is used. There is also an API to query the effective
> max memzone: rte_memzone_max_get().
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?
> /* 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? Does it need any
special secondary process handling?
> +
> + 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?
> +#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".
> + __func__, arr->count, arr->len);
> rte_errno = ENOSPC;
> return NULL;
> }
> @@ -396,7 +401,7 @@ rte_eal_memzone_init(void)
>
> if (rte_eal_process_type() == RTE_PROC_PRIMARY &&
> rte_fbarray_init(&mcfg->memzones, "memzone",
> - RTE_MAX_MEMZONE, sizeof(struct rte_memzone))) {
> + rte_memzone_max_get(), sizeof(struct rte_memzone))) {
> RTE_LOG(ERR, EAL, "Cannot allocate memzone list\n");
> ret = -1;
> } else if (rte_eal_process_type() == RTE_PROC_SECONDARY &&
> @@ -430,3 +435,20 @@ void rte_memzone_walk(void (*func)(const struct rte_memzone *, void *),
> }
> rte_rwlock_read_unlock(&mcfg->mlock);
> }
> +
> +int
> +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.
--
Thanks,
Anatoly
next prev parent reply other threads:[~2023-05-18 15:55 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-19 8:36 [RFC] " Ophir Munk
2023-04-19 8:48 ` Ophir Munk
2023-04-19 13:42 ` [EXT] " Devendra Singh Rawat
2023-04-24 21:07 ` Ophir Munk
2023-04-19 14:42 ` Stephen Hemminger
2023-04-24 21:43 ` Ophir Munk
2023-04-19 14:51 ` Tyler Retzlaff
2023-04-20 7:43 ` Thomas Monjalon
2023-04-20 18:20 ` Tyler Retzlaff
2023-04-21 8:34 ` Thomas Monjalon
2023-04-21 11:08 ` Morten Brørup
2023-04-21 14:57 ` Thomas Monjalon
2023-04-21 15:19 ` Morten Brørup
2023-04-25 16:38 ` Ophir Munk
2023-04-25 13:46 ` Ophir Munk
2023-04-25 16:40 ` [RFC V2] " Ophir Munk
2023-05-03 7:26 ` [PATCH V3] " Ophir Munk
2023-05-03 21:41 ` Morten Brørup
2023-05-25 6:47 ` Ophir Munk
2023-05-04 7:27 ` David Marchand
2023-05-25 6:35 ` Ophir Munk
2023-05-18 15:54 ` Burakov, Anatoly [this message]
2023-05-25 6:43 ` Ophir Munk
2023-05-24 22:25 ` [PATCH v4] " Ophir Munk
2023-05-25 14:53 ` Burakov, Anatoly
2023-05-30 11:37 ` Ophir Munk
2023-05-26 9:55 ` David Marchand
2023-05-28 12:09 ` [EXT] " Alok Prasad
2023-05-30 13:32 ` Thomas Monjalon
2023-05-31 7:56 ` Ophir Munk
2023-05-31 7:52 ` [PATCH V5] " Ophir Munk
2023-05-31 8:41 ` [PATCH V6] " Ophir Munk
2023-06-05 8:52 ` [PATCH V7] " Ophir Munk
2023-06-05 10:50 ` [PATCH V8] " Ophir Munk
2023-06-05 16:50 ` Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=84723e01-f3f1-203c-55e3-bc73da9ff75c@intel.com \
--to=anatoly.burakov@intel.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=dsinghrawat@marvell.com \
--cc=lmargalit@nvidia.com \
--cc=matan@nvidia.com \
--cc=ophirmu@mellanox.com \
--cc=ophirmu@nvidia.com \
--cc=palok@marvell.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).