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 DC36643C3D; Tue, 5 Mar 2024 21:53:34 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A212F402AC; Tue, 5 Mar 2024 21:53:34 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 3818F40270 for ; Tue, 5 Mar 2024 21:53:33 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1086) id 5017B20B74C0; Tue, 5 Mar 2024 12:53:32 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 5017B20B74C0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1709672012; bh=z8Zhy7Y08mJJbYrD/GQNy8sRvtNEcOQrIP85sE3SgBQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ZjGis7jXtqChg08l2Tn9h4T70K2iGjHwY46H23PexgkDz+r8xjLRxT/JUg8VExfsj B7NUddsi7yRq2LqBQeygufbga2ie/fO5gVaZdK5p3IBFVHL5ULU7swQr494RKDqKHJ T5wQfaoeBrNpmKx4/TKg9Kd0TvFZ/B4Mg9v2+P7M= Date: Tue, 5 Mar 2024 12:53:32 -0800 From: Tyler Retzlaff To: Mattias =?iso-8859-1?Q?R=F6nnblom?= Cc: Mattias =?iso-8859-1?Q?R=F6nnblom?= , dev@dpdk.org, Heng Wang Subject: Re: [RFC 2/7] eal: add generic bit manipulation macros Message-ID: <20240305205332.GA28153@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <20240302135328.531940-1-mattias.ronnblom@ericsson.com> <20240302135328.531940-3-mattias.ronnblom@ericsson.com> <20240304164224.GB16065@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <5c43fd54-6e59-4882-9efa-f3b3dff98b11@lysator.liu.se> <20240305182221.GA24679@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <79dbc7d5-a519-4349-9652-6c458e11b0c0@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: <79dbc7d5-a519-4349-9652-6c458e11b0c0@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 Tue, Mar 05, 2024 at 09:02:34PM +0100, Mattias Rönnblom wrote: > On 2024-03-05 19:22, Tyler Retzlaff wrote: > >On Tue, Mar 05, 2024 at 07:08:36PM +0100, Mattias Rönnblom wrote: > >>On 2024-03-04 17:42, Tyler Retzlaff wrote: > >>>On Sat, Mar 02, 2024 at 02:53:23PM +0100, Mattias Rönnblom wrote: > >>>>Add bit-level test/set/clear/assign macros operating on both 32-bit > >>>>and 64-bit words by means of C11 generic selection. > >>>> > >>>>Signed-off-by: Mattias Rönnblom > >>>>--- > >>> > >>>_Generic is nice here. should we discourage direct use of the inline > >>>functions in preference of using the macro always? either way lgtm. > >>> > >> > >>That was something I considered, but decided against it for RFC v1. > >>I wasn't even sure people would like _Generic. > >> > >>The big upside of having only the _Generic macros would be a much > >>smaller API, but maybe a tiny bit less (type-)safe to use. > > > >i'm curious what misuse pattern you anticipate or have seen that may be > >less type-safe? just so i can look out for them. > > > > That was just a gut feeling, not to be taken too seriously. > > uint32_t *p = some_void_pointer; > /../ > rte_bit_set32(p, 17); > > A code section like this is redundant in the way the type (or at > least type size) is coded both into the function name, and the > pointer type. The use of rte_set_bit() will eliminate this, which is > good (DRY), and bad, because now the type isn't "double-checked". > > As you can see, it's a pretty weak argument. > > >i (perhaps naively) have liked generic functions for their selection of > >the "correct" type and for _Generic if no leg/case exists compiler > >error (as opposed to e.g. silent truncation). > > > >> > >>Also, _Generic is new for DPDK, so who knows what issues it might > >>cause with old compilers. > > > >i was thinking about this overnight, it's supposed to be standard C11 > >and my use on various compilers showed no problem but I can't recall if > >i did any evaluation when consuming as a part of a C++ translation unit > >so there could be problems. > > > > It would be unfortunate if DPDK was prohibited from using _Generic. I agree, I don't think it should be prohibited. If C++ poses a problem we can work to find solutions. > > >> > >>Thanks. > >> > >>>Acked-by: Tyler Retzlaff > >>> > >>>> lib/eal/include/rte_bitops.h | 81 ++++++++++++++++++++++++++++++++++++ > >>>> 1 file changed, 81 insertions(+) > >>>> > >>>>diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h > >>>>index 9a368724d5..afd0f11033 100644 > >>>>--- a/lib/eal/include/rte_bitops.h > >>>>+++ b/lib/eal/include/rte_bitops.h > >>>>@@ -107,6 +107,87 @@ extern "C" { > >>>> #define RTE_FIELD_GET64(mask, reg) \ > >>>> ((typeof(mask))(((reg) & (mask)) >> rte_ctz64(mask))) > >>>>+/** > >>>>+ * Test bit in word. > >>>>+ * > >>>>+ * Generic selection macro to test the value of a bit in a 32-bit or > >>>>+ * 64-bit word. The type of operation depends on the type of the @c > >>>>+ * addr parameter. > >>>>+ * > >>>>+ * This macro does not give any guarantees in regards to memory > >>>>+ * ordering or atomicity. > >>>>+ * > >>>>+ * @param addr > >>>>+ * A pointer to the word to modify. > >>>>+ * @param nr > >>>>+ * The index of the bit. > >>>>+ */ > >>>>+#define rte_bit_test(addr, nr) \ > >>>>+ _Generic((addr), \ > >>>>+ uint32_t *: rte_bit_test32, \ > >>>>+ uint64_t *: rte_bit_test64)(addr, nr) > >>>>+ > >>>>+/** > >>>>+ * Set bit in word. > >>>>+ * > >>>>+ * Generic selection macro to set a bit in a 32-bit or 64-bit > >>>>+ * word. The type of operation depends on the type of the @c addr > >>>>+ * parameter. > >>>>+ * > >>>>+ * This macro does not give any guarantees in regards to memory > >>>>+ * ordering or atomicity. > >>>>+ * > >>>>+ * @param addr > >>>>+ * A pointer to the word to modify. > >>>>+ * @param nr > >>>>+ * The index of the bit. > >>>>+ */ > >>>>+#define rte_bit_set(addr, nr) \ > >>>>+ _Generic((addr), \ > >>>>+ uint32_t *: rte_bit_set32, \ > >>>>+ uint64_t *: rte_bit_set64)(addr, nr) > >>>>+ > >>>>+/** > >>>>+ * Clear bit in word. > >>>>+ * > >>>>+ * Generic selection macro to clear a bit in a 32-bit or 64-bit > >>>>+ * word. The type of operation depends on the type of the @c addr > >>>>+ * parameter. > >>>>+ * > >>>>+ * This macro does not give any guarantees in regards to memory > >>>>+ * ordering or atomicity. > >>>>+ * > >>>>+ * @param addr > >>>>+ * A pointer to the word to modify. > >>>>+ * @param nr > >>>>+ * The index of the bit. > >>>>+ */ > >>>>+#define rte_bit_clear(addr, nr) \ > >>>>+ _Generic((addr), \ > >>>>+ uint32_t *: rte_bit_clear32, \ > >>>>+ uint64_t *: rte_bit_clear64)(addr, nr) > >>>>+ > >>>>+/** > >>>>+ * Assign a value to a bit in word. > >>>>+ * > >>>>+ * Generic selection macro to assign a value to a bit in a 32-bit or 64-bit > >>>>+ * word. The type of operation depends on the type of the @c addr parameter. > >>>>+ * > >>>>+ * This macro does not give any guarantees in regards to memory > >>>>+ * ordering or atomicity. > >>>>+ * > >>>>+ * @param addr > >>>>+ * A pointer to the word to modify. > >>>>+ * @param nr > >>>>+ * The index of the bit. > >>>>+ * @param value > >>>>+ * The new value of the bit - true for '1', or false for '0'. > >>>>+ */ > >>>>+#define rte_bit_assign(addr, nr, value) \ > >>>>+ _Generic((addr), \ > >>>>+ uint32_t *: rte_bit_assign32, \ > >>>>+ uint64_t *: rte_bit_assign64)(addr, nr, value) > >>>>+ > >>>> /** > >>>> * Test if a particular bit in a 32-bit word is set. > >>>> * > >>>>-- > >>>>2.34.1