[ovs-dev] [PATCH] ovsdb: Expose vhost-user socket directory in ovsdb

Wojciechowicz, RobertX robertx.wojciechowicz at intel.com
Wed Jun 1 09:48:32 UTC 2016


Hi Aaron,

thank you for review.
I will address your suggestions and prepare the second version of this patch.

Br,
Robert

> -----Original Message-----
> From: Aaron Conole [mailto:aconole at redhat.com]
> Sent: Tuesday, May 31, 2016 8:32 PM
> To: Wojciechowicz, RobertX <robertx.wojciechowicz at intel.com>
> Cc: dev at openvswitch.org
> Subject: Re: [ovs-dev] [PATCH] ovsdb: Expose vhost-user socket directory in
> ovsdb
> 
> 
> Hi Robert,
> 
> Robert Wojciechowicz <robertx.wojciechowicz at intel.com> writes:
> 
> > In order to correctly interoperate with Openstack and ODL,
> > the vhost-user socket directory must be exposed from OVS via OVSDB.
> > Different distros may package OVS in different ways,
> > so the locations of these sockets may vary depending on how
> > ovs-vswitchd has been started. Some clients need information where
> > the sockets are located when instantiating Qemu virtual machines.
> > The full vhost-user socket directory is constructed from current
> > OVS working directory and optionally from specified subdirectory.
> > This patch exposes vhost-user socket directory in Open_vSwitch
> > table in other_config column in two following keys:
> > 1. ovs-run-dir    - OVS working directory
> > 2. vhost-sock-dir - subdirectory of ovs-run-dir (might be empty)
> >
> > Signed-off-by: Robert Wojciechowicz <robertx.wojciechowicz at intel.com>
> 
> I think this idea makes sense.  Neutron plugin would want to know this
> sort of thing.  I don't know much about the thinking w.r.t. putting
> these kinds of values in database responses, but it seems to be a good
> place for interop with other projects.
> 
> Just some nits below.
> 
> > ---
> >  lib/netdev-dpdk.c    | 38 ++++++++++++++++++++++++++++++++++----
> >  lib/netdev-dpdk.h    |  9 +++++++++
> >  vswitchd/bridge.c    |  2 ++
> >  vswitchd/vswitch.xml | 11 +++++++++++
> >  4 files changed, 56 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> > index 9ffa7ff..4e68bd6 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -138,9 +138,13 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF /
> ROUND_DOWN_POW2(MAX_NB_MBUF/MIN_NB_MBUF))
> >
> >  #ifdef VHOST_CUSE
> >  static char *cuse_dev_name = NULL;    /* Character device
> cuse_dev_name. */
> > +#else
> > +static char *sock_dir_subcomponent = NULL;   /* Subdir of OVS run dir
> > +                                                for vhost-user sockets */
> >  #endif
> >  static char *vhost_sock_dir = NULL;   /* Location of vhost-user sockets */
> >
> > +
> >  /*
> >   * Maximum amount of time in micro seconds to try and enqueue to vhost.
> >   */
> > @@ -3086,9 +3090,6 @@ dpdk_init__(const struct smap
> *ovs_other_config)
> >      bool auto_determine = true;
> >      int err = 0;
> >      cpu_set_t cpuset;
> > -#ifndef VHOST_CUSE
> > -    char *sock_dir_subcomponent;
> > -#endif
> >
> >      if (!smap_get_bool(ovs_other_config, "dpdk-init", false)) {
> >          VLOG_INFO("DPDK Disabled - to change this requires a restart.\n");
> > @@ -3119,10 +3120,11 @@ dpdk_init__(const struct smap
> *ovs_other_config)
> >              VLOG_ERR("vhost-user sock directory request '%s/%s' has invalid"
> >                       "characters '..' - using %s instead.",
> >                       ovs_rundir(), sock_dir_subcomponent, ovs_rundir());
> > +            sock_dir_subcomponent = "";
> >          }
> > -        free(sock_dir_subcomponent);
> >      } else {
> >          vhost_sock_dir = sock_dir_subcomponent;
> > +        sock_dir_subcomponent = "";
> >  #endif
> >      }
> >
> > @@ -3244,6 +3246,34 @@ dpdk_init(const struct smap *ovs_other_config)
> >      }
> >  }
> >
> > +void
> > +dpdk_set_config(const struct ovsrec_open_vswitch *cfg)
> > +{
> > +    struct smap dpdk_args;
> > +    const struct ovsdb_datum *datum;
> > +    size_t i;
> > +
> > +    ovs_mutex_lock(&dpdk_mutex);
> > +
> > +    smap_init(&dpdk_args);
> > +    /* read current values from database */
> > +    datum = ovsrec_open_vswitch_get_other_config(cfg,
> OVSDB_TYPE_STRING,
> > +                                                 OVSDB_TYPE_STRING);
> > +    for (i = 0; i < datum->n; i++) {
> > +      smap_add(&dpdk_args, datum->keys[i].string, datum-
> >values[i].string);
> > +    }
> > +    /* add default paths to the database */
> > +    smap_add_format(&dpdk_args, "ovs-run-dir", "%s", ovs_rundir());
> > +    if (sock_dir_subcomponent) {
> 
> I never see sock_dir_subcomponent go null.  Does that make this check
> useless?  If this is solely to protect against the 'race' where the dpdk
> init hasn't been called, could you just initialize that var to ""
> instead of NULL?
> 
> > +      smap_add_format(&dpdk_args, "vhost-sock-dir", "%s",
> > +                      sock_dir_subcomponent);
> > +    }
> > +    ovsrec_open_vswitch_set_other_config(cfg, &dpdk_args);
> > +    smap_destroy(&dpdk_args);
> > +
> > +    ovs_mutex_unlock(&dpdk_mutex);
> > +}
> > +
> >  static const struct netdev_class dpdk_class =
> >      NETDEV_DPDK_CLASS(
> >          "dpdk",
> > diff --git a/lib/netdev-dpdk.h b/lib/netdev-dpdk.h
> > index ee748e0..08abcac 100644
> > --- a/lib/netdev-dpdk.h
> > +++ b/lib/netdev-dpdk.h
> > @@ -3,6 +3,8 @@
> >
> >  #include <config.h>
> >
> > +#include "lib/vswitch-idl.h"
> > +
> 
> Don't include this here.  Forward declare the ovsrec_open_vswitch
> struct.
> 
> >  struct dp_packet;
> >  struct smap;
> >
> > @@ -25,6 +27,7 @@ struct smap;
> >
> >  void netdev_dpdk_register(void);
> >  void free_dpdk_buf(struct dp_packet *);
> > +void dpdk_set_config(const struct ovsrec_open_vswitch *cfg);
> 
> Move this out of the #if block.
> 
> >  int pmd_thread_setaffinity_cpu(unsigned cpu);
> >
> >  #else
> > @@ -45,6 +48,12 @@ free_dpdk_buf(struct dp_packet *buf
> OVS_UNUSED)
> >      /* Nothing */
> >  }
> >
> > +static inline void
> > +dpdk_set_config(const struct ovsrec_open_vswitch *cfg)
> > +{
> > +    /* Nothing */
> > +}
> > +
> 
> And move this to the netdev-nodpdk.c file.  Otherwise, this change will
> add extra dependencies for the vswitch idl, and I think that should be
> avoided.
> 
> >  static inline int
> >  pmd_thread_setaffinity_cpu(unsigned cpu OVS_UNUSED)
> >  {
> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> > index 41ec4ba..d248721 100644
> > --- a/vswitchd/bridge.c
> > +++ b/vswitchd/bridge.c
> > @@ -393,6 +393,7 @@ bridge_init(const char *remote)
> >      ovsdb_idl_omit(idl, &ovsrec_open_vswitch_col_db_version);
> >      ovsdb_idl_omit(idl, &ovsrec_open_vswitch_col_system_type);
> >      ovsdb_idl_omit(idl, &ovsrec_open_vswitch_col_system_version);
> > +    ovsdb_idl_omit_alert(idl, &ovsrec_open_vswitch_col_other_config);
> >
> >      ovsdb_idl_omit_alert(idl, &ovsrec_bridge_col_datapath_id);
> >      ovsdb_idl_omit_alert(idl, &ovsrec_bridge_col_datapath_version);
> > @@ -2957,6 +2958,7 @@ bridge_run(void)
> >
> >          if (cfg) {
> >              ovsrec_open_vswitch_set_cur_cfg(cfg, cfg->next_cfg);
> > +            dpdk_set_config(cfg);
> >              discover_types(cfg);
> >          }
> >
> > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> > index 944d8ec..8d06d29 100644
> > --- a/vswitchd/vswitch.xml
> > +++ b/vswitchd/vswitch.xml
> > @@ -311,6 +311,17 @@
> >          </p>
> >        </column>
> >
> > +      <column name="other_config" key="ovs-run-dir"
> > +              type='{"type": "string"}'>
> > +        <p>
> > +          Specifies the Open vSwitch run directory.
> > +        </p>
> > +        <p>
> > +          Defaults to the working directory of the application. Changing this
> > +          value requires restarting the daemon.
> > +        </p>
> > +      </column>
> > +
> >        <column name="other_config" key="n-handler-threads"
> >                type='{"type": "integer", "minInteger": 1}'>
> >          <p>



More information about the dev mailing list