From mboxrd@z Thu Jan 1 00:00:00 1970 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) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 20 Sep 2018 02:54:26 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.53,397,1531810800"; d="scan'208";a="264221505" Received: from aburakov-mobl1.ger.corp.intel.com (HELO [10.237.220.55]) ([10.237.220.55]) by fmsmga005.fm.intel.com with ESMTP; 20 Sep 2018 02:54:21 -0700 To: Andrew Rybchenko , dev@dpdk.org 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 References: <25b7ffe7-2e94-a326-e00e-11b2f3303147@solarflare.com> From: "Burakov, Anatoly" Message-ID: Date: Thu, 20 Sep 2018 10:54:20 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <25b7ffe7-2e94-a326-e00e-11b2f3303147@solarflare.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v2 02/20] mem: allow memseg lists to be marked as external X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 20 Sep 2018 09:54:28 -0000 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