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

[Wiles, Keith]

On 6/3/16, 2:18 PM, "Neil Horman" <nhorman at tuxdriver.com> wrote:

>On Fri, Jun 03, 2016 at 07:07:50PM +0000, Wiles, Keith wrote:
>> On 6/3/16, 2:00 PM, "dev on behalf of Wiles, Keith" <dev-bounces at dpdk.org on behalf of keith.wiles at intel.com> wrote:
>> 
>> >On 6/3/16, 1:52 PM, "Arnon Warshavsky" <arnon at qwilt.com<mailto:arnon at qwilt.com>> wrote:
>> >
>> >
>> >
>> >On Fri, Jun 3, 2016 at 9:38 PM, Neil Horman <nhorman at tuxdriver.com<mailto:nhorman at 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 at tuxdriver.com<mailto:nhorman at 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
>> >
>> >Arnon,
>> >
>> >Yes, I too was thinking about access type APIs, but had not come to a full conclusion yet. As long as the API for get/put can return any value, we can add a layer on top of these primary get/put APIs to do some basic type checking. This way the developer can add his/her own type checking APIs or we provide a couple basic types for simple values.
>> 
>> One more thing. I had not thought about default values as the defaults are handle directly by the code when an option is not applied. I think it should be left up to the developer to add default values to the storage or handle it when an option is not found in the storage.
>> 
>> If I understand your code above the API would pass in a default value if one did not exist in the storage, which I guess is reasonable. Anyone think this is a good idea or not?
>> 
>
>I'm not opposed to default values, but it seems to me that if we are splitting
>out a configuration storage library from dpdk, part of the initzliation of that
>library can be installing default values.  That is to say, instead of having the
>code specific areas assume a default value if none is present in the config, an
>init function for the configuration storage library would just populate the
>keystore.  That way all the dpdk itself has to do is a key lookup.

+1

If someone needs or wants default values in the API call then a wrapper functions around the basic keystore APIs can be done by the developer or we can add a new set of APIs to provide that type of feature, just like the variable type APIs. Just as long as the basic APIs do not exclude we can add it later.

>
>Neil
>
>> >
>> >Does that make sense?
>> >
>> >++Keith
>> >
>> 
>> 
>> 
>