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

[Neil Horman <nhorman@tuxdriver.com>]

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.  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/<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.
Neil

> Thanks for helping to find a better solution.
> -- 
> Thomas
>