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 872C97EB0 for ; Thu, 4 Dec 2014 16:33:09 +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 1XwYOs-0002IE-Q4; Thu, 04 Dec 2014 10:33:05 -0500 Date: Thu, 4 Dec 2014 10:32:57 -0500 From: Neil Horman To: Thomas Monjalon Message-ID: <20141204153256.GE16249@hmsreliant.think-freely.org> References: <1417688048-23076-1-git-send-email-chaozhu@linux.vnet.ibm.com> <1950672.AxEg8fkUWW@xps13> <20141204132939.GB16249@hmsreliant.think-freely.org> <1795169.cqFrYtuj77@xps13> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1795169.cqFrYtuj77@xps13> User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Score: -2.9 (--) X-Spam-Status: No Cc: dev@dpdk.org, Chao Zhu 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: Thu, 04 Dec 2014 15:33:09 -0000 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. 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. 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. > > > 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) > > 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. Neil > Thanks for helping to find a better solution. > -- > Thomas >