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 7C58E439EC; Sun, 28 Jan 2024 11:00:38 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 43435402AB; Sun, 28 Jan 2024 11:00:38 +0100 (CET) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id 03E7840265 for ; Sun, 28 Jan 2024 11:00:36 +0100 (CET) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id A74F97DB3 for ; Sun, 28 Jan 2024 11:00:35 +0100 (CET) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id 98A577DB2; Sun, 28 Jan 2024 11:00:35 +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 6D1F87E0C; Sun, 28 Jan 2024 11:00:32 +0100 (CET) Message-ID: <413840c1-a263-4118-adfe-d4ae0ec0b52d@lysator.liu.se> Date: Sun, 28 Jan 2024 11:00:31 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] RFC: use C11 alignas instead of GCC attribute aligned To: =?UTF-8?Q?Morten_Br=C3=B8rup?= , Tyler Retzlaff , dev@dpdk.org Cc: =?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> Content-Language: en-US From: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35E9F1AA@smartserver.smartshare.dk> 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-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. > Continuously cleaning up old cruft in DPDK is important, especially for reducing the learning curve for newcomers to the project. > > For backwards compatibility, we can still keep the obsolete macros, but they should be removed from the project itself. > >> >>>> >>>> __rte_cache_aligned is shorter, provides a tiny bit of abstraction, >> and >>>> is already an established DPDK standard. So just keep the macro. If >> it >>>> would change, I would argue for it to be changed to >> rte_cache_aligned >>>> (i.e., just moving it out of __ namespace, and maybe making it >>>> all-uppercase). >>>> >>>> Non-trivial C programs wrap things all the time, standard or not. >> It's >>>> not something to be overly concerned about, imo. >>> >>> Using the cache alignment macro was obviously a bad example for >> discussing the __rte_aligned() macro. >>> >>> FYI, Tyler later agreed to introducing the RTE_CACHE_ALIGNAS you had >> proposed in an earlier correspondence. >>> >>>> >>>>> >>>>> Note: I don't mind convenience macros for common use cases, so we >>>> could also introduce the macro suggested by Mattias [1]: >>>>> >>>>> #define RTE_CACHE_ALIGNAS alignas(RTE_CACHE_LINE_SIZE) >>>>> >>>>> [1]: https://inbox.dpdk.org/dev/dc3f3131-38e6-4219-861e- >>>> b31ec10c08bb@lysator.liu.se/ >>>>> >>>>>> >>>>>> 2. where should we place alignas(n) or __rte_macro (if we use a >>>> macro) >>>>>> >>>>>> Should it be on the same line as the variable or field or on the >>>>>> preceeding line? >>>>>> >>>>>> /* same line example struct */ >>>>>> struct T { >>>>>> /* alignas(64) applies to field0 *not* struct T type >>>> declaration >>>>>> */ >>>>>> alignas(64) void *field0; >>>>>> void *field1; >>>>>> >>>>>> ... other fields ... >>>>>> >>>>>> alignas(64) uint64_t field5; >>>>>> uint32_t field6; >>>>>> >>>>>> ... more fields ... >>>>>> >>>>>> }; >>>>>> >>>>>> /* same line example array */ >>>>>> alignas(64) static const uint32_t array[4] = { ... }; >>>>>> >>>>>> -- or -- >>>>>> >>>>>> /* preceeding line example struct */ >>>>>> struct T { >>>>>> /* alignas(64) applies to field0 *not* struct T type >>>> declaration >>>>>> */ >>>>>> alignas(64) >>>>>> void *field0; >>>>>> void *field1; >>>>>> >>>>>> ... other fields ... >>>>>> >>>>>> alignas(64) >>>>>> uint64_t field5; >>>>>> uint32_t field6; >>>>>> >>>>>> ... more fields ... >>>>>> >>>>>> }; >>>>>> >>>>>> /* preceeding line example array */ >>>>>> alignas(64) >>>>>> static const uint32_t array[4] = { ... }; >>>>>> >>>>> >>>>> Searching the net for what other projects do, I came across this >>>> required placement [2]: >>>>> >>>>> uint64_t alignas(64) field5; >>>>> >>>>> [2]: >>>> >> https://lore.kernel.org/buildroot/20230730000851.6faa3391@windsurf/T/ >>>>> >>>>> So let's follow the standard's intention and put them on the same >>>> line. >>>>> On an case-by-case basis, we can wrap lines if it improves >>>> readability, like we do with function headers that have a lot of >>>> attributes. >>>>> >>>>>> >>>>>> I'll submit patches for lib/* once the discussion is concluded. >>>>>> >>>>>> thanks folks >>>>>