[dpdk-dev] [PATCH v3] Fix two compile issues with i686 platform
On 2014/12/8 19:38, Neil Horman wrote:
> On Mon, Dec 08, 2014 at 03:37:19AM +, Qiu, Michael wrote:
>> On 12/8/2014 11:00 AM, Neil Horman wrote:
>>> On Mon, Dec 08, 2014 at 02:46:51AM +, Qiu, Michael wrote:
On 12/5/2014 11:25 PM, Neil Horman wrote:
> On Fri, Dec 05, 2014 at 03:02:33PM +, 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,
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.
>>> Honestly, no. Because intels testing schedule shouldn't drive the
>>> inclusion of
>>> upstream fixes. Also, I'm not asking for a major redesign of anything, I'm
>>> asking for a proper fix for a very straightforward problem. I've attached
>>> the
>>> proper fix below.
>>>
>>> Regards
>>> Neil
>> We test dpdk upstream now as 1,8 rc2 and rc3 released :)
>>
> Yes, I don't take issue with you testing dpdk, on the contrary, I appreciate
> it.
> What I take issue with is that you are asserting that the blockage of your
> testing is reason to ignore a proper fix an issue, rather than some
> substandard
> one.
Agree :)
>> I know that what you mean. but lots of please using "hugepage_sz" do you
>> confirm it will not affect other issue?
>>
> 5. There are 5 placees that use hugepage_sz, as the patch below indicates.
> Thats no alot.
>
> Also, I take issue with the assertion that this patch creates no
[dpdk-dev] [PATCH v3] Fix two compile issues with i686 platform
On 12/8/2014 11:39 AM, Qiu, Michael wrote:
> On 12/8/2014 11:00 AM, Neil Horman wrote:
>> On Mon, Dec 08, 2014 at 02:46:51AM +, Qiu, Michael wrote:
>>> On 12/5/2014 11:25 PM, Neil Horman wrote:
On Fri, Dec 05, 2014 at 03:02:33PM +, 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,
>>>
>>> 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.
>>>
>> Honestly, no. Because intels testing schedule shouldn't drive the inclusion
>> of
>> upstream fixes. Also, I'm not asking for a major redesign of anything, I'm
>> asking for a proper fix for a very straightforward problem. I've attached
>> the
>> proper fix below.
>>
>> Regards
>> Neil
> We test dpdk upstream now as 1,8 rc2 and rc3 released :)
>
> I know that what you mean. but lots of please using "hugepage_sz" do you
Sorry, please/places
> confirm it will not affect other issue?
>
> On other hand, we use 32 bit address in 32 bit platform for better
> performance(some of places also use uintptr_t for address check and
> alignment).
>
> And it should not acceptable in 32 bit platform to use 64-bit platform
> specification affairs(like RTE_PGSIZE_16G).
>
> Thanks,
> Michael
>
>>
>> diff --git a/lib/librte_eal/common/eal_common_memory.c
>> b/lib/librte_eal/common/eal_common_memory.c
>> index 412b432..31a391c 100644
>> --- a/lib/librte_eal/common/eal_common_memory.c
>> +++ b/lib/librte_eal/common/eal_common_memory.c
>> @@ -96,7 +96,7 @@ rte_dump_physmem_layout(FILE *f)
>>
[dpdk-dev] [PATCH v3] Fix two compile issues with i686 platform
On 12/8/2014 11:00 AM, Neil Horman wrote:
> On Mon, Dec 08, 2014 at 02:46:51AM +, Qiu, Michael wrote:
>> On 12/5/2014 11:25 PM, Neil Horman wrote:
>>> On Fri, Dec 05, 2014 at 03:02:33PM +, 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,
>>
>> 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.
>>
> Honestly, no. Because intels testing schedule shouldn't drive the inclusion
> of
> upstream fixes. Also, I'm not asking for a major redesign of anything, I'm
> asking for a proper fix for a very straightforward problem. I've attached the
> proper fix below.
>
> Regards
> Neil
We test dpdk upstream now as 1,8 rc2 and rc3 released :)
I know that what you mean. but lots of please using "hugepage_sz" do you
confirm it will not affect other issue?
On other hand, we use 32 bit address in 32 bit platform for better
performance(some of places also use uintptr_t for address check and
alignment).
And it should not acceptable in 32 bit platform to use 64-bit platform
specification affairs(like RTE_PGSIZE_16G).
Thanks,
Michael
>
>
> diff --git a/lib/librte_eal/common/eal_common_memory.c
> b/lib/librte_eal/common/eal_common_memory.c
> index 412b432..31a391c 100644
> --- a/lib/librte_eal/common/eal_common_memory.c
> +++ b/lib/librte_eal/common/eal_common_memory.c
> @@ -96,7 +96,7 @@ rte_dump_physmem_layout(FILE *f)
>
> fprintf(f, "Segment %u: phys:0x%"PRIx64", len:%zu, "
> "virt:%p, socket_id:%"PRId32", "
> -"hugepage_sz:%zu, nchannel:%"PRIx32", "
> +
[dpdk-dev] [PATCH v3] Fix two compile issues with i686 platform
On 12/5/2014 11:25 PM, Neil Horman wrote:
> On Fri, Dec 05, 2014 at 03:02:33PM +, 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,
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
>
>
[dpdk-dev] [PATCH v3] Fix two compile issues with i686 platform
On Mon, Dec 08, 2014 at 02:46:51AM +, Qiu, Michael wrote:
> On 12/5/2014 11:25 PM, Neil Horman wrote:
> > On Fri, Dec 05, 2014 at 03:02:33PM +, 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,
>
> 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.
>
Honestly, no. Because intels testing schedule shouldn't drive the inclusion of
upstream fixes. Also, I'm not asking for a major redesign of anything, I'm
asking for a proper fix for a very straightforward problem. I've attached the
proper fix below.
Regards
Neil
diff --git a/lib/librte_eal/common/eal_common_memory.c
b/lib/librte_eal/common/eal_common_memory.c
index 412b432..31a391c 100644
--- a/lib/librte_eal/common/eal_common_memory.c
+++ b/lib/librte_eal/common/eal_common_memory.c
@@ -96,7 +96,7 @@ rte_dump_physmem_layout(FILE *f)
fprintf(f, "Segment %u: phys:0x%"PRIx64", len:%zu, "
"virt:%p, socket_id:%"PRId32", "
- "hugepage_sz:%zu, nchannel:%"PRIx32", "
+ "hugepage_sz:%llu, nchannel:%"PRIx32", "
"nrank:%"PRIx32"\n", i,
mcfg->memseg[i].phys_addr,
mcfg->memseg[i].len,
diff --git a/lib/librte_eal/common/eal_internal_cfg.h
b/lib/librte_eal/common/eal_internal_cfg.h
index aac6abf..e2ecb0d 100644
--- a/lib/librte_eal/common/eal_internal_cfg.h
+++ b/lib/librte_eal/common/eal_internal_cfg.h
@@ -49,7 +49,7 @@
* mount points of hugepages
*/
struct hugepage_info {
- size_t hugepage_sz; /**< size
[dpdk-dev] [PATCH v3] Fix two compile issues with i686 platform
Michael,
I'm looking at it. I'll give you feedback soon.
On 2014/12/5 14:56, Qiu, Michael wrote:
> Hi Chao
>
> Would you please take a look at this patch?
>
> It's solved issue introduce by Power Arch support patch.
>
> Your comments are very precious :)
>
> Thanks,
> Michael
> On 12/5/2014 2:03 PM, 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
>>
>> app/test/test_memzone.c| 18 --
>> lib/librte_eal/common/eal_common_memzone.c | 2 ++
>> lib/librte_eal/common/include/rte_memory.h | 14 --
>> lib/librte_eal/linuxapp/eal/eal_memory.c | 12 +---
>> 4 files changed, 27 insertions(+), 19 deletions(-)
>>
>> diff --git a/app/test/test_memzone.c b/app/test/test_memzone.c
>> index 5da6903..7bab8b5 100644
>> --- a/app/test/test_memzone.c
>> +++ b/app/test/test_memzone.c
>> @@ -145,8 +145,10 @@ test_memzone_reserve_flags(void)
>> hugepage_1GB_avail = 1;
>> if (ms[i].hugepage_sz == RTE_PGSIZE_16M)
>> hugepage_16MB_avail = 1;
>> +#ifdef RTE_ARCH_64
>> if (ms[i].hugepage_sz == RTE_PGSIZE_16G)
>> hugepage_16GB_avail = 1;
>> +#endif
>> }
>> /* Display the availability of 2MB ,1GB, 16MB, 16GB pages */
>> if (hugepage_2MB_avail)
>> @@ -234,8 +236,8 @@ test_memzone_reserve_flags(void)
>> return -1;
>> }
>>
>> -/* Check if 1GB huge pages are unavailable, that function fails
>> unless
>> - * HINT flag is indicated
>> +/* Check if 2MB huge pages are unavailable, that function
>> + * fails unless HINT flag is indicated
>> */
>> if (!hugepage_2MB_avail) {
>> mz = rte_memzone_reserve("flag_zone_2M_HINT", size,
>> SOCKET_ID_ANY,
>> @@ -295,8 +297,9 @@ test_memzone_reserve_flags(void)
>> return -1;
>> }
>>
>> -/* Check if 1GB huge pages are unavailable, that function fails
>> - * unless HINT flag is indicated
>> +#ifdef RTE_ARCH_64
>> +/* Check if 16GB huge pages are unavailable, that function
>> + * fails unless HINT flag is indicated
>> */
>> if (!hugepage_16GB_avail) {
>> mz = rte_memzone_reserve("flag_zone_16G_HINT", size,
>> @@ -318,7 +321,9 @@ test_memzone_reserve_flags(void)
>> return -1;
>> }
>> }
>> +#endif
>> }
>> +#ifdef RTE_ARCH_64
>> /*As with 16MB tests above for 16GB huge page requests*/
>> if (hugepage_16GB_avail) {
>> mz = rte_memzone_reserve("flag_zone_16G", size, SOCKET_ID_ANY,
>> @@ -343,8 +348,8 @@ test_memzone_reserve_flags(void)
>> return -1;
>> }
>>
>> -/* Check if 1GB huge pages are unavailable, that function fails
>> - * unless HINT flag is indicated
>> +/* Check if 16MB huge pages are unavailable, that function
>> + * fails unless HINT flag is indicated
>> */
>> if (!hugepage_16MB_avail) {
>> mz = rte_memzone_reserve("flag_zone_16M_HINT", size,
>> @@ -376,6 +381,7 @@ test_memzone_reserve_flags(void)
>> }
>> }
>> }
>> +#endif
>> return 0;
>> }
>>
>> diff --git a/lib/librte_eal/common/eal_common_memzone.c
>> b/lib/librte_eal/common/eal_common_memzone.c
>> index b5a5d72..ee233ad 100644
>> --- a/lib/librte_eal/common/eal_common_memzone.c
>> +++ b/lib/librte_eal/common/eal_common_memzone.c
>> @@ -221,12 +221,14 @@ memzone_reserve_aligned_thread_unsafe(const char
>> *name, size_t len,
>> if ((flags & RTE_MEMZONE_1GB) &&
>> free_memseg[i].hugepage_sz == RTE_PGSIZE_2M)
>> continue;
>> +#ifdef RTE_ARCH_64
>> if ((flags & RTE_MEMZONE_16MB) &&
>>
[dpdk-dev] [PATCH v3] Fix two compile issues with i686 platform
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
[dpdk-dev] [PATCH v3] Fix two compile issues with i686 platform
On Fri, Dec 05, 2014 at 03:02:33PM +, 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
[dpdk-dev] [PATCH v3] Fix two compile issues with i686 platform
Hi Chao
Would you please take a look at this patch?
It's solved issue introduce by Power Arch support patch.
Your comments are very precious :)
Thanks,
Michael
On 12/5/2014 2:03 PM, 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
>
> app/test/test_memzone.c| 18 --
> lib/librte_eal/common/eal_common_memzone.c | 2 ++
> lib/librte_eal/common/include/rte_memory.h | 14 --
> lib/librte_eal/linuxapp/eal/eal_memory.c | 12 +---
> 4 files changed, 27 insertions(+), 19 deletions(-)
>
> diff --git a/app/test/test_memzone.c b/app/test/test_memzone.c
> index 5da6903..7bab8b5 100644
> --- a/app/test/test_memzone.c
> +++ b/app/test/test_memzone.c
> @@ -145,8 +145,10 @@ test_memzone_reserve_flags(void)
> hugepage_1GB_avail = 1;
> if (ms[i].hugepage_sz == RTE_PGSIZE_16M)
> hugepage_16MB_avail = 1;
> +#ifdef RTE_ARCH_64
> if (ms[i].hugepage_sz == RTE_PGSIZE_16G)
> hugepage_16GB_avail = 1;
> +#endif
> }
> /* Display the availability of 2MB ,1GB, 16MB, 16GB pages */
> if (hugepage_2MB_avail)
> @@ -234,8 +236,8 @@ test_memzone_reserve_flags(void)
> return -1;
> }
>
> - /* Check if 1GB huge pages are unavailable, that function fails
> unless
> - * HINT flag is indicated
> + /* Check if 2MB huge pages are unavailable, that function
> + * fails unless HINT flag is indicated
>*/
> if (!hugepage_2MB_avail) {
> mz = rte_memzone_reserve("flag_zone_2M_HINT", size,
> SOCKET_ID_ANY,
> @@ -295,8 +297,9 @@ test_memzone_reserve_flags(void)
> return -1;
> }
>
> - /* Check if 1GB huge pages are unavailable, that function fails
> - * unless HINT flag is indicated
> +#ifdef RTE_ARCH_64
> + /* Check if 16GB huge pages are unavailable, that function
> + * fails unless HINT flag is indicated
>*/
> if (!hugepage_16GB_avail) {
> mz = rte_memzone_reserve("flag_zone_16G_HINT", size,
> @@ -318,7 +321,9 @@ test_memzone_reserve_flags(void)
> return -1;
> }
> }
> +#endif
> }
> +#ifdef RTE_ARCH_64
> /*As with 16MB tests above for 16GB huge page requests*/
> if (hugepage_16GB_avail) {
> mz = rte_memzone_reserve("flag_zone_16G", size, SOCKET_ID_ANY,
> @@ -343,8 +348,8 @@ test_memzone_reserve_flags(void)
> return -1;
> }
>
> - /* Check if 1GB huge pages are unavailable, that function fails
> - * unless HINT flag is indicated
> + /* Check if 16MB huge pages are unavailable, that function
> + * fails unless HINT flag is indicated
>*/
> if (!hugepage_16MB_avail) {
> mz = rte_memzone_reserve("flag_zone_16M_HINT", size,
> @@ -376,6 +381,7 @@ test_memzone_reserve_flags(void)
> }
> }
> }
> +#endif
> return 0;
> }
>
> diff --git a/lib/librte_eal/common/eal_common_memzone.c
> b/lib/librte_eal/common/eal_common_memzone.c
> index b5a5d72..ee233ad 100644
> --- a/lib/librte_eal/common/eal_common_memzone.c
> +++ b/lib/librte_eal/common/eal_common_memzone.c
> @@ -221,12 +221,14 @@ memzone_reserve_aligned_thread_unsafe(const char *name,
> size_t len,
> if ((flags & RTE_MEMZONE_1GB) &&
> free_memseg[i].hugepage_sz == RTE_PGSIZE_2M)
> continue;
> +#ifdef RTE_ARCH_64
> if ((flags & RTE_MEMZONE_16MB) &&
> free_memseg[i].hugepage_sz == RTE_PGSIZE_16G)
> continue;
> if ((flags & RTE_MEMZONE_16GB) &&
>
[dpdk-dev] [PATCH v3] Fix two compile issues with i686 platform
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
app/test/test_memzone.c| 18 --
lib/librte_eal/common/eal_common_memzone.c | 2 ++
lib/librte_eal/common/include/rte_memory.h | 14 --
lib/librte_eal/linuxapp/eal/eal_memory.c | 12 +---
4 files changed, 27 insertions(+), 19 deletions(-)
diff --git a/app/test/test_memzone.c b/app/test/test_memzone.c
index 5da6903..7bab8b5 100644
--- a/app/test/test_memzone.c
+++ b/app/test/test_memzone.c
@@ -145,8 +145,10 @@ test_memzone_reserve_flags(void)
hugepage_1GB_avail = 1;
if (ms[i].hugepage_sz == RTE_PGSIZE_16M)
hugepage_16MB_avail = 1;
+#ifdef RTE_ARCH_64
if (ms[i].hugepage_sz == RTE_PGSIZE_16G)
hugepage_16GB_avail = 1;
+#endif
}
/* Display the availability of 2MB ,1GB, 16MB, 16GB pages */
if (hugepage_2MB_avail)
@@ -234,8 +236,8 @@ test_memzone_reserve_flags(void)
return -1;
}
- /* Check if 1GB huge pages are unavailable, that function fails
unless
-* HINT flag is indicated
+ /* Check if 2MB huge pages are unavailable, that function
+* fails unless HINT flag is indicated
*/
if (!hugepage_2MB_avail) {
mz = rte_memzone_reserve("flag_zone_2M_HINT", size,
SOCKET_ID_ANY,
@@ -295,8 +297,9 @@ test_memzone_reserve_flags(void)
return -1;
}
- /* Check if 1GB huge pages are unavailable, that function fails
-* unless HINT flag is indicated
+#ifdef RTE_ARCH_64
+ /* Check if 16GB huge pages are unavailable, that function
+* fails unless HINT flag is indicated
*/
if (!hugepage_16GB_avail) {
mz = rte_memzone_reserve("flag_zone_16G_HINT", size,
@@ -318,7 +321,9 @@ test_memzone_reserve_flags(void)
return -1;
}
}
+#endif
}
+#ifdef RTE_ARCH_64
/*As with 16MB tests above for 16GB huge page requests*/
if (hugepage_16GB_avail) {
mz = rte_memzone_reserve("flag_zone_16G", size, SOCKET_ID_ANY,
@@ -343,8 +348,8 @@ test_memzone_reserve_flags(void)
return -1;
}
- /* Check if 1GB huge pages are unavailable, that function fails
-* unless HINT flag is indicated
+ /* Check if 16MB huge pages are unavailable, that function
+* fails unless HINT flag is indicated
*/
if (!hugepage_16MB_avail) {
mz = rte_memzone_reserve("flag_zone_16M_HINT", size,
@@ -376,6 +381,7 @@ test_memzone_reserve_flags(void)
}
}
}
+#endif
return 0;
}
diff --git a/lib/librte_eal/common/eal_common_memzone.c
b/lib/librte_eal/common/eal_common_memzone.c
index b5a5d72..ee233ad 100644
--- a/lib/librte_eal/common/eal_common_memzone.c
+++ b/lib/librte_eal/common/eal_common_memzone.c
@@ -221,12 +221,14 @@ memzone_reserve_aligned_thread_unsafe(const char *name,
size_t len,
if ((flags & RTE_MEMZONE_1GB) &&
free_memseg[i].hugepage_sz == RTE_PGSIZE_2M)
continue;
+#ifdef RTE_ARCH_64
if ((flags & RTE_MEMZONE_16MB) &&
free_memseg[i].hugepage_sz == RTE_PGSIZE_16G)
continue;
if ((flags & RTE_MEMZONE_16GB) &&
free_memseg[i].hugepage_sz == RTE_PGSIZE_16M)
continue;
+#endif
/* this segment is the best until now */
if (memseg_idx == -1) {
diff --git a/lib/librte_eal/common/include/rte_memory.h
b/lib/librte_eal/common/include/rte_memory.h
index 1990833..6bcb92b 100644
---
