On 01/09/2025 13:54, David Marchand wrote:
> 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.
>
Thanks for the review David.
>
>> ---
>> 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?
>
Yes, I reworked to reuse ovs_numa_dump_cores_with_cmask() with a wrapper
above it for the specifics of what is needed here in v2.
>
>> + 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';).
>
Thanks for this suggestion. I did something like this with additional
modifications.
>
>> + }
>> + 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.
>
Sure, it is now patch 2/3 before the conversion in 3/3
>>
>> @@ -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);
>> }
>> }
>
>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
