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 C443C440D2; Sun, 26 May 2024 08:45:57 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2FD9D402D5; Sun, 26 May 2024 08:45:57 +0200 (CEST) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id 72E7C402B2 for ; Sun, 26 May 2024 08:45:53 +0200 (CEST) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id 92FC519315 for ; Sun, 26 May 2024 08:45:52 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id 878581926D; Sun, 26 May 2024 08:45:52 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 4.0.0 (2022-12-13) on hermod.lysator.liu.se X-Spam-Level: X-Spam-Status: No, score=-1.3 required=5.0 tests=ALL_TRUSTED,AWL, T_SCC_BODY_TEXT_LINE autolearn=disabled version=4.0.0 X-Spam-Score: -1.3 Received: from [192.168.1.59] (h-62-63-215-114.A163.priv.bahnhof.se [62.63.215.114]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id 11CEE19312; Sun, 26 May 2024 08:45:48 +0200 (CEST) Message-ID: <9adfe1ee-6a3b-4575-ad02-5f9b67bf45cc@lysator.liu.se> Date: Sun, 26 May 2024 08:45:48 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= Subject: Re: [PATCH v3 1/7] eal: generic 64 bit counter To: Stephen Hemminger , dev@dpdk.org Cc: =?UTF-8?Q?Morten_Br=C3=B8rup?= , Tyler Retzlaff References: <20240510050507.14381-1-stephen@networkplumber.org> <20240514153845.42489-1-stephen@networkplumber.org> <20240514153845.42489-2-stephen@networkplumber.org> Content-Language: en-US In-Reply-To: <20240514153845.42489-2-stephen@networkplumber.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Virus-Scanned: ClamAV using ClamSMTP 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 2024-05-14 17:35, Stephen Hemminger wrote: > This header implements 64 bit counters that are NOT atomic > but are safe against load/store splits on 32 bit platforms. > > Signed-off-by: Stephen Hemminger > Acked-by: Morten Brørup > --- > lib/eal/include/meson.build | 1 + > lib/eal/include/rte_counter.h | 91 +++++++++++++++++++++++++++++++++++ > 2 files changed, 92 insertions(+) > create mode 100644 lib/eal/include/rte_counter.h > > diff --git a/lib/eal/include/meson.build b/lib/eal/include/meson.build > index e94b056d46..c070dd0079 100644 > --- a/lib/eal/include/meson.build > +++ b/lib/eal/include/meson.build > @@ -12,6 +12,7 @@ headers += files( > 'rte_class.h', > 'rte_common.h', > 'rte_compat.h', > + 'rte_counter.h', > 'rte_debug.h', > 'rte_dev.h', > 'rte_devargs.h', > diff --git a/lib/eal/include/rte_counter.h b/lib/eal/include/rte_counter.h > new file mode 100644 > index 0000000000..8068d6d26e > --- /dev/null > +++ b/lib/eal/include/rte_counter.h > @@ -0,0 +1,91 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright (c) Stephen Hemminger > + */ > + > +#ifndef _RTE_COUNTER_H_ > +#define _RTE_COUNTER_H_ > + > +#ifdef __cplusplus > +extern "C" { > +#endif > + > +/** > + * @file > + * RTE Counter > + * > + * A counter is 64 bit value that is safe from split read/write > + * on 32 bit platforms. It assumes that only one cpu at a time > + * will update the counter, and another CPU may want to read it. It's not totally obvious what "split read/write" means. I think there is a word for this already; atomic. Atomic read/load and atomic write/store. "A counter is value which can be atomically read, atomically written to, but does not allow atomic arithmetic operations (such as add), making them mostly useful in single-writer scenarios." > + * > + * This is a much weaker guarantee than full atomic variables > + * but is faster since no locked operations are required for update. > + */ > + > +#include This shouldn't read rte_stdatomic.h? > + > +#ifdef RTE_ARCH_64 > +/* > + * On a platform that can support native 64 bit type, no special handling. > + * These are just wrapper around 64 bit value. > + */ > +typedef uint64_t rte_counter64_t; > + > +/** > + * Add value to counter. > + */ > +__rte_experimental > +static inline void > +rte_counter64_add(rte_counter64_t *counter, uint32_t val) Shouldn't 'val' also be uint64_t? Can't see it would be slower. > +{ > + *counter += val; > +} > + > +__rte_experimental > +static inline uint64_t > +rte_counter64_fetch(const rte_counter64_t *counter) > +{ > + return *counter; > +} > + > +__rte_experimental > +static inline void > +rte_counter64_reset(rte_counter64_t *counter) > +{ > + *counter = 0; > +} > + > +#else > +/* > + * On a 32 bit platform need to use atomic to force the compler to not > + * split 64 bit read/write. > + */ > +typedef RTE_ATOMIC(uint64_t) rte_counter64_t; To have an API that sometimes, for certain build configurations and architectures, makes some object _Atomic, makes me somewhat uneasy. All direct accesses to the object in question (e.g., my_counter++) will be atomic with SEQ CST memory model. The alternative, to always use the regular type (uint64_t in this case), and cast to _Atomic (RTE_ATOMIC()) also seems less than ideal. The atomic bit operations in the bitops patch set takes the latter approach. > + > +__rte_experimental > +static inline void > +rte_counter64_add(rte_counter64_t *counter, uint32_t val) > +{ > + rte_atomic_fetch_add_explicit(counter, val, rte_memory_order_relaxed); This is overkill, and will generate a locked instruction on x86. Use an atomic load, a non-atomic add, and an atomic store. A non-atomic load would do, but with RTE_ATOMIC() I don't think there's a safe way to achieve that. uint64_t value = *counter; would be a non-atomic load on non-C11-atomics-builds, but an atomic load with SEQ CST memory ordering on C11-atomics-enabled builds. > +} > + > +__rte_experimental > +static inline uint64_t > +rte_counter64_fetch(const rte_counter64_t *counter) > +{ > + return rte_atomic_load_explicit(counter, rte_memory_order_relaxed); > +} > + > +__rte_experimental > +static inline void > +rte_counter64_reset(rte_counter64_t *counter) > +{ > + rte_atomic_store_explicit(counter, 0, rte_memory_order_relaxed); > +} > +#endif > + > + > +#ifdef __cplusplus > +} > +#endif > + > +#endif /* _RTE_COUNTER_H_ */