RE: [PATCH v2 0/3] allow easier use of high lcore-ids

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Monday, 7 April 2025 14.42
> 
> On Mon, Apr 07, 2025 at 02:25:46PM +0200, Morten Brørup wrote:
> > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > Sent: Monday, 7 April 2025 13.56
> > >
> > > On Mon, Apr 07, 2025 at 01:32:59PM +0200, Morten Brørup wrote:
> > > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > > > Sent: Monday, 7 April 2025 12.41
> > > > >
> > > > > On Mon, Apr 07, 2025 at 12:15:13PM +0200, Morten Brørup wrote:
> > > > > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > Sent:
> > > > > > > Monday, 7 April 2025 11.49
> > > > > > >
> > > > > > > On Mon, Apr 07, 2025 at 09:04:05AM +0200, David Marchand
> wrote:
> > > > > > > > Hello Bruce,
> > > > > > > >
> > > > > > > > On Tue, Apr 1, 2025 at 4:08 PM Bruce Richardson
> > > > > > > > <bruce.richardson@intel.com> wrote:
> > > > > > > > >
> > > > > > > > > On Mon, Mar 24, 2025 at 05:30:26PM +0000, Bruce
> Richardson
> > > > > wrote:
> > > > > > > > > > Traditionally, DPDK has had a direct mapping of
> internal
> > > > > lcore-
> > > > > > > ids, to
> > > > > > > > > > the actual core numbers in use. With higher core
> count
> > > > > servers
> > > > > > > becoming
> > > > > > > > > > more prevalent the issue becomes one of increasing
> memory
> > > > > > > footprint when
> > > > > > > > > > using such a scheme, due to the need to have all
> arrays
> > > > > > > dimensioned for
> > > > > > > > > > all cores on the system, whether or not those cores
> are
> > > in
> > > > > use by
> > > > > > > the
> > > > > > > > > > app.
> > > > > > > > > >
> > > > > > > > > > Therefore, the decision was made in the past to not
> > > expand
> > > > > the
> > > > > > > > > > build-time RTE_MAX_LCORE value beyond 128. Instead,
> it
> > > was
> > > > > > > recommended
> > > > > > > > > > that users use the "--lcores" EAL parameter to take
> the
> > > high-
> > > > > > > numbered
> > > > > > > > > > cores they wish to use and map them to lcore-ids
> within
> > > the 0
> > > > > -
> > > > > > > 128
> > > > > > > > > > range. While this works, this is a little clunky as
> it
> > > means
> > > > > that
> > > > > > > > > > instead of just passing, for example, "-l 130-139",
> the
> > > user
> > > > > must
> > > > > > > > > > instead pass "--lcores 0@130,1@131,2@132,3@133,...."
> > > > > > > > > >
> > > > > > > > > > This patchset attempts to simplify the situation by
> > > adding a
> > > > > new
> > > > > > > flag to
> > > > > > > > > > do this mapping automatically. To use cores 130-139
> and
> > > map
> > > > > them
> > > > > > > to ids
> > > > > > > > > > 0-9 internally, the EAL args now become: "-l 130-139
> --
> > > map-
> > > > > lcore-
> > > > > > > ids",
> > > > > > > > > > or using the shorter "-M" version of the flag: "-Ml
> 130-
> > > 139".
> > > > > > > > > >
> > > > > > > > > > Adding this new parameter required some rework of the
> > > > > existing
> > > > > > > arg
> > > > > > > > > > parsing code, because in current DPDK the args are
> parsed
> > > and
> > > > > > > checked in
> > > > > > > > > > the order they appear on the commandline. This means
> that
> > > > > using
> > > > > > > the
> > > > > > > > > > example above, the core parameter 130-139 will be
> > > rejected
> > > > > > > immediately
> > > > > > > > > > before the "map-lcore-ids" parameter is seen. To work
> > > around
> > > > > > > this, the
> > > > > > > > > > core (and service core) parameters are not parsed
> when
> > > seen,
> > > > > > > instead
> > > > > > > > > > they are only saved off and parsed after all
> arguments
> > > are
> > > > > > > parsed. The
> > > > > > > > > > "-l" and "-c" parameters are converted into "--
> lcores"
> > > > > arguments,
> > > > > > > so all
> > > > > > > > > > assigning of lcore ids is done there in all cases.
> > > > > > > > > >
> > > > > > > > > > RFC->v2: * converted printf to DEBUG log * added "-M"
> as
> > > > > shorter
> > > > > > > > > > version of flag * added documentation * renamed
> internal
> > > API
> > > > > that
> > > > > > > > > > was changed to avoid any potential
> > > > > > > hidden
> > > > > > > > > >   runtime issues.
> > > > > > > > > >
> > > > > > > > > > Bruce Richardson (3): eal: centralize core parameter
> > > parsing
> > > > > eal:
> > > > > > > > > > convert core masks and lists to core sets eal: allow
> > > > > automatic
> > > > > > > > > > mapping of high lcore ids
> > > > > > > > > >
> > > > > > > > > Ping for review.
> > > > > > > > >
> > > > > > > > > At a high level, does this feature seem useful to
> users?
> > > > > > > >
> > > > > > > > This seems useful, though I am not I would touch the
> existing
> > > > > > > options.
> > > > > > > > I would have gone with a simple -L option (taking the
> same
> > > kind
> > > > > of
> > > > > > > > input than -l but with new behavior), and not combine a
> flag
> > > with
> > > > > > > > existing options.
> > > > > > > >
> > > > > > >
> > > > > > > That would be an easier patchset to do up. However, it
> would
> > > then
> > > > > mean
> > > > > > > that we have no less than 4 different ways to specify the
> cores
> > > to
> > > > > use:
> > > > > > > "- c", "-l", "-L", "--lcores" - and therefore need 4
> different
> > > sets
> > > > > of
> > > > > > > parsing options for them, and more checks to ensure we have
> > > only
> > > > > one of
> > > > > > > the 4 specified in any run. That's why I chose the modifier
> > > option,
> > > > > and
> > > > > > > to try and consolidate the core setup a bit.
> > > > > > >
> > > > > > > However, if having a completely new option is preferred, I
> am
> > > happy
> > > > > > > enough to do up a different patchset for that.
> > > > > > >
> > > > > > > > I scanned through the series, not much to say.  Maybe add
> a
> > > unit
> > > > > test
> > > > > > > > for new cmdline option.
> > > > > > > >
> > > > > > > Sure. Once it's decided what approach (if any) to take,
> I'll do
> > > up
> > > > > a
> > > > > > > new patchset, taking into account any relevant feedback on
> this
> > > > > set.
> > > > > > >
> > > > > > > /Bruce
> > > > > >
> > > > > > Changing the EAL parameter parser to a "two pass parser"
> makes
> > > sense.
> > > > > I
> > > > > > think checking for existence of more than one lcore
> specification
> > > > > options
> > > > > > should suffice; we don't need to accept multiple lcore
> > > specification
> > > > > > options and check for conflicts.
> > > > > >
> > > > > > When remapping, do we need to support gaps in the "lcore"
> > > (logical
> > > > > cores)
> > > > > > array, e.g. for secondary processes, or can it be continuous
> from
> > > 0
> > > > > to
> > > > > > the number of specified lcores?
> > > > > >
> > > > > > And are new EAL parameters for this really beneficial?
> Doesn't
> > > e.g.
> > > > > "-l
> > > > > > 0-9@130-139,100@140" suffice?
> > > > > >
> > > > > Actually, I believe "0-9@130-139"[1]  is not the same as
> > > > > "0@130,1@131,2@132,...". The latter affinities one thread to
> one
> > > core
> > > > > ten
> > > > > times over, while the former affinitizes 10 threads to 10 cores
> -
> > > > > leaving
> > > > > those threads free to move about within the 10 cores specified.
> > > >
> > > > Interesting. The documentation [GSG] isn't clear to me about
> this; a
> > > example there could help clarify.
> > > >
> > > > [GSG]:
> > >
> https://doc.dpdk.org/guides/linux_gsg/linux_eal_parameters.html#lcore-
> > > related-options
> > > >
> > >
> > > Yep, agreed.
> > >
> > > > If users are manually passing lcore parameters to the EAL, then I
> see
> > > why some sort of remapping shorthand is useful.
> > > > IMO, if the mappings are relatively exotic, it should be
> acceptable
> > > requiring an external script to build a long list of mapping
> parameters
> > > and then invoke the application with those script-generated EAL
> > > parameters.
> > > > This would reduce the scope to support relatively simple, common
> > > mappings.
> > > >
> > > > Could we expand the --lcores syntax to support common mappings?
> > > >
> > > > E.g. "0-9@130+" to do what I thought.
> > > > The lack of "()" treats the entries individually (not as a
> group),
> > > and the "+" indicates auto-increment.
> > > >
> > > > A more advanced example:
> > > > "0-9@(130-131)+", meaning lcore 0 gets cpus 130-131, lcore 1 gets
> > > cpus 132-133, etc.
> > > >
> > >
> > > My issues with the above syntax idea is:
> > > * I think it's overly complicating the lcores parameter adding in
> the
> > >   support for the "+" symbol - especially in the advanced case
> example
> > > you
> > >   provide. I worry about code maintainability here.
> > > * More significantly for me, I think it's also getting things
> backwards
> > > in
> > >   that it is focusing more on the lcore ids visible to the app,
> rather
> > > than
> > >   the physical cores to be used. For the example above of 0-9@130+,
> > > what I
> > >   would expect is that the user is mainly thinking about the cores
> he
> > > wants
> > >   to use 130-139, which are then to be mapped to lower ids. If we
> had
> > > the
> > >   syntax reversed where the physical cores were first, I'd say it
> would
> > >   make more sense, e.g. 130-139@0+
> >
> > I 100 % agree on the syntax being backwards.
> > A good reason for introducing a new parameter, rather than expanding
> on "--lcores".
> >
> 
> Yes and no.
> I agree with not expanding on --lcores, but I also don't think any new
> parameter added should attempt to reproduce all of what lcores does. I
> would leave --lcores as-is, as the power-tool for lcore config e.g.
> what
> you talk about below for mapping multiple lcores to the same physical
> cpu.
> 
> > We should consider deprecating the old (backwards) syntax, so users
> don’t get confused about one EAL parameter being "backwards" of the
> other.
> >
> 
> I disagree with this. I would be ok with deprecating the old "-c"
> coremask
> syntax - I think the time is long past when we should be dealing with
> masks. However, removing "-l" and "--lcores" flag is, to me anyway, too
> big
> and jarring a change for end users for us to consider.
> 
> > > * finally, as I found out last month, there are systems on which
> the
> > > cores
> > >   are spread across numa-nodes on odd/even boundaries, so to have
> an
> > > app
> > >   running on socket 0, you need to give the core ids as a list i.e.
> > >   0,2,4,6, and cannot use ranges. [This also reenforces the point
> above
> > >   too, where again it's the internal ids we need to generalize, not
> the
> > >   physical cpus]
> >
> > For this, we could use "/2" (like in subnets), or "+2" as increment
> parameter.
> >
> > >
> > > My thinking on the matter, and I'm happy to be corrected if I'm
> wrong
> > > here,
> > > is that end-users are unlikely to significantly care what the
> actual
> > > lcore
> > > ids are internally in the program, so long as they work and are
> unique.
> >
> > Generally yes.
> > Note that we should allow the same physical CPU core to be assigned
> to multiple lcores...
> > If a CPU core is shared between multiple worker lcores, then it could
> be problematic, but with a mix of lcore roles, this might be handy for
> some applications. E.g. a virtual machine with only one CPU core could
> use that single CPU core as both main and worker lcore, or as main and
> service lcore.
> > Sharing an lcore between threads requires the developer to take
> special care of e.g. spinlocks, but the EAL parameter parser should not
> prohibit it. It might log a notice, though.
> >
> 
> This is already taken care of via --lcores, so I don't see the need to
> reimplement it using a new flag also. Any new flags we add should be
> kept
> deliberately simple.
> 
> > > What does matter is the physical cpus on which the code is to run.
> > > Therefore, my approach to this is to find the simplest possible
> > > solution
> > > whereby the user can just provide a list of cores and tell EAL to
> just
> > > map
> > > them to reasonable values. For the "reasonable" values, I would
> imagine
> > > that for the vast majority of cases starting "0" is what is wanted.
> For
> > > anything beyond that, we already have the existing --lcores syntax
> to
> > > be
> > > used.
> >
> > Agree with all of the above. :-)
> >
> 
> Great. That still leaves us with the problem of what exactly to do as
> the
> best solution. Here are some alternatives that I see:
> 
> 1. Add a modifier flag for -l and -c parameters to auto-remap the lcore
> ids
>    to zero, so user is just specifying physical CPU id's.
> 2. Add a new flag for specifying physical cpu ids, which auto-remaps
> the
>    cores specified to zero.
>    2a. To simplify our code and user experience we could at the same
> time:
>        * deprecate "-c" flag for coremasks
>        * make "-l" and "--lcores" the same flag just in long and short
>          versions. This should not break anything as "-l" parameter is
>          just as subset of what "--lcores" provides.
>        * that would leave us with effectively two core flag paths:
>           - -l/--lcores, behaviour as now, full explicit control
>           - -L/--lcores-remapped, takes simplified core list (only ","
> and
>             "-" supported as with "-l" now), and maps them to zero-
> based.

+1 for 2a.

> 
> Third options? Any other feedback?

If the internal lcore ids are effectively hidden by passing physical CPU ids, any related options should take physical CPU ids too. I.e. the options for choosing main lcore and service lcores also need variants with other names.
The service cores are passed as a core mask; this should be deprecated (like "-c"). And superseded by a parameter taking a core list.