From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Rybchenko <arybchenko@solarflare.com>
Subject: Re: [PATCH v2 02/20] mem: allow memseg lists to be
	marked as external
Date: Thu, 20 Sep 2018 12:30:40 +0300
Message-ID: <25b7ffe7-2e94-a326-e00e-11b2f3303147@solarflare.com>
References: <cover.1536064999.git.anatoly.burakov@intel.com>
 <cover.1537363686.git.anatoly.burakov@intel.com>
 <a4b1554a94b783bf5dc9dc1a80258f5f7613e221.1537363686.git.anatoly.burakov@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 8bit
Cc: Neil Horman <nhorman@tuxdriver.com>, John McNamara
 <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
 Hemant Agrawal <hemant.agrawal@nxp.com>, Shreyansh Jain
 <shreyansh.jain@nxp.com>, Matan Azrad <matan@mellanox.com>, Shahaf Shuler
 <shahafs@mellanox.com>, Yongseok Koh <yskoh@mellanox.com>, Maxime Coquelin
 <maxime.coquelin@redhat.com>, Tiwei Bie <tiwei.bie@intel.com>, Zhihong Wang
 <zhihong.wang@intel.com>, Bruce Richardson <bruce.richardson@intel.com>,
 Olivier Matz <olivier.matz@6wind.com>, <laszlo.madarassy@ericsson.com>,
 <laszlo.vadkerti@ericsson.com>, <andras.kovacs@ericsson.com>,
 <winnie.tian@ericsson.com>, <daniel.andrasi@ericsson.com>,
 <janos.kobor@ericsson.com>, <geza.koblo@ericsson.com>,
 <srinath.mannam@broadcom.com>, <scott.branden@broadcom.com>,
 <ajit.khaparde@broadcom.com>, <keith.wiles@intel.com>, <thomas@monjalon.net>
To: Anatoly Burakov <anatoly.burakov@intel.com>, <dev@dpdk.org>
Return-path: <dev-bounces@dpdk.org>
Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com
 [148.163.129.52]) by dpdk.org (Postfix) with ESMTP id A18595F11
 for <dev@dpdk.org>; Thu, 20 Sep 2018 11:34:12 +0200 (CEST)
In-Reply-To: <a4b1554a94b783bf5dc9dc1a80258f5f7613e221.1537363686.git.anatoly.burakov@intel.com>
Content-Language: en-GB
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

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@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@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;

<...>