From: "Burakov, Anatoly" <anatoly.burakov@intel.com>
To: "Stojaczyk, DariuszX" <dariuszx.stojaczyk@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>
Cc: "stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] memory: fix alignment in eal_get_virtual_area()
Date: Mon, 16 Jul 2018 15:16:46 +0100 [thread overview]
Message-ID: <b29da0ec-86c7-aa6c-5da3-669e90acefb6@intel.com> (raw)
In-Reply-To: <b2f1f227-223e-dbd1-4d6f-205c60e373fa@intel.com>
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?
--
Thanks,
Anatoly
next prev parent reply other threads:[~2018-07-16 14:16 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-13 19:08 Dariusz Stojaczyk
2018-06-14 7:29 ` Burakov, Anatoly
2018-07-12 21:52 ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
2018-07-16 12:58 ` [dpdk-dev] " Burakov, Anatoly
2018-07-16 13:29 ` Stojaczyk, DariuszX
2018-07-16 14:01 ` Burakov, Anatoly
2018-07-16 14:16 ` Burakov, Anatoly [this message]
2018-07-17 9:53 ` Stojaczyk, DariuszX
2018-07-17 9:22 ` Xu, Qian Q
2018-07-17 9:25 ` Burakov, Anatoly
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b29da0ec-86c7-aa6c-5da3-669e90acefb6@intel.com \
--to=anatoly.burakov@intel.com \
--cc=dariuszx.stojaczyk@intel.com \
--cc=dev@dpdk.org \
--cc=stable@dpdk.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).