Minor comment inline. Acked-by: Ilya Maximets <i.maxim...@samsung.com>
On 05.08.2016 23:57, Daniele Di Proietto wrote: > netdev_dpdk_vhost_destruct() calls rte_vhost_driver_unregister(), which > can trigger the destroy_device() callback. destroy_device() will try to > take two mutexes already held by netdev_dpdk_vhost_destruct(), causing a > deadlock. > > This problem can be solved by dropping the mutexes before calling > rte_vhost_driver_unregister(). The netdev_dpdk_vhost_destruct() and > construct() call are already serialized by netdev_mutex. > > This commit also makes clear that dev->vhost_id is constant and can be > accessed without taking any mutexes in the lifetime of the devices. > > Fixes: 8d38823bdf8b("netdev-dpdk: fix memory leak") > Reported-by: Ilya Maximets <i.maxim...@samsung.com> > Signed-off-by: Daniele Di Proietto <diproiet...@vmware.com> > --- > lib/netdev-dpdk.c | 34 ++++++++++++++++++++++++---------- > 1 file changed, 24 insertions(+), 10 deletions(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index f37ec1c..98bff62 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -355,8 +355,10 @@ struct netdev_dpdk { > /* True if vHost device is 'up' and has been reconfigured at least once > */ > bool vhost_reconfigured; > > - /* Identifier used to distinguish vhost devices from each other */ > - char vhost_id[PATH_MAX]; > + /* Identifier used to distinguish vhost devices from each other. It does > + * not change during the lifetime of a struct netdev_dpdk. It can be > read > + * without holding any mutex. */ > + const char vhost_id[PATH_MAX]; > > /* In dpdk_list. */ > struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex); > @@ -846,7 +848,8 @@ netdev_dpdk_vhost_cuse_construct(struct netdev *netdev) > } > > ovs_mutex_lock(&dpdk_mutex); > - strncpy(dev->vhost_id, netdev->name, sizeof(dev->vhost_id)); > + strncpy(CONST_CAST(char *, dev->vhost_id), netdev->name, > + sizeof dev->vhost_id); > err = vhost_construct_helper(netdev); > ovs_mutex_unlock(&dpdk_mutex); > return err; > @@ -878,7 +881,7 @@ netdev_dpdk_vhost_user_construct(struct netdev *netdev) > /* Take the name of the vhost-user port and append it to the location > where > * the socket is to be created, then register the socket. > */ > - snprintf(dev->vhost_id, sizeof(dev->vhost_id), "%s/%s", > + snprintf(CONST_CAST(char *,dev->vhost_id), sizeof(dev->vhost_id), > "%s/%s", Space between arguments of 'CONST_CAST()' and parenthesized operand of 'sizeof'. > vhost_sock_dir, name); > > err = rte_vhost_driver_register(dev->vhost_id, flags); > @@ -938,6 +941,17 @@ netdev_dpdk_destruct(struct netdev *netdev) > ovs_mutex_unlock(&dpdk_mutex); > } > > +/* rte_vhost_driver_unregister() can call back destroy_device(), which will > + * try to acquire 'dpdk_mutex' and possibly 'dev->mutex'. To avoid a > + * deadlock, none of the mutexes must be held while calling this function. */ > +static int > +dpdk_vhost_driver_unregister(struct netdev_dpdk *dev) > + OVS_EXCLUDED(dpdk_mutex) > + OVS_EXCLUDED(dev->mutex) > +{ > + return rte_vhost_driver_unregister(dev->vhost_id); > +} > + > static void > netdev_dpdk_vhost_destruct(struct netdev *netdev) > { > @@ -955,12 +969,6 @@ netdev_dpdk_vhost_destruct(struct netdev *netdev) > dev->vhost_id); > } > > - if (rte_vhost_driver_unregister(dev->vhost_id)) { > - VLOG_ERR("Unable to remove vhost-user socket %s", dev->vhost_id); > - } else { > - fatal_signal_remove_file_to_unlink(dev->vhost_id); > - } > - > free(ovsrcu_get_protected(struct ingress_policer *, > &dev->ingress_policer)); > > @@ -970,6 +978,12 @@ netdev_dpdk_vhost_destruct(struct netdev *netdev) > > ovs_mutex_unlock(&dev->mutex); > ovs_mutex_unlock(&dpdk_mutex); > + > + if (dpdk_vhost_driver_unregister(dev)) { > + VLOG_ERR("Unable to remove vhost-user socket %s", dev->vhost_id); > + } else { > + fatal_signal_remove_file_to_unlink(dev->vhost_id); > + } > } > > static void > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev