From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailout1.w1.samsung.com (mailout1.w1.samsung.com [210.118.77.11]) by dpdk.org (Postfix) with ESMTP id BB1BA2B92 for ; Wed, 29 Aug 2018 15:00:47 +0200 (CEST) Received: from eucas1p2.samsung.com (unknown [182.198.249.207]) by mailout1.w1.samsung.com (KnoxPortal) with ESMTP id 20180829130046euoutp01c7295cd0b59a9b5531c0bbcafbea90a2~PXHLKUmBY1207712077euoutp01N for ; Wed, 29 Aug 2018 13:00:46 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout1.w1.samsung.com 20180829130046euoutp01c7295cd0b59a9b5531c0bbcafbea90a2~PXHLKUmBY1207712077euoutp01N DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1535547646; bh=NVuYpdWJbqLGXI7eEAYCPqsRYlGwCl9VtnPH48Y9ixM=; h=Subject:To:Cc:From:Date:In-Reply-To:References:From; b=SjpATL0MFC5Z9nCgfmO3yQPP+hsfto3oao6rseMWpkykQHHRvn9wTQZJrlZRaUJVQ rlo3LfHOmoBTqGB3D7tKXOFckAV/7L9dUuVMkvjAQhesACf/Ayxg0h6cDGBHBIFVSg oNy49IfyngiavIuX1Lrxy5FFUo9vZYVAMlaRmzPE= Received: from eusmges2new.samsung.com (unknown [203.254.199.244]) by eucas1p1.samsung.com (KnoxPortal) with ESMTP id 20180829130045eucas1p19dd2f3cb99435d015f7cf508731b61f1~PXHKbzB181863918639eucas1p1s; Wed, 29 Aug 2018 13:00:45 +0000 (GMT) Received: from eucas1p1.samsung.com ( [182.198.249.206]) by eusmges2new.samsung.com (EUCPMTA) with SMTP id CF.75.04294.CF8968B5; Wed, 29 Aug 2018 14:00:44 +0100 (BST) Received: from eusmtrp2.samsung.com (unknown [182.198.249.139]) by eucas1p1.samsung.com (KnoxPortal) with ESMTPA id 20180829130044eucas1p111b03c624b50c67a2ed0fe4aa038adda~PXHJmVLuc1480114801eucas1p1R; Wed, 29 Aug 2018 13:00:44 +0000 (GMT) Received: from eusmgms1.samsung.com (unknown [182.198.249.179]) by eusmtrp2.samsung.com (KnoxPortal) with ESMTP id 20180829130044eusmtrp2067a635bab266a6d27b1c057665cc83f~PXHJU3RA_1741617416eusmtrp2e; Wed, 29 Aug 2018 13:00:44 +0000 (GMT) X-AuditID: cbfec7f4-84fff700000010c6-0c-5b8698fcac9b Received: from eusmtip2.samsung.com ( [203.254.199.222]) by eusmgms1.samsung.com (EUCPMTA) with SMTP id E7.C1.04284.BF8968B5; Wed, 29 Aug 2018 14:00:43 +0100 (BST) Received: from [106.109.129.180] (unknown [106.109.129.180]) by eusmtip2.samsung.com (KnoxPortal) with ESMTPA id 20180829130043eusmtip28fa952076c5e637ee893f3b6154a8ca3~PXHI6OqRR0729707297eusmtip2e; Wed, 29 Aug 2018 13:00:43 +0000 (GMT) To: Anatoly Burakov , dev@dpdk.org Cc: solal.pirelli@gmail.com, stable@dpdk.org From: Ilya Maximets Date: Wed, 29 Aug 2018 16:02:07 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <2624b855f3454691212cdb244f04926631c391a2.1535544966.git.anatoly.burakov@intel.com> Content-Language: en-GB Content-Transfer-Encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprDKsWRmVeSWpSXmKPExsWy7djPc7p/ZrRFG7w8ambx6N5iZot3n7Yz WVxp/8lucWLqDBaLfx1/2B1YPX4tWMrqsXPWXXaPxXteMnn0bVnFGMASxWWTkpqTWZZapG+X wJVx5vxzxoLZyhU/l7exNzCelO5i5OSQEDCReHDzBHsXIxeHkMAKRokdi2axgCSEBL4wStx+ GwmR+MwocfzFIiaYjtlzFrJCJJYzSizbehKq/SOjxPcDs9lAqoQFbCU2Nu1kBrFFgOxP8++A dTML6EiseL6MEcRmA7JPrT4CZrMIqEqcXLARrEZUIELiyIOFYHFeAUGJkzOfgJ3EKZAgsfba UVaIOeISTV9WQtnyEtvfzmEGOUJCYDq7xKzTJ6CayyRuHT3GBnG2i8SGQ92MELawxKvjW9gh bBmJ/zvnQ71WL3G/5SUjxKAORonph/5BJewltrw+B9TAAbRNU2L9Ln2IsKPEmi/bmUDCEgJ8 EjfeCkLcwycxadt0Zogwr0RHmxBEtYrE74PLmSFsKYmb7z6zT2BUmoXky1lIPpuF5LNZCHsX MLKsYhRPLS3OTU8tNspLLdcrTswtLs1L10vOz93ECEwzp/8d/7KDcdefpEOMAhyMSjy8HMGt 0UKsiWXFlbmHGCU4mJVEeIMM2qKFeFMSK6tSi/Lji0pzUosPMUpzsCiJ8/JppUULCaQnlqRm p6YWpBbBZJk4OKUaGB0blQRrjG+UMutOT/vZeFTlnVOjQqqLL4c1Y5tp03r19ffWlbzxzukr n7Tcc/IBm4dv7A6eMdjKZ5VXqBtz9922eV1rlG4XilaXPDYu0Jv2/KiH1G7Fduf8QOVVDvn5 Cy64RRltr1OaUPdvy3N2Xo//U/bffjOTLdLP45WQteOTVeyic8JFlViKMxINtZiLihMBwoEk ui8DAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrPIsWRmVeSWpSXmKPExsVy+t/xe7q/Z7RFG/y/bG3x6N5iZot3n7Yz WVxp/8lucWLqDBaLfx1/2B1YPX4tWMrqsXPWXXaPxXteMnn0bVnFGMASpWdTlF9akqqQkV9c YqsUbWhhpGdoaaFnZGKpZ2hsHmtlZKqkb2eTkpqTWZZapG+XoJdx5vxzxoLZyhU/l7exNzCe lO5i5OSQEDCRmD1nIWsXIxeHkMBSRompW+6zQSSkJH78usAKYQtL/LnWxQZR9B6oaMYrsCJh AVuJjU07mUFsESD70/w7TCA2s4COxIrnyxhBbCGBzYwSK/t5QWw2oPip1UfA4rwCdhLnJ+0G W8AioCpxcsFGsF5RgQiJ1ctfsELUCEqcnPmEBcTmFEiQWHvtKCvEfHWJP/MuMUPY4hJNX1ZC xeUltr+dwzyBUWgWkvZZSFpmIWmZhaRlASPLKkaR1NLi3PTcYkO94sTc4tK8dL3k/NxNjMDI 2nbs5+YdjJc2Bh9iFOBgVOLhTQhtjRZiTSwrrsw9xCjBwawkwhtk0BYtxJuSWFmVWpQfX1Sa k1p8iNEU6LmJzFKiyfnAqM8riTc0NTS3sDQ0NzY3NrNQEuc9b1AZJSSQnliSmp2aWpBaBNPH xMEp1cDIujz/2VWrLZ+YLl5ts5r4/Yu/h1IGy1zNm88keib5uae8WPikePEl0bP/eBZPYWZ6 dt+xecenteZ3FZYv03z2oeDvViE7fc0jaq1PFmoJ7pm50ic8OnS2xTMHkZMCuTbT08M+vTh4 5o2H5VWdPyU1s6dbpG/9yybFoHFQPG9z7retfHmRcX9ylViKMxINtZiLihMBSqclN8ICAAA= Message-Id: <20180829130044eucas1p111b03c624b50c67a2ed0fe4aa038adda~PXHJmVLuc1480114801eucas1p1R@eucas1p1.samsung.com> X-CMS-MailID: 20180829130044eucas1p111b03c624b50c67a2ed0fe4aa038adda X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20180829122139epcas1p13ad45026365d788c072f2ed7a38349fb X-EPHeader: CA CMS-TYPE: 201P X-CMS-RootMailID: 20180829122139epcas1p13ad45026365d788c072f2ed7a38349fb References: <2624b855f3454691212cdb244f04926631c391a2.1535544966.git.anatoly.burakov@intel.com> Subject: Re: [dpdk-dev] [PATCH] mem: fix undefined behavior in NUMA code 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: Wed, 29 Aug 2018 13:00:47 -0000 Hi. Thanks for the fix. Comments inline. Best regards, Ilya Maximets. On 29.08.2018 15:21, Anatoly Burakov wrote: > When NUMA-aware hugepages config option is set, we rely on > libnuma to tell the kernel to allocate hugepages on a specific > NUMA node. However, we allocate node mask before we check if > NUMA is available in the first place, which, according to > the manpage [1], causes undefined behaviour. > > Fix by only using nodemask when we have NUMA available. > > [1] https://linux.die.net/man/3/numa_alloc_onnode > > Bugzilla ID: 20 > > Fixes: 1b72605d2416 ("mem: balanced allocation of hugepages") > Cc: i.maximets@samsung.com > Cc: stable@dpdk.org > > Signed-off-by: Anatoly Burakov > --- > lib/librte_eal/linuxapp/eal/eal_memory.c | 28 ++++++++++++++---------- > 1 file changed, 16 insertions(+), 12 deletions(-) > > diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c > index dbf19499e..4976eeacd 100644 > --- a/lib/librte_eal/linuxapp/eal/eal_memory.c > +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c > @@ -263,7 +263,7 @@ map_all_hugepages(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi, > int node_id = -1; > int essential_prev = 0; > int oldpolicy; > - struct bitmask *oldmask = numa_allocate_nodemask(); > + struct bitmask *oldmask = NULL; > bool have_numa = true; > unsigned long maxnode = 0; > > @@ -275,6 +275,7 @@ map_all_hugepages(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi, > > if (have_numa) { > RTE_LOG(DEBUG, EAL, "Trying to obtain current memory policy.\n"); > + oldmask = numa_allocate_nodemask(); > if (get_mempolicy(&oldpolicy, oldmask->maskp, > oldmask->size + 1, 0, 0) < 0) { > RTE_LOG(ERR, EAL, > @@ -390,19 +391,22 @@ map_all_hugepages(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi, > > out: > #ifdef RTE_EAL_NUMA_AWARE_HUGEPAGES > - if (maxnode) { > - RTE_LOG(DEBUG, EAL, > - "Restoring previous memory policy: %d\n", oldpolicy); > - if (oldpolicy == MPOL_DEFAULT) { > - numa_set_localalloc(); > - } else if (set_mempolicy(oldpolicy, oldmask->maskp, > - oldmask->size + 1) < 0) { > - RTE_LOG(ERR, EAL, "Failed to restore mempolicy: %s\n", > - strerror(errno)); > - numa_set_localalloc(); > + if (have_numa) { > + if (maxnode) { > + RTE_LOG(DEBUG, EAL, > + "Restoring previous memory policy: %d\n", > + oldpolicy); > + if (oldpolicy == MPOL_DEFAULT) { > + numa_set_localalloc(); > + } else if (set_mempolicy(oldpolicy, oldmask->maskp, > + oldmask->size + 1) < 0) { > + RTE_LOG(ERR, EAL, "Failed to restore mempolicy: %s\n", > + strerror(errno)); > + numa_set_localalloc(); > + } > } > + numa_free_cpumask(oldmask); > } > - numa_free_cpumask(oldmask); The original intend was to avoid ugly nested 'if's as possible. 'maxnode' is only initialized in NUMA case. So, there is no need to check for 'has_numa'. 'numa_free_cpumask' has 'free' semantics and checks for the argument. It is safe to call it with NULL. If you want to be fully compliant with man page, you may use less invasive change like this: --- diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c index dbf19499e..d0b9f3a2f 100644 --- a/lib/librte_eal/linuxapp/eal/eal_memory.c +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c @@ -390,7 +390,7 @@ map_all_hugepages(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi, out: #ifdef RTE_EAL_NUMA_AWARE_HUGEPAGES - if (maxnode) { + if (have_numa && maxnode) { RTE_LOG(DEBUG, EAL, "Restoring previous memory policy: %d\n", oldpolicy); if (oldpolicy == MPOL_DEFAULT) { @@ -402,7 +402,8 @@ map_all_hugepages(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi, numa_set_localalloc(); } } - numa_free_cpumask(oldmask); + if (oldmask) + numa_free_cpumask(oldmask); #endif return i; } --- But still, checking both 'have_numa && maxnode', IMHO, is unnecessary. As this change is cosmetic (issue doesn't produce any real bug), I'd like to avoid changing the functional code to something less readable. This also will complicate 'git blame' process. What do you think? > #endif > return i; > } >