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 B3A4243F7C; Thu, 9 May 2024 13:02:57 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3651A402D7; Thu, 9 May 2024 13:02:57 +0200 (CEST) Received: from mail-oa1-f47.google.com (mail-oa1-f47.google.com [209.85.160.47]) by mails.dpdk.org (Postfix) with ESMTP id 73E4C402B7 for ; Thu, 9 May 2024 13:02:55 +0200 (CEST) Received: by mail-oa1-f47.google.com with SMTP id 586e51a60fabf-23f5a31d948so422124fac.0 for ; Thu, 09 May 2024 04:02:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bytedance.com; s=google; t=1715252574; x=1715857374; darn=dpdk.org; h=user-agent:in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date:from:to:cc :subject:date:message-id:reply-to; bh=K0CLU0p97BvP4oeyrQpkYXDd7aQI7b0EbNEL2ARl8Ec=; b=iAxSAIKKSlQzDNilnmEV/pttZPURSgwxXKAEuISILlkdJ09Unc8j/8C6yTkpaJNxJN z6xsZ8G+du2boHXDF53f45a79kfDWovWlochkLCV6Dta+S2AlkZ1NfBXp5KvcOb7VG6A rJvalmd/VOy84G1Fxg6yVsAWZkw4YjP4kaVrPJmyIY+M7YFc07MA1phikJCbNZtdmAgT vgpP26ljQkmSSYfi4At1Sh0k311VHUwNYhw/2QB4BHOcPqWtNrqoDgvM4GRY4cXxVkD/ 5FYOnMYcc9ISU06T2fHxu8cqum4oY+pBWsgH7dRlcqvJLbISqyWzoO7Iya3PMzK0zFMG YYWg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1715252574; x=1715857374; h=user-agent:in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=K0CLU0p97BvP4oeyrQpkYXDd7aQI7b0EbNEL2ARl8Ec=; b=FKqh7kqgxs3NquSvxTAyX1h78SE2L5uaT0Db4+5RTAA7P2QW4+t7UEtsGyeCyOmPqa gqZN9bemKKKf130lnRrPN0Vi8vcE5JUYKXz37ZVZ4HaW/uQYBYuOmibss3s7nIOjXP79 lWGMEPvvp5r4crMKoVy2tQD63p7i+4Bfekz5Em8uKk4xeVeBq/IOx2XBy5fu7jBBm+aP OT90RaCENIjKwNuaPU3CEgN8pPxJyhZX+cI6OZi9MSXQ+WwwcIQOZNRVQ3nQisrPkgw/ arF8iN9wACN0NpiahH6szbfeO+M8YRpoY/DdjVviUfn4Gr3X5WnYKQbSJe8PETbIDA8X v5ww== X-Forwarded-Encrypted: i=1; AJvYcCUlaRj1zrxRBxZgqLsxlnHnhjVW6PweDA/oOrGEDY/MiIBiYt1WcOct8W8U/E9YwzzkDIb6lHn7LSja4+A= X-Gm-Message-State: AOJu0Yyv5Ku0X9GeWXdIackbwcaGOTruwLRy2XEl2vXFRazjxK7uV658 Vb3F1U58iqKpY72sHDDhVIcM5tp3Noz9EzvjBdOeibsF+Tx66sQHyFadMcRb+00= X-Google-Smtp-Source: AGHT+IHWLQWgPnOnNqTa/L69sJFC/Bc7+IrIHb8Y82rNnuwOMb+UieVSsDGSEsmZSnwVgx/kVesX4Q== X-Received: by 2002:a05:6870:f721:b0:229:f988:4305 with SMTP id 586e51a60fabf-240985a481amr5762687fac.10.1715252574461; Thu, 09 May 2024 04:02:54 -0700 (PDT) Received: from ste-uk-lab-gw ([93.115.195.2]) by smtp.gmail.com with ESMTPSA id af79cd13be357-792bf2a11e8sm54964385a.58.2024.05.09.04.02.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 09 May 2024 04:02:54 -0700 (PDT) Date: Thu, 9 May 2024 12:02:51 +0100 From: Daniel Gregory To: Stephen Hemminger Cc: Ruifeng Wang , dev@dpdk.org, Punit Agrawal , Liang Ma Subject: Re: [PATCH] eal/arm: replace RTE_BUILD_BUG on non-constant Message-ID: <20240509110251.GA3795959@ste-uk-lab-gw> Mail-Followup-To: Stephen Hemminger , Ruifeng Wang , dev@dpdk.org, Punit Agrawal , Liang Ma References: <20240502142116.63760-1-daniel.gregory@bytedance.com> <20240502092045.02a426fe@hermes.local> <20240502174420.GA3883350@ste-uk-lab-gw> <20240502144826.42d7012a@hermes.local> <20240503094605.GB3883350@ste-uk-lab-gw> <20240503175624.54dd0a72@hermes.local> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240503175624.54dd0a72@hermes.local> User-Agent: Mutt/1.10.1 (2018-07-13) 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 05:56:24PM -0700, Stephen Hemminger wrote: > On Fri, 3 May 2024 10:46:05 +0100 > Daniel Gregory wrote: > > > On Thu, May 02, 2024 at 02:48:26PM -0700, Stephen Hemminger wrote: > > > There are already constant checks like this elsewhere in the file. > > > > Yes, but they're in macros, rather than inlined functions, so my > > understanding was that at compile time, macro expansion has put the > > memorder constant in the _Static_assert call as opposed to still being > > a function parameter in the inline definition. > > Gcc and clang are smart enough that it is possible to use the internal > __builtin_constant_p() in the function. Some examples in DPDK: > > static __rte_always_inline int > rte_mempool_do_generic_get(struct rte_mempool *mp, void **obj_table, > unsigned int n, struct rte_mempool_cache *cache) > { > int ret; > unsigned int remaining; > uint32_t index, len; > void **cache_objs; > > /* No cache provided */ > if (unlikely(cache == NULL)) { > remaining = n; > goto driver_dequeue; > } > > /* The cache is a stack, so copy will be in reverse order. */ > cache_objs = &cache->objs[cache->len]; > > if (__extension__(__builtin_constant_p(n)) && n <= cache->len) { > > It should be possible to use RTE_BUILD_BUG_ON() or static_assert here. Yes, it's possible to use RTE_BUILD_BUG_ON(!__builtin_constant_p(n)) on Clang, I am simply not seeing it succeed. In fact, the opposite check, that the memorder is not constant, builds just fine with Clang 16. diff --git a/lib/eal/arm/include/rte_pause_64.h b/lib/eal/arm/include/rte_pause_64.h index 5cb8b59056..d0646320e6 100644 --- a/lib/eal/arm/include/rte_pause_64.h +++ b/lib/eal/arm/include/rte_pause_64.h @@ -153,8 +153,7 @@ rte_wait_until_equal_16(volatile uint16_t *addr, uint16_t expected, { uint16_t value; - RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire && - memorder != rte_memory_order_relaxed); + RTE_BUILD_BUG_ON(__builtin_constant_p(memorder)); __RTE_ARM_LOAD_EXC_16(addr, value, memorder) if (value != expected) { @@ -172,8 +171,7 @@ rte_wait_until_equal_32(volatile uint32_t *addr, uint32_t expected, { uint32_t value; - RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire && - memorder != rte_memory_order_relaxed); + RTE_BUILD_BUG_ON(__builtin_constant_p(memorder)); __RTE_ARM_LOAD_EXC_32(addr, value, memorder) if (value != expected) { @@ -191,8 +189,7 @@ rte_wait_until_equal_64(volatile uint64_t *addr, uint64_t expected, { uint64_t value; - RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire && - memorder != rte_memory_order_relaxed); + RTE_BUILD_BUG_ON(__builtin_constant_p(memorder)); __RTE_ARM_LOAD_EXC_64(addr, value, memorder) if (value != expected) { diff --git a/lib/eal/include/generic/rte_pause.h b/lib/eal/include/generic/rte_pause.h index f2a1eadcbd..3973488865 100644 --- a/lib/eal/include/generic/rte_pause.h +++ b/lib/eal/include/generic/rte_pause.h @@ -80,6 +80,7 @@ static __rte_always_inline void rte_wait_until_equal_16(volatile uint16_t *addr, uint16_t expected, rte_memory_order memorder) { + RTE_BUILD_BUG_ON(__builtin_constant_p(memorder)); assert(memorder == rte_memory_order_acquire || memorder == rte_memory_order_relaxed); while (rte_atomic_load_explicit((volatile __rte_atomic uint16_t *)addr, memorder) @@ -91,6 +92,7 @@ static __rte_always_inline void rte_wait_until_equal_32(volatile uint32_t *addr, uint32_t expected, rte_memory_order memorder) { + RTE_BUILD_BUG_ON(__builtin_constant_p(memorder)); assert(memorder == rte_memory_order_acquire || memorder == rte_memory_order_relaxed); while (rte_atomic_load_explicit((volatile __rte_atomic uint32_t *)addr, memorder) @@ -102,6 +104,7 @@ static __rte_always_inline void rte_wait_until_equal_64(volatile uint64_t *addr, uint64_t expected, rte_memory_order memorder) { + RTE_BUILD_BUG_ON(__builtin_constant_p(memorder)); assert(memorder == rte_memory_order_acquire || memorder == rte_memory_order_relaxed); while (rte_atomic_load_explicit((volatile __rte_atomic uint64_t *)addr, memorder) This seemed odd, and it doesn't line up with what the GCC documentation says about __builtin_constant_p: > [__builtin_constant_p] does not return 1 when you pass a constant > numeric value to the inline function unless you specify the -O option. https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html So I did some more looking and the behaviour I've seen is that both Clang and GCC don't allow for static checks on inline function parameters: static inline __attribute__((always_inline)) int fn(int val) { _Static_assert(val == 0, "val nonzero"); return 0; } int main(void) { return fn(0); } ( https://godbolt.org/z/TrfWqYoGo ) Both GCC and Clang's __builtin_constant_p return 1 at runtime when called on an inline function parameter static inline __attribute__((always_inline)) int fn(int val) { return __builtin_constant_p(val); } int main(void) { return fn(0); } ( https://godbolt.org/z/hTGqvj91K ) But GCC doesn't allow for static assertions on the return value of __builtin_constant_p, error-ing that the expression is not constant: static inline __attribute__((always_inline)) int fn(int val) { _Static_assert(__builtin_constant_p(val), "v isn't constant"); return 0; } int main(void) { return fn(0); } ( https://godbolt.org/z/4799nEK1T ) And on Clang these static assertions are allowed, but always fail ( https://godbolt.org/z/6j16c5MbT ). I don't really have the experience with Clang or GCC to assess whether this is intended behaviour or a bug (I've not seen evidence this behaviour is new or different from previous versions), it's just what I've observed. > Changing a compile time check into a runtime check means that buggy programs > blow up much later and in a confusing manner. And it impacts all code that > is doing a spin lock, certainly one of the hottest paths in DPDK. True, buggy programs will error later, but other than converting the rte_wait_until_equal_* functions to macros, I haven't seen an approach that will let us use a static assert on the passed in memory order value right now. However, new enough GCC and Clang versions will optimise out runtime checks that are guaranteed to pass, such as in this case when an inline functions is called with a constant value. To give a reduced example: #include enum E { X, Y, Z, }; static inline __attribute__((always_inline)) void add(int *dst, enum E e) { assert(e == X || e == Y); if (e == X) (*dst)++; else (*dst) += 4; } } int fn(int num) { add(&num, Y); return num; } (GCC https://godbolt.org/z/GKGEa7Wdd ) (Clang https://godbolt.org/z/1vYv8MYcs ) With optimisations they both produce a fn that doesn't perform any check on the value of e, and simply return the input+4. Therefore, this patch shouldn't have an impact on the performance of locks when built in release mode.