On 08/08/2016 01:02, "Kavanagh, Mark B" <mark.b.kavan...@intel.com> wrote:
>>
>>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
Thanks for the reviews, I applied this to master
>
>>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'.
Fixed, thanks
>>
>>> 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
