From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 562F97F50 for ; Mon, 8 Dec 2014 16:01:05 +0100 (CET) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga102.jf.intel.com with ESMTP; 08 Dec 2014 06:58:50 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.07,539,1413270000"; d="scan'208";a="620437996" Received: from kmsmsx151.gar.corp.intel.com ([172.21.73.86]) by orsmga001.jf.intel.com with ESMTP; 08 Dec 2014 06:59:29 -0800 Received: from kmsmsx154.gar.corp.intel.com (172.21.73.14) by KMSMSX151.gar.corp.intel.com (172.21.73.86) with Microsoft SMTP Server (TLS) id 14.3.195.1; Mon, 8 Dec 2014 22:59:28 +0800 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by KMSMSX154.gar.corp.intel.com (172.21.73.14) with Microsoft SMTP Server (TLS) id 14.3.195.1; Mon, 8 Dec 2014 22:59:27 +0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.110]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.5]) with mapi id 14.03.0195.001; Mon, 8 Dec 2014 22:59:25 +0800 From: "Qiu, Michael" To: Neil Horman Thread-Topic: [dpdk-dev] [PATCH v3] Fix two compile issues with i686 platform Thread-Index: AQHQEFE2U0vIPmApvkOkCk8Kal0Dyw== Date: Mon, 8 Dec 2014 14:59:24 +0000 Message-ID: <533710CFB86FA344BFBF2D6802E60286C9DB66@SHSMSX101.ccr.corp.intel.com> References: <533710CFB86FA344BFBF2D6802E60286C9CB12@SHSMSX101.ccr.corp.intel.com> <1417684369-21330-1-git-send-email-michael.qiu@intel.com> <54816D73.1020906@linux.vnet.ibm.com> <20141205142205.GB29245@hmsreliant.think-freely.org> <20141205150233.GA9704@bricha3-MOBL3> <20141205152406.GD29245@hmsreliant.think-freely.org> <533710CFB86FA344BFBF2D6802E60286C9D736@SHSMSX101.ccr.corp.intel.com> <20141208025952.GA24461@localhost.localdomain> <533710CFB86FA344BFBF2D6802E60286C9D78A@SHSMSX101.ccr.corp.intel.com> <20141208113738.GA18697@hmsreliant.think-freely.org> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" , Michael Qiu Subject: Re: [dpdk-dev] [PATCH v3] Fix two compile issues with i686 platform X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 08 Dec 2014 15:01:06 -0000 On 2014/12/8 19:38, Neil Horman wrote:=0A= > On Mon, Dec 08, 2014 at 03:37:19AM +0000, Qiu, Michael wrote:=0A= >> On 12/8/2014 11:00 AM, Neil Horman wrote:=0A= >>> On Mon, Dec 08, 2014 at 02:46:51AM +0000, Qiu, Michael wrote:=0A= >>>> On 12/5/2014 11:25 PM, Neil Horman wrote:=0A= >>>>> On Fri, Dec 05, 2014 at 03:02:33PM +0000, Bruce Richardson wrote:=0A= >>>>>> On Fri, Dec 05, 2014 at 09:22:05AM -0500, Neil Horman wrote:=0A= >>>>>>> On Fri, Dec 05, 2014 at 04:31:47PM +0800, Chao Zhu wrote:=0A= >>>>>>>> On 2014/12/4 17:12, Michael Qiu wrote:=0A= >>>>>>>>> lib/librte_eal/linuxapp/eal/eal_memory.c:324:4: error: comparison= =0A= >>>>>>>>> is always false due to limited range of data type [-Werror=3Dtype= -limits]=0A= >>>>>>>>> || (hugepage_sz =3D=3D RTE_PGSIZE_16G)) {=0A= >>>>>>>>> ^=0A= >>>>>>>>> cc1: all warnings being treated as errors=0A= >>>>>>>>>=0A= >>>>>>>>> lib/librte_eal/linuxapp/eal/eal.c(461): error #2259: non-pointer= =0A= >>>>>>>>> conversion from "long long" to "void *" may lose significant bits= =0A= >>>>>>>>> RTE_PTR_ALIGN_CEIL((uintptr_t)addr, RTE_PGSIZE_16M);=0A= >>>>>>>>>=0A= >>>>>>>>> This was introuduced by commit b77b5639:=0A= >>>>>>>>> mem: add huge page sizes for IBM Power=0A= >>>>>>>>>=0A= >>>>>>>>> The root cause is that size_t and uintptr_t are 32-bit in i686=0A= >>>>>>>>> platform, but RTE_PGSIZE_16M and RTE_PGSIZE_16G are always 64-bit= .=0A= >>>>>>>>>=0A= >>>>>>>>> Define RTE_PGSIZE_16G only in 64 bit platform to avoid=0A= >>>>>>>>> this issue.=0A= >>>>>>>>>=0A= >>>>>>>>> Signed-off-by: Michael Qiu =0A= >>>>>>>>> ---=0A= >>>>>>>>> v3 ---> v2=0A= >>>>>>>>> Change RTE_PGSIZE_16G from ULL to UL=0A= >>>>>>>>> to keep all entries consistent=0A= >>>>>>>>>=0A= >>>>>>>>> V2 ---> v1=0A= >>>>>>>>> Change two type entries to one, and=0A= >>>>>>>>> leave RTE_PGSIZE_16G only valid for=0A= >>>>>>>>> 64-bit platform=0A= >>>>>>>>>=0A= >>>>>>> NACK, this is the wrong way to fix this problem. Pagesizes are ind= ependent of=0A= >>>>>>> architecutre. While a system can't have a hugepage size that excee= ds its=0A= >>>>>>> virtual address limit, theres no need to do per-architecture specia= l casing of=0A= >>>>>>> page sizes here. Instead of littering the code with ifdef RTE_ARCH= _64=0A= >>>>>>> everytime you want to check a page size, just convert the size_t to= a uint64_t=0A= >>>>>>> and you can allow all of the enumerated page types on all architecu= tres, and=0A= >>>>>>> save yourself some ifdeffing in the process.=0A= >>>>>>>=0A= >>>>>>> Neil=0A= >>>>>> While I get your point, I find it distasteful to use a uint64_t for = memory sizes,=0A= >>>>>> when there is the size_t type defined for that particular purpose.= =0A= >>>>>> However, I suppose that reducing the number of #ifdefs compared to u= sing the=0A= >>>>>> "correct" datatypes for objects is a more practical optino - however= distastful=0A= >>>>>> I find it.=0A= >>>>> size_t isn't defined for memory sizes in the sense that we're using t= hem here.=0A= >>>>> size_t is meant to address the need for a type to describe object siz= es on a=0A= >>>>> particular system, and it itself is sized accordingly (32 bits on a 3= 2 bit arch,=0A= >>>>> 64 on 64), so that you can safely store a size that the system in que= stion might=0A= >>>>> maximally allocate/return. In this situation we are describing memor= y sizes=0A= >>>>> that might occur no a plurality of arches, and so size_t is inappropr= iate=0A= >>>>> because it as a type is not sized for anything other than the arch it= is being=0A= >>>>> built for. The pragmatic benefits of ennumerating page sizes in a si= ngle=0A= >>>>> canonical location far outweigh the desire to use a misappropriated t= ype to=0A= >>>>> describe them.=0A= >>>> Neil,=0A= >>>>=0A= >>>> This patch fix two compile issues, and we need to do *dpdk testing=0A= >>>> affairs*, if it is blocked in build stage, we can do *nothing* for te= sting.=0A= >>>>=0A= >>>> I've get you mind and your concern. But we should take care of changin= g=0A= >>>> the type of "hugepage_sz", because lots of places using it.=0A= >>>>=0A= >>>> Would you mind if we consider this as hot fix, and we can post a bette= r=0A= >>>> fix later(like in dpdk 2.0)? Otherwise all test cycle are blocked.=0A= >>>>=0A= >>> Honestly, no. Because intels testing schedule shouldn't drive the incl= usion of=0A= >>> upstream fixes. Also, I'm not asking for a major redesign of anything,= I'm=0A= >>> asking for a proper fix for a very straightforward problem. I've attac= hed the=0A= >>> proper fix below.=0A= >>>=0A= >>> Regards=0A= >>> Neil=0A= >> We test dpdk upstream now as 1,8 rc2 and rc3 released :)=0A= >>=0A= > Yes, I don't take issue with you testing dpdk, on the contrary, I appreci= ate it.=0A= > What I take issue with is that you are asserting that the blockage of you= r=0A= > testing is reason to ignore a proper fix an issue, rather than some subst= andard=0A= > one.=0A= =0A= Agree :)=0A= =0A= >> I know that what you mean. but lots of please using "hugepage_sz" do you= =0A= >> confirm it will not affect other issue?=0A= >>=0A= > 5. There are 5 placees that use hugepage_sz, as the patch below indicate= s.=0A= > Thats no alot.=0A= >=0A= > Also, I take issue with the assertion that this patch creates no addition= al=0A= > problems. I'm sure it creates no additional problems that your patch wou= ldn't=0A= > also create, arguably less. If we were really being pragmatic here, I wo= uld=0A= > point out that this problem was caused by the fact that support for an en= tire=0A= > new architecture was integrated during the -rc phase of a release, which = seems=0A= > extreemely risky to me, and as such, the most appropriate thing to do wou= ld be=0A= > to back support for ppc out until after the 1.8 release when it could be= =0A= > properly tested. Instead we are slamming in hacked up fixes that throw a= rch=0A= > specific ifdefs througout the code.=0A= >=0A= >> On other hand, we use 32 bit address in 32 bit platform for better=0A= >> performance(some of places also use uintptr_t for address check and=0A= >> alignment).=0A= >>=0A= > This has nothing to do with address bus size.=0A= =0A= Actually, it does, this is one of what I'm fixed. But it also introduced=0A= by support Power Arch.=0A= =0A= Other places I have not check yet.=0A= =0A= Anyway, I will verify your solution, and to see any potential issues.=0A= =0A= Thanks=0A= Michael=0A= >> And it should not acceptable in 32 bit platform to use 64-bit platform= =0A= >> specification affairs(like RTE_PGSIZE_16G).=0A= >>=0A= > Ok, so add a single arch specific runtime check during hugepage mapping t= o exit=0A= > on the 16G size use on 32 bit systems. Thats a fair and reasonable thing= to do,=0A= > though I think the hugepage remap is already ifdeffed for 54 bit arches o= nly.=0A= >=0A= > Neil=0A= >=0A= >=0A= =0A=