From a3af10a801a40fe990ee5db63c6dd6cb97713e4c Mon Sep 17 00:00:00 2001 From: Jeff Darcy Date: Tue, 28 Apr 2015 04:40:00 -0400 Subject: core: use reference counting for mem_acct structures When freeing memory, our memory-accounting code expects to be able to dereference from the (previously) allocated block to its owning translator. However, as we have already found once in option validation and twice in logging, that translator might itself have been freed and the dereference attempt causes on of our daemons to crash with SIGSEGV. This patch attempts to fix that as follows: * We no longer embed a struct mem_acct directly in a struct xlator, but instead allocate it separately. * Allocated memory blocks now contain a pointer to the mem_acct instead of the xlator. * The mem_acct structure contains a reference count, manipulated in both the normal and translator allocate/free code using atomic increments and decrements. * Because it's now a separate structure, we can defer freeing the mem_acct until its reference count reaches zero (either way). * Some unit tests were disabled, because they embedded their own copies of the implementation for what they were supposedly testing. Life's too short to spend time fixing tests that seem designed to impede progress by requiring a certain implementation as well as behavior. Change-Id: Id929b11387927136f78626901729296b6c0d0fd7 BUG: 1219026 Signed-off-by: Jeff Darcy Reviewed-on: http://review.gluster.org/10417 Tested-by: Gluster Build System Reviewed-by: Krishnan Parthasarathi Reviewed-by: Niels de Vos Reviewed-by: Pranith Kumar Karampuri Reviewed-on: http://review.gluster.org/10723 Tested-by: NetBSD Build System --- libglusterfs/src/mem-pool.c | 101 ++++++++++++++++++++++++++------------------ 1 file changed, 59 insertions(+), 42 deletions(-) (limited to 'libglusterfs/src/mem-pool.c') diff --git a/libglusterfs/src/mem-pool.c b/libglusterfs/src/mem-pool.c index 0f9d792c0f1..7d95364c4b2 100644 --- a/libglusterfs/src/mem-pool.c +++ b/libglusterfs/src/mem-pool.c @@ -59,30 +59,32 @@ gf_mem_set_acct_info (xlator_t *xl, char **alloc_ptr, size_t size, GF_ASSERT (xl != NULL); - GF_ASSERT (xl->mem_acct.rec != NULL); + GF_ASSERT (xl->mem_acct != NULL); - GF_ASSERT (type <= xl->mem_acct.num_types); + GF_ASSERT (type <= xl->mem_acct->num_types); - LOCK(&xl->mem_acct.rec[type].lock); + LOCK(&xl->mem_acct->rec[type].lock); { - if (!xl->mem_acct.rec[type].typestr) - xl->mem_acct.rec[type].typestr = typestr; - xl->mem_acct.rec[type].size += size; - xl->mem_acct.rec[type].num_allocs++; - xl->mem_acct.rec[type].total_allocs++; - xl->mem_acct.rec[type].max_size = - max (xl->mem_acct.rec[type].max_size, - xl->mem_acct.rec[type].size); - xl->mem_acct.rec[type].max_num_allocs = - max (xl->mem_acct.rec[type].max_num_allocs, - xl->mem_acct.rec[type].num_allocs); + if (!xl->mem_acct->rec[type].typestr) + xl->mem_acct->rec[type].typestr = typestr; + xl->mem_acct->rec[type].size += size; + xl->mem_acct->rec[type].num_allocs++; + xl->mem_acct->rec[type].total_allocs++; + xl->mem_acct->rec[type].max_size = + max (xl->mem_acct->rec[type].max_size, + xl->mem_acct->rec[type].size); + xl->mem_acct->rec[type].max_num_allocs = + max (xl->mem_acct->rec[type].max_num_allocs, + xl->mem_acct->rec[type].num_allocs); } - UNLOCK(&xl->mem_acct.rec[type].lock); + UNLOCK(&xl->mem_acct->rec[type].lock); + + INCREMENT_ATOMIC (xl->mem_acct->lock, xl->mem_acct->refcnt); header = (struct mem_header *) ptr; header->type = type; header->size = size; - header->xlator = xl; + header->mem_acct = xl->mem_acct; header->magic = GF_MEM_HEADER_MAGIC; ptr += sizeof (struct mem_header); @@ -149,27 +151,23 @@ __gf_malloc (size_t size, uint32_t type, const char *typestr) void * __gf_realloc (void *ptr, size_t size) { - uint32_t type = 0; size_t tot_size = 0; - xlator_t *xl = NULL; char *new_ptr; - struct mem_header *header = NULL; + struct mem_header *old_header = NULL; + struct mem_header *new_header = NULL; + struct mem_header tmp_header; if (!THIS->ctx->mem_acct_enable) return REALLOC (ptr, size); REQUIRE(NULL != ptr); - tot_size = size + GF_MEM_HEADER_SIZE + GF_MEM_TRAILER_SIZE; - - header = (struct mem_header *) (ptr - GF_MEM_HEADER_SIZE); - - GF_ASSERT (header->magic == GF_MEM_HEADER_MAGIC); + old_header = (struct mem_header *) (ptr - GF_MEM_HEADER_SIZE); + GF_ASSERT (old_header->magic == GF_MEM_HEADER_MAGIC); + tmp_header = *old_header; - xl = (xlator_t *) header->xlator; - type = header->type; - - new_ptr = realloc (header, tot_size); + tot_size = size + GF_MEM_HEADER_SIZE + GF_MEM_TRAILER_SIZE; + new_ptr = realloc (old_header, tot_size); if (!new_ptr) { gf_msg_nomem ("", GF_LOG_ALERT, tot_size); return NULL; @@ -181,8 +179,25 @@ __gf_realloc (void *ptr, size_t size) * in ptr, but the compiler warnings complained * about the casting to and forth from void ** to * char **. - */ + * TBD: it would be nice to adjust the memory accounting info here, + * but calling gf_mem_set_acct_info here is wrong because it bumps + * up counts as though this is a new allocation - which it's not. + * The consequence of doing nothing here is only that the sizes will be + * wrong, but at least the counts won't be. + uint32_t type = 0; + xlator_t *xl = NULL; + type = header->type; + xl = (xlator_t *) header->xlator; gf_mem_set_acct_info (xl, &new_ptr, size, type, NULL); + */ + + new_header = (struct mem_header *) new_ptr; + *new_header = tmp_header; + new_header->size = size; + + new_ptr += sizeof (struct mem_header); + /* data follows in this gap of 'size' bytes */ + *(uint32_t *) (new_ptr + size) = GF_MEM_TRAILER_MAGIC; return (void *)new_ptr; } @@ -235,7 +250,7 @@ __gf_mem_invalidate (void *ptr) struct mem_invalid inval = { .magic = GF_MEM_INVALID_MAGIC, - .xlator = header->xlator, + .mem_acct = header->mem_acct, .type = header->type, .size = header->size, .baseaddr = ptr + GF_MEM_HEADER_SIZE, @@ -271,7 +286,7 @@ void __gf_free (void *free_ptr) { void *ptr = NULL; - xlator_t *xl = NULL; + struct mem_acct *mem_acct; struct mem_header *header = NULL; if (!THIS->ctx->mem_acct_enable) { @@ -288,11 +303,8 @@ __gf_free (void *free_ptr) //Possible corruption, assert here GF_ASSERT (GF_MEM_HEADER_MAGIC == header->magic); - //gf_free expects xl to be available - GF_ASSERT (header->xlator != NULL); - xl = header->xlator; - - if (!xl->mem_acct.rec) { + mem_acct = header->mem_acct; + if (!mem_acct) { goto free; } @@ -300,16 +312,21 @@ __gf_free (void *free_ptr) GF_ASSERT (GF_MEM_TRAILER_MAGIC == *(uint32_t *)((char *)free_ptr + header->size)); - LOCK (&xl->mem_acct.rec[header->type].lock); + LOCK (&mem_acct->rec[header->type].lock); { - xl->mem_acct.rec[header->type].size -= header->size; - xl->mem_acct.rec[header->type].num_allocs--; + mem_acct->rec[header->type].size -= header->size; + mem_acct->rec[header->type].num_allocs--; /* If all the instaces are freed up then ensure typestr is * set to NULL */ - if (!xl->mem_acct.rec[header->type].num_allocs) - xl->mem_acct.rec[header->type].typestr = NULL; + if (!mem_acct->rec[header->type].num_allocs) + mem_acct->rec[header->type].typestr = NULL; } - UNLOCK (&xl->mem_acct.rec[header->type].lock); + UNLOCK (&mem_acct->rec[header->type].lock); + + if (DECREMENT_ATOMIC (mem_acct->lock, mem_acct->refcnt) == 0) { + FREE (mem_acct); + } + free: #ifdef DEBUG __gf_mem_invalidate (ptr); -- cgit