| Message ID | 1463999326-3637-1-git-send-email-robertx.wojciechowicz@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi, this patch is actually a supplement to the conversion of DPDK configuration from command line to DB (http://openvswitch.org/pipermail/dev/2016-April/070308.html). Currently the full vhost-user socket directory consists of two components: ovs_rundir and subdirectory for vhost-user sockets. Combining them user can determine the full vhost-sock directory. Currently the default vhost-sock directory is not available in OVSDB, so I exposed those components in this patch. Br, Robert > -----Original Message----- > From: Wojciechowicz, RobertX > Sent: Monday, May 23, 2016 12:29 PM > To: dev@openvswitch.org > Cc: Wojciechowicz, RobertX <robertx.wojciechowicz@intel.com> > Subject: [PATCH] ovsdb: Expose vhost-user socket directory in ovsdb > > 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@intel.com> > --- > 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) { > + 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" > + > 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); > 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 */ > +} > + > 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> > -- > 1.8.3.1
Hi, did anyone by any chance have time to have a look at this patch? Br, Robert > -----Original Message----- > From: dev [mailto:dev-bounces@openvswitch.org] On Behalf Of > Wojciechowicz, RobertX > Sent: Monday, May 23, 2016 12:45 PM > To: dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH] ovsdb: Expose vhost-user socket directory in > ovsdb > > Hi, > > this patch is actually a supplement to the conversion of DPDK configuration > from command line to DB (http://openvswitch.org/pipermail/dev/2016- > April/070308.html). > Currently the full vhost-user socket directory consists of two components: > ovs_rundir and subdirectory for vhost-user sockets. Combining them user > can > determine the full vhost-sock directory. > Currently the default vhost-sock directory is not available in OVSDB, > so I exposed those components in this patch. > > Br, > Robert > > > -----Original Message----- > > From: Wojciechowicz, RobertX > > Sent: Monday, May 23, 2016 12:29 PM > > To: dev@openvswitch.org > > Cc: Wojciechowicz, RobertX <robertx.wojciechowicz@intel.com> > > Subject: [PATCH] ovsdb: Expose vhost-user socket directory in ovsdb > > > > 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@intel.com> > > --- > > 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) { > > + 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" > > + > > 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); > > 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 */ > > +} > > + > > 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> > > -- > > 1.8.3.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
Hi Robert, one comment below. I've checked it applies cleanly to the latest master branch. Also with utilities/checkpatch.py is ok. Thanks, Antonio > -----Original Message----- > From: dev [mailto:dev-bounces@openvswitch.org] On Behalf Of Robert > Wojciechowicz > Sent: Monday, May 23, 2016 11:29 AM > To: dev@openvswitch.org > Subject: [ovs-dev] [PATCH] ovsdb: Expose vhost-user socket directory > in ovsdb > > 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@intel.com> > --- > 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) [Antonio F] Just wondering if inside the previous lines where the vhu sock does not exist, should we consider to restore vhost_sock_dir to ovs_rundir(), like: err = stat(vhost_sock_dir, &s); if (err) { VLOG_ERR("vhost-user sock directory '%s' does not exist.", vhost_sock_dir); + vhost_sock_dir = xasprintf("%s", ovs_rundir()); + sock_dir_subcomponent = ""; } ? Or is that ok to leave it as it is now? > 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) { > + 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" > + > 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); > 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 */ > +} > + > 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> > -- > 1.8.3.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
"Fischetti, Antonio" <antonio.fischetti@intel.com> writes: > Hi Robert, > one comment below. > I've checked it applies cleanly to the latest master branch. > Also with utilities/checkpatch.py is ok. > > Thanks, > Antonio > >> -----Original Message----- >> From: dev [mailto:dev-bounces@openvswitch.org] On Behalf Of Robert >> Wojciechowicz >> Sent: Monday, May 23, 2016 11:29 AM >> To: dev@openvswitch.org >> Subject: [ovs-dev] [PATCH] ovsdb: Expose vhost-user socket directory >> in ovsdb >> >> 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@intel.com> >> --- >> 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) > > [Antonio F] Just wondering if inside the previous lines > where the vhu sock does not exist, should we consider to > restore vhost_sock_dir to ovs_rundir(), like: > > err = stat(vhost_sock_dir, &s); > if (err) { > VLOG_ERR("vhost-user sock directory '%s' does not exist.", > vhost_sock_dir); > > + vhost_sock_dir = xasprintf("%s", ovs_rundir()); > + sock_dir_subcomponent = ""; > > } > > ? > Or is that ok to leave it as it is now? I haven't reviewed the whole patch, but I don't think this would be an appropriate change. As it stands, if a user sees this error message, they may simply mkdir the missing subdirectory, and then dpdk vhost-user sockets will begin to work. With your change, not only will the error message be displayed, but user sockets will be popping up in unexpected places, meaning a restart will be required to resolve the issue fully. >> 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) { >> + 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" >> + >> 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); >> 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 */ >> +} >> + >> 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> >> -- >> 1.8.3.1 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> http://openvswitch.org/mailman/listinfo/dev > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
Hi Robert, Robert Wojciechowicz <robertx.wojciechowicz@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@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>
Ok, it makes sense. It looks ok to me now. Acked-by: Antonio Fischetti <antonio.fischetti@intel.com> > -----Original Message----- > From: Aaron Conole [mailto:aconole@redhat.com] > Sent: Tuesday, May 31, 2016 7:20 PM > To: Fischetti, Antonio <antonio.fischetti@intel.com> > Cc: Wojciechowicz, RobertX <robertx.wojciechowicz@intel.com>; > dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH] ovsdb: Expose vhost-user socket > directory in ovsdb > > "Fischetti, Antonio" <antonio.fischetti@intel.com> writes: > > > Hi Robert, > > one comment below. > > I've checked it applies cleanly to the latest master branch. > > Also with utilities/checkpatch.py is ok. > > > > Thanks, > > Antonio > > > >> -----Original Message----- > >> From: dev [mailto:dev-bounces@openvswitch.org] On Behalf Of Robert > >> Wojciechowicz > >> Sent: Monday, May 23, 2016 11:29 AM > >> To: dev@openvswitch.org > >> Subject: [ovs-dev] [PATCH] ovsdb: Expose vhost-user socket > directory > >> in ovsdb > >> > >> 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@intel.com> > >> --- > >> 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) > > > > [Antonio F] Just wondering if inside the previous lines > > where the vhu sock does not exist, should we consider to > > restore vhost_sock_dir to ovs_rundir(), like: > > > > err = stat(vhost_sock_dir, &s); > > if (err) { > > VLOG_ERR("vhost-user sock directory '%s' does not > exist.", > > vhost_sock_dir); > > > > + vhost_sock_dir = xasprintf("%s", ovs_rundir()); > > + sock_dir_subcomponent = ""; > > > > } > > > > ? > > Or is that ok to leave it as it is now? > > I haven't reviewed the whole patch, but I don't think this would be > an > appropriate change. As it stands, if a user sees this error message, > they may simply mkdir the missing subdirectory, and then dpdk vhost- > user > sockets will begin to work. With your change, not only will the > error > message be displayed, but user sockets will be popping up in > unexpected > places, meaning a restart will be required to resolve the issue > fully. > > >> 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) { > >> + 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" > >> + > >> 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); > >> 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 */ > >> +} > >> + > >> 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> > >> -- > >> 1.8.3.1 > >> > >> _______________________________________________ > >> dev mailing list > >> dev@openvswitch.org > >> http://openvswitch.org/mailman/listinfo/dev > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev
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@redhat.com] > Sent: Tuesday, May 31, 2016 8:32 PM > To: Wojciechowicz, RobertX <robertx.wojciechowicz@intel.com> > Cc: dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH] ovsdb: Expose vhost-user socket directory in > ovsdb > > > Hi Robert, > > Robert Wojciechowicz <robertx.wojciechowicz@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@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>
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) { + 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" + 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); 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 */ +} + 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>
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@intel.com> --- 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(-)