From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 3B10D14EC; Mon, 16 Jul 2018 16:16:49 +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 fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Jul 2018 07:16:48 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,361,1526367600"; d="scan'208";a="240678801" 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:16:46 -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:16:46 +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: 8bit 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:16:50 -0000 On 16-Jul-18 3:01 PM, Burakov, Anatoly wrote: > 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 :) On reflection, I think i understand what you're getting at now, and that a different fix is required :) The issue at hand isn't whether the requested address is or isn't aligned - it's that we need to make sure we always get aligned address as a result. You have highlighted a case where we might ask for a page-aligned address, but end up getting a different one, but since we've set no_align to "true", we won't align the resulting "wrong" address. So it seems to me that the issue is, is there a possibility that we get an unaligned address? The answer lies in a different flag - addr_is_hint. That will tell us if we will discard the resulting address if we don't get what we want. So really, the only cases we should *not* align the resulting address are: 1) if page size is equal to that of system page size, or 2) if requested addr isn't NULL, *and* it's page aligned, *and* addr_is_hint is not true (i.e. we will discard the address if it's not the one we will get) In the second case, that "addr_is_hint" is our guarantee that we don't need to align address. So, resulting code should rather look like: no_align = (requested_addr != NULL && ((uintptr_t)requested_addr & (page_sz - 1)) && !addr_is_hint) || page_sz == system_page_sz; Makes sense? -- Thanks, Anatoly