[PATCH v2] common/mlx5: Optimize mlx5 mempool get extmem

[Slava Ovsiienko]

Hi,

Thank you for this optimizing patch.
My concern is this line:
> +	heap = malloc(mp->size * sizeof(struct mlx5_range));
The pool size can be huge and it might cause the large memory allocation 
(on host CPU side).

What is the reason causing "hours" of registering? Reallocs per each pool element?
The mp struct has "struct rte_mempool_memhdr_list mem_list" member.
I think we should consider populating this list with data from
"struct rte_pktmbuf_extmem *ext_mem" on pool creation.

Because of it seems the rte_mempool_mem_iter() functionality is
completely broken for the pools with external memory, and that's why
mlx5 implemented the dedicated branch to handle their registration.

With best regards,
Slava

> -----Original Message-----
> From: Aaron Conole <aconole at redhat.com>
> Sent: Tuesday, October 10, 2023 5:38 PM
> To: dev at dpdk.org
> Cc: John Romein <romein at astron.nl>; Raslan Darawsheh
> <rasland at nvidia.com>; Elena Agostini <eagostini at nvidia.com>; Dmitry
> Kozlyuk <dkozlyuk at nvidia.com>; Matan Azrad <matan at nvidia.com>; Slava
> Ovsiienko <viacheslavo at nvidia.com>; Ori Kam <orika at nvidia.com>;
> Suanming Mou <suanmingm at nvidia.com>
> Subject: [PATCH v2] common/mlx5: Optimize mlx5 mempool get extmem
> 
> From: John Romein <romein at astron.nl>
> 
> This patch reduces the time to allocate and register tens of gigabytes of GPU
> memory from hours to seconds, by sorting the heap only once instead of for
> each object in the mempool.
> 
> Fixes: 690b2a88c2f7 ("common/mlx5: add mempool registration facilities")
> 
> Signed-off-by: John Romein <romein at astron.nl>
> ---
>  drivers/common/mlx5/mlx5_common_mr.c | 69 ++++++++--------------------
>  1 file changed, 20 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/common/mlx5/mlx5_common_mr.c
> b/drivers/common/mlx5/mlx5_common_mr.c
> index 40ff9153bd..77b66e444b 100644
> --- a/drivers/common/mlx5/mlx5_common_mr.c
> +++ b/drivers/common/mlx5/mlx5_common_mr.c
> @@ -1389,63 +1389,23 @@ mlx5_mempool_get_chunks(struct
> rte_mempool *mp, struct mlx5_range **out,
>  	return 0;
>  }
> 
> -struct mlx5_mempool_get_extmem_data {
> -	struct mlx5_range *heap;
> -	unsigned int heap_size;
> -	int ret;
> -};
> -
>  static void
>  mlx5_mempool_get_extmem_cb(struct rte_mempool *mp, void *opaque,
>  			   void *obj, unsigned int obj_idx)
>  {
> -	struct mlx5_mempool_get_extmem_data *data = opaque;
> +	struct mlx5_range *heap = opaque;
>  	struct rte_mbuf *mbuf = obj;
>  	uintptr_t addr = (uintptr_t)mbuf->buf_addr;
> -	struct mlx5_range *seg, *heap;
>  	struct rte_memseg_list *msl;
>  	size_t page_size;
>  	uintptr_t page_start;
> -	unsigned int pos = 0, len = data->heap_size, delta;
> 
>  	RTE_SET_USED(mp);
> -	RTE_SET_USED(obj_idx);
> -	if (data->ret < 0)
> -		return;
> -	/* Binary search for an already visited page. */
> -	while (len > 1) {
> -		delta = len / 2;
> -		if (addr < data->heap[pos + delta].start) {
> -			len = delta;
> -		} else {
> -			pos += delta;
> -			len -= delta;
> -		}
> -	}
> -	if (data->heap != NULL) {
> -		seg = &data->heap[pos];
> -		if (seg->start <= addr && addr < seg->end)
> -			return;
> -	}
> -	/* Determine the page boundaries and remember them. */
> -	heap = realloc(data->heap, sizeof(heap[0]) * (data->heap_size + 1));
> -	if (heap == NULL) {
> -		free(data->heap);
> -		data->heap = NULL;
> -		data->ret = -1;
> -		return;
> -	}
> -	data->heap = heap;
> -	data->heap_size++;
> -	seg = &heap[data->heap_size - 1];
>  	msl = rte_mem_virt2memseg_list((void *)addr);
>  	page_size = msl != NULL ? msl->page_sz : rte_mem_page_size();
>  	page_start = RTE_PTR_ALIGN_FLOOR(addr, page_size);
> -	seg->start = page_start;
> -	seg->end = page_start + page_size;
> -	/* Maintain the heap order. */
> -	qsort(data->heap, data->heap_size, sizeof(heap[0]),
> -	      mlx5_range_compare_start);
> +	heap[obj_idx].start = page_start;
> +	heap[obj_idx].end = page_start + page_size;
>  }
> 
>  /**
> @@ -1457,15 +1417,26 @@ static int
>  mlx5_mempool_get_extmem(struct rte_mempool *mp, struct mlx5_range
> **out,
>  			unsigned int *out_n)
>  {
> -	struct mlx5_mempool_get_extmem_data data;
> +	unsigned int out_size = 1;
> +	struct mlx5_range *heap;
> 
>  	DRV_LOG(DEBUG, "Recovering external pinned pages of mempool
> %s",
>  		mp->name);
> -	memset(&data, 0, sizeof(data));
> -	rte_mempool_obj_iter(mp, mlx5_mempool_get_extmem_cb,
> &data);
> -	*out = data.heap;
> -	*out_n = data.heap_size;
> -	return data.ret;
> +	heap = malloc(mp->size * sizeof(struct mlx5_range));
> +	if (heap == NULL)
> +		return -1;
> +	rte_mempool_obj_iter(mp, mlx5_mempool_get_extmem_cb, heap);
> +	qsort(heap, mp->size, sizeof(heap[0]), mlx5_range_compare_start);
> +	/* remove duplicates */
> +	for (unsigned int i = 1; i < mp->size; i++)
> +		if (heap[out_size - 1].start != heap[i].start)
> +			heap[out_size++] = heap[i];
> +	heap = realloc(heap, out_size * sizeof(struct mlx5_range));
> +	if (heap == NULL)
> +		return -1;
> +	*out = heap;
> +	*out_n = out_size;
> +	return 0;
>  }
> 
>  /**
> --
> 2.41.0