From f0f5ac18e55062dcde28003cebc09fa3eb6ae6a5 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Mon, 2 May 2011 13:50:52 +0200 Subject: talloc: use TC_UNDEFINE_SHRINK_CHUNK() instead of TC_INVALIDATE_SHRINK_CHUNK() for realloc path If we optimize on top of raw realloc() we need TC_INVALIDATE_SHRINK_CHUNK together with TC_UNDEFINE_GROW_CHUNK (with was missing and caused false positive valgrind warnings). But that is really slow, as we do a lot of talloc_realloc calls in samba. That's why we only to TC_UNDEFINE_SHRINK_CHUNK() for now. metze --- lib/talloc/talloc.c | 37 ++++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) (limited to 'lib/talloc/talloc.c') diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c index 91452bfadac..2a956dcf0d4 100644 --- a/lib/talloc/talloc.c +++ b/lib/talloc/talloc.c @@ -178,6 +178,32 @@ static struct { TC_INVALIDATE_SHRINK_VALGRIND_CHUNK(_tc, _new_size); \ } while (0) +#define TC_UNDEFINE_SHRINK_FILL_CHUNK(_tc, _new_size) do { \ + if (unlikely(talloc_fill.enabled)) { \ + size_t _flen = (_tc)->size - (_new_size); \ + char *_fptr = (char *)TC_PTR_FROM_CHUNK(_tc); \ + _fptr += (_new_size); \ + memset(_fptr, talloc_fill.fill_value, _flen); \ + } \ +} while (0) + +#if defined(DEVELOPER) && defined(VALGRIND_MAKE_MEM_UNDEFINED) +/* Mark the unused bytes as undefined */ +#define TC_UNDEFINE_SHRINK_VALGRIND_CHUNK(_tc, _new_size) do { \ + size_t _flen = (_tc)->size - (_new_size); \ + char *_fptr = (char *)TC_PTR_FROM_CHUNK(_tc); \ + _fptr += (_new_size); \ + VALGRIND_MAKE_MEM_UNDEFINED(_fptr, _flen); \ +} while (0) +#else +#define TC_UNDEFINE_SHRINK_VALGRIND_CHUNK(_tc, _new_size) do { } while (0) +#endif + +#define TC_UNDEFINE_SHRINK_CHUNK(_tc, _new_size) do { \ + TC_UNDEFINE_SHRINK_FILL_CHUNK(_tc, _new_size); \ + TC_UNDEFINE_SHRINK_VALGRIND_CHUNK(_tc, _new_size); \ +} while (0) + #if defined(DEVELOPER) && defined(VALGRIND_MAKE_MEM_UNDEFINED) /* Mark the new bytes as undefined */ #define TC_UNDEFINE_GROW_VALGRIND_CHUNK(_tc, _new_size) do { \ @@ -1365,7 +1391,16 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons } return ptr; } else if ((tc->size - size) < 1024) { - TC_INVALIDATE_SHRINK_CHUNK(tc, size); + /* + * if we call TC_INVALIDATE_SHRINK_CHUNK() here + * we would need to call TC_UNDEFINE_GROW_CHUNK() + * after each realloc call, which slows down + * testing a lot :-(. + * + * That is why we only mark memory as undefined here. + */ + TC_UNDEFINE_SHRINK_CHUNK(tc, size); + /* do not shrink if we have less than 1k to gain */ tc->size = size; return ptr; -- cgit From 2d514be1ed3b8245157a0a51186ec7f9db828202 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Mon, 16 May 2011 19:06:07 +0200 Subject: talloc: split the handling of FLAG_POOL/FLAG_POOLMEM in _talloc_free_internal The optimization of the object_count == 1 case should only happen for when we're not destroying the pool itself. And it should only happen if the pool itself is still valid. If the pool isn't valid (it has TALLOC_FLAG_FREE), object_count == 1 does not mean that the pool is the last object, which can happen if you use talloc_steal/move() on memory from the pool and then free the pool itself. Thanks to Volker for noticing this! metze --- lib/talloc/talloc.c | 98 ++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 66 insertions(+), 32 deletions(-) (limited to 'lib/talloc/talloc.c') diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c index 2a956dcf0d4..0efeb7a7ada 100644 --- a/lib/talloc/talloc.c +++ b/lib/talloc/talloc.c @@ -709,6 +709,65 @@ _PUBLIC_ void *_talloc_reference_loc(const void *context, const void *ptr, const static void *_talloc_steal_internal(const void *new_ctx, const void *ptr); +static inline void _talloc_free_poolmem(struct talloc_chunk *tc, + const char *location) +{ + struct talloc_chunk *pool; + void *next_tc; + unsigned int *pool_object_count; + + pool = (struct talloc_chunk *)tc->pool; + next_tc = TC_POOLMEM_NEXT_CHUNK(tc); + + tc->flags |= TALLOC_FLAG_FREE; + + /* we mark the freed memory with where we called the free + * from. This means on a double free error we can report where + * the first free came from + */ + tc->name = location; + + TC_INVALIDATE_FULL_CHUNK(tc); + + pool_object_count = talloc_pool_objectcount(pool); + + if (unlikely(*pool_object_count == 0)) { + talloc_abort("Pool object count zero!"); + return; + } + + *pool_object_count -= 1; + + if (unlikely(*pool_object_count == 1 && !(pool->flags & TALLOC_FLAG_FREE))) { + /* + * if there is just one object left in the pool + * and pool->flags does not have TALLOC_FLAG_FREE, + * it means this is the pool itself and + * the rest is available for new objects + * again. + */ + pool->pool = TC_POOL_FIRST_CHUNK(pool); + TC_INVALIDATE_POOL(pool); + } else if (unlikely(*pool_object_count == 0)) { + /* + * we mark the freed memory with where we called the free + * from. This means on a double free error we can report where + * the first free came from + */ + pool->name = location; + + TC_INVALIDATE_FULL_CHUNK(pool); + free(pool); + } else if (pool->pool == next_tc) { + /* + * if pool->pool still points to end of + * 'tc' (which is stored in the 'next_tc' variable), + * we can reclaim the memory of 'tc'. + */ + pool->pool = tc; + } +} + /* internal talloc_free call */ @@ -823,21 +882,10 @@ static inline int _talloc_free_internal(void *ptr, const char *location) */ tc->name = location; - if (tc->flags & (TALLOC_FLAG_POOL|TALLOC_FLAG_POOLMEM)) { - struct talloc_chunk *pool; - void *next_tc = NULL; + if (tc->flags & TALLOC_FLAG_POOL) { unsigned int *pool_object_count; - if (unlikely(tc->flags & TALLOC_FLAG_POOL)) { - pool = tc; - } else { - pool = (struct talloc_chunk *)tc->pool; - next_tc = TC_POOLMEM_NEXT_CHUNK(tc); - - TC_INVALIDATE_FULL_CHUNK(tc); - } - - pool_object_count = talloc_pool_objectcount(pool); + pool_object_count = talloc_pool_objectcount(tc); if (unlikely(*pool_object_count == 0)) { talloc_abort("Pool object count zero!"); @@ -846,26 +894,12 @@ static inline int _talloc_free_internal(void *ptr, const char *location) *pool_object_count -= 1; - if (unlikely(*pool_object_count == 1)) { - /* - * if there is just object left in the pool - * it means this is the pool itself and - * the rest is available for new objects - * again. - */ - pool->pool = TC_POOL_FIRST_CHUNK(pool); - TC_INVALIDATE_POOL(pool); - } else if (unlikely(*pool_object_count == 0)) { - TC_INVALIDATE_FULL_CHUNK(pool); - free(pool); - } else if (pool->pool == next_tc) { - /* - * if pool->pool still points to end of - * 'tc' (which is stored in the 'next_tc' variable), - * we can reclaim the memory of 'tc'. - */ - pool->pool = tc; + if (unlikely(*pool_object_count == 0)) { + TC_INVALIDATE_FULL_CHUNK(tc); + free(tc); } + } else if (tc->flags & TALLOC_FLAG_POOLMEM) { + _talloc_free_poolmem(tc, location); } else { TC_INVALIDATE_FULL_CHUNK(tc); free(tc); -- cgit From 14b662ee4f278764b9dfd620851e908d29f29fc4 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Mon, 16 May 2011 20:23:13 +0200 Subject: talloc: make use of _talloc_free_poolmem() in _talloc_realloc() This should follow the same logic... metze --- lib/talloc/talloc.c | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) (limited to 'lib/talloc/talloc.c') diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c index 0efeb7a7ada..fcd86d754a9 100644 --- a/lib/talloc/talloc.c +++ b/lib/talloc/talloc.c @@ -1550,7 +1550,6 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons } new_ptr = talloc_alloc_pool(tc, size + TC_HDR_SIZE); - *talloc_pool_objectcount(pool_tc) -= 1; if (new_ptr == NULL) { new_ptr = malloc(TC_HDR_SIZE+size); @@ -1559,21 +1558,8 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons if (new_ptr) { memcpy(new_ptr, tc, MIN(tc->size,size) + TC_HDR_SIZE); - TC_INVALIDATE_FULL_CHUNK(tc); - if (*talloc_pool_objectcount(pool_tc) == 1) { - /* - * If the pool is empty now reclaim everything. - */ - pool_tc->pool = TC_POOL_FIRST_CHUNK(pool_tc); - TC_INVALIDATE_POOL(pool_tc); - } else if (next_tc == pool_tc->pool) { - /* - * If it was reallocated and tc was the last - * chunk, we can reclaim the memory of tc. - */ - pool_tc->pool = tc; - } + _talloc_free_poolmem(tc, __location__ "_talloc_realloc"); } } else { -- cgit From 7102105c8954627dc30a851327cf2642ac0783d5 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Mon, 16 May 2011 20:15:59 +0200 Subject: talloc: make really sure only optimize realloc if there's only one pool chunk *talloc_pool_objectcount(pool_tc) == 2 doesn't mean the one of the objects is the pool itself! So we better check for == 1 and calculate the chunk count. metze --- lib/talloc/talloc.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 'lib/talloc/talloc.c') diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c index fcd86d754a9..f3ed9c85672 100644 --- a/lib/talloc/talloc.c +++ b/lib/talloc/talloc.c @@ -1479,8 +1479,13 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons size_t new_chunk_size = TC_ALIGN16(TC_HDR_SIZE + size); size_t space_needed; size_t space_left; + unsigned int chunk_count = *talloc_pool_objectcount(pool_tc); - if (*talloc_pool_objectcount(pool_tc) == 2) { + if (!(pool_tc->flags & TALLOC_FLAG_FREE)) { + chunk_count -= 1; + } + + if (chunk_count == 1) { /* * optimize for the case where 'tc' is the only * chunk in the pool. -- cgit From c281f2fc1a359d0d3b91b94438f11bb7c88170b5 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Tue, 17 May 2011 08:19:04 +0200 Subject: talloc: setup the new 'tc' before TC_UNDEFINE_GROW_CHUNK() _talloc_realloc() metze --- lib/talloc/talloc.c | 1 + 1 file changed, 1 insertion(+) (limited to 'lib/talloc/talloc.c') diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c index f3ed9c85672..95534052bc8 100644 --- a/lib/talloc/talloc.c +++ b/lib/talloc/talloc.c @@ -1512,6 +1512,7 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons memmove(pool_tc->pool, tc, old_used); new_ptr = pool_tc->pool; + tc = (struct talloc_chunk *)new_ptr; TC_UNDEFINE_GROW_CHUNK(tc, size); /* -- cgit From f3b855d2ff9576715afe50d75678829c6bc0842d Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Fri, 8 Apr 2011 12:27:05 +0200 Subject: talloc: use _talloc_free_internal() in talloc_free_children() metze --- lib/talloc/talloc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib/talloc/talloc.c') diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c index 95534052bc8..31308589af1 100644 --- a/lib/talloc/talloc.c +++ b/lib/talloc/talloc.c @@ -1291,7 +1291,7 @@ _PUBLIC_ void talloc_free_children(void *ptr) struct talloc_chunk *p = talloc_parent_chunk(tc->child->refs); if (p) new_parent = TC_PTR_FROM_CHUNK(p); } - if (unlikely(talloc_free(child) == -1)) { + if (unlikely(_talloc_free_internal(child, __location__) == -1)) { if (new_parent == null_context) { struct talloc_chunk *p = talloc_parent_chunk(ptr); if (p) new_parent = TC_PTR_FROM_CHUNK(p); -- cgit From 38633c9f0b7f86673f08903999583ad5b62c3548 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Fri, 8 Apr 2011 12:30:46 +0200 Subject: talloc: fixed a use after free error in talloc_free_children() This is similar to commit 6f51a1f45bf4de062cce7a562477e8140630a53d. metze --- lib/talloc/talloc.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) (limited to 'lib/talloc/talloc.c') diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c index 31308589af1..8333f41c07d 100644 --- a/lib/talloc/talloc.c +++ b/lib/talloc/talloc.c @@ -1287,13 +1287,28 @@ _PUBLIC_ void talloc_free_children(void *ptr) final choice is the null context. */ void *child = TC_PTR_FROM_CHUNK(tc->child); const void *new_parent = null_context; + struct talloc_chunk *old_parent = NULL; if (unlikely(tc->child->refs)) { struct talloc_chunk *p = talloc_parent_chunk(tc->child->refs); if (p) new_parent = TC_PTR_FROM_CHUNK(p); } + /* finding the parent here is potentially quite + expensive, but the alternative, which is to change + talloc to always have a valid tc->parent pointer, + makes realloc more expensive where there are a + large number of children. + + The reason we need the parent pointer here is that + if _talloc_free_internal() fails due to references + or a failing destructor we need to re-parent, but + the free call can invalidate the prev pointer. + */ + if (new_parent == null_context && (tc->child->refs || tc->child->destructor)) { + old_parent = talloc_parent_chunk(ptr); + } if (unlikely(_talloc_free_internal(child, __location__) == -1)) { if (new_parent == null_context) { - struct talloc_chunk *p = talloc_parent_chunk(ptr); + struct talloc_chunk *p = old_parent; if (p) new_parent = TC_PTR_FROM_CHUNK(p); } _talloc_steal_internal(new_parent, child); -- cgit From df2cb2f672569e5d113fe2e77fdc1ee16c8b646d Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Tue, 17 May 2011 08:50:45 +0200 Subject: talloc: splitout _talloc_free_children_internal() metze Autobuild-User: Stefan Metzmacher Autobuild-Date: Tue May 17 10:49:13 CEST 2011 on sn-devel-104 --- lib/talloc/talloc.c | 77 +++++++++++++++++++---------------------------------- 1 file changed, 27 insertions(+), 50 deletions(-) (limited to 'lib/talloc/talloc.c') diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c index 8333f41c07d..4700aa99e8c 100644 --- a/lib/talloc/talloc.c +++ b/lib/talloc/talloc.c @@ -768,6 +768,10 @@ static inline void _talloc_free_poolmem(struct talloc_chunk *tc, } } +static inline void _talloc_free_children_internal(struct talloc_chunk *tc, + void *ptr, + const char *location); + /* internal talloc_free call */ @@ -838,41 +842,7 @@ static inline int _talloc_free_internal(void *ptr, const char *location) tc->flags |= TALLOC_FLAG_LOOP; - while (tc->child) { - /* we need to work out who will own an abandoned child - if it cannot be freed. In priority order, the first - choice is owner of any remaining reference to this - pointer, the second choice is our parent, and the - final choice is the null context. */ - void *child = TC_PTR_FROM_CHUNK(tc->child); - const void *new_parent = null_context; - struct talloc_chunk *old_parent = NULL; - if (unlikely(tc->child->refs)) { - struct talloc_chunk *p = talloc_parent_chunk(tc->child->refs); - if (p) new_parent = TC_PTR_FROM_CHUNK(p); - } - /* finding the parent here is potentially quite - expensive, but the alternative, which is to change - talloc to always have a valid tc->parent pointer, - makes realloc more expensive where there are a - large number of children. - - The reason we need the parent pointer here is that - if _talloc_free_internal() fails due to references - or a failing destructor we need to re-parent, but - the free call can invalidate the prev pointer. - */ - if (new_parent == null_context && (tc->child->refs || tc->child->destructor)) { - old_parent = talloc_parent_chunk(ptr); - } - if (unlikely(_talloc_free_internal(child, location) == -1)) { - if (new_parent == null_context) { - struct talloc_chunk *p = old_parent; - if (p) new_parent = TC_PTR_FROM_CHUNK(p); - } - _talloc_steal_internal(new_parent, child); - } - } + _talloc_free_children_internal(tc, ptr, location); tc->flags |= TALLOC_FLAG_FREE; @@ -1264,21 +1234,10 @@ _PUBLIC_ void *talloc_init(const char *fmt, ...) return ptr; } -/* - this is a replacement for the Samba3 talloc_destroy_pool functionality. It - should probably not be used in new code. It's in here to keep the talloc - code consistent across Samba 3 and 4. -*/ -_PUBLIC_ void talloc_free_children(void *ptr) +static inline void _talloc_free_children_internal(struct talloc_chunk *tc, + void *ptr, + const char *location) { - struct talloc_chunk *tc; - - if (unlikely(ptr == NULL)) { - return; - } - - tc = talloc_chunk_from_ptr(ptr); - while (tc->child) { /* we need to work out who will own an abandoned child if it cannot be freed. In priority order, the first @@ -1306,7 +1265,7 @@ _PUBLIC_ void talloc_free_children(void *ptr) if (new_parent == null_context && (tc->child->refs || tc->child->destructor)) { old_parent = talloc_parent_chunk(ptr); } - if (unlikely(_talloc_free_internal(child, __location__) == -1)) { + if (unlikely(_talloc_free_internal(child, location) == -1)) { if (new_parent == null_context) { struct talloc_chunk *p = old_parent; if (p) new_parent = TC_PTR_FROM_CHUNK(p); @@ -1316,6 +1275,24 @@ _PUBLIC_ void talloc_free_children(void *ptr) } } +/* + this is a replacement for the Samba3 talloc_destroy_pool functionality. It + should probably not be used in new code. It's in here to keep the talloc + code consistent across Samba 3 and 4. +*/ +_PUBLIC_ void talloc_free_children(void *ptr) +{ + struct talloc_chunk *tc; + + if (unlikely(ptr == NULL)) { + return; + } + + tc = talloc_chunk_from_ptr(ptr); + + _talloc_free_children_internal(tc, ptr, __location__); +} + /* Allocate a bit of memory as a child of an existing pointer */ -- cgit