From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-f51.google.com (mail-ed1-f51.google.com [209.85.208.51]) by dpdk.org (Postfix) with ESMTP id 379821B4C6 for ; Thu, 27 Sep 2018 15:43:00 +0200 (CEST) Received: by mail-ed1-f51.google.com with SMTP id h4-v6so5080323edi.6 for ; Thu, 27 Sep 2018 06:43:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=netronome-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=aSXpWY/oeGDnTVD4Rf281d6PrtHvSz38pGkVRrdWLY0=; b=1Kx/UjaSj9au9r3kV9vYrD+a999SrkMivXO5eAK88aKP03F0ZPbaRWZTwFXe2ZbmcT 4yV92NS+bfrGoVPdAu2tes2THtzhyKO9KldJn6sc32ni4tQsmUSHfikRQkeXzSJXDcAK 5qEnjEoP+swS6vdLPPMnjoqPjdm7Y+fhJB/pYncrDLr3WNL59p3zUh47brL9JxsTe563 L7ODU2gWFHXj92+IK0W5NTL9XEj4UmjI4cOuUULyoeAFfpD7OjZWn2CV/EvqeHg/oPhI OL2wnCjQ9Pq/WFngaFWqwJAzaTrJaPGBJkd0NNfgpYh/UKHKiJI/CTto0pifhwXQzLaW uahw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=aSXpWY/oeGDnTVD4Rf281d6PrtHvSz38pGkVRrdWLY0=; b=oEQntOhlyPFuRaZozpqByWvoBbsRz8tGLaNI5adtxwNzbvXLMm4Z7oxhP6UpVIu1+r lS7jeYhw6Sioqo35RzGBHXI8biF4uo4TgAUEXhmzKDuxeErTx91LgR6oipfYpJ3PgyaY prP7qczgmu3pLeONMn932FmfYuZkjNwAzj2sqtpq5lHf/70+0KQNvetLEODD0J3ybJEq Yga6OgjzcRgY3tUZrHuwKxxKXAULguxRyo3x49ibXdpALkjML1Oqlm/AnuqOK/rX13hd spg7f/AIn8IItTck/3iLDJnkFrgT612cq3p+5zULZUBVNJtZMi/fmsUUD1UfkPl4Inw/ RYxQ== X-Gm-Message-State: ABuFfogEfswXqfSaXmCi0XcDigUOYC0lVDtrkIRdnbFB/TyK3aOGPybA d7V42NcH1nCrTpp6ykk3Ir2xAQJQAQ7+nH43NJUqVA== X-Google-Smtp-Source: ACcGV63W0EIdv5rY48yD5wIbSwpYmuz07QbYtHE1sUa1RYiqg4Fgmkn6zyKDcBbuPqw7z+NdAzUwX35rY7NsEi/qiEs= X-Received: by 2002:a05:6402:1545:: with SMTP id p5mr17244997edx.104.1538055779875; Thu, 27 Sep 2018 06:42:59 -0700 (PDT) MIME-Version: 1.0 References: <0b8c2280-f627-87b2-0768-0a4b26b69186@intel.com> In-Reply-To: <0b8c2280-f627-87b2-0768-0a4b26b69186@intel.com> From: Alejandro Lucero Date: Thu, 27 Sep 2018 14:42:48 +0100 Message-ID: To: "Burakov, Anatoly" Cc: dev , "Mcnamara, John" , marko.kovacevic@intel.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 , "Wiles, Keith" , Bruce Richardson , Thomas Monjalon , Shreyansh Jain , Shahaf Shuler , Andrew Rybchenko Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v6 04/21] mem: do not check for invalid socket ID 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, 27 Sep 2018 13:43:00 -0000 On Thu, Sep 27, 2018 at 2:22 PM Burakov, Anatoly wrote: > On 27-Sep-18 2:14 PM, Alejandro Lucero wrote: > > On Thu, Sep 27, 2018 at 11:41 AM Anatoly Burakov < > anatoly.burakov@intel.com> > > wrote: > > > >> We will be assigning "invalid" socket ID's to external heap, and > >> malloc will now be able to verify if a supplied socket ID is in > >> fact a valid one, rendering parameter checks for sockets > >> obsolete. > >> > >> This changes the semantics of what we understand by "socket ID", > >> so document the change in the release notes. > >> > >> Signed-off-by: Anatoly Burakov > >> --- > >> doc/guides/rel_notes/release_18_11.rst | 7 +++++++ > >> lib/librte_eal/common/eal_common_memzone.c | 8 +++++--- > >> lib/librte_eal/common/malloc_heap.c | 2 +- > >> lib/librte_eal/common/rte_malloc.c | 4 ---- > >> 4 files changed, 13 insertions(+), 8 deletions(-) > >> > >> diff --git a/doc/guides/rel_notes/release_18_11.rst > >> b/doc/guides/rel_notes/release_18_11.rst > >> index 5fc71e208..6ee236302 100644 > >> --- a/doc/guides/rel_notes/release_18_11.rst > >> +++ b/doc/guides/rel_notes/release_18_11.rst > >> @@ -98,6 +98,13 @@ API Changes > >> users of memseg-walk-related functions, as they will now have to > skip > >> externally allocated segments in most cases if the intent is to > only > >> iterate > >> over internal DPDK memory. > >> + - ``socket_id`` parameter across the entire DPDK has gained > additional > >> + meaning, as some socket ID's will now be representing externally > >> allocated > >> + memory. No changes will be required for existing code as backwards > >> + compatibility will be kept, and those who do not use this feature > >> will not > >> + see these extra socket ID's. Any new API's must not check socket ID > >> + parameters themselves, and must instead leave it to the memory > >> subsystem to > >> + decide whether socket ID is a valid one. > >> > >> ABI Changes > >> ----------- > >> diff --git a/lib/librte_eal/common/eal_common_memzone.c > >> b/lib/librte_eal/common/eal_common_memzone.c > >> index 7300fe05d..b7081afbf 100644 > >> --- a/lib/librte_eal/common/eal_common_memzone.c > >> +++ b/lib/librte_eal/common/eal_common_memzone.c > >> @@ -120,13 +120,15 @@ memzone_reserve_aligned_thread_unsafe(const char > >> *name, size_t len, > >> return NULL; > >> } > >> > >> - if ((socket_id != SOCKET_ID_ANY) && > >> - (socket_id >= RTE_MAX_NUMA_NODES || socket_id < 0)) { > >> + if ((socket_id != SOCKET_ID_ANY) && socket_id < 0) { > >> > > > > Should not it be better to use RTE_MAX_HEAP instead of removing the > check? > > First of all, maximum number of heaps should not concern the rest of the > code - this is purely internal detail of rte_malloc. > > In a previous patch you say that: "Switch over all parts of EAL to use heap ID instead of NUMA node ID to identify heaps. Heap ID for DPDK-internal heaps is NUMA node's index within the detected NUMA node list. Heap ID for external heaps will be order of their creation." If I understand this right, heaps linked to physical sockets get a heap ID, and then external heaps will get IDs starting from the higher socket/heap ID + 1. So, assuming RTE_MAX_HEAPS is really the maximum number of allowed heaps (which does not seem so reading your next paragraph), it would be a good sanity check to use RTE_MAX_HEAPS for the socket id. More importantly, socket ID is completely independent from number of > heaps. Socket ID is incremented each time a new heap is created, and > they are not reused. If you create and destroy a heap 100 times - you'll > get 100 different socket ID's, even though max number of heaps is less > than that. > > I do not understand this. It is true there is no check regarding RTE_MAX_HEAPS when creating new heaps, then nor sure what the limit refers to. And then there is code like dumping heaps info or getting info from the heap based on socket id that will not work. > > > > > > > >> rte_errno = EINVAL; > >> return NULL; > >> } > >> > >> - if (!rte_eal_has_hugepages()) > >> + /* only set socket to SOCKET_ID_ANY if we aren't allocating for > an > >> + * external heap. > >> + */ > >> + if (!rte_eal_has_hugepages() && socket_id < RTE_MAX_NUMA_NODES) > >> socket_id = SOCKET_ID_ANY; > >> > >> contig = (flags & RTE_MEMZONE_IOVA_CONTIG) != 0; > >> diff --git a/lib/librte_eal/common/malloc_heap.c > >> b/lib/librte_eal/common/malloc_heap.c > >> index 1d1e35708..73e478076 100644 > >> --- a/lib/librte_eal/common/malloc_heap.c > >> +++ b/lib/librte_eal/common/malloc_heap.c > >> @@ -647,7 +647,7 @@ malloc_heap_alloc(const char *type, size_t size, int > >> socket_arg, > >> if (size == 0 || (align && !rte_is_power_of_2(align))) > >> return NULL; > >> > >> - if (!rte_eal_has_hugepages()) > >> + if (!rte_eal_has_hugepages() && socket_arg < RTE_MAX_NUMA_NODES) > >> socket_arg = SOCKET_ID_ANY; > >> > >> if (socket_arg == SOCKET_ID_ANY) > >> diff --git a/lib/librte_eal/common/rte_malloc.c > >> b/lib/librte_eal/common/rte_malloc.c > >> index 73d6df31d..9ba1472c3 100644 > >> --- a/lib/librte_eal/common/rte_malloc.c > >> +++ b/lib/librte_eal/common/rte_malloc.c > >> @@ -47,10 +47,6 @@ rte_malloc_socket(const char *type, size_t size, > >> unsigned int align, > >> if (!rte_eal_has_hugepages()) > >> socket_arg = SOCKET_ID_ANY; > >> > >> - /* Check socket parameter */ > >> - if (socket_arg >= RTE_MAX_NUMA_NODES) > >> - return NULL; > >> - > >> > > > > Sane than before. Better to keep the sanity check using RTE_MAX_HEAPS. > > same as above :) > > > -- > Thanks, > Anatoly >