Re: [PATCH v9 00/18] Simplify running with high-numbered CPUs

[Bruce Richardson <bruce.richardson@intel.com>]

On Mon, Oct 06, 2025 at 04:10:52PM +0200, David Marchand wrote:
> On Fri, 3 Oct 2025 at 10:15, 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.
> >
> > V9: rebase to latest main. CI complains cannot apply v8 patches.
> >
> > V8:
> > * dropped the final two patches from the series, dropping the new -L
> >   option in favour of the -R modifier.
> > * reordered patch 11 to be with the other argparse patches (now patch 5)
> > * added patch 12, which uses macros to initialize the args structure
> >   from the arguments header file, avoiding potential issues when we add
> >   new args.
> > * simplified and consolidated lcore mask and core list parsing to always
> >   work off cpusets rather than arrays of uint8
> > * enhanced debug printouts to also work better with cpusets and handle
> >   core values in those sets >= RTE_MAX_LCORE
> > * for completeness, ensure the new -R option works for coremasks, and
> >   for cases where no explicit core-list or coremask is specified.
> 
> I was planning to merge the first part of the series (before reaching
> the cpuset rework and addition of remap option).
> 
> I am facing two issues for which I prefer other's opinions.
> 
> 
> - First, I see a change in how non-option arguments are handled with
> the switch to argparse.
> $ ./build-mini/app/dpdk-test --no-huge -m 2048 -l 0,1 func_reentrancy_autotest
> ARGPARSE: too many positional arguments func_reentrancy_autotest!
> 
> Passing the test name after -- does work, but it was working without
> -- before the patch, so we are introducing a regression here.
> 

Looking for input on how much we need to handle here.

The quick patch below adds support for ignoring trailing args in argparse,
which means that the command above would work. However, gnu getopt also
does argument reordering which means that this command works right now (on
linux anyway):

	./build/app/dpdk-test -l 0,1 lcores_autotest --no-huge

since getopt moves the lcores_autotest to the end.

Is this behaviour what we need to emulate too, or is adding a flag to
ignore trailing args sufficient? [I'd tend toward it being sufficient - mixing
app args and EAL args together is not a great idea IMHO]


> 
> - Second, stopping at patch "eal: gather EAL args before processing",
> I see a crash in this same func_reentrancy_autotest unit test.
> 
> Here, I would fix the unit test itself, as it passes a NULL argv[0]
> which is invalid according to the C standard.

For this, I think the easiest (and most correct) fix is just to catch the
null argv[0] condition and return error, rather than actually trying to
continue arg parsing. I see right now that my patches only check for
argc==0 and argv==NULL. I suggest just extending that to also check that
final argv[0] == NULL condition.

/Bruce

diff --git a/lib/argparse/rte_argparse.c b/lib/argparse/rte_argparse.c
index e4851bd046..de57400e74 100644
--- a/lib/argparse/rte_argparse.c
+++ b/lib/argparse/rte_argparse.c
@@ -695,8 +695,22 @@ parse_args(const struct rte_argparse *obj, bool *arg_parsed,
                        /* process positional parameters. */
                        position_index++;
                        if (position_index > position_count) {
-                               ARGPARSE_LOG(ERR, "too many positional arguments %s!", curr_argv);
-                               return -EINVAL;
+                               if (!obj->ignore_trailing_args) {
+                                       ARGPARSE_LOG(ERR, "too many positional arguments %s!",
+                                               curr_argv);
+                                       return -EINVAL;
+                               }
+                               /* check if this is a trailing non-flag argument i.e. no more
+                                * flag arguments are present.
+                                */
+                               for (int j = i + 1; j < argc; j++) {
+                                       if (argv[j][0] == '-') {
+                                               ARGPARSE_LOG(ERR, "unexpected non-flag argument \"%s\"",
+                                                       argv[j]);
+                                               return -EINVAL;
+                                       }
+                               }
+                               break;
                        }
                        arg = find_position_arg(obj, position_index);
                        ret = parse_arg_val(obj, arg->name_long, arg, curr_argv);
diff --git a/lib/argparse/rte_argparse.h b/lib/argparse/rte_argparse.h
index 991f084927..760d59fd8a 100644
--- a/lib/argparse/rte_argparse.h
+++ b/lib/argparse/rte_argparse.h
@@ -158,6 +158,8 @@ struct rte_argparse {
        const char *epilog;
        /** Whether exit when error. */
        bool exit_on_error;
+       /** Don't error out if trailing non-flag arguments are present, ignore them. */
+       bool ignore_trailing_args;
        /** User callback for parsing arguments. */
        rte_arg_parser_t callback;
        /** Opaque which used to invoke callback. */