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 7F846440D2; Sun, 26 May 2024 09:34:56 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1FE5C402D5; Sun, 26 May 2024 09:34:56 +0200 (CEST) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id 95300402B2 for ; Sun, 26 May 2024 09:34:54 +0200 (CEST) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id 3216B195B0 for ; Sun, 26 May 2024 09:34:54 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id 26985195AF; Sun, 26 May 2024 09:34:54 +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 AA1131955A; Sun, 26 May 2024 09:34:51 +0200 (CEST) Message-ID: Date: Sun, 26 May 2024 09:34:51 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 1/7] eal: generic 64 bit counter To: =?UTF-8?Q?Morten_Br=C3=B8rup?= , Stephen Hemminger , dev@dpdk.org, =?UTF-8?Q?Mattias_R=C3=B6nnblom?= , Ferruh Yigit , bruce.richardson@intel.com References: <20240510050507.14381-1-stephen@networkplumber.org> <20240514153845.42489-1-stephen@networkplumber.org> <20240514153845.42489-2-stephen@networkplumber.org> <98CBD80474FA8B44BF855DF32C47DC35E9F454@smartserver.smartshare.dk> Content-Language: en-US From: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35E9F454@smartserver.smartshare.dk> 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-15 11:30, Morten Brørup wrote: > +To: @Mattias, @Ferruh, @Bruce, participants in a related discussion > >> From: Stephen Hemminger [mailto:stephen@networkplumber.org] >> Sent: Tuesday, 14 May 2024 17.35 >> >> 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 >> --- > > With a long term perspective, I consider this patch very useful. > And its 32 bit implementation can be optimized for various architectures/compilers later. > > > In addition, it would be "nice to have" if reset() and fetch() could be called from another thread than the thread adding to the counter. > reset() from a different thread, and you enter the "multiple writer" domain. Both reset and add needs to be atomic over the whole read-modify-write cycle. (Add and reset are really no different.) ...unless you keep an offset per counter, per the discussion in the other thread, where I proposed something like that as a common counter API (in case we really needed MT safe reset). It seems to me that we shouldn't provide a MT safe reset. By some means, the user must assure there is only a single writer (the "resetter" or the "adder"). > As previously discussed [1], I think it can be done without significantly affecting fast path add() performance, by using an "offset" with Release-Consume ordering. > > [1]: https://inbox.dpdk.org/dev/98CBD80474FA8B44BF855DF32C47DC35E9F427@smartserver.smartshare.dk/ > > > rte_counter64_add(rte_counter64_t *counter, uint32_t val) > { > // Write "counter" with memory_order_relaxed, so > // it eventually becomes visible in other threads. > > rte_counter64_t ctr = *counter + val; > rte_atomic_store_explicit(counter, ctr, rte_memory_order_relaxed); > } > > rte_counter64_get(rte_counter64_t *counter, rte_counter64_t *offset) > { > // Read "offset" with memory_order_consume, so: > // - no reads or writes in the current thread dependent on "offset" > // can be reordered before this load, and > // - writes to "counter" (a data-dependent variable) > // in other threads that release "offset" are visible in the current thread. > > rte_counter64_t off = rte_atomic_load_explicit(offset, rte_memory_order_consume); > rte_counter64_t ctr = rte_atomic_load_explicit(counter, rte_memory_order_relaxed); > > return ctr - off; > } > > rte_counter64_reset(rte_counter64_t *counter, rte_counter64_t *offset) > { > // Write "offset" with memory_order_release, so > // "counter" cannot be visible after it. > > rte_counter64_t ctr = rte_atomic_load_explicit(offset, rte_memory_order_relaxed); > rte_atomic_store_explicit(offset, ctr, rte_memory_order_release); > } > > > Support for counters shared by multi threads, e.g. rte_eth_devices[rxq->port_id].data->rx_mbuf_alloc_failed, should be provided too: > > rte_counter64_mt_add(rte_counter64_t *counter, uint32_t val) > { > rte_atomic_fetch_add_explicit(counter, val, rte_memory_order_relaxed); > } > > >> 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. >> + * >> + * This is a much weaker guarantee than full atomic variables >> + * but is faster since no locked operations are required for update. >> + */ >> + >> +#include >> + >> +#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) >> +{ >> + *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; >> + >> +__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); >> +} >> + >> +__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_ */ >> -- >> 2.43.0 >