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 4DF6D41B14; Tue, 29 Aug 2023 17:57:36 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E0BB940293; Tue, 29 Aug 2023 17:57:35 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id BB6DC40279; Tue, 29 Aug 2023 17:57:33 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id C1E262129BF3; Tue, 29 Aug 2023 08:57:32 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com C1E262129BF3 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1693324652; bh=YDbJITZD3FQef5KIyFblNHqiZrOxh5fwml12XjSJgHA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=OJt/Xz9UFTMrM7cIawhxhqzC5+GH5aW/aB9zPWWZm9bkIBHt6lmgmOfJzTyJRerDC JAIQcxv3qUluPVqK/Xge6vrGHR+hwP5SmVA/Hj49c2mEJvGhMRjkXdZP9cA1jPuBLe RBzqm/+yLfJITjJchoqI5bi5oZGa3roowYgZUmx8= Date: Tue, 29 Aug 2023 08:57:32 -0700 From: Tyler Retzlaff To: dev@dpdk.org Cc: techboard@dpdk.org, Bruce Richardson , Honnappa Nagarahalli , Ruifeng Wang , Jerin Jacob , Sunil Kumar Kori , Mattias =?iso-8859-1?Q?R=F6nnblom?= , Joyce Kong , David Christensen , Konstantin Ananyev , David Hunt , Thomas Monjalon , David Marchand Subject: Re: [PATCH v6 0/6] rte atomics API for optional stdatomic Message-ID: <20230829155732.GA19416@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1691717521-1025-1-git-send-email-roretzla@linux.microsoft.com> <1692738045-32363-1-git-send-email-roretzla@linux.microsoft.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1692738045-32363-1-git-send-email-roretzla@linux.microsoft.com> 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 ping for additional reviewers. thanks! On Tue, Aug 22, 2023 at 02:00:39PM -0700, Tyler Retzlaff wrote: > This series introduces API additions prefixed in the rte namespace that allow > the optional use of stdatomics.h from C11 using enable_stdatomics=true for > targets where enable_stdatomics=false no functional change is intended. > > Be aware this does not contain all changes to use stdatomics across the DPDK > tree it only introduces the minimum to allow the option to be used which is > a pre-requisite for a clean CI (probably using clang) that can be run > with enable_stdatomics=true enabled. > > It is planned that subsequent series will be introduced per lib/driver as > appropriate to further enable stdatomics use when enable_stdatomics=true. > > Notes: > > * Additional libraries beyond EAL make visible atomics use across the > API/ABI surface they will be converted in the subsequent series. > > * The eal: add rte atomic qualifier with casts patch needs some discussion > as to whether or not the legacy rte_atomic APIs should be converted to > work with enable_stdatomic=true right now some implementation dependent > casts are used to prevent cascading / having to convert too much in > the intial series. > > * Windows will obviously need complete conversion of libraries including > atomics that are not crossing API/ABI boundaries. those conversions will > introduced in separate series as new along side the existing msvc series. > > Please keep in mind we would like to prioritize the review / acceptance of > this patch since it needs to be completed in the 23.11 merge window. > > Thank you all for the discussion that lead to the formation of this series. > > v6: > * Adjust checkpatches to warn about use of __rte_atomic_thread_fence > and suggest use of rte_atomic_thread_fence. Use the existing check > more generic check for __atomic_xxx to catch use of __atomic_thread_fence > and recommend rte_atomic_xxx. > > v5: > * Add RTE_ATOMIC to doxygen configuration PREDEFINED macros list to > fix documentation generation failure > * Fix two typos in expansion of C11 atomics macros strong -> weak and > add missing _explicit > * Adjust devtools/checkpatches messages based on feedback. i have chosen > not to try and catch use of C11 atomics or _Atomic since using those > directly will be picked up by existing CI passes where by compilation > error where enable_stdatomic=false (the default for most platforms) > > v4: > * Move the definition of #define RTE_ATOMIC(type) to patch 1 where it > belongs (a mistake in v3) > * Provide comments for both RTE_ATOMIC and __rte_atomic macros indicating > their use as specified or qualified contexts. > > v3: > * Remove comments from APIs mentioning the mapping to C++ memory model > memory orders > * Introduce and use new macro RTE_ATOMIC(type) to be used in contexts > where _Atomic is used as a type specifier to declare variables. The > macro allows more clarity about what the atomic type being specified > is. e.g. _Atomic(T *) vs _Atomic(T) it is easier to understand that > the former is an atomic pointer type and the latter is an atomic > type. it also has the benefit of (in the future) being interoperable > with c++23 syntactically > note: Morten i have retained your 'reviewed-by' tags if you disagree > given the changes in the above version please indicate as such but > i believe the changes are in the spirit of the feedback you provided > > v2: > * Wrap meson_options.txt option description to newline and indent to > be consistent with other options. > * Provide separate typedef of rte_memory_order for enable_stdatomic=true > VS enable_stdatomic=false instead of a single typedef to int > note: slight tweak to reviewers feedback i've chosen to use a typedef > for both enable_stdatomic={true,false} (just seemed more consistent) > * Bring in assert.h and use static_assert macro instead of _Static_assert > keyword to better interoperate with c/c++ > * Directly include rte_stdatomic.h where into other places it is consumed > instead of hacking it globally into rte_config.h > * Provide and use __rte_atomic_thread_fence to allow conditional expansion > within the body of existing rte_atomic_thread_fence inline function to > maintain per-arch optimizations when enable_stdatomic=false > > Tyler Retzlaff (6): > eal: provide rte stdatomics optional atomics API > eal: adapt EAL to present rte optional atomics API > eal: add rte atomic qualifier with casts > distributor: adapt for EAL optional atomics API changes > bpf: adapt for EAL optional atomics API changes > devtools: forbid new direct use of GCC atomic builtins > > app/test/test_mcslock.c | 6 +- > config/meson.build | 1 + > devtools/checkpatches.sh | 12 +- > doc/api/doxy-api.conf.in | 1 + > lib/bpf/bpf_pkt.c | 6 +- > lib/distributor/distributor_private.h | 2 +- > lib/distributor/rte_distributor_single.c | 44 +++---- > lib/eal/arm/include/rte_atomic_32.h | 4 +- > lib/eal/arm/include/rte_atomic_64.h | 36 +++--- > lib/eal/arm/include/rte_pause_64.h | 26 ++-- > lib/eal/arm/rte_power_intrinsics.c | 8 +- > lib/eal/common/eal_common_trace.c | 16 +-- > lib/eal/include/generic/rte_atomic.h | 67 +++++++---- > lib/eal/include/generic/rte_pause.h | 50 ++++---- > lib/eal/include/generic/rte_rwlock.h | 48 ++++---- > lib/eal/include/generic/rte_spinlock.h | 20 ++-- > lib/eal/include/meson.build | 1 + > lib/eal/include/rte_mcslock.h | 51 ++++---- > lib/eal/include/rte_pflock.h | 25 ++-- > lib/eal/include/rte_seqcount.h | 19 +-- > lib/eal/include/rte_stdatomic.h | 198 +++++++++++++++++++++++++++++++ > lib/eal/include/rte_ticketlock.h | 43 +++---- > lib/eal/include/rte_trace_point.h | 5 +- > lib/eal/loongarch/include/rte_atomic.h | 4 +- > lib/eal/ppc/include/rte_atomic.h | 54 ++++----- > lib/eal/riscv/include/rte_atomic.h | 4 +- > lib/eal/x86/include/rte_atomic.h | 8 +- > lib/eal/x86/include/rte_spinlock.h | 2 +- > lib/eal/x86/rte_power_intrinsics.c | 7 +- > meson_options.txt | 2 + > 30 files changed, 501 insertions(+), 269 deletions(-) > create mode 100644 lib/eal/include/rte_stdatomic.h > > -- > 1.8.3.1