From e48f791a9ab301a9db2acd26bce47488d227925b Mon Sep 17 00:00:00 2001 From: Nick Barnes Date: Mon, 24 Feb 2025 17:12:16 +0000 Subject: [PATCH] Merge atomic counter inc/dec functions and use them consistently. --- runtime/bigarray.c | 8 ++-- runtime/caml/atomic_refcount.h | 37 ------------------ runtime/caml/camlatomic.h | 24 ++++++++++++ runtime/caml/dune | 1 - runtime/caml/misc.h | 6 +-- runtime/caml/mlvalues.h | 1 + runtime/caml/platform.h | 10 +---- runtime/domain.c | 10 ++--- runtime/major_gc.c | 68 +++++++++++++++++----------------- runtime/runtime_events.c | 2 +- runtime/shared_heap.c | 2 +- 11 files changed, 74 insertions(+), 95 deletions(-) delete mode 100644 runtime/caml/atomic_refcount.h diff --git a/runtime/bigarray.c b/runtime/bigarray.c index 8296e4e0bde..7aa877dcb32 100644 --- a/runtime/bigarray.c +++ b/runtime/bigarray.c @@ -27,7 +27,7 @@ #include "caml/memory.h" #include "caml/mlvalues.h" #include "caml/signals.h" -#include "caml/atomic_refcount.h" +#include "caml/camlatomic.h" #define int8 caml_ba_int8 #define uint8 caml_ba_uint8 @@ -293,7 +293,7 @@ CAMLexport void caml_ba_finalize(value v) free(b->data); caml_free_dependent_memory(v, caml_ba_byte_size(b)); } else { - if (caml_atomic_refcount_decr(&b->proxy->refcount) == 1) { + if (!caml_atomic_counter_decr(&b->proxy->refcount)) { free(b->proxy->data); caml_free_dependent_memory(v, b->proxy->size); free(b->proxy); @@ -1133,12 +1133,12 @@ static void caml_ba_update_proxy(struct caml_ba_array * b1, /* If b1 is already a proxy for a larger array, increment refcount of proxy */ b2->proxy = b1->proxy; - caml_atomic_refcount_incr(&b1->proxy->refcount); + (void)caml_atomic_counter_incr(&b1->proxy->refcount); } else { /* Otherwise, create proxy and attach it to both b1 and b2 */ proxy = malloc(sizeof(struct caml_ba_proxy)); if (proxy == NULL) caml_raise_out_of_memory(); - caml_atomic_refcount_init(&proxy->refcount, 2); + caml_atomic_counter_init(&proxy->refcount, 2); /* initial refcount: 2 = original array + sub array */ proxy->data = b1->data; proxy->size = caml_ba_byte_size(b1); diff --git a/runtime/caml/atomic_refcount.h b/runtime/caml/atomic_refcount.h deleted file mode 100644 index aba5ce7f67b..00000000000 --- a/runtime/caml/atomic_refcount.h +++ /dev/null @@ -1,37 +0,0 @@ -/**************************************************************************/ -/* */ -/* OCaml */ -/* */ -/* Florian Angeletti, projet Cambium, Inria */ -/* */ -/* Copyright 2022 Institut National de Recherche en Informatique et */ -/* en Automatique. */ -/* */ -/* All rights reserved. This file is distributed under the terms of */ -/* the GNU Lesser General Public License version 2.1, with the */ -/* special exception on linking described in the file LICENSE. */ -/* */ -/**************************************************************************/ - -#ifndef CAML_ATOMIC_REFCOUNT_H -#define CAML_ATOMIC_REFCOUNT_H - -#ifdef CAML_INTERNALS - -#include "camlatomic.h" - -Caml_inline void caml_atomic_refcount_init(atomic_uintnat* refc, uintnat n){ - atomic_store_release(refc, n); -} - -Caml_inline uintnat caml_atomic_refcount_decr(atomic_uintnat* refcount){ - return atomic_fetch_add (refcount, -1); -} - -Caml_inline uintnat caml_atomic_refcount_incr(atomic_uintnat* refcount){ - return atomic_fetch_add (refcount, 1); -} - -#endif /* CAML_INTERNALS */ - -#endif // CAML_ATOMIC_REFCOUNT_H diff --git a/runtime/caml/camlatomic.h b/runtime/caml/camlatomic.h index f9fd36d936a..663b99d77f3 100644 --- a/runtime/caml/camlatomic.h +++ b/runtime/caml/camlatomic.h @@ -17,6 +17,7 @@ #define CAML_ATOMIC_H #include "config.h" +#include "misc.h" /* On platforms supporting C11 atomics, this file just includes . @@ -95,6 +96,29 @@ typedef struct { intnat repr; } atomic_intnat; #define atomic_store_relaxed(p, v) \ atomic_store_explicit((p), (v), memory_order_relaxed) +Caml_inline void caml_atomic_counter_init(atomic_uintnat* counter, uintnat n) +{ + atomic_store_release(counter, n); +} + +/* atomically decrements the counter and returns the new value */ + +Caml_inline uintnat caml_atomic_counter_decr(atomic_uintnat* counter) +{ + uintnat old = atomic_fetch_sub(counter, 1); + CAMLassert (old > 0); + return old-1; +} + +/* atomically increments the counter and returns the new value */ + +Caml_inline uintnat caml_atomic_counter_incr(atomic_uintnat* counter) +{ + uintnat old = atomic_fetch_add(counter, 1); + CAMLassert(old+1 != 0); + return old+1; +} + #endif /* CAML_INTERNALS */ #endif /* CAML_ATOMIC_H */ diff --git a/runtime/caml/dune b/runtime/caml/dune index bad7dc6bc11..ef914da10fa 100644 --- a/runtime/caml/dune +++ b/runtime/caml/dune @@ -50,7 +50,6 @@ (address_class.h as caml/address_class.h) (addrmap.h as caml/addrmap.h) (alloc.h as caml/alloc.h) - (atomic_refcount.h as caml/atomic_refcount.h) (backtrace_prim.h as caml/backtrace_prim.h) (backtrace.h as caml/backtrace.h) (bigarray.h as caml/bigarray.h) diff --git a/runtime/caml/misc.h b/runtime/caml/misc.h index 441edca9d33..bc7f2d6ab86 100644 --- a/runtime/caml/misc.h +++ b/runtime/caml/misc.h @@ -27,8 +27,6 @@ #include #include -#include "camlatomic.h" - /* Deprecation warnings */ #if defined(__GNUC__) || defined(__clang__) @@ -201,6 +199,8 @@ CAMLdeprecated_typedef(addr, char *); can obtain the domain id with Caml_state->id. These functions must be reentrant. */ #ifndef __cplusplus +#include + typedef void (*caml_timing_hook) (void); extern _Atomic caml_timing_hook caml_major_slice_begin_hook; extern _Atomic caml_timing_hook caml_major_slice_end_hook; @@ -505,7 +505,7 @@ CAMLextern int caml_read_directory(char_os * dirname, /* runtime message flags. Settable with v= in OCAMLRUNPARAM */ -extern atomic_uintnat caml_verb_gc; +extern _Atomic uintnat caml_verb_gc; /* Bits which may be set in caml_verb_gc. The quotations are from the * OCaml manual. */ diff --git a/runtime/caml/mlvalues.h b/runtime/caml/mlvalues.h index 6b06b9920f0..19716a0224b 100644 --- a/runtime/caml/mlvalues.h +++ b/runtime/caml/mlvalues.h @@ -18,6 +18,7 @@ #include "config.h" #include "misc.h" +#include "camlatomic.h" #include "tsan.h" #ifdef __cplusplus diff --git a/runtime/caml/platform.h b/runtime/caml/platform.h index afa661922a0..c231ad743e1 100644 --- a/runtime/caml/platform.h +++ b/runtime/caml/platform.h @@ -54,14 +54,6 @@ Caml_inline void cpu_relax(void) { } -/* Atomic read-modify-write instructions, with full fences */ - -Caml_inline uintnat atomic_fetch_add_verify_ge0(atomic_uintnat* p, uintnat v) { - uintnat result = atomic_fetch_add(p,v); - CAMLassert ((intnat)result > 0); - return result; -} - /* If we're using glibc, use a custom condition variable implementation to avoid this bug: https://sourceware.org/bugzilla/show_bug.cgi?id=25847 @@ -308,7 +300,7 @@ typedef uintnat barrier_status; the last arrival. */ Caml_inline barrier_status caml_plat_barrier_arrive(caml_plat_barrier* barrier) { - return 1 + atomic_fetch_add(&barrier->arrived, 1); + return caml_atomic_counter_incr(&barrier->arrived); } /* -- Single-sense -- diff --git a/runtime/domain.c b/runtime/domain.c index 99b7d58b424..29a871d1603 100644 --- a/runtime/domain.c +++ b/runtime/domain.c @@ -715,7 +715,7 @@ static void domain_create(uintnat initial_minor_heap_wsize, s->unique_id = fresh_domain_unique_id(); domain_state->unique_id = s->unique_id; s->running = 1; - atomic_fetch_add(&caml_num_domains_running, 1); + (void)caml_atomic_counter_incr(&caml_num_domains_running); domain_state->c_stack = NULL; domain_state->exn_handler = NULL; @@ -1443,7 +1443,7 @@ static void decrement_stw_domains_still_processing(void) if so, clear the stw_leader to allow the new stw sections to start. */ intnat am_last = - atomic_fetch_add(&stw_request.num_domains_still_processing, -1) == 1; + caml_atomic_counter_decr(&stw_request.num_domains_still_processing) == 0; if( am_last ) { /* release the STW lock to allow new STW sections */ @@ -1661,8 +1661,8 @@ int caml_try_run_on_all_domains_with_spin_work( stw_request.data = data; stw_request.num_domains = stw_domains.participating_domains; /* stw_request.barrier doesn't need resetting */ - atomic_store_release(&stw_request.num_domains_still_processing, - stw_domains.participating_domains); + caml_atomic_counter_init(&stw_request.num_domains_still_processing, + stw_domains.participating_domains); int is_alone = stw_request.num_domains == 1; int should_sync = sync && !is_alone; @@ -2141,7 +2141,7 @@ static void domain_terminate (void) /* This is the last thing we do because we need to be able to rely on caml_domain_alone (which uses caml_num_domains_running) in at least the shared_heap lockfree fast paths */ - atomic_fetch_add(&caml_num_domains_running, -1); + (void)caml_atomic_counter_decr(&caml_num_domains_running); } CAMLprim value caml_ml_domain_cpu_relax(value t) diff --git a/runtime/major_gc.c b/runtime/major_gc.c index e99ef3814e7..7ac54a56aa0 100644 --- a/runtime/major_gc.c +++ b/runtime/major_gc.c @@ -325,10 +325,10 @@ static void ephe_next_cycle (void) { caml_plat_lock_blocking(&ephe_lock); - atomic_fetch_add(&ephe_cycle_info.ephe_cycle, +1); + (void)caml_atomic_counter_incr(&ephe_cycle_info.ephe_cycle); CAMLassert(atomic_load_acquire(&ephe_cycle_info.num_domains_done) <= atomic_load_acquire(&ephe_cycle_info.num_domains_todo)); - atomic_store(&ephe_cycle_info.num_domains_done, 0); + (void)caml_atomic_counter_init(&ephe_cycle_info.num_domains_done, 0); caml_plat_unlock(&ephe_lock); } @@ -343,12 +343,12 @@ static void ephe_todo_list_emptied (void) /* Force next ephemeron marking cycle in order to avoid reasoning about * whether the domain has already incremented * [ephe_cycle_info.num_domains_done] counter. */ - atomic_store(&ephe_cycle_info.num_domains_done, 0); - atomic_fetch_add(&ephe_cycle_info.ephe_cycle, +1); + caml_atomic_counter_init(&ephe_cycle_info.num_domains_done, 0); + (void)caml_atomic_counter_incr(&ephe_cycle_info.ephe_cycle); /* Since the todo list is empty, this domain does not need to participate in * further ephemeron cycles. */ - atomic_fetch_add_verify_ge0(&ephe_cycle_info.num_domains_todo, -1); + (void)caml_atomic_counter_decr(&ephe_cycle_info.num_domains_todo); CAMLassert(atomic_load_acquire(&ephe_cycle_info.num_domains_done) <= atomic_load_acquire(&ephe_cycle_info.num_domains_todo)); @@ -380,7 +380,7 @@ static void record_ephe_marking_done (uintnat ephe_cycle) caml_plat_lock_blocking(&ephe_lock); if (ephe_cycle == atomic_load(&ephe_cycle_info.ephe_cycle)) { Caml_state->ephe_info->cycle = ephe_cycle; - atomic_fetch_add(&ephe_cycle_info.num_domains_done, +1); + (void)caml_atomic_counter_incr(&ephe_cycle_info.num_domains_done); CAMLassert(atomic_load_acquire(&ephe_cycle_info.num_domains_done) <= atomic_load_acquire(&ephe_cycle_info.num_domains_todo)); } @@ -470,7 +470,7 @@ void caml_orphan_ephemerons (caml_domain_state* domain_state) if (ephe_info->must_sweep_ephe) { ephe_info->must_sweep_ephe = 0; - atomic_fetch_add_verify_ge0(&num_domains_to_ephe_sweep, -1); + (void)caml_atomic_counter_decr(&num_domains_to_ephe_sweep); } CAMLassert (ephe_info->must_sweep_ephe == 0); CAMLassert (ephe_info->live == 0); @@ -483,7 +483,7 @@ void caml_orphan_finalisers (caml_domain_state* domain_state) if (f->todo_head != NULL || f->first.size != 0 || f->last.size != 0) { /* have some final structures */ - atomic_fetch_add(&num_domains_orphaning_finalisers, +1); + (void)caml_atomic_counter_incr(&num_domains_orphaning_finalisers); if (caml_gc_phase != Phase_sweep_and_mark_main) { /* Force a major GC cycle to simplify constraints for orphaning finalisers. See note attached to the declaration of @@ -502,18 +502,18 @@ void caml_orphan_finalisers (caml_domain_state* domain_state) /* Create a dummy final info */ f = domain_state->final_info = caml_alloc_final_info(); - atomic_fetch_add_verify_ge0(&num_domains_orphaning_finalisers, -1); + (void)caml_atomic_counter_decr(&num_domains_orphaning_finalisers); } /* [caml_orphan_finalisers] is called in a while loop in [domain_terminate]. We take care to decrement the [num_domains_to_final_update*] counters only if we have not already decremented it for the current cycle. */ if(!f->updated_first) { - atomic_fetch_add_verify_ge0(&num_domains_to_final_update_first, -1); + (void)caml_atomic_counter_decr(&num_domains_to_final_update_first); f->updated_first = 1; } if(!f->updated_last) { - atomic_fetch_add_verify_ge0(&num_domains_to_final_update_last, -1); + (void)caml_atomic_counter_decr(&num_domains_to_final_update_last); f->updated_last = 1; } } @@ -1257,7 +1257,7 @@ static intnat mark(intnat budget) { } else { ephe_next_cycle (); domain_state->marking_done = 1; - atomic_fetch_add_verify_ge0(&num_domains_to_mark, -1); + (void)caml_atomic_counter_decr(&num_domains_to_mark); } } } @@ -1308,7 +1308,7 @@ void caml_darken(void* state, value v, volatile value* ignored) { if (Has_status_hd(hd, caml_global_heap_state.UNMARKED)) { caml_domain_state* domain_state = (caml_domain_state*)state; if (domain_state->marking_done) { - atomic_fetch_add(&num_domains_to_mark, 1); + (void)caml_atomic_counter_incr(&num_domains_to_mark); domain_state->marking_done = 0; } if (Tag_hd(hd) == Cont_tag) { @@ -1633,23 +1633,23 @@ static void cycle_major_heap_from_stw_single( domain->swept_words = 0; - atomic_store_release(&num_domains_to_sweep, num_domains_in_stw); - atomic_store_release(&num_domains_to_mark, num_domains_in_stw); + caml_atomic_counter_init(&num_domains_to_sweep, num_domains_in_stw); + caml_atomic_counter_init(&num_domains_to_mark, num_domains_in_stw); caml_gc_phase = Phase_sweep_main; atomic_store(&caml_gc_mark_phase_requested, 0); - atomic_store(&ephe_cycle_info.num_domains_todo, num_domains_in_stw); - atomic_store(&ephe_cycle_info.ephe_cycle, 1); - atomic_store(&ephe_cycle_info.num_domains_done, 0); + caml_atomic_counter_init(&ephe_cycle_info.num_domains_todo, num_domains_in_stw); + caml_atomic_counter_init(&ephe_cycle_info.ephe_cycle, 1); + caml_atomic_counter_init(&ephe_cycle_info.num_domains_done, 0); - atomic_store_release(&num_domains_to_ephe_sweep, 0); + caml_atomic_counter_init(&num_domains_to_ephe_sweep, 0); /* Will be set to the correct number when switching to [Phase_sweep_ephe] */ - atomic_store_release(&num_domains_to_final_update_first, - num_domains_in_stw); - atomic_store_release(&num_domains_to_final_update_last, - num_domains_in_stw); + caml_atomic_counter_init(&num_domains_to_final_update_first, + num_domains_in_stw); + caml_atomic_counter_init(&num_domains_to_final_update_last, + num_domains_in_stw); caml_code_fragment_cleanup_from_stw_single(); } @@ -1819,7 +1819,7 @@ static void stw_try_complete_gc_phase( caml_gc_phase = Phase_mark_final; } else if (is_complete_phase_mark_final()) { caml_gc_phase = Phase_sweep_ephe; - atomic_store_release(&num_domains_to_ephe_sweep, participant_count); + caml_atomic_counter_init(&num_domains_to_ephe_sweep, participant_count); for (int i = 0; i < participant_count; i++) participating[i]->ephe_info->must_sweep_ephe = 1; } @@ -1898,7 +1898,7 @@ static void major_collection_slice(intnat howmuch, commit_major_slice_sweepwork (work_done); if (work_done == 0) { domain_state->sweeping_done = 1; - atomic_fetch_add_verify_ge0(&num_domains_to_sweep, -1); + (void)caml_atomic_counter_decr(&num_domains_to_sweep); } } @@ -1947,7 +1947,7 @@ static void major_collection_slice(intnat howmuch, get_major_slice_markwork(mode) > 0 && caml_final_update_first(domain_state)) { /* This domain has updated finalise first values */ - atomic_fetch_add_verify_ge0(&num_domains_to_final_update_first, -1); + (void)caml_atomic_counter_decr(&num_domains_to_final_update_first); if (!domain_state->marking_done && get_major_slice_markwork(mode) > 0) goto mark_again; @@ -1958,7 +1958,7 @@ static void major_collection_slice(intnat howmuch, get_major_slice_markwork(mode) > 0 && caml_final_update_last(domain_state)) { /* This domain has updated finalise last values */ - atomic_fetch_add_verify_ge0(&num_domains_to_final_update_last, -1); + (void)caml_atomic_counter_decr(&num_domains_to_final_update_last); /* Nothing has been marked while updating last */ } @@ -2030,7 +2030,7 @@ static void major_collection_slice(intnat howmuch, /* If the todo list is empty, then the ephemeron has no sweeping work * to do. */ if (domain_state->ephe_info->todo == 0) { - atomic_fetch_add_verify_ge0(&num_domains_to_ephe_sweep, -1); + (void)caml_atomic_counter_decr(&num_domains_to_ephe_sweep); } } @@ -2049,7 +2049,7 @@ static void major_collection_slice(intnat howmuch, CAML_EV_END(EV_MAJOR_EPHE_SWEEP); if (domain_state->ephe_info->todo == 0) { - atomic_fetch_add_verify_ge0(&num_domains_to_ephe_sweep, -1); + (void)caml_atomic_counter_decr(&num_domains_to_ephe_sweep); } } } @@ -2246,7 +2246,7 @@ void caml_finish_sweeping (void) /* just finished sweeping */ CAMLassert(Caml_state->sweeping_done == 0); Caml_state->sweeping_done = 1; - atomic_fetch_add_verify_ge0(&num_domains_to_sweep, -1); + (void)caml_atomic_counter_decr(&num_domains_to_sweep); break; } caml_handle_incoming_interrupts(); @@ -2352,8 +2352,8 @@ int caml_init_major_gc(caml_domain_state* d) { if (caml_gc_phase == Phase_sweep_main) { d->sweeping_done = 1; d->marking_done = 0; - atomic_fetch_add(&num_domains_to_mark, 1); - atomic_fetch_add(&ephe_cycle_info.num_domains_todo, 1); + (void)caml_atomic_counter_incr(&num_domains_to_mark); + (void)caml_atomic_counter_incr(&ephe_cycle_info.num_domains_todo); } else { d->sweeping_done = 1; d->marking_done = 1; @@ -2375,8 +2375,8 @@ int caml_init_major_gc(caml_domain_state* d) { d->mark_stack = NULL; return -1; } - atomic_fetch_add(&num_domains_to_final_update_first, 1); - atomic_fetch_add(&num_domains_to_final_update_last, 1); + (void)caml_atomic_counter_incr(&num_domains_to_final_update_first); + (void)caml_atomic_counter_incr(&num_domains_to_final_update_last); return 0; } diff --git a/runtime/runtime_events.c b/runtime/runtime_events.c index 3cba59949dd..a8e923f121d 100644 --- a/runtime/runtime_events.c +++ b/runtime/runtime_events.c @@ -698,7 +698,7 @@ CAMLprim value caml_runtime_events_user_register(value event_name, CAMLparam3(event_name, event_tag, event_type); CAMLlocal2(list_item, event); - int index = atomic_fetch_add(&runtime_custom_event_index, 1); + uintnat index = atomic_fetch_add(&runtime_custom_event_index, 1); if (index > RUNTIME_EVENTS_MAX_CUSTOM_EVENTS) { caml_invalid_argument( diff --git a/runtime/shared_heap.c b/runtime/shared_heap.c index 2d4c760582e..312917054b4 100644 --- a/runtime/shared_heap.c +++ b/runtime/shared_heap.c @@ -2173,7 +2173,7 @@ void caml_compact_heap(caml_domain_state* domain_state, caml_global_barrier(participating_count); if (participants[0] == Caml_state) { /* We are done, increment the compaction count */ - atomic_fetch_add(&caml_compactions_count, 1); + (void)caml_atomic_counter_incr(&caml_compactions_count); CAML_GC_MESSAGE(COMPACT, "Compaction %lu completed (algorithm %lu).\n", caml_compactions_count,