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 8086843A21; Wed, 31 Jan 2024 17:05:05 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1DC354026B; Wed, 31 Jan 2024 17:05:05 +0100 (CET) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id 22CBB40269 for ; Wed, 31 Jan 2024 17:05:03 +0100 (CET) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id DD3BB17B57 for ; Wed, 31 Jan 2024 17:05:02 +0100 (CET) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id CBB5017A72; Wed, 31 Jan 2024 17:05:02 +0100 (CET) X-Spam-Checker-Version: SpamAssassin 4.0.0 (2022-12-13) on hermod.lysator.liu.se X-Spam-Level: X-Spam-Status: No, score=-1.4 required=5.0 tests=ALL_TRUSTED,AWL, T_SCC_BODY_TEXT_LINE autolearn=disabled version=4.0.0 X-Spam-Score: -1.4 Received: from [192.168.1.59] (h-62-63-215-114.A163.priv.bahnhof.se [62.63.215.114]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id 5F12517A6F; Wed, 31 Jan 2024 17:04:59 +0100 (CET) Message-ID: Date: Wed, 31 Jan 2024 17:04:59 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] RFC: use C11 alignas instead of GCC attribute aligned Content-Language: en-US To: Tyler Retzlaff Cc: =?UTF-8?Q?Morten_Br=C3=B8rup?= , dev@dpdk.org, =?UTF-8?Q?Mattias_R=C3=B6nnblom?= , Anatoly Burakov , Bruce Richardson , David Christensen , Harry van Haaren , Konstantin Ananyev , Min Zhou , Ruifeng Wang , Stanislaw Kardach , thomas@monjalon.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> <20240130173928.GA2943@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> From: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= In-Reply-To: <20240130173928.GA2943@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Virus-Scanned: ClamAV using ClamSMTP 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 2024-01-30 18:39, Tyler Retzlaff wrote: > 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. > Seems like the same kind of problem with potentially the exact same solution: mandate a new "__rte_xxx" placement and use MSVC __declspec. Different RFC, yes, different discussion: not so sure. >> >>> 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 >>>