DPDK patches and discussions
 help / color / mirror / Atom feed
From: Neil Horman <nhorman@tuxdriver.com>
To: Jia Yu <jyu@vmware.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] lib: include rte_memory.h for __rte_cache_aligned
Date: Wed, 10 Dec 2014 19:28:38 -0500	[thread overview]
Message-ID: <20141211002838.GA24240@localhost.localdomain> (raw)
In-Reply-To: <D0ADD64A.2E3F8%jyu@vmware.com>

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

  reply	other threads:[~2014-12-11  0:29 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-11-07 17:28 [dpdk-dev] [PATCH] " Jia Yu
2014-11-10  9:46 ` Thomas Monjalon
2014-11-17 19:07   ` Jia Yu
2014-12-08 15:04 ` [dpdk-dev] " Neil Horman
2014-12-09  8:53   ` Olivier MATZ
2014-12-09  9:11     ` Jia Yu
2014-12-09 15:22     ` Neil Horman
2014-12-10 19:09       ` Jia Yu
2014-12-11  0:28         ` Neil Horman [this message]
2014-12-11  0:36           ` Thomas Monjalon
2014-12-11 14:17             ` Neil Horman
2014-12-11 21:14               ` Thomas Monjalon
2014-12-11  0:51       ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20141211002838.GA24240@localhost.localdomain \
    --to=nhorman@tuxdriver.com \
    --cc=dev@dpdk.org \
    --cc=jyu@vmware.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).