[dpdk-dev] [PATCH] lib: include rte_memory.h for __rte_cache_aligned

[dpdk-dev] [PATCH] lib: include rte_memory.h for __rte_cache_aligned

[Jia Yu]

Include rte_memory.h for lib files that use __rte_cache_aligned
attribute.

Signed-off-by: Jia Yu <jyu@vmware.com>
---
 lib/librte_distributor/rte_distributor.c        | 1 +
 lib/librte_eal/common/include/rte_malloc_heap.h | 1 +
 lib/librte_ip_frag/rte_ip_frag.h                | 1 +
 lib/librte_malloc/malloc_elem.h                 | 2 ++
 lib/librte_mbuf/rte_mbuf.h                      | 1 +
 lib/librte_port/rte_port_frag.c                 | 1 +
 lib/librte_table/rte_table_acl.c                | 1 +
 lib/librte_table/rte_table_array.c              | 1 +
 lib/librte_table/rte_table_hash_ext.c           | 1 +
 lib/librte_table/rte_table_hash_key16.c         | 1 +
 lib/librte_table/rte_table_hash_key32.c         | 1 +
 lib/librte_table/rte_table_hash_key8.c          | 1 +
 lib/librte_table/rte_table_hash_lru.c           | 1 +
 lib/librte_table/rte_table_lpm.c                | 1 +
 lib/librte_table/rte_table_lpm_ipv6.c           | 1 +
 15 files changed, 16 insertions(+)

diff --git a/lib/librte_distributor/rte_distributor.c b/lib/librte_distributor/rte_distributor.c
index 656ee5c..c3f7981 100644
--- a/lib/librte_distributor/rte_distributor.c
+++ b/lib/librte_distributor/rte_distributor.c
@@ -35,6 +35,7 @@
 #include <sys/queue.h>
 #include <string.h>
 #include <rte_mbuf.h>
+#include <rte_memory.h>
 #include <rte_memzone.h>
 #include <rte_errno.h>
 #include <rte_string_fns.h>
diff --git a/lib/librte_eal/common/include/rte_malloc_heap.h b/lib/librte_eal/common/include/rte_malloc_heap.h
index f727b7a..716216f 100644
--- a/lib/librte_eal/common/include/rte_malloc_heap.h
+++ b/lib/librte_eal/common/include/rte_malloc_heap.h
@@ -37,6 +37,7 @@
 #include <stddef.h>
 #include <sys/queue.h>
 #include <rte_spinlock.h>
+#include <rte_memory.h>
 
 /* Number of free lists per heap, grouped by size. */
 #define RTE_HEAP_NUM_FREELISTS  5
diff --git a/lib/librte_ip_frag/rte_ip_frag.h b/lib/librte_ip_frag/rte_ip_frag.h
index 230a903..3989a5a 100644
--- a/lib/librte_ip_frag/rte_ip_frag.h
+++ b/lib/librte_ip_frag/rte_ip_frag.h
@@ -46,6 +46,7 @@
 
 #include <rte_malloc.h>
 #include <rte_mbuf.h>
+#include <rte_memory.h>
 #include <rte_ip.h>
 #include <rte_byteorder.h>
 
diff --git a/lib/librte_malloc/malloc_elem.h b/lib/librte_malloc/malloc_elem.h
index 1d666a5..6e8c3e8 100644
--- a/lib/librte_malloc/malloc_elem.h
+++ b/lib/librte_malloc/malloc_elem.h
@@ -34,6 +34,8 @@
 #ifndef MALLOC_ELEM_H_
 #define MALLOC_ELEM_H_
 
+#include <rte_memory.h>
+
 /* dummy definition of struct so we can use pointers to it in malloc_elem struct */
 struct malloc_heap;
 
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index e8f9bfc..5998db0 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -54,6 +54,7 @@
 
 #include <stdint.h>
 #include <rte_mempool.h>
+#include <rte_memory.h>
 #include <rte_atomic.h>
 #include <rte_prefetch.h>
 #include <rte_branch_prediction.h>
diff --git a/lib/librte_port/rte_port_frag.c b/lib/librte_port/rte_port_frag.c
index 9f1bd3c..048ae2e 100644
--- a/lib/librte_port/rte_port_frag.c
+++ b/lib/librte_port/rte_port_frag.c
@@ -34,6 +34,7 @@
 
 #include <rte_ether.h>
 #include <rte_ip_frag.h>
+#include <rte_memory.h>
 
 #include "rte_port_frag.h"
 
diff --git a/lib/librte_table/rte_table_acl.c b/lib/librte_table/rte_table_acl.c
index c6d389e..1d88201 100644
--- a/lib/librte_table/rte_table_acl.c
+++ b/lib/librte_table/rte_table_acl.c
@@ -36,6 +36,7 @@
 
 #include <rte_common.h>
 #include <rte_mbuf.h>
+#include <rte_memory.h>
 #include <rte_malloc.h>
 #include <rte_log.h>
 
diff --git a/lib/librte_table/rte_table_array.c b/lib/librte_table/rte_table_array.c
index f0f5e1e..bb1ed38 100644
--- a/lib/librte_table/rte_table_array.c
+++ b/lib/librte_table/rte_table_array.c
@@ -36,6 +36,7 @@
 
 #include <rte_common.h>
 #include <rte_mbuf.h>
+#include <rte_memory.h>
 #include <rte_malloc.h>
 #include <rte_log.h>
 
diff --git a/lib/librte_table/rte_table_hash_ext.c b/lib/librte_table/rte_table_hash_ext.c
index 6e26d98..5096be5 100644
--- a/lib/librte_table/rte_table_hash_ext.c
+++ b/lib/librte_table/rte_table_hash_ext.c
@@ -36,6 +36,7 @@
 
 #include <rte_common.h>
 #include <rte_mbuf.h>
+#include <rte_memory.h>
 #include <rte_malloc.h>
 #include <rte_log.h>
 
diff --git a/lib/librte_table/rte_table_hash_key16.c b/lib/librte_table/rte_table_hash_key16.c
index f5ec87d..6976317 100644
--- a/lib/librte_table/rte_table_hash_key16.c
+++ b/lib/librte_table/rte_table_hash_key16.c
@@ -35,6 +35,7 @@
 
 #include <rte_common.h>
 #include <rte_mbuf.h>
+#include <rte_memory.h>
 #include <rte_malloc.h>
 #include <rte_log.h>
 
diff --git a/lib/librte_table/rte_table_hash_key32.c b/lib/librte_table/rte_table_hash_key32.c
index e8f4812..5ac91c0 100644
--- a/lib/librte_table/rte_table_hash_key32.c
+++ b/lib/librte_table/rte_table_hash_key32.c
@@ -35,6 +35,7 @@
 
 #include <rte_common.h>
 #include <rte_mbuf.h>
+#include <rte_memory.h>
 #include <rte_malloc.h>
 #include <rte_log.h>
 
diff --git a/lib/librte_table/rte_table_hash_key8.c b/lib/librte_table/rte_table_hash_key8.c
index d60c96e..9216eaf 100644
--- a/lib/librte_table/rte_table_hash_key8.c
+++ b/lib/librte_table/rte_table_hash_key8.c
@@ -35,6 +35,7 @@
 
 #include <rte_common.h>
 #include <rte_mbuf.h>
+#include <rte_memory.h>
 #include <rte_malloc.h>
 #include <rte_log.h>
 
diff --git a/lib/librte_table/rte_table_hash_lru.c b/lib/librte_table/rte_table_hash_lru.c
index d1a4984..9826957 100644
--- a/lib/librte_table/rte_table_hash_lru.c
+++ b/lib/librte_table/rte_table_hash_lru.c
@@ -36,6 +36,7 @@
 
 #include <rte_common.h>
 #include <rte_mbuf.h>
+#include <rte_memory.h>
 #include <rte_malloc.h>
 #include <rte_log.h>
 
diff --git a/lib/librte_table/rte_table_lpm.c b/lib/librte_table/rte_table_lpm.c
index a175ff3..5e3e0f2 100644
--- a/lib/librte_table/rte_table_lpm.c
+++ b/lib/librte_table/rte_table_lpm.c
@@ -36,6 +36,7 @@
 
 #include <rte_common.h>
 #include <rte_mbuf.h>
+#include <rte_memory.h>
 #include <rte_malloc.h>
 #include <rte_byteorder.h>
 #include <rte_log.h>
diff --git a/lib/librte_table/rte_table_lpm_ipv6.c b/lib/librte_table/rte_table_lpm_ipv6.c
index e3d59d0..62fdb33 100644
--- a/lib/librte_table/rte_table_lpm_ipv6.c
+++ b/lib/librte_table/rte_table_lpm_ipv6.c
@@ -36,6 +36,7 @@
 
 #include <rte_common.h>
 #include <rte_mbuf.h>
+#include <rte_memory.h>
 #include <rte_malloc.h>
 #include <rte_byteorder.h>
 #include <rte_log.h>
-- 
1.9.1

Re: [dpdk-dev] [PATCH] lib: include rte_memory.h for __rte_cache_aligned

[Thomas Monjalon]

2014-11-07 09:28, Jia Yu:
> Include rte_memory.h for lib files that use __rte_cache_aligned
> attribute.

Please could you explain what was the error?
As I suspect it's a fix, it would be clearer to start your title with "fix".

Thanks
-- 
Thomas

Re: [dpdk-dev] [PATCH] lib: include rte_memory.h for __rte_cache_aligned

[Jia Yu]

Thanks for reviewing this change.

No error message was found without this patch. This patch follows a good
coding practice to include the header files when it¹s needed, rather than
through some other header files. In the past, we found some data structure
was not cache line aligned due to missing the include, and caused memory
corruption. Compiler did not give warning or error when
__rte_cache_aligned attribute definition is missing. With DPDK latest, I
checked the data structures that used __rte_cache_aligned attributed in
these files, and they are properly aligned. So this patch is mainly for
better coding style now.

pahole -s libfile.[o, a]

ip_frag_pkt	192	1
ip_frag_tbl_stat	64	0
malloc_elem	64	0
rte_mbuf	128	1
rte_table_lpm	1088	1
rte_table_hash	64	1

Thanks,
Jia



On 11/10/14, 1:46 AM, "Thomas Monjalon" <thomas.monjalon@6wind.com> wrote:

>2014-11-07 09:28, Jia Yu:
>> Include rte_memory.h for lib files that use __rte_cache_aligned
>> attribute.
>
>Please could you explain what was the error?
>As I suspect it's a fix, it would be clearer to start your title with
>"fix".
>
>Thanks
>-- 
>Thomas

Re: [dpdk-dev] lib: include rte_memory.h for __rte_cache_aligned

[Neil Horman]

On Fri, Nov 07, 2014 at 09:28:09AM -0800, Jia Yu wrote:
> Include rte_memory.h for lib files that use __rte_cache_aligned
> attribute.
> 
> Signed-off-by: Jia Yu <jyu@vmware.com>
> 
Why?  I presume there was a build break or something.  Please repost with a
changelog that details what this patch is for.
Neil

> ---
> lib/librte_distributor/rte_distributor.c        | 1 +
>  lib/librte_eal/common/include/rte_malloc_heap.h | 1 +
>  lib/librte_ip_frag/rte_ip_frag.h                | 1 +
>  lib/librte_malloc/malloc_elem.h                 | 2 ++
>  lib/librte_mbuf/rte_mbuf.h                      | 1 +
>  lib/librte_port/rte_port_frag.c                 | 1 +
>  lib/librte_table/rte_table_acl.c                | 1 +
>  lib/librte_table/rte_table_array.c              | 1 +
>  lib/librte_table/rte_table_hash_ext.c           | 1 +
>  lib/librte_table/rte_table_hash_key16.c         | 1 +
>  lib/librte_table/rte_table_hash_key32.c         | 1 +
>  lib/librte_table/rte_table_hash_key8.c          | 1 +
>  lib/librte_table/rte_table_hash_lru.c           | 1 +
>  lib/librte_table/rte_table_lpm.c                | 1 +
>  lib/librte_table/rte_table_lpm_ipv6.c           | 1 +
>  15 files changed, 16 insertions(+)
> 
> diff --git a/lib/librte_distributor/rte_distributor.c b/lib/librte_distributor/rte_distributor.c
> index 656ee5c..c3f7981 100644
> --- a/lib/librte_distributor/rte_distributor.c
> +++ b/lib/librte_distributor/rte_distributor.c
> @@ -35,6 +35,7 @@
>  #include <sys/queue.h>
>  #include <string.h>
>  #include <rte_mbuf.h>
> +#include <rte_memory.h>
>  #include <rte_memzone.h>
>  #include <rte_errno.h>
>  #include <rte_string_fns.h>
> diff --git a/lib/librte_eal/common/include/rte_malloc_heap.h b/lib/librte_eal/common/include/rte_malloc_heap.h
> index f727b7a..716216f 100644
> --- a/lib/librte_eal/common/include/rte_malloc_heap.h
> +++ b/lib/librte_eal/common/include/rte_malloc_heap.h
> @@ -37,6 +37,7 @@
>  #include <stddef.h>
>  #include <sys/queue.h>
>  #include <rte_spinlock.h>
> +#include <rte_memory.h>
>  
>  /* Number of free lists per heap, grouped by size. */
>  #define RTE_HEAP_NUM_FREELISTS  5
> diff --git a/lib/librte_ip_frag/rte_ip_frag.h b/lib/librte_ip_frag/rte_ip_frag.h
> index 230a903..3989a5a 100644
> --- a/lib/librte_ip_frag/rte_ip_frag.h
> +++ b/lib/librte_ip_frag/rte_ip_frag.h
> @@ -46,6 +46,7 @@
>  
>  #include <rte_malloc.h>
>  #include <rte_mbuf.h>
> +#include <rte_memory.h>
>  #include <rte_ip.h>
>  #include <rte_byteorder.h>
>  
> diff --git a/lib/librte_malloc/malloc_elem.h b/lib/librte_malloc/malloc_elem.h
> index 1d666a5..6e8c3e8 100644
> --- a/lib/librte_malloc/malloc_elem.h
> +++ b/lib/librte_malloc/malloc_elem.h
> @@ -34,6 +34,8 @@
>  #ifndef MALLOC_ELEM_H_
>  #define MALLOC_ELEM_H_
>  
> +#include <rte_memory.h>
> +
>  /* dummy definition of struct so we can use pointers to it in malloc_elem struct */
>  struct malloc_heap;
>  
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index e8f9bfc..5998db0 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -54,6 +54,7 @@
>  
>  #include <stdint.h>
>  #include <rte_mempool.h>
> +#include <rte_memory.h>
>  #include <rte_atomic.h>
>  #include <rte_prefetch.h>
>  #include <rte_branch_prediction.h>
> diff --git a/lib/librte_port/rte_port_frag.c b/lib/librte_port/rte_port_frag.c
> index 9f1bd3c..048ae2e 100644
> --- a/lib/librte_port/rte_port_frag.c
> +++ b/lib/librte_port/rte_port_frag.c
> @@ -34,6 +34,7 @@
>  
>  #include <rte_ether.h>
>  #include <rte_ip_frag.h>
> +#include <rte_memory.h>
>  
>  #include "rte_port_frag.h"
>  
> diff --git a/lib/librte_table/rte_table_acl.c b/lib/librte_table/rte_table_acl.c
> index c6d389e..1d88201 100644
> --- a/lib/librte_table/rte_table_acl.c
> +++ b/lib/librte_table/rte_table_acl.c
> @@ -36,6 +36,7 @@
>  
>  #include <rte_common.h>
>  #include <rte_mbuf.h>
> +#include <rte_memory.h>
>  #include <rte_malloc.h>
>  #include <rte_log.h>
>  
> diff --git a/lib/librte_table/rte_table_array.c b/lib/librte_table/rte_table_array.c
> index f0f5e1e..bb1ed38 100644
> --- a/lib/librte_table/rte_table_array.c
> +++ b/lib/librte_table/rte_table_array.c
> @@ -36,6 +36,7 @@
>  
>  #include <rte_common.h>
>  #include <rte_mbuf.h>
> +#include <rte_memory.h>
>  #include <rte_malloc.h>
>  #include <rte_log.h>
>  
> diff --git a/lib/librte_table/rte_table_hash_ext.c b/lib/librte_table/rte_table_hash_ext.c
> index 6e26d98..5096be5 100644
> --- a/lib/librte_table/rte_table_hash_ext.c
> +++ b/lib/librte_table/rte_table_hash_ext.c
> @@ -36,6 +36,7 @@
>  
>  #include <rte_common.h>
>  #include <rte_mbuf.h>
> +#include <rte_memory.h>
>  #include <rte_malloc.h>
>  #include <rte_log.h>
>  
> diff --git a/lib/librte_table/rte_table_hash_key16.c b/lib/librte_table/rte_table_hash_key16.c
> index f5ec87d..6976317 100644
> --- a/lib/librte_table/rte_table_hash_key16.c
> +++ b/lib/librte_table/rte_table_hash_key16.c
> @@ -35,6 +35,7 @@
>  
>  #include <rte_common.h>
>  #include <rte_mbuf.h>
> +#include <rte_memory.h>
>  #include <rte_malloc.h>
>  #include <rte_log.h>
>  
> diff --git a/lib/librte_table/rte_table_hash_key32.c b/lib/librte_table/rte_table_hash_key32.c
> index e8f4812..5ac91c0 100644
> --- a/lib/librte_table/rte_table_hash_key32.c
> +++ b/lib/librte_table/rte_table_hash_key32.c
> @@ -35,6 +35,7 @@
>  
>  #include <rte_common.h>
>  #include <rte_mbuf.h>
> +#include <rte_memory.h>
>  #include <rte_malloc.h>
>  #include <rte_log.h>
>  
> diff --git a/lib/librte_table/rte_table_hash_key8.c b/lib/librte_table/rte_table_hash_key8.c
> index d60c96e..9216eaf 100644
> --- a/lib/librte_table/rte_table_hash_key8.c
> +++ b/lib/librte_table/rte_table_hash_key8.c
> @@ -35,6 +35,7 @@
>  
>  #include <rte_common.h>
>  #include <rte_mbuf.h>
> +#include <rte_memory.h>
>  #include <rte_malloc.h>
>  #include <rte_log.h>
>  
> diff --git a/lib/librte_table/rte_table_hash_lru.c b/lib/librte_table/rte_table_hash_lru.c
> index d1a4984..9826957 100644
> --- a/lib/librte_table/rte_table_hash_lru.c
> +++ b/lib/librte_table/rte_table_hash_lru.c
> @@ -36,6 +36,7 @@
>  
>  #include <rte_common.h>
>  #include <rte_mbuf.h>
> +#include <rte_memory.h>
>  #include <rte_malloc.h>
>  #include <rte_log.h>
>  
> diff --git a/lib/librte_table/rte_table_lpm.c b/lib/librte_table/rte_table_lpm.c
> index a175ff3..5e3e0f2 100644
> --- a/lib/librte_table/rte_table_lpm.c
> +++ b/lib/librte_table/rte_table_lpm.c
> @@ -36,6 +36,7 @@
>  
>  #include <rte_common.h>
>  #include <rte_mbuf.h>
> +#include <rte_memory.h>
>  #include <rte_malloc.h>
>  #include <rte_byteorder.h>
>  #include <rte_log.h>
> diff --git a/lib/librte_table/rte_table_lpm_ipv6.c b/lib/librte_table/rte_table_lpm_ipv6.c
> index e3d59d0..62fdb33 100644
> --- a/lib/librte_table/rte_table_lpm_ipv6.c
> +++ b/lib/librte_table/rte_table_lpm_ipv6.c
> @@ -36,6 +36,7 @@
>  
>  #include <rte_common.h>
>  #include <rte_mbuf.h>
> +#include <rte_memory.h>
>  #include <rte_malloc.h>
>  #include <rte_byteorder.h>
>  #include <rte_log.h>

Re: [dpdk-dev] lib: include rte_memory.h for __rte_cache_aligned

[Olivier MATZ]

Hi Neil,

On 12/08/2014 04:04 PM, Neil Horman wrote:
> On Fri, Nov 07, 2014 at 09:28:09AM -0800, Jia Yu wrote:
>> Include rte_memory.h for lib files that use __rte_cache_aligned
>> attribute.
>>
>> Signed-off-by: Jia Yu <jyu@vmware.com>
>>
> Why?  I presume there was a build break or something.  Please repost with a
> changelog that details what this patch is for.
> Neil

I don't know if Yu's issue was the same, but I had a very "fun" issue
with __rte_cache_aligned in my application. Consider the following code:

	struct per_core_foo {
		...
	} __rte_cache_aligned;

	struct global_foo {
		struct per_core_foo foo[RTE_MAX_CORE];
	};

If __rte_cache_aligned is not defined (rte_memory.h is not included),
the code compiles but the structure is not aligned... it defines the
structure and creates a global variable called __rte_cache_aligned.
And this can lead to really bad things if this code is in a .h that
is included by files that may or may not include rte_memory.h

I have no idea about how we could prevent this issue, except using
__attribute__((aligned(CACHE_LINE))) instead of __rte_cache_aligned.

Anyway this could probably explain the willing to include rte_memory.h
everywhere.

Regards,
Olivier

Re: [dpdk-dev] lib: include rte_memory.h for __rte_cache_aligned

[Jia Yu]

Yes, Olivier¹s observation is consistent with ours. Compilation didn¹t
complain
if rte_memory.h is not included.

Currently, the lib files indirectly included rte_mbuf.h or rte_mempool.h.
These two header files already included rte_memory.h. Therefore without
this patch, things still work (as validated by parole).

For good practice, it¹s better to explicitly include rte_memory.h to avoid
the problem.

Thanks,
Jia



On 12/9/14, 12:53 AM, "Olivier MATZ" <olivier.matz@6wind.com> wrote:

>Hi Neil,
>
>On 12/08/2014 04:04 PM, Neil Horman wrote:
>> On Fri, Nov 07, 2014 at 09:28:09AM -0800, Jia Yu wrote:
>>> Include rte_memory.h for lib files that use __rte_cache_aligned
>>> attribute.
>>>
>>> Signed-off-by: Jia Yu <jyu@vmware.com>
>>>
>> Why?  I presume there was a build break or something.  Please repost
>>with a
>> changelog that details what this patch is for.
>> Neil
>
>I don't know if Yu's issue was the same, but I had a very "fun" issue
>with __rte_cache_aligned in my application. Consider the following code:
>
>	struct per_core_foo {
>		...
>	} __rte_cache_aligned;
>
>	struct global_foo {
>		struct per_core_foo foo[RTE_MAX_CORE];
>	};
>
>If __rte_cache_aligned is not defined (rte_memory.h is not included),
>the code compiles but the structure is not aligned... it defines the
>structure and creates a global variable called __rte_cache_aligned.
>And this can lead to really bad things if this code is in a .h that
>is included by files that may or may not include rte_memory.h
>
>I have no idea about how we could prevent this issue, except using
>__attribute__((aligned(CACHE_LINE))) instead of __rte_cache_aligned.
>
>Anyway this could probably explain the willing to include rte_memory.h
>everywhere.
>
>Regards,
>Olivier
>

Re: [dpdk-dev] lib: include rte_memory.h for __rte_cache_aligned

[Neil Horman]

On Tue, Dec 09, 2014 at 09:53:18AM +0100, Olivier MATZ wrote:
> Hi Neil,
> 
> On 12/08/2014 04:04 PM, Neil Horman wrote:
> >On Fri, Nov 07, 2014 at 09:28:09AM -0800, Jia Yu wrote:
> >>Include rte_memory.h for lib files that use __rte_cache_aligned
> >>attribute.
> >>
> >>Signed-off-by: Jia Yu <jyu@vmware.com>
> >>
> >Why?  I presume there was a build break or something.  Please repost with a
> >changelog that details what this patch is for.
> >Neil
> 
> I don't know if Yu's issue was the same, but I had a very "fun" issue
> with __rte_cache_aligned in my application. Consider the following code:
> 
> 	struct per_core_foo {
> 		...
> 	} __rte_cache_aligned;
> 
> 	struct global_foo {
> 		struct per_core_foo foo[RTE_MAX_CORE];
> 	};
> 
> If __rte_cache_aligned is not defined (rte_memory.h is not included),
> the code compiles but the structure is not aligned... it defines the
> structure and creates a global variable called __rte_cache_aligned.
> And this can lead to really bad things if this code is in a .h that
> is included by files that may or may not include rte_memory.h
> 
> I have no idea about how we could prevent this issue, except using
> __attribute__((aligned(CACHE_LINE))) instead of __rte_cache_aligned.
> 
> Anyway this could probably explain the willing to include rte_memory.h
> everywhere.
> 
> Regards,
> Olivier
> 
> 

So, that is a great explination, and would be good to have in the changelog.

Also, to avoid the problem that you describe, while its preferred to have it at
the end of a struct, you can also put the alignment attribute right after the
struct keyword in gcc:
https://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html#Attribute-Syntax

That seems like it would solve the problem going forward.

Neil

Re: [dpdk-dev] lib: include rte_memory.h for __rte_cache_aligned

[Jia Yu]

Hi Neil,

Moving __rte_cache_aligned right after struct keyword will help. On the
other hand, enforcing this rule for existing (100+) and future definitions
will be difficult. It¹s clearer and a good practice to include header file
explicitly.

Thanks,
Jia


On 12/9/14, 7:22 AM, "Neil Horman" <nhorman@tuxdriver.com> wrote:

>On Tue, Dec 09, 2014 at 09:53:18AM +0100, Olivier MATZ wrote:
>> Hi Neil,
>> 
>> On 12/08/2014 04:04 PM, Neil Horman wrote:
>> >On Fri, Nov 07, 2014 at 09:28:09AM -0800, Jia Yu wrote:
>> >>Include rte_memory.h for lib files that use __rte_cache_aligned
>> >>attribute.
>> >>
>> >>Signed-off-by: Jia Yu <jyu@vmware.com>
>> >>
>> >Why?  I presume there was a build break or something.  Please repost
>>with a
>> >changelog that details what this patch is for.
>> >Neil
>> 
>> I don't know if Yu's issue was the same, but I had a very "fun" issue
>> with __rte_cache_aligned in my application. Consider the following code:
>> 
>> 	struct per_core_foo {
>> 		...
>> 	} __rte_cache_aligned;
>> 
>> 	struct global_foo {
>> 		struct per_core_foo foo[RTE_MAX_CORE];
>> 	};
>> 
>> If __rte_cache_aligned is not defined (rte_memory.h is not included),
>> the code compiles but the structure is not aligned... it defines the
>> structure and creates a global variable called __rte_cache_aligned.
>> And this can lead to really bad things if this code is in a .h that
>> is included by files that may or may not include rte_memory.h
>> 
>> I have no idea about how we could prevent this issue, except using
>> __attribute__((aligned(CACHE_LINE))) instead of __rte_cache_aligned.
>> 
>> Anyway this could probably explain the willing to include rte_memory.h
>> everywhere.
>> 
>> Regards,
>> Olivier
>> 
>> 
>
>So, that is a great explination, and would be good to have in the
>changelog.
>
>Also, to avoid the problem that you describe, while its preferred to have
>it at
>the end of a struct, you can also put the alignment attribute right after
>the
>struct keyword in gcc:
>https://urldefense.proofpoint.com/v2/url?u=https-3A__gcc.gnu.org_onlinedoc
>s_gcc_Attribute-2DSyntax.html-23Attribute-2DSyntax&d=AAIBAg&c=Sqcl0Ez6M0X8
>aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=q34pQj5yiMxs5OseYCxXLQ&m=mIyHF3ASZxRmbPs
>acyMyIABQlSafUdV9PqknKAtfOuI&s=pKoAAkIYRX31K-gR5oSwpcA5mLj4nG7uEzyh0z_uwxU
>&e= 
>
>That seems like it would solve the problem going forward.
>
>Neil
>

Re: [dpdk-dev] lib: include rte_memory.h for __rte_cache_aligned

[Neil Horman]

On Wed, Dec 10, 2014 at 07:09:03PM +0000, Jia Yu wrote:
> Hi Neil,
> 
> Moving __rte_cache_aligned right after struct keyword will help. On the
> other hand, enforcing this rule for existing (100+) and future definitions
> will be difficult. It¹s clearer and a good practice to include header file
> explicitly.
> 
You need to include the header file regardless of what you do.  The advantage to
placing the macro right after the struct keyword is that failure to include the
header will result in a compiler error, rather than silent behavioral changes
and run time breakage.

You don't have to enforce putting the attribute after the struct keyword, I'd
say just move them now to protect the current code.  Subsequent patch authors
will see the existing style and follow suit.  Or they won't, and we'll point out
the issue during review.

Regards
Neil

> Thanks,
> Jia
> 
> 
> On 12/9/14, 7:22 AM, "Neil Horman" <nhorman@tuxdriver.com> wrote:
> 
> >On Tue, Dec 09, 2014 at 09:53:18AM +0100, Olivier MATZ wrote:
> >> Hi Neil,
> >> 
> >> On 12/08/2014 04:04 PM, Neil Horman wrote:
> >> >On Fri, Nov 07, 2014 at 09:28:09AM -0800, Jia Yu wrote:
> >> >>Include rte_memory.h for lib files that use __rte_cache_aligned
> >> >>attribute.
> >> >>
> >> >>Signed-off-by: Jia Yu <jyu@vmware.com>
> >> >>
> >> >Why?  I presume there was a build break or something.  Please repost
> >>with a
> >> >changelog that details what this patch is for.
> >> >Neil
> >> 
> >> I don't know if Yu's issue was the same, but I had a very "fun" issue
> >> with __rte_cache_aligned in my application. Consider the following code:
> >> 
> >> 	struct per_core_foo {
> >> 		...
> >> 	} __rte_cache_aligned;
> >> 
> >> 	struct global_foo {
> >> 		struct per_core_foo foo[RTE_MAX_CORE];
> >> 	};
> >> 
> >> If __rte_cache_aligned is not defined (rte_memory.h is not included),
> >> the code compiles but the structure is not aligned... it defines the
> >> structure and creates a global variable called __rte_cache_aligned.
> >> And this can lead to really bad things if this code is in a .h that
> >> is included by files that may or may not include rte_memory.h
> >> 
> >> I have no idea about how we could prevent this issue, except using
> >> __attribute__((aligned(CACHE_LINE))) instead of __rte_cache_aligned.
> >> 
> >> Anyway this could probably explain the willing to include rte_memory.h
> >> everywhere.
> >> 
> >> Regards,
> >> Olivier
> >> 
> >> 
> >
> >So, that is a great explination, and would be good to have in the
> >changelog.
> >
> >Also, to avoid the problem that you describe, while its preferred to have
> >it at
> >the end of a struct, you can also put the alignment attribute right after
> >the
> >struct keyword in gcc:
> >https://urldefense.proofpoint.com/v2/url?u=https-3A__gcc.gnu.org_onlinedoc
> >s_gcc_Attribute-2DSyntax.html-23Attribute-2DSyntax&d=AAIBAg&c=Sqcl0Ez6M0X8
> >aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=q34pQj5yiMxs5OseYCxXLQ&m=mIyHF3ASZxRmbPs
> >acyMyIABQlSafUdV9PqknKAtfOuI&s=pKoAAkIYRX31K-gR5oSwpcA5mLj4nG7uEzyh0z_uwxU
> >&e= 
> >
> >That seems like it would solve the problem going forward.
> >
> >Neil
> >
> 
> 

Re: [dpdk-dev] lib: include rte_memory.h for __rte_cache_aligned

[Thomas Monjalon]

Hi Neil,

2014-12-10 19:28, Neil Horman:
> On Wed, Dec 10, 2014 at 07:09:03PM +0000, Jia Yu wrote:
> > Hi Neil,
> > 
> > Moving __rte_cache_aligned right after struct keyword will help. On the
> > other hand, enforcing this rule for existing (100+) and future definitions
> > will be difficult. It¹s clearer and a good practice to include header file
> > explicitly.
> > 
> You need to include the header file regardless of what you do.  The advantage to
> placing the macro right after the struct keyword is that failure to include the
> header will result in a compiler error, rather than silent behavioral changes
> and run time breakage.
> 
> You don't have to enforce putting the attribute after the struct keyword, I'd
> say just move them now to protect the current code.  Subsequent patch authors
> will see the existing style and follow suit.  Or they won't, and we'll point out
> the issue during review.

It should be a different patch for next release cycle.
Let's apply this fix only for 1.8.0.

-- 
Thomas

Re: [dpdk-dev] lib: include rte_memory.h for __rte_cache_aligned

[Thomas Monjalon]

2014-12-09 10:22, Neil Horman:
> On Tue, Dec 09, 2014 at 09:53:18AM +0100, Olivier MATZ wrote:
> > On 12/08/2014 04:04 PM, Neil Horman wrote:
> > >On Fri, Nov 07, 2014 at 09:28:09AM -0800, Jia Yu wrote:
> > >>Include rte_memory.h for lib files that use __rte_cache_aligned
> > >>attribute.
> > >>
> > >>Signed-off-by: Jia Yu <jyu@vmware.com>
> > >>
> > >Why?  I presume there was a build break or something.  Please repost with a
> > >changelog that details what this patch is for.
> > >Neil
> > 
> > I don't know if Yu's issue was the same, but I had a very "fun" issue
> > with __rte_cache_aligned in my application. Consider the following code:
> > 
> > 	struct per_core_foo {
> > 		...
> > 	} __rte_cache_aligned;
> > 
> > 	struct global_foo {
> > 		struct per_core_foo foo[RTE_MAX_CORE];
> > 	};
> > 
> > If __rte_cache_aligned is not defined (rte_memory.h is not included),
> > the code compiles but the structure is not aligned... it defines the
> > structure and creates a global variable called __rte_cache_aligned.
> > And this can lead to really bad things if this code is in a .h that
> > is included by files that may or may not include rte_memory.h
> > 
> > I have no idea about how we could prevent this issue, except using
> > __attribute__((aligned(CACHE_LINE))) instead of __rte_cache_aligned.
> > 
> > Anyway this could probably explain the willing to include rte_memory.h
> > everywhere.
> > 
> > Regards,
> > Olivier
> 
> So, that is a great explination, and would be good to have in the changelog.

Acked-by: Thomas Monjalon <thomas.monjalon@6wind.com>

Applied with Olivier's explanation.

Thanks
-- 
Thomas

Re: [dpdk-dev] lib: include rte_memory.h for __rte_cache_aligned

[Neil Horman]

On Thu, Dec 11, 2014 at 01:36:54AM +0100, Thomas Monjalon wrote:
> Hi Neil,
> 
> 2014-12-10 19:28, Neil Horman:
> > On Wed, Dec 10, 2014 at 07:09:03PM +0000, Jia Yu wrote:
> > > Hi Neil,
> > > 
> > > Moving __rte_cache_aligned right after struct keyword will help. On the
> > > other hand, enforcing this rule for existing (100+) and future definitions
> > > will be difficult. It¹s clearer and a good practice to include header file
> > > explicitly.
> > > 
> > You need to include the header file regardless of what you do.  The advantage to
> > placing the macro right after the struct keyword is that failure to include the
> > header will result in a compiler error, rather than silent behavioral changes
> > and run time breakage.
> > 
> > You don't have to enforce putting the attribute after the struct keyword, I'd
> > say just move them now to protect the current code.  Subsequent patch authors
> > will see the existing style and follow suit.  Or they won't, and we'll point out
> > the issue during review.
> 
> It should be a different patch for next release cycle.
> Let's apply this fix only for 1.8.0.
> 
Why?  Theres no harm in doing so now.
Neil

Re: [dpdk-dev] lib: include rte_memory.h for __rte_cache_aligned

[Thomas Monjalon]

2014-12-11 09:17, Neil Horman:
> On Thu, Dec 11, 2014 at 01:36:54AM +0100, Thomas Monjalon wrote:
> > Hi Neil,
> > 
> > 2014-12-10 19:28, Neil Horman:
> > > On Wed, Dec 10, 2014 at 07:09:03PM +0000, Jia Yu wrote:
> > > > Hi Neil,
> > > > 
> > > > Moving __rte_cache_aligned right after struct keyword will help. On the
> > > > other hand, enforcing this rule for existing (100+) and future definitions
> > > > will be difficult. It¹s clearer and a good practice to include header file
> > > > explicitly.
> > > > 
> > > You need to include the header file regardless of what you do.  The advantage to
> > > placing the macro right after the struct keyword is that failure to include the
> > > header will result in a compiler error, rather than silent behavioral changes
> > > and run time breakage.
> > > 
> > > You don't have to enforce putting the attribute after the struct keyword, I'd
> > > say just move them now to protect the current code.  Subsequent patch authors
> > > will see the existing style and follow suit.  Or they won't, and we'll point out
> > > the issue during review.
> > 
> > It should be a different patch for next release cycle.
> > Let's apply this fix only for 1.8.0.
> > 
> Why?  Theres no harm in doing so now.

It's a coding style good practice, not a bug fix.
We are now integrating only bug fixes and docs.
We have to put a limit to avoid last minutes bugs.

-- 
Thomas