"Traynor, Kevin" <kevin.tray...@intel.com> writes:
>> -----Original Message-----
>> From: Aaron Conole [mailto:acon...@redhat.com]
>> Sent: Monday, January 18, 2016 8:29 PM
>> To: dev@openvswitch.org
>> Cc: Flavio Leitner; Traynor, Kevin; Panu Matilainen; Zoltan Kiss
>> Subject: [PATCH v5 2/4] netdev-dpdk: Convert initialization from cmdline to
>> db
>>
>> Existing DPDK integration is provided by use of command line options which
>> must be split out and passed to librte in a special manner. However, this
>> forces any configuration to be passed by way of a special DPDK flag, and
>> interferes with ovs+dpdk packaging solutions.
>>
>> This commit delays dpdk initialization until after the OVS database
>> connection is established, at which point ovs initializes librte. It
>> pulls all of the config data from the OVS database, and assembles a
>> new argv/argc pair to be passed along.
>>
>> Signed-off-by: Aaron Conole <acon...@redhat.com>
>> ---
>> v2:
>> * Removed trailing whitespace
>> * Followed for() loop brace coding style
>> * Automatically enable DPDK when adding a DPDK enabled port
>> * Fixed an issue on startup when DPDK enabled ports are present
>> * Updated the documentation (including vswitch.xml) and documented all
>> new parameters
>> * Dropped the premature initialization test
>>
>> v3:
>> * Improved description language in INSTALL.DPDK.md
>> * Fixed the ovs-vsctl examples for DPDK
>> * Returned to the global dpdk-init (bullet 3 from v2)
>> * Fixed a build error when compiling without dpdk support enabled
>> * converted to xstrdup, for consistency after rebasing
>>
>> v4:
>> * No change
>>
>> v5:
>> * Adjust the ovs-dev script to account for the new dpdk configuration
>> * Update the ovs-vswitchd.8.in pointing to INSTALL.DPDK.md
>
> Hi Aaron. I've only one real comment below on this patch. The other patches
> look fine to me but I didn't get a chance to test.
I think I owe you a beer for all your hard work.
Thanks for the reviews, Kevin!
> thanks,
> Kevin.
>
>>
>> FAQ.md | 6 +-
>> INSTALL.DPDK.md | 81 ++++++++++++++-----
>> lib/netdev-dpdk.c | 191 ++++++++++++++++++++++++++++++++-----------
>> --
>> lib/netdev-dpdk.h | 22 ++++--
>> utilities/ovs-dev.py | 11 ++-
>> vswitchd/bridge.c | 3 +
>> vswitchd/ovs-vswitchd.8.in | 5 +-
>> vswitchd/ovs-vswitchd.c | 25 +-----
>> vswitchd/vswitch.xml | 118 +++++++++++++++++++++++++++-
>> 9 files changed, 346 insertions(+), 116 deletions(-)
>>
>> diff --git a/FAQ.md b/FAQ.md
>> index 29b2e19..c233118 100644
>> --- a/FAQ.md
>> +++ b/FAQ.md
>> @@ -431,9 +431,9 @@ A: Yes. How you configure it depends on what you mean by
>> "promiscuous
>>
>> A: Firstly, you must have a DPDK-enabled version of Open vSwitch.
>>
>> - If your version is DPDK-enabled it will support the --dpdk
>> - argument on the command line and will display lines with
>> - "EAL:..." during startup when --dpdk is supplied.
>> + If your version is DPDK-enabled it will support the other_config:dpdk-
>> init
>> + configuration in the database and will display lines with
>> + "EAL:..." during startup when other_config:dpdk-init is set to 'true'.
>>
>> Secondly, when adding a DPDK port, unlike a system port, the
>> type for the interface must be specified. For example;
>> diff --git a/INSTALL.DPDK.md b/INSTALL.DPDK.md
>> index 96b686c..46bd1a8 100644
>> --- a/INSTALL.DPDK.md
>> +++ b/INSTALL.DPDK.md
>> @@ -143,22 +143,64 @@ Using the DPDK with ovs-vswitchd:
>>
>> 5. Start vswitchd:
>>
>> - DPDK configuration arguments can be passed to vswitchd via `--dpdk`
>> - argument. This needs to be first argument passed to vswitchd process.
>> - dpdk arg -c is ignored by ovs-dpdk, but it is a required parameter
>> - for dpdk initialization.
>> + DPDK configuration arguments can be passed to vswitchd via Open_vSwitch
>> + other_config database. The recognized configuration options are listed.
>> + Defaults will be provided for all values not explicitly set.
>> +
>> + * dpdk-init
>> + Specifies whether OVS should initialize and support DPDK ports. This is
>> + a boolean, and defaults to false.
>
> I'm assuming you've renamed from 'dpdk' to 'dpdk-init', so it could be =false
> and there is still the possibility of adding a lazy init at a later time?
>
> If so, I think it is good idea because we wouldn't want a situation in the
> future where 'dpdk'=false but an introduced lazy init means DPDK is used.
Yes, that's _exactly_ the intent :D Seems clearer than dpdk=lazy (which
might imply the wrong thing...)
>> +
>> + * dpdk-lcore-mask
>> + Specifies the CPU cores on which dpdk lcore threads should be spawned.
>> + The DPDK lcore threads are used for DPDK library tasks, such as
>> + library internal message processing, logging, etc. Value should be in
>> + the form of a hex string (so '0x123') similar to the 'taskset' mask
>> + input.
>> + If not specified, the value will be determined by choosing the lowest
>> + CPU core from initial cpu affinity list. Otherwise, the value will be
>> + passed directly to the DPDK library.
>> + For performance reasons, it is best to set this to a single core on
>> + the system, rather than allow lcore threads to float.
>> +
>> + * dpdk-mem-channels
>> + This sets the number of memory spread channels per CPU socket. It is
>> purely
>> + an optimization flag.
>> +
>> + * dpdk-alloc-mem
>> + This sets the total memory to preallocate from hugepages regardless of
>> + processor socket. It is recommended to use dpdk-socket-mem instead.
>> +
>> + * dpdk-socket-mem
>> + Comma separated list of memory to pre-allocate from hugepages on specific
>> + sockets.
>> +
>> + * dpdk-hugepage-dir
>> + Directory where hugetlbfs is mounted
>> +
>> + * cuse-dev-name
>> + Option to set the vhost_cuse character device name.
>> +
>> + * vhost-sock-dir
>> + Option to set the path to the vhost_user unix socket files.
>> +
>> + NOTE: Changing any of these options requires restarting the ovs-vswitchd
>> + application.
>> +
>> + Open vSwitch can be started as normal. DPDK will not be initialized until
>> + the first DPDK-enabled port is added to the bridge.
>
> lazy init has been removed, so this is not true anymore.
D'oh!
>>
>> ```
>> export DB_SOCK=/usr/local/var/run/openvswitch/db.sock
>> - ovs-vswitchd --dpdk -c 0x1 -n 4 -- unix:$DB_SOCK --pidfile --detach
>> + ovs-vswitchd unix:$DB_SOCK --pidfile --detach
>> ```
>>
>> If allocated more than one GB hugepage (as for IVSHMEM), set amount and
>> use NUMA node 0 memory:
>>
>> ```
>> - ovs-vswitchd --dpdk -c 0x1 -n 4 --socket-mem 1024,0 \
>> - -- unix:$DB_SOCK --pidfile --detach
>> + ovs-vsctl --no-wait set Open_vSwitch .
>> other_config:dpdk_socket_mem="1024,0"
>> + ovs-vswitchd unix:$DB_SOCK --pidfile --detach
>> ```
>>
>> 6. Add bridge & ports
>> @@ -521,11 +563,13 @@ have arbitrary names.
>> `/usr/local/var/run/openvswitch/vhost-user-1`, which you must provide
>> to your VM on the QEMU command line. More instructions on this can be
>> found in the next section "DPDK vhost-user VM configuration"
>> - Note: If you wish for the vhost-user sockets to be created in a
>> - directory other than `/usr/local/var/run/openvswitch`, you may specify
>> - another location on the ovs-vswitchd command line like so:
>>
>> - `./vswitchd/ovs-vswitchd --dpdk -vhost_sock_dir /my-dir -c 0x1 ...`
>> + - If you wish for the vhost-user sockets to be created in a directory
>> other
>> + than `/usr/local/var/run/openvswitch`, you may specify another location
>> + in the ovsdb like:
>> +
>> + `./vswitchd/ovs-vsctl --no-wait \
>> + set Open_vSwitch . other_config:vhost-sock-dir=path`
>>
>> DPDK vhost-user VM configuration:
>> ---------------------------------
>> @@ -638,14 +682,13 @@ DPDK vhost-cuse VM configuration:
>>
>> 1. This step is only needed if using an alternative character device.
>>
>> - The new character device filename must be specified on the vswitchd
>> - commandline:
>> + The new character device filename must be specified in the ovsdb:
>>
>> - `./vswitchd/ovs-vswitchd --dpdk --cuse_dev_name my-vhost-net -c 0x1
>> ...`
>> + `./utilities/ovs-vsctl --no-wait set Open_vSwitch . \
>> + other_config:cuse-dev-name=my-vhost-net`
>>
>> - Note that the `--cuse_dev_name` argument and associated string must be
>> the first
>> - arguments after `--dpdk` and come before the EAL arguments. In the
>> example
>> - above, the character device to be used will be `/dev/my-vhost-net`.
>> + In the example above, the character device to be used will be
>> + `/dev/my-vhost-net`.
>>
>> 2. This step is only needed if reusing the standard character device. It
>> will
>> conflict with the kernel vhost character device so the user must first
>> @@ -758,8 +801,8 @@ steps.
>>
>> <my-vhost-device> refers to "vhost-net" if using the `/dev/vhost-
>> net`
>> device. If you have specificed a different name on the ovs-vswitchd
>> - commandline using the "--cuse_dev_name" parameter, please specify
>> that
>> - filename instead.
>> + database using the "other_config:cuse-dev-name" parameter, please
>> + specify that filename instead.
>>
>> 2. Disable SELinux or set to permissive mode
>>
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index 78f013d..b7bb3eb 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -29,6 +29,7 @@
>> #include <stdio.h>
>> #include <sys/types.h>
>> #include <sys/stat.h>
>> +#include <getopt.h>
>>
>> #include "dirs.h"
>> #include "dp-packet.h"
>> @@ -49,6 +50,8 @@
>> #include "timeval.h"
>> #include "unixctl.h"
>> #include "openvswitch/vlog.h"
>> +#include "smap.h"
>> +#include "vswitch-idl.h"
>>
>> #include "rte_config.h"
>> #include "rte_mbuf.h"
>> @@ -551,8 +554,15 @@ netdev_dpdk_cast(const struct netdev *netdev)
>> static struct netdev *
>> netdev_dpdk_alloc(void)
>> {
>> - struct netdev_dpdk *netdev = dpdk_rte_mzalloc(sizeof *netdev);
>> - return &netdev->up;
>> + struct netdev_dpdk *netdev;
>> +
>> + if (!rte_eal_init_ret) { /* Only after successful initialization */
>> + netdev = dpdk_rte_mzalloc(sizeof *netdev);
>> + if (netdev) {
>> + return &netdev->up;
>> + }
>> + }
>> + return NULL;
>> }
>>
>> static void
>> @@ -670,6 +680,10 @@ netdev_dpdk_vhost_cuse_construct(struct netdev *netdev_)
>> struct netdev_dpdk *netdev = netdev_dpdk_cast(netdev_);
>> int err;
>>
>> + if (rte_eal_init_ret) {
>> + return rte_eal_init_ret;
>> + }
>> +
>> ovs_mutex_lock(&dpdk_mutex);
>> strncpy(netdev->vhost_id, netdev->up.name, sizeof(netdev->vhost_id));
>> err = vhost_construct_helper(netdev_);
>> @@ -683,6 +697,10 @@ netdev_dpdk_vhost_user_construct(struct netdev *netdev_)
>> struct netdev_dpdk *netdev = netdev_dpdk_cast(netdev_);
>> int err;
>>
>> + if (rte_eal_init_ret) {
>> + return rte_eal_init_ret;
>> + }
>> +
>> ovs_mutex_lock(&dpdk_mutex);
>> /* Take the name of the vhost-user port and append it to the location
>> where
>> * the socket is to be created, then register the socket.
>> @@ -1776,6 +1794,8 @@ new_device(struct virtio_net *dev)
>> struct netdev_dpdk *netdev;
>> bool exists = false;
>>
>> +
>> +
>
> added whitespace
>
> <snip>
Weird, my git hook didn't catch this. Okay, I'll fix it.
>> diff --git a/lib/netdev-dpdk.h b/lib/netdev-dpdk.h
>> index 646d3e2..cde8956 100644
>> --- a/lib/netdev-dpdk.h
>> +++ b/lib/netdev-dpdk.h
>> @@ -2,6 +2,9 @@
>> #define NETDEV_DPDK_H
>>
>> #include <config.h>
>> +#include "smap.h"
>> +#include "ovs-thread.h"
>> +#include "vswitch-idl.h"
>>
>> struct dp_packet;
>>
>> @@ -22,7 +25,9 @@ struct dp_packet;
>>
>> #define NON_PMD_CORE_ID LCORE_ID_ANY
>>
>> -int dpdk_init(int argc, char **argv);
>> +struct ovsrec_open_vswitch;
>> +
>> +void dpdk_init(const struct ovsrec_open_vswitch *ovs_cfg);
>> void netdev_dpdk_register(void);
>> void free_dpdk_buf(struct dp_packet *);
>> int pmd_thread_setaffinity_cpu(unsigned cpu);
>> @@ -30,16 +35,19 @@ int pmd_thread_setaffinity_cpu(unsigned cpu);
>> #else
>>
>> #define NON_PMD_CORE_ID UINT32_MAX
>> -
>> #include "util.h"
>>
>> -static inline int
>> -dpdk_init(int argc, char **argv)
>> +static inline void
>> +dpdk_init(const struct ovsrec_open_vswitch *ovs_cfg)
>> {
>> - if (argc >= 2 && !strcmp(argv[1], "--dpdk")) {
>> - ovs_fatal(0, "DPDK support not built into this copy of Open
>> vSwitch.");
>> + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
>> +
>> + if (ovs_cfg && ovsthread_once_start(&once)) {
>> + if(smap_get_bool(&ovs_cfg->other_config, "dpdk", false)) {
>> + ovs_fatal(0, "DPDK not supported in this copy of Open
>> vSwitch.");
>
> The db entry has changed from 'dpdk' to 'dpdk-init' in this patchset
D'oh! I knew I must've missed something.
>> + }
>> + ovsthread_once_done(&once);
>> }
>> - return 0;
>> }
>>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
