From 0bbb9e72f21ec65809b43c56652f6b1fc04a0542 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Tue, 9 Jan 2018 13:59:33 -0500 Subject: [PATCH 1/3] lib: add MTYPE for synchronization primitives Signed-off-by: Quentin Young --- lib/frr_pthread.c | 1 + lib/frr_pthread.h | 3 +++ 2 files changed, 4 insertions(+) diff --git a/lib/frr_pthread.c b/lib/frr_pthread.c index 19dfbaf54b..de522e5ef9 100644 --- a/lib/frr_pthread.c +++ b/lib/frr_pthread.c @@ -26,6 +26,7 @@ #include "hash.h" DEFINE_MTYPE_STATIC(LIB, FRR_PTHREAD, "FRR POSIX Thread"); +DEFINE_MTYPE(LIB, PTHREAD_PRIM, "POSIX synchronization primitives"); static unsigned int next_id = 0; diff --git a/lib/frr_pthread.h b/lib/frr_pthread.h index f6000340a7..7915b43a46 100644 --- a/lib/frr_pthread.h +++ b/lib/frr_pthread.h @@ -21,8 +21,11 @@ #define _FRR_PTHREAD_H #include +#include "memory.h" #include "thread.h" +DECLARE_MTYPE(PTHREAD_PRIM); + struct frr_pthread { /* pthread id */ From f09a656d24204cf607ec3a0842439cf922d21f15 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Tue, 9 Jan 2018 14:27:44 -0500 Subject: [PATCH 2/3] bgpd: improve bgp thread startup characteristics Replace atomic spinlock with condition variable. Signed-off-by: Quentin Young --- bgpd/bgp_io.c | 72 ++++++++++++++++++++++++++++++++++++--------------- bgpd/bgp_io.h | 21 +++++++-------- bgpd/bgpd.c | 8 +++--- 3 files changed, 64 insertions(+), 37 deletions(-) diff --git a/bgpd/bgp_io.c b/bgpd/bgp_io.c index cc9c1bda56..c9662e2121 100644 --- a/bgpd/bgp_io.c +++ b/bgpd/bgp_io.c @@ -50,20 +50,37 @@ static bool validate_header(struct peer *); #define BGP_IO_TRANS_ERR (1 << 0) // EAGAIN or similar occurred #define BGP_IO_FATAL_ERR (1 << 1) // some kind of fatal TCP error -/* Start and stop routines for I/O pthread + control variables +/* Plumbing & control variables for thread lifecycle * ------------------------------------------------------------------------ */ -_Atomic bool bgp_io_thread_run; -_Atomic bool bgp_io_thread_started; - -void bgp_io_init() -{ - bgp_io_thread_run = false; - bgp_io_thread_started = false; -} +bool bgp_io_thread_run; +pthread_mutex_t *running_cond_mtx; +pthread_cond_t *running_cond; /* Unused callback for thread_add_read() */ static int bgp_io_dummy(struct thread *thread) { return 0; } +/* Poison pill task */ +static int bgp_io_finish(struct thread *thread) +{ + bgp_io_thread_run = false; + return 0; +} + +/* Extern lifecycle control functions. init -> start -> stop + * ------------------------------------------------------------------------ */ +void bgp_io_init() +{ + bgp_io_thread_run = false; + + running_cond_mtx = XCALLOC(MTYPE_PTHREAD_PRIM, sizeof(pthread_mutex_t)); + running_cond = XCALLOC(MTYPE_PTHREAD_PRIM, sizeof(pthread_cond_t)); + + pthread_mutex_init(running_cond_mtx, NULL); + pthread_cond_init(running_cond, NULL); + + pthread_mutex_lock(running_cond_mtx); +} + void *bgp_io_start(void *arg) { struct frr_pthread *fpt = frr_pthread_get(PTHREAD_IO); @@ -79,9 +96,12 @@ void *bgp_io_start(void *arg) struct thread task; - atomic_store_explicit(&bgp_io_thread_run, true, memory_order_seq_cst); - atomic_store_explicit(&bgp_io_thread_started, true, - memory_order_seq_cst); + bgp_io_thread_run = true; + pthread_mutex_lock(running_cond_mtx); + { + pthread_cond_signal(running_cond); + } + pthread_mutex_unlock(running_cond_mtx); while (bgp_io_thread_run) { if (thread_fetch(fpt->master, &task)) { @@ -95,29 +115,33 @@ void *bgp_io_start(void *arg) return NULL; } -static int bgp_io_finish(struct thread *thread) +void bgp_io_wait_running() { - atomic_store_explicit(&bgp_io_thread_run, false, memory_order_seq_cst); - return 0; + while (!bgp_io_thread_run) + pthread_cond_wait(running_cond, running_cond_mtx); } int bgp_io_stop(void **result, struct frr_pthread *fpt) { thread_add_event(fpt->master, &bgp_io_finish, NULL, 0, NULL); pthread_join(fpt->thread, result); + + pthread_mutex_unlock(running_cond_mtx); + pthread_mutex_destroy(running_cond_mtx); + pthread_cond_destroy(running_cond); + + XFREE(MTYPE_PTHREAD_PRIM, running_cond_mtx); + XFREE(MTYPE_PTHREAD_PRIM, running_cond); + return 0; } /* Extern API -------------------------------------------------------------- */ -void bgp_io_running(void) -{ - while (!atomic_load_explicit(&bgp_io_thread_started, - memory_order_seq_cst)) - frr_pthread_yield(); -} void bgp_writes_on(struct peer *peer) { + assert(bgp_io_thread_run); + assert(peer->status != Deleted); assert(peer->obuf); assert(peer->ibuf); @@ -135,6 +159,8 @@ void bgp_writes_on(struct peer *peer) void bgp_writes_off(struct peer *peer) { + assert(bgp_io_thread_run); + struct frr_pthread *fpt = frr_pthread_get(PTHREAD_IO); thread_cancel_async(fpt->master, &peer->t_write, NULL); @@ -145,6 +171,8 @@ void bgp_writes_off(struct peer *peer) void bgp_reads_on(struct peer *peer) { + assert(bgp_io_thread_run); + assert(peer->status != Deleted); assert(peer->ibuf); assert(peer->fd); @@ -164,6 +192,8 @@ void bgp_reads_on(struct peer *peer) void bgp_reads_off(struct peer *peer) { + assert(bgp_io_thread_run); + struct frr_pthread *fpt = frr_pthread_get(PTHREAD_IO); thread_cancel_async(fpt->master, &peer->t_read, NULL); diff --git a/bgpd/bgp_io.h b/bgpd/bgp_io.h index 73587366d7..7cfd1db710 100644 --- a/bgpd/bgp_io.h +++ b/bgpd/bgp_io.h @@ -31,21 +31,11 @@ /** * Initializes data structures and flags for the write thread. * - * This function should be called from the main thread before + * This function must be called from the main thread before * bgp_writes_start() is invoked. */ extern void bgp_io_init(void); -/** - * Ensure that the BGP IO thread is actually up and running - * - * This function must be called immediately after the thread - * has been created for running. This is because we want - * to make sure that the io thread is ready before other - * threads start attempting to use it. - */ -extern void bgp_io_running(void); - /** * Start function for write thread. * @@ -53,6 +43,15 @@ extern void bgp_io_running(void); */ extern void *bgp_io_start(void *arg); +/** + * Wait until the IO thread is ready to accept jobs. + * + * This function must be called immediately after the thread has been created + * for running. Use of other functions before calling this one will result in + * undefined behavior. + */ +extern void bgp_io_wait_running(void); + /** * Start function for write thread. * diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 4d8e4ffe37..0a46cf42dc 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -7438,13 +7438,11 @@ void bgp_pthreads_run() pthread_attr_setschedpolicy(&attr, SCHED_FIFO); /* - * Please ensure that the io thread is running - * by calling bgp_io_running. The BGP threads - * depend on it being running when we start - * looking for it. + * I/O related code assumes the thread is ready for work at all times, + * so we wait until it is. */ frr_pthread_run(PTHREAD_IO, &attr, NULL); - bgp_io_running(); + bgp_io_wait_running(); frr_pthread_run(PTHREAD_KEEPALIVES, &attr, NULL); } From f42ebe8a6750ba86d7a0550bfa6d11c446315b5a Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Wed, 10 Jan 2018 11:20:09 -0500 Subject: [PATCH 3/3] bgpd: move startup sync lock around Condition needs to be set inside critical section, otherwise i/o thread can deadlock. Also unlock mutex once finished with it, no need to hold the lock for the life of the program. Signed-off-by: Quentin Young --- bgpd/bgp_io.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/bgpd/bgp_io.c b/bgpd/bgp_io.c index c9662e2121..6861b0ee22 100644 --- a/bgpd/bgp_io.c +++ b/bgpd/bgp_io.c @@ -78,6 +78,7 @@ void bgp_io_init() pthread_mutex_init(running_cond_mtx, NULL); pthread_cond_init(running_cond, NULL); + /* unlocked in bgp_io_wait_running() */ pthread_mutex_lock(running_cond_mtx); } @@ -96,9 +97,9 @@ void *bgp_io_start(void *arg) struct thread task; - bgp_io_thread_run = true; pthread_mutex_lock(running_cond_mtx); { + bgp_io_thread_run = true; pthread_cond_signal(running_cond); } pthread_mutex_unlock(running_cond_mtx); @@ -119,6 +120,9 @@ void bgp_io_wait_running() { while (!bgp_io_thread_run) pthread_cond_wait(running_cond, running_cond_mtx); + + /* locked in bgp_io_init() */ + pthread_mutex_unlock(running_cond_mtx); } int bgp_io_stop(void **result, struct frr_pthread *fpt) @@ -126,7 +130,6 @@ int bgp_io_stop(void **result, struct frr_pthread *fpt) thread_add_event(fpt->master, &bgp_io_finish, NULL, 0, NULL); pthread_join(fpt->thread, result); - pthread_mutex_unlock(running_cond_mtx); pthread_mutex_destroy(running_cond_mtx); pthread_cond_destroy(running_cond);