From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id D1F953F9 for ; Mon, 8 Dec 2014 05:58:07 +0100 (CET) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga102.fm.intel.com with ESMTP; 07 Dec 2014 20:58:03 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.07,536,1413270000"; d="scan'208";a="634246874" Received: from kmsmsx152.gar.corp.intel.com ([172.21.73.87]) by fmsmga001.fm.intel.com with ESMTP; 07 Dec 2014 20:58:02 -0800 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by KMSMSX152.gar.corp.intel.com (172.21.73.87) with Microsoft SMTP Server (TLS) id 14.3.195.1; Mon, 8 Dec 2014 12:58:01 +0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.110]) by shsmsx102.ccr.corp.intel.com ([169.254.2.216]) with mapi id 14.03.0195.001; Mon, 8 Dec 2014 12:57:59 +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 04:57:58 +0000 Message-ID: <533710CFB86FA344BFBF2D6802E60286C9D7F9@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> 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 04:58:08 -0000 On 12/8/2014 11:39 AM, 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 inde= pendent of=0A= >>>>>> architecutre. While a system can't have a hugepage size that exceed= s its=0A= >>>>>> virtual address limit, theres no need to do per-architecture special= 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 architecut= res, 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 m= emory 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 us= ing 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 th= em here.=0A= >>>> size_t is meant to address the need for a type to describe object size= s on a=0A= >>>> particular system, and it itself is sized accordingly (32 bits on a 32= bit arch,=0A= >>>> 64 on 64), so that you can safely store a size that the system in ques= tion might=0A= >>>> maximally allocate/return. In this situation we are describing memory= sizes=0A= >>>> that might occur no a plurality of arches, and so size_t is inappropri= ate=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 sin= gle=0A= >>>> canonical location far outweigh the desire to use a misappropriated ty= pe 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 tes= ting.=0A= >>>=0A= >>> I've get you mind and your concern. But we should take care of changing= =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 better= =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 inclu= sion 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 attach= ed 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= > I know that what you mean. but lots of please using "hugepage_sz" do you= =0A= =0A= Sorry, please/places=0A= > confirm it will not affect other issue?=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= > And it should not acceptable in 32 bit platform to use 64-bit platform=0A= > specification affairs(like RTE_PGSIZE_16G).=0A= >=0A= > Thanks,=0A= > Michael=0A= >=0A= >>=0A= >> diff --git a/lib/librte_eal/common/eal_common_memory.c b/lib/librte_eal/= common/eal_common_memory.c=0A= >> index 412b432..31a391c 100644=0A= >> --- a/lib/librte_eal/common/eal_common_memory.c=0A= >> +++ b/lib/librte_eal/common/eal_common_memory.c=0A= >> @@ -96,7 +96,7 @@ rte_dump_physmem_layout(FILE *f)=0A= >> =0A= >> fprintf(f, "Segment %u: phys:0x%"PRIx64", len:%zu, "=0A= >> "virt:%p, socket_id:%"PRId32", "=0A= >> - "hugepage_sz:%zu, nchannel:%"PRIx32", "=0A= >> + "hugepage_sz:%llu, nchannel:%"PRIx32", "=0A= >> "nrank:%"PRIx32"\n", i,=0A= >> mcfg->memseg[i].phys_addr,=0A= >> mcfg->memseg[i].len,=0A= >> diff --git a/lib/librte_eal/common/eal_internal_cfg.h b/lib/librte_eal/c= ommon/eal_internal_cfg.h=0A= >> index aac6abf..e2ecb0d 100644=0A= >> --- a/lib/librte_eal/common/eal_internal_cfg.h=0A= >> +++ b/lib/librte_eal/common/eal_internal_cfg.h=0A= >> @@ -49,7 +49,7 @@=0A= >> * mount points of hugepages=0A= >> */=0A= >> struct hugepage_info {=0A= >> - size_t hugepage_sz; /**< size of a huge page */=0A= >> + uint64_t hugepage_sz; /**< size of a huge page */=0A= >> const char *hugedir; /**< dir where hugetlbfs is mounted */=0A= >> uint32_t num_pages[RTE_MAX_NUMA_NODES];=0A= >> /**< number of hugepages of that size on each socket */=0A= >> diff --git a/lib/librte_eal/common/include/rte_memory.h b/lib/librte_eal= /common/include/rte_memory.h=0A= >> index 1990833..7f8103f 100644=0A= >> --- a/lib/librte_eal/common/include/rte_memory.h=0A= >> +++ b/lib/librte_eal/common/include/rte_memory.h=0A= >> @@ -92,7 +92,7 @@ struct rte_memseg {=0A= >> phys_addr_t ioremap_addr; /**< Real physical address inside the VM */= =0A= >> #endif=0A= >> size_t len; /**< Length of the segment. */=0A= >> - size_t hugepage_sz; /**< The pagesize of underlying memory */=0A= >> + uint64_t hugepage_sz; /**< The pagesize of underlying memory */= =0A= >> int32_t socket_id; /**< NUMA socket ID. */=0A= >> uint32_t nchannel; /**< Number of channels. */=0A= >> uint32_t nrank; /**< Number of ranks. */=0A= >> diff --git a/lib/librte_eal/common/include/rte_memzone.h b/lib/librte_ea= l/common/include/rte_memzone.h=0A= >> index 7d47bff..3006e81 100644=0A= >> --- a/lib/librte_eal/common/include/rte_memzone.h=0A= >> +++ b/lib/librte_eal/common/include/rte_memzone.h=0A= >> @@ -83,7 +83,7 @@ struct rte_memzone {=0A= >> #endif=0A= >> size_t len; /**< Length of the memzone. */=0A= >> =0A= >> - size_t hugepage_sz; /**< The page size of underlying mem= ory */=0A= >> + uint64_t hugepage_sz; /**< The page size of underlying m= emory */=0A= >> =0A= >> int32_t socket_id; /**< NUMA socket ID. */=0A= >> =0A= >> diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/l= inuxapp/eal/eal_memory.c=0A= >> index e6cb919..bae2507 100644=0A= >> --- a/lib/librte_eal/linuxapp/eal/eal_memory.c=0A= >> +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c=0A= >> @@ -300,7 +300,7 @@ map_all_hugepages(struct hugepage_file *hugepg_tbl,= =0A= >> #endif=0A= >> =0A= >> for (i =3D 0; i < hpi->num_pages[0]; i++) {=0A= >> - size_t hugepage_sz =3D hpi->hugepage_sz;=0A= >> + uint64_t hugepage_sz =3D hpi->hugepage_sz;=0A= >> =0A= >> if (orig) {=0A= >> hugepg_tbl[i].file_id =3D i;=0A= >>=0A= >=0A= =0A=