From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wg0-f42.google.com (mail-wg0-f42.google.com [74.125.82.42]) by dpdk.org (Postfix) with ESMTP id C53C9282 for ; Mon, 8 Dec 2014 12:50:49 +0100 (CET) Received: by mail-wg0-f42.google.com with SMTP id z12so6021122wgg.15 for ; Mon, 08 Dec 2014 03:50:49 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:organization :user-agent:in-reply-to:references:mime-version :content-transfer-encoding:content-type; bh=8+P1IkZdRNgf2E+4OsshY58TT2qDnO3IeGiguH2yJEM=; b=V5Y0RvmkOWl1l4MRp6bt7wo5eKKUybnvoGRBgUUFHf/x8dSOapDy/dZ9gW+raA8hJw h8agnO7vlvMVwbj5roGBn9IAdNTEfL9y0ev7CIgTu6lr2arNqSajpV+Tz/KmiOlJJgGN 4NSLMwCbW25xGKGE6wu/GVvURm1pQGgDOi+r/GAmeK6bBdN+qtjI8XXGmiZu/Tagv4Fb 1ytmPOwUyPczFfd/EXznXJNulLdP+58o7xXCOJb3xcvmn7k37EEbqlQRmgvSTeFNShaS 5amypbwx5bjUwUs1oiRa433Snt7UIBliVh9HcyezjqhF4J2N0gUAEmsygtVpzPbi5KI2 18oQ== X-Gm-Message-State: ALoCoQkGJGoeb1zrvmk5jKAbuzZWiua0GauYnVhvOE9Ox8H1LeeBr81BGs7vyfW/U9VJV+EbTr4E X-Received: by 10.180.93.37 with SMTP id cr5mr23739309wib.76.1418039449629; Mon, 08 Dec 2014 03:50:49 -0800 (PST) Received: from xps13.localnet (136-92-190-109.dsl.ovh.fr. [109.190.92.136]) by mx.google.com with ESMTPSA id pu3sm56304883wjc.14.2014.12.08.03.50.48 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 08 Dec 2014 03:50:48 -0800 (PST) From: Thomas Monjalon To: Neil Horman Date: Mon, 08 Dec 2014 12:50:22 +0100 Message-ID: <6238111.4KRSYIhO5Y@xps13> Organization: 6WIND User-Agent: KMail/4.14.3 (Linux/3.17.4-1-ARCH; KDE/4.14.3; x86_64; ; ) In-Reply-To: <20141208113738.GA18697@hmsreliant.think-freely.org> References: <533710CFB86FA344BFBF2D6802E60286C9CB12@SHSMSX101.ccr.corp.intel.com> <533710CFB86FA344BFBF2D6802E60286C9D78A@SHSMSX101.ccr.corp.intel.com> <20141208113738.GA18697@hmsreliant.think-freely.org> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Cc: dev@dpdk.org 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: Mon, 08 Dec 2014 11:50:50 -0000 2014-12-08 06:37, Neil Horman: > 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 > > >>>>>>> --- > > >>>>>>> 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 agree. Neil's patch seems a lot better. > > 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, No, it was integrated between -rc1 and -rc2. But -rc1 was not really a release candidate. It was a first step after mbuf rework. This tag was requested for validation but it was a bad idea. We won't create such wrong tag anymore. PPC was integrated before the real RC phase. > 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. I think we can fix it (without ugly ifdefs) and avoid going back. Thanks for your help. -- Thomas > > 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