From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 6DA7C2BD3; Tue, 17 Jul 2018 11:25:28 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 Jul 2018 02:25:28 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,365,1526367600"; d="scan'208";a="73001744" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.237.220.102]) ([10.237.220.102]) by fmsmga001.fm.intel.com with ESMTP; 17 Jul 2018 02:25:27 -0700 To: "Xu, Qian Q" , "Stojaczyk, DariuszX" , "dev@dpdk.org" , Thomas Monjalon Cc: "stable@dpdk.org" References: <1528916894-1991-1-git-send-email-dariuszx.stojaczyk@intel.com> <82F45D86ADE5454A95A89742C8D1410E3BB55381@shsmsx102.ccr.corp.intel.com> From: "Burakov, Anatoly" Message-ID: <785e03ec-8def-857f-ac9e-04d902254586@intel.com> Date: Tue, 17 Jul 2018 10:25:26 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <82F45D86ADE5454A95A89742C8D1410E3BB55381@shsmsx102.ccr.corp.intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] memory: fix alignment in eal_get_virtual_area() X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Jul 2018 09:25:30 -0000 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 ; 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 ; 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 :) > > 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 :) -- Thanks, Anatoly