From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from e28smtp04.in.ibm.com (e28smtp04.in.ibm.com [122.248.162.4]) by dpdk.org (Postfix) with ESMTP id 38DE07DF4 for ; Fri, 5 Dec 2014 10:11:25 +0100 (CET) Received: from /spool/local by e28smtp04.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 5 Dec 2014 14:41:22 +0530 Received: from d28dlp02.in.ibm.com (9.184.220.127) by e28smtp04.in.ibm.com (192.168.1.134) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Fri, 5 Dec 2014 14:41:19 +0530 Received: from d28relay04.in.ibm.com (d28relay04.in.ibm.com [9.184.220.61]) by d28dlp02.in.ibm.com (Postfix) with ESMTP id 8346C3940049 for ; Fri, 5 Dec 2014 14:41:18 +0530 (IST) Received: from d28av01.in.ibm.com (d28av01.in.ibm.com [9.184.220.63]) by d28relay04.in.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id sB59BoZ361931684 for ; Fri, 5 Dec 2014 14:41:51 +0530 Received: from d28av01.in.ibm.com (localhost [127.0.0.1]) by d28av01.in.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id sB59BEqm027394 for ; Fri, 5 Dec 2014 14:41:14 +0530 Received: from [127.0.0.1] ([9.186.104.147]) by d28av01.in.ibm.com (8.14.4/8.14.4/NCO v10.0 AVin) with ESMTP id sB59BBGF027133; Fri, 5 Dec 2014 14:41:13 +0530 Message-ID: <548176B1.8040100@linux.vnet.ibm.com> Date: Fri, 05 Dec 2014 17:11:13 +0800 From: Chao Zhu User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-Version: 1.0 To: Neil Horman , Thomas Monjalon References: <1417688048-23076-1-git-send-email-chaozhu@linux.vnet.ibm.com> <1795169.cqFrYtuj77@xps13> <20141204153256.GE16249@hmsreliant.think-freely.org> <6950163.rWPXMLohSt@xps13> <20141204200538.GB18930@hmsreliant.think-freely.org> In-Reply-To: <20141204200538.GB18930@hmsreliant.think-freely.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 14120509-0013-0000-0000-000002A06C22 Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH] Fix KNI compiling issue on IBM Power 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 09:11:27 -0000 On 2014/12/5 4:05, Neil Horman wrote: > On Thu, Dec 04, 2014 at 04:59:59PM +0100, Thomas Monjalon wrote: >> 2014-12-04 10:32, Neil Horman: >>> On Thu, Dec 04, 2014 at 02:47:03PM +0100, Thomas Monjalon wrote: >>>> 2014-12-04 08:29, Neil Horman: >>>>> On Thu, Dec 04, 2014 at 12:59:31PM +0100, Thomas Monjalon wrote: >>>>>>> Because of different cache line size, the alignment of struct >>>>>>> rte_kni_mbuf in rte_kni_common.h doesn't work on IBM Power. This patch >>>>>>> changed from 64 to RTE_CACHE_LINE_SIZE micro to do the alignment. >>>>>>> >>>>>>> Signed-off-by: Chao Zhu >>>>>> Acked-by: Thomas Monjalon >>>>>> >>>>>> Applied >>>>>> >>>>> Woah! Slow down here, I'm not sure if this makes sense to fix his way. The >>>>> exact same ifndef/define/endif construct is used for this macro in rte_memory.h. >>>>> Currently their defined to the same vaule, but if that ever changes, this macro >>>>> will return different values based on the order in which header files are >>>>> included. That doesn't seem appropriate at all. >>>> I agree (was my comment) but the patch was applied as a hot fix. >>>> A better fix has to be found for DPDK 2.0. >>>> Do you agree this fix is enough for DPDK 1.8 release? >>>> >>> I really don't like the idea of hacks like this being used. >> It's not really a hack to replace a hardcoded value by a constant. >> I think you should agree it's better (but not perfect). >> > I'm not referring to replacing a hardcoded value with a constant macro. The > hack I'm referring to is that of defining that macro in multiple places using > the ifndef/define/endif construct. Generally its fine to use that mechanism to > define a macro if you want to allow for builds to override it on the command > line or some such, but you've got the same construct in multiple header files > with this patch, which in turn leads to the possibility of the definition > location changing dependent on which header file is included first in a > compilation unit. Thats the hack. I agree. It's better to have one definition for all the use. Actually, the RTE_CACHE_LINE_SIZE macro was defined in many places, such as rte_acl_osdep_alone.h and rte_memory.h. Of cause, we can have it defined in some common place. If needed, I can do it. However, I do prefer we can have a build system do detect and make a global configuration header file. >>> Truthfully, I would rather the KNI just not be built on power for now, >>> it is after all a new feature for which not everything works yet (e.g. the >>> acl library and the ixgbe rxtx vec code). >>> With this in place, KNI will build now, but it means that anything >>> changes cache line sizes until it gets fixed properly runs the risk of >>> introducing wierd behavioral issues at compile time. >> It was also the case before: 64 was hardcoded for KNI. >> > See above, not concerned with the hardcoded vs macro idea, just how the macro is > implemented. > >>> I'm also concerned about the fact that, since we have no bug tracker for DPDK, >>> indicating that there will be an improved fix in 2.0 isn't really a guarantee, >>> in that it requires that someone remember to do it. >> Please be confident that I keep it noted and I'll do what I can to have it >> properly fixed. >> By the way, submitting a fix now would store the need in patchwork. >> > Yes, of course it would fix the problem, all problems could be fixed now if we > could just have the time to do everything immediately, but alas that is not the > case, and its also the reason why I don't really trust your memory (or mine, or > any of our collective memories), as the master todo list for things like this. > I'm too busy to do a proper fix now, I'm assuming you are as well, but Chao > apparently feels this is important enough to address (based on the fact that > he's proposed a fix for the problem). As such, Chao is the one who should be > addressing this issue. Until then, KNI can just not build on powerpc. > >>>>>> I wonder if we could try to guess the cache line size instead of >>>>>> configuring it in many places. >>>>>> Maybe we could use something like sysconf(_SC_LEVEL1_DCACHE_LINESIZE)? >>>>>> >>>>> This is a good idea, but I think its a bit broken for a few reasons: >>>>> >>>>> 1) _SC_LEVEL1_DCACHE_LINESIZE I don't think is POSIX mandated, so there is every >>>>> possibility that the above won't work on BSD >>>>> >>>>> 2) While getting the cache line size dynamically is a great idea, dpdk has >>>>> several locations that size structures based on processor cache line size, which >>>>> implicitly requires a static cache line definition. >>>> It can be guessed dynamically in the first build step (kind of configure). >>>> >>> That would work, though that seems like cause to really start redesigning the >>> build system to use autoconf/automake so we can run utilities to do that sort of >>> thing more easily (not opposed to that mind you, just illustrating that its more >>> work) >> I'm convinced we need to work on the build system but it's another discussion >> for next weeks. Speaking about that, the AF_PACKET PMD cannot be enabled because >> dependencies are not checked before building it. >> > I'm fine with that. If we're going to make the build system contain a depedency > checking mechanism, we'll start dynamically enabling them when support is > detected. Until then I'm fine with it being an opt in operation, as you know at > build time what you're minimum kernel support levels are. > > Speaking of enabling however, be careful of a double standard here. I know that > igb_uio won't build on some kernels either (linvlle posted in the > irc channel about it earlier), because we don't detect the presence of needed > defines. Yet IGB_UIO is still universally enabled... > >>>>> It seems the right thing to do, in my mind is to define RTE_CACHE_LINE_SIZE per >>>>> arch (perhaps in common/include/arch//rte_.h), then just let >>>>> the build break if a given arch doesn't define it (i.e. make definig that value >>>>> an arch reqirement). >>>> It's the other option. For IBM Power, it's currently overwritten in the Makefile: >>>> http://dpdk.org/browse/dpdk/tree/mk/arch/ppc_64/rte.vars.mk >>>> >>> Thats a sensible solution in my mind, though it is limited by the assumption >>> that any given arch has only a single cache line size (I dno't think thats a >>> problem, but it might be). If it is, the dynamic solution above is superior. >> I think we won't solve the hypothetical problem of heterogeneous CPUs in >> first step. I'd like to start with your proposal of a arch variable. >> >> -- >> Thomas >>