diff mbox series

[v5,1/9] eal: generic 64 bit counter

Message ID 20240516154327.64104-2-stephen@networkplumber.org (mailing list archive)
State Superseded
Delegated to: Thomas Monjalon
Headers
Series Generic 64 bit counters |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Stephen Hemminger May 16, 2024, 3:40 p.m. UTC 
  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>
---
 lib/eal/include/meson.build   |  1 +
 lib/eal/include/rte_counter.h | 98 +++++++++++++++++++++++++++++++++++
 2 files changed, 99 insertions(+)
 create mode 100644 lib/eal/include/rte_counter.h

Comments

Wathsala Wathawana Vithanage May 16, 2024, 6:22 p.m. UTC | #1 
Hi Stephen,

> +__rte_experimental
> +static inline uint64_t
> +rte_counter64_fetch(const rte_counter64_t *counter) {
> +	return *counter;
> +}

What if the address pointed by counter is not aligned and the
architecture doesn't support atomic (untorn) loads on non-aligned loads? 

--wathsala
Stephen Hemminger May 16, 2024, 9:42 p.m. UTC | #2 
On Thu, 16 May 2024 18:22:23 +0000
Wathsala Wathawana Vithanage <wathsala.vithanage@arm.com> wrote:

> Hi Stephen,
> 
> > +__rte_experimental
> > +static inline uint64_t
> > +rte_counter64_fetch(const rte_counter64_t *counter) {
> > +	return *counter;
> > +}  
> 
> What if the address pointed by counter is not aligned and the
> architecture doesn't support atomic (untorn) loads on non-aligned loads? 
> 
> --wathsala

Then the driver is using it incorrectly. For the use case of a set of counters
(even if embedded in another struct), the compiler takes care of this.

Remember this is an internal API, not something that needs to handle user
abuse.
Honnappa Nagarahalli May 17, 2024, 2:39 a.m. UTC | #3 
> On May 16, 2024, at 4:42 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> On Thu, 16 May 2024 18:22:23 +0000
> Wathsala Wathawana Vithanage <wathsala.vithanage@arm.com> wrote:
> 
>> Hi Stephen,
>> 
>>> +__rte_experimental
>>> +static inline uint64_t
>>> +rte_counter64_fetch(const rte_counter64_t *counter) {
>>> + return *counter;
>>> +}  
>> 
>> What if the address pointed by counter is not aligned and the
>> architecture doesn't support atomic (untorn) loads on non-aligned loads? 
>> 
>> --wathsala
> 
> Then the driver is using it incorrectly. For the use case of a set of counters
> (even if embedded in another struct), the compiler takes care of this.
> 
> Remember this is an internal API, not something that needs to handle user
> abuse.
If it is internal API, should the API name have double underscore prefix to indicate the same?
Stephen Hemminger May 17, 2024, 3:29 a.m. UTC | #4 
On Fri, 17 May 2024 02:39:02 +0000
Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> wrote:

> > On May 16, 2024, at 4:42 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
> > 
> > On Thu, 16 May 2024 18:22:23 +0000
> > Wathsala Wathawana Vithanage <wathsala.vithanage@arm.com> wrote:
> >   
> >> Hi Stephen,
> >>   
> >>> +__rte_experimental
> >>> +static inline uint64_t
> >>> +rte_counter64_fetch(const rte_counter64_t *counter) {
> >>> + return *counter;
> >>> +}    
> >> 
> >> What if the address pointed by counter is not aligned and the
> >> architecture doesn't support atomic (untorn) loads on non-aligned loads? 
> >> 
> >> --wathsala  
> > 
> > Then the driver is using it incorrectly. For the use case of a set of counters
> > (even if embedded in another struct), the compiler takes care of this.
> > 
> > Remember this is an internal API, not something that needs to handle user
> > abuse.  
> If it is internal API, should the API name have double underscore prefix to indicate the same?
> 

Other parts of ethdev have internal API's with similar names already.
None of them use underscore.

INTERNAL {
	global:

	rte_eth_dev_allocate;
	rte_eth_dev_allocated;
	rte_eth_dev_attach_secondary;
	rte_eth_dev_callback_process;
	rte_eth_dev_create;
	rte_eth_dev_destroy;
	rte_eth_dev_get_by_name;
	rte_eth_dev_is_rx_hairpin_queue;
	rte_eth_dev_is_tx_hairpin_queue;
	rte_eth_dev_probing_finish;
	rte_eth_dev_release_port;
	rte_eth_dev_internal_reset;
	rte_eth_devargs_parse;
	rte_eth_devices;
	rte_eth_dma_zone_free;
	rte_eth_dma_zone_reserve;
	rte_eth_hairpin_queue_peer_bind;
	rte_eth_hairpin_queue_peer_unbind;
	rte_eth_hairpin_queue_peer_update;
	rte_eth_ip_reassembly_dynfield_register;
	rte_eth_link_speed_ethtool; # WINDOWS_NO_EXPORT
	rte_eth_link_speed_glink; # WINDOWS_NO_EXPORT
	rte_eth_link_speed_gset; # WINDOWS_NO_EXPORT
	rte_eth_pkt_burst_dummy;
	rte_eth_representor_id_get;
	rte_eth_switch_domain_alloc;
	rte_eth_switch_domain_free;
	rte_flow_fp_default_ops;
};
Honnappa Nagarahalli May 17, 2024, 4:39 a.m. UTC | #5 
> On May 16, 2024, at 10:29 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> On Fri, 17 May 2024 02:39:02 +0000
> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> wrote:
> 
>>> On May 16, 2024, at 4:42 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
>>> 
>>> On Thu, 16 May 2024 18:22:23 +0000
>>> Wathsala Wathawana Vithanage <wathsala.vithanage@arm.com> wrote:
>>> 
>>>> Hi Stephen,
>>>> 
>>>>> +__rte_experimental
>>>>> +static inline uint64_t
>>>>> +rte_counter64_fetch(const rte_counter64_t *counter) {
>>>>> + return *counter;
>>>>> +}    
>>>> 
>>>> What if the address pointed by counter is not aligned and the
>>>> architecture doesn't support atomic (untorn) loads on non-aligned loads? 
>>>> 
>>>> --wathsala  
>>> 
>>> Then the driver is using it incorrectly. For the use case of a set of counters
>>> (even if embedded in another struct), the compiler takes care of this.
>>> 
>>> Remember this is an internal API, not something that needs to handle user
>>> abuse.  
>> If it is internal API, should the API name have double underscore prefix to indicate the same?
>> 
> 
> Other parts of ethdev have internal API's with similar names already.
> None of them use underscore.
At least, these are marked internal.
Should we use ‘@internal’ in the function descriptions?

> 
> INTERNAL {
> global:
> 
> rte_eth_dev_allocate;
> rte_eth_dev_allocated;
> rte_eth_dev_attach_secondary;
> rte_eth_dev_callback_process;
> rte_eth_dev_create;
> rte_eth_dev_destroy;
> rte_eth_dev_get_by_name;
> rte_eth_dev_is_rx_hairpin_queue;
> rte_eth_dev_is_tx_hairpin_queue;
> rte_eth_dev_probing_finish;
> rte_eth_dev_release_port;
> rte_eth_dev_internal_reset;
> rte_eth_devargs_parse;
> rte_eth_devices;
> rte_eth_dma_zone_free;
> rte_eth_dma_zone_reserve;
> rte_eth_hairpin_queue_peer_bind;
> rte_eth_hairpin_queue_peer_unbind;
> rte_eth_hairpin_queue_peer_update;
> rte_eth_ip_reassembly_dynfield_register;
> rte_eth_link_speed_ethtool; # WINDOWS_NO_EXPORT
> rte_eth_link_speed_glink; # WINDOWS_NO_EXPORT
> rte_eth_link_speed_gset; # WINDOWS_NO_EXPORT
> rte_eth_pkt_burst_dummy;
> rte_eth_representor_id_get;
> rte_eth_switch_domain_alloc;
> rte_eth_switch_domain_free;
> rte_flow_fp_default_ops;
> };
diff mbox series

Patch

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..d623195d63
--- /dev/null
+++ b/lib/eal/include/rte_counter.h
@@ -0,0 +1,98 @@ 
+/* 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.
+ */
+
+#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_set(rte_counter64_t *counter, uint64_t val)
+{
+	*counter = val;
+}
+
+#else
+
+#include <rte_stdatomic.h>
+
+/*
+ * 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_consume);
+}
+
+__rte_experimental
+static inline void
+rte_counter64_set(rte_counter64_t *counter, uint64_t val)
+{
+	rte_atomic_store_explicit(counter, val, rte_memory_order_release);
+}
+#endif
+
+__rte_experimental
+static inline void
+rte_counter64_reset(rte_counter64_t *counter)
+{
+	rte_counter64_set(counter, 0);
+}
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_COUNTER_H_ */