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
