DPDK patches and discussions
 help / color / mirror / Atom feed
From: Neil Horman <nhorman@tuxdriver.com>
To: "Qiu, Michael" <michael.qiu@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, Michael Qiu <qdy220091330@gmail.com>
Subject: Re: [dpdk-dev] [PATCH v3] Fix two compile issues with i686 platform
Date: Mon, 8 Dec 2014 06:37:38 -0500	[thread overview]
Message-ID: <20141208113738.GA18697@hmsreliant.think-freely.org> (raw)
In-Reply-To: <533710CFB86FA344BFBF2D6802E60286C9D78A@SHSMSX101.ccr.corp.intel.com>

On Mon, Dec 08, 2014 at 03:37:19AM +0000, Qiu, Michael wrote:
> On 12/8/2014 11:00 AM, Neil Horman wrote:
> > On Mon, Dec 08, 2014 at 02:46:51AM +0000, Qiu, Michael wrote:
> >> 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@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.
> >>
> > 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.

> 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 additional
problems.  I'm sure it creates no additional problems that your patch wouldn't
also create, arguably less.  If we were really being pragmatic here, I would
point out that this problem was caused by the fact that support for an entire
new architecture was integrated during the -rc phase of a release, which seems
extreemely risky to me, and as such, the most appropriate thing to do would be
to back support for ppc out until after the 1.8 release when it could be
properly tested.  Instead we are slamming in hacked up fixes that throw arch
specific ifdefs througout the code.

> 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).
> 
This has nothing to do with address bus size.

> And it should not acceptable in 32 bit platform to use 64-bit platform
> specification affairs(like RTE_PGSIZE_16G).
> 
Ok, so add a single arch specific runtime check during hugepage mapping to exit
on the 16G size use on 32 bit systems.  Thats a fair and reasonable thing to do,
though I think the hugepage remap is already ifdeffed for 54 bit arches only.

Neil

  parent reply	other threads:[~2014-12-08 11:38 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <1417329845-7482-1-git-send-email-michael.qiu@intel.com>
     [not found] ` <1417594223-2573-1-git-send-email-michael.qiu@intel.com>
2014-12-03 11:32   ` [dpdk-dev] [PATCH v2] " Qiu, Michael
2014-12-03 15:40   ` Bruce Richardson
2014-12-04  2:49     ` Qiu, Michael
2014-12-04  9:03       ` Thomas Monjalon
2014-12-04 10:21         ` Qiu, Michael
2014-12-04  9:12           ` [dpdk-dev] [PATCH v3] " Michael Qiu
2014-12-05  6:56             ` Qiu, Michael
2014-12-05  7:04               ` Chao Zhu
2014-12-05  8:31             ` Chao Zhu
2014-12-05 14:22               ` Neil Horman
2014-12-05 15:02                 ` Bruce Richardson
2014-12-05 15:24                   ` Neil Horman
2014-12-08  2:46                     ` Qiu, Michael
2014-12-08  2:59                       ` Neil Horman
2014-12-08  3:37                         ` Qiu, Michael
2014-12-08  4:57                           ` Qiu, Michael
2014-12-08 11:37                           ` Neil Horman [this message]
2014-12-08 11:50                             ` Thomas Monjalon
2014-12-08 14:59                             ` Qiu, Michael
2014-12-10 10:46             ` [dpdk-dev] [PATCH 0/2 v4] " Michael Qiu
2014-12-10 10:46               ` [dpdk-dev] [PATCH 1/2 v4] Fix compile issue with hugepage_sz in 32-bit system Michael Qiu
2014-12-10 10:46               ` [dpdk-dev] [PATCH 2/2] Fix compile issue of eal with icc compile Michael Qiu
2014-12-11  0:56               ` [dpdk-dev] [PATCH 0/2 v4] Fix two compile issues with i686 platform Thomas Monjalon
2014-12-11 13:25                 ` Neil Horman
2014-12-11 15:28                   ` Qiu, Michael
2014-12-11 21:21                     ` Thomas Monjalon
2014-12-12 11:38                       ` Neil Horman
2014-12-12 15:09                         ` Thomas Monjalon
2014-12-12 15:29                           ` Neil Horman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20141208113738.GA18697@hmsreliant.think-freely.org \
    --to=nhorman@tuxdriver.com \
    --cc=dev@dpdk.org \
    --cc=michael.qiu@intel.com \
    --cc=qdy220091330@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).