> 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.
Will keep it in for the reason you mention above.
>
> [...]
> > +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.
Makes sense, I've introduced a helper function in v4.
>
> [...]
> > +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.
Fixed in v4.
>
> > +
> > + /* 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.
Fixed in v4.
>
> > +
> > + *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.
I've attempted to simplify this in v4 and added in the #define as suggested.
>
> Anyway, nitpicking over tabs vs spaces should be a fair indicator its
> getting there :)
>
> - Panu -
Thanks for your feedback.
Ciara
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
