From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 6382E282 for ; Mon, 8 Dec 2014 04:37:53 +0100 (CET) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga101.jf.intel.com with ESMTP; 07 Dec 2014 19:37:29 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.07,536,1413270000"; d="scan'208";a="650049654" Received: from pgsmsx106.gar.corp.intel.com ([10.221.44.98]) by orsmga002.jf.intel.com with ESMTP; 07 Dec 2014 19:37:28 -0800 Received: from pgsmsx103.gar.corp.intel.com (10.221.44.82) by pgsmsx106.gar.corp.intel.com (10.221.44.98) with Microsoft SMTP Server (TLS) id 14.3.195.1; Mon, 8 Dec 2014 11:37:26 +0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.110.14) by PGSMSX103.gar.corp.intel.com (10.221.44.82) with Microsoft SMTP Server (TLS) id 14.3.195.1; Mon, 8 Dec 2014 11:37:26 +0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.110]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.240]) with mapi id 14.03.0195.001; Mon, 8 Dec 2014 11:37:21 +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 03:37:19 +0000 Message-ID: <533710CFB86FA344BFBF2D6802E60286C9D78A@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> 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 03:37:58 -0000 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-l= imits]=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 indep= endent of=0A= >>>>> architecutre. While a system can't have a hugepage size that exceeds= 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_6= 4=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 architecutr= es, 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 me= mory 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 usi= ng the=0A= >>>> "correct" datatypes for objects is a more practical optino - however d= istastful=0A= >>>> I find it.=0A= >>> size_t isn't defined for memory sizes in the sense that we're using the= m here.=0A= >>> size_t is meant to address the need for a type to describe object sizes= 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 quest= ion 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 inappropria= te=0A= >>> because it as a type is not sized for anything other than the arch it i= s being=0A= >>> built for. The pragmatic benefits of ennumerating page sizes in a sing= le=0A= >>> canonical location far outweigh the desire to use a misappropriated typ= e 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 test= ing.=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 inclus= ion 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 attache= d the=0A= > proper fix below.=0A= >=0A= > Regards=0A= > Neil=0A= =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= 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= >=0A= > diff --git a/lib/librte_eal/common/eal_common_memory.c b/lib/librte_eal/c= ommon/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/co= mmon/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_eal= /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 memo= ry */=0A= > + uint64_t hugepage_sz; /**< The page size of underlying me= mory */=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/li= nuxapp/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=