From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id ED6196A87 for ; Thu, 11 Dec 2014 01:29:28 +0100 (CET) Received: from [2001:470:8:a08:215:ff:fecc:4872] (helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63) (envelope-from ) id 1Xyrce-0001y7-DE; Wed, 10 Dec 2014 19:28:59 -0500 Date: Wed, 10 Dec 2014 19:28:38 -0500 From: Neil Horman To: Jia Yu 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-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Score: -2.9 (--) X-Spam-Status: No Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] lib: include rte_memory.h for __rte_cache_aligned X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Dec 2014 00:29:30 -0000 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" 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&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 > > > >