Add an acquire fence after young_limit triggers an allocation failure

gadmm · gadmm · commit d365a92dbfe2 · 2022-04-07T16:37:05.000+02:00
The output has been disassembled (gcc -O2, arch=x86-64) to confirm
that `CAMLunlikely` works as intended and produces equivalent code as
before.
diff --git a/runtime/caml/domain.h b/runtime/caml/domain.h
@@ -23,17 +23,29 @@ extern "C" {
 
 #ifdef CAML_INTERNALS
 
+#include "camlatomic.h"
 #include "config.h"
 #include "mlvalues.h"
 #include "domain_state.h"
 #include "platform.h"
 
 /* is the minor heap full or an external interrupt has been triggered */
-#define Caml_check_gc_interrupt(dom_st)   \
-  (CAMLalloc_point_here, \
-   CAMLunlikely( \
-     (uintnat)(dom_st)->young_ptr < \
-     atomic_load_explicit(&((dom_st)->young_limit), memory_order_relaxed)))
+Caml_inline int caml_check_gc_interrupt(caml_domain_state * dom_st)
+{
+  CAMLalloc_point_here;
+  uintnat young_limit =
+    atomic_load_explicit(&dom_st->young_limit, memory_order_relaxed);
+  if ((uintnat)dom_st->young_ptr < young_limit) {
+    /* Synchronise for the case when [young_limit] was used to interrupt
+       us. */
+    atomic_thread_fence(memory_order_acquire);
+    return 1;
+  }
+  return 0;
+}
+
+#define Caml_check_gc_interrupt(dom_st)           \
+  (CAMLunlikely(caml_check_gc_interrupt(dom_st)))
 
 asize_t caml_norm_minor_heap_size (intnat);
 int caml_reallocate_minor_heap(asize_t);
diff --git a/runtime/signals.c b/runtime/signals.c
@@ -45,13 +45,6 @@ static caml_plat_mutex signal_install_mutex = CAML_PLAT_MUTEX_INITIALIZER;
 int caml_check_pending_signals(void)
 {
   int i;
-  /* [MM] This fence compensates for the fact that Caml_check_gc_interrupt
-     reads young_limit with a relaxed load.  It is possible in theory to
-     see young_limit updated without caml_pending_signals being set
-     and then resetting young_limit after the check.  This would delay
-     processing the pending signal until young_limit is updated again.
-     There may be nicer ways to address this scenario. */
-  atomic_thread_fence(memory_order_acquire);
   for (i = 0; i < NSIG_WORDS; i++) {
     if (atomic_load_explicit(&caml_pending_signals[i], memory_order_relaxed))
       return 1;
diff --git a/runtime/signals_nat.c b/runtime/signals_nat.c
@@ -54,6 +54,10 @@ void caml_garbage_collection(void)
   Pop_frame_pointer(sp);
   retaddr = *(uintnat*)sp;
 
+  /* Synchronise for the case when [young_limit] was used to interrupt
+     us. */
+  atomic_thread_fence(memory_order_acquire);
+
   { /* Find the frame descriptor for the current allocation */
     uintnat h = Hash_retaddr(retaddr, fds.mask);
     while (1) {
