Re: [dpdk-dev] [RFC] Yet another option for DPDK options

[Arnon Warshavsky <arnon@qwilt.com>]

On Fri, Jun 3, 2016 at 9:38 PM, Neil Horman <nhorman@tuxdriver.com> wrote:

> On Fri, Jun 03, 2016 at 06:29:13PM +0000, Wiles, Keith wrote:
> >
> > On 6/3/16, 12:44 PM, "Neil Horman" <nhorman@tuxdriver.com> wrote:
> >
> > >On Fri, Jun 03, 2016 at 04:04:14PM +0000, Wiles, Keith wrote:
> > >> Sorry, I deleted all of the text as it was getting a bit long.
> > >>
> > >> Here are my thoughts as of now, which is a combination of many
> suggestions I read from everyone’s emails. I hope this is not too hard to
> understand.
> > >>
> > >> - Break out the current command line options out of the DPDK common
> code and move into a new lib.
> > >>   - At this point I was thinking of keeping the rte_eal_init(args,
> argv) API and just have it pass the args/argv to the new lib to create the
> data storage.
> > >>      - Maybe move the rte_eal_init() API to the new lib or keep it in
> the common eal code. Do not want to go hog wild.
> > >>   - The rte_eal_init(args, argv) would then call to the new API
> rte_eal_initialize(void), which in turn queries the data storage. (still
> thinking here)
> > >These three items seem to be the exact opposite of my suggestion.  The
> point of
> > >this change was to segregate the parsing of configuration away from the
> > >initalization dpdk using that configurtion.  By keeping rte_eal_init in
> such a
> > >way that the command line is directly passed into it, you've not
> changed that
> > >implicit binding to command line options.
> >
> > Neil,
> >
> > You maybe reading the above wrong or I wrote it wrong, which is a high
> possibility. I want to move the command line parsing out of DPDK an into a
> library, but I still believe I need to provide some backward compatibility
> for ABI and to reduce the learning curve. The current applications can
> still call the rte_eal_init(), which then calls the new lib parser for dpdk
> command line options and then calls rte_eal_initialize() or move to the new
> API rte_eal_initialize() preceded by a new library call to parse the old
> command line args. At some point we can deprecate the rte_eal_init() if we
> think it is reasonable.
> >
> > >
> > >I can understand if you want to keep rte_eal_init as is for ABI
> purposes, but
> > >then you should create an rte_eal_init2(foo), where foo is some handle
> to in
> > >memory parsed configuration, so that applications can preform that
> separation.
> >
> > I think you describe what I had planned here. The rte_eal_initialize()
> routine is the new rte_eal_init2() API and the rte_eal_init() was only for
> backward compatibility was my thinking. I figured the argument to
> rte_eal_initialize() would be something to be decided, but it will mostly
> likely be some type of pointer to the storage.
> >
> > I hope that clears that up, but let me know.
> >
> yes, that clarifies your thinking, and I agree with it.  Thank you!
> Neil
>
> > ++Keith
> >
> > >
> > >Neil
> > >
> > >>   - The example apps args needs to be passed to the examples as is
> for now, then we can convert them one at a time if needed.
> > >>
> > >> - I would like to keep the storage of the data separate from the file
> parser as they can use the ‘set’ routines to build the data storage up.
> > >>   - Keeping them split allows for new parsers to be created, while
> keeping the data storage from changing.
> > >> - The rte_cfg code could be modified to use the new configuration if
> someone wants to take on that task ☺
> > >>
> > >> - Next is the data storage and how we can access the data in a clean
> simple way.
> > >> - I want to have some simple level of hierarchy in the data.
> > >>   - Having a string containing at least two levels
> “primary:secondary”.
> > >>      - Primary string is something like “EAL” or “Pktgen” or
> “testpmd” to divide the data storage into logical major groups.
> > >>         - The primary allows us to have groups and then we can have
> common secondary strings in different groups if needed.
> > >>      - Secondary string can be whatever the developer of that group
> would like e.g. simple “EAL:foobar”, two levels “testpmd:foo.bar”
> > >>
> > >>   - The secondary string is treated as a single string if it has a
> hierarchy or not, but referencing a single value in the data storage.
> > >>      - Key value pairs (KVP) or a hashmap data store.
> > >>         - The key here is the whole string “EAL:foobar” not just
> “foobar” secondary string.
> > >>            - If we want to have the two split I am ok with that as
> well meaning the API would be:
> > >>              rte_map_get(mapObj, “EAL”, “foo.bar”);
> > >>              rte_map_set(mapObj, “EAL”, “foo.bar”, value);
> > >>            - Have the primary as a different section in the data
> store, would allow for dumping that section maybe easier, not sure.
> > >>               - I am leaning toward
> > >>      - Not going to try splitting up the string or parse it as it is
> up to the developer to make it unique in the data store.
> > >> - Use a code design to make the strings simple to use without having
> typos be a problem.
> > >>    - Not sure what the design is yet, but I do not want to have to
> concat two string or split strings in the code.
> > >>
> > >> This is as far as I have gotten and got tired of typing ☺
> > >>
> > >> I hope this will satisfy most everyone’s needs for now.
> > >>
> > >>
> > >> Regards,
> > >> Keith
> > >>
> > >>
> > >>
> > >
> >
> >
> >
>

Keith

What about the data types of the values?
I would assume that as a library it can provide the service of typed
get/set and not leave conversion and validation to the app.

rte_map_get_int(map,section,key)
rte_map_get_double(...)
rte_map_get_string(...)
rte_map_get_bytes(...,destBuff , destBuffSize) //e.g byte array of RSS key

This may also allow some basic validity of the configuration file

Another point I forgot about is default values.
We sometimes use a notation where the app also specifies a default value in
case the configuration did not specify it
  rte_map_get_int(map,section,key , defaultValue )
and specify if this was a mandatory that has no default
  rte_map_get_int_crash_if_missing (map,section,key)


/Arnon