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 1CB9543A00; Mon, 29 Jan 2024 20:43:59 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id DF005402C8; Mon, 29 Jan 2024 20:43:58 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id D1F7740294 for ; Mon, 29 Jan 2024 20:43:56 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1086) id 315E8201F19F; Mon, 29 Jan 2024 11:43:56 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 315E8201F19F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1706557436; bh=u6oGHgk+O82YtqanLLpdaagMiOJF1GB7LnpiAvNRPJs=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=VOaLRRSqcCwUZhBoTaoTCNG8pnpZ9XGXte4zgwkvM1QPAJsu6iRmvLV/DSLr19IEl vNB1BR13aNppTfyXZQE1Do62volLSwGcfQ/o76YQ56Xgn0YyfjskjW3FtTk3GXVpvK 4JnTMvOdqj5fB6q6tLV2wS6FjqDGm4/dAUHuxjLk= Date: Mon, 29 Jan 2024 11:43:56 -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: <20240129194356.GA25654@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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <413840c1-a263-4118-adfe-d4ae0ec0b52d@lysator.liu.se> 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 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 { ... }; 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