[dpdk-dev] [PATCH] memory: fix alignment in eal_get_virtual_area()

[Burakov, Anatoly]

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 <dariuszx.stojaczyk at intel.com>; dev at dpdk.org
>>> Cc: stable at 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 at intel.com
>>>> Cc: stable at dpdk.org
>>>>
>>>> Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk at intel.com>
>>>> ---
>>>>    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