RE: [PATCH v3 1/7] eal: generic 64 bit counter

["Morten Brørup" <mb@smartsharesystems.com>]

+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 <stephen@networkplumber.org>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> ---

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.

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 <stephen@networkplumber.org>
> + */
> +
> +#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 <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)
> +{
> +	*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