> >Minor comment inline. > >Acked-by: Ilya Maximets <i.maxim...@samsung.com> >
Other than the comment mentioned by Ilya, this LGTM also - thanks again for resolving, Daniele. Acked-by: mark.b.kavan...@intel.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 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev