On 05/11/2015 01:56 PM, Ciara Loftus wrote:
This patch adds support for a new port type to the userspace
datapath called dpdkvhostuser.
A new dpdkvhostuser port will create a unix domain socket which
when provided to QEMU is used to facilitate communication between
the virtio-net device on the VM and the OVS port on the host.
vhost-cuse ('dpdkvhost') ports are still available, and will be
enabled if vhost-cuse support is detected in the DPDK build
specified during compilation of the switch. Otherwise, vhost-user
ports are enabled.
Signed-off-by: Ciara Loftus <[email protected]>
---
[...]
diff --git a/acinclude.m4 b/acinclude.m4
index e9d0ed9..2873480 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -218,6 +218,9 @@ AC_DEFUN([OVS_CHECK_DPDK], [
DPDK_vswitchd_LDFLAGS=-Wl,--whole-archive,$DPDK_LIB,--no-whole-archive
AC_SUBST([DPDK_vswitchd_LDFLAGS])
AC_DEFINE([DPDK_NETDEV], [1], [System uses the DPDK module.])
+
+ OVS_GREP_IFELSE([$RTE_SDK/include/rte_config.h], [define
RTE_LIBRTE_VHOST_USER 1],
+ [], [AC_DEFINE([VHOST_CUSE], [1], [DPDK vhost-cuse support
enabled, vhost-user disabled.])])
else
RTE_SDK=
fi
This isn't really needed, you could just include rte_config.h from
netdev-dpdk.c ... but maybe this is better afterall as it leaves a trace
in the build log as to which version was chosen.
[...]
+static int
+new_device_vhost_user(struct virtio_net *dev)
+{
+ struct netdev_dpdk *netdev;
+ bool exists = false;
+
+ ovs_mutex_lock(&dpdk_mutex);
+ /* Add device to the vhost port with the same name as that passed down. */
+ LIST_FOR_EACH(netdev, list_node, &dpdk_list) {
+ if (strncmp(dev->ifname, netdev->socket_path, IF_NAME_SZ) == 0) {
+ ovs_mutex_lock(&netdev->mutex);
+ ovsrcu_set(&netdev->virtio_dev, dev);
+ ovs_mutex_unlock(&netdev->mutex);
+ exists = true;
+ dev->flags |= VIRTIO_DEV_RUNNING;
+ /* Disable notifications. */
+ set_irq_status(dev);
+ break;
+ }
+ }
+ ovs_mutex_unlock(&dpdk_mutex);
+
+ if (!exists) {
+ VLOG_INFO("vHost Device '%s' (%ld) can't be added - name not found",
+ dev->ifname, dev->device_fh);
+
+ return -1;
+ }
+
+ VLOG_INFO("vHost Device '%s' (%ld) has been added",
+ dev->ifname, dev->device_fh);
+ return 0;
+}
+
/*
* Remove a virtio-net device from the specific vhost port. Use dev->remove
* flag to stop any more packets from being sent or received to/from a VM and
Sorry for missing this the last time around, but this too seems like
unwanted code duplication since the whole thing differs from the vhost
case by just that one strncmp() line. Its a little trickier than the
constructor case since the struct member to compare differs but solvable.
[...]
+static int
+process_vhost_flags(char* flag, char* default_val, int size, char** argv,
char** new_val)
+{
+ int changed = 0;
The indentation is off here, there's a tab where other code on this
level is at four spaces.
+
+ /* Depending on which version of vhost is in use, process the
vhost-specific
+ * flag if it is provided on the vswitchd command line, otherwise resort to
+ * a default value.
+ *
+ * For vhost-user: Process "--cuse_dev_name" to set the custom location of
+ * the vhost-user socket(s).
+ * For vhost-cuse: Process "--vhost_sock_dir" to set the custom name of the
+ * vhost-cuse character device.
+ */
+ if (!strcmp(argv[1], flag) &&
+ (strlen(argv[2]) <= size)) {
Why the line-split? This fits easily on one line ... okay it was that
way already, but no reason not to "fix" it when moving around.
+
+ *new_val = strdup(argv[2]);
+
+ VLOG_ERR("User-provided %s in use: %s", flag, *new_val);
+ changed = 1;
+ } else {
+ *new_val = default_val;
+ VLOG_INFO("No %s provided - defaulting to %s", flag, default_val);
+ }
+
+ return changed;
+}
+
int
dpdk_init(int argc, char **argv)
{
int result;
int base = 0;
char *pragram_name = argv[0];
+ int flag_processed = 0;
if (argc < 2 || strcmp(argv[1], "--dpdk"))
return 0;
@@ -1869,27 +1983,17 @@ dpdk_init(int argc, char **argv)
argc--;
argv++;
- /* If the cuse_dev_name parameter has been provided, set 'cuse_dev_name' to
- * this string if it meets the correct criteria. Otherwise, set it to the
- * default (vhost-net).
- */
- if (!strcmp(argv[1], "--cuse_dev_name") &&
- (strlen(argv[2]) <= NAME_MAX)) {
-
- cuse_dev_name = strdup(argv[2]);
+ flag_processed = (process_vhost_flags("--cuse_dev_name", "vhost-net",
PATH_MAX, argv, &cuse_dev_name) ||
+ process_vhost_flags("--vhost_sock_dir", strdup(ovs_rundir()),
NAME_MAX, argv, &vhost_sock_dir));
This seems problematic in a number of ways:
- The lines are rather long and wrap nastily on a 80 char terminal.
- For what it does, its unnecessarily hard to read and understand. It
seems like it accepts both variants (and sort of does) but only the
first will actually work, and then the rest will get passed down to dpdk
which will choke on it. Or something like that.
- AFAICS it'll mumble about vhost-cuse defaults when vhost-cuse is not
even enabled, and about vhost-user on vhost-cuse build if defaults are used.
Certainly there are any number of ways to make it clearer and all but at
least I wouldn't mind a simple #ifdef here as it makes the thing so
blatantly obvious.
In addition, the cuse_dev_name/vhost_sock_dir allocation is
inconsistent: in other cases its strdup()'ed but default cuse name is
not. Doesn't matter technically since nothing is freeing it at the
moment but in my experience its better to avoid such inconsistencies in
the first place, they have a tendency to come back to bite you.
Anyway, nitpicking over tabs vs spaces should be a fair indicator its
getting there :)
- Panu -
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
