[dpdk-dev] [PATCH v2 02/20] mem: allow memseg lists to be marked as external

[Andrew Rybchenko]

On 9/19/18 4:56 PM, Anatoly Burakov wrote:
> When we allocate and use DPDK memory, we need to be able to
> differentiate between DPDK hugepage segments and segments that
> were made part of DPDK but are externally allocated. Add such
> a property to memseg lists.
>
> This breaks the ABI, so bump the EAL library ABI version and
> document the change in release notes.
>
> All current calls for memseg walk functions were adjusted to
> ignore external segments where it made sense.
>
> Mempools is a special case, because we may be asked to allocate
> a mempool on a specific socket, and we need to ignore all page
> sizes on other heaps or other sockets. Previously, this
> assumption of knowing all page sizes was not a problem, but it
> will be now, so we have to match socket ID with page size when
> calculating minimum page size for a mempool.
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov at intel.com>

A couple of minor questions/suggestions below, but it is OK to
go as is even if rejected.

Acked-by: Andrew Rybchenko <arybchenko at solarflare.com>

<...>

> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> index 03e6b5f73..d61c77da3 100644
> --- a/lib/librte_mempool/rte_mempool.c
> +++ b/lib/librte_mempool/rte_mempool.c
> @@ -99,25 +99,40 @@ static unsigned optimize_object_size(unsigned obj_size)
>   	return new_obj_size * RTE_MEMPOOL_ALIGN;
>   }
>   
> +struct pagesz_walk_arg {
> +	int socket_id;
> +	size_t min;
> +};
> +
>   static int
>   find_min_pagesz(const struct rte_memseg_list *msl, void *arg)
>   {
> -	size_t *min = arg;
> +	struct pagesz_walk_arg *wa = arg;
> +	bool valid;
>   
> -	if (msl->page_sz < *min)
> -		*min = msl->page_sz;
> +	valid = msl->socket_id == wa->socket_id;

Is it intended that we accept externally allocated segment
if it is on requested socket? If so, it would be good to add
comment to explain why.

> +	valid |= wa->socket_id == SOCKET_ID_ANY && msl->external == 0;
> +
> +	if (!valid)
> +		return 0;
> +
> +	if (msl->page_sz < wa->min)
> +		wa->min = msl->page_sz;

I'd suggest to keep single return (it is just a bit shorter)
if (valid && msl->page_sz < wa->min)
          wa->min = msl->page_sz;

<...>