From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Burakov, Anatoly" Subject: Re: [PATCH v2 02/20] mem: allow memseg lists to be marked as external Date: Thu, 20 Sep 2018 10:54:20 +0100 Message-ID: References: <25b7ffe7-2e94-a326-e00e-11b2f3303147@solarflare.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: Neil Horman , John McNamara , Marko Kovacevic , Hemant Agrawal , Shreyansh Jain , Matan Azrad , Shahaf Shuler , Yongseok Koh , Maxime Coquelin , Tiwei Bie , Zhihong Wang , Bruce Richardson , Olivier Matz , 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: Andrew Rybchenko , dev@dpdk.org Return-path: Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id EB16F5F11 for ; Thu, 20 Sep 2018 11:54:27 +0200 (CEST) In-Reply-To: <25b7ffe7-2e94-a326-e00e-11b2f3303147@solarflare.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 20-Sep-18 10:30 AM, Andrew Rybchenko wrote: > 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 > > A couple of minor questions/suggestions below, but it is OK to > go as is even if rejected. > > Acked-by: Andrew Rybchenko > > <...> > >> 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. Accepting externally allocated segments is precisely the point here - we want to find page size of underlying memory, regardless of whether it's internal or external. We use socket ID to identify valid page sizes for a particular heap (since socket ID is technically a heap identifier, as far as external code is concerned), but within that heap there can be multiple segment lists corresponding to that socket ID, each with its own page size. > >> +    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; Sure. If there will be other comments that warrant a v3 respin, i'll incorporate this feedback :) Thanks for the review! > > <...> > -- Thanks, Anatoly