From mboxrd@z Thu Jan 1 00:00:00 1970 From: Neil Horman Subject: Re: [PATCH] Fix KNI compiling issue on IBM Power Date: Fri, 5 Dec 2014 09:42:00 -0500 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-Transfer-Encoding: quoted-printable Cc: dev-VfR2kkLFssw@public.gmane.org To: Thomas Monjalon Return-path: Content-Disposition: inline In-Reply-To: <1585880.VN9dgqiZzk@xps13> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" 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 wro= te: > > > > > > > > Because of different cache line size, the alignment of st= ruct > > > > > > > > rte_kni_mbuf in rte_kni_common.h doesn't work on IBM Powe= r. This patch > > > > > > > > changed from 64 to RTE_CACHE_LINE_SIZE micro to do the al= ignment. > > > > > > > >=20 > > > > > > > > Signed-off-by: Chao Zhu > > > > > > >=20 > > > > > > > Acked-by: Thomas Monjalon > > > > > > >=20 > > > > > > > Applied > > > > > > >=20 > > > > > > Woah! Slow down here, I'm not sure if this makes sense to fi= x his way. The > > > > > > exact same ifndef/define/endif construct is used for this mac= ro in rte_memory.h. > > > > > > Currently their defined to the same vaule, but if that ever c= hanges, this macro > > > > > > will return different values based on the order in which head= er files are > > > > > > included. That doesn't seem appropriate at all. > > > > >=20 > > > > > 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? > > > > >=20 > > > > I really don't like the idea of hacks like this being used. > > >=20 > > > 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). > > >=20 > > I'm not referring to replacing a hardcoded value with a constant macr= o. The > > hack I'm referring to is that of defining that macro in multiple plac= es using > > the ifndef/define/endif construct. Generally its fine to use that me= chanism 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 head= er files > > with this patch, which in turn leads to the possibility of the defini= tion > > location changing dependent on which header file is included first in= a > > compilation unit. Thats the hack. >=20 > Before this patch: > Cache line size was hardcoded to 64 for KNI. So changing cache line siz= e > constant in other files lead to a mismatch. >=20 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 ma= kefiles. >=20 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. >=20 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. >=20 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 anythin= g > > > > changes cache line sizes until it gets fixed properly runs the ri= sk of > > > > introducing wierd behavioral issues at compile time. > > >=20 > > > It was also the case before: 64 was hardcoded for KNI. > > >=20 > > See above, not concerned with the hardcoded vs macro idea, just how t= he macro is > > implemented. > >=20 > > > > I'm also concerned about the fact that, since we have no bug trac= ker 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. > > >=20 > > > 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. > > >=20 > > 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 (o= r mine, or > > any of our collective memories), as the master todo list for things l= ike this. > > I'm too busy to do a proper fix now, I'm assuming you are as well, bu= t Chao > > apparently feels this is important enough to address (based on the fa= ct that > > he's proposed a fix for the problem). As such, Chao is the one who s= hould be > > addressing this issue. Until then, KNI can just not build on powerpc= . > >=20 > > > > > > > I wonder if we could try to guess the cache line size inste= ad of > > > > > > > configuring it in many places. > > > > > > > Maybe we could use something like sysconf(_SC_LEVEL1_DCACHE= _LINESIZE)? > > > > > > >=20 > > > > > > This is a good idea, but I think its a bit broken for a few r= easons: > > > > > >=20 > > > > > > 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 > > > > > >=20 > > > > > > 2) While getting the cache line size dynamically is a great i= dea, dpdk has > > > > > > several locations that size structures based on processor cac= he line size, which > > > > > > implicitly requires a static cache line definition. > > > > >=20 > > > > > It can be guessed dynamically in the first build step (kind of = configure). > > > > >=20 > > > > That would work, though that seems like cause to really start red= esigning 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 illustratin= g that its more > > > > work) > > >=20 > > > 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 en= abled because > > > dependencies are not checked before building it. > > >=20 > > 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 suppor= t 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. > >=20 > > 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 o= f needed > > defines. Yet IGB_UIO is still universally enabled... >=20 > I'm really surprised. Please post an email to report the problem. > To my knowledge, igb_uio build on every supported Linux distributions > (kernel >=3D 2.6.32). >=20 >>From the IRC session: linville CC [M] /home/linville/git/dpdk/build/build/lib/librte_eal/linuxapp/igb_uio/igb_u= io.o linville /home/linville/git/dpdk/build/build/lib/librte_eal/linuxapp/igb_uio/igb_u= io.c: In function =E2=80=98store_max_vfs=E2=80=99: linville /home/linville/git/dpdk/build/build/lib/librte_eal/linuxapp/igb_uio/igb_u= io.c:86:2: error: implicit declaration of function =E2=80=98strict_strtoul=E2=80=99 [-Werror=3Dimplicit-function-declaration] linville if (0 !=3D strict_strtoul(buf, 0, &max_vfs)) linville ^ linville /home/linville/git/dpdk/build/build/lib/librte_eal/linuxapp/igb_uio/igb_u= io.c: In function =E2=80=98igbuio_dom0_mmap_phys=E2=80=99: linville /home/linville/git/dpdk/build/build/lib/librte_eal/linuxapp/igb_uio/igb_u= io.c:290:30: error: =E2=80=98_PAGE_IOMAP=E2=80=99 undeclared (first use in this functi= on) linville vma->vm_page_prot.pgprot |=3D _PAGE_IOMAP; linville ^ linville /home/linville/git/dpdk/build/build/lib/librte_eal/linuxapp/igb_uio/igb_u= io.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