On 08.02.2019 13:36, Kevin Traynor wrote: > Hi Ilya, > > Thanks for raising this and explaining in detail so it can be discussed.
Thanks for sharing your thoughts. > Comments below, > > On 02/01/2019 01:11 PM, Ilya Maximets wrote: >> This patch deprecates the usage of current configuration knobs for >> DPDK EAL arguments: >> >> - dpdk-alloc-mem >> - dpdk-socket-mem >> - dpdk-socket-limit >> - dpdk-lcore-mask >> - dpdk-hugepage-dir >> - dpdk-extra >> >> All above configuration options replaced with single 'dpdk-options' >> which has same format as old 'dpdk-extra', i.e. just a string with >> all the DPDK EAL arguments. >> >> All the documentation about old configuration knobs removed. Users >> could still use an old interface, but the deprecation warning will be >> printed. In case 'dpdk-options' provided, values of old options will >> be ignored. New users will start using new 'dpdk-options' as this is >> the only documented way providing EAL arguments. >> >> Rationale: >> >> From release to release DPDK becomes more complex. New EAL arguments >> appears, old arguments becomes deprecated. Some changes their meaning >> and behaviour. It's not easy task to manage all this and current >> code in OVS that tries to manage DPDK options is not flexible/extendable >> enough even to track all the dependencies between options in DPDK 18.11. >> For example, '--socket-mem' changed its meaning, '--legacy-mem' and >> '--socket-limit' appeared. But '--legacy-mem' and '--socket-limit' >> could not be used at the same time and, also, we want to provide >> good default value for '--socket-limit' to keep the consistent >> behaviour of OVS memory usage. And this will be worse in the future. >> > > I think it is an exception that shows it is quite stable. There's > probably been something like 10 DPDK releases (didn't check exactly) > since these params were introduced and this seems to be first time that > there was an issue with an argument. > > It was also a very rare change in DPDK where the memory mgmt was > re-designed and re-written from the ground up. That meant an extra flag > was needed to keep the same behavior. Perhaps if the existing flag was > not reused, there wouldn't have been an issue at all (or at least it > would have been much clearer). I want to clarify that the 'socket-mem' related issue is not fully solved in current OVS. It's still possible for user to specify the 'dpdk-socket-limit' knob and use '--legacy-mem' in 'dpdk-extra'. OVS is not able to track this in current parsing code without introducing nasty workarounds. > >> Another point is that even now we have mutually exclusive database >> configs in OVS and we have to handle them. i.e we're providing >> 'dpdk-alloc-mem' and 'dpdk-socket-mem' at the same time. Additionally, >> user allowed to configure same options in 'dpdk-extra'. So, we have >> similar/equal options in a three places in ovsdb and we need to >> choose which one to use. It's pretty easy for user to get into >> inconsistent database configuration, because users could update >> one field without removing others, and OVS has to resolve all the >> conflicts somehow constructing the EAL args. But we do not know in >> practice, if the resulted arguments are the arguments that user wanted >> to see or just forget to remove the higher priority knob. >> > > That's true about dpdk-alloc-mem and dpdk-socket-mem. I'm not sure if > the docs give some direction on that. IIRC, it is documented and checked > about precedence when using dpdk-extra's. > >> Next point is that we have 'dpdk-lcore-mask' (-c) which is actually not >> so user-friendly as '-l', but we have no option for it. Will we create >> additional 'dpdk-lcore-list' ? I guess, no, because it's not really >> important thing. But users will stuck with this not so user-friendly >> masks. >> > > I'm not sure which is more user-friendly but I agree adding > dpdk-lcore-list is not needed, and probably could add confusion > (especially as dpdk-lcore-mask doesn't need '0x' prefix). IMHO, numbers are more visual than masks. But yes, this will add more confusion. > >> Another thing is that OVS is not really need to check the consistency >> of the EAL arguments because DPDK could check it instead. DPDK will >> always check the args and it will do it better. There is no real >> need to duplicate this functionality. >> >> Keeping all the options in a single string 'dpdk-options' allows: >> >> * To have only one place for all the configs, i.e. it will be harder >> for user to forget to remove something from the single string while >> updating it. >> * Not to check the consistency between different configs, DPDK could >> check the cmdline itself. >> * Not to document every DPDK EAL argument in OVS. We can just >> describe few of them and point to DPDK docs for more information. >> * OVS still able to provide some minimal default arguments. >> Thanks to DPDK 18.11 we only need to specify an lcore. All other >> arguments are not necessary. (We still also providing default >> --socket-limit in case --socket-mem passed without it and without >> --legacy-mem) >> > > It would seem wrong to tell the OVS user to fill in the DPDK EAL > arguments in a type of passthrough string to DPDK but then also insert > defaults to that. That is the good point. Maybe we could drop the defaults except the lcore list (which is mandatory in dpdk) if 'dpdk-options' provided. > >> Usage example: >> >> ovs-vsctl set Open_vSwitch . \ >> other_config:dpdk-options="-l 1 -n 4 --socket-mem 1024,1024" >> > > It is possible to do that now. Sure, this could be done with 'dpdk-extra'. But as I said, 'dpdk-extra' is not the right name for the only configuration option. Anyway, the main target for this patch is not to change the user API, but to simplify the code. i.e. there is an option to keep the old API if we'll stop checking it for consistency in OVS code. > >> Signed-off-by: Ilya Maximets <i.maxim...@samsung.com> > > > What I like about the current scheme is that it caters for users who are > very familiar with DPDK and will customise their EAL arguments, but it > also provides an abstraction and defaults for the bare minimum items for > OVS users who aren't as familiar with DPDK. > > For example, with the socket-mem change you've highlighted, if this > wasn't handled in OVS (and thanks for that), then every OVS user would > have to find this very subtle change themselves and adjust their > arguments. So I believe that there is value in OVS creating a little > abstraction and defaults for some of the most commonly used args. With DPDK 18.11 we only need to provide default value for the lcore mask. And we're providing it. So, users may not think about EAL args. You don't need to have any DPDK knob configured. We may highlight some incompatible EAL ARGS changes in NEWS while upgrading to new DPDK release. This should be user-visible, because NEWS should be the first thing you read before upgrading your OVS version. No real need to track the changes and try to preserve previous behaviour in code. OTOH, it might be better for some users if we'll not force the 'socket-limit'. i.e. they will have better memory model out-of-the-box without need to disable limits. But we're forcing them to look at the release notes of OVS and DPDK to find out that there is a better solution for them. In fact, we're pushing many users to the past, protecting them from the better life. And they could never know about that because we're hiding changes making everything work same as before. > > Sorry, but overall I don't think there's a big problem that needs to be > addressed and there would be a heavy price in disruption for users. I see your point. That's what an RFCs are made for, to gather all the opinions and make a good decision. > > thanks, > Kevin. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
