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

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

[Chao Zhu]

This patch solves the KNI compiling problem on IBM Power by using RTE_CACHE_LINE_SIZE
micro.

Chao Zhu (1):
  Fix KNI compiling issue on IBM Power

 .../linuxapp/eal/include/exec-env/rte_kni_common.h |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

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

[Chao Zhu]

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>
---
 .../linuxapp/eal/include/exec-env/rte_kni_common.h |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
index e548161..6fc6442 100644
--- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
+++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
@@ -67,6 +67,9 @@
  * KNI name is part of memzone name.
  */
 #define RTE_KNI_NAMESIZE 32
+#ifndef RTE_CACHE_LINE_SIZE
+#define RTE_CACHE_LINE_SIZE 64                  /**< Cache line size. */
+#endif
 
 /*
  * Request id.
@@ -108,7 +111,7 @@ struct rte_kni_fifo {
  * Padding is necessary to assure the offsets of these fields
  */
 struct rte_kni_mbuf {
-	void *buf_addr __attribute__((__aligned__(64)));
+	void *buf_addr __attribute__((__aligned__(RTE_CACHE_LINE_SIZE)));
 	char pad0[10];
 	uint16_t data_off;      /**< Start address of data in segment buffer. */
 	char pad1[4];
@@ -118,7 +121,7 @@ struct rte_kni_mbuf {
 	uint32_t pkt_len;       /**< Total pkt len: sum of all segment data_len. */
 
 	/* fields on second cache line */
-	char pad3[8] __attribute__((__aligned__(64)));
+	char pad3[8] __attribute__((__aligned__(RTE_CACHE_LINE_SIZE)));
 	void *pool;
 	void *next;
 };
-- 
1.7.1

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

[Thomas Monjalon]

> 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

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

Thanks
-- 
Thomas

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

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

Neil

> Thanks
> -- 
> Thomas
> 

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

[Thomas Monjalon]

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

> 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

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

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

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

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

[Thomas Monjalon]

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

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

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

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

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

-- 
Thomas

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

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

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

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

[Chao Zhu]

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

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

[Thomas Monjalon]

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.

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

[Neil Horman]

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

>From the IRC session:

linville   CC [M]
/home/linville/git/dpdk/build/build/lib/librte_eal/linuxapp/igb_uio/igb_uio.o
linville
/home/linville/git/dpdk/build/build/lib/librte_eal/linuxapp/igb_uio/igb_uio.c:
In function ‘store_max_vfs’:
linville
/home/linville/git/dpdk/build/build/lib/librte_eal/linuxapp/igb_uio/igb_uio.c:86:2:
error: implicit declaration of function ‘strict_strtoul’
[-Werror=implicit-function-declaration]
linville   if (0 != strict_strtoul(buf, 0, &max_vfs))
linville   ^
linville
/home/linville/git/dpdk/build/build/lib/librte_eal/linuxapp/igb_uio/igb_uio.c:
In function ‘igbuio_dom0_mmap_phys’:
linville
/home/linville/git/dpdk/build/build/lib/librte_eal/linuxapp/igb_uio/igb_uio.c:290:30:
error: ‘_PAGE_IOMAP’ undeclared (first use in this function)
linville   vma->vm_page_prot.pgprot |= _PAGE_IOMAP;
linville                               ^
linville
/home/linville/git/dpdk/build/build/lib/librte_eal/linuxapp/igb_uio/igb_uio.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

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

[Thomas Monjalon]

2014-12-05 09:42, Neil Horman:
> 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:
> > > > 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).
> > 
> 
> From the IRC session:
> 
> linville   CC [M]
> /home/linville/git/dpdk/build/build/lib/librte_eal/linuxapp/igb_uio/igb_uio.o
> linville
> /home/linville/git/dpdk/build/build/lib/librte_eal/linuxapp/igb_uio/igb_uio.c:
> In function ‘store_max_vfs’:
> linville
> /home/linville/git/dpdk/build/build/lib/librte_eal/linuxapp/igb_uio/igb_uio.c:86:2:
> error: implicit declaration of function ‘strict_strtoul’
> [-Werror=implicit-function-declaration]
> linville   if (0 != strict_strtoul(buf, 0, &max_vfs))
> linville   ^

Yes, this issue is being fixed. Jincheng Miao should send a v3:
	http://dpdk.org/ml/archives/dev/2014-December/009182.html

> linville
> /home/linville/git/dpdk/build/build/lib/librte_eal/linuxapp/igb_uio/igb_uio.c:
> In function ‘igbuio_dom0_mmap_phys’:
> linville
> /home/linville/git/dpdk/build/build/lib/librte_eal/linuxapp/igb_uio/igb_uio.c:290:30:
> error: ‘_PAGE_IOMAP’ undeclared (first use in this function)
> linville   vma->vm_page_prot.pgprot |= _PAGE_IOMAP;
> linville                               ^

Xen is disabled by default. So this issue hasn't been raised yet.
Fixes are welcome.

> linville
> /home/linville/git/dpdk/build/build/lib/librte_eal/linuxapp/igb_uio/igb_uio.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

There are some bugs.
Thanks for reporting (it would more visible in another thread).

-- 
Thomas