From mboxrd@z Thu Jan 1 00:00:00 1970 From: Neil Horman Subject: Re: lib: include rte_memory.h for __rte_cache_aligned Date: Wed, 10 Dec 2014 19:28:38 -0500 Message-ID: <20141211002838.GA24240@localhost.localdomain> References: <1415381289-43291-1-git-send-email-jyu@vmware.com> <20141208150401.GB3907@localhost.localdomain> <5486B87E.5010404@6wind.com> <20141209152204.GD28871@hmsreliant.think-freely.org> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable Cc: "dev-VfR2kkLFssw@public.gmane.org" To: Jia Yu Return-path: Content-Disposition: inline In-Reply-To: List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" On Wed, Dec 10, 2014 at 07:09:03PM +0000, Jia Yu wrote: > Hi Neil, >=20 > Moving __rte_cache_aligned right after struct keyword will help. On the > other hand, enforcing this rule for existing (100+) and future definiti= ons > will be difficult. It=B9s clearer and a good practice to include header= file > explicitly. >=20 You need to include the header file regardless of what you do. The advan= tage to placing the macro right after the struct keyword is that failure to inclu= de the header will result in a compiler error, rather than silent behavioral cha= nges 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 aut= hors will see the existing style and follow suit. Or they won't, and we'll po= int out the issue during review. Regards Neil > Thanks, > Jia >=20 >=20 > On 12/9/14, 7:22 AM, "Neil Horman" wrote: >=20 > >On Tue, Dec 09, 2014 at 09:53:18AM +0100, Olivier MATZ wrote: > >> Hi Neil, > >>=20 > >> 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 repos= t > >>with a > >> >changelog that details what this patch is for. > >> >Neil > >>=20 > >> I don't know if Yu's issue was the same, but I had a very "fun" issu= e > >> with __rte_cache_aligned in my application. Consider the following c= ode: > >>=20 > >> struct per_core_foo { > >> ... > >> } __rte_cache_aligned; > >>=20 > >> struct global_foo { > >> struct per_core_foo foo[RTE_MAX_CORE]; > >> }; > >>=20 > >> 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 > >>=20 > >> I have no idea about how we could prevent this issue, except using > >> __attribute__((aligned(CACHE_LINE))) instead of __rte_cache_aligned. > >>=20 > >> Anyway this could probably explain the willing to include rte_memory= .h > >> everywhere. > >>=20 > >> Regards, > >> Olivier > >>=20 > >>=20 > > > >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 h= ave > >it at > >the end of a struct, you can also put the alignment attribute right af= ter > >the > >struct keyword in gcc: > >https://urldefense.proofpoint.com/v2/url?u=3Dhttps-3A__gcc.gnu.org_onl= inedoc > >s_gcc_Attribute-2DSyntax.html-23Attribute-2DSyntax&d=3DAAIBAg&c=3DSqcl= 0Ez6M0X8 > >aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=3Dq34pQj5yiMxs5OseYCxXLQ&m=3DmIyHF3A= SZxRmbPs > >acyMyIABQlSafUdV9PqknKAtfOuI&s=3DpKoAAkIYRX31K-gR5oSwpcA5mLj4nG7uEzyh0= z_uwxU > >&e=3D=20 > > > >That seems like it would solve the problem going forward. > > > >Neil > > >=20 >=20