* [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 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
* 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
* [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
* 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 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
* [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 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 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).