lib: include rte_memory.h for __rte_cache_aligned

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

2014-11-07 09:28, Jia Yu:

Quoted 2 lines

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

Signature · 1 line

Thomas

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

Signed-off-by Jia Yu <[email protected]>

---
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(+)

lib/librte_distributor/rte_distributor.c +1 -0

index 656ee5c..c3f7981 100644
@@ -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>

lib/librte_eal/common/include/rte_malloc_heap.h +1 -0

index f727b7a..716216f 100644
@@ -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

lib/librte_ip_frag/rte_ip_frag.h +1 -0

index 230a903..3989a5a 100644
@@ -46,6 +46,7 @@
 
 #include <rte_malloc.h>
 #include <rte_mbuf.h>
+#include <rte_memory.h>
 #include <rte_ip.h>
 #include <rte_byteorder.h>
 

lib/librte_malloc/malloc_elem.h +2 -0

index 1d666a5..6e8c3e8 100644
@@ -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;
 

lib/librte_mbuf/rte_mbuf.h +1 -0

index e8f9bfc..5998db0 100644
@@ -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>

lib/librte_port/rte_port_frag.c +1 -0

index 9f1bd3c..048ae2e 100644
@@ -34,6 +34,7 @@
 
 #include <rte_ether.h>
 #include <rte_ip_frag.h>
+#include <rte_memory.h>
 
 #include "rte_port_frag.h"
 

lib/librte_table/rte_table_acl.c +1 -0

index c6d389e..1d88201 100644
@@ -36,6 +36,7 @@
 
 #include <rte_common.h>
 #include <rte_mbuf.h>
+#include <rte_memory.h>
 #include <rte_malloc.h>
 #include <rte_log.h>
 

lib/librte_table/rte_table_array.c +1 -0

index f0f5e1e..bb1ed38 100644
@@ -36,6 +36,7 @@
 
 #include <rte_common.h>
 #include <rte_mbuf.h>
+#include <rte_memory.h>
 #include <rte_malloc.h>
 #include <rte_log.h>
 

lib/librte_table/rte_table_hash_ext.c +1 -0

index 6e26d98..5096be5 100644
@@ -36,6 +36,7 @@
 
 #include <rte_common.h>
 #include <rte_mbuf.h>
+#include <rte_memory.h>
 #include <rte_malloc.h>
 #include <rte_log.h>
 

lib/librte_table/rte_table_hash_key16.c +1 -0

index f5ec87d..6976317 100644
@@ -35,6 +35,7 @@
 
 #include <rte_common.h>
 #include <rte_mbuf.h>
+#include <rte_memory.h>
 #include <rte_malloc.h>
 #include <rte_log.h>
 

lib/librte_table/rte_table_hash_key32.c +1 -0

index e8f4812..5ac91c0 100644
@@ -35,6 +35,7 @@
 
 #include <rte_common.h>
 #include <rte_mbuf.h>
+#include <rte_memory.h>
 #include <rte_malloc.h>
 #include <rte_log.h>
 

lib/librte_table/rte_table_hash_key8.c +1 -0

index d60c96e..9216eaf 100644
@@ -35,6 +35,7 @@
 
 #include <rte_common.h>
 #include <rte_mbuf.h>
+#include <rte_memory.h>
 #include <rte_malloc.h>
 #include <rte_log.h>
 

lib/librte_table/rte_table_hash_lru.c +1 -0

index d1a4984..9826957 100644
@@ -36,6 +36,7 @@
 
 #include <rte_common.h>
 #include <rte_mbuf.h>
+#include <rte_memory.h>
 #include <rte_malloc.h>
 #include <rte_log.h>
 

lib/librte_table/rte_table_lpm.c +1 -0

index a175ff3..5e3e0f2 100644
@@ -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>

lib/librte_table/rte_table_lpm_ipv6.c +1 -0

index e3d59d0..62fdb33 100644
@@ -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>

Quoted 5 lines · Jia Yu

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

Signed-off-by Jia Yu <[email protected]>

Why? I presume there was a build break or something. Please repost with a changelog that details what this patch is for.
Neil

Hi Neil,

Quoted 10 lines · Neil Horman

Quoted 5 lines · Jia Yu

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

Signed-off-by Jia Yu <[email protected]>

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

Hi Neil,

Quoted 9 lines · Neil Horman

Quoted 5 lines · Jia Yu

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

Signed-off-by Jia Yu <[email protected]>

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

Quoted 31 lines · Olivier MATZ

Hi Neil,

Quoted 7 lines · Neil Horman

Quoted 5 lines · Jia Yu

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

Signed-off-by Jia Yu <[email protected]>

Why? I presume there was a build break or something. Please repost

with a

Quoted 2 lines

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

Quoted 10 lines

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

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

Quoted 61 lines · Neil Horman

Quoted 31 lines · Olivier MATZ

Hi Neil,

Quoted 7 lines · Neil Horman

Quoted 5 lines · Jia Yu

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

Signed-off-by Jia Yu <[email protected]>

Why? I presume there was a build break or something. Please repost

with a

Quoted 2 lines

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

Quoted 10 lines

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

Quoted 7 lines

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.

Hi Neil,

2014-12-10 19:28, Neil Horman:

Quoted 13 lines · Jia Yu

Quoted 7 lines

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.

Quoted 4 lines

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.

Quoted 21 lines

Hi Neil,

2014-12-10 19:28, Neil Horman:

Quoted 17 lines · Jia Yu

Quoted 7 lines

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.

Quoted 3 lines

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.

Quoted 31 lines · Neil Horman

Quoted 9 lines · Jia Yu

Quoted 5 lines

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

Signed-off-by Jia Yu <[email protected]>

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.

Quoted 5 lines

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.