From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id DE25B235; Mon, 16 Jul 2018 16:01:22 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Jul 2018 07:01:21 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,361,1526367600"; d="scan'208";a="240675045" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.237.220.102]) ([10.237.220.102]) by orsmga005.jf.intel.com with ESMTP; 16 Jul 2018 07:01:05 -0700 To: "Stojaczyk, DariuszX" , "dev@dpdk.org" Cc: "stable@dpdk.org" References: <1528916894-1991-1-git-send-email-dariuszx.stojaczyk@intel.com> From: "Burakov, Anatoly" Message-ID: Date: Mon, 16 Jul 2018 15:01:04 +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: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] memory: fix alignment in eal_get_virtual_area() 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: Mon, 16 Jul 2018 14:01:23 -0000 On 16-Jul-18 2:29 PM, Stojaczyk, DariuszX wrote: > >> -----Original Message----- >> From: Burakov, Anatoly >> Sent: Monday, July 16, 2018 2:58 PM >> To: Stojaczyk, DariuszX ; dev@dpdk.org >> Cc: stable@dpdk.org >> Subject: Re: [PATCH] memory: fix alignment in eal_get_virtual_area() >> >> On 13-Jun-18 8:08 PM, Dariusz Stojaczyk wrote: >>> Although the alignment mechanism works as intended, the >>> `no_align` bool flag was set incorrectly. We were aligning >>> buffers that didn't need extra alignment, and weren't >>> aligning ones that really needed it. >>> >>> Fixes: b7cc54187ea4 ("mem: move virtual area function in common directory") >>> Cc: anatoly.burakov@intel.com >>> Cc: stable@dpdk.org >>> >>> Signed-off-by: Dariusz Stojaczyk >>> --- >>> lib/librte_eal/common/eal_common_memory.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/lib/librte_eal/common/eal_common_memory.c >> b/lib/librte_eal/common/eal_common_memory.c >>> index 4f0688f..a7c89f0 100644 >>> --- a/lib/librte_eal/common/eal_common_memory.c >>> +++ b/lib/librte_eal/common/eal_common_memory.c >>> @@ -70,7 +70,7 @@ eal_get_virtual_area(void *requested_addr, size_t *size, >>> * system page size is the same as requested page size. >>> */ >>> no_align = (requested_addr != NULL && >>> - ((uintptr_t)requested_addr & (page_sz - 1)) == 0) || >>> + ((uintptr_t)requested_addr & (page_sz - 1))) || >>> page_sz == system_page_sz; >>> >>> do { >>> >> >> This patch is wrong - no alignment should be performed if address is >> already alighed, e.g. if requested_addr & (page_sz - 1) == 0. The >> original code was correct. > > If we provide an aligned address with ADDR_IS_HINT flag and OS decides not to use it, we end up with potentially unaligned address that needs to be manually aligned and that's what this patch does. If the requested address wasn't aligned to the provided page_sz, why would we bother aligning it manually? no_align is a flag that indicates whether we should or shouldn't align the resulting end address - it is not meant to align requested address. If requested_addr was NULL, no_align will be set to "false" (we don't know what we get, so we must reserve additional space for alignment purposes). However, it will be set to "true" if page size is equal to system size (the OS will return pointer that is already aligned to system page size, so we don't need to align the result and thus don't need to reserve additional space for alignment). If requested address wasn't null, again we don't need alignment if system page size is equal to requested page size, as any resulting address will be already page-aligned (hence no_align set to "true"). If requested address wasn't already page-aligned and page size is not equal to system page size, then we set "no_align" to false, because we will need to align the resulting address. The crucial part to understand is that the logic here is inverted - "if requested address isn't NULL, and if the requested address is already aligned (i.e. (addr & pgsz-1) == 0), then we *don't* need to align the address". So, if the requested address is *not* aligned, "no_align" must be set to false - because we *will* need to align the address. As an added bonus, we have regression testing identifying this patch as cause for numerous regressions :) > > D. > >> >> Thomas, could you please revert this patch? >> >> -- >> Thanks, >> Anatoly -- Thanks, Anatoly