On 09/09/2025 13:29, Eelco Chaudron wrote:
>
>
> 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_;
Hi Eelco, thanks for the comments, they are resolved in v3.
>
> 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.
>
You are right of course - never trust a native English speaker to use
the right words
>> + 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);
>
Good idea. I used ds_put_format() with a few modifications from above.
>> + 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?
>
Actually, it's 3 between most of them. I've tidied this up for the ones
I added to be consistent.
>> +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
