From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chao Zhu Subject: Re: [PATCH] Fix KNI compiling issue on IBM Power Date: Fri, 05 Dec 2014 17:11:13 +0800 Message-ID: <548176B1.8040100@linux.vnet.ibm.com> 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> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Cc: dev-VfR2kkLFssw@public.gmane.org To: Neil Horman , Thomas Monjalon Return-path: In-Reply-To: <20141204200538.GB18930-B26myB8xz7F8NnZeBjwnZQMhkBWG/bsMQH7oEaQurus@public.gmane.org> 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 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 >>