From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Burakov, Anatoly" Subject: Re: [PATCH] memory: fix alignment in eal_get_virtual_area() Date: Mon, 16 Jul 2018 15:16:46 +0100 Message-ID: References: <1528916894-1991-1-git-send-email-dariuszx.stojaczyk@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: "stable@dpdk.org" To: "Stojaczyk, DariuszX" , "dev@dpdk.org" Return-path: In-Reply-To: Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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