talloc_pooled_object patchset

[Jeremy Allison]

On Tue, Aug 06, 2013 at 07:47:22AM +0200, Volker Lendecke wrote:
> Hi!
> 
> Attached find a patchset that implements
> talloc_pooled_object. The idea is basically that
> tevent_req_create only does one malloc instead of three, and
> for example cp_smb_filename also does just one:
> 
> talloc_pooled_object allocates a basic object as a pool
> similar to talloc(). Additionally, this object itself is a
> pool that will host num_members of user-visible size
> member_size.
> 
> A very simple test fetching a 1G file with aio read size = 1
> with smbclient according to valgrind's callgrind gives a
> reduction of user-space instructions of roughly 7%. In more
> complex tests, I think this has also the potential to reduce
> malloc fragmentation.
> 
> This is not yet ready for pushing, with valgrind I still see
> invalid writes. But I suspect this is due to invalid
> valgrind instrumentation, this patchset does survive
> autobuild.

Here are the fixes on top of your patchset that
fix the valgrind issues.

Note the actual code you wrote is 100% good, it's
merely the valgrind marking of regions as NOACCESS/
UNDEFINED that needed fixing :-).

I'm going to break this up into small patches
and then submit on top of your patchset for
review so we can get this code in !

Cheers & thanks for the *excellent* work here !

Are there more areas we can use talloc_pooled_object
as a custom allocator to reduce our allocations
and fragmentations ? Anywhere we're allocating
talloc'ed structures would seem to be good for
this.

Cheers,

	Jeremy.
-------------- next part --------------
diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c
index 8d8e621..9090610 100644
--- a/lib/talloc/talloc.c
+++ b/lib/talloc/talloc.c
@@ -609,6 +609,10 @@ static inline void *__talloc_with_prefix(const void *context, size_t size,
 			return NULL;
 		}
 
+#if defined(DEVELOPER) && defined(VALGRIND_MAKE_MEM_UNDEFINED)
+		VALGRIND_MAKE_MEM_UNDEFINED(ptr, total_len);
+#endif
+
 		tc = (struct talloc_chunk *)(ptr + prefix_len);
 		tc->flags = TALLOC_MAGIC;
 		tc->pool  = NULL;
@@ -729,7 +733,6 @@ _PUBLIC_ void *_talloc_pooled_object(const void *ctx, size_t size,
 #if defined(DEVELOPER) && defined(VALGRIND_MAKE_MEM_UNDEFINED)
 	VALGRIND_MAKE_MEM_UNDEFINED(pool_hdr->end, size);
 #endif
-	memset(pool_hdr->end, 0, size);
 
 	pool_hdr->end = ((char *)pool_hdr->end + TC_ALIGN16(size));
 
@@ -885,11 +888,10 @@ static inline void _talloc_free_poolmem(struct talloc_chunk *tc,
 		 */
 		pool_tc->name = location;
 
-		TC_INVALIDATE_FULL_CHUNK(pool_tc);
-
 		if (pool_tc->flags & TALLOC_FLAG_POOLMEM) {
 			_talloc_free_poolmem(pool_tc, location);
 		} else {
+			TC_INVALIDATE_FULL_CHUNK(pool_tc);
 			free(pool);
 		}
 		return;
@@ -1680,6 +1682,11 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons
 
 		if (new_ptr == NULL) {
 			new_ptr = malloc(TC_HDR_SIZE+size);
+#if defined(DEVELOPER) && defined(VALGRIND_MAKE_MEM_UNDEFINED)
+			if (new_ptr) {
+				VALGRIND_MAKE_MEM_UNDEFINED(new_ptr, TC_HDR_SIZE+size);
+			}
+#endif
 			malloced = true;
 		}
 
@@ -1690,6 +1697,9 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons
 	} else {
 		new_ptr = malloc(size + TC_HDR_SIZE);
 		if (new_ptr) {
+#if defined(DEVELOPER) && defined(VALGRIND_MAKE_MEM_UNDEFINED)
+			VALGRIND_MAKE_MEM_UNDEFINED(new_ptr, size+TC_HDR_SIZE);
+#endif
 			memcpy(new_ptr, tc, MIN(tc->size, size) + TC_HDR_SIZE);
 			free(tc);
 		}
@@ -1722,6 +1732,25 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons
 				size_t old_used = TC_HDR_SIZE + tc->size;
 				size_t new_used = TC_HDR_SIZE + size;
 				new_ptr = start;
+#if defined(DEVELOPER) && defined(VALGRIND_MAKE_MEM_UNDEFINED)
+				{
+					/*
+					 * The area from
+					 * start -> tc may have
+					 * been freed and thus been marked as
+					 * VALGRIND_MEM_NOACCESS. Set it to
+					 * VALGRIND_MEM_UNDEFINED so we can
+					 * copy into it without valgrind errors.
+					 * We can't just mark
+					 * new_ptr -> new_ptr + old_used
+					 * as this may overlap on top of tc,
+					 * (which is why we use memmove, not
+					 * memcpy below) hence the MIN.
+					 */
+					size_t undef_len = MIN((((char *)tc) - ((char *)new_ptr)),old_used);
+					VALGRIND_MAKE_MEM_UNDEFINED(new_ptr, undef_len);
+				}
+#endif
 				memmove(new_ptr, tc, old_used);
 
 				tc = (struct talloc_chunk *)new_ptr;
@@ -1788,6 +1817,11 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons
 		if (new_ptr == NULL) {
 			new_ptr = malloc(TC_HDR_SIZE+size);
 			malloced = true;
+#if defined(DEVELOPER) && defined(VALGRIND_MAKE_MEM_UNDEFINED)
+			if (new_ptr) {
+				VALGRIND_MAKE_MEM_UNDEFINED(new_ptr, TC_HDR_SIZE+size);
+			}
+#endif
 		}
 
 		if (new_ptr) {