On 12/5/2014 11:25 PM, Neil Horman wrote: > 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 <michael.qiu at intel.com> >>>>> --- >>>>> 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, This patch fix two compile issues, and we need to do *dpdk testing affairs*, if it is blocked in build stage, we can do *nothing* for testing. I've get you mind and your concern. But we should take care of changing the type of "hugepage_sz", because lots of places using it. Would you mind if we consider this as hot fix, and we can post a better fix later(like in dpdk 2.0)? Otherwise all test cycle are blocked. Thanks, Michael > Neil > >