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 AAC44430C6; Tue, 22 Aug 2023 00:27:20 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 28D0840DF6; Tue, 22 Aug 2023 00:27:20 +0200 (CEST) Received: from forward500c.mail.yandex.net (forward500c.mail.yandex.net [178.154.239.208]) by mails.dpdk.org (Postfix) with ESMTP id 1A10D4021D; Tue, 22 Aug 2023 00:27:18 +0200 (CEST) Received: from mail-nwsmtp-smtp-production-main-33.iva.yp-c.yandex.net (mail-nwsmtp-smtp-production-main-33.iva.yp-c.yandex.net [IPv6:2a02:6b8:c0c:1f21:0:640:9366:0]) by forward500c.mail.yandex.net (Yandex) with ESMTP id 292B25E7C5; Tue, 22 Aug 2023 01:27:16 +0300 (MSK) Received: by mail-nwsmtp-smtp-production-main-33.iva.yp-c.yandex.net (smtp/Yandex) with ESMTPSA id BRRwDDhBT4Y0-IrtjaQFb; Tue, 22 Aug 2023 01:27:15 +0300 X-Yandex-Fwd: 1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yandex.ru; s=mail; t=1692656835; bh=C0PaA4nCt9bReyXzS7VSYahCGEd25q3U5/NrxX42hBY=; h=From:In-Reply-To:Cc:Date:References:To:Subject:Message-ID; b=SXWZCdF0lXMfYB2wdcpr9N4xZ1fxz5dX3Q69UaHjpQnL8PlsqqYcyCsoxR1N0ViJf KONGWkNASnsc6PegRvq4hdde8qzbMddDXCyVGJvoInVwA5G75u9wfuFQpq+Lz+jgwM igBTpLyJy6hu+geuylIvMruzrnlh+fJhqc3W3JwA= Authentication-Results: mail-nwsmtp-smtp-production-main-33.iva.yp-c.yandex.net; dkim=pass header.i=@yandex.ru Message-ID: <715db831-dd5f-3d74-eaaa-dc490e6c71a9@yandex.ru> Date: Mon, 21 Aug 2023 23:27:11 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [PATCH v5 0/6] optional rte optional stdatomics API Content-Language: en-US To: Tyler Retzlaff , dev@dpdk.org Cc: techboard@dpdk.org, Bruce Richardson , Honnappa Nagarahalli , Ruifeng Wang , Jerin Jacob , Sunil Kumar Kori , =?UTF-8?Q?Mattias_R=c3=b6nnblom?= , Joyce Kong , David Christensen , David Hunt , Thomas Monjalon , David Marchand References: <1691717521-1025-1-git-send-email-roretzla@linux.microsoft.com> <1692308537-2646-1-git-send-email-roretzla@linux.microsoft.com> From: Konstantin Ananyev In-Reply-To: <1692308537-2646-1-git-send-email-roretzla@linux.microsoft.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 > 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. > > 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 | 8 +- > 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, 499 insertions(+), 267 deletions(-) > create mode 100644 lib/eal/include/rte_stdatomic.h > Series-acked-by: Konstantin Ananyev