From 8e4222fc7e23a478b021930ade3cb7d20938e398 Mon Sep 17 00:00:00 2001 From: Andrew Yourtchenko Date: Wed, 2 Aug 2017 06:36:07 -0400 Subject: 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 --- src/plugins/acl/acl.c | 51 +++++++++++++++++++++------ src/plugins/acl/acl.h | 6 ++-- src/plugins/acl/fa_node.c | 87 ++++++++++++++++++++++++++++++++--------------- src/plugins/acl/fa_node.h | 3 ++ 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 */ -- cgit 1.2.3-korg