From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bruce Richardson Subject: Re: [PATCH v3] Fix two compile issues with i686 platform Date: Fri, 5 Dec 2014 15:02:33 +0000 Message-ID: <20141205150233.GA9704@bricha3-MOBL3> 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> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: dev-VfR2kkLFssw@public.gmane.org, Michael Qiu To: Neil Horman Return-path: Content-Disposition: inline In-Reply-To: <20141205142205.GB29245-B26myB8xz7F8NnZeBjwnZQMhkBWG/bsMQH7oEaQurus@public.gmane.org> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" On Fri, Dec 05, 2014 at 09:22:05AM -0500, Neil Horman wrote: > On Fri, Dec 05, 2014 at 04:31:47PM +0800, Chao Zhu wrote: > > > > On 2014/12/4 17:12, Michael Qiu wrote: > > >lib/librte_eal/linuxapp/eal/eal_memory.c:324:4: error: comparison > > >is always false due to limited range of data type [-Werror=type-limits] > > > || (hugepage_sz == RTE_PGSIZE_16G)) { > > > ^ > > >cc1: all warnings being treated as errors > > > > > >lib/librte_eal/linuxapp/eal/eal.c(461): error #2259: non-pointer > > >conversion from "long long" to "void *" may lose significant bits > > > RTE_PTR_ALIGN_CEIL((uintptr_t)addr, RTE_PGSIZE_16M); > > > > > >This was introuduced by commit b77b5639: > > > mem: add huge page sizes for IBM Power > > > > > >The root cause is that size_t and uintptr_t are 32-bit in i686 > > >platform, but RTE_PGSIZE_16M and RTE_PGSIZE_16G are always 64-bit. > > > > > >Define RTE_PGSIZE_16G only in 64 bit platform to avoid > > >this issue. > > > > > >Signed-off-by: Michael Qiu > > >--- > > > v3 ---> v2 > > > Change RTE_PGSIZE_16G from ULL to UL > > > to keep all entries consistent > > > > > > V2 ---> v1 > > > Change two type entries to one, and > > > leave RTE_PGSIZE_16G only valid for > > > 64-bit platform > > > > NACK, this is the wrong way to fix this problem. Pagesizes are independent of > architecutre. While a system can't have a hugepage size that exceeds its > virtual address limit, theres no need to do per-architecture special casing of > page sizes here. Instead of littering the code with ifdef RTE_ARCH_64 > everytime you want to check a page size, just convert the size_t to a uint64_t > and you can allow all of the enumerated page types on all architecutres, and > save yourself some ifdeffing in the process. > > Neil While I get your point, I find it distasteful to use a uint64_t for memory sizes, when there is the size_t type defined for that particular purpose. However, I suppose that reducing the number of #ifdefs compared to using the "correct" datatypes for objects is a more practical optino - however distastful I find it. /Bruce