erts: Fix confusion of callbacks destroying_mbc() vs remove_mbc()

Problem #1 Goodfit was crippled by the fact that destroying_mbc()
was called _before_ the carriers was unlinked from mbc_list.

Problem #2 destroying_mbc() was called for carriers that later could be
resurrected from dc_list without a matching call to creating_mbc().
This was mostly a practical problem for the new test case
alloc_SUITE:migration that use the callbacks to create/destroy a mutex.

Solution:
destroying_mbc() is now only called just before a carrier is
destroyed (deallocated or put in mseg cache).

remove_mbc() is called both (like before) when inserted into cpool
but now also when last block is freed and mbc is scheduled for
destruction but may later be resurrected from dc_list.

3 files changed, 38 insertions, 13 deletions

diff --git a/erts/emulator/beam/erl_alloc_util.c b/erts/emulator/beam/erl_alloc_util.c
index 3861196cfc..173cb091b6 100644
--- a/erts/emulator/beam/erl_alloc_util.c
+++ b/erts/emulator/beam/erl_alloc_util.c
@@ -1437,6 +1437,16 @@ erts_alcu_fix_alloc_shrink(Allctr_t *allctr, erts_aint32_t flgs)
 
 static void dealloc_carrier(Allctr_t *allctr, Carrier_t *crr, int superaligned);
 
+static ERTS_INLINE void
+dealloc_mbc(Allctr_t *allctr, Carrier_t *crr)
+{
+    ASSERT(IS_MB_CARRIER(crr));
+    if (allctr->destroying_mbc)
+        allctr->destroying_mbc(allctr, crr);
+
+    dealloc_carrier(allctr, crr, 1);
+}
+
 #ifdef ERTS_SMP
 
 static ERTS_INLINE Allctr_t*
@@ -3242,7 +3252,7 @@ check_pending_dealloc_carrier(Allctr_t *allctr,
 
 	    dcrr = crr;
 	    crr = crr->next;
-	    dealloc_carrier(allctr, dcrr, 1);
+	    dealloc_mbc(allctr, dcrr);
 	    i++;
 	} while (crr && i < ERTS_ALC_MAX_DEALLOC_CARRIER);
 
@@ -3273,11 +3283,14 @@ static void
 schedule_dealloc_carrier(Allctr_t *allctr, Carrier_t *crr)
 {
     Allctr_t *orig_allctr;
+    Block_t *blk;
     int check_pending_dealloc;
     erts_aint_t max_size;
 
+    ASSERT(IS_MB_CARRIER(crr));
+
     if (!ERTS_ALC_IS_CPOOL_ENABLED(allctr)) {
-	dealloc_carrier(allctr, crr, 1);
+	dealloc_mbc(allctr, crr);
 	return;
     }
 
@@ -3320,7 +3333,7 @@ schedule_dealloc_carrier(Allctr_t *allctr, Carrier_t *crr)
 
     if (crr->cpool.thr_prgr == ERTS_THR_PRGR_INVALID
 	|| erts_thr_progress_has_reached(crr->cpool.thr_prgr)) {
-	dealloc_carrier(allctr, crr, 1);
+	dealloc_mbc(allctr, crr);
 	return;
     }
 
@@ -3901,9 +3914,6 @@ destroy_carrier(Allctr_t *allctr, Block_t *blk, Carrier_t **busy_pcrr_pp)
 	}
 #endif
 
-	if (allctr->destroying_mbc)
-	    (*allctr->destroying_mbc)(allctr, crr);
-
 #ifdef ERTS_SMP
 	if (busy_pcrr_pp && *busy_pcrr_pp) {
 	    ERTS_ALC_CPOOL_ASSERT(*busy_pcrr_pp == crr);
@@ -3927,12 +3937,15 @@ destroy_carrier(Allctr_t *allctr, Block_t *blk, Carrier_t **busy_pcrr_pp)
 	    else
 #endif
 		STAT_SYS_ALLOC_MBC_FREE(allctr, crr_sz);
+
+            if (allctr->remove_mbc)
+                allctr->remove_mbc(allctr, crr);
 	}
 
 #ifdef ERTS_SMP
 	schedule_dealloc_carrier(allctr, crr);
 #else
-	dealloc_carrier(allctr, crr, 1);
+	dealloc_mbc(allctr, crr);
 #endif
     }
 }
diff --git a/erts/emulator/beam/erl_alloc_util.h b/erts/emulator/beam/erl_alloc_util.h
index df1f0aa65a..f4a2ae7ff3 100644
--- a/erts/emulator/beam/erl_alloc_util.h
+++ b/erts/emulator/beam/erl_alloc_util.h
@@ -277,7 +277,7 @@ typedef struct ErtsDoubleLink_t_ {
 typedef struct {
     erts_atomic_t next;
     erts_atomic_t prev;
-    Allctr_t *orig_allctr;
+    Allctr_t *orig_allctr;      /* read-only while carrier is alive */
     ErtsThrPrgrVal thr_prgr;
     erts_atomic_t max_size;
     UWord abandon_limit;
diff --git a/erts/emulator/beam/erl_ao_firstfit_alloc.c b/erts/emulator/beam/erl_ao_firstfit_alloc.c
index 7c2a5c3323..19420af8ab 100644
--- a/erts/emulator/beam/erl_ao_firstfit_alloc.c
+++ b/erts/emulator/beam/erl_ao_firstfit_alloc.c
@@ -209,7 +209,9 @@ static Block_t*	aoff_get_free_block(Allctr_t *, Uint, Block_t *, Uint);
 static void aoff_link_free_block(Allctr_t *, Block_t*);
 static void aoff_unlink_free_block(Allctr_t *allctr, Block_t *del);
 static void aoff_creating_mbc(Allctr_t*, Carrier_t*);
+#ifdef DEBUG
 static void aoff_destroying_mbc(Allctr_t*, Carrier_t*);
+#endif
 static void aoff_add_mbc(Allctr_t*, Carrier_t*);
 static void aoff_remove_mbc(Allctr_t*, Carrier_t*);
 static UWord aoff_largest_fblk_in_mbc(Allctr_t*, Carrier_t*);
@@ -271,7 +273,11 @@ erts_aoffalc_start(AOFFAllctr_t *alc,
 
     allctr->get_next_mbc_size		= NULL;
     allctr->creating_mbc		= aoff_creating_mbc;
+#ifdef DEBUG
     allctr->destroying_mbc		= aoff_destroying_mbc;
+#else
+    allctr->destroying_mbc		= NULL;
+#endif
     allctr->add_mbc                     = aoff_add_mbc;
     allctr->remove_mbc                  = aoff_remove_mbc;
     allctr->largest_fblk_in_mbc         = aoff_largest_fblk_in_mbc;
@@ -885,17 +891,18 @@ static void aoff_creating_mbc(Allctr_t *allctr, Carrier_t *carrier)
     HARD_CHECK_TREE(NULL, 0, *root, 0);
 }
 
+#define IS_CRR_IN_TREE(CRR,ROOT) \
+    ((CRR)->rbt_node.parent || (ROOT) == &(CRR)->rbt_node)
+
+#ifdef DEBUG
 static void aoff_destroying_mbc(Allctr_t *allctr, Carrier_t *carrier)
 {
     AOFFAllctr_t *alc = (AOFFAllctr_t *) allctr;
     AOFF_Carrier_t *crr = (AOFF_Carrier_t*) carrier;
-    AOFF_RBTree_t *root = alc->mbc_root;
 
-    if (crr->rbt_node.parent || &crr->rbt_node == root) {
-	aoff_remove_mbc(allctr, carrier);
-    }
-    /*else already removed */
+    ASSERT(!IS_CRR_IN_TREE(crr, alc->mbc_root));
 }
+#endif
 
 static void aoff_add_mbc(Allctr_t *allctr, Carrier_t *carrier)
 {
@@ -903,6 +910,7 @@ static void aoff_add_mbc(Allctr_t *allctr, Carrier_t *carrier)
     AOFF_Carrier_t *crr = (AOFF_Carrier_t*) carrier;
     AOFF_RBTree_t **root = &alc->mbc_root;
 
+    ASSERT(!IS_CRR_IN_TREE(crr, *root));
     HARD_CHECK_TREE(NULL, 0, *root, 0);   
 
     /* Link carrier in address order tree
@@ -919,6 +927,10 @@ static void aoff_remove_mbc(Allctr_t *allctr, Carrier_t *carrier)
     AOFF_RBTree_t **root = &alc->mbc_root;
 
     ASSERT(allctr == ERTS_ALC_CARRIER_TO_ALLCTR(carrier));
+
+    if (!IS_CRR_IN_TREE(crr,*root))
+        return;
+
     HARD_CHECK_TREE(NULL, 0, *root, 0);
 
     rbt_delete(root, &crr->rbt_node);