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

["Stojaczyk, DariuszX" <dariuszx.stojaczyk@intel.com>]

> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Monday, July 16, 2018 4:17 PM
> To: Stojaczyk, DariuszX <dariuszx.stojaczyk@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] memory: fix alignment in eal_get_virtual_area()
> 
> 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@intel.com>; 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 <dariuszx.stojaczyk@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.

I haven't seen such use case in the code and I deliberately didn't handle it. I believe that was my problem.

> >
> > 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.

That's correct.

> 
> 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)

That's correct.

> 
> 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?

I think you forgot "== 0" at the page alignment check. Otherwise we won't align any misaligned requested address. But the separate patch you sent got it right.

Thanks,
D.

> 
> --
> Thanks,
> Anatoly