From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 3A8D1A052B; Tue, 28 Jul 2020 11:29:28 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 819211C0D9; Tue, 28 Jul 2020 11:29:16 +0200 (CEST) Received: from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net [217.70.183.197]) by dpdk.org (Postfix) with ESMTP id 5C3381C0CF for ; Tue, 28 Jul 2020 11:29:14 +0200 (CEST) X-Originating-IP: 90.92.205.40 Received: from u256.net (lfbn-idf2-1-1144-40.w90-92.abo.wanadoo.fr [90.92.205.40]) (Authenticated sender: grive@u256.net) by relay5-d.mail.gandi.net (Postfix) with ESMTPSA id AFFD81C0003; Tue, 28 Jul 2020 09:29:11 +0000 (UTC) Date: Tue, 28 Jul 2020 11:29:06 +0200 From: =?utf-8?Q?Ga=C3=ABtan?= Rivet To: Morten =?utf-8?Q?Br=C3=B8rup?= Cc: Honnappa Nagarahalli , Parav Pandit , dev@dpdk.org, ferruh.yigit@intel.com, thomas@monjalon.net, Ray Kinsella , Neil Horman , rasland@mellanox.com, orika@mellanox.com, matan@mellanox.com, Joyce Kong , nd Message-ID: <20200728092906.3qmnncichcettmxr@u256.net> References: <20200610171728.89-2-parav@mellanox.com> <20200724143906.7453-1-parav@mellanox.com> <20200724143906.7453-2-parav@mellanox.com> <98CBD80474FA8B44BF855DF32C47DC35C61160@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35C61169@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35C61169@smartserver.smartshare.dk> Subject: Re: [dpdk-dev] [PATCH v10 01/10] eal: introduce macro for bit definition X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 28/07/20 10:24 +0200, Morten Brørup wrote: > + Ray and Neil as ABI Policy maintainers. > > > From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli@arm.com] > > Sent: Tuesday, July 28, 2020 4:19 AM > > > > > > > > > > > > > Subject: [dpdk-dev] [PATCH v10 01/10] eal: introduce macro for > > bit > > > > definition > > > > > > > > > > There are several drivers which duplicate bit generation macro. > > > > > Introduce a generic bit macros so that such drivers avoid > > redefining > > > > same in > > > > > multiple drivers. > > > > > > > > > > Signed-off-by: Parav Pandit > > > > > Acked-by: Matan Azrad > > > > > Acked-by: Morten Brørup > > > > > --- > > > > > Changelog: > > > > > v4->v5: > > > > > - Addressed comments from Morten Brørup > > > > > - Renamed newly added macro to RTE_BIT64 > > > > > - Added doxygen comment section for the macro > > > > > v1->v2: > > > > > - Addressed comments from Thomas and Gaten. > > > > > - Avoided new file, added macro to rte_bitops.h > > > > > --- > > > > > lib/librte_eal/include/rte_bitops.h | 8 ++++++++ > > > > > 1 file changed, 8 insertions(+) > > > > > > > > > > diff --git a/lib/librte_eal/include/rte_bitops.h > > > > > b/lib/librte_eal/include/rte_bitops.h > > > > > index 740927f3b..ca46a110f 100644 > > > > > --- a/lib/librte_eal/include/rte_bitops.h > > > > > +++ b/lib/librte_eal/include/rte_bitops.h > > > > > @@ -17,6 +17,14 @@ > > > > > #include > > > > > #include > > > > > > > > > > +/** > > > > > + * Get the uint64_t value for a specified bit set. > > > > > + * > > > > > + * @param nr > > > > > + * The bit number in range of 0 to 63. > > > > > + */ > > > > > +#define RTE_BIT64(nr) (UINT64_C(1) << (nr)) > > > > In general, the macros have been avoided in this file. Suggest > > > > changing this to an inline function. > > > > > > That has been discussed already, and rejected for good reasons: > > > http://inbox.dpdk.org/dev/AM0PR05MB4866823B0170B90F679A2765D1640@ > > > AM0PR05MB4866.eurprd05.prod.outlook.com/ > > Thank you for the link. > > In this patch series, I see the macro being used in enum initialization > > (7/10 in v11) as well as in functions (8/10 in v11). Does it make sense > > to introduce use inline functions and use the inline functions for > > 8/10? > > If we do this, we should document in rte_bitops.h that inline functions > > should be used wherever possible. > > I would agree, but only in theory. I disagree in reality, and argue that there should only be macros for this. Here is why: > > rte_byteorder.h has both RTE_BEnn() macros and rte_cpu_to_be_nn() functions, for doing the same thing at compile time or at run time. There are no compile time warnings if the wrong one is being used, so I am certain that we can find code that uses the macro where the function should be used, or vice versa. > Hi, It is not clear to me, reading this thread, what is the motivation to enforce use of inline functions? Is it perf, compiler type checking, or usage checks? Macros are checked at compile time when possible, though it can be improved upon. But I agree with Morten, proposing two forms ensures devs will sometimes use the wrong one, and we would need a practical way to check usages. > Which opens another, higher level, question: Would it be possible to add a compile time check macro in rte_common.h for these and similar? > Can you clarify your idea? Is is something similar to: #define _BIT64(n) (UINT64_C(1) << (n)) static inline uint64_t bit64(uint64_t n) { assert(n < 64); return (UINT64_C(1) << n); } /* Integer Constant Expression? */ #define ICE_P(x) (sizeof(int) == sizeof(*(1 ? ((void*)((x) * 0l)) : (int*)1))) #define BIT64(n) (ICE_P(n) ? _BIT64(n) : bit64(n)) I don't think so, but this is as close as automatic compile-time check and automatic use of proper macro vs. function I know of, did you have something else in mind? In this kind of code: #include #include #include #include enum vals { ZERO = 0, ONE = BIT64(1), TWO = BIT64(2), THREE = BIT64(3), }; int main(void) { uint64_t x = ONE; x = BIT64(0); x = BIT64(1); x = BIT64(60); x = BIT64(64); x = BIT64(x); printf("x: 0x%" PRIx64 "\n", x); return 0; } The enum is defined using the macro, x = BIT64(64); triggers the following warning with GCC: constant_bitop.c:6:32: warning: left shift count >= width of type [-Wshift-count-overflow] 6 | #define _BIT64(n) (UINT64_C(1) << (n)) and x = BIT64(x); triggers the assert() at runtime. > Furthermore: For the RTE_BITnn() operations in this patch set, I expect the compiler to generate perfectly efficient code using the macro for run time use. I.e. there would be no performance advantage by also implementing the macros as functions for run time use. > Regards, -- Gaëtan