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

[Mattias Rönnblom]

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

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 <stdatomic.h>

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_ */