Re: [dpdk-dev] [PATCH] Fix KNI compiling issue on IBM Power

[Thomas Monjalon <thomas.monjalon@6wind.com>]

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 <chaozhu@linux.vnet.ibm.com>
> > > > > > 
> > > > > > Acked-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> > > > > > 
> > > > > > 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.

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.

Next step:
Don't have any default value in these .h files but define it in a arch-header.

Next next step:
Detect the cache line size when compiling.

> > > 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).

> > > > > It seems the right thing to do, in my mind is to define RTE_CACHE_LINE_SIZE per
> > > > > arch (perhaps in common/include/arch/<arch>/rte_<something>.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.