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

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

On Mon, Apr 07, 2025 at 08:14:50AM -0700, Stephen Hemminger wrote:
> On Mon, 7 Apr 2025 12:15:13 +0200
> Morten Brørup <mb@smartsharesystems.com> 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.
> 
> There already is a first pass to catch log parameters, could the offset arg be handled there?
> 
It could, but I'd rather not get into further handling of args in a
two-pass setup. If we go that way, we might be better to do a completely
"delayed-parsing" setup, where we use getopt to put all arguments into a
structure with named pointers for each arg type. Thereafter we do the
actual processing of args from the structure itself, allowing us to do all
arg processing in a fixed/known order. Unfortunately, that would be a
significant change in how things are done.

Also, from the discussion on this thread, there seems to be some support
for having a completely new cmdline arg that takes the core list and always
remaps them, rather than using a modifier to existing args. Your opinions
on the relative benefits/drawbacks of the two approaches are welcome! :-)

/Bruce