From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 33BD143A0F; Tue, 30 Jan 2024 18:39:31 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id BAF36402CD; Tue, 30 Jan 2024 18:39:30 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 09386402AC for ; Tue, 30 Jan 2024 18:39:29 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1086) id 59DE62057C03; Tue, 30 Jan 2024 09:39:28 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 59DE62057C03 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1706636368; bh=oE0GMhnXyVK7MZwynhNzcXhalVovUEgYSqjJUCYDAmM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ny3We8IWj4QekjT2L9SQER6ih6UYVX8f5rGtgoD6D94o4AzOxejKPXiLc//kkjsZH ZntkfsQporGRDvsI+PvEmW467q5BKD434lbC7UdRCJk6/sHdBRoUA2QGNNSPlphqma IHG+0iBA/kBCnf6vfHYtjKDUi96Ye42/yBcC6kcY= Date: Tue, 30 Jan 2024 09:39:28 -0800 From: Tyler Retzlaff To: Mattias =?iso-8859-1?Q?R=F6nnblom?= Cc: Morten =?iso-8859-1?Q?Br=F8rup?= , dev@dpdk.org, Mattias =?iso-8859-1?Q?R=F6nnblom?= , Anatoly Burakov , Bruce Richardson , David Christensen , Harry van Haaren , Konstantin Ananyev , Min Zhou , Ruifeng Wang , Stanislaw Kardach , thomas@monjalon.net Subject: Re: [PATCH] RFC: use C11 alignas instead of GCC attribute aligned Message-ID: <20240130173928.GA2943@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1700069997-4399-1-git-send-email-roretzla@linux.microsoft.com> <20240125183713.GA27715@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <98CBD80474FA8B44BF855DF32C47DC35E9F1A3@smartserver.smartshare.dk> <91ba1ece-10dd-4698-acd9-6b51cfc63cd9@lysator.liu.se> <98CBD80474FA8B44BF855DF32C47DC35E9F1A7@smartserver.smartshare.dk> <635f0d9f-6665-426b-b778-d61e5e732fbe@lysator.liu.se> <98CBD80474FA8B44BF855DF32C47DC35E9F1AA@smartserver.smartshare.dk> <413840c1-a263-4118-adfe-d4ae0ec0b52d@lysator.liu.se> <20240129194356.GA25654@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Tue, Jan 30, 2024 at 09:08:21AM +0100, Mattias Rönnblom wrote: > On 2024-01-29 20:43, Tyler Retzlaff wrote: > >On Sun, Jan 28, 2024 at 11:00:31AM +0100, Mattias Rönnblom wrote: > >>On 2024-01-28 09:57, Morten Brørup wrote: > >>>>From: Mattias Rönnblom [mailto:hofors@lysator.liu.se] > >>>>Sent: Saturday, 27 January 2024 20.15 > >>>> > >>>>On 2024-01-26 11:18, Morten Brørup wrote: > >>>>>>From: Mattias Rönnblom [mailto:hofors@lysator.liu.se] > >>>>>>Sent: Friday, 26 January 2024 11.05 > >>>>>> > >>>>>>On 2024-01-25 23:53, Morten Brørup wrote: > >>>>>>>>From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > >>>>>>>>Sent: Thursday, 25 January 2024 19.37 > >>>>>>>> > >>>>>>>>ping. > >>>>>>>> > >>>>>>>>Please review this thread if you have time, the main point of > >>>>>>>>discussion > >>>>>>>>I would like to receive consensus on the following questions. > >>>>>>>> > >>>>>>>>1. Should we continue to expand common alignments behind an > >>>>>>__rte_macro > >>>>>>>> > >>>>>>>> i.e. what do we prefer to appear in code > >>>>>>>> > >>>>>>>> alignas(RTE_CACHE_LINE_MIN_SIZE) > >>>>>>>> > >>>>>>>> -- or -- > >>>>>>>> > >>>>>>>> __rte_cache_aligned > >>>>>>>> > >>>>>>>>One of the benefits of dropping the macro is it provides a clear > >>>>>>visual > >>>>>>>>indicator that it is not placed in the same location or get > >>>>applied > >>>>>>>>to types as is done with __attribute__((__aligned__(n))). > >>>>>>> > >>>>>>>We don't want our own proprietary variant of something that already > >>>>>>exists in the C standard. Now that we have moved to C11, the __rte > >>>>>>alignment macros should be considered obsolete. > >>>>>> > >>>>>>Making so something cache-line aligned is not in C11. > >>>>> > >>>>>We are talking about the __rte_aligned() macro, not the cache > >>>>alignment macro. > >>>>> > >>>> > >>>>OK, in that case, what is the relevance of question 1 above? > >>> > >>>With this in mind, try re-reading Tyler's clarifications in this tread. > >>> > >>>Briefly: alignas() can be attached to variables and structure fields, but not to types (like __rte_aligned()), so to align a structure: > >>> > >>>struct foo { > >>> int alignas(64) bar; /* alignas(64) must be here */ > >>> int baz; > >>>}; /* __rte_aligned(64) was here, but alignas(64) cannot be here. */ > >>> > >>>So the question is: Do we want to eliminate the __rte_aligned() macro - which relies on compiler attributes - and migrate to using the C11 standard alignas()? > >>> > >>>I think yes; after updating to C11, the workaround for pre-C11 not offering alignment is obsolete, and its removal should be on the roadmap. > >>> > >> > >>OK, thanks for the explanation. Interesting limitation in the standard. > >> > >>If the construct the standard is offering is less effective (in this > >>case, less readable) and the non-standard-based option is possible > >>to implement on all compilers (i.e., on MSVC too), then we should > >>keep the custom option. Especially if it's already there, but also > >>in cases where it isn't. > >> > >>In fact, one could argue *everything* related to alignment should go > >>through something rte_, __rte_ or RTE_-prefixed. So, "int > >>RTE_ALIGNAS(64) bar;". Maybe that would be silly, but it would be > >>consistent with RTE_CACHE_ALIGNAS. > >> > >>I would worry more about allowing DPDK developers writing clean and > >>readable code, than very slightly lowering the bar for the fraction > >>of newcomers experienced with the latest and greatest from the C > >>standard, and *not* familiar with age-old GCC extensions. > > > >I’d just like to summarize where my understanding is at after reviewing > >this discussion and my downstream branch. But I also want to make it > >clear that we probably need to use both standard C and non-standard > >attribute/declspec for object and struct/union type alignment > >respectively. > > > >I've assumed we prefer avoiding per-compiler conditional expansion when > >possible through the use of standard C mechanisms. But there are > >instances when alignas is awkward. > > > >So I think the following is consistent with what Mattias is advocating > >sans any discussions related to actual naming of macros. > > > >We should have 2 macros, upon which others may be built to expand to > >well-known values for e.g. cache line size. > > > >RTE_ALIGNAS(n) object; > > > >* This macro is used to align C objects i.e. variable, array, struct/union > > fields etc. > >* Trivially expands to alignas(n) for all toolchains. > >* Placed in a location that both C and C++ translation units accept that > > is on the same line preceeding the object type. > > example: > > // RTE_ALIGNAS(n) object; > > RTE_ALIGNAS(16) char somearray[16]; > > > >RTE_ALIGN_TYPE(n) > > > >* This macro is used to align struct/union types. > >* Conditionally expands to __declspec(align(n)) (msvc) and > > __attribute__((__aligned__(n))) (for all other toolchains) > >* Placed in a location that for all gcc,clang,msvc and both C and C++ > > translation units accept. > > example: > > // {struct,union} RTE_ALIGN_TYPE(n) tag { ... }; > > struct RTE_ALIGN_TYPE(64) sometype { ... }; > > > > Sorry if I've missed some discussion on the list, but the current > pattern of putting __rte_aligned(X) at the end doesn't work with > MSVC, or why are we doing this? C11 purism doesn't seem like much of > a driving force. __rte_aligned(X) at the end doesn't work with MSVC __declspec(align(n)) > > If one defined a macro as __declspec(align(X)) on MSVC and > __attribute__(__aligned__(X)) on other compilers, could it do the > work of both the above RTE_ALIGNAS() and RTE_ALIGN_TYPE()? > > struct { int a; } ; yes for struct/union. but only when placed at location you mark as when compiling both C and C++ for all toolchains. maybe, for objects but ideally, we prefer alignas for consistent semantics defined by standard rather than accomodating potential implementation differences when conditionally expanding __aligned vs __declspec. as you have noted __declspec has limitations/variations when compared to __attribute__((__aligned__(n))). > > You would have to mandate the placement of such a __rte_aligned > plug-in replacement being at rather than (the more intuitive?) > , since clang doesn't like __attribute__s before the struct/union > keyword, correct? for struct/union there is a single placement accepted by all toolchains for both C and C++ and it is . > > What about other __attribute__ wrappers like > __rte_packed; would they also need to change placement to make DPDK > work with MSVC? packing is a different problem that needs a separate RFC and discussion of it's own. > > >I'm not picky about what the names actualy are if you have better > >suggestions i'm happy to adopt them. > > > >Thoughts? Comments? > > > >Appreciate the discussion this has been helpful. > > > >ty > >