From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id 4385C2C7A for ; Fri, 5 Dec 2014 16:24:31 +0100 (CET) Received: from hmsreliant.think-freely.org ([2001:470:8:a08:7aac:c0ff:fec2:933b] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63) (envelope-from ) id 1Xwujs-0001Kq-CM; Fri, 05 Dec 2014 10:24:25 -0500 Date: Fri, 5 Dec 2014 10:24:06 -0500 From: Neil Horman To: Bruce Richardson Message-ID: <20141205152406.GD29245@hmsreliant.think-freely.org> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20141205150233.GA9704@bricha3-MOBL3> User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Score: -2.9 (--) X-Spam-Status: No 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: Fri, 05 Dec 2014 15:24:31 -0000 On Fri, Dec 05, 2014 at 03:02:33PM +0000, Bruce Richardson wrote: > 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. size_t isn't defined for memory sizes in the sense that we're using them here. size_t is meant to address the need for a type to describe object sizes on a particular system, and it itself is sized accordingly (32 bits on a 32 bit arch, 64 on 64), so that you can safely store a size that the system in question might maximally allocate/return. In this situation we are describing memory sizes that might occur no a plurality of arches, and so size_t is inappropriate because it as a type is not sized for anything other than the arch it is being built for. The pragmatic benefits of ennumerating page sizes in a single canonical location far outweigh the desire to use a misappropriated type to describe them. Neil