erts: Fair handover of code permissions to first waiter

No more simple release-the-herd that could cause very bad latency's
(seen thousands of failed retries in tprof_SUITE).

At retry, the BIF must UNCONDITIONALLY call erts_try_seize_code_*_permission.

Checked all call sites and fixed the ones in erl_bif_trace.c accordingly,
to seize permission *before* checking if trace session is still alive.

A handover-ed permission will be automatically released if the process
exits *before* the BIF has been called. But after that, there is still no
automatic release of seized permissions for exiting processes. Why? Because the
job protected by the permission must be completed even if the process
dies. Which means the BIF C function must either release the permission before
returning or schedule aux jobs that are sure to complete the job and release the
permission.

Author: sverker

erts/emulator/beam/code_ix.c

@@ -175,7 +175,7 @@ ErtsCodeIndex erts_staging_code_ix(void);
  * System thread progress must not be blocked.
  * Caller must not already have the code modification or staging permissions.
  * Caller is suspended and *must* yield if 0 is returned. */
-int erts_try_seize_code_load_permission(struct process* c_p);
+bool erts_try_seize_code_load_permission(struct process* c_p);
 
 /** @brief Release code loading permission. Resumes any suspended waiters. */
 void erts_release_code_load_permission(void);
@@ -188,15 +188,20 @@ void erts_release_code_load_permission(void);
  *
  * * Main process lock (only) must be held.
  * * System thread progress must not be blocked.
- * * Caller is suspended and *must* yield if 0 is returned.
  * * Caller must not already have the code modification or staging permissions.
  *   
  *   That is, it is _NOT_ possible to add code modification permission when you
  *   already have staging permission. The other way around is fine however.
+ *
+ * If true is returned, calling BIF must either release before returning or
+ * schedule aux job that will eventually release.
+ *
+ * If false is returned, caller is suspended and *must* schedule itself to
+ * UNCONDITIONALLY call this function again.
  */
-int erts_try_seize_code_stage_permission(struct process* c_p);
+bool erts_try_seize_code_stage_permission(struct process* c_p);
 
-/** @brief Release code stage permission. Resumes any suspended waiters. */
+/** @brief Release code stage permission. Resumes first suspended waiter. */
 void erts_release_code_stage_permission(void);
 
 /** @brief Try to seize exclusive code modification permission. Needed for
@@ -207,11 +212,17 @@ void erts_release_code_stage_permission(void);
  *
  * * Main process lock (only) must be held.
  * * System thread progress must not be blocked.
- * * Caller is suspended and *must* yield if 0 is returned.
  * * Caller must not already have the code modification permission, but may
  *   have staging permission.
+ *
+ * If true is returned, calling BIF must either release before returning
+ * or schedule aux job that will eventually release.
+ *
+ * If false is returned, caller is suspended and *must* schedule itself to
+ * UNCONDITIONALLY call this function again.
+ *
  */
-int erts_try_seize_code_mod_permission(struct process* c_p);
+bool erts_try_seize_code_mod_permission(struct process* c_p);
 
 /** @brief As \c erts_try_seize_code_mod_permission but for aux work.
  *
@@ -220,7 +231,7 @@ int erts_try_seize_code_mod_permission(struct process* c_p);
  * On failure return false and aux work func(arg) will be scheduled when
  * permission is released.
  */
-int erts_try_seize_code_mod_permission_aux(void (*func)(void *),
+bool erts_try_seize_code_mod_permission_aux(void (*func)(void *),
                                            void *arg);
 
 #ifdef ERTS_ENABLE_LOCK_CHECK
@@ -229,10 +240,21 @@ void erts_lc_soften_code_mod_permission_check(void);
 # define erts_lc_soften_code_mod_permission_check() ((void)0)
 #endif
 
-/** @brief Release code modification permission. Resumes any suspended
- * waiters. */
+/** @brief Release code modification permission. Resumes first suspended
+ * waiter.
+ */
 void erts_release_code_mod_permission(void);
 
+/** @brief Reject all code permissions. Resumes first suspended waiters.
+ *         Is called by exiting or garbing process that got permission(s)
+ *         but don't want it. A garbing process should be scheduled to try again
+ *         to seize permission after GC is done.
+ *
+ *  @param c_p[in]      Current process.
+ */
+void erts_reject_code_permissions(struct process* c_p);
+
+
 /* Prepare the "staging area" to be a complete copy of the active code.
  *
  * Code staging permission must have been seized.

erts/emulator/beam/code_ix.h

@@ -4583,6 +4583,10 @@ BIF_RETTYPE erts_debug_get_internal_state_1(BIF_ALIST_1)
         else if (ERTS_IS_ATOM_STR("debugger_support", BIF_ARG_1)) {
             return erts_debugger_flags & ERTS_DEBUGGER_ENABLED ? am_true : am_false;
         }
+        else if (ERTS_IS_ATOM_STR("dirty_gc_limit", BIF_ARG_1)) {
+            ASSERT(IS_SSMALL(ERTS_POTENTIALLY_LONG_GC_HSIZE));
+            return make_small(ERTS_POTENTIALLY_LONG_GC_HSIZE);
+        }
     }
     else if (is_tuple(BIF_ARG_1)) {
 	Eterm* tp = tuple_val(BIF_ARG_1);

erts/emulator/beam/erl_bif_info.c

@@ -316,28 +316,18 @@ erts_internal_trace_pattern_4(BIF_ALIST_4)
 {
     ErtsTraceSession* session;
 
-    if (!term_to_session(BIF_ARG_1, &session, false)) {
-        goto session_error;
-    }
-
     if (!erts_try_seize_code_mod_permission(BIF_P)) {
-        erts_deref_trace_session(session);
         ERTS_BIF_YIELD4(BIF_TRAP_EXPORT(BIF_erts_internal_trace_pattern_4),
                         BIF_P, BIF_ARG_1, BIF_ARG_2, BIF_ARG_3, BIF_ARG_4);
     }
 
-    /* Double check liveness with seized code mod permission */
-    if (!erts_is_trace_session_alive(session)) {
+    if (!term_to_session(BIF_ARG_1, &session, false)) {
         erts_release_code_mod_permission();
-        erts_deref_trace_session(session);
-        goto session_error;
+        BIF_P->fvalue = am_session;
+        BIF_ERROR(BIF_P, BADARG | EXF_HAS_EXT_INFO);
     }
 
     return trace_pattern(BIF_P, session, BIF_ARG_2, BIF_ARG_3, BIF_ARG_4);
-
-session_error:
-    BIF_P->fvalue = am_session;
-    BIF_ERROR(BIF_P, BADARG | EXF_HAS_EXT_INFO);
 }
 
 static Eterm
@@ -911,30 +901,21 @@ Eterm erts_internal_trace_4(BIF_ALIST_4)
     ErtsTraceSession* session;
     Eterm ret;
 
-    if (!term_to_session(BIF_ARG_1, &session, false)) {
-        goto session_error;
-    }
     if (!erts_try_seize_code_mod_permission(BIF_P)) {
-        erts_deref_trace_session(session);
         ERTS_BIF_YIELD4(BIF_TRAP_EXPORT(BIF_erts_internal_trace_4),
                         BIF_P, BIF_ARG_1, BIF_ARG_2, BIF_ARG_3, BIF_ARG_4);
     }
 
-    /* Double check liveness with seized code mod permission */
-    if (!erts_is_trace_session_alive(session)) {
+    if (!term_to_session(BIF_ARG_1, &session, false)) {
         erts_release_code_mod_permission();
-        erts_deref_trace_session(session);
-        goto session_error;
+        BIF_P->fvalue = am_session;
+        BIF_ERROR(BIF_P, BADARG | EXF_HAS_EXT_INFO);
     }
 
     ret = trace(BIF_P, session, BIF_ARG_2, BIF_ARG_3, BIF_ARG_4);
 
     erts_deref_trace_session(session);
     return ret;
-
-session_error:
-    BIF_P->fvalue = am_session;
-    BIF_ERROR(BIF_P, BADARG | EXF_HAS_EXT_INFO);
 }
 
 static
@@ -1298,19 +1279,24 @@ Eterm
 erts_internal_trace_session_destroy_1(BIF_ALIST_1)
 {
     ErtsTraceSession* session;
+
+    if (!erts_try_seize_code_mod_permission(BIF_P)) {
+        ERTS_BIF_YIELD1(BIF_TRAP_EXPORT(BIF_erts_internal_trace_session_destroy_1),
+                        BIF_P, BIF_ARG_1);
+    }
+
     if (!term_to_session(BIF_ARG_1, &session, true)) {
+        erts_release_code_mod_permission();
         BIF_P->fvalue = am_badopt;
         BIF_ERROR(BIF_P, BADARG | EXF_HAS_EXT_INFO);
     }
     if (!erts_is_trace_session_alive(session)) {
+        erts_release_code_mod_permission();
         erts_deref_trace_session(session);
         BIF_RET(am_false);
     }
-    if (!erts_try_seize_code_mod_permission(BIF_P)) {
-        erts_deref_trace_session(session);
-        ERTS_BIF_YIELD1(BIF_TRAP_EXPORT(BIF_erts_internal_trace_session_destroy_1),
-                        BIF_P, BIF_ARG_1);
-    }
+
+
     if (erts_atomic_cmpxchg_nob(&session->state,
                                 ERTS_TRACE_SESSION_CLEARING,
                                 ERTS_TRACE_SESSION_ALIVE)
@@ -1423,27 +1409,19 @@ Eterm erts_internal_trace_info_3(BIF_ALIST_3)
     bool to_be_continued = false;
     Eterm ret;
 
-    if (BIF_ARG_1 == am_any) {
-        /* trace:session_info */
-        session = NULL;
-    }
-    else if (!term_to_session(BIF_ARG_1, &session, true)) {
-        goto session_error;
-    }
-
     if (!erts_try_seize_code_mod_permission(BIF_P)) {
-        if (session) {
-            erts_deref_trace_session(session);
-        }
         ERTS_BIF_YIELD3(BIF_TRAP_EXPORT(BIF_erts_internal_trace_info_3),
                         BIF_P, BIF_ARG_1, BIF_ARG_2, BIF_ARG_3);
     }
 
-    /* Double check session liveness with seized code mod permission */
-    if (session && !erts_is_trace_session_alive(session)) {
+    if (BIF_ARG_1 == am_any) {
+        /* trace:session_info */
+        session = NULL;
+    }
+    else if (!term_to_session(BIF_ARG_1, &session, false)) {
         erts_release_code_mod_permission();
-        erts_deref_trace_session(session);
-        goto session_error;
+        BIF_P->fvalue = am_session;
+        BIF_ERROR(BIF_P, BADARG | EXF_HAS_EXT_INFO);
     }
 
     ret = trace_info(BIF_P, session, BIF_ARG_2, BIF_ARG_3, &to_be_continued);
@@ -1454,10 +1432,6 @@ Eterm erts_internal_trace_info_3(BIF_ALIST_3)
         }
     }
     return ret;
-
-session_error:
-    BIF_P->fvalue = am_session;
-    BIF_ERROR(BIF_P, BADARG | EXF_HAS_EXT_INFO);
 }
 
 static

erts/emulator/beam/erl_bif_trace.c

@@ -715,6 +715,37 @@ check_for_possibly_long_gc(Process *p, Uint ygen_usage)
     }
 }
 
+static void set_proc_state_gc(Process* p, bool is_garbing)
+{
+    /*
+     * Q: Why two GC flags? One in 'state' and one in 'xstate'?
+     *
+     * A: Just code convenience. We needed a GC flag in 'xstate' to be accessed
+     *    atomically together with the HANDOVER_CODE_*_PERM flags. The old one
+     *    in 'state' could be replaced by the new one in 'xstate' but that
+     *    would require refactoring all the places where it's currently read.
+     */
+    if (is_garbing) {
+        const erts_aint32_t old_xstate =
+            erts_atomic32_read_bor_nob(&p->xstate, ERTS_PXSFLG_GC);
+        erts_atomic32_read_bor_nob(&p->state, ERTS_PSFLG_GC);
+
+        if (old_xstate & (ERTS_PXSFLG_HANDOVER_CODE_MOD_PERM |
+                          ERTS_PXSFLG_HANDOVER_CODE_STAGE_PERM)) {
+            /*
+             * We don't want to hold code permissions during a potentially
+             * long GC. Process will retry to seize permission(s) after GC is
+             * done. This can only happen with a race between other processes
+             * giving us code permission and sending us GC signal.
+             */
+            erts_reject_code_permissions(p);
+        }
+    }
+    else {
+        erts_atomic32_read_band_nob(&p->xstate, ~ERTS_PXSFLG_GC);
+        erts_atomic32_read_band_nob(&p->state, ~ERTS_PSFLG_GC);
+    }
+}
 
 /*
  * Garbage collect a process.
@@ -768,7 +799,7 @@ garbage_collect(Process* p, ErlHeapFragment *live_hf_end,
 
     ERTS_MSACC_SET_STATE_CACHED(ERTS_MSACC_STATE_GC);
 
-    erts_atomic32_read_bor_nob(&p->state, ERTS_PSFLG_GC);
+    set_proc_state_gc(p, true);
     if (erts_system_monitor_long_gc)
 	start_time = erts_get_monotonic_time(esdp);
 
@@ -849,7 +880,7 @@ garbage_collect(Process* p, ErlHeapFragment *live_hf_end,
     delay_gc_after_start:
         /* erts_send_exit_signal looks for ERTS_PSFLG_GC, so
            we have to remove it after the signal is sent */
-        erts_atomic32_read_band_nob(&p->state, ~ERTS_PSFLG_GC);
+        set_proc_state_gc(p, false);
 
         /* We have to make sure that we have space for need on the heap */
         res = delay_garbage_collection(p, need, fcalls);
@@ -864,7 +895,7 @@ garbage_collect(Process* p, ErlHeapFragment *live_hf_end,
     ERTS_CHK_OFFHEAP(p);
     ErtsGcQuickSanityCheck(p);
 
-    erts_atomic32_read_band_nob(&p->state, ~ERTS_PSFLG_GC);
+    set_proc_state_gc(p, false);
 
     if (ERTS_IS_P_TRACED_FL(p, F_TRACE_GC)) {
         trace_gc(p, gc_trace_end_tag, reclaimed_now, THE_NON_VALUE);
@@ -1006,7 +1037,7 @@ garbage_collect_hibernate(Process* p, int check_long_gc)
         p->flags = flags;
     }
 
-    erts_atomic32_read_bor_nob(&p->state, ERTS_PSFLG_GC);
+    set_proc_state_gc(p, true);
     ErtsGcQuickSanityCheck(p);
 
     heap_size = p->heap_sz + (p->old_htop - p->old_heap) + p->mbuf_sz;
@@ -1089,7 +1120,7 @@ garbage_collect_hibernate(Process* p, int check_long_gc)
 
     ErtsGcQuickSanityCheck(p);
 
-    erts_atomic32_read_band_nob(&p->state, ~ERTS_PSFLG_GC);
+    set_proc_state_gc(p, false);
 
     return gc_cost(final_size, final_size);
 }
@@ -1182,7 +1213,7 @@ erts_garbage_collect_literals(Process* p, Eterm* literals,
     /*
      * Set GC state.
      */
-    erts_atomic32_read_bor_nob(&p->state, ERTS_PSFLG_GC);
+    set_proc_state_gc(p, true);
 
     /*
      * Just did a major collection (which has discarded the old heap),
@@ -1349,7 +1380,7 @@ erts_garbage_collect_literals(Process* p, Eterm* literals,
     /*
      * Restore status.
      */
-    erts_atomic32_read_band_nob(&p->state, ~ERTS_PSFLG_GC);
+    set_proc_state_gc(p, false);
 
     reds += (Sint64) gc_cost((p->htop - p->heap) + byte_lit_size/sizeof(Uint), 0);
 

erts/emulator/beam/erl_gc.c

@@ -23,12 +23,12 @@
 #ifndef __ERL_GC_H__
 #define __ERL_GC_H__
 
+#define ERTS_POTENTIALLY_LONG_GC_HSIZE (128*1024) /* Words */
+
 #if defined(ERL_WANT_GC_INTERNALS__) || defined(ERTS_DO_INCL_GLB_INLINE_FUNC_DEF)
 
 /* GC declarations used by beam/erl_gc.c */
 
-#define ERTS_POTENTIALLY_LONG_GC_HSIZE (128*1024) /* Words */
-
 #include "erl_map.h"
 #include "erl_fun.h"
 #include "erl_bits.h"

erts/emulator/beam/erl_gc.h

@@ -14212,6 +14212,11 @@ erts_do_exit_process(Process* p, Eterm reason)
 
     erts_proc_unlock(p, ERTS_PROC_LOCKS_ALL_MINOR);
 
+    if (erts_atomic32_read_nob(&p->xstate) & (ERTS_PXSFLG_HANDOVER_CODE_MOD_PERM |
+                                              ERTS_PXSFLG_HANDOVER_CODE_STAGE_PERM)) {
+        erts_reject_code_permissions(p);
+    }
+
     if (ERTS_IS_P_TRACED_FL(p, F_TRACE_PROCS))
         trace_proc(p, ERTS_PROC_LOCK_MAIN, p, am_exit, reason);
 

erts/emulator/beam/erl_process.c

@@ -1404,6 +1404,15 @@ void erts_check_for_holes(Process* p);
 /* MAYBE_SELF_SIGS - We might have outstanding signals
    from ourselves to ourselves. */
 #define ERTS_PXSFLG_MAYBE_SELF_SIGS	(((erts_aint32_t) 1) << 8)
+/* HANDOVER_CODE_MOD_PERM - Waiting process was given the code mod permission
+   but has not yet seized the lock. */
+#define ERTS_PXSFLG_HANDOVER_CODE_MOD_PERM   (((erts_aint32_t) 1) << 9)
+/* HANDOVER_CODE_STAGE_PERM - Waiting process was given the code stage permission
+   but has not yet seized the lock. */
+#define ERTS_PXSFLG_HANDOVER_CODE_STAGE_PERM (((erts_aint32_t) 1) << 10)
+/* GC - Process is garbage collecting */
+#define ERTS_PXSFLG_GC			     (((erts_aint32_t) 1) << 11)
+
 
 #define ERTS_PXSFLGS_QMASK 		ERTS_PSFLGS_QMASK
 #define ERTS_PXSFLGS_IN_CPU_PRQ_MASK_OFFSET 0

erts/emulator/beam/erl_process.h

@@ -146,6 +146,7 @@ MODULES= \
 	erts_test_destructor \
 	crypto_reference \
 	literal_area_collector_test \
+        erts_test_sync_tracer \
         ext_records
 
 NO_OPT= bs_bincomp \