memory: fix alignment in eal_get_virtual_area()
Checks
Commit Message
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(-)
Comments
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 {
>
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
14/06/2018 09:29, Burakov, Anatoly:
> 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>
>
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
Applied, thanks
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.
Thomas, could you please revert this patch?
> -----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?
Maybe there's additional case I'm not seeing, but this patch should not be reverted.
D.
>
> Thomas, could you please revert this patch?
>
> --
> Thanks,
> Anatoly
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.
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
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.
>
> 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?
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Burakov, Anatoly
> Sent: Monday, July 16, 2018 10:01 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 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.
>
> 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 :)
Yes, we have met many mulit-process related issues(hang, block) due to the patches,
so that RC1's quality is impacted by this patch seriously.
How about current fix plan? It's a little urgent. Thx.
>
> >
> > D.
> >
> >>
> >> Thomas, could you please revert this patch?
> >>
> >> --
> >> Thanks,
> >> Anatoly
>
>
> --
> Thanks,
> Anatoly
On 17-Jul-18 10:22 AM, Xu, Qian Q wrote:
>
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Burakov, Anatoly
>> Sent: Monday, July 16, 2018 10:01 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 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.
>>
>> 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 :)
>
> Yes, we have met many mulit-process related issues(hang, block) due to the patches,
> so that RC1's quality is impacted by this patch seriously.
> How about current fix plan? It's a little urgent. Thx.
Hi Qian,
I've sent a patch to fix this:
http://patches.dpdk.org/project/dpdk/list/?series=607
It was already tested by Lei, but you're welcome to pile on :)
> -----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
@@ -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 {