DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 1/2] mem: add write memory barrier before changing heap state
@ 2014-04-15 13:50 David Marchand
  2014-04-15 13:50 ` [dpdk-dev] [PATCH 2/2] mem: fix initialization check for malloc heap David Marchand
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: David Marchand @ 2014-04-15 13:50 UTC (permalink / raw)
  To: dev

From: Didier Pallard <didier.pallard@6wind.com>

a write memory barrier is needed before changing heap state
value, else some concurrent core may see state changing before
all initialization values are written to memory, causing
unpredictable results in malloc function.

Signed-off-by: Didier Pallard <didier.pallard@6wind.com>
---
 lib/librte_malloc/malloc_heap.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_malloc/malloc_heap.c b/lib/librte_malloc/malloc_heap.c
index f4a0294..64668cb 100644
--- a/lib/librte_malloc/malloc_heap.c
+++ b/lib/librte_malloc/malloc_heap.c
@@ -147,6 +147,7 @@ malloc_heap_init(struct malloc_heap *heap)
 			 */
 			heap->numa_socket = heap - mcfg->malloc_heaps;
 			rte_spinlock_init(&heap->lock);
+			rte_wmb();
 			heap->initialised = INITIALISED;
 		}
 	}
-- 
1.7.10.4

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-dev] [PATCH 2/2] mem: fix initialization check for malloc heap
  2014-04-15 13:50 [dpdk-dev] [PATCH 1/2] mem: add write memory barrier before changing heap state David Marchand
@ 2014-04-15 13:50 ` David Marchand
  2014-04-15 14:08 ` [dpdk-dev] [PATCH 1/2] mem: add write memory barrier before changing heap state Richardson, Bruce
  2014-04-15 14:44 ` Neil Horman
  2 siblings, 0 replies; 12+ messages in thread
From: David Marchand @ 2014-04-15 13:50 UTC (permalink / raw)
  To: dev

From: Didier Pallard <didier.pallard@6wind.com>

initialised field must be checked against INITIALISED value before
allowing malloc to occur, else some core may pass the test and start
malloc while heap is in INITIALISING state and is not fully initialized.

Signed-off-by: Didier Pallard <didier.pallard@6wind.com>
---
 lib/librte_malloc/malloc_heap.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_malloc/malloc_heap.c b/lib/librte_malloc/malloc_heap.c
index 64668cb..eed8a63 100644
--- a/lib/librte_malloc/malloc_heap.c
+++ b/lib/librte_malloc/malloc_heap.c
@@ -197,7 +197,7 @@ void *
 malloc_heap_alloc(struct malloc_heap *heap,
 		const char *type __attribute__((unused)), size_t size, unsigned align)
 {
-	if (!heap->initialised)
+	if (heap->initialised != INITIALISED)
 		malloc_heap_init(heap);
 
 	size = CACHE_LINE_ROUNDUP(size);
@@ -227,7 +227,7 @@ int
 malloc_heap_get_stats(const struct malloc_heap *heap,
 		struct rte_malloc_socket_stats *socket_stats)
 {
-	if (!heap->initialised)
+	if (heap->initialised != INITIALISED)
 		return -1;
 
 	struct malloc_elem *elem = heap->free_head;
-- 
1.7.10.4

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] mem: add write memory barrier before changing heap state
  2014-04-15 13:50 [dpdk-dev] [PATCH 1/2] mem: add write memory barrier before changing heap state David Marchand
  2014-04-15 13:50 ` [dpdk-dev] [PATCH 2/2] mem: fix initialization check for malloc heap David Marchand
@ 2014-04-15 14:08 ` Richardson, Bruce
  2014-04-16  8:55   ` didier.pallard
  2014-04-15 14:44 ` Neil Horman
  2 siblings, 1 reply; 12+ messages in thread
From: Richardson, Bruce @ 2014-04-15 14:08 UTC (permalink / raw)
  To: David Marchand, dev

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of David Marchand
> Sent: Tuesday, April 15, 2014 2:51 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH 1/2] mem: add write memory barrier before
> changing heap state
> 
> From: Didier Pallard <didier.pallard@6wind.com>
> 
> a write memory barrier is needed before changing heap state value, else some
> concurrent core may see state changing before all initialization values are
> written to memory, causing unpredictable results in malloc function.
> 
> Signed-off-by: Didier Pallard <didier.pallard@6wind.com>

No barrier should be necessary here. As in a number of other places, such as rings, compiler barriers can be used in place of write memory barriers, due to IA ordering rules. However, in this case, both variables referenced are volatile variables and so the assignments to them cannot be reordered by the compiler so no compiler barrier is necessary either.

Regards,
/Bruce

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] mem: add write memory barrier before changing heap state
  2014-04-15 13:50 [dpdk-dev] [PATCH 1/2] mem: add write memory barrier before changing heap state David Marchand
  2014-04-15 13:50 ` [dpdk-dev] [PATCH 2/2] mem: fix initialization check for malloc heap David Marchand
  2014-04-15 14:08 ` [dpdk-dev] [PATCH 1/2] mem: add write memory barrier before changing heap state Richardson, Bruce
@ 2014-04-15 14:44 ` Neil Horman
  2014-04-18 12:56   ` [dpdk-dev] [PATCH 0/2] rework heap initialisation David Marchand
  2 siblings, 1 reply; 12+ messages in thread
From: Neil Horman @ 2014-04-15 14:44 UTC (permalink / raw)
  To: David Marchand; +Cc: dev

On Tue, Apr 15, 2014 at 03:50:58PM +0200, David Marchand wrote:
> From: Didier Pallard <didier.pallard@6wind.com>
> 
> a write memory barrier is needed before changing heap state
> value, else some concurrent core may see state changing before
> all initialization values are written to memory, causing
> unpredictable results in malloc function.
> 
> Signed-off-by: Didier Pallard <didier.pallard@6wind.com>
> ---
>  lib/librte_malloc/malloc_heap.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/librte_malloc/malloc_heap.c b/lib/librte_malloc/malloc_heap.c
> index f4a0294..64668cb 100644
> --- a/lib/librte_malloc/malloc_heap.c
> +++ b/lib/librte_malloc/malloc_heap.c
> @@ -147,6 +147,7 @@ malloc_heap_init(struct malloc_heap *heap)
>  			 */
>  			heap->numa_socket = heap - mcfg->malloc_heaps;
>  			rte_spinlock_init(&heap->lock);
> +			rte_wmb();
>  			heap->initialised = INITIALISED;
>  		}
>  	}
> -- 
> 1.7.10.4
> 
> 
This certainly won't hurt anything, but from my read we have a fixed array of
malloc_heaps in the mem_config array.  It would seem to me that the more
appropriate solution would be to initalize all of the malloc_heaps before
starting any additional threads or processes that access the dpdk.  I.e. call
malloc_heap_init for every entry in the array during rte_eal_init.  That
prevents you from even needing to keep the ->initalized member around because
its ready during the library initalization

Neil
 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] mem: add write memory barrier before changing heap state
  2014-04-15 14:08 ` [dpdk-dev] [PATCH 1/2] mem: add write memory barrier before changing heap state Richardson, Bruce
@ 2014-04-16  8:55   ` didier.pallard
  0 siblings, 0 replies; 12+ messages in thread
From: didier.pallard @ 2014-04-16  8:55 UTC (permalink / raw)
  To: Richardson, Bruce; +Cc: dev

On 04/15/2014 04:08 PM, Richardson, Bruce wrote:
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of David Marchand
>> Sent: Tuesday, April 15, 2014 2:51 PM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH 1/2] mem: add write memory barrier before
>> changing heap state
>>
>> From: Didier Pallard <didier.pallard@6wind.com>
>>
>> a write memory barrier is needed before changing heap state value, else some
>> concurrent core may see state changing before all initialization values are
>> written to memory, causing unpredictable results in malloc function.
>>
>> Signed-off-by: Didier Pallard <didier.pallard@6wind.com>
> No barrier should be necessary here. As in a number of other places, such as rings, compiler barriers can be used in place of write memory barriers, due to IA ordering rules. However, in this case, both variables referenced are volatile variables and so the assignments to them cannot be reordered by the compiler so no compiler barrier is necessary either.
>
> Regards,
> /Bruce

Hi bruce,

Indeed a compiler barrier is absolutely needed here. volatile variable 
use is absolutely not a serializing instruction from compiler point of 
view; only atomic variable use is serializing, due to asm volatile 
(memory) directive use.
Here is the assembler generated with and without rte_wmb:


With rte_wmb

  142:   f0 45 0f b1 07          lock cmpxchg %r8d,(%r15)
  147:   0f 94 c0                sete   %al
  14a:   84 c0                   test   %al,%al
  14c:   74 ea                   je     138 <malloc_heap_alloc+0x68>
  14e:   49 c7 47 10 00 00 00    movq   $0x0,0x10(%r15)
  155:   00
  156:   41 c7 47 18 00 00 00    movl   $0x0,0x18(%r15)
  15d:   00
  15e:   41 c7 47 08 00 00 00    movl   $0x0,0x8(%r15)
  165:   00
  166:   41 c7 47 1c 00 00 00    movl   $0x0,0x1c(%r15)
  16d:   00
  16e:   49 c7 47 20 00 00 00    movq   $0x0,0x20(%r15)
  175:   00
  176:   45 89 57 04             mov    %r10d,0x4(%r15)
  17a:   0f ae f8                sfence
* 17d:   41 c7 07 02 00 00 00    movl   $0x2,(%r15)**
** 184:   41 8b 37                mov    (%r15),%esi**
* 187:   83 fe 02                cmp    $0x2,%esi
  18a:   75 b4                   jne    140 <malloc_heap_alloc+0x70>
  18c:   0f 1f 40 00             nopl   0x0(%rax)
  190:   48 83 c3 3f             add    $0x3f,%rbx


Without rte_wmb

  142:   f0 45 0f b1 07          lock cmpxchg %r8d,(%r15)
  147:   0f 94 c0                sete   %al
  14a:   84 c0                   test   %al,%al
  14c:   74 ea                   je     138 <malloc_heap_alloc+0x68>
  14e:   49 c7 47 10 00 00 00    movq   $0x0,0x10(%r15)
  155:   00
  156:   41 c7 47 08 00 00 00    movl   $0x0,0x8(%r15)
  15d:   00
* 15e:   41 c7 07 02 00 00 00    movl   $0x2,(%r15)**
** 165:   41 8b 37                mov    (%r15),%esi**
* 168:   41 c7 47 18 00 00 00    movl $0x0,0x18(%r15)
  16f:   00
  170:   41 c7 47 1c 00 00 00    movl   $0x0,0x1c(%r15)
  177:   00
  178:   49 c7 47 20 00 00 00    movq   $0x0,0x20(%r15)
  17f:   00
  180:   45 89 57 04             mov    %r10d,0x4(%r15)
  184:   83 fe 02                cmp    $0x2,%esi
  187:   75 b7                   jne    140 <malloc_heap_alloc+0x70>
  189:   0f 1f 80 00 00 00 00    nopl   0x0(%rax)
  190:   48 83 c3 3f             add    $0x3f,%rbx

It's clear that the *heap->initialised = INITIALISED;* instruction has 
been reordered by the compiler.

About rte_wmb and rte_rmb use, i agree with you that on intel 
architecture those macro should do nothing more than compiler barrier, 
due to Intel architecture choices.
But for code completness, i think those memory barriers should remain in 
place in the code, and rte_*mb should map to compiler barrier on intel 
architecture.

didier

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-dev] [PATCH 0/2] rework heap initialisation
  2014-04-15 14:44 ` Neil Horman
@ 2014-04-18 12:56   ` David Marchand
  2014-04-18 12:56     ` [dpdk-dev] [PATCH 1/2] malloc: get rid of numa_socket field David Marchand
  2014-04-18 12:56     ` [dpdk-dev] [PATCH 2/2] malloc: simplify heap initialisation David Marchand
  0 siblings, 2 replies; 12+ messages in thread
From: David Marchand @ 2014-04-18 12:56 UTC (permalink / raw)
  To: dev

Following Neil's suggestion, here is a patchset that removes the need for a
synchronisation mechanism when initialising heap objects.
As a consequence, this patchset replaces the two patches Didier proposed
earlier.

-- 
David Marchand

David Marchand (2):
  malloc: get rid of numa_socket field
  malloc: simplify heap initialisation

 lib/librte_eal/common/include/rte_malloc_heap.h |    8 ----
 lib/librte_malloc/malloc_heap.c                 |   46 +++--------------------
 2 files changed, 5 insertions(+), 49 deletions(-)

-- 
1.7.10.4

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-dev] [PATCH 1/2] malloc: get rid of numa_socket field
  2014-04-18 12:56   ` [dpdk-dev] [PATCH 0/2] rework heap initialisation David Marchand
@ 2014-04-18 12:56     ` David Marchand
  2014-04-18 13:08       ` Neil Horman
  2014-04-18 12:56     ` [dpdk-dev] [PATCH 2/2] malloc: simplify heap initialisation David Marchand
  1 sibling, 1 reply; 12+ messages in thread
From: David Marchand @ 2014-04-18 12:56 UTC (permalink / raw)
  To: dev

We don't really need this field as it is only used when creating the memzone
object associated to this heap.

Signed-off-by: David Marchand <david.marchand@6wind.com>
---
 lib/librte_eal/common/include/rte_malloc_heap.h |    1 -
 lib/librte_malloc/malloc_heap.c                 |   13 +++++--------
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/lib/librte_eal/common/include/rte_malloc_heap.h b/lib/librte_eal/common/include/rte_malloc_heap.h
index d9e959d..ea2a3f5 100644
--- a/lib/librte_eal/common/include/rte_malloc_heap.h
+++ b/lib/librte_eal/common/include/rte_malloc_heap.h
@@ -48,7 +48,6 @@ enum heap_state {
  */
 struct malloc_heap {
 	enum heap_state volatile initialised;
-	unsigned numa_socket;
 	rte_spinlock_t lock;
 	struct malloc_elem * volatile free_head;
 	unsigned mz_count;
diff --git a/lib/librte_malloc/malloc_heap.c b/lib/librte_malloc/malloc_heap.c
index f4a0294..375f212 100644
--- a/lib/librte_malloc/malloc_heap.c
+++ b/lib/librte_malloc/malloc_heap.c
@@ -82,6 +82,8 @@ malloc_heap_add_memzone(struct malloc_heap *heap, size_t size, unsigned align)
 	/* ensure the data we want to allocate will fit in the memzone */
 	const size_t min_size = size + align + MALLOC_ELEM_OVERHEAD * 2;
 	const struct rte_memzone *mz = NULL;
+	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
+	unsigned numa_socket = heap - mcfg->malloc_heaps;
 
 	size_t mz_size = min_size;
 	if (mz_size < block_size)
@@ -89,14 +91,14 @@ malloc_heap_add_memzone(struct malloc_heap *heap, size_t size, unsigned align)
 
 	char mz_name[RTE_MEMZONE_NAMESIZE];
 	rte_snprintf(mz_name, sizeof(mz_name), "MALLOC_S%u_HEAP_%u",
-			heap->numa_socket, heap->mz_count++);
+		     numa_socket, heap->mz_count++);
 
 	/* try getting a block. if we fail and we don't need as big a block
 	 * as given in the config, we can shrink our request and try again
 	 */
 	do {
-		mz = rte_memzone_reserve(mz_name, mz_size,
-				heap->numa_socket, mz_flags);
+		mz = rte_memzone_reserve(mz_name, mz_size, numa_socket,
+					 mz_flags);
 		if (mz == NULL)
 			mz_size /= 2;
 	} while (mz == NULL && mz_size > min_size);
@@ -141,11 +143,6 @@ malloc_heap_init(struct malloc_heap *heap)
 			heap->mz_count = 0;
 			heap->alloc_count = 0;
 			heap->total_size = 0;
-			/*
-			 * Find NUMA socket of heap that is being initialised, so that
-			 * malloc_heaps[n].numa_socket == n
-			 */
-			heap->numa_socket = heap - mcfg->malloc_heaps;
 			rte_spinlock_init(&heap->lock);
 			heap->initialised = INITIALISED;
 		}
-- 
1.7.10.4

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-dev] [PATCH 2/2] malloc: simplify heap initialisation
  2014-04-18 12:56   ` [dpdk-dev] [PATCH 0/2] rework heap initialisation David Marchand
  2014-04-18 12:56     ` [dpdk-dev] [PATCH 1/2] malloc: get rid of numa_socket field David Marchand
@ 2014-04-18 12:56     ` David Marchand
  2014-04-18 13:09       ` Neil Horman
  1 sibling, 1 reply; 12+ messages in thread
From: David Marchand @ 2014-04-18 12:56 UTC (permalink / raw)
  To: dev

There should be no real need for this initialised field as the whole structure
is set to 0 in rte_config_init() by primary process, and secondary processes
wait for this to happen before anything else (looking at mem_config magic).

Signed-off-by: David Marchand <david.marchand@6wind.com>
---
 lib/librte_eal/common/include/rte_malloc_heap.h |    7 -----
 lib/librte_malloc/malloc_heap.c                 |   33 -----------------------
 2 files changed, 40 deletions(-)

diff --git a/lib/librte_eal/common/include/rte_malloc_heap.h b/lib/librte_eal/common/include/rte_malloc_heap.h
index ea2a3f5..5e139cf 100644
--- a/lib/librte_eal/common/include/rte_malloc_heap.h
+++ b/lib/librte_eal/common/include/rte_malloc_heap.h
@@ -37,17 +37,10 @@
 #include <stddef.h>
 #include <rte_spinlock.h>
 
-enum heap_state {
-	NOT_INITIALISED = 0,
-	INITIALISING,
-	INITIALISED
-};
-
 /**
  * Structure to hold malloc heap
  */
 struct malloc_heap {
-	enum heap_state volatile initialised;
 	rte_spinlock_t lock;
 	struct malloc_elem * volatile free_head;
 	unsigned mz_count;
diff --git a/lib/librte_malloc/malloc_heap.c b/lib/librte_malloc/malloc_heap.c
index 375f212..882749c 100644
--- a/lib/librte_malloc/malloc_heap.c
+++ b/lib/librte_malloc/malloc_heap.c
@@ -123,33 +123,6 @@ malloc_heap_add_memzone(struct malloc_heap *heap, size_t size, unsigned align)
 }
 
 /*
- * initialise a malloc heap object. The heap is locked with a private
- * lock while being initialised. This function should only be called the
- * first time a thread calls malloc - if even then, as heaps are per-socket
- * not per-thread.
- */
-static void
-malloc_heap_init(struct malloc_heap *heap)
-{
-	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
-
-	rte_eal_mcfg_wait_complete(mcfg);
-	while (heap->initialised != INITIALISED) {
-		if (rte_atomic32_cmpset(
-				(volatile uint32_t*)&heap->initialised,
-				NOT_INITIALISED, INITIALISING)) {
-
-			heap->free_head = NULL;
-			heap->mz_count = 0;
-			heap->alloc_count = 0;
-			heap->total_size = 0;
-			rte_spinlock_init(&heap->lock);
-			heap->initialised = INITIALISED;
-		}
-	}
-}
-
-/*
  * Iterates through the freelist for a heap to find a free element
  * which can store data of the required size and with the requested alignment.
  * Returns null on failure, or pointer to element on success, with the pointer
@@ -193,9 +166,6 @@ void *
 malloc_heap_alloc(struct malloc_heap *heap,
 		const char *type __attribute__((unused)), size_t size, unsigned align)
 {
-	if (!heap->initialised)
-		malloc_heap_init(heap);
-
 	size = CACHE_LINE_ROUNDUP(size);
 	align = CACHE_LINE_ROUNDUP(align);
 	rte_spinlock_lock(&heap->lock);
@@ -223,9 +193,6 @@ int
 malloc_heap_get_stats(const struct malloc_heap *heap,
 		struct rte_malloc_socket_stats *socket_stats)
 {
-	if (!heap->initialised)
-		return -1;
-
 	struct malloc_elem *elem = heap->free_head;
 
 	/* Initialise variables for heap */
-- 
1.7.10.4

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] malloc: get rid of numa_socket field
  2014-04-18 12:56     ` [dpdk-dev] [PATCH 1/2] malloc: get rid of numa_socket field David Marchand
@ 2014-04-18 13:08       ` Neil Horman
  2014-04-30  9:46         ` Thomas Monjalon
  0 siblings, 1 reply; 12+ messages in thread
From: Neil Horman @ 2014-04-18 13:08 UTC (permalink / raw)
  To: David Marchand; +Cc: dev

On Fri, Apr 18, 2014 at 02:56:17PM +0200, David Marchand wrote:
> We don't really need this field as it is only used when creating the memzone
> object associated to this heap.
> 
> Signed-off-by: David Marchand <david.marchand@6wind.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>

> ---
>  lib/librte_eal/common/include/rte_malloc_heap.h |    1 -
>  lib/librte_malloc/malloc_heap.c                 |   13 +++++--------
>  2 files changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/librte_eal/common/include/rte_malloc_heap.h b/lib/librte_eal/common/include/rte_malloc_heap.h
> index d9e959d..ea2a3f5 100644
> --- a/lib/librte_eal/common/include/rte_malloc_heap.h
> +++ b/lib/librte_eal/common/include/rte_malloc_heap.h
> @@ -48,7 +48,6 @@ enum heap_state {
>   */
>  struct malloc_heap {
>  	enum heap_state volatile initialised;
> -	unsigned numa_socket;
>  	rte_spinlock_t lock;
>  	struct malloc_elem * volatile free_head;
>  	unsigned mz_count;
> diff --git a/lib/librte_malloc/malloc_heap.c b/lib/librte_malloc/malloc_heap.c
> index f4a0294..375f212 100644
> --- a/lib/librte_malloc/malloc_heap.c
> +++ b/lib/librte_malloc/malloc_heap.c
> @@ -82,6 +82,8 @@ malloc_heap_add_memzone(struct malloc_heap *heap, size_t size, unsigned align)
>  	/* ensure the data we want to allocate will fit in the memzone */
>  	const size_t min_size = size + align + MALLOC_ELEM_OVERHEAD * 2;
>  	const struct rte_memzone *mz = NULL;
> +	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
> +	unsigned numa_socket = heap - mcfg->malloc_heaps;
>  
>  	size_t mz_size = min_size;
>  	if (mz_size < block_size)
> @@ -89,14 +91,14 @@ malloc_heap_add_memzone(struct malloc_heap *heap, size_t size, unsigned align)
>  
>  	char mz_name[RTE_MEMZONE_NAMESIZE];
>  	rte_snprintf(mz_name, sizeof(mz_name), "MALLOC_S%u_HEAP_%u",
> -			heap->numa_socket, heap->mz_count++);
> +		     numa_socket, heap->mz_count++);
>  
>  	/* try getting a block. if we fail and we don't need as big a block
>  	 * as given in the config, we can shrink our request and try again
>  	 */
>  	do {
> -		mz = rte_memzone_reserve(mz_name, mz_size,
> -				heap->numa_socket, mz_flags);
> +		mz = rte_memzone_reserve(mz_name, mz_size, numa_socket,
> +					 mz_flags);
>  		if (mz == NULL)
>  			mz_size /= 2;
>  	} while (mz == NULL && mz_size > min_size);
> @@ -141,11 +143,6 @@ malloc_heap_init(struct malloc_heap *heap)
>  			heap->mz_count = 0;
>  			heap->alloc_count = 0;
>  			heap->total_size = 0;
> -			/*
> -			 * Find NUMA socket of heap that is being initialised, so that
> -			 * malloc_heaps[n].numa_socket == n
> -			 */
> -			heap->numa_socket = heap - mcfg->malloc_heaps;
>  			rte_spinlock_init(&heap->lock);
>  			heap->initialised = INITIALISED;
>  		}
> -- 
> 1.7.10.4
> 
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] malloc: simplify heap initialisation
  2014-04-18 12:56     ` [dpdk-dev] [PATCH 2/2] malloc: simplify heap initialisation David Marchand
@ 2014-04-18 13:09       ` Neil Horman
  2014-04-30  9:47         ` Thomas Monjalon
  0 siblings, 1 reply; 12+ messages in thread
From: Neil Horman @ 2014-04-18 13:09 UTC (permalink / raw)
  To: David Marchand; +Cc: dev

On Fri, Apr 18, 2014 at 02:56:18PM +0200, David Marchand wrote:
> There should be no real need for this initialised field as the whole structure
> is set to 0 in rte_config_init() by primary process, and secondary processes
> wait for this to happen before anything else (looking at mem_config magic).
> 
> Signed-off-by: David Marchand <david.marchand@6wind.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>

> ---
>  lib/librte_eal/common/include/rte_malloc_heap.h |    7 -----
>  lib/librte_malloc/malloc_heap.c                 |   33 -----------------------
>  2 files changed, 40 deletions(-)
> 
> diff --git a/lib/librte_eal/common/include/rte_malloc_heap.h b/lib/librte_eal/common/include/rte_malloc_heap.h
> index ea2a3f5..5e139cf 100644
> --- a/lib/librte_eal/common/include/rte_malloc_heap.h
> +++ b/lib/librte_eal/common/include/rte_malloc_heap.h
> @@ -37,17 +37,10 @@
>  #include <stddef.h>
>  #include <rte_spinlock.h>
>  
> -enum heap_state {
> -	NOT_INITIALISED = 0,
> -	INITIALISING,
> -	INITIALISED
> -};
> -
>  /**
>   * Structure to hold malloc heap
>   */
>  struct malloc_heap {
> -	enum heap_state volatile initialised;
>  	rte_spinlock_t lock;
>  	struct malloc_elem * volatile free_head;
>  	unsigned mz_count;
> diff --git a/lib/librte_malloc/malloc_heap.c b/lib/librte_malloc/malloc_heap.c
> index 375f212..882749c 100644
> --- a/lib/librte_malloc/malloc_heap.c
> +++ b/lib/librte_malloc/malloc_heap.c
> @@ -123,33 +123,6 @@ malloc_heap_add_memzone(struct malloc_heap *heap, size_t size, unsigned align)
>  }
>  
>  /*
> - * initialise a malloc heap object. The heap is locked with a private
> - * lock while being initialised. This function should only be called the
> - * first time a thread calls malloc - if even then, as heaps are per-socket
> - * not per-thread.
> - */
> -static void
> -malloc_heap_init(struct malloc_heap *heap)
> -{
> -	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
> -
> -	rte_eal_mcfg_wait_complete(mcfg);
> -	while (heap->initialised != INITIALISED) {
> -		if (rte_atomic32_cmpset(
> -				(volatile uint32_t*)&heap->initialised,
> -				NOT_INITIALISED, INITIALISING)) {
> -
> -			heap->free_head = NULL;
> -			heap->mz_count = 0;
> -			heap->alloc_count = 0;
> -			heap->total_size = 0;
> -			rte_spinlock_init(&heap->lock);
> -			heap->initialised = INITIALISED;
> -		}
> -	}
> -}
> -
> -/*
>   * Iterates through the freelist for a heap to find a free element
>   * which can store data of the required size and with the requested alignment.
>   * Returns null on failure, or pointer to element on success, with the pointer
> @@ -193,9 +166,6 @@ void *
>  malloc_heap_alloc(struct malloc_heap *heap,
>  		const char *type __attribute__((unused)), size_t size, unsigned align)
>  {
> -	if (!heap->initialised)
> -		malloc_heap_init(heap);
> -
>  	size = CACHE_LINE_ROUNDUP(size);
>  	align = CACHE_LINE_ROUNDUP(align);
>  	rte_spinlock_lock(&heap->lock);
> @@ -223,9 +193,6 @@ int
>  malloc_heap_get_stats(const struct malloc_heap *heap,
>  		struct rte_malloc_socket_stats *socket_stats)
>  {
> -	if (!heap->initialised)
> -		return -1;
> -
>  	struct malloc_elem *elem = heap->free_head;
>  
>  	/* Initialise variables for heap */
> -- 
> 1.7.10.4
> 
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] malloc: get rid of numa_socket field
  2014-04-18 13:08       ` Neil Horman
@ 2014-04-30  9:46         ` Thomas Monjalon
  0 siblings, 0 replies; 12+ messages in thread
From: Thomas Monjalon @ 2014-04-30  9:46 UTC (permalink / raw)
  To: David Marchand; +Cc: dev

2014-04-18 09:08, Neil Horman:
> On Fri, Apr 18, 2014 at 02:56:17PM +0200, David Marchand wrote:
> > We don't really need this field as it is only used when creating the
> > memzone object associated to this heap.
> > 
> > Signed-off-by: David Marchand <david.marchand@6wind.com>
> 
> Acked-by: Neil Horman <nhorman@tuxdriver.com>

Applied for version 1.6.0r2.

Thanks
-- 
Thomas

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] malloc: simplify heap initialisation
  2014-04-18 13:09       ` Neil Horman
@ 2014-04-30  9:47         ` Thomas Monjalon
  0 siblings, 0 replies; 12+ messages in thread
From: Thomas Monjalon @ 2014-04-30  9:47 UTC (permalink / raw)
  To: David Marchand; +Cc: dev

2014-04-18 09:09, Neil Horman:
> On Fri, Apr 18, 2014 at 02:56:18PM +0200, David Marchand wrote:
> > There should be no real need for this initialised field as the whole
> > structure is set to 0 in rte_config_init() by primary process, and
> > secondary processes wait for this to happen before anything else (looking
> > at mem_config magic).
> > 
> > Signed-off-by: David Marchand <david.marchand@6wind.com>
> 
> Acked-by: Neil Horman <nhorman@tuxdriver.com>

Applied for version 1.6.0r2.

Thanks
-- 
Thomas

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2014-04-30  9:47 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-15 13:50 [dpdk-dev] [PATCH 1/2] mem: add write memory barrier before changing heap state David Marchand
2014-04-15 13:50 ` [dpdk-dev] [PATCH 2/2] mem: fix initialization check for malloc heap David Marchand
2014-04-15 14:08 ` [dpdk-dev] [PATCH 1/2] mem: add write memory barrier before changing heap state Richardson, Bruce
2014-04-16  8:55   ` didier.pallard
2014-04-15 14:44 ` Neil Horman
2014-04-18 12:56   ` [dpdk-dev] [PATCH 0/2] rework heap initialisation David Marchand
2014-04-18 12:56     ` [dpdk-dev] [PATCH 1/2] malloc: get rid of numa_socket field David Marchand
2014-04-18 13:08       ` Neil Horman
2014-04-30  9:46         ` Thomas Monjalon
2014-04-18 12:56     ` [dpdk-dev] [PATCH 2/2] malloc: simplify heap initialisation David Marchand
2014-04-18 13:09       ` Neil Horman
2014-04-30  9:47         ` Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).