Re: [ovs-dev] netdev-dpdk: Fix deadlock in destroy_device().

2016-08-09 Thread Daniele Di Proietto 





On 08/08/2016 01:02, "Kavanagh, Mark B"  wrote:

>>
>>Minor comment inline.
>>
>>Acked-by: Ilya Maximets 
>>
>
>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 
>>> Signed-off-by: Daniele Di Proietto 
>>> ---
>>>  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(_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(_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(_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 *,
>>>>ingress_policer));
>>>
>>> @@ -970,6 +978,12 @@ netdev_dpdk_vhost_destruct(struct netdev *netdev)
>>>
>>>  ovs_mutex_unlock(>mutex);
>>>  ovs_mutex_unlock(_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

Re: [ovs-dev] netdev-dpdk: Fix deadlock in destroy_device().

2016-08-08 Thread Kavanagh, Mark B 
>
>Minor comment inline.
>
>Acked-by: Ilya Maximets 
>

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 
>> Signed-off-by: Daniele Di Proietto 
>> ---
>>  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(_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(_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(_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 *,
>>>ingress_policer));
>>
>> @@ -970,6 +978,12 @@ netdev_dpdk_vhost_destruct(struct netdev *netdev)
>>
>>  ovs_mutex_unlock(>mutex);
>>  ovs_mutex_unlock(_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

Re: [ovs-dev] netdev-dpdk: Fix deadlock in destroy_device().

2016-08-08 Thread Ilya Maximets 
Minor comment inline.

Acked-by: Ilya Maximets 

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 
> Signed-off-by: Daniele Di Proietto 
> ---
>  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(_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(_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(_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 *,
>>ingress_policer));
>  
> @@ -970,6 +978,12 @@ netdev_dpdk_vhost_destruct(struct netdev *netdev)
>  
>  ovs_mutex_unlock(>mutex);
>  ovs_mutex_unlock(_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