Re: [PATCH v11 00/21]

[David Marchand <david.marchand@redhat.com>]

Hello Bruce,

On Thu, 9 Oct 2025 at 15:01, Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> The ultimate of this patchset is to make it easier to run on systems
> with large numbers of cores, by simplifying the process of using core
> numbers >RTE_MAX_LCORE. The new EAL arg ``-remap-lcore-ids``, also
> shortened to ``-R``, is added to DPDK to support this.
>
> However, in order to add this new flag easily, the first dozen or more
> patches rework the argument handling in EAL to simplify things, using
> the argparse library for argument handling.
>
> When processing cmdline arguments in DPDK, we always do so with very
> little context. So, for example, when processing the "-l" flag, we have
> no idea whether there will be later a --proc-type=secondary flag. We
> have all sorts of post-arg-processing checks in place to try and catch
> these scenarios.
>
> To improve this situation, this patchset tries to simplify the handling
> of argument processing, by explicitly doing an initial pass to collate
> all arguments into a structure. Thereafter, the actual arg parsing is
> done in a fixed order, meaning that e.g. when processing the
> --main-lcore flag, we have already processed the service core flags. We
> also can far quicker and easier check for conflicting options, since
> they can all be checked for NULL/non-NULL in the arg structure
> immediately after the struct has been populated.
>
> An additional benefit of this work is that the argument parsing for EAL
> is much more centralised into common options and the options list file.
> This single list with ifdefs makes it clear to the viewer what options
> are common across OS's, vs what are unix-only or linux-only.
>
> Once the cleanup and rework is done, adding the new options for
> remapping cores becomes a lot simpler, since we can very easily check
> for scenarios like multi-process and handle those appropriately.
>
>
> V11:
> * fix issues flagged by unit tests in CI and subsequent testing:
>   - when passing in an lcore >= MAX_LCORES, return error rather than
>     ignoring it. (compatibility issue)
>   - return error when an invalid lcore set of "1-3-5" is passed in,
>     rather than just treating it as "3-5".

I did some tweaking on the series (and put my sob for taking the
bullet if I broke something ;-)), namely:
- squashed the init arg list patch into the initial patch that
introduces eal_option_list.h,
- inverted order of the patch on coremask rework with the one
introducing lcore remapping,
- I updated patch 6 as Chengwen requested, and I fixed return codes
for parse_arg_corelist(),
- I updated the doc for patch 8 as Chengwen requested,

I fixed a few reintroductions of socket-mem (should be numa-mem) in
intermediate patches.

I noticed that the leak reported earlier on patch "eal: gather EAL
args before processing" is still present when stopping at this commit,
and it is fixed in the next commit.
We could have avoid this transient issue, but I did not spend time to
fix as it is just a leak in the event wrong EAL options are passed.

There were some little checkpatch issues I fixed (plus some spurious
empty lines/spaces).
But I left the options definitions as is (wrt line length warning).

Wrt storing the cores as a fixed size cpuset, this storage is internal
and we can change in the future (no ABI concern afaics).

Series applied.

Thanks Chengwen for the reviews on argparse.

Thanks Bruce, this was kind of an unexpected long road.
This is a nice cleanup and I like this auto magic option and the debug logs.


I have two questions which could be addressed in followup patches but
seem more risky than what I touched, and require a new round of CI:
- are we missing a build check on RTE_MAX_LCORE < CPU_SETSIZE?
- should eal_clean_saved_args() be called in rte_eal_cleanup()?


And finally my dumb idea:

Do you think it would be feasible to extend this remapping mechanism
for multi process?
I would like to start all processes with only the -R option (and each
application has a dedicated cpu affinity, set by an external mechanism
out of DPDK).
Then some exchanges between primary and secondary processes are done
at init, with secondary announcing a number of lcores it needs, and
the primary replying with a lcoreid base for remapping.
One problem is that it would require tracking life and death of the
secondary processes to that primary can reallocate unused lcores
ranges.


-- 
David Marchand