[dpdk-dev] [PATCH 18.11] malloc: fix deadlock when using malloc stats

[dpdk-dev] [PATCH 18.11] malloc: fix deadlock when using malloc stats

[Anatoly Burakov]

Currently, malloc statistics and external heap creation code
use memory hotplug lock as a way to synchronize accesses to
heaps (as in, locking the hotplug lock to prevent list of heaps
from changing under our feet). At the same time, malloc
statistics code will also lock the heap because it needs to
access heap data and does not want any other thread to allocate
anything from that heap.

In such scheme, it is possible to enter a deadlock with the
following sequence of events:

thread 1		thread 2
rte_malloc()
			rte_malloc_dump_stats()
take heap lock
			take hotplug lock
failed to allocate,
attempt to take
hotplug lock
			attempt to take heap lock

Neither thread will be able to continue, as both of them are
waiting for the other one to drop the lock. Adding an
additional lock will require an ABI change, so instead of
that, make malloc statistics calls thread-unsafe with
respect to creating/destroying heaps.

Fixes: 72cf92b31855 ("malloc: index heaps using heap ID rather than NUMA node")
Cc: stable@dpdk.org

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---

Notes:
    IMO this is the best we can do for 18.11 without breaking ABI.
    For 19.02, we can introduce a new global heap lock (something
    i should've done in the first place...), so this patch is
    not applicable to 19.02. For 19.02, we can fix this properly
    by introducing another lock and breaking the EAL ABI.
    
    Not sure where to put docs update, feedback welcome.

 lib/librte_eal/common/include/rte_malloc.h |  9 +++++++++
 lib/librte_eal/common/rte_malloc.c         | 19 +++----------------
 2 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/lib/librte_eal/common/include/rte_malloc.h b/lib/librte_eal/common/include/rte_malloc.h
index 7249e6aae..cde6232f9 100644
--- a/lib/librte_eal/common/include/rte_malloc.h
+++ b/lib/librte_eal/common/include/rte_malloc.h
@@ -251,6 +251,9 @@ rte_malloc_validate(const void *ptr, size_t *size);
 /**
  * Get heap statistics for the specified heap.
  *
+ * @note This function is not thread-safe with respect to
+ *    ``rte_malloc_heap_create()``/``rte_malloc_heap_destroy()`` functions.
+ *
  * @param socket
  *   An unsigned integer specifying the socket to get heap statistics for
  * @param socket_stats
@@ -461,6 +464,9 @@ rte_malloc_heap_socket_is_external(int socket_id);
  * Dump for the specified type to a file. If the type argument is
  * NULL, all memory types will be dumped.
  *
+ * @note This function is not thread-safe with respect to
+ *    ``rte_malloc_heap_create()``/``rte_malloc_heap_destroy()`` functions.
+ *
  * @param f
  *   A pointer to a file for output
  * @param type
@@ -473,6 +479,9 @@ rte_malloc_dump_stats(FILE *f, const char *type);
 /**
  * Dump contents of all malloc heaps to a file.
  *
+ * @note This function is not thread-safe with respect to
+ *    ``rte_malloc_heap_create()``/``rte_malloc_heap_destroy()`` functions.
+ *
  * @param f
  *   A pointer to a file for output
  */
diff --git a/lib/librte_eal/common/rte_malloc.c b/lib/librte_eal/common/rte_malloc.c
index 0da5ad5e8..bc2b74d19 100644
--- a/lib/librte_eal/common/rte_malloc.c
+++ b/lib/librte_eal/common/rte_malloc.c
@@ -156,20 +156,14 @@ rte_malloc_get_socket_stats(int socket,
 		struct rte_malloc_socket_stats *socket_stats)
 {
 	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
-	int heap_idx, ret = -1;
-
-	rte_rwlock_read_lock(&mcfg->memory_hotplug_lock);
+	int heap_idx;
 
 	heap_idx = malloc_socket_to_heap_id(socket);
 	if (heap_idx < 0)
-		goto unlock;
+		return -1;
 
-	ret = malloc_heap_get_stats(&mcfg->malloc_heaps[heap_idx],
+	return malloc_heap_get_stats(&mcfg->malloc_heaps[heap_idx],
 			socket_stats);
-unlock:
-	rte_rwlock_read_unlock(&mcfg->memory_hotplug_lock);
-
-	return ret;
 }
 
 /*
@@ -181,14 +175,10 @@ rte_malloc_dump_heaps(FILE *f)
 	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
 	unsigned int idx;
 
-	rte_rwlock_read_lock(&mcfg->memory_hotplug_lock);
-
 	for (idx = 0; idx < RTE_MAX_HEAPS; idx++) {
 		fprintf(f, "Heap id: %u\n", idx);
 		malloc_heap_dump(&mcfg->malloc_heaps[idx], f);
 	}
-
-	rte_rwlock_read_unlock(&mcfg->memory_hotplug_lock);
 }
 
 int
@@ -262,8 +252,6 @@ rte_malloc_dump_stats(FILE *f, __rte_unused const char *type)
 	unsigned int heap_id;
 	struct rte_malloc_socket_stats sock_stats;
 
-	rte_rwlock_read_lock(&mcfg->memory_hotplug_lock);
-
 	/* Iterate through all initialised heaps */
 	for (heap_id = 0; heap_id < RTE_MAX_HEAPS; heap_id++) {
 		struct malloc_heap *heap = &mcfg->malloc_heaps[heap_id];
@@ -280,7 +268,6 @@ rte_malloc_dump_stats(FILE *f, __rte_unused const char *type)
 		fprintf(f, "\tAlloc_count:%u,\n",sock_stats.alloc_count);
 		fprintf(f, "\tFree_count:%u,\n", sock_stats.free_count);
 	}
-	rte_rwlock_read_unlock(&mcfg->memory_hotplug_lock);
 	return;
 }
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH 18.11] malloc: fix deadlock when using malloc stats

[Thomas Monjalon]

04/12/2018 13:22, Anatoly Burakov:
> Currently, malloc statistics and external heap creation code
> use memory hotplug lock as a way to synchronize accesses to
> heaps (as in, locking the hotplug lock to prevent list of heaps
> from changing under our feet). At the same time, malloc
> statistics code will also lock the heap because it needs to
> access heap data and does not want any other thread to allocate
> anything from that heap.
> 
> In such scheme, it is possible to enter a deadlock with the
> following sequence of events:
> 
> thread 1		thread 2
> rte_malloc()
> 			rte_malloc_dump_stats()
> take heap lock
> 			take hotplug lock
> failed to allocate,
> attempt to take
> hotplug lock
> 			attempt to take heap lock
> 
> Neither thread will be able to continue, as both of them are
> waiting for the other one to drop the lock. Adding an
> additional lock will require an ABI change, so instead of
> that, make malloc statistics calls thread-unsafe with
> respect to creating/destroying heaps.
> 
> Fixes: 72cf92b31855 ("malloc: index heaps using heap ID rather than NUMA node")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
> 
> Notes:
>     IMO this is the best we can do for 18.11 without breaking ABI.
>     For 19.02, we can introduce a new global heap lock (something
>     i should've done in the first place...), so this patch is
>     not applicable to 19.02. For 19.02, we can fix this properly
>     by introducing another lock and breaking the EAL ABI.
>     
>     Not sure where to put docs update, feedback welcome.

This patch is also changing the API, because functions become not thread-safe.
I think you should note it in the release notes.
About 19.02, do we want to take this patch (with release notes updated)?

Re: [dpdk-dev] [PATCH 18.11] malloc: fix deadlock when using malloc stats

[Burakov, Anatoly]

On 21-Dec-18 12:09 PM, Thomas Monjalon wrote:
> 04/12/2018 13:22, Anatoly Burakov:
>> Currently, malloc statistics and external heap creation code
>> use memory hotplug lock as a way to synchronize accesses to
>> heaps (as in, locking the hotplug lock to prevent list of heaps
>> from changing under our feet). At the same time, malloc
>> statistics code will also lock the heap because it needs to
>> access heap data and does not want any other thread to allocate
>> anything from that heap.
>>
>> In such scheme, it is possible to enter a deadlock with the
>> following sequence of events:
>>
>> thread 1		thread 2
>> rte_malloc()
>> 			rte_malloc_dump_stats()
>> take heap lock
>> 			take hotplug lock
>> failed to allocate,
>> attempt to take
>> hotplug lock
>> 			attempt to take heap lock
>>
>> Neither thread will be able to continue, as both of them are
>> waiting for the other one to drop the lock. Adding an
>> additional lock will require an ABI change, so instead of
>> that, make malloc statistics calls thread-unsafe with
>> respect to creating/destroying heaps.
>>
>> Fixes: 72cf92b31855 ("malloc: index heaps using heap ID rather than NUMA node")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> ---
>>
>> Notes:
>>      IMO this is the best we can do for 18.11 without breaking ABI.
>>      For 19.02, we can introduce a new global heap lock (something
>>      i should've done in the first place...), so this patch is
>>      not applicable to 19.02. For 19.02, we can fix this properly
>>      by introducing another lock and breaking the EAL ABI.
>>      
>>      Not sure where to put docs update, feedback welcome.
> 
> This patch is also changing the API, because functions become not thread-safe.
> I think you should note it in the release notes.
> About 19.02, do we want to take this patch (with release notes updated)?
> 

Yes and yes.

Technically, they still are thread-safe when it comes to individual heap 
access - they just aren't thread-safe with regards to 
creating/destroying heaps (so, we may enter the dump function, and a 
heap may be added/removed while we're iterating over the list of heaps).

I'll send a v2 with release notes update.

-- 
Thanks,
Anatoly

[dpdk-dev] [PATCH 19.02 v2] malloc: fix deadlock when using malloc stats

[Anatoly Burakov]

Currently, malloc statistics and external heap creation code
use memory hotplug lock as a way to synchronize accesses to
heaps (as in, locking the hotplug lock to prevent list of heaps
from changing under our feet). At the same time, malloc
statistics code will also lock the heap because it needs to
access heap data and does not want any other thread to allocate
anything from that heap.

In such scheme, it is possible to enter a deadlock with the
following sequence of events:

thread 1		thread 2
rte_malloc()
			rte_malloc_dump_stats()
take heap lock
			take hotplug lock
failed to allocate,
attempt to take
hotplug lock
			attempt to take heap lock

Neither thread will be able to continue, as both of them are
waiting for the other one to drop the lock. Adding an
additional lock will require an ABI change, so instead of
that, make malloc statistics calls thread-unsafe with
respect to creating/destroying heaps.

Fixes: 72cf92b31855 ("malloc: index heaps using heap ID rather than NUMA node")
Cc: stable@dpdk.org

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---

Notes:
    This is the best we can do for 19.02 without breaking ABI.

 doc/guides/rel_notes/release_19_02.rst     |  4 ++++
 lib/librte_eal/common/include/rte_malloc.h |  9 +++++++++
 lib/librte_eal/common/rte_malloc.c         | 19 +++----------------
 3 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/doc/guides/rel_notes/release_19_02.rst b/doc/guides/rel_notes/release_19_02.rst
index 47768288a..0b248d55d 100644
--- a/doc/guides/rel_notes/release_19_02.rst
+++ b/doc/guides/rel_notes/release_19_02.rst
@@ -126,6 +126,10 @@ API Changes
   - In cases where memfd support would have been required to provide segment
     fd's (such as in-memory or no-huge mode)
 
+* eal: Functions ``rte_malloc_dump_stats()``, ``rte_malloc_dump_heaps()`` and
+  ``rte_malloc_get_socket_stats()`` are no longer safe to call concurrently with
+  ``rte_malloc_heap_create()`` or ``rte_malloc_heap_destroy()`` function calls.
+
 * pdump: The ``rte_pdump_set_socket_dir()``, the parameter ``path`` of
   ``rte_pdump_init()`` and enum ``rte_pdump_socktype`` were deprecated
   since 18.05 and are removed in this release.
diff --git a/lib/librte_eal/common/include/rte_malloc.h b/lib/librte_eal/common/include/rte_malloc.h
index a5290b074..54a12467a 100644
--- a/lib/librte_eal/common/include/rte_malloc.h
+++ b/lib/librte_eal/common/include/rte_malloc.h
@@ -251,6 +251,9 @@ rte_malloc_validate(const void *ptr, size_t *size);
 /**
  * Get heap statistics for the specified heap.
  *
+ * @note This function is not thread-safe with respect to
+ *    ``rte_malloc_heap_create()``/``rte_malloc_heap_destroy()`` functions.
+ *
  * @param socket
  *   An unsigned integer specifying the socket to get heap statistics for
  * @param socket_stats
@@ -461,6 +464,9 @@ rte_malloc_heap_socket_is_external(int socket_id);
  * Dump for the specified type to a file. If the type argument is
  * NULL, all memory types will be dumped.
  *
+ * @note This function is not thread-safe with respect to
+ *    ``rte_malloc_heap_create()``/``rte_malloc_heap_destroy()`` functions.
+ *
  * @param f
  *   A pointer to a file for output
  * @param type
@@ -473,6 +479,9 @@ rte_malloc_dump_stats(FILE *f, const char *type);
 /**
  * Dump contents of all malloc heaps to a file.
  *
+ * @note This function is not thread-safe with respect to
+ *    ``rte_malloc_heap_create()``/``rte_malloc_heap_destroy()`` functions.
+ *
  * @param f
  *   A pointer to a file for output
  */
diff --git a/lib/librte_eal/common/rte_malloc.c b/lib/librte_eal/common/rte_malloc.c
index 09051c236..b39de3c99 100644
--- a/lib/librte_eal/common/rte_malloc.c
+++ b/lib/librte_eal/common/rte_malloc.c
@@ -156,20 +156,14 @@ rte_malloc_get_socket_stats(int socket,
 		struct rte_malloc_socket_stats *socket_stats)
 {
 	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
-	int heap_idx, ret = -1;
-
-	rte_rwlock_read_lock(&mcfg->memory_hotplug_lock);
+	int heap_idx;
 
 	heap_idx = malloc_socket_to_heap_id(socket);
 	if (heap_idx < 0)
-		goto unlock;
+		return -1;
 
-	ret = malloc_heap_get_stats(&mcfg->malloc_heaps[heap_idx],
+	return malloc_heap_get_stats(&mcfg->malloc_heaps[heap_idx],
 			socket_stats);
-unlock:
-	rte_rwlock_read_unlock(&mcfg->memory_hotplug_lock);
-
-	return ret;
 }
 
 /*
@@ -181,14 +175,10 @@ rte_malloc_dump_heaps(FILE *f)
 	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
 	unsigned int idx;
 
-	rte_rwlock_read_lock(&mcfg->memory_hotplug_lock);
-
 	for (idx = 0; idx < RTE_MAX_HEAPS; idx++) {
 		fprintf(f, "Heap id: %u\n", idx);
 		malloc_heap_dump(&mcfg->malloc_heaps[idx], f);
 	}
-
-	rte_rwlock_read_unlock(&mcfg->memory_hotplug_lock);
 }
 
 int
@@ -262,8 +252,6 @@ rte_malloc_dump_stats(FILE *f, __rte_unused const char *type)
 	unsigned int heap_id;
 	struct rte_malloc_socket_stats sock_stats;
 
-	rte_rwlock_read_lock(&mcfg->memory_hotplug_lock);
-
 	/* Iterate through all initialised heaps */
 	for (heap_id = 0; heap_id < RTE_MAX_HEAPS; heap_id++) {
 		struct malloc_heap *heap = &mcfg->malloc_heaps[heap_id];
@@ -280,7 +268,6 @@ rte_malloc_dump_stats(FILE *f, __rte_unused const char *type)
 		fprintf(f, "\tAlloc_count:%u,\n",sock_stats.alloc_count);
 		fprintf(f, "\tFree_count:%u,\n", sock_stats.free_count);
 	}
-	rte_rwlock_read_unlock(&mcfg->memory_hotplug_lock);
 	return;
 }
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH 19.02 v2] malloc: fix deadlock when using malloc stats

[Thomas Monjalon]

21/12/2018 13:26, Anatoly Burakov:
> Currently, malloc statistics and external heap creation code
> use memory hotplug lock as a way to synchronize accesses to
> heaps (as in, locking the hotplug lock to prevent list of heaps
> from changing under our feet). At the same time, malloc
> statistics code will also lock the heap because it needs to
> access heap data and does not want any other thread to allocate
> anything from that heap.
> 
> In such scheme, it is possible to enter a deadlock with the
> following sequence of events:
> 
> thread 1		thread 2
> rte_malloc()
> 			rte_malloc_dump_stats()
> take heap lock
> 			take hotplug lock
> failed to allocate,
> attempt to take
> hotplug lock
> 			attempt to take heap lock
> 
> Neither thread will be able to continue, as both of them are
> waiting for the other one to drop the lock. Adding an
> additional lock will require an ABI change, so instead of
> that, make malloc statistics calls thread-unsafe with
> respect to creating/destroying heaps.
> 
> Fixes: 72cf92b31855 ("malloc: index heaps using heap ID rather than NUMA node")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
> 
> Notes:
>     This is the best we can do for 19.02 without breaking ABI.

Applied, thanks