diff mbox series

[1/3] net/virtio-user: fix deadlock in memory events callback

Message ID 20180905042852.6212-2-tiwei.bie@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Maxime Coquelin
Headers
Series Some fixes/improvements for virtio-user memory table |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Tiwei Bie Sept. 5, 2018, 4:28 a.m. UTC 
  Deadlock can occur when allocating memory if a vhost-kernel
based virtio-user device is in use. To fix the deadlock,
we will take memory hotplug lock explicitly in virtio-user
when necessary, and always call the _thread_unsafe memory
functions.

Bugzilla ID: 81
Fixes: 12ecb2f63b12 ("net/virtio-user: support memory hotplug")
Cc: stable@dpdk.org

Reported-by: Seán Harte <seanbh@gmail.com>
Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 drivers/net/virtio/virtio_user/vhost_kernel.c |  6 +++++-
 .../net/virtio/virtio_user/virtio_user_dev.c  | 19 +++++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

Comments

Sean Harte Sept. 5, 2018, 8:10 a.m. UTC | #1 
On 5 September 2018 at 05:28, Tiwei Bie <tiwei.bie@intel.com> wrote:
> Deadlock can occur when allocating memory if a vhost-kernel
> based virtio-user device is in use. To fix the deadlock,
> we will take memory hotplug lock explicitly in virtio-user
> when necessary, and always call the _thread_unsafe memory
> functions.
>
> Bugzilla ID: 81
> Fixes: 12ecb2f63b12 ("net/virtio-user: support memory hotplug")
> Cc: stable@dpdk.org
>
> Reported-by: Seán Harte <seanbh@gmail.com>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>  drivers/net/virtio/virtio_user/vhost_kernel.c |  6 +++++-
>  .../net/virtio/virtio_user/virtio_user_dev.c  | 19 +++++++++++++++++++
>  2 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio/virtio_user/vhost_kernel.c b/drivers/net/virtio/virtio_user/vhost_kernel.c
> index b2444096c..897fee0af 100644
> --- a/drivers/net/virtio/virtio_user/vhost_kernel.c
> +++ b/drivers/net/virtio/virtio_user/vhost_kernel.c
> @@ -115,7 +115,11 @@ prepare_vhost_memory_kernel(void)
>         wa.region_nr = 0;
>         wa.vm = vm;
>
> -       if (rte_memseg_contig_walk(add_memory_region, &wa) < 0) {
> +       /*
> +        * The memory lock has already been taken by memory subsystem
> +        * or virtio_user_start_device().
> +        */
> +       if (rte_memseg_contig_walk_thread_unsafe(add_memory_region, &wa) < 0) {
>                 free(vm);
>                 return NULL;
>         }
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> index 7df600b02..869e96f87 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> @@ -13,6 +13,8 @@
>  #include <sys/types.h>
>  #include <sys/stat.h>
>
> +#include <rte_eal_memconfig.h>
> +
>  #include "vhost.h"
>  #include "virtio_user_dev.h"
>  #include "../virtio_ethdev.h"
> @@ -109,9 +111,24 @@ is_vhost_user_by_type(const char *path)
>  int
>  virtio_user_start_device(struct virtio_user_dev *dev)
>  {
> +       struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
>         uint64_t features;
>         int ret;
>
> +       /*
> +        * XXX workaround!
> +        *
> +        * We need to make sure that the locks will be
> +        * taken in the correct order to avoid deadlocks.
> +        *
> +        * Before releasing this lock, this thread should
> +        * not trigger any memory hotplug events.
> +        *
> +        * This is a temporary workaround, and should be
> +        * replaced when we get proper supports from the
> +        * memory subsystem in the future.
> +        */
> +       rte_rwlock_read_lock(&mcfg->memory_hotplug_lock);
>         pthread_mutex_lock(&dev->mutex);
>
>         if (is_vhost_user_by_type(dev->path) && dev->vhostfd < 0)
> @@ -152,10 +169,12 @@ virtio_user_start_device(struct virtio_user_dev *dev)
>
>         dev->started = true;
>         pthread_mutex_unlock(&dev->mutex);
> +       rte_rwlock_read_unlock(&mcfg->memory_hotplug_lock);
>
>         return 0;
>  error:
>         pthread_mutex_unlock(&dev->mutex);
> +       rte_rwlock_read_unlock(&mcfg->memory_hotplug_lock);
>         /* TODO: free resource here or caller to check */
>         return -1;
>  }
> --
> 2.18.0
>

Tested-by: Seán Harte <seanbh@gmail.com>
Reviewed-by: Seán Harte <seanbh@gmail.com>
Burakov, Anatoly Sept. 7, 2018, 9:30 a.m. UTC | #2 
On 05-Sep-18 5:28 AM, Tiwei Bie wrote:
> Deadlock can occur when allocating memory if a vhost-kernel
> based virtio-user device is in use. To fix the deadlock,
> we will take memory hotplug lock explicitly in virtio-user
> when necessary, and always call the _thread_unsafe memory
> functions.
> 
> Bugzilla ID: 81
> Fixes: 12ecb2f63b12 ("net/virtio-user: support memory hotplug")
> Cc: stable@dpdk.org
> 
> Reported-by: Seán Harte <seanbh@gmail.com>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>   drivers/net/virtio/virtio_user/vhost_kernel.c |  6 +++++-
>   .../net/virtio/virtio_user/virtio_user_dev.c  | 19 +++++++++++++++++++
>   2 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio/virtio_user/vhost_kernel.c b/drivers/net/virtio/virtio_user/vhost_kernel.c
> index b2444096c..897fee0af 100644
> --- a/drivers/net/virtio/virtio_user/vhost_kernel.c
> +++ b/drivers/net/virtio/virtio_user/vhost_kernel.c
> @@ -115,7 +115,11 @@ prepare_vhost_memory_kernel(void)
>   	wa.region_nr = 0;
>   	wa.vm = vm;
>   
> -	if (rte_memseg_contig_walk(add_memory_region, &wa) < 0) {
> +	/*
> +	 * The memory lock has already been taken by memory subsystem
> +	 * or virtio_user_start_device().
> +	 */
> +	if (rte_memseg_contig_walk_thread_unsafe(add_memory_region, &wa) < 0) {
>   		free(vm);
>   		return NULL;
>   	}
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> index 7df600b02..869e96f87 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> @@ -13,6 +13,8 @@
>   #include <sys/types.h>
>   #include <sys/stat.h>
>   
> +#include <rte_eal_memconfig.h>
> +
>   #include "vhost.h"
>   #include "virtio_user_dev.h"
>   #include "../virtio_ethdev.h"
> @@ -109,9 +111,24 @@ is_vhost_user_by_type(const char *path)
>   int
>   virtio_user_start_device(struct virtio_user_dev *dev)
>   {
> +	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
>   	uint64_t features;
>   	int ret;
>   
> +	/*
> +	 * XXX workaround!
> +	 *
> +	 * We need to make sure that the locks will be
> +	 * taken in the correct order to avoid deadlocks.
> +	 *
> +	 * Before releasing this lock, this thread should
> +	 * not trigger any memory hotplug events.
> +	 *
> +	 * This is a temporary workaround, and should be
> +	 * replaced when we get proper supports from the
> +	 * memory subsystem in the future.
> +	 */
> +	rte_rwlock_read_lock(&mcfg->memory_hotplug_lock);
>   	pthread_mutex_lock(&dev->mutex);
>   
>   	if (is_vhost_user_by_type(dev->path) && dev->vhostfd < 0)
> @@ -152,10 +169,12 @@ virtio_user_start_device(struct virtio_user_dev *dev)
>   
>   	dev->started = true;
>   	pthread_mutex_unlock(&dev->mutex);
> +	rte_rwlock_read_unlock(&mcfg->memory_hotplug_lock);
>   
>   	return 0;
>   error:
>   	pthread_mutex_unlock(&dev->mutex);
> +	rte_rwlock_read_unlock(&mcfg->memory_hotplug_lock);
>   	/* TODO: free resource here or caller to check */
>   	return -1;
>   }
> 

For the memory parts,

Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
Maxime Coquelin Sept. 11, 2018, 12:52 p.m. UTC | #3 
On 09/05/2018 06:28 AM, Tiwei Bie wrote:
> Deadlock can occur when allocating memory if a vhost-kernel
> based virtio-user device is in use. To fix the deadlock,
> we will take memory hotplug lock explicitly in virtio-user
> when necessary, and always call the _thread_unsafe memory
> functions.
> 
> Bugzilla ID: 81
> Fixes: 12ecb2f63b12 ("net/virtio-user: support memory hotplug")
> Cc: stable@dpdk.org
> 
> Reported-by: Seán Harte <seanbh@gmail.com>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>   drivers/net/virtio/virtio_user/vhost_kernel.c |  6 +++++-
>   .../net/virtio/virtio_user/virtio_user_dev.c  | 19 +++++++++++++++++++
>   2 files changed, 24 insertions(+), 1 deletion(-)
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime
diff mbox series

Patch

diff --git a/drivers/net/virtio/virtio_user/vhost_kernel.c b/drivers/net/virtio/virtio_user/vhost_kernel.c
index b2444096c..897fee0af 100644
--- a/drivers/net/virtio/virtio_user/vhost_kernel.c
+++ b/drivers/net/virtio/virtio_user/vhost_kernel.c
@@ -115,7 +115,11 @@  prepare_vhost_memory_kernel(void)
 	wa.region_nr = 0;
 	wa.vm = vm;
 
-	if (rte_memseg_contig_walk(add_memory_region, &wa) < 0) {
+	/*
+	 * The memory lock has already been taken by memory subsystem
+	 * or virtio_user_start_device().
+	 */
+	if (rte_memseg_contig_walk_thread_unsafe(add_memory_region, &wa) < 0) {
 		free(vm);
 		return NULL;
 	}
diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 7df600b02..869e96f87 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -13,6 +13,8 @@ 
 #include <sys/types.h>
 #include <sys/stat.h>
 
+#include <rte_eal_memconfig.h>
+
 #include "vhost.h"
 #include "virtio_user_dev.h"
 #include "../virtio_ethdev.h"
@@ -109,9 +111,24 @@  is_vhost_user_by_type(const char *path)
 int
 virtio_user_start_device(struct virtio_user_dev *dev)
 {
+	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
 	uint64_t features;
 	int ret;
 
+	/*
+	 * XXX workaround!
+	 *
+	 * We need to make sure that the locks will be
+	 * taken in the correct order to avoid deadlocks.
+	 *
+	 * Before releasing this lock, this thread should
+	 * not trigger any memory hotplug events.
+	 *
+	 * This is a temporary workaround, and should be
+	 * replaced when we get proper supports from the
+	 * memory subsystem in the future.
+	 */
+	rte_rwlock_read_lock(&mcfg->memory_hotplug_lock);
 	pthread_mutex_lock(&dev->mutex);
 
 	if (is_vhost_user_by_type(dev->path) && dev->vhostfd < 0)
@@ -152,10 +169,12 @@  virtio_user_start_device(struct virtio_user_dev *dev)
 
 	dev->started = true;
 	pthread_mutex_unlock(&dev->mutex);
+	rte_rwlock_read_unlock(&mcfg->memory_hotplug_lock);
 
 	return 0;
 error:
 	pthread_mutex_unlock(&dev->mutex);
+	rte_rwlock_read_unlock(&mcfg->memory_hotplug_lock);
 	/* TODO: free resource here or caller to check */
 	return -1;
 }