Re: [dpdk-dev] [PATCH 2/4] malloc: separate destroying memseg list and heap data

[Yongseok Koh <yskoh@mellanox.com>]

On Thu, Nov 29, 2018 at 01:48:33PM +0000, Anatoly Burakov wrote:
> Currently, destroying external heap chunk and its memseg list is
> part of one process. When we will gain the ability to unregister
> external memory from DPDK that doesn't have any heap structures
> associated with it, we need to be able to find and destroy
> memseg lists as well as heap data separately.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---

Acked-by: Yongseok Koh <yskoh@mellanox.com>

Thanks

>  lib/librte_eal/common/malloc_heap.c |  70 +++++++++++++++----
>  lib/librte_eal/common/malloc_heap.h |   6 ++
>  lib/librte_eal/common/rte_malloc.c  | 104 ++++++++++------------------
>  3 files changed, 102 insertions(+), 78 deletions(-)
> 
> diff --git a/lib/librte_eal/common/malloc_heap.c b/lib/librte_eal/common/malloc_heap.c
> index 25693481f..fa0cb0799 100644
> --- a/lib/librte_eal/common/malloc_heap.c
> +++ b/lib/librte_eal/common/malloc_heap.c
> @@ -1067,12 +1067,9 @@ malloc_heap_dump(struct malloc_heap *heap, FILE *f)
>  }
>  
>  static int
> -destroy_seg(struct malloc_elem *elem, size_t len)
> +destroy_elem(struct malloc_elem *elem, size_t len)
>  {
>  	struct malloc_heap *heap = elem->heap;
> -	struct rte_memseg_list *msl;
> -
> -	msl = elem->msl;
>  
>  	/* notify all subscribers that a memory area is going to be removed */
>  	eal_memalloc_mem_event_notify(RTE_MEM_EVENT_FREE, elem, len);
> @@ -1085,13 +1082,6 @@ destroy_seg(struct malloc_elem *elem, size_t len)
>  
>  	memset(elem, 0, sizeof(*elem));
>  
> -	/* destroy the fbarray backing this memory */
> -	if (rte_fbarray_destroy(&msl->memseg_arr) < 0)
> -		return -1;
> -
> -	/* reset the memseg list */
> -	memset(msl, 0, sizeof(*msl));
> -
>  	return 0;
>  }
>  
> @@ -1158,6 +1148,62 @@ malloc_heap_create_external_seg(void *va_addr, rte_iova_t iova_addrs[],
>  	return msl;
>  }
>  
> +struct extseg_walk_arg {
> +	void *va_addr;
> +	size_t len;
> +	struct rte_memseg_list *msl;
> +};
> +
> +static int
> +extseg_walk(const struct rte_memseg_list *msl, void *arg)
> +{
> +	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
> +	struct extseg_walk_arg *wa = arg;
> +
> +	if (msl->base_va == wa->va_addr && msl->len == wa->len) {
> +		unsigned int found_idx;
> +
> +		/* msl is const */
> +		found_idx = msl - mcfg->memsegs;
> +		wa->msl = &mcfg->memsegs[found_idx];
> +		return 1;
> +	}
> +	return 0;
> +}
> +
> +struct rte_memseg_list *
> +malloc_heap_find_external_seg(void *va_addr, size_t len)
> +{
> +	struct extseg_walk_arg wa;
> +	int res;
> +
> +	wa.va_addr = va_addr;
> +	wa.len = len;
> +
> +	res = rte_memseg_list_walk_thread_unsafe(extseg_walk, &wa);
> +
> +	if (res != 1) {
> +		/* 0 means nothing was found, -1 shouldn't happen */
> +		if (res == 0)
> +			rte_errno = ENOENT;
> +		return NULL;
> +	}
> +	return wa.msl;
> +}
> +
> +int
> +malloc_heap_destroy_external_seg(struct rte_memseg_list *msl)
> +{
> +	/* destroy the fbarray backing this memory */
> +	if (rte_fbarray_destroy(&msl->memseg_arr) < 0)
> +		return -1;
> +
> +	/* reset the memseg list */
> +	memset(msl, 0, sizeof(*msl));
> +
> +	return 0;
> +}
> +
>  int
>  malloc_heap_add_external_memory(struct malloc_heap *heap,
>  		struct rte_memseg_list *msl)
> @@ -1206,7 +1252,7 @@ malloc_heap_remove_external_memory(struct malloc_heap *heap, void *va_addr,
>  		rte_errno = EBUSY;
>  		return -1;
>  	}
> -	return destroy_seg(elem, len);
> +	return destroy_elem(elem, len);
>  }
>  
>  int
> diff --git a/lib/librte_eal/common/malloc_heap.h b/lib/librte_eal/common/malloc_heap.h
> index 255a315b8..ca9ff666f 100644
> --- a/lib/librte_eal/common/malloc_heap.h
> +++ b/lib/librte_eal/common/malloc_heap.h
> @@ -44,6 +44,12 @@ malloc_heap_create_external_seg(void *va_addr, rte_iova_t iova_addrs[],
>  		unsigned int n_pages, size_t page_sz, const char *seg_name,
>  		unsigned int socket_id);
>  
> +struct rte_memseg_list *
> +malloc_heap_find_external_seg(void *va_addr, size_t len);
> +
> +int
> +malloc_heap_destroy_external_seg(struct rte_memseg_list *msl);
> +
>  int
>  malloc_heap_add_external_memory(struct malloc_heap *heap,
>  		struct rte_memseg_list *msl);
> diff --git a/lib/librte_eal/common/rte_malloc.c b/lib/librte_eal/common/rte_malloc.c
> index 66bfe63c3..9a82e3386 100644
> --- a/lib/librte_eal/common/rte_malloc.c
> +++ b/lib/librte_eal/common/rte_malloc.c
> @@ -396,6 +396,7 @@ rte_malloc_heap_memory_remove(const char *heap_name, void *va_addr, size_t len)
>  {
>  	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
>  	struct malloc_heap *heap = NULL;
> +	struct rte_memseg_list *msl;
>  	int ret;
>  
>  	if (heap_name == NULL || va_addr == NULL || len == 0 ||
> @@ -420,9 +421,19 @@ rte_malloc_heap_memory_remove(const char *heap_name, void *va_addr, size_t len)
>  		goto unlock;
>  	}
>  
> +	msl = malloc_heap_find_external_seg(va_addr, len);
> +	if (msl == NULL) {
> +		ret = -1;
> +		goto unlock;
> +	}
> +
>  	rte_spinlock_lock(&heap->lock);
>  	ret = malloc_heap_remove_external_memory(heap, va_addr, len);
>  	rte_spinlock_unlock(&heap->lock);
> +	if (ret != 0)
> +		goto unlock;
> +
> +	ret = malloc_heap_destroy_external_seg(msl);
>  
>  unlock:
>  	rte_rwlock_write_unlock(&mcfg->memory_hotplug_lock);
> @@ -430,63 +441,12 @@ rte_malloc_heap_memory_remove(const char *heap_name, void *va_addr, size_t len)
>  	return ret;
>  }
>  
> -struct sync_mem_walk_arg {
> -	void *va_addr;
> -	size_t len;
> -	int result;
> -	bool attach;
> -};
> -
> -static int
> -sync_mem_walk(const struct rte_memseg_list *msl, void *arg)
> -{
> -	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
> -	struct sync_mem_walk_arg *wa = arg;
> -	size_t len = msl->page_sz * msl->memseg_arr.len;
> -
> -	if (msl->base_va == wa->va_addr &&
> -			len == wa->len) {
> -		struct rte_memseg_list *found_msl;
> -		int msl_idx, ret;
> -
> -		/* msl is const */
> -		msl_idx = msl - mcfg->memsegs;
> -		found_msl = &mcfg->memsegs[msl_idx];
> -
> -		if (wa->attach) {
> -			ret = rte_fbarray_attach(&found_msl->memseg_arr);
> -		} else {
> -			/* notify all subscribers that a memory area is about to
> -			 * be removed
> -			 */
> -			eal_memalloc_mem_event_notify(RTE_MEM_EVENT_FREE,
> -					msl->base_va, msl->len);
> -			ret = rte_fbarray_detach(&found_msl->memseg_arr);
> -		}
> -
> -		if (ret < 0) {
> -			wa->result = -rte_errno;
> -		} else {
> -			/* notify all subscribers that a new memory area was
> -			 * added
> -			 */
> -			if (wa->attach)
> -				eal_memalloc_mem_event_notify(
> -						RTE_MEM_EVENT_ALLOC,
> -						msl->base_va, msl->len);
> -			wa->result = 0;
> -		}
> -		return 1;
> -	}
> -	return 0;
> -}
> -
>  static int
>  sync_memory(const char *heap_name, void *va_addr, size_t len, bool attach)
>  {
>  	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
>  	struct malloc_heap *heap = NULL;
> -	struct sync_mem_walk_arg wa;
> +	struct rte_memseg_list *msl;
>  	int ret;
>  
>  	if (heap_name == NULL || va_addr == NULL || len == 0 ||
> @@ -513,23 +473,35 @@ sync_memory(const char *heap_name, void *va_addr, size_t len, bool attach)
>  	}
>  
>  	/* find corresponding memseg list to sync to */
> -	wa.va_addr = va_addr;
> -	wa.len = len;
> -	wa.result = -ENOENT; /* fail unless explicitly told to succeed */
> -	wa.attach = attach;
> -
> -	/* we're already holding a read lock */
> -	rte_memseg_list_walk_thread_unsafe(sync_mem_walk, &wa);
> -
> -	if (wa.result < 0) {
> -		rte_errno = -wa.result;
> +	msl = malloc_heap_find_external_seg(va_addr, len);
> +	if (msl == NULL) {
>  		ret = -1;
> -	} else {
> -		/* notify all subscribers that a new memory area was added */
> -		if (attach)
> +		goto unlock;
> +	}
> +
> +	if (attach) {
> +		ret = rte_fbarray_attach(&msl->memseg_arr);
> +		if (ret == 0) {
> +			/* notify all subscribers that a new memory area was
> +			 * added.
> +			 */
>  			eal_memalloc_mem_event_notify(RTE_MEM_EVENT_ALLOC,
>  					va_addr, len);
> -		ret = 0;
> +		} else {
> +			ret = -1;
> +			goto unlock;
> +		}
> +	} else {
> +		/* notify all subscribers that a memory area is about to
> +		 * be removed.
> +		 */
> +		eal_memalloc_mem_event_notify(RTE_MEM_EVENT_FREE,
> +				msl->base_va, msl->len);
> +		ret = rte_fbarray_detach(&msl->memseg_arr);
> +		if (ret < 0) {
> +			ret = -1;
> +			goto unlock;
> +		}
>  	}
>  unlock:
>  	rte_rwlock_read_unlock(&mcfg->memory_hotplug_lock);
> -- 
> 2.17.1