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 3313643F76; Sat, 4 May 2024 01:31:02 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C4D794025D; Sat, 4 May 2024 01:31:01 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 2BE0B40150 for ; Sat, 4 May 2024 01:31:00 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id 0B35C207DBD6; Fri, 3 May 2024 16:30:59 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 0B35C207DBD6 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1714779059; bh=xHDYk826JHa8O6DcFSIq6LAKrVYbbaQlwjwTXowIC1g=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=rYlSHfv8rF44NcEXVlLRhbRm9bsJakkpislGMAo9qEkl1YXlZtoHgR/IssTjPKelC QzjSky4jMsistIgrsNVqzXdeXbwXTALK8v9X+hBqV6pvjfxxKfXfwjiYlgTwG6US7N qcxltnLtiO+tUR+F1ngyD/X/xLmPEVtSy2Msp6oM= Date: Fri, 3 May 2024 16:30:59 -0700 From: Tyler Retzlaff To: Mattias =?iso-8859-1?Q?R=F6nnblom?= Cc: Mattias =?iso-8859-1?Q?R=F6nnblom?= , dev@dpdk.org, Heng Wang , Stephen Hemminger , Morten =?iso-8859-1?Q?Br=F8rup?= Subject: Re: [RFC v6 5/6] eal: add atomic bit operations Message-ID: <20240503233059.GA25843@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <20240430120810.108928-2-mattias.ronnblom@ericsson.com> <20240502055706.112443-1-mattias.ronnblom@ericsson.com> <20240502055706.112443-6-mattias.ronnblom@ericsson.com> <1e703819-d3b2-4127-95c2-33d32d535432@lysator.liu.se> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1e703819-d3b2-4127-95c2-33d32d535432@lysator.liu.se> 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 On Fri, May 03, 2024 at 08:41:09AM +0200, Mattias Rönnblom wrote: > On 2024-05-02 07:57, Mattias Rönnblom wrote: > >Add atomic bit test/set/clear/assign/flip and > >test-and-set/clear/assign/flip functions. > > > >All atomic bit functions allow (and indeed, require) the caller to > >specify a memory order. > > > >RFC v6: > > * Have rte_bit_atomic_test() accept const-marked bitsets. > > > >RFC v4: > > * Add atomic bit flip. > > * Mark macro-generated private functions experimental. > > > >RFC v3: > > * Work around lack of C++ support for _Generic (Tyler Retzlaff). > > > >RFC v2: > > o Add rte_bit_atomic_test_and_assign() (for consistency). > > o Fix bugs in rte_bit_atomic_test_and_[set|clear](). > > o Use to support MSVC. > > > >Signed-off-by: Mattias Rönnblom > >Acked-by: Morten Brørup > >Acked-by: Tyler Retzlaff > >--- > > lib/eal/include/rte_bitops.h | 428 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 428 insertions(+) > > > >diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h > >index caec4f36bb..9cde982113 100644 > >--- a/lib/eal/include/rte_bitops.h > >+++ b/lib/eal/include/rte_bitops.h > >@@ -21,6 +21,7 @@ > > #include > > #include > >+#include > > #ifdef __cplusplus > > extern "C" { > >@@ -399,6 +400,202 @@ extern "C" { > > uint32_t *: __rte_bit_once_flip32, \ > > uint64_t *: __rte_bit_once_flip64)(addr, nr) > >+/** > >+ * @warning > >+ * @b EXPERIMENTAL: this API may change without prior notice. > >+ * > >+ * Test if a particular bit in a word is set with a particular memory > >+ * order. > >+ * > >+ * Test a bit with the resulting memory load ordered as per the > >+ * specified memory order. > >+ * > >+ * @param addr > >+ * A pointer to the word to query. > >+ * @param nr > >+ * The index of the bit. > >+ * @param memory_order > >+ * The memory order to use. See for details. > >+ * @return > >+ * Returns true if the bit is set, and false otherwise. > >+ */ > >+#define rte_bit_atomic_test(addr, nr, memory_order) \ > >+ _Generic((addr), \ > >+ uint32_t *: __rte_bit_atomic_test32, \ > >+ const uint32_t *: __rte_bit_atomic_test32, \ > >+ uint64_t *: __rte_bit_atomic_test64, \ > >+ const uint64_t *: __rte_bit_atomic_test64)(addr, nr, \ > >+ memory_order) > > Should __rte_bit_atomic_test32()'s addr parameter be marked > volatile, and two volatile-marked branches added to the above list? off-topic comment relating to the generic type selection list above, i was reading C17 DR481 recently and i think we may want to avoid providing qualified and unauqlified types in the list. DR481: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2396.htm#dr_481 "the controlling expression of a generic selection shall have type compatibile with at most one of the types named in its generic association list." "the type of the controlling expression is the type of the expression as if it had undergone an lvalue conversion" "lvalue conversion drops type qualifiers" so the unqualified type of the controlling expression is only matched selection list which i guess that means the qualified entries in the list are never selected. i suppose the implication here is we couldn't then provide 2 inline functions one for volatile qualified and for not volatile qualified. as for a single function where the parameter is or isn't volatile qualified. if we're always forwarding to an intrinsic i've always assumed (perhaps incorrectly) that the intrinsic itself did what was semantically correct even without qualification. as you note i believe there is a convenience element in providing the volatile qualified version since it means the function like macro / inline function will accept both volatile qualified and unqualified whereas if we did not qualify the parameter it would require the caller/user to strip the volatile qualification if present with casts. i imagine in most cases we are just forwarding, in which case it seems not horrible to provide the qualified version. > Both the C11-style GCC built-ins and the C11-proper atomic functions > have addresses marked volatile. The Linux kernel and the old __sync > GCC built-ins on the other hand, doesn't (although I think you still > get volatile semantics). The only point of "volatile", as far as I > can see, is to avoid warnings in case the user passed a > volatile-marked pointer. The drawback is that *you're asking for > volatile semantics*, although with the current compilers, it seems > like that is what you get, regardless if you asked for it or not. > > Just to be clear: even these functions would accept volatile-marked > pointers, non-volatile pointers should be accepted as well (and > should generally be preferred). > > Isn't parallel programming in C lovely. it's super! > >