Re: lib: include rte_memory.h for __rte_cache_aligned

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" [off-list ref] wrote:

quoted

On Tue, Dec 09, 2014 at 09:53:18AM +0100, Olivier MATZ wrote:

quoted

Hi Neil,

On 12/08/2014 04:04 PM, Neil Horman wrote:

quoted

On Fri, Nov 07, 2014 at 09:28:09AM -0800, Jia Yu wrote:

quoted

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

Signed-off-by: Jia Yu <redacted>

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

with a

quoted

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