Overall I like this change but as a user of ovs with dpdk and A maintainer of a plugin to deploy it I have some comments inline.
> -----Original Message----- > From: Aaron Conole [mailto:acon...@bytheb.org] > Sent: Wednesday, February 10, 2016 1:23 PM > To: Traynor, Kevin > Cc: dev@openvswitch.org; Flavio Leitner; Mooney, Sean K > Subject: Re: [ovs-dev] [PATCH v8 0/5] Convert DPDK configuration from > command line to DB based > > "Traynor, Kevin" <kevin.tray...@intel.com> writes: > >> -----Original Message----- > >> From: Aaron Conole [mailto:acon...@redhat.com] > >> Sent: Friday, January 29, 2016 5:57 PM > >> To: dev@openvswitch.org > >> Cc: Flavio Leitner <f...@sysclose.org>; Panu Matilainen > >> <pmati...@redhat.com>; Traynor, Kevin <kevin.tray...@intel.com>; > >> Zoltan Kiss <zoltan.k...@linaro.org>; Christian Ehrhardt > >> <christian.ehrha...@canonical.com> > >> Subject: [PATCH v8 0/5] Convert DPDK configuration from command line > >> to DB based > >> > >> Currently, configuration of DPDK parameters is done via the command > >> line through a --dpdk **OPTIONS** -- command line argument. This has > >> a number of challenges, including: > >> * It must be the first option passed to ovs-vswitchd > >> * It breaks from the way most other things are configured in OVS > >> * It doesn't allow an easy way to populate defaults > >> > >> > >> This series brings the following changes to openvswitch: > >> * All DPDK options are taken from the ovs database rather than the > >> command line > >> * DPDK lcores are optionally auto-assigned to a single core based on > the > >> bridge coremask. > >> * Updated documentation > >> > >> v2: > >> * Dropped the vhost-user socket configuration options. Those can be > re-added > >> as an extension > >> * Incorporated feedback from Kevin Traynor. > >> > >> v3: > >> * Went back to a global dpdk-init > >> * Language cleanup and various minor fixes > >> > >> v4: > >> * Added a way to pass arbitrary eal arguments > >> > >> v5: > >> * Restore the socket-mem default, and fix up the ovs-dev.py script, > along > >> with the manpage for ovsdb-server > >> > >> v6: > >> * Correct a documentation issue with INSTALL.DPDK.md > >> * Correct a non-dpdk enabled OVS incorrect warning variable > >> * Remove an excess whitespace > >> > >> v7: > >> * After testing by Christian with dpdk-alloc-mem > >> > >> v8: > >> * Confirmed ``make check`` operation with and without dpdk. > >> Retested on live-host > > > > Hi, > > > > I've done some testing on this patchset and I couldn't find any > > issues. > > Cool; does that mean I have your Tested-by? :) > > > - tested that -c and -n defaults and explicit values are catered for > > - tested dpdk-init=t/f leads to dpdk initialization or not > > - tested that use of both dpdk-socket-mem and dpdk-alloc-mem is > > caught > > - tested that a string can be passed in through extra_args > > - tested the code won't catch using a db entry dpdk-socket-mem and > also > > putting --socket-mem in extra_args, however dpdk will barf > > > > On command line args vs. db entries vs. a string of args in the db, if > > there is doubt on this then let's debate further. This will change how > > ovs with dpdk is used, so better debate it out and get it right. > > I don't think there's any real doubt. I think the approach is the best > way to do this. I have agreement from almost everyone else, I think? > Anyone still need to be convinced? From an orchestration point of view the explcit values are nice but form a Deployment point of view I favor only having dpdk_init:true/false and dpdk_extra:<cmd line sting> In particular if is set a value in dpdk_extra and in the future you add a explicit entry in the db to Hold that value I never want it to brake my deployment code which still uses dpdk_extra. Where db entries Exist for an item it would make my life eaiser if they were always set automatically when the vswitchd starts e.g. if I never spcify the vhostuser_socket_dir it would be nice if it was set by ovsvswitchd to the compile time default. Similar if I override the default of a value in dpdk_extra then it would be nice if when an explicit value for that item exits In the db that it be updated to match the value specified in the dpdk_extra args. If this is not the case I cannot true the explcit values in the ovsdb as they may not be valid. > > > There's one or two of the db entries that may be able to reused later > > for other things e.g. vhostuser socket location, so that would be a + > for them. > > Backwards compatibility would be a + for command line args. Daniele > > has mentioned scripting also. I'm sure there's other +/-'s. > > I don't know - scripting vswitchd? I think that sounds a little strange; It depend on your usecase. I am a developer on neutron integrating ovs with dpkd with Openstack. I also maintain a devstack plugin to deploy ovs with dpdk from source. In keeping with other service in devstack we run the ovsvswitchd in a screen session. https://github.com/openstack/networking-ovs-dpdk/blob/master/devstack/ovs-dpdk/ovs-dpdk-init#L528 we heavily script the compilation and deployment of ovs and treat it like any other ephemeral service. Our ovs-dpdk service file is 700 lines long and we have separate logic to compile and install it. In production sure it won't be started and stopped frequently but when used in a development environment It can have a very sort lifetime. > it isn't some kind of ephemeral service that comes and goes. And it's > not like this patch prevents the same kinds of arbitrary commands to be > passed to the EAL (since 4/5 does precisely that). The only change > required is doing ovs-vsctl before ovs-vswitch in the 'starting > vswitchd' case. Is that really a huge deal? > > There's always pros and cons. I haven't heard any explict NAK, or any > explicit ACK. It would be nice for that to happen, since I can't > maintain this series out-of-tree forever, and there are other things I'd > really like to get to (as fun as db entries may be). > > Any suggestions on how to move forward? I was planning on posting a > rebased v9 - should I still do that? > > -Aaron > > > Kevin. > > > >> > >> Aaron Conole (5): > >> netdev-dpdk: Restore thread affinity after DPDK init > >> netdev-dpdk: Convert initialization from cmdline to db > >> netdev-dpdk: Autofill lcore coremask if absent > >> netdev-dpdk: Allow arbitrary eal arguments > >> NEWS: Announce the DPDK EAL configuration change > >> > >> FAQ.md | 6 +- > >> INSTALL.DPDK.md | 90 ++++++++++--- > >> NEWS | 5 + > >> lib/netdev-dpdk.c | 327 > ++++++++++++++++++++++++++++++++++++++----- > >> -- > >> lib/netdev-dpdk.h | 22 ++- > >> utilities/ovs-dev.py | 7 +- > >> vswitchd/bridge.c | 3 + > >> vswitchd/ovs-vswitchd.8.in | 5 +- > >> vswitchd/ovs-vswitchd.c | 25 +--- > >> vswitchd/vswitch.xml | 128 +++++++++++++++++- > >> 10 files changed, 513 insertions(+), 105 deletions(-) > >> > >> -- > >> 2.5.0 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
