[dpdk-dev] lib: include rte_memory.h for __rte_cache_aligned
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 +, 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
[dpdk-dev] lib: include rte_memory.h for __rte_cache_aligned
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
> > >>
> > >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
Applied with Olivier's explanation.
Thanks
--
Thomas
[dpdk-dev] lib: include rte_memory.h for __rte_cache_aligned
Hi Neil, 2014-12-10 19:28, Neil Horman: > On Wed, Dec 10, 2014 at 07:09:03PM +, 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
[dpdk-dev] lib: include rte_memory.h for __rte_cache_aligned
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" 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
>> >>
>> >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=AAIBAg=Sqcl0Ez6M0X8
>aeM67LKIiDJAXVeAw-YihVMNtXt-uEs=q34pQj5yiMxs5OseYCxXLQ=mIyHF3ASZxRmbPs
>acyMyIABQlSafUdV9PqknKAtfOuI=pKoAAkIYRX31K-gR5oSwpcA5mLj4nG7uEzyh0z_uwxU
>=
>
>That seems like it would solve the problem going forward.
>
>Neil
>
[dpdk-dev] lib: include rte_memory.h for __rte_cache_aligned
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
> >>
> >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
[dpdk-dev] lib: include rte_memory.h for __rte_cache_aligned
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
>>
> 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
[dpdk-dev] lib: include rte_memory.h for __rte_cache_aligned
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" 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
>>>
>> 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
>
[dpdk-dev] lib: include rte_memory.h for __rte_cache_aligned
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 > 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 > #include > #include > +#include > #include > #include > #include > 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 > #include > #include > +#include > > /* 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 > #include > +#include > #include > #include > > 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 > + > /* 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 > #include > +#include > #include > #include > #include > 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 > #include > +#include > > #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 > #include > +#include > #include > #include > > 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 > #include > +#include > #include > #include > > 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 > #include > +#include > #include > #include > > 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 > #include > +#include > #include > #include > > 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 > #include > +#include > #include > #include > > 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 > +++
