From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id 00A687DF4 for ; Fri, 5 Dec 2014 15:42:13 +0100 (CET) Received: from hmsreliant.think-freely.org ([2001:470:8:a08:7aac:c0ff:fec2:933b] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63) (envelope-from ) id 1Xwu58-0000hO-GA; Fri, 05 Dec 2014 09:42:09 -0500 Date: Fri, 5 Dec 2014 09:42:00 -0500 From: Neil Horman To: Thomas Monjalon Message-ID: <20141205144200.GC29245@hmsreliant.think-freely.org> References: <1417688048-23076-1-git-send-email-chaozhu@linux.vnet.ibm.com> <6950163.rWPXMLohSt@xps13> <20141204200538.GB18930@hmsreliant.think-freely.org> <1585880.VN9dgqiZzk@xps13> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1585880.VN9dgqiZzk@xps13> User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Score: -2.9 (--) X-Spam-Status: No 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 14:42:14 -0000 On Fri, Dec 05, 2014 at 02:10:27PM +0100, Thomas Monjalon wrote: > 2014-12-04 15:05, Neil Horman: > > 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. > > Before this patch: > Cache line size was hardcoded to 64 for KNI. So changing cache line size > constant in other files lead to a mismatch. > Before this patch: RTE_CACHE_LINE_SIZE is defined in rte_memory.h After this patch: RTE_CACHE_LINE_SIZE is defined both in rte_memory.h and rte_kni_common.h, as well as on the command line for ppc64, and in rte_acl_osdep_alone.h > After this patch: > The same constant is used everywhere and initialized to 64 in every .h files. > It is possible to override this value on the make command line or in makefiles. > No, its not, the same macro name is used in all locations, but which definition of the macro gets used depends entirely on the order in which the headers are included, and weather its defined on the compilation command line. That is a hack. > Next step: > Don't have any default value in these .h files but define it in a arch-header. > Yes, it is, and it seems like a fairly easy step to take, something that Chao can do instead of just comming up with a new location to define RTE_CACHE_LINE_SIZE in. > Next next step: > Detect the cache line size when compiling. > Sure, thats a longer term effort. > > > > 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... > > I'm really surprised. Please post an email to report the problem. > To my knowledge, igb_uio build on every supported Linux distributions > (kernel >= 2.6.32). > >>From the IRC session: linville CC [M] /home/linville/git/dpdk/build/build/lib/librte_eal/linuxapp/igb_uio/igb_uio.o linville /home/linville/git/dpdk/build/build/lib/librte_eal/linuxapp/igb_uio/igb_uio.c: In function ‘store_max_vfs’: linville /home/linville/git/dpdk/build/build/lib/librte_eal/linuxapp/igb_uio/igb_uio.c:86:2: error: implicit declaration of function ‘strict_strtoul’ [-Werror=implicit-function-declaration] linville if (0 != strict_strtoul(buf, 0, &max_vfs)) linville ^ linville /home/linville/git/dpdk/build/build/lib/librte_eal/linuxapp/igb_uio/igb_uio.c: In function ‘igbuio_dom0_mmap_phys’: linville /home/linville/git/dpdk/build/build/lib/librte_eal/linuxapp/igb_uio/igb_uio.c:290:30: error: ‘_PAGE_IOMAP’ undeclared (first use in this function) linville vma->vm_page_prot.pgprot |= _PAGE_IOMAP; linville ^ linville /home/linville/git/dpdk/build/build/lib/librte_eal/linuxapp/igb_uio/igb_uio.c:290:30: note: each undeclared identifier is reported only once for each function it appears in linville cc1: all warnings being treated as errors linville does igb_uio only work with certain old kernels? linville I'm running 3.18-rc7