flamenco, runtime: tidy up banks lock usage

Author: yufeng-jump

src/discof/exec/fd_exec_tile.c

@@ -95,7 +95,6 @@ scratch_footprint( fd_topo_tile_t const * tile FD_PARAM_UNUSED ) {
 
 static void
 execute_txn( fd_exec_tile_ctx_t * ctx ) {
-  fd_banks_lock( ctx->banks );
   ctx->exec_res = fd_runtime_prepare_and_execute_txn(
       ctx->banks,
       ctx->txn_ctx,
@@ -105,7 +104,6 @@ execute_txn( fd_exec_tile_ctx_t * ctx ) {
       ctx->capture_ctx,
       1
   );
-  fd_banks_unlock( ctx->banks );
 }
 
 static void

src/discof/writer/fd_writer_tile.c

@@ -314,7 +314,6 @@ after_frag( fd_writer_tile_ctx_t * ctx,
     }
     fd_exec_txn_ctx_t * txn_ctx = ctx->txn_ctx[ in_idx ];
 
-    fd_banks_lock( ctx->banks );
     ctx->bank = fd_banks_get_bank( ctx->banks, txn_ctx->slot );
     if( FD_UNLIKELY( !ctx->bank ) ) {
       FD_LOG_CRIT(( "Could not find bank for slot %lu", txn_ctx->slot ));
@@ -345,9 +344,6 @@ after_frag( fd_writer_tile_ctx_t * ctx,
           ctx->capture_ctx );
 
       } FD_SPAD_FRAME_END;
-      fd_banks_unlock( ctx->banks );
-    } else {
-      fd_banks_unlock( ctx->banks );
     }
 
     /* Notify the replay tile that we are done with this txn. */

src/flamenco/runtime/fd_bank.c

@@ -435,9 +435,12 @@ fd_banks_init_bank( fd_banks_t * banks, ulong slot ) {
   fd_bank_t *      bank_pool = fd_banks_get_bank_pool( banks );
   fd_banks_map_t * bank_map  = fd_banks_get_bank_map( banks );
 
+  fd_rwlock_write( &banks->rwlock );
+
   fd_bank_t * bank = fd_banks_pool_ele_acquire( bank_pool );
   if( FD_UNLIKELY( bank==NULL ) ) {
     FD_LOG_WARNING(( "Failed to acquire bank" ));
+    fd_rwlock_unwrite( &banks->rwlock );
     return NULL;
   }
 
@@ -484,6 +487,7 @@ fd_banks_init_bank( fd_banks_t * banks, ulong slot ) {
   banks->root     = slot;
   banks->root_idx = fd_banks_pool_idx( bank_pool, bank );
 
+  fd_rwlock_unwrite( &banks->rwlock );
   return bank;
 }
 
@@ -492,18 +496,21 @@ fd_banks_get_bank( fd_banks_t * banks, ulong slot ) {
   fd_bank_t *      bank_pool = fd_banks_get_bank_pool( banks );
   fd_banks_map_t * bank_map  = fd_banks_get_bank_map( banks );
 
+  fd_rwlock_read( &banks->rwlock );
   ulong idx = fd_banks_map_idx_query_const( bank_map, &slot, ULONG_MAX, bank_pool );
   if( FD_UNLIKELY( idx==ULONG_MAX ) ) {
     FD_LOG_DEBUG(( "Failed to get bank idx for slot %lu", slot ));
+    fd_rwlock_unread( &banks->rwlock );
     return NULL;
   }
 
   fd_bank_t * bank = fd_banks_pool_ele( bank_pool, idx );
   if( FD_UNLIKELY( !bank ) ) {
     FD_LOG_WARNING(( "Failed to get bank for slot %lu", slot ));
+    fd_rwlock_unread( &banks->rwlock );
     return NULL;
   }
-
+  fd_rwlock_unread( &banks->rwlock );
   return bank;
 }
 
@@ -866,6 +873,7 @@ fd_banks_publish( fd_banks_t * banks, ulong slot ) {
 void
 fd_banks_clear_bank( fd_banks_t * banks, fd_bank_t * bank ) {
 
+  fd_rwlock_read( &banks->rwlock );
   /* Get the parent bank. */
   fd_bank_t * parent_bank = fd_banks_pool_ele( fd_banks_get_bank_pool( banks ), bank->parent_idx );
 
@@ -877,7 +885,7 @@ fd_banks_clear_bank( fd_banks_t * banks, fd_bank_t * bank ) {
       /* assign the bank to the idx corresponding to the parent.     */                                                     \
       fd_bank_##name##_pool_idx_release( name##_pool, bank->name##_pool_idx );                                              \
       bank->name##_dirty    = 0;                                                                                            \
-      bank->name##_pool_idx = !!parent_bank ? parent_bank->name##_pool_idx : fd_bank_##name##_pool_idx_null( name##_pool ); \
+      bank->name##_pool_idx = parent_bank ? parent_bank->name##_pool_idx : fd_bank_##name##_pool_idx_null( name##_pool );   \
     }
 
   #define HAS_COW_0(type, name, footprint) \
@@ -889,24 +897,31 @@ fd_banks_clear_bank( fd_banks_t * banks, fd_bank_t * bank ) {
   #undef X
   #undef HAS_COW_0
   #undef HAS_COW_1
+
+  fd_rwlock_unread( &banks->rwlock );
 }
 
 fd_bank_t *
 fd_banks_rekey_root_bank( fd_banks_t * banks, ulong slot ) {
 
+  fd_rwlock_write( &banks->rwlock );
+
   if( FD_UNLIKELY( !banks ) ) {
     FD_LOG_WARNING(( "Banks is NULL" ));
+    fd_rwlock_unwrite( &banks->rwlock );
     return NULL;
   }
 
   if( FD_UNLIKELY( banks->root_idx==fd_banks_pool_idx_null( fd_banks_get_bank_pool( banks ) ) ) ) {
     FD_LOG_WARNING(( "Root bank does not exist" ));
+    fd_rwlock_unwrite( &banks->rwlock );
     return NULL;
   }
 
   fd_bank_t * bank = fd_banks_pool_ele( fd_banks_get_bank_pool( banks ), banks->root_idx );
   if( FD_UNLIKELY( !bank ) ) {
     FD_LOG_WARNING(( "Failed to get root bank" ));
+    fd_rwlock_unwrite( &banks->rwlock );
     return NULL;
   }
 
@@ -915,16 +930,19 @@ fd_banks_rekey_root_bank( fd_banks_t * banks, ulong slot ) {
   bank = fd_banks_map_ele_remove( fd_banks_get_bank_map( banks ), &bank->slot_, NULL, fd_banks_get_bank_pool( banks ) );
   if( FD_UNLIKELY( !bank ) ) {
     FD_LOG_WARNING(( "Failed to remove root bank" ));
+    fd_rwlock_unwrite( &banks->rwlock );
     return NULL;
   }
 
   bank->slot_ = slot;
 
   if( FD_UNLIKELY( !fd_banks_map_ele_insert( fd_banks_get_bank_map( banks ), bank, fd_banks_get_bank_pool( banks ) ) ) ) {
     FD_LOG_WARNING(( "Failed to insert root bank" ));
+    fd_rwlock_unwrite( &banks->rwlock );
     return NULL;
   }
 
+  fd_rwlock_unwrite( &banks->rwlock );
   return bank;
 }
 
@@ -983,21 +1001,26 @@ fd_banks_publish_prepare( fd_banks_t * banks,
   fd_bank_t *      bank_pool = fd_banks_get_bank_pool( banks );
   fd_banks_map_t * bank_map  = fd_banks_get_bank_map( banks );
 
+  fd_rwlock_read( &banks->rwlock );
+
   fd_bank_t * root = fd_banks_root( banks );
   if( FD_UNLIKELY( !root ) ) {
     FD_LOG_WARNING(( "failed to get root bank" ));
+    fd_rwlock_unread( &banks->rwlock );
     return 0;
   }
 
   /* Early exit if target is the same as the old root. */
   if( FD_UNLIKELY( fd_bank_slot_get( root )==target_slot ) ) {
     FD_LOG_WARNING(( "target slot %lu is the same as the old root slot %lu", target_slot, fd_bank_slot_get( root ) ));
+    fd_rwlock_unread( &banks->rwlock );
     return 0;
   }
 
   fd_bank_t * target_bank = fd_banks_map_ele_query( bank_map, &target_slot, NULL, bank_pool );
   if( FD_UNLIKELY( !target_bank ) ) {
     FD_LOG_WARNING(( "failed to get bank for target slot %lu", target_slot ));
+    fd_rwlock_unread( &banks->rwlock );
     return 0;
   }
 
@@ -1101,5 +1124,6 @@ fd_banks_publish_prepare( fd_banks_t * banks,
     curr = rooted_child_bank;
   }
 
+  fd_rwlock_unread( &banks->rwlock );
   return advanced_publishable_block;
 }

src/flamenco/runtime/fd_bank.h

@@ -436,7 +436,16 @@ struct fd_banks {
   ulong       root;            /* root slot */
   ulong       root_idx;        /* root idx */
 
-  fd_rwlock_t rwlock;          /* rwlock for fd_banks_t */
+  /* This lock is only used to serialize banks fork tree reads with
+     respect to fork tree writes.  In other words, tree traversals
+     cannot happen at the same time as a tree pruning operation or a
+     tree insertion operation.  So the public APIs on banks take either
+     a read lock or a write lock depending on what they do on the fork
+     tree.  For example, publishing takes a write lock, and bank lookups
+     take a read lock.  Notably, individual banks can still be
+     concurrently accessed or modified, and this lock does not offer
+     synchronization on individual fields within a bank. */
+  fd_rwlock_t rwlock;
 
   ulong       pool_offset;     /* offset of pool from banks */
   ulong       map_offset;      /* offset of map from banks */
@@ -621,28 +630,6 @@ FD_BANKS_ITER(X)
 #undef HAS_COW_0
 #undef HAS_COW_1
 
-/* FIXME: these locks do not fundamentally fix the minority fork racy
-   publishing issue.  Remove these once we have refcnts.
-
-   fd_banks_lock() and fd_banks_unlock() are locks to be acquired and
-   freed around accessing or modifying a specific bank. This is only
-   required if there is concurrent access to a bank while operations on
-   its underlying map are being performed.
-
-   Under the hood, this is a wrapper around fd_banks_t's rwlock.
-   This is done so a caller can safely read/write a specific bank.
-   Otherwise, we run the risk of accessing/modifying a bank that may be
-   freed. This is acquiring and freeing a read lock around fd_banks_t. */
-
-static inline void
-fd_banks_lock( fd_banks_t * banks ) {
-  fd_rwlock_read( &banks->rwlock );
-}
-
-static inline void
-fd_banks_unlock( fd_banks_t * banks ) {
-  fd_rwlock_unread( &banks->rwlock );
-}
 
 /* fd_banks_root returns the current root slot for the bank. */
 
@@ -716,7 +703,12 @@ fd_banks_init_bank( fd_banks_t * banks,
                     ulong        slot );
 
 /* fd_bank_get_bank() returns a bank for a given slot. If said bank
-   does not exist, NULL is returned. */
+   does not exist, NULL is returned.
+
+   The returned pointer is valid so long as the underlying bank does not
+   get pruned by a publishing operation.  Higher level components are
+   responsible for ensuring that publishing does not happen while a bank
+   is being accessed.  This is done through the reference counter. */
 
 fd_bank_t *
 fd_banks_get_bank( fd_banks_t * banks,
@@ -756,7 +748,7 @@ fd_banks_publish( fd_banks_t * banks,
 
    This function will memset all non-CoW fields to 0.
 
-   For all non-CoW fields, we will reset the indices to its parent. */
+   For all CoW fields, we will reset the indices to its parent. */
 
 void
 fd_banks_clear_bank( fd_banks_t * banks,