From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 78E007D53 for ; Wed, 23 Aug 2017 15:50:57 +0200 (CEST) Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 23 Aug 2017 06:50:56 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,417,1498546800"; d="scan'208";a="121888026" Received: from smonroyx-mobl.ger.corp.intel.com (HELO [10.237.221.28]) ([10.237.221.28]) by orsmga004.jf.intel.com with ESMTP; 23 Aug 2017 06:50:55 -0700 To: Xueming Li , dev@dpdk.org References: <20170823022926.169272-1-xuemingl@mellanox.com> From: Sergio Gonzalez Monroy Message-ID: Date: Wed, 23 Aug 2017 14:50:54 +0100 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <20170823022926.169272-1-xuemingl@mellanox.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] eal/malloc: fix malloc cookie check X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 23 Aug 2017 13:50:58 -0000 Hi Xueming, I think I have understood what you are doing and why the change is required, but I believe we can improve the commit message. It might be even a good idea to split the patches: 1. RTE_MALLOC_DEBUG fix 2. malloc_elem_free fix On 23/08/2017 03:29, Xueming Li wrote: > From: xuemingl 'From: ..' not needed. > DPDK uses it's own memory management, few regular memory profiler tool > support DPDK now. Malloc cookie check provides limited memory corruption > check, better than nothing. This patch fixes the following: > 1. Replace broken generated configuration macro RTE_LIBRTE_MALLOC_DEBUG > with RTE_MALLOC_DEBUG > 2. Fix malloc size calculation when RTE_MALLOC_DEBUG cookie check > enabled. For the 2nd change, you should mention what is the problem. The issue is that malloc_elem_free is clearing (setting to 0) the trailer cookie when RTE_MALLOC_DEBUG is enabled. You could also add that you would trigger this issue if the elem being freed does not join the next elem. Thanks, Sergio > From real test, it IS very helpful to detect memory corruption. > > A better designed DPDK application should quit nicely with resoure > releasing so that all allocated memory could be checked against cookie. > > Fixes: af75078 ("first public release") > Cc: Sergio Gonzalez Monroy > > Signed-off-by: Xueming Li > --- > lib/librte_eal/common/malloc_elem.c | 8 ++++---- > lib/librte_eal/common/malloc_elem.h | 4 ++-- > test/test/test_malloc.c | 10 +++++----- > 3 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/lib/librte_eal/common/malloc_elem.c b/lib/librte_eal/common/malloc_elem.c > index 150769057..889dffd21 100644 > --- a/lib/librte_eal/common/malloc_elem.c > +++ b/lib/librte_eal/common/malloc_elem.c > @@ -275,14 +275,14 @@ malloc_elem_free(struct malloc_elem *elem) > return -1; > > rte_spinlock_lock(&(elem->heap->lock)); > - size_t sz = elem->size - sizeof(*elem); > + size_t sz = elem->size - sizeof(*elem) - MALLOC_ELEM_TRAILER_LEN; > uint8_t *ptr = (uint8_t *)&elem[1]; > struct malloc_elem *next = RTE_PTR_ADD(elem, elem->size); > if (next->state == ELEM_FREE){ > /* remove from free list, join to this one */ > elem_free_list_remove(next); > join_elem(elem, next); > - sz += sizeof(*elem); > + sz += (sizeof(*elem) + MALLOC_ELEM_TRAILER_LEN); > } > > /* check if previous element is free, if so join with it and return, > @@ -291,8 +291,8 @@ malloc_elem_free(struct malloc_elem *elem) > if (elem->prev != NULL && elem->prev->state == ELEM_FREE) { > elem_free_list_remove(elem->prev); > join_elem(elem->prev, elem); > - sz += sizeof(*elem); > - ptr -= sizeof(*elem); > + sz += (sizeof(*elem) + MALLOC_ELEM_TRAILER_LEN); > + ptr -= (sizeof(*elem) + MALLOC_ELEM_TRAILER_LEN); > elem = elem->prev; > } > malloc_elem_free_list_insert(elem); > diff --git a/lib/librte_eal/common/malloc_elem.h b/lib/librte_eal/common/malloc_elem.h > index f04b2d1e4..ce39129d9 100644 > --- a/lib/librte_eal/common/malloc_elem.h > +++ b/lib/librte_eal/common/malloc_elem.h > @@ -53,13 +53,13 @@ struct malloc_elem { > volatile enum elem_state state; > uint32_t pad; > size_t size; > -#ifdef RTE_LIBRTE_MALLOC_DEBUG > +#ifdef RTE_MALLOC_DEBUG > uint64_t header_cookie; /* Cookie marking start of data */ > /* trailer cookie at start + size */ > #endif > } __rte_cache_aligned; > > -#ifndef RTE_LIBRTE_MALLOC_DEBUG > +#ifndef RTE_MALLOC_DEBUG > static const unsigned MALLOC_ELEM_TRAILER_LEN = 0; > > /* dummy function - just check if pointer is non-null */ > diff --git a/test/test/test_malloc.c b/test/test/test_malloc.c > index 013fd4407..5558acda4 100644 > --- a/test/test/test_malloc.c > +++ b/test/test/test_malloc.c > @@ -108,7 +108,7 @@ test_align_overlap_per_lcore(__attribute__((unused)) void *arg) > } > for(j = 0; j < 1000 ; j++) { > if( *(char *)p1 != 0) { > - printf("rte_zmalloc didn't zero" > + printf("rte_zmalloc didn't zero " > "the allocated memory\n"); > ret = -1; > } > @@ -180,7 +180,7 @@ test_reordered_free_per_lcore(__attribute__((unused)) void *arg) > } > for(j = 0; j < 1000 ; j++) { > if( *(char *)p1 != 0) { > - printf("rte_zmalloc didn't zero" > + printf("rte_zmalloc didn't zero " > "the allocated memory\n"); > ret = -1; > } > @@ -293,7 +293,7 @@ test_multi_alloc_statistics(void) > struct rte_malloc_socket_stats pre_stats, post_stats ,first_stats, second_stats; > size_t size = 2048; > int align = 1024; > -#ifndef RTE_LIBRTE_MALLOC_DEBUG > +#ifndef RTE_MALLOC_DEBUG > int trailer_size = 0; > #else > int trailer_size = RTE_CACHE_LINE_SIZE; > @@ -623,7 +623,7 @@ test_rte_malloc_validate(void) > const size_t request_size = 1024; > size_t allocated_size; > char *data_ptr = rte_malloc(NULL, request_size, RTE_CACHE_LINE_SIZE); > -#ifdef RTE_LIBRTE_MALLOC_DEBUG > +#ifdef RTE_MALLOC_DEBUG > int retval; > char *over_write_vals = NULL; > #endif > @@ -645,7 +645,7 @@ test_rte_malloc_validate(void) > if (allocated_size < request_size) > err_return(); > > -#ifdef RTE_LIBRTE_MALLOC_DEBUG > +#ifdef RTE_MALLOC_DEBUG > > /****** change the header to be bad */ > char save_buf[64];