"Fischetti, Antonio" <antonio.fische...@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-boun...@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.wojciechow...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
