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 8C8D2439E3; Sat, 27 Jan 2024 21:34:28 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0FEFF402D5; Sat, 27 Jan 2024 21:34:28 +0100 (CET) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id 25CA540291 for ; Sat, 27 Jan 2024 21:34:27 +0100 (CET) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id DEFC85D3F for ; Sat, 27 Jan 2024 21:34:26 +0100 (CET) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id D2F795DBC; Sat, 27 Jan 2024 21:34:26 +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 D2D9C5CFA; Sat, 27 Jan 2024 21:34:24 +0100 (CET) Message-ID: Date: Sat, 27 Jan 2024 21:34:24 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: rte_atomic_*_explicit To: Tyler Retzlaff , =?UTF-8?Q?Morten_Br=C3=B8rup?= Cc: dev@dpdk.org, Tyler Retzlaff , konstantin.v.ananyev@yandex.ru, =?UTF-8?Q?Mattias_R=C3=B6nnblom?= , honnappa.nagarahalli@arm.com References: <0e268757-8368-456f-ba2f-10a1969c498f@lysator.liu.se> <98CBD80474FA8B44BF855DF32C47DC35E9F1A2@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35E9F1A8@smartserver.smartshare.dk> <20240126213520.GB22822@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> Content-Language: en-US From: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= In-Reply-To: <20240126213520.GB22822@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-26 22:35, Tyler Retzlaff wrote: > On Fri, Jan 26, 2024 at 11:52:11AM +0100, Morten Brørup wrote: >>> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se] >>> Sent: Friday, 26 January 2024 09.07 >>> >>> On 2024-01-25 23:10, Morten Brørup wrote: >>>>> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se] >>>>> Sent: Thursday, 25 January 2024 19.54 >>>>> >>>>> Why do rte_stdatomic.h functions have the suffix "_explicit"? >>>>> Especially >>>>> since there aren't any wrappers for the implicit variants. >>>>> >>>>> More to type, more to read. >>>> >>>> They have the "_explicit" suffix to make their names similar to those >>> in stdatomic.h. >>>> >>> >>> OK, so to avoid a situation where someone accidentally misinterpret >>> rte_atomic_fetch_add(&counter, 1, rte_memory_order_relaxed); >>> as what, exactly? >>> >>> If you have a wrapper, why not take the opportunity and reap some >>> benefits from this and fix/extend the standard API, making it a better >>> fit for your application. Like removing the redundant "_explicit", >>> "fetch"-less add/sub, maybe and a function for single-writer atomic add >>> (non-atomic read + non-atomic add + atomic store), etc. >>> >>> Prohibiting implicit operations was done already, so it's already now >>> not a straight-off copy of the standard API. >>> >>>> You might consider their existence somewhat temporary until C11 >>> stdatomics can be fully phased in, so there's another argument for >>> similar names. (This probably does not happen as long as compilers >>> generate slower code for C11 stdatomics than with their atomic built- >>> ins.) >>>> >>> >>> To me, it seems reasonable a wrapper API should stay as long as it >>> provide a benefit over the standard API. One could imagine that being a >>> long time. >>> >>> I imagine some DPDK developers being tired of migrating from one >>> atomics >>> API to another. -> GCC built-ins (-> attempted C11 >>> stdatomics?) -> . Now you are adding a future "-> CXY >>> atomics" move as well, and with that it also seems natural to add a >>> change back to a wrapper or complementary API, when CXY didn't turned >>> out good enough for some particular platform, or when some non- >>> complaint >>> compiler comes along. >> >> Yes, more migrations seem to be on the roadmap. >> >> We can take the opportunity to change direction now, and decide to keep the API long term. >> Then it would need more documentation (basically copying function descriptions from ), and the functions could have the "_explicit" suffix removed (macros with the suffix could be added for backwards compatibility), and more functions - like the ones you suggested above - could be added. >> >> What do people think? >> 1. Keep considering a temporary wrapper for until compilers reach some undefined level of maturity, or >> 2. Consider stable, clean it up (remove "_explicit" suffix), add documentation to the macros, and extend it. >> >> Living in a DPDK-only environment, I would prefer option 2; but if mixing DPDK code with non-DPDK code (that uses ) it might be weird. > > rte_stdatomic.h should be considered temporary, but how long temporary > is depends on when we can deprecate support for distributions and the > older toolchains they are tied to. > > the macros were introduced to allow a path to gradually moving to > standard c11 atomics. gcc versions available on distributions we > promise support for is currently the biggest barrier to direct > adoption. > > * older versions of gcc generate sub-optimal code when using c11 > atomic generics in some instances. > > * gcc c++23 support is incomplete, in particular at the time we > evaluated using the c11 atomics directly but gcc c++23 held us > back because the stdatomic.h it shipped didn't interoperate. > So you expect at a particular point in time, both the following will be true: * All currently (at that point) supported and future compilers generate correct and efficient code for all currently and future ISA/CPUs. * There's nothing add, subtract or clean up in the standard API to improve it for DPDK-internal and DPDK application code use. Seeing things like rte_atomic_fetch_add_explicit(&s->num_mapped_cores, 1, rte_memory_order_relaxed); doesn't exactly make you long for "raw" C11 atomics. It's not the like "rte_" being deleted improve much on that mess. Compare it with the kernel's: atomic_inc(&s->num_mapped_cores); What could in DPDK be: rte_atomic_inc(&s->num_mapped_cores); or, at a bare minimum rte_atomic_inc(&->num_mapped_cores, rte_memory_order_relaxed); > * the macros allow developers to easily evaluate/compare with and > without standard C atomics with a single build-time option > -Denable_stdatomic=true > > * eventually we will want to move directly to the names from the > standard, arguably just search and replace of rte_atomic with > atomic is the most mechanically trivial - hence there is some > value in keeping _explicit suffix. > is a public API, and thus will be picked up by applications wanting to leverage DPDK for such services. Search-and-replace will only affect DPDK itself. >>> >>> I suggested fixing the original API, or at least have a >>> wrapper API, already at the point DPDK moved to direct GCC built-in >>> calls. Then we wouldn't have had this atomics API ping-pong. >> >> The decision back then might have been too hasty, and based on incomplete assumptions. >> Please shout louder next time you think a mistake is in the making. >> >>> >>>>> >>>>> When was this API introduced? Shouldn't it say "experimental" >>>>> somewhere? >>>> >>>> They were introduced as part of the migration to C11. >>>> I suppose they were not marked experimental because they replaced >>> something we didn't want anymore (the compiler built-ins for atomics, >>> e.g. __atomic_load_n()). I don't recall if we discussed experimental >>> marking or not. >> >> In hindsight, they should probably have been marked "experimental". > > i'm not sure i feel strongly that they need to be marked experimental > and by marked i assume we're only talking about placing a comment in a > file rather than __rte_experimental which has no application here. > > typically we do that when we may want to change or remove the api without > going through the normal deprecation and removal process. for these > macros it is difficult to imagine why we would change them as that would > only cause them to deviate from the signature or behavior of the > standard C generics... why would we do that if our intention is to > eventually fully migrate to direct use of the standard C names? > > ty