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

Aaron Conole aconole at redhat.com
Tue May 31 18:32:00 UTC 2016


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