On 8 Sep 2025, at 17:26, Kevin Traynor via dev 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.
>
> The core list is validated to prevent invalid cores being passed to
> DPDK rte_eal_init().
>
> 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]>
> ---
> lib/dpdk.c | 76 +++++++++++++++++++++++++++++++++++--
> tests/pmd.at | 15 --------
> tests/system-dpdk-macros.at | 15 ++++++++
> tests/system-dpdk.at | 57 ++++++++++++++++++++++++++++
> tests/testsuite.at | 1 +
> 5 files changed, 145 insertions(+), 19 deletions(-)
>
> diff --git a/lib/dpdk.c b/lib/dpdk.c
> index a6f82dd8a..0256b6d23 100644
> --- a/lib/dpdk.c
> +++ b/lib/dpdk.c
> @@ -65,4 +65,58 @@ args_contains(const struct svec *args, const char *value)
> }
>
> +static int
> +compare_core_ids(const void *a_, const void *b_)
> +{
> + const unsigned *a = a_;
> + const unsigned *b = b_;
I would add a newline here.
> + return *a < *b ? -1 : *a > *b;
> +}
> +
> +/* Converts a hexadecimal core mask to DPDK lcore list format.
> + * Returns NULL if no valid bits are set in the mask. */
> +static char *
> +cmask_to_lcore_list(const char *cmask)
> +{
> + struct ovs_numa_dump *lcore_dump;
> + struct ovs_numa_info_core *core;
> + unsigned int *core_ids = NULL;
> + unsigned int num_cores;
> + char *lcores = NULL;
> + unsigned int i = 0;
> +
> + lcore_dump = ovs_numa_dump_cores_with_cmask(cmask);
> +
> + num_cores = ovs_numa_dump_count(lcore_dump);
> + core_ids = xmalloc(num_cores * sizeof *core_ids);
> +
> + FOR_EACH_CORE_ON_DUMP(core, lcore_dump) {
We seem to use spaces after macro loops, so
FOR_EACH_CORE_ON_DUMP (core, lcore_dump) {
> + core_ids[i++] = core->core_id;
> + }
> + ovs_numa_dump_destroy(lcore_dump);
> +
> + qsort(core_ids, num_cores, sizeof *core_ids, compare_core_ids);
> +
> + if (num_cores > RTE_MAX_LCORE) {
> + VLOG_INFO("Concatenating DPDK lcore list as dpdk-lcore-mask has >"
> + " %u lcores", RTE_MAX_LCORE);
Might be my English, but concatenating means combining to me, so what are we
combining? We are truncating the list here.
> + num_cores = RTE_MAX_LCORE;
> + }
> +
> + for (i = 0; i < num_cores; i++) {
> + char *new_lcores = NULL;
> +
> + new_lcores = xasprintf("%s%d@%u,", lcores ? lcores : "", i,
> + core_ids[i]);
> + free(lcores);
This allocating, copying, freeing, seems very inefficient; maybe we should use
the ds_put_format() family here?
Not tested;
ds_init(&lcores_ds);
for (i = 0; i < num_cores; i++) {
ds_put_format(&lcores_ds, “%s%d@%u”, i ? “,” : “”, i, core_ids[i]);
}
free(core_ids);
return ds_steal_cstr(&lcores_ds);
> + lcores = new_lcores;
> + }
> + if (lcores) {
> + lcores[strlen(lcores) - 1] = '\0';
> + }
> +
> + free(core_ids);
> + return lcores;
> +}
> +
> static void
> construct_dpdk_options(const struct smap *ovs_other_config, struct svec
> *args)
> @@ -71,8 +125,9 @@ 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 *);
> } opts[] = {
> - {"dpdk-lcore-mask", "-c" },
> - {"dpdk-hugepage-dir", "--huge-dir" },
> - {"dpdk-socket-limit", "--socket-limit"},
> + {"dpdk-lcore-mask", "--lcores", cmask_to_lcore_list},
> + {"dpdk-hugepage-dir", "--huge-dir", NULL},
> + {"dpdk-socket-limit", "--socket-limit", NULL},
> };
>
> @@ -86,9 +141,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);
> }
> }
> diff --git a/tests/pmd.at b/tests/pmd.at
> index 35a44b4df..8254ac3b0 100644
> --- a/tests/pmd.at
> +++ b/tests/pmd.at
> @@ -27,19 +27,4 @@ flow_dump_prepend_pmd () {
> m4_divert_pop([PREPARE_TESTS])
>
> -dnl CHECK_CPU_DISCOVERED([n_cpu])
> -dnl
> -dnl Waits until CPUs discovered and checks if number of discovered CPUs
> -dnl is greater or equal to 'n_cpu'. Without parameters checks that at
> -dnl least one CPU discovered.
> -m4_define([CHECK_CPU_DISCOVERED], [
> - PATTERN="Discovered [[0-9]]* NUMA nodes and [[0-9]]* CPU cores"
> - OVS_WAIT_UNTIL([grep "$PATTERN" ovs-vswitchd.log])
> - N_CPU=$(grep "$PATTERN" ovs-vswitchd.log | sed -e 's/.* \([[0-9]]*\) CPU
> cores/\1/')
> - if [[ -z "$1" ]]
> - then AT_CHECK([test "$N_CPU" -gt "0"])
> - else AT_SKIP_IF([test "$N_CPU" -lt "$1"])
> - fi
> -])
> -
> dnl CHECK_PMD_THREADS_CREATED([n_threads], [numa_id], [+line])
> dnl
> diff --git a/tests/system-dpdk-macros.at b/tests/system-dpdk-macros.at
> index f8ba76673..aae49d90f 100644
> --- a/tests/system-dpdk-macros.at
> +++ b/tests/system-dpdk-macros.at
> @@ -200,2 +200,17 @@ m4_define([CONFIGURE_VETH_OFFLOADS],
> AT_CHECK([ethtool -K $1 txvlan off], [0], [ignore], [ignore])]
> )
> +
> +dnl CHECK_CPU_DISCOVERED([n_cpu])
> +dnl
> +dnl Waits until CPUs discovered and checks if number of discovered CPUs
> +dnl is greater or equal to 'n_cpu'. Without parameters checks that at
> +dnl least one CPU discovered.
I know this is moved around, but maybe clean up the comments while at it:
+dnl Waits until CPUs are discovered and checks if the number of discovered CPUs
+dnl is greater or equal to 'n_cpu'. Without the 'n_cpu' parameter checks that
+dnl at least one CPU is discovered.
> +m4_define([CHECK_CPU_DISCOVERED], [
> + PATTERN="Discovered [[0-9]]* NUMA nodes and [[0-9]]* CPU cores"
> + OVS_WAIT_UNTIL([grep "$PATTERN" ovs-vswitchd.log])
> + N_CPU=$(grep "$PATTERN" ovs-vswitchd.log | sed -e 's/.* \([[0-9]]*\) CPU
> cores/\1/')
Make this line wrap;
+ N_CPU=$(grep "$PATTERN" ovs-vswitchd.log \
+ | sed -e 's/.* \([[0-9]]*\) CPU cores/\1/')
> + if [[ -z "$1" ]]
> + then AT_CHECK([test "$N_CPU" -gt "0"])
> + else AT_SKIP_IF([test "$N_CPU" -lt "$1"])
> + fi
> +])
> \ No newline at end of file
> diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
> index e79c75565..2294098f3 100644
> --- a/tests/system-dpdk.at
> +++ b/tests/system-dpdk.at
> @@ -52,7 +52,64 @@ AT_CLEANUP
> dnl
> --------------------------------------------------------------------------
All tests have two new lines between tests, guess we want to keep it the same
for all tests?
> +dnl Check dpdk-lcore-mask conversion for only first bit
> +AT_SETUP([OVS-DPDK - dpdk-lcore-mask conversion - single])
> +AT_KEYWORDS([dpdk])
> +OVS_DPDK_PRE_CHECK()
> +OVS_DPDK_START_OVSDB()
> +OVS_DPDK_START_VSWITCHD([--no-pci])
> +CHECK_CPU_DISCOVERED()
> +AT_CHECK([ovs-vsctl --no-wait set Open_vSwitch .
> other_config:dpdk-lcore-mask=0x1])
> +AT_CHECK([ovs-vsctl --no-wait set Open_vSwitch .
> other_config:dpdk-init=true])
> +expected_lcores="lcores 0@0"
> +OVS_WAIT_UNTIL([grep "$expected_lcores" ovs-vswitchd.log])
> +OVS_DPDK_STOP_VSWITCHD
> +AT_CLEANUP
> +dnl
> --------------------------------------------------------------------------
>
> +dnl Check dpdk-lcore-mask conversion for only multiple bits
> +AT_SETUP([OVS-DPDK - dpdk-lcore-mask conversion - multi])
> +AT_KEYWORDS([dpdk])
> +OVS_DPDK_PRE_CHECK()
> +OVS_DPDK_START_OVSDB()
> +OVS_DPDK_START_VSWITCHD([--no-pci])
> +CHECK_CPU_DISCOVERED(4)
> +AT_CHECK([ovs-vsctl --no-wait set Open_vSwitch .
> other_config:dpdk-lcore-mask=0xf])
> +AT_CHECK([ovs-vsctl --no-wait set Open_vSwitch .
> other_config:dpdk-init=true])
> +expected_lcores="lcores 0@0,1@1,2@2,3@3"
> +OVS_WAIT_UNTIL([grep "$expected_lcores" ovs-vswitchd.log])
> +OVS_DPDK_STOP_VSWITCHD
> +AT_CLEANUP
> +dnl
> --------------------------------------------------------------------------
>
> +dnl Check dpdk-lcore-mask conversion for max length string
> +AT_SETUP([OVS-DPDK - dpdk-lcore-mask conversion - non-contig])
> +AT_KEYWORDS([dpdk])
> +OVS_DPDK_PRE_CHECK()
> +OVS_DPDK_START_OVSDB()
> +OVS_DPDK_START_VSWITCHD([--no-pci])
> +CHECK_CPU_DISCOVERED(8)
> +AT_CHECK([ovs-vsctl --no-wait set Open_vSwitch .
> other_config:dpdk-lcore-mask=0xca])
> +AT_CHECK([ovs-vsctl --no-wait set Open_vSwitch .
> other_config:dpdk-init=true])
> +expected_lcores="lcores 0@1,1@3,2@6,3@7"
> +OVS_WAIT_UNTIL([grep "$expected_lcores" ovs-vswitchd.log])
> +OVS_DPDK_STOP_VSWITCHD
> +AT_CLEANUP
> dnl
> --------------------------------------------------------------------------
> +
> +dnl Check dpdk-lcore-mask conversion for zero
> +AT_SETUP([OVS-DPDK - dpdk-lcore-mask conversion - zeromask])
> +AT_KEYWORDS([dpdk])
> +OVS_DPDK_PRE_CHECK()
> +OVS_DPDK_START_OVSDB()
> +OVS_DPDK_START_VSWITCHD([--no-pci])
> +AT_CHECK([ovs-vsctl --no-wait set Open_vSwitch .
> other_config:dpdk-lcore-mask=0x0])
> +AT_CHECK([ovs-vsctl --no-wait set Open_vSwitch .
> other_config:dpdk-init=true])
> +OVS_WAIT_UNTIL([grep "Ignoring database defined option 'dpdk-lcore-mask' due
> to invalid value '0x0'" ovs-vswitchd.log])
> +expected_lcores="lcores 0@0"
> +OVS_WAIT_UNTIL([grep "$expected_lcores" ovs-vswitchd.log])
> +OVS_DPDK_STOP_VSWITCHD(["/Ignoring database defined option/d"])
> +AT_CLEANUP
> +dnl
> --------------------------------------------------------------------------
> +
> dnl Add standard DPDK PHY port
> AT_SETUP([OVS-DPDK - add standard DPDK port])
> diff --git a/tests/testsuite.at b/tests/testsuite.at
> index 9d77a9f51..51e12583b 100644
> --- a/tests/testsuite.at
> +++ b/tests/testsuite.at
> @@ -20,4 +20,5 @@ m4_include([tests/ovs-macros.at])
> m4_include([tests/ovsdb-macros.at])
> m4_include([tests/ofproto-macros.at])
> +m4_include([tests/system-dpdk-macros.at])
>
> m4_include([tests/completion.at])
> --
> 2.51.0
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
