vlib: move thread barrier around mod of global node next data

The old code modified the node next array prior to obtaining the thread
barrier. Then it updated the runtime node data, and upon barrier release
caused reforking of each worker thread. The reforking clones the main
thread nodes and reconstructs the runtime node structure. This cloning
is not 100% "deep" in the sense that the node next array is
shared (i.e., only the pointer is copied). So prior to the barrier being
obtained the node's next array is being changed while workers are
actively using it (bad). Treating the node next array as read-only in
the workers and sharing it is a decent optimization so instead of trying
to fix that just move the barrier a little earlier in the process to
protect the node next array as well.

This was tripping an assert in next frame ownership change by way of the
ip4-arp node. The assert verifies that the node's next array length is
equal to the runtime next node count. The race above was lost and the
node next array data was updated in the main thread while the arp code
was still executing in a worker.

This was being hit when many arp requests were being sent from both ends
of a tunnel during which the add next node function was called, which
often led to an assert b/c the next node array was out of sync with the
runtime next node count.

- PS#2 update - move barrier sync to just above code that modifies state.

Ticket: VPP-1783
Type: fix

Signed-off-by: Christian E. Hopps <chopps@chopps.org>
Change-Id: I868784e28f994ee0922aaaae11c4894a3f4f1fe7
Signed-off-by: Christian E. Hopps <chopps@chopps.org>

diff --git a/src/vlib/node.c b/src/vlib/node.c
index 0e6038d..b6a44b2 100644 (file)
--- a/src/vlib/node.c
+++ b/src/vlib/node.c
@@ -128,10 +128,6 @@ vlib_node_runtime_update (vlib_main_t * vm, u32 node_index, u32 next_index)
   vlib_pending_frame_t *pf;
   i32 i, j, n_insert;
 
-  ASSERT (vlib_get_thread_index () == 0);
-
-  vlib_worker_thread_barrier_sync (vm);
-
   node = vec_elt (nm->nodes, node_index);
   r = vlib_node_get_runtime (vm, node_index);
 
@@ -176,8 +172,6 @@ vlib_node_runtime_update (vlib_main_t * vm, u32 node_index, u32 next_index)
   nf->node_runtime_index = next_node->runtime_index;
 
   vlib_worker_thread_node_runtime_update ();
-
-  vlib_worker_thread_barrier_release (vm);
 }
 
 uword
@@ -210,6 +204,8 @@ vlib_node_add_next_with_slot (vlib_main_t * vm,
   vlib_node_t *node, *next;
   uword *p;
 
+  ASSERT (vlib_get_thread_index () == 0);
+
   node = vec_elt (nm->nodes, node_index);
   next = vec_elt (nm->nodes, next_node_index);
 
@@ -224,6 +220,8 @@ vlib_node_add_next_with_slot (vlib_main_t * vm,
       return p[0];
     }
 
+  vlib_worker_thread_barrier_sync (vm);
+
   if (slot == ~0)
     slot = vec_len (node->next_nodes);
 
@@ -254,6 +252,7 @@ vlib_node_add_next_with_slot (vlib_main_t * vm,
     /* *INDENT-ON* */
   }
 
+  vlib_worker_thread_barrier_release (vm);
   return slot;
 }