Hello Kevin,
On Thu, 28 Aug 2025 at 17:44, Kevin Traynor <[email protected]> wrote:
>
> OVS currently uses other_config:dpdk-lcore-mask <coremask> directly
> in DPDK rte_eal_init() with '-c <coremask>' argument.
>
> '-c' argument is now deprecated from DPDK and will be removed in
> DPDK 25.11, so OVS will no longer be able to use the '-c <coremask>'
> argument.
>
> Convert dpdk-lcore-mask core mask to a core list that can be used with
> '--lcores' and add some tests.
>
> Using the '--lcores' argument also adds compability for using a core
> in the core mask that is greater than the max lcore, similar to commit
> fe53b478f86e ("dpdk: Fix main lcore on systems with many cores.")
>
> Signed-off-by: Kevin Traynor <[email protected]>
Thanks for working on this topic.
I have some comments.
> ---
> lib/dpdk.c | 76 +++++++++++++++++++++++++++++++++++--
> tests/pmd.at | 15 --------
> tests/system-dpdk-macros.at | 15 ++++++++
> tests/system-dpdk.at | 73 +++++++++++++++++++++++++++++++++++
> tests/testsuite.at | 1 +
> 5 files changed, 161 insertions(+), 19 deletions(-)
>
> diff --git a/lib/dpdk.c b/lib/dpdk.c
> index 1f4f2bf08..a3e397f3c 100644
> --- a/lib/dpdk.c
> +++ b/lib/dpdk.c
> @@ -65,4 +65,58 @@ args_contains(const struct svec *args, const char *value)
> }
>
> +#define LCORE_MAX_STR_SIZE 23 /* Max size of single lcore assignment.
> */
> +#define LCORE_MAX_ARGS_LENGTH 256 /* Max size of DPDK lcores list. */
> +
> +/* Converts a hexadecimal core mask to DPDK lcore list format. */
> +static char *
> +cmask_to_lcore_list(const char *cmask)
> +{
> + char cvalue[LCORE_MAX_STR_SIZE] = "";
> + char *lcores = NULL;
> + int num_lcores = 0;
> + int core_id = 0;
> + int end_idx = 0;
> +
> + /* Ignore leading 0x. */
> + if (!strncmp(cmask, "0x", 2) || !strncmp(cmask, "0X", 2)) {
> + end_idx = 2;
> + }
> +
> + for (int i = strlen(cmask) - 1; i >= end_idx; i--) {
> + char hex = cmask[i];
> + int bin;
> +
> + bin = hexit_value(hex);
> + if (bin == -1) {
> + VLOG_WARN("Invalid lcore mask: %c", cmask[i]);
> + bin = 0;
> + }
> + for (int j = 0; j < 4; j++) {
> + if ((bin >> j) & 0x1) {
This two nested loops stuff seems quite close to
ovs_numa_dump_cores_with_cmask().
Could we use it instead?
> + if (!num_lcores) {
> + lcores = xmalloc(LCORE_MAX_STR_SIZE);
> + lcores[0] = '\0';
> + snprintf(cvalue, LCORE_MAX_STR_SIZE, "0@%d", core_id);
> + num_lcores++;
> + } else {
> + int new_size = strlen(lcores) + LCORE_MAX_STR_SIZE;
> +
> + if (new_size > LCORE_MAX_ARGS_LENGTH) {
> + VLOG_INFO("Concatenating DPDK lcore list from"
> + " dpdk-lcore-mask as string too long.");
> + return lcores;
> + }
> + lcores = xrealloc(lcores, new_size);
> + snprintf(cvalue, LCORE_MAX_STR_SIZE, ",%d@%d",
> + num_lcores++, core_id);
> + }
> + strncat(lcores, cvalue, LCORE_MAX_STR_SIZE);
Rather than check for the first lcore and use fixed sized arrays, we
can do an unconditional:
char *new_lcores;
xasprintf(&new_lcores, "%s%d@%u,", lcores, num_lcores++, j);
free(lcores);
lcores = new_lcores;
And then after the loop, remove the trailing comma (something like
lcores[strlen(lcores) - 1] = '\0';).
> + }
> + core_id++;
> + }
> + }
> + return lcores;
> +}
> +
> static void
> construct_dpdk_options(const struct smap *ovs_other_config, struct svec
> *args)
> @@ -71,10 +125,11 @@ construct_dpdk_options(const struct smap
> *ovs_other_config, struct svec *args)
> const char *ovs_configuration;
> const char *dpdk_option;
> + char *(*param_conversion) (const char *);
> bool default_enabled;
> const char *default_value;
> } opts[] = {
> - {"dpdk-lcore-mask", "-c", false, NULL},
> - {"dpdk-hugepage-dir", "--huge-dir", false, NULL},
> - {"dpdk-socket-limit", "--socket-limit", false, NULL},
> + {"dpdk-lcore-mask", "--lcores", cmask_to_lcore_list, false, NULL},
> + {"dpdk-hugepage-dir", "--huge-dir", NULL, false, NULL},
> + {"dpdk-socket-limit", "--socket-limit", NULL, false, NULL},
> };
Not really the subject of this patch.. but the whole default_* stuff
is not needed (was it ever?).
Another thing that seems strange to me is that the conversion helper
gets called with a "default" value.
So since it is unneeed, we might as well remove this (as a preparation
patch?) before adding the conversion helper.
>
> @@ -91,9 +146,22 @@ construct_dpdk_options(const struct smap
> *ovs_other_config, struct svec *args)
> if (value) {
> if (!args_contains(args, opts[i].dpdk_option)) {
> + char *dpdk_val = NULL;
> +
> + if (opts[i].param_conversion) {
> + dpdk_val = (opts[i].param_conversion)(value);
> + if (!dpdk_val) {
> + VLOG_WARN("Ignoring database defined option '%s'"
> + " due to invalid value '%s'",
> + opts[i].ovs_configuration, value);
> + continue;
> + }
> + value = dpdk_val;
> + }
> svec_add(args, opts[i].dpdk_option);
> svec_add(args, value);
> + free(dpdk_val);
> } else {
> VLOG_WARN("Ignoring database defined option '%s' due to "
> - "dpdk-extra config", opts[i].dpdk_option);
> + "dpdk-extra config", opts[i].ovs_configuration);
> }
> }
--
David Marchand
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
