acl-plugin: multicore: CSIT c100k 2-core stateful ACL test does not pass (VPP-912)

Fix several threading-related issues uncovered by the CSIT scale/performance test:

- make the per-interface add/del counters per-thread

- preallocate the per-worker session pools rather than
  attempting to resize them within the datapath

- move the bihash initialization to the moment of ACL
  being applied rather than later during the connection creation

- adjust the connection cleaning logic to not require
  the signaling from workers to main thread

- make the connection lists check in the main thread robust against workers
  updating the list heads at the same time

- add more information to "show acl-plugin sessions" to aid in debugging

Change-Id: If82ef715e4993614df11db5e9afa7fa6b522d9bc
Signed-off-by: Andrew Yourtchenko <ayourtch@gmail.com>

4 files changed, 106 insertions, 41 deletions

diff --git a/src/plugins/acl/acl.c b/src/plugins/acl/acl.c
index 02fcb4fe5d9..6cd2d0c605f 100644
--- a/src/plugins/acl/acl.c
+++ b/src/plugins/acl/acl.c
@@ -1939,23 +1939,54 @@ acl_show_aclplugin_fn (vlib_main_t * vm,
       u8 * out0 = format(0, "");
       u16 wk;
       u32 show_bihash_verbose = 0;
+      u32 show_session_thread_id = ~0;
+      u32 show_session_session_index = ~0;
+      unformat (input, "thread %u index %u", &show_session_thread_id, &show_session_session_index);
       unformat (input, "verbose %u", &show_bihash_verbose);
-      pool_foreach (swif, im->sw_interfaces,
-      ({
-        u32 sw_if_index =  swif->sw_if_index;
-        u64 n_adds = sw_if_index < vec_len(am->fa_session_adds_by_sw_if_index) ? am->fa_session_adds_by_sw_if_index[sw_if_index] : 0;
-        u64 n_dels = sw_if_index < vec_len(am->fa_session_dels_by_sw_if_index) ? am->fa_session_dels_by_sw_if_index[sw_if_index] : 0;
-        out0 = format(out0, "sw_if_index %d: add %lu - del %lu = %lu\n", sw_if_index, n_adds, n_dels, n_adds - n_dels);
-      }));
       {
         u64 n_adds = am->fa_session_total_adds;
         u64 n_dels = am->fa_session_total_dels;
-        out0 = format(out0, "TOTAL: add %lu - del %lu = %lu\n", n_adds, n_dels, n_adds - n_dels);
+        out0 = format(out0, "Sessions total: add %lu - del %lu = %lu\n", n_adds, n_dels, n_adds - n_dels);
       }
-      out0 = format(out0, "\n\nPer-worker data:\n");
+      out0 = format(out0, "\n\nPer-thread data:\n");
       for (wk = 0; wk < vec_len (am->per_worker_data); wk++) {
         acl_fa_per_worker_data_t *pw = &am->per_worker_data[wk];
-	out0 = format(out0, "Worker #%d:\n", wk);
+	out0 = format(out0, "Thread #%d:\n", wk);
+        if (show_session_thread_id == wk && show_session_session_index < pool_len(pw->fa_sessions_pool)) {
+	  out0 = format(out0, "  session index %u:\n", show_session_session_index);
+          fa_session_t *sess = pw->fa_sessions_pool + show_session_session_index;
+          u64 *m =  (u64 *)&sess->info;
+          out0 = format(out0, "    info: %016llx %016llx %016llx %016llx %016llx %016llx\n", m[0], m[1], m[2], m[3], m[4], m[5]);
+	  out0 = format(out0, "    sw_if_index: %u\n", sess->sw_if_index);
+	  out0 = format(out0, "    tcp_flags_seen: %x\n", sess->tcp_flags_seen.as_u16);
+	  out0 = format(out0, "    last active time: %lu\n", sess->last_active_time);
+	  out0 = format(out0, "    thread index: %u\n", sess->thread_index);
+	  out0 = format(out0, "    link enqueue time: %lu\n", sess->link_enqueue_time);
+	  out0 = format(out0, "    link next index: %u\n", sess->link_next_idx);
+	  out0 = format(out0, "    link prev index: %u\n", sess->link_prev_idx);
+	  out0 = format(out0, "    link list id: %u\n", sess->link_list_id);
+        }
+	out0 = format(out0, "  connection add/del stats:\n", wk);
+        pool_foreach (swif, im->sw_interfaces,
+        ({
+          u32 sw_if_index =  swif->sw_if_index;
+          u64 n_adds = sw_if_index < vec_len(pw->fa_session_adds_by_sw_if_index) ? pw->fa_session_adds_by_sw_if_index[sw_if_index] : 0;
+          u64 n_dels = sw_if_index < vec_len(pw->fa_session_dels_by_sw_if_index) ? pw->fa_session_dels_by_sw_if_index[sw_if_index] : 0;
+          out0 = format(out0, "    sw_if_index %d: add %lu - del %lu = %lu\n", sw_if_index, n_adds, n_dels, n_adds - n_dels);
+        }));
+
+	out0 = format(out0, "  connection timeout type lists:\n", wk);
+        u8 tt = 0;
+        for(tt = 0; tt < ACL_N_TIMEOUTS; tt++) {
+          u32 head_session_index = pw->fa_conn_list_head[tt];
+          out0 = format(out0, "  fa_conn_list_head[%d]: %d\n", tt, head_session_index);
+          if (~0 != head_session_index) {
+            fa_session_t *sess = pw->fa_sessions_pool + head_session_index;
+	    out0 = format(out0, "    last active time: %lu\n", sess->last_active_time);
+	    out0 = format(out0, "    link enqueue time: %lu\n", sess->link_enqueue_time);
+          }
+        }
+
 	out0 = format(out0, "  Next expiry time: %lu\n", pw->next_expiry_time);
 	out0 = format(out0, "  Requeue until time: %lu\n", pw->requeue_until_time);
 	out0 = format(out0, "  Current time wait interval: %lu\n", pw->current_time_wait_interval);
diff --git a/src/plugins/acl/acl.h b/src/plugins/acl/acl.h
index c9f403e9d1b..b84ed7a42dc 100644
--- a/src/plugins/acl/acl.h
+++ b/src/plugins/acl/acl.h
@@ -144,6 +144,9 @@ typedef struct {
   u32 **input_sw_if_index_vec_by_acl;
   u32 **output_sw_if_index_vec_by_acl;
 
+  /* Total count of interface+direction pairs enabled */
+  u32 fa_total_enabled_count;
+
   /* Do we use hash-based ACL matching or linear */
   int use_hash_acl_matching;
 
@@ -172,9 +175,6 @@ typedef struct {
   u32 fa_cleaner_node_index;
   /* FA session timeouts, in seconds */
   u32 session_timeout_sec[ACL_N_TIMEOUTS];
-  /* session add/delete counters */
-  u64 *fa_session_adds_by_sw_if_index;
-  u64 *fa_session_dels_by_sw_if_index;
   /* total session adds/dels */
   u64 fa_session_total_adds;
   u64 fa_session_total_dels;
diff --git a/src/plugins/acl/fa_node.c b/src/plugins/acl/fa_node.c
index 0bbc7423a39..a2ba61ba520 100644
--- a/src/plugins/acl/fa_node.c
+++ b/src/plugins/acl/fa_node.c
@@ -599,20 +599,23 @@ fa_session_get_timeout (acl_main_t * am, fa_session_t * sess)
 }
 
 static void
-acl_fa_ifc_init_sessions (acl_main_t * am, int sw_if_index0)
+acl_fa_verify_init_sessions (acl_main_t * am)
 {
-  /// FIXME-MULTICORE: lock around this function
-#ifdef FA_NODE_VERBOSE_DEBUG
-  clib_warning
-    ("Initializing bihash for sw_if_index %d num buckets %lu memory size %llu",
-     sw_if_index0, am->fa_conn_table_hash_num_buckets,
-     am->fa_conn_table_hash_memory_size);
-#endif
-  BV (clib_bihash_init) (&am->fa_sessions_hash,
+  if (!am->fa_sessions_hash_is_initialized) {
+    u16 wk;
+    /* Allocate the per-worker sessions pools */
+    for (wk = 0; wk < vec_len (am->per_worker_data); wk++) {
+      acl_fa_per_worker_data_t *pw = &am->per_worker_data[wk];
+      pool_alloc_aligned(pw->fa_sessions_pool, am->fa_conn_table_max_entries, CLIB_CACHE_LINE_BYTES);
+    }
+
+    /* ... and the interface session hash table */
+    BV (clib_bihash_init) (&am->fa_sessions_hash,
 			 "ACL plugin FA session bihash",
 			 am->fa_conn_table_hash_num_buckets,
 			 am->fa_conn_table_hash_memory_size);
-  am->fa_sessions_hash_is_initialized = 1;
+    am->fa_sessions_hash_is_initialized = 1;
+  }
 }
 
 static inline fa_session_t *get_session_ptr(acl_main_t *am, u16 thread_index, u32 session_index)
@@ -622,6 +625,12 @@ static inline fa_session_t *get_session_ptr(acl_main_t *am, u16 thread_index, u3
   return sess;
 }
 
+static inline int is_valid_session_ptr(acl_main_t *am, u16 thread_index, fa_session_t *sess)
+{
+  acl_fa_per_worker_data_t *pw = &am->per_worker_data[thread_index];
+  return ((sess - pw->fa_sessions_pool) < pool_len(pw->fa_sessions_pool));
+}
+
 static void
 acl_fa_conn_list_add_session (acl_main_t * am, fa_full_session_id_t sess_id, u64 now)
 {
@@ -648,9 +657,6 @@ acl_fa_conn_list_add_session (acl_main_t * am, fa_full_session_id_t sess_id, u64
 
   if (~0 == pw->fa_conn_list_head[list_id]) {
     pw->fa_conn_list_head[list_id] = sess_id.session_index;
-    /* If it is a first conn in any list, kick the cleaner */
-    vlib_process_signal_event (am->vlib_main, am->fa_cleaner_node_index,
-                                 ACL_FA_CLEANER_RESCHEDULE, 0);
   }
 }
 
@@ -733,8 +739,8 @@ acl_fa_delete_session (acl_main_t * am, u32 sw_if_index, fa_full_session_id_t se
   pool_put_index (pw->fa_sessions_pool, sess_id.session_index);
   /* Deleting from timer structures not needed,
      as the caller must have dealt with the timers. */
-  vec_validate (am->fa_session_dels_by_sw_if_index, sw_if_index);
-  am->fa_session_dels_by_sw_if_index[sw_if_index]++;
+  vec_validate (pw->fa_session_dels_by_sw_if_index, sw_if_index);
+  pw->fa_session_dels_by_sw_if_index[sw_if_index]++;
   clib_smp_atomic_add(&am->fa_session_total_dels, 1);
 }
 
@@ -749,10 +755,14 @@ acl_fa_can_add_session (acl_main_t * am, int is_input, u32 sw_if_index)
 static u64
 acl_fa_get_list_head_expiry_time(acl_main_t *am, acl_fa_per_worker_data_t *pw, u64 now, u16 thread_index, int timeout_type)
 {
-  if (~0 == pw->fa_conn_list_head[timeout_type]) {
+  fa_session_t *sess = get_session_ptr(am, thread_index, pw->fa_conn_list_head[timeout_type]);
+  /*
+   * We can not check just the index here because inbetween the worker thread might
+   * dequeue the connection from the head just as we are about to check it.
+   */
+  if (!is_valid_session_ptr(am, thread_index, sess)) {
     return ~0LL; // infinity.
   } else {
-    fa_session_t *sess = get_session_ptr(am, thread_index, pw->fa_conn_list_head[timeout_type]);
     u64 timeout_time =
               sess->link_enqueue_time + fa_session_get_list_timeout (am, sess);
     return timeout_time;
@@ -893,17 +903,13 @@ acl_fa_add_session (acl_main_t * am, int is_input, u32 sw_if_index, u64 now,
 
 
 
-  if (!acl_fa_ifc_has_sessions (am, sw_if_index))
-    {
-      acl_fa_ifc_init_sessions (am, sw_if_index);
-    }
-
+  ASSERT(am->fa_sessions_hash_is_initialized == 1);
   BV (clib_bihash_add_del) (&am->fa_sessions_hash,
 			    &kv, 1);
   acl_fa_conn_list_add_session(am, f_sess_id, now);
 
-  vec_validate (am->fa_session_adds_by_sw_if_index, sw_if_index);
-  am->fa_session_adds_by_sw_if_index[sw_if_index]++;
+  vec_validate (pw->fa_session_adds_by_sw_if_index, sw_if_index);
+  pw->fa_session_adds_by_sw_if_index[sw_if_index]++;
   clib_smp_atomic_add(&am->fa_session_total_adds, 1);
 }
 
@@ -1342,8 +1348,10 @@ acl_fa_worker_conn_cleaner_process(vlib_main_t * vm,
      if (num_expired >= am->fa_max_deleted_sessions_per_interval) {
        /* there was too much work, we should get an interrupt ASAP */
        pw->interrupt_is_needed = 1;
+       pw->interrupt_is_unwanted = 0;
      } else if (num_expired <= am->fa_min_deleted_sessions_per_interval) {
        /* signal that they should trigger us less */
+       pw->interrupt_is_needed = 0;
        pw->interrupt_is_unwanted = 1;
      } else {
        /* the current rate of interrupts is ok */
@@ -1359,11 +1367,11 @@ send_one_worker_interrupt (vlib_main_t * vm, acl_main_t *am, int thread_index)
 {
   acl_fa_per_worker_data_t *pw = &am->per_worker_data[thread_index];
   if (!pw->interrupt_is_pending) {
+    pw->interrupt_is_pending = 1;
     vlib_node_set_interrupt_pending (vlib_mains[thread_index],
                   acl_fa_worker_session_cleaner_process_node.index);
-    pw->interrupt_is_pending = 1;
     /* if the interrupt was requested, mark that done. */
-    pw->interrupt_is_needed = 0;
+    /* pw->interrupt_is_needed = 0; */
   }
 }
 
@@ -1430,8 +1438,8 @@ acl_fa_session_cleaner_process (vlib_main_t * vm, vlib_node_runtime_t * rt,
         }
       }
 
-      /* If no pending connections then no point in timing out */
-      if (!has_pending_conns)
+      /* If no pending connections and no ACL applied then no point in timing out */
+      if (!has_pending_conns && (0 == am->fa_total_enabled_count))
         {
           am->fa_cleaner_cnt_wait_without_timeout++;
           (void) vlib_process_wait_for_event (vm);
@@ -1493,6 +1501,10 @@ acl_fa_session_cleaner_process (vlib_main_t * vm, vlib_node_runtime_t * rt,
 	    clib_warning("ACL_FA_CLEANER_DELETE_BY_SW_IF_INDEX bitmap: %U", format_bitmap_hex, clear_sw_if_index_bitmap);
 #endif
 	    vec_foreach(pw0, am->per_worker_data) {
+              if ((pw0 == am->per_worker_data) && (vec_len(vlib_mains) > 1)) {
+                /* thread 0 in multithreaded scenario is not used */
+                continue;
+              }
               CLIB_MEMORY_BARRIER ();
 	      while (pw0->clear_in_process) {
                 CLIB_MEMORY_BARRIER ();
@@ -1527,6 +1539,10 @@ acl_fa_session_cleaner_process (vlib_main_t * vm, vlib_node_runtime_t * rt,
 	    clib_warning("CLEANER mains len: %d per-worker len: %d", vec_len(vlib_mains), vec_len(am->per_worker_data));
 #endif
 	    vec_foreach(pw0, am->per_worker_data) {
+              if ((pw0 == am->per_worker_data) && (vec_len(vlib_mains) > 1)) {
+                /* thread 0 in multithreaded scenario is not used */
+                continue;
+              }
               CLIB_MEMORY_BARRIER ();
 	      while (pw0->clear_in_process) {
                 CLIB_MEMORY_BARRIER ();
@@ -1568,6 +1584,10 @@ acl_fa_session_cleaner_process (vlib_main_t * vm, vlib_node_runtime_t * rt,
       int interrupts_unwanted = 0;
 
       vec_foreach(pw0, am->per_worker_data) {
+        if ((pw0 == am->per_worker_data) && (vec_len(vlib_mains) > 1)) {
+          /* thread 0 in multithreaded scenario is not used */
+          continue;
+        }
         if (pw0->interrupt_is_needed) {
           interrupts_needed++;
           /* the per-worker value is reset when sending the interrupt */
@@ -1580,6 +1600,8 @@ acl_fa_session_cleaner_process (vlib_main_t * vm, vlib_node_runtime_t * rt,
       if (interrupts_needed) {
         /* they need more interrupts, do less waiting around next time */
         am->fa_current_cleaner_timer_wait_interval /= 2;
+        /* never go into zero-wait either though - we need to give the space to others */
+        am->fa_current_cleaner_timer_wait_interval += 1; 
       } else if (interrupts_unwanted) {
         /* slowly increase the amount of sleep up to a limit */
         if (am->fa_current_cleaner_timer_wait_interval < max_timer_wait_interval)
@@ -1596,6 +1618,15 @@ void
 acl_fa_enable_disable (u32 sw_if_index, int is_input, int enable_disable)
 {
   acl_main_t *am = &acl_main;
+  if (enable_disable) {
+    acl_fa_verify_init_sessions(am);
+    am->fa_total_enabled_count++;
+    vlib_process_signal_event (am->vlib_main, am->fa_cleaner_node_index,
+                                 ACL_FA_CLEANER_RESCHEDULE, 0);
+  } else {
+    am->fa_total_enabled_count--;
+  }
+
   if (is_input)
     {
       ASSERT(clib_bitmap_get(am->fa_in_acl_on_sw_if_index, sw_if_index) != enable_disable);
diff --git a/src/plugins/acl/fa_node.h b/src/plugins/acl/fa_node.h
index e4cd2ab57d8..a8dece4edcf 100644
--- a/src/plugins/acl/fa_node.h
+++ b/src/plugins/acl/fa_node.h
@@ -109,6 +109,9 @@ typedef struct {
   /* per-worker ACL_N_TIMEOUTS of conn lists */
   u32 *fa_conn_list_head;
   u32 *fa_conn_list_tail;
+  /* adds and deletes per-worker-per-interface */
+  u64 *fa_session_dels_by_sw_if_index;
+  u64 *fa_session_adds_by_sw_if_index;
   /* Vector of expired connections retrieved from lists */
   u32 *expired;
   /* the earliest next expiry time */