From b5061659d3cc011118024861c2f048e67affbd39 Mon Sep 17 00:00:00 2001 From: Maria Matejka Date: Thu, 4 Feb 2021 15:08:52 +0100 Subject: [PATCH 1/7] POSIX threads and thread-local storage is needed for concurrent execution --- aclocal.m4 | 14 ++++++++++++-- configure.ac | 46 +++++++++++++++++++--------------------------- lib/birdlib.h | 4 ---- lib/timer.c | 10 ---------- sysdep/unix/log.c | 11 ----------- 5 files changed, 31 insertions(+), 54 deletions(-) diff --git a/aclocal.m4 b/aclocal.m4 index 1613d680..3405b85b 100644 --- a/aclocal.m4 +++ b/aclocal.m4 @@ -1,5 +1,6 @@ dnl ** Additional Autoconf tests for BIRD configure script dnl ** (c) 1999 Martin Mares +dnl ** (c) 2021 Maria Matejka AC_DEFUN([BIRD_CHECK_THREAD_LOCAL], [ @@ -9,14 +10,23 @@ AC_DEFUN([BIRD_CHECK_THREAD_LOCAL], AC_COMPILE_IFELSE([ AC_LANG_PROGRAM( [ - _Thread_local static int x = 42; + static _Thread_local int x = 42; ], [] ) ], [bird_cv_thread_local=yes], + [AC_COMPILE_IFELSE([ + AC_LANG_PROGRAM( + [ + static __thread int x = 42; + ], + [] + ) + ], + [bird_cv_thread_local=__thread], [bird_cv_thread_local=no] - ) + )]) ) ]) diff --git a/configure.ac b/configure.ac index 64181d29..5c0cf002 100644 --- a/configure.ac +++ b/configure.ac @@ -36,12 +36,6 @@ AC_ARG_ENABLE([memcheck], [enable_memcheck=yes] ) -AC_ARG_ENABLE([pthreads], - [AS_HELP_STRING([--enable-pthreads], [enable POSIX threads support @<:@try@:>@])], - [], - [enable_pthreads=try] -) - AC_ARG_ENABLE([libssh], [AS_HELP_STRING([--enable-libssh], [enable LibSSH support in RPKI @<:@try@:>@])], [], @@ -125,25 +119,19 @@ if test -z "$GCC" ; then fi BIRD_CHECK_THREAD_LOCAL -if test "$bird_cv_thread_local" = yes ; then - AC_DEFINE([HAVE_THREAD_LOCAL], [1], [Define to 1 if _Thread_local is available]) +if test "$bird_cv_thread_local" = no ; then + AC_MSG_ERROR([This program requires thread local storage.]) +elif test "$bird_cv_thread_local" != yes ; then + AC_DEFINE_UNQUOTED([_Thread_local], [$bird_cv_thread_local], [Legacy _Thread_local]) fi -if test "$enable_pthreads" != no ; then - BIRD_CHECK_PTHREADS +BIRD_CHECK_PTHREADS - if test "$bird_cv_lib_pthreads" = yes ; then - AC_DEFINE([USE_PTHREADS], [1], [Define to 1 if pthreads are enabled]) - CFLAGS="$CFLAGS -pthread" - LDFLAGS="$LDFLAGS -pthread" - proto_bfd=bfd - elif test "$enable_pthreads" = yes ; then - AC_MSG_ERROR([POSIX threads not available.]) - fi - - if test "$enable_pthreads" = try ; then - enable_pthreads="$bird_cv_lib_pthreads" - fi +if test "$bird_cv_lib_pthreads" = yes ; then + CFLAGS="$CFLAGS -pthread" + LDFLAGS="$LDFLAGS -pthread" +else + AC_MSG_ERROR([POSIX threads not available.]) fi # This is assumed to be necessary for proper BIRD build @@ -304,8 +292,7 @@ if test "$enable_mpls_kernel" != no ; then fi fi -all_protocols="$proto_bfd babel bgp mrt ospf perf pipe radv rip rpki static" - +all_protocols="bfd babel bgp mrt ospf perf pipe radv rip rpki static" all_protocols=`echo $all_protocols | sed 's/ /,/g'` if test "$with_protocols" = all ; then @@ -351,9 +338,15 @@ case $sysdesc in esac AC_CHECK_HEADERS_ONCE([alloca.h syslog.h]) -AC_CHECK_HEADER([sys/mman.h], [AC_DEFINE([HAVE_MMAP], [1], [Define to 1 if mmap() is available.])]) +AC_CHECK_HEADER([sys/mman.h], [AC_DEFINE([HAVE_MMAP], [1], [Define to 1 if mmap() is available.])], have_mman=no) +AC_CHECK_FUNC([aligned_alloc], [AC_DEFINE([HAVE_ALIGNED_ALLOC], [1], [Define to 1 if aligned_alloc() is available.])], have_aligned_alloc=no) AC_CHECK_MEMBERS([struct sockaddr.sa_len], [], [], [#include ]) +if test "$have_aligned_alloc" = "no" && test "$have_mman" = "no" ; then + AC_MSG_ERROR([No means of aligned alloc found. Need mmap() or aligned_alloc().]) +fi + + AC_C_BIGENDIAN( [AC_DEFINE([CPU_BIG_ENDIAN], [1], [Define to 1 if cpu is big endian])], [AC_DEFINE([CPU_LITTLE_ENDIAN], [1], [Define to 1 if cpu is little endian])], @@ -402,7 +395,7 @@ if test "$enable_debug" = yes ; then fi fi - if test "enable_debug_expensive" = yes ; then + if test "$enable_debug_expensive" = yes ; then AC_DEFINE([ENABLE_EXPENSIVE_CHECKS], [1], [Define to 1 if you want to run expensive consistency checks.]) fi fi @@ -467,7 +460,6 @@ AC_MSG_RESULT([ Object directory: $objdir]) AC_MSG_RESULT([ Iproute2 directory: $iproutedir]) AC_MSG_RESULT([ System configuration: $sysdesc]) AC_MSG_RESULT([ Debugging: $enable_debug]) -AC_MSG_RESULT([ POSIX threads: $enable_pthreads]) AC_MSG_RESULT([ Routing protocols: $protocols]) AC_MSG_RESULT([ LibSSH support in RPKI: $enable_libssh]) AC_MSG_RESULT([ Kernel MPLS support: $enable_mpls_kernel]) diff --git a/lib/birdlib.h b/lib/birdlib.h index 2e642d38..dc8bd00f 100644 --- a/lib/birdlib.h +++ b/lib/birdlib.h @@ -77,10 +77,6 @@ static inline int u64_cmp(u64 i1, u64 i2) #define STATIC_ASSERT(EXP) _Static_assert(EXP, #EXP) #define STATIC_ASSERT_MSG(EXP,MSG) _Static_assert(EXP, MSG) -#ifndef HAVE_THREAD_LOCAL -#define _Thread_local -#endif - /* Microsecond time */ typedef s64 btime; diff --git a/lib/timer.c b/lib/timer.c index 381163d0..f978a0f3 100644 --- a/lib/timer.c +++ b/lib/timer.c @@ -40,8 +40,6 @@ struct timeloop main_timeloop; -#ifdef USE_PTHREADS - #include /* Data accessed and modified from proto/bfd/io.c */ @@ -62,14 +60,6 @@ timeloop_init_current(void) void wakeup_kick_current(void); -#else - -/* Just use main timelooop */ -static inline struct timeloop * timeloop_current(void) { return &main_timeloop; } -static inline void timeloop_init_current(void) { } - -#endif - btime current_time(void) { diff --git a/sysdep/unix/log.c b/sysdep/unix/log.c index 4e9df069..a23903b7 100644 --- a/sysdep/unix/log.c +++ b/sysdep/unix/log.c @@ -36,8 +36,6 @@ static list *current_log_list; static char *current_syslog_name; /* NULL -> syslog closed */ -#ifdef USE_PTHREADS - #include static pthread_mutex_t log_mutex; @@ -48,15 +46,6 @@ static pthread_t main_thread; void main_thread_init(void) { main_thread = pthread_self(); } static int main_thread_self(void) { return pthread_equal(pthread_self(), main_thread); } -#else - -static inline void log_lock(void) { } -static inline void log_unlock(void) { } -void main_thread_init(void) { } -static int main_thread_self(void) { return 1; } - -#endif - #ifdef HAVE_SYSLOG_H #include From feb17ced234bad13ae64b52a3f86241f74517997 Mon Sep 17 00:00:00 2001 From: Maria Matejka Date: Fri, 18 Jun 2021 18:10:42 +0200 Subject: [PATCH 2/7] Dropping the POSIX thread-local variables in favor of much easier-to-use C11 thread-local variables --- lib/timer.c | 47 ++++++++++++++------------------------------ lib/timer.h | 1 + proto/bfd/io.c | 53 +++++++++++++------------------------------------- 3 files changed, 29 insertions(+), 72 deletions(-) diff --git a/lib/timer.c b/lib/timer.c index f978a0f3..6efcadb4 100644 --- a/lib/timer.c +++ b/lib/timer.c @@ -43,38 +43,23 @@ struct timeloop main_timeloop; #include /* Data accessed and modified from proto/bfd/io.c */ -pthread_key_t current_time_key; - -static inline struct timeloop * -timeloop_current(void) -{ - return pthread_getspecific(current_time_key); -} - -static inline void -timeloop_init_current(void) -{ - pthread_key_create(¤t_time_key, NULL); - pthread_setspecific(current_time_key, &main_timeloop); -} +_Thread_local struct timeloop *local_timeloop; void wakeup_kick_current(void); btime current_time(void) { - return timeloop_current()->last_time; + return local_timeloop->last_time; } btime current_real_time(void) { - struct timeloop *loop = timeloop_current(); + if (!local_timeloop->real_time) + times_update_real_time(local_timeloop); - if (!loop->real_time) - times_update_real_time(loop); - - return loop->real_time; + return local_timeloop->real_time; } @@ -128,30 +113,29 @@ tm_new(pool *p) void tm_set(timer *t, btime when) { - struct timeloop *loop = timeloop_current(); - uint tc = timers_count(loop); + uint tc = timers_count(local_timeloop); if (!t->expires) { t->index = ++tc; t->expires = when; - BUFFER_PUSH(loop->timers) = t; - HEAP_INSERT(loop->timers.data, tc, timer *, TIMER_LESS, TIMER_SWAP); + BUFFER_PUSH(local_timeloop->timers) = t; + HEAP_INSERT(local_timeloop->timers.data, tc, timer *, TIMER_LESS, TIMER_SWAP); } else if (t->expires < when) { t->expires = when; - HEAP_INCREASE(loop->timers.data, tc, timer *, TIMER_LESS, TIMER_SWAP, t->index); + HEAP_INCREASE(local_timeloop->timers.data, tc, timer *, TIMER_LESS, TIMER_SWAP, t->index); } else if (t->expires > when) { t->expires = when; - HEAP_DECREASE(loop->timers.data, tc, timer *, TIMER_LESS, TIMER_SWAP, t->index); + HEAP_DECREASE(local_timeloop->timers.data, tc, timer *, TIMER_LESS, TIMER_SWAP, t->index); } #ifdef CONFIG_BFD /* Hack to notify BFD loops */ - if ((loop != &main_timeloop) && (t->index == 1)) + if ((local_timeloop != &main_timeloop) && (t->index == 1)) wakeup_kick_current(); #endif } @@ -168,11 +152,10 @@ tm_stop(timer *t) if (!t->expires) return; - struct timeloop *loop = timeloop_current(); - uint tc = timers_count(loop); + uint tc = timers_count(local_timeloop); - HEAP_DELETE(loop->timers.data, tc, timer *, TIMER_LESS, TIMER_SWAP, t->index); - BUFFER_POP(loop->timers); + HEAP_DELETE(local_timeloop->timers.data, tc, timer *, TIMER_LESS, TIMER_SWAP, t->index); + BUFFER_POP(local_timeloop->timers); t->index = -1; t->expires = 0; @@ -230,7 +213,7 @@ void timer_init(void) { timers_init(&main_timeloop, &root_pool); - timeloop_init_current(); + local_timeloop = &main_timeloop; } diff --git a/lib/timer.h b/lib/timer.h index c5ea430c..bc568ee6 100644 --- a/lib/timer.h +++ b/lib/timer.h @@ -42,6 +42,7 @@ static inline timer *timers_first(struct timeloop *loop) { return (loop->timers.used > 1) ? loop->timers.data[1] : NULL; } extern struct timeloop main_timeloop; +extern _Thread_local struct timeloop *local_timeloop; btime current_time(void); btime current_real_time(void); diff --git a/proto/bfd/io.c b/proto/bfd/io.c index 1cd9365a..8fdc84fb 100644 --- a/proto/bfd/io.c +++ b/proto/bfd/io.c @@ -52,29 +52,15 @@ struct birdloop * Current thread context */ -static pthread_key_t current_loop_key; -extern pthread_key_t current_time_key; - -static inline struct birdloop * -birdloop_current(void) -{ - return pthread_getspecific(current_loop_key); -} +static _Thread_local struct birdloop *birdloop_current; static inline void birdloop_set_current(struct birdloop *loop) { - pthread_setspecific(current_loop_key, loop); - pthread_setspecific(current_time_key, loop ? &loop->time : &main_timeloop); + birdloop_current = loop; + local_timeloop = loop ? &loop->time : &main_timeloop; } -static inline void -birdloop_init_current(void) -{ - pthread_key_create(¤t_loop_key, NULL); -} - - /* * Wakeup code for birdloop */ @@ -162,10 +148,8 @@ wakeup_kick(struct birdloop *loop) void wakeup_kick_current(void) { - struct birdloop *loop = birdloop_current(); - - if (loop && loop->poll_active) - wakeup_kick(loop); + if (birdloop_current && birdloop_current->poll_active) + wakeup_kick(birdloop_current); } @@ -195,15 +179,13 @@ events_fire(struct birdloop *loop) void ev2_schedule(event *e) { - struct birdloop *loop = birdloop_current(); - - if (loop->poll_active && EMPTY_LIST(loop->event_list)) - wakeup_kick(loop); + if (birdloop_current->poll_active && EMPTY_LIST(birdloop_current->event_list)) + wakeup_kick(birdloop_current); if (e->n.next) rem_node(&e->n); - add_tail(&loop->event_list, &e->n); + add_tail(&birdloop_current->event_list, &e->n); } @@ -238,9 +220,7 @@ sockets_add(struct birdloop *loop, sock *s) void sk_start(sock *s) { - struct birdloop *loop = birdloop_current(); - - sockets_add(loop, s); + sockets_add(birdloop_current, s); } static void @@ -261,14 +241,12 @@ sockets_remove(struct birdloop *loop, sock *s) void sk_stop(sock *s) { - struct birdloop *loop = birdloop_current(); + sockets_remove(birdloop_current, s); - sockets_remove(loop, s); - - if (loop->poll_active) + if (birdloop_current->poll_active) { - loop->close_scheduled = 1; - wakeup_kick(loop); + birdloop_current->close_scheduled = 1; + wakeup_kick(birdloop_current); } else close(s->fd); @@ -392,11 +370,6 @@ static void * birdloop_main(void *arg); struct birdloop * birdloop_new(void) { - /* FIXME: this init should be elsewhere and thread-safe */ - static int init = 0; - if (!init) - { birdloop_init_current(); init = 1; } - pool *p = rp_new(NULL, "Birdloop root"); struct birdloop *loop = mb_allocz(p, sizeof(struct birdloop)); loop->pool = p; From 1db83a507a9ae287815d62733d1337074993b433 Mon Sep 17 00:00:00 2001 From: Maria Matejka Date: Thu, 4 Feb 2021 15:52:42 +0100 Subject: [PATCH 3/7] Locking subsystem: Just a global BIRD lock to begin with. --- lib/birdlib.h | 1 + lib/lists.c | 2 +- lib/locking.h | 46 ++++++++++++++++++ sysdep/unix/Makefile | 2 +- sysdep/unix/coroutine.c | 102 ++++++++++++++++++++++++++++++++++++++++ sysdep/unix/io.c | 3 ++ sysdep/unix/main.c | 3 ++ 7 files changed, 157 insertions(+), 2 deletions(-) create mode 100644 lib/locking.h create mode 100644 sysdep/unix/coroutine.c diff --git a/lib/birdlib.h b/lib/birdlib.h index dc8bd00f..3dc39d19 100644 --- a/lib/birdlib.h +++ b/lib/birdlib.h @@ -9,6 +9,7 @@ #ifndef _BIRD_BIRDLIB_H_ #define _BIRD_BIRDLIB_H_ +#include "sysdep/config.h" #include "lib/alloca.h" /* Ugly structure offset handling macros */ diff --git a/lib/lists.c b/lib/lists.c index 58d51073..dc2e4cbb 100644 --- a/lib/lists.c +++ b/lib/lists.c @@ -26,7 +26,7 @@ #define _BIRD_LISTS_C_ -#include "nest/bird.h" +#include "lib/birdlib.h" #include "lib/lists.h" LIST_INLINE int diff --git a/lib/locking.h b/lib/locking.h new file mode 100644 index 00000000..eb1bc8fa --- /dev/null +++ b/lib/locking.h @@ -0,0 +1,46 @@ +/* + * BIRD Library -- Locking + * + * (c) 2020--2021 Maria Matejka + * + * Can be freely distributed and used under the terms of the GNU GPL. + */ + +#ifndef _BIRD_LOCKING_H_ +#define _BIRD_LOCKING_H_ + +struct domain_generic; + +/* Here define the global lock order; first to last. */ +struct lock_order { + struct domain_generic *the_bird; +}; + +#define LOCK_ORDER_DEPTH (sizeof(struct lock_order) / sizeof(struct domain_generic *)) + +extern _Thread_local struct lock_order locking_stack; +extern _Thread_local struct domain_generic **last_locked; + +#define DOMAIN(type) struct domain__##type +#define DEFINE_DOMAIN(type) DOMAIN(type) { struct domain_generic *type; } + +#define DOMAIN_NEW(type, name) (DOMAIN(type)) { .type = domain_new(name) } +struct domain_generic *domain_new(const char *name); + +#define DOMAIN_NULL(type) (DOMAIN(type)) {} + +#define LOCK_DOMAIN(type, d) do_lock(((d).type), &(locking_stack.type)) +#define UNLOCK_DOMAIN(type, d) do_unlock(((d).type), &(locking_stack.type)) + +/* Internal for locking */ +void do_lock(struct domain_generic *dg, struct domain_generic **lsp); +void do_unlock(struct domain_generic *dg, struct domain_generic **lsp); + +/* Use with care. To be removed in near future. */ +DEFINE_DOMAIN(the_bird); +extern DOMAIN(the_bird) the_bird_domain; + +#define the_bird_lock() LOCK_DOMAIN(the_bird, the_bird_domain) +#define the_bird_unlock() UNLOCK_DOMAIN(the_bird, the_bird_domain) + +#endif diff --git a/sysdep/unix/Makefile b/sysdep/unix/Makefile index d0d36b5f..69cf8131 100644 --- a/sysdep/unix/Makefile +++ b/sysdep/unix/Makefile @@ -1,4 +1,4 @@ -src := alloc.c io.c krt.c log.c main.c random.c +src := alloc.c io.c krt.c log.c main.c random.c coroutine.c obj := $(src-o-files) $(all-daemon) $(cf-local) diff --git a/sysdep/unix/coroutine.c b/sysdep/unix/coroutine.c new file mode 100644 index 00000000..05f101fb --- /dev/null +++ b/sysdep/unix/coroutine.c @@ -0,0 +1,102 @@ +/* + * BIRD Coroutines + * + * (c) 2017 Martin Mares + * (c) 2020 Maria Matejka + * + * Can be freely distributed and used under the terms of the GNU GPL. + */ + +#ifndef _GNU_SOURCE +#define _GNU_SOURCE +#endif + +#undef LOCAL_DEBUG + +#undef DEBUG_LOCKING + +#include "lib/birdlib.h" +#include "lib/locking.h" +#include "lib/resource.h" + +/* + * Implementation of coroutines based on POSIX threads + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +/* + * Locking subsystem + */ + +#define ASSERT_NO_LOCK ASSERT_DIE(last_locked == NULL) + +struct domain_generic { + pthread_mutex_t mutex; + struct domain_generic **prev; + struct lock_order *locked_by; + const char *name; +}; + +#define DOMAIN_INIT(_name) { .mutex = PTHREAD_MUTEX_INITIALIZER, .name = _name } + +static struct domain_generic the_bird_domain_gen = DOMAIN_INIT("The BIRD"); + +DOMAIN(the_bird) the_bird_domain = { .the_bird = &the_bird_domain_gen }; + +struct domain_generic * +domain_new(const char *name) +{ + struct domain_generic *dg = xmalloc(sizeof(struct domain_generic)); + *dg = (struct domain_generic) DOMAIN_INIT(name); + return dg; +} + +void +domain_free(struct domain_generic *dg) +{ + pthread_mutex_destroy(&dg->mutex); + xfree(dg); +} + +_Thread_local struct lock_order locking_stack = {}; +_Thread_local struct domain_generic **last_locked = NULL; + +void do_lock(struct domain_generic *dg, struct domain_generic **lsp) +{ + if (lsp <= last_locked) + bug("Trying to lock in a bad order"); + if (*lsp) + bug("Inconsistent locking stack state on lock"); + + pthread_mutex_lock(&dg->mutex); + + if (dg->prev || dg->locked_by) + bug("Previous unlock not finished correctly"); + dg->prev = last_locked; + *lsp = dg; + last_locked = lsp; + dg->locked_by = &locking_stack; +} + +void do_unlock(struct domain_generic *dg, struct domain_generic **lsp) +{ + if (dg->locked_by != &locking_stack) + bug("Inconsistent domain state on unlock"); + if ((last_locked != lsp) || (*lsp != dg)) + bug("Inconsistent locking stack state on unlock"); + dg->locked_by = NULL; + last_locked = dg->prev; + *lsp = NULL; + dg->prev = NULL; + pthread_mutex_unlock(&dg->mutex); +} + diff --git a/sysdep/unix/io.c b/sysdep/unix/io.c index 3d67d0a7..29467867 100644 --- a/sysdep/unix/io.c +++ b/sysdep/unix/io.c @@ -36,6 +36,7 @@ #include "lib/resource.h" #include "lib/socket.h" #include "lib/event.h" +#include "lib/locking.h" #include "lib/timer.h" #include "lib/string.h" #include "nest/iface.h" @@ -2263,7 +2264,9 @@ io_loop(void) /* And finally enter poll() to find active sockets */ watchdog_stop(); + the_bird_unlock(); pout = poll(pfd, nfds, poll_tout); + the_bird_lock(); watchdog_start(); if (pout < 0) diff --git a/sysdep/unix/main.c b/sysdep/unix/main.c index 7e8ea0dc..dabfc554 100644 --- a/sysdep/unix/main.c +++ b/sysdep/unix/main.c @@ -28,6 +28,7 @@ #include "lib/resource.h" #include "lib/socket.h" #include "lib/event.h" +#include "lib/locking.h" #include "lib/timer.h" #include "lib/string.h" #include "nest/route.h" @@ -959,6 +960,8 @@ main(int argc, char **argv) dup2(0, 2); } + the_bird_lock(); + main_thread_init(); write_pid_file(); From 1289c1c5eede5b3d015d06b725d30024ccac51bd Mon Sep 17 00:00:00 2001 From: Maria Matejka Date: Mon, 8 Feb 2021 09:51:59 +0100 Subject: [PATCH 4/7] Coroutines: A simple and lightweight parallel execution framework. --- lib/coro.h | 26 ++++++++++++++++ sysdep/unix/coroutine.c | 69 +++++++++++++++++++++++++++++++++++++++++ sysdep/unix/io.c | 27 +++++++++++++++- sysdep/unix/log.c | 34 ++++++++++++++++++-- sysdep/unix/unix.h | 1 + 5 files changed, 153 insertions(+), 4 deletions(-) create mode 100644 lib/coro.h diff --git a/lib/coro.h b/lib/coro.h new file mode 100644 index 00000000..51712b36 --- /dev/null +++ b/lib/coro.h @@ -0,0 +1,26 @@ +/* + * BIRD Coroutines + * + * (c) 2017 Martin Mares + * (c) 2020 Maria Matejka + * + * Can be freely distributed and used under the terms of the GNU GPL. + */ + +#ifndef _BIRD_CORO_H_ +#define _BIRD_CORO_H_ + +#include "lib/resource.h" + +/* A completely opaque coroutine handle. */ +struct coroutine; + +/* Coroutines are independent threads bound to pools. + * You request a coroutine by calling coro_run(). + * It is forbidden to free a running coroutine from outside. + * The running coroutine must free itself by rfree() before returning. + */ +struct coroutine *coro_run(pool *, void (*entry)(void *), void *data); + + +#endif diff --git a/sysdep/unix/coroutine.c b/sysdep/unix/coroutine.c index 05f101fb..71847505 100644 --- a/sysdep/unix/coroutine.c +++ b/sysdep/unix/coroutine.c @@ -17,7 +17,14 @@ #include "lib/birdlib.h" #include "lib/locking.h" +#include "lib/coro.h" #include "lib/resource.h" +#include "lib/timer.h" + +/* Using a rather big stack for coroutines to allow for stack-local allocations. + * In real world, the kernel doesn't alloc this memory until it is used. + * */ +#define CORO_STACK_SIZE 1048576 /* * Implementation of coroutines based on POSIX threads @@ -100,3 +107,65 @@ void do_unlock(struct domain_generic *dg, struct domain_generic **lsp) pthread_mutex_unlock(&dg->mutex); } +/* Coroutines */ +struct coroutine { + resource r; + pthread_t id; + pthread_attr_t attr; + void (*entry)(void *); + void *data; +}; + +static _Thread_local _Bool coro_cleaned_up = 0; + +static void coro_free(resource *r) +{ + struct coroutine *c = (void *) r; + ASSERT_DIE(pthread_equal(pthread_self(), c->id)); + pthread_attr_destroy(&c->attr); + coro_cleaned_up = 1; +} + +static struct resclass coro_class = { + .name = "Coroutine", + .size = sizeof(struct coroutine), + .free = coro_free, +}; + +static void *coro_entry(void *p) +{ + struct coroutine *c = p; + ASSERT_DIE(c->entry); + + c->entry(c->data); + ASSERT_DIE(coro_cleaned_up); + + return NULL; +} + +struct coroutine *coro_run(pool *p, void (*entry)(void *), void *data) +{ + ASSERT_DIE(entry); + ASSERT_DIE(p); + + struct coroutine *c = ralloc(p, &coro_class); + + c->entry = entry; + c->data = data; + + int e = 0; + + if (e = pthread_attr_init(&c->attr)) + die("pthread_attr_init() failed: %M", e); + + if (e = pthread_attr_setstacksize(&c->attr, CORO_STACK_SIZE)) + die("pthread_attr_setstacksize(%u) failed: %M", CORO_STACK_SIZE, e); + + if (e = pthread_attr_setdetachstate(&c->attr, PTHREAD_CREATE_DETACHED)) + die("pthread_attr_setdetachstate(PTHREAD_CREATE_DETACHED) failed: %M", e); + + if (e = pthread_create(&c->id, &c->attr, coro_entry, c)) + die("pthread_create() failed: %M", e); + + return c; +} diff --git a/sysdep/unix/io.c b/sysdep/unix/io.c index 29467867..40841ea4 100644 --- a/sysdep/unix/io.c +++ b/sysdep/unix/io.c @@ -2176,6 +2176,15 @@ static int short_loops = 0; #define SHORT_LOOP_MAX 10 #define WORK_EVENTS_MAX 10 +static int poll_reload_pipe[2]; + +void +io_loop_reload(void) +{ + char b; + write(poll_reload_pipe[1], &b, 1); +} + void io_loop(void) { @@ -2187,6 +2196,9 @@ io_loop(void) int fdmax = 256; struct pollfd *pfd = xmalloc(fdmax * sizeof(struct pollfd)); + if (pipe(poll_reload_pipe) < 0) + die("pipe(poll_reload_pipe) failed: %m"); + watchdog_start1(); for(;;) { @@ -2205,7 +2217,12 @@ io_loop(void) poll_tout = MIN(poll_tout, timeout); } - nfds = 0; + /* A hack to reload main io_loop() when something has changed asynchronously. */ + pfd[0].fd = poll_reload_pipe[0]; + pfd[0].events = POLLIN; + + nfds = 1; + WALK_LIST(n, sock_list) { pfd[nfds] = (struct pollfd) { .fd = -1 }; /* everything other set to 0 by this */ @@ -2277,6 +2294,14 @@ io_loop(void) } if (pout) { + if (pfd[0].revents & POLLIN) + { + /* IO loop reload requested */ + char b; + read(poll_reload_pipe[0], &b, 1); + continue; + } + times_update(&main_timeloop); /* guaranteed to be non-empty */ diff --git a/sysdep/unix/log.c b/sysdep/unix/log.c index a23903b7..dc2b14b3 100644 --- a/sysdep/unix/log.c +++ b/sysdep/unix/log.c @@ -15,6 +15,7 @@ * user's manual. */ +#include #include #include #include @@ -35,6 +36,10 @@ static FILE *dbgf; static list *current_log_list; static char *current_syslog_name; /* NULL -> syslog closed */ +static _Atomic uint max_coro_id = ATOMIC_VAR_INIT(1); +static _Thread_local uint this_coro_id; + +#define THIS_CORO_ID (this_coro_id ?: (this_coro_id = atomic_fetch_add_explicit(&max_coro_id, 1, memory_order_acq_rel))) #include @@ -178,7 +183,7 @@ log_commit(int class, buffer *buf) l->pos += msg_len; } - fprintf(l->fh, "%s <%s> ", tbuf, class_names[class]); + fprintf(l->fh, "%s [%04x] <%s> ", tbuf, THIS_CORO_ID, class_names[class]); } fputs(buf->start, l->fh); fputc('\n', l->fh); @@ -288,6 +293,8 @@ die(const char *msg, ...) exit(1); } +static struct timespec dbg_time_start; + /** * debug - write to debug output * @msg: a printf-like message @@ -300,12 +307,33 @@ debug(const char *msg, ...) { #define MAX_DEBUG_BUFSIZE 16384 va_list args; - char buf[MAX_DEBUG_BUFSIZE]; + char buf[MAX_DEBUG_BUFSIZE], *pos = buf; + int max = MAX_DEBUG_BUFSIZE; va_start(args, msg); if (dbgf) { - if (bvsnprintf(buf, MAX_DEBUG_BUFSIZE, msg, args) < 0) + struct timespec dbg_time; + clock_gettime(CLOCK_MONOTONIC, &dbg_time); + uint nsec; + uint sec; + + if (dbg_time.tv_nsec > dbg_time_start.tv_nsec) + { + nsec = dbg_time.tv_nsec - dbg_time_start.tv_nsec; + sec = dbg_time.tv_sec - dbg_time_start.tv_sec; + } + else + { + nsec = 1000000000 + dbg_time.tv_nsec - dbg_time_start.tv_nsec; + sec = dbg_time.tv_sec - dbg_time_start.tv_sec - 1; + } + + int n = bsnprintf(pos, max, "%u.%09u: [%04x] ", sec, nsec, THIS_CORO_ID); + pos += n; + max -= n; + + if (bvsnprintf(pos, max, msg, args) < 0) bug("Extremely long debug output, split it."); fputs(buf, dbgf); diff --git a/sysdep/unix/unix.h b/sysdep/unix/unix.h index ad85d1ea..313c97c3 100644 --- a/sysdep/unix/unix.h +++ b/sysdep/unix/unix.h @@ -106,6 +106,7 @@ extern volatile sig_atomic_t async_shutdown_flag; void io_init(void); void io_loop(void); +void io_loop_reload(void); void io_log_dump(void); int sk_open_unix(struct birdsock *s, char *name); struct rfile *rf_open(struct pool *, const char *name, const char *mode); From df3264f51ff38c9366398564a9d342a26bc83f37 Mon Sep 17 00:00:00 2001 From: Maria Matejka Date: Mon, 24 May 2021 13:41:23 +0200 Subject: [PATCH 5/7] Lock position checking allows for safe lock unions --- lib/locking.h | 6 ++---- sysdep/unix/coroutine.c | 22 +++++++++++++++------- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/lib/locking.h b/lib/locking.h index eb1bc8fa..eef60154 100644 --- a/lib/locking.h +++ b/lib/locking.h @@ -16,16 +16,14 @@ struct lock_order { struct domain_generic *the_bird; }; -#define LOCK_ORDER_DEPTH (sizeof(struct lock_order) / sizeof(struct domain_generic *)) - extern _Thread_local struct lock_order locking_stack; extern _Thread_local struct domain_generic **last_locked; #define DOMAIN(type) struct domain__##type #define DEFINE_DOMAIN(type) DOMAIN(type) { struct domain_generic *type; } -#define DOMAIN_NEW(type, name) (DOMAIN(type)) { .type = domain_new(name) } -struct domain_generic *domain_new(const char *name); +#define DOMAIN_NEW(type, name) (DOMAIN(type)) { .type = domain_new(name, OFFSETOF(struct lock_order, type)) } +struct domain_generic *domain_new(const char *name, uint order); #define DOMAIN_NULL(type) (DOMAIN(type)) {} diff --git a/sysdep/unix/coroutine.c b/sysdep/unix/coroutine.c index 71847505..2eba142c 100644 --- a/sysdep/unix/coroutine.c +++ b/sysdep/unix/coroutine.c @@ -44,26 +44,31 @@ * Locking subsystem */ +_Thread_local struct lock_order locking_stack = {}; +_Thread_local struct domain_generic **last_locked = NULL; + #define ASSERT_NO_LOCK ASSERT_DIE(last_locked == NULL) struct domain_generic { pthread_mutex_t mutex; + uint order; struct domain_generic **prev; struct lock_order *locked_by; const char *name; }; -#define DOMAIN_INIT(_name) { .mutex = PTHREAD_MUTEX_INITIALIZER, .name = _name } +#define DOMAIN_INIT(_name, _order) { .mutex = PTHREAD_MUTEX_INITIALIZER, .name = _name, .order = _order } -static struct domain_generic the_bird_domain_gen = DOMAIN_INIT("The BIRD"); +static struct domain_generic the_bird_domain_gen = DOMAIN_INIT("The BIRD", OFFSETOF(struct lock_order, the_bird)); DOMAIN(the_bird) the_bird_domain = { .the_bird = &the_bird_domain_gen }; struct domain_generic * -domain_new(const char *name) +domain_new(const char *name, uint order) { + ASSERT_DIE(order < sizeof(struct lock_order)); struct domain_generic *dg = xmalloc(sizeof(struct domain_generic)); - *dg = (struct domain_generic) DOMAIN_INIT(name); + *dg = (struct domain_generic) DOMAIN_INIT(name, order); return dg; } @@ -74,11 +79,11 @@ domain_free(struct domain_generic *dg) xfree(dg); } -_Thread_local struct lock_order locking_stack = {}; -_Thread_local struct domain_generic **last_locked = NULL; - void do_lock(struct domain_generic *dg, struct domain_generic **lsp) { + if ((char *) lsp - (char *) &locking_stack != dg->order) + bug("Trying to lock on bad position: order=%u, lsp=%p, base=%p", dg->order, lsp, &locking_stack); + if (lsp <= last_locked) bug("Trying to lock in a bad order"); if (*lsp) @@ -96,6 +101,9 @@ void do_lock(struct domain_generic *dg, struct domain_generic **lsp) void do_unlock(struct domain_generic *dg, struct domain_generic **lsp) { + if ((char *) lsp - (char *) &locking_stack != dg->order) + bug("Trying to unlock on bad position: order=%u, lsp=%p, base=%p", dg->order, lsp, &locking_stack); + if (dg->locked_by != &locking_stack) bug("Inconsistent domain state on unlock"); if ((last_locked != lsp) || (*lsp != dg)) From a2af807357875f866f149b0f6db4d463d4533204 Mon Sep 17 00:00:00 2001 From: Maria Matejka Date: Mon, 28 Jun 2021 15:43:45 +0200 Subject: [PATCH 6/7] Debug messages with timestamps. On most of current hardware, getting monotonic clock is fast enough to get it and write for each debug message. --- sysdep/unix/log.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sysdep/unix/log.c b/sysdep/unix/log.c index dc2b14b3..f48588b6 100644 --- a/sysdep/unix/log.c +++ b/sysdep/unix/log.c @@ -439,6 +439,8 @@ done: void log_init_debug(char *f) { + clock_gettime(CLOCK_MONOTONIC, &dbg_time_start); + if (dbgf && dbgf != stderr) fclose(dbgf); if (!f) From a4451535c69b8f934523905a8131ae2f16be2146 Mon Sep 17 00:00:00 2001 From: Maria Matejka Date: Wed, 4 Aug 2021 22:48:51 +0200 Subject: [PATCH 7/7] Unified time for whole BIRD In previous versions, every thread used its own time structures, effectively leading to different time in every thread and strange logging messages. The time processing code now uses global atomic variables to keep current time available for fast concurrent reading and safe updates. --- lib/timer.c | 29 +++++---------- lib/timer.h | 14 ++++---- proto/bfd/io.c | 8 ++--- sysdep/unix/io.c | 90 +++++++++++++++++++--------------------------- sysdep/unix/main.c | 1 + 5 files changed, 56 insertions(+), 86 deletions(-) diff --git a/lib/timer.c b/lib/timer.c index 6efcadb4..ff1fb5ef 100644 --- a/lib/timer.c +++ b/lib/timer.c @@ -32,6 +32,7 @@ #include "nest/bird.h" +#include "lib/coro.h" #include "lib/heap.h" #include "lib/resource.h" #include "lib/timer.h" @@ -45,23 +46,11 @@ struct timeloop main_timeloop; /* Data accessed and modified from proto/bfd/io.c */ _Thread_local struct timeloop *local_timeloop; +_Atomic btime last_time; +_Atomic btime real_time; + void wakeup_kick_current(void); -btime -current_time(void) -{ - return local_timeloop->last_time; -} - -btime -current_real_time(void) -{ - if (!local_timeloop->real_time) - times_update_real_time(local_timeloop); - - return local_timeloop->real_time; -} - #define TIMER_LESS(a,b) ((a)->expires < (b)->expires) #define TIMER_SWAP(heap,a,b,t) (t = heap[a], heap[a] = heap[b], heap[b] = t, \ @@ -164,8 +153,6 @@ tm_stop(timer *t) void timers_init(struct timeloop *loop, pool *p) { - times_init(loop); - BUFFER_INIT(loop->timers, p, 4); BUFFER_PUSH(loop->timers) = NULL; } @@ -178,8 +165,8 @@ timers_fire(struct timeloop *loop) btime base_time; timer *t; - times_update(loop); - base_time = loop->last_time; + times_update(); + base_time = current_time(); while (t = timers_first(loop)) { @@ -190,8 +177,8 @@ timers_fire(struct timeloop *loop) { btime when = t->expires + t->recurrent; - if (when <= loop->last_time) - when = loop->last_time + t->recurrent; + if (when <= base_time) + when = base_time + t->recurrent; if (t->randomize) when += random() % (t->randomize + 1); diff --git a/lib/timer.h b/lib/timer.h index bc568ee6..b201b8c8 100644 --- a/lib/timer.h +++ b/lib/timer.h @@ -14,6 +14,10 @@ #include "lib/buffer.h" #include "lib/resource.h" +#include + +extern _Atomic btime last_time; +extern _Atomic btime real_time; typedef struct timer { @@ -31,8 +35,6 @@ typedef struct timer struct timeloop { BUFFER_(timer *) timers; - btime last_time; - btime real_time; }; static inline uint timers_count(struct timeloop *loop) @@ -44,8 +46,8 @@ static inline timer *timers_first(struct timeloop *loop) extern struct timeloop main_timeloop; extern _Thread_local struct timeloop *local_timeloop; -btime current_time(void); -btime current_real_time(void); +#define current_time() atomic_load_explicit(&last_time, memory_order_acquire) +#define current_real_time() atomic_load_explicit(&real_time, memory_order_acquire) //#define now (current_time() TO_S) //#define now_real (current_real_time() TO_S) @@ -95,9 +97,7 @@ tm_start_max(timer *t, btime after) } /* In sysdep code */ -void times_init(struct timeloop *loop); -void times_update(struct timeloop *loop); -void times_update_real_time(struct timeloop *loop); +void times_update(void); /* For I/O loop */ void timers_init(struct timeloop *loop, pool *p); diff --git a/proto/bfd/io.c b/proto/bfd/io.c index 8fdc84fb..c5f1e024 100644 --- a/proto/bfd/io.c +++ b/proto/bfd/io.c @@ -172,7 +172,7 @@ events_init(struct birdloop *loop) static void events_fire(struct birdloop *loop) { - times_update(&loop->time); + times_update(); ev_run_list(&loop->event_list); } @@ -332,7 +332,7 @@ sockets_fire(struct birdloop *loop) sock **psk = loop->poll_sk.data; int poll_num = loop->poll_fd.used - 1; - times_update(&loop->time); + times_update(); /* Last fd is internal wakeup fd */ if (pfd[poll_num].revents & POLLIN) @@ -365,7 +365,7 @@ sockets_fire(struct birdloop *loop) * Birdloop */ -static void * birdloop_main(void *arg); +static void *birdloop_main(void *arg); struct birdloop * birdloop_new(void) @@ -461,7 +461,7 @@ birdloop_main(void *arg) events_fire(loop); timers_fire(&loop->time); - times_update(&loop->time); + times_update(); if (events_waiting(loop)) timeout = 0; else if (t = timers_first(&loop->time)) diff --git a/sysdep/unix/io.c b/sysdep/unix/io.c index 40841ea4..90bb5d64 100644 --- a/sysdep/unix/io.c +++ b/sysdep/unix/io.c @@ -123,55 +123,50 @@ rf_fileno(struct rfile *f) btime boot_time; + void -times_init(struct timeloop *loop) +times_update(void) { struct timespec ts; int rv; + btime old_time = current_time(); + btime old_real_time = current_real_time(); + rv = clock_gettime(CLOCK_MONOTONIC, &ts); if (rv < 0) die("Monotonic clock is missing"); if ((ts.tv_sec < 0) || (((u64) ts.tv_sec) > ((u64) 1 << 40))) log(L_WARN "Monotonic clock is crazy"); - - loop->last_time = ts.tv_sec S + ts.tv_nsec NS; - loop->real_time = 0; -} - -void -times_update(struct timeloop *loop) -{ - struct timespec ts; - int rv; - - rv = clock_gettime(CLOCK_MONOTONIC, &ts); - if (rv < 0) - die("clock_gettime: %m"); - + btime new_time = ts.tv_sec S + ts.tv_nsec NS; - if (new_time < loop->last_time) + if (new_time < old_time) log(L_ERR "Monotonic clock is broken"); - loop->last_time = new_time; - loop->real_time = 0; -} - -void -times_update_real_time(struct timeloop *loop) -{ - struct timespec ts; - int rv; - rv = clock_gettime(CLOCK_REALTIME, &ts); if (rv < 0) die("clock_gettime: %m"); - loop->real_time = ts.tv_sec S + ts.tv_nsec NS; -} + btime new_real_time = ts.tv_sec S + ts.tv_nsec NS; + if (!atomic_compare_exchange_strong_explicit( + &last_time, + &old_time, + new_time, + memory_order_acq_rel, + memory_order_relaxed)) + DBG("Time update collision: last_time"); + + if (!atomic_compare_exchange_strong_explicit( + &real_time, + &old_real_time, + new_real_time, + memory_order_acq_rel, + memory_order_relaxed)) + DBG("Time update collision: real_time"); +} /** * DOC: Sockets @@ -2017,30 +2012,17 @@ struct event_log_entry static struct event_log_entry event_log[EVENT_LOG_LENGTH]; static struct event_log_entry *event_open; static int event_log_pos, event_log_num, watchdog_active; -static btime last_time; +static btime last_io_time; static btime loop_time; static void io_update_time(void) { - struct timespec ts; - int rv; - - /* - * This is third time-tracking procedure (after update_times() above and - * times_update() in BFD), dedicated to internal event log and latency - * tracking. Hopefully, we consolidate these sometimes. - */ - - rv = clock_gettime(CLOCK_MONOTONIC, &ts); - if (rv < 0) - die("clock_gettime: %m"); - - last_time = ts.tv_sec S + ts.tv_nsec NS; + last_io_time = current_time(); if (event_open) { - event_open->duration = last_time - event_open->timestamp; + event_open->duration = last_io_time - event_open->timestamp; if (event_open->duration > config->latency_limit) log(L_WARN "Event 0x%p 0x%p took %d ms", @@ -2069,7 +2051,7 @@ io_log_event(void *hook, void *data) en->hook = hook; en->data = data; - en->timestamp = last_time; + en->timestamp = last_io_time; en->duration = 0; event_log_num++; @@ -2097,14 +2079,14 @@ io_log_dump(void) struct event_log_entry *en = event_log + (event_log_pos + i) % EVENT_LOG_LENGTH; if (en->hook) log(L_DEBUG " Event 0x%p 0x%p at %8d for %d ms", en->hook, en->data, - (int) ((last_time - en->timestamp) TO_MS), (int) (en->duration TO_MS)); + (int) ((last_io_time - en->timestamp) TO_MS), (int) (en->duration TO_MS)); } } void watchdog_sigalrm(int sig UNUSED) { - /* Update last_time and duration, but skip latency check */ + /* Update last_io_time and duration, but skip latency check */ config->latency_limit = 0xffffffff; io_update_time(); @@ -2117,7 +2099,7 @@ watchdog_start1(void) { io_update_time(); - loop_time = last_time; + loop_time = last_io_time; } static inline void @@ -2125,7 +2107,7 @@ watchdog_start(void) { io_update_time(); - loop_time = last_time; + loop_time = last_io_time; event_log_num = 0; if (config->watchdog_timeout) @@ -2146,7 +2128,7 @@ watchdog_stop(void) watchdog_active = 0; } - btime duration = last_time - loop_time; + btime duration = last_io_time - loop_time; if (duration > config->watchdog_warning) log(L_WARN "I/O loop cycle took %d ms for %d events", (int) (duration TO_MS), event_log_num); @@ -2202,7 +2184,7 @@ io_loop(void) watchdog_start1(); for(;;) { - times_update(&main_timeloop); + times_update(); events = ev_run_list(&global_event_list); events = ev_run_list_limited(&global_work_list, WORK_EVENTS_MAX) || events; timers_fire(&main_timeloop); @@ -2212,7 +2194,7 @@ io_loop(void) poll_tout = (events ? 0 : 3000); /* Time in milliseconds */ if (t = timers_first(&main_timeloop)) { - times_update(&main_timeloop); + times_update(); timeout = (tm_remains(t) TO_MS) + 1; poll_tout = MIN(poll_tout, timeout); } @@ -2302,7 +2284,7 @@ io_loop(void) continue; } - times_update(&main_timeloop); + times_update(); /* guaranteed to be non-empty */ current_sock = SKIP_BACK(sock, n, HEAD(sock_list)); diff --git a/sysdep/unix/main.c b/sysdep/unix/main.c index dabfc554..d35424ff 100644 --- a/sysdep/unix/main.c +++ b/sysdep/unix/main.c @@ -903,6 +903,7 @@ main(int argc, char **argv) dmalloc_debug(0x2f03d00); #endif + times_update(); resource_sys_init(); parse_args(argc, argv); log_switch(1, NULL, NULL);