From 1ac267a2d9b422c26d3aba0a1b1cab91fbc3394d Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Wed, 12 Sep 2018 21:23:52 +0200 Subject: [PATCH 1/3] lib: remove frr_pthread->id All I can see is an unneccessary complication. If there's some purpose here it needs to be documented... Signed-off-by: David Lamparter --- bgpd/bgp_io.c | 14 ++-- bgpd/bgp_keepalives.c | 4 +- bgpd/bgpd.c | 23 +++--- bgpd/bgpd.h | 8 +- lib/frr_pthread.c | 139 ++++++++++------------------------- lib/frr_pthread.h | 21 ------ tests/bgpd/test_aspath.c | 6 +- tests/bgpd/test_capability.c | 2 +- zebra/zserv.c | 1 - 9 files changed, 68 insertions(+), 150 deletions(-) diff --git a/bgpd/bgp_io.c b/bgpd/bgp_io.c index c3bfbe4a90..95c3f15a66 100644 --- a/bgpd/bgp_io.c +++ b/bgpd/bgp_io.c @@ -23,7 +23,7 @@ #include #include // for pthread_mutex_unlock, pthread_mutex_lock -#include "frr_pthread.h" // for frr_pthread_get, frr_pthread +#include "frr_pthread.h" #include "linklist.h" // for list_delete, list_delete_all_node, lis... #include "log.h" // for zlog_debug, safe_strerror, zlog_err #include "memory.h" // for MTYPE_TMP, XCALLOC, XFREE @@ -56,7 +56,7 @@ static bool validate_header(struct peer *); void bgp_writes_on(struct peer *peer) { - struct frr_pthread *fpt = frr_pthread_get(PTHREAD_IO); + struct frr_pthread *fpt = bgp_pth_io; assert(fpt->running); assert(peer->status != Deleted); @@ -74,7 +74,7 @@ void bgp_writes_on(struct peer *peer) void bgp_writes_off(struct peer *peer) { - struct frr_pthread *fpt = frr_pthread_get(PTHREAD_IO); + struct frr_pthread *fpt = bgp_pth_io; assert(fpt->running); thread_cancel_async(fpt->master, &peer->t_write, NULL); @@ -85,7 +85,7 @@ void bgp_writes_off(struct peer *peer) void bgp_reads_on(struct peer *peer) { - struct frr_pthread *fpt = frr_pthread_get(PTHREAD_IO); + struct frr_pthread *fpt = bgp_pth_io; assert(fpt->running); assert(peer->status != Deleted); @@ -105,7 +105,7 @@ void bgp_reads_on(struct peer *peer) void bgp_reads_off(struct peer *peer) { - struct frr_pthread *fpt = frr_pthread_get(PTHREAD_IO); + struct frr_pthread *fpt = bgp_pth_io; assert(fpt->running); thread_cancel_async(fpt->master, &peer->t_read, NULL); @@ -130,7 +130,7 @@ static int bgp_process_writes(struct thread *thread) if (peer->fd < 0) return -1; - struct frr_pthread *fpt = frr_pthread_get(PTHREAD_IO); + struct frr_pthread *fpt = bgp_pth_io; pthread_mutex_lock(&peer->io_mtx); { @@ -182,7 +182,7 @@ static int bgp_process_reads(struct thread *thread) if (peer->fd < 0 || bm->terminating) return -1; - struct frr_pthread *fpt = frr_pthread_get(PTHREAD_IO); + struct frr_pthread *fpt = bgp_pth_io; pthread_mutex_lock(&peer->io_mtx); { diff --git a/bgpd/bgp_keepalives.c b/bgpd/bgp_keepalives.c index aeb95f91ba..91fa8fa373 100644 --- a/bgpd/bgp_keepalives.c +++ b/bgpd/bgp_keepalives.c @@ -229,7 +229,7 @@ void bgp_keepalives_on(struct peer *peer) if (CHECK_FLAG(peer->thread_flags, PEER_THREAD_KEEPALIVES_ON)) return; - struct frr_pthread *fpt = frr_pthread_get(PTHREAD_KEEPALIVES); + struct frr_pthread *fpt = bgp_pth_ka; assert(fpt->running); /* placeholder bucket data to use for fast key lookups */ @@ -259,7 +259,7 @@ void bgp_keepalives_off(struct peer *peer) if (!CHECK_FLAG(peer->thread_flags, PEER_THREAD_KEEPALIVES_ON)) return; - struct frr_pthread *fpt = frr_pthread_get(PTHREAD_KEEPALIVES); + struct frr_pthread *fpt = bgp_pth_ka; assert(fpt->running); /* placeholder bucket data to use for fast key lookups */ diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 7df8de55f0..552244b2f6 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -7750,35 +7750,36 @@ static const struct cmd_variable_handler bgp_viewvrf_var_handlers[] = { {.completions = NULL}, }; +struct frr_pthread *bgp_pth_io; +struct frr_pthread *bgp_pth_ka; + static void bgp_pthreads_init() { + assert(!bgp_pth_io); + assert(!bgp_pth_ka); + frr_pthread_init(); struct frr_pthread_attr io = { - .id = PTHREAD_IO, .start = frr_pthread_attr_default.start, .stop = frr_pthread_attr_default.stop, }; struct frr_pthread_attr ka = { - .id = PTHREAD_KEEPALIVES, .start = bgp_keepalives_start, .stop = bgp_keepalives_stop, }; - frr_pthread_new(&io, "BGP I/O thread", "bgpd_io"); - frr_pthread_new(&ka, "BGP Keepalives thread", "bgpd_ka"); + bgp_pth_io = frr_pthread_new(&io, "BGP I/O thread", "bgpd_io"); + bgp_pth_ka = frr_pthread_new(&ka, "BGP Keepalives thread", "bgpd_ka"); } void bgp_pthreads_run() { - struct frr_pthread *io = frr_pthread_get(PTHREAD_IO); - struct frr_pthread *ka = frr_pthread_get(PTHREAD_KEEPALIVES); - - frr_pthread_run(io, NULL); - frr_pthread_run(ka, NULL); + frr_pthread_run(bgp_pth_io, NULL); + frr_pthread_run(bgp_pth_ka, NULL); /* Wait until threads are ready. */ - frr_pthread_wait_running(io); - frr_pthread_wait_running(ka); + frr_pthread_wait_running(bgp_pth_io); + frr_pthread_wait_running(bgp_pth_ka); } void bgp_pthreads_finish() diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h index 0a8962b4c7..f644c0bd37 100644 --- a/bgpd/bgpd.h +++ b/bgpd/bgpd.h @@ -24,6 +24,7 @@ #include "qobj.h" #include +#include "frr_pthread.h" #include "lib/json.h" #include "vrf.h" #include "vty.h" @@ -97,6 +98,9 @@ enum bgp_af_index { for (afi = AFI_IP; afi < AFI_MAX; afi++) \ for (safi = SAFI_UNICAST; safi < SAFI_MAX; safi++) +extern struct frr_pthread *bgp_pth_io; +extern struct frr_pthread *bgp_pth_ka; + /* BGP master for system wide configurations and variables. */ struct bgp_master { /* BGP instance list. */ @@ -105,10 +109,6 @@ struct bgp_master { /* BGP thread master. */ struct thread_master *master; -/* BGP pthreads. */ -#define PTHREAD_IO (1 << 1) -#define PTHREAD_KEEPALIVES (1 << 2) - /* work queues */ struct work_queue *process_main_queue; diff --git a/lib/frr_pthread.c b/lib/frr_pthread.c index d48b23f38a..6541437f55 100644 --- a/lib/frr_pthread.c +++ b/lib/frr_pthread.c @@ -26,108 +26,77 @@ #include "frr_pthread.h" #include "memory.h" -#include "hash.h" +#include "linklist.h" DEFINE_MTYPE(LIB, FRR_PTHREAD, "FRR POSIX Thread"); DEFINE_MTYPE(LIB, PTHREAD_PRIM, "POSIX synchronization primitives"); -/* id for next created pthread */ -static _Atomic uint32_t next_id = 0; - /* default frr_pthread start/stop routine prototypes */ static void *fpt_run(void *arg); static int fpt_halt(struct frr_pthread *fpt, void **res); /* default frr_pthread attributes */ struct frr_pthread_attr frr_pthread_attr_default = { - .id = 0, .start = fpt_run, .stop = fpt_halt, }; -/* hash table to keep track of all frr_pthreads */ -static struct hash *frr_pthread_hash; -static pthread_mutex_t frr_pthread_hash_mtx = PTHREAD_MUTEX_INITIALIZER; - -/* frr_pthread_hash->hash_cmp */ -static int frr_pthread_hash_cmp(const void *value1, const void *value2) -{ - const struct frr_pthread *tq1 = value1; - const struct frr_pthread *tq2 = value2; - - return (tq1->attr.id == tq2->attr.id); -} - -/* frr_pthread_hash->hash_key */ -static unsigned int frr_pthread_hash_key(void *value) -{ - return ((struct frr_pthread *)value)->attr.id; -} +/* list to keep track of all frr_pthreads */ +static pthread_mutex_t frr_pthread_list_mtx = PTHREAD_MUTEX_INITIALIZER; +static struct list *frr_pthread_list; /* ------------------------------------------------------------------------ */ void frr_pthread_init() { - pthread_mutex_lock(&frr_pthread_hash_mtx); + pthread_mutex_lock(&frr_pthread_list_mtx); { - frr_pthread_hash = hash_create(frr_pthread_hash_key, - frr_pthread_hash_cmp, NULL); + frr_pthread_list = list_new(); + frr_pthread_list->del = (void (*)(void *))&frr_pthread_destroy; } - pthread_mutex_unlock(&frr_pthread_hash_mtx); + pthread_mutex_unlock(&frr_pthread_list_mtx); } void frr_pthread_finish() { - pthread_mutex_lock(&frr_pthread_hash_mtx); + pthread_mutex_lock(&frr_pthread_list_mtx); { - hash_clean(frr_pthread_hash, - (void (*)(void *))frr_pthread_destroy); - hash_free(frr_pthread_hash); + list_delete_and_null(&frr_pthread_list); } - pthread_mutex_unlock(&frr_pthread_hash_mtx); + pthread_mutex_unlock(&frr_pthread_list_mtx); } struct frr_pthread *frr_pthread_new(struct frr_pthread_attr *attr, const char *name, const char *os_name) { - static struct frr_pthread holder = {}; struct frr_pthread *fpt = NULL; attr = attr ? attr : &frr_pthread_attr_default; - pthread_mutex_lock(&frr_pthread_hash_mtx); + fpt = XCALLOC(MTYPE_FRR_PTHREAD, sizeof(struct frr_pthread)); + /* initialize mutex */ + pthread_mutex_init(&fpt->mtx, NULL); + /* create new thread master */ + fpt->master = thread_master_create(name); + /* set attributes */ + fpt->attr = *attr; + name = (name ? name : "Anonymous thread"); + fpt->name = XSTRDUP(MTYPE_FRR_PTHREAD, name); + if (os_name) + snprintf(fpt->os_name, OS_THREAD_NAMELEN, "%s", os_name); + /* initialize startup synchronization primitives */ + fpt->running_cond_mtx = XCALLOC( + MTYPE_PTHREAD_PRIM, sizeof(pthread_mutex_t)); + fpt->running_cond = XCALLOC(MTYPE_PTHREAD_PRIM, + sizeof(pthread_cond_t)); + pthread_mutex_init(fpt->running_cond_mtx, NULL); + pthread_cond_init(fpt->running_cond, NULL); + + pthread_mutex_lock(&frr_pthread_list_mtx); { - holder.attr.id = attr->id; - - if (!hash_lookup(frr_pthread_hash, &holder)) { - fpt = XCALLOC(MTYPE_FRR_PTHREAD, - sizeof(struct frr_pthread)); - /* initialize mutex */ - pthread_mutex_init(&fpt->mtx, NULL); - /* create new thread master */ - fpt->master = thread_master_create(name); - /* set attributes */ - fpt->attr = *attr; - name = (name ? name : "Anonymous thread"); - fpt->name = XSTRDUP(MTYPE_FRR_PTHREAD, name); - if (os_name) - snprintf(fpt->os_name, OS_THREAD_NAMELEN, - "%s", os_name); - if (attr == &frr_pthread_attr_default) - fpt->attr.id = frr_pthread_get_id(); - /* initialize startup synchronization primitives */ - fpt->running_cond_mtx = XCALLOC( - MTYPE_PTHREAD_PRIM, sizeof(pthread_mutex_t)); - fpt->running_cond = XCALLOC(MTYPE_PTHREAD_PRIM, - sizeof(pthread_cond_t)); - pthread_mutex_init(fpt->running_cond_mtx, NULL); - pthread_cond_init(fpt->running_cond, NULL); - - /* insert into global thread hash */ - hash_get(frr_pthread_hash, fpt, hash_alloc_intern); - } + listnode_add(frr_pthread_list, fpt); } - pthread_mutex_unlock(&frr_pthread_hash_mtx); + pthread_mutex_unlock(&frr_pthread_list_mtx); return fpt; } @@ -180,21 +149,6 @@ int frr_pthread_set_name(struct frr_pthread *fpt, const char *name, return ret; } -struct frr_pthread *frr_pthread_get(uint32_t id) -{ - static struct frr_pthread holder = {}; - struct frr_pthread *fpt; - - pthread_mutex_lock(&frr_pthread_hash_mtx); - { - holder.attr.id = id; - fpt = hash_lookup(frr_pthread_hash, &holder); - } - pthread_mutex_unlock(&frr_pthread_hash_mtx); - - return fpt; -} - int frr_pthread_run(struct frr_pthread *fpt, const pthread_attr_t *attr) { int ret; @@ -239,31 +193,16 @@ int frr_pthread_stop(struct frr_pthread *fpt, void **result) return ret; } -/* - * Callback for hash_iterate to stop all frr_pthread's. - */ -static void frr_pthread_stop_all_iter(struct hash_backet *hb, void *arg) -{ - struct frr_pthread *fpt = hb->data; - frr_pthread_stop(fpt, NULL); -} - void frr_pthread_stop_all() { - pthread_mutex_lock(&frr_pthread_hash_mtx); + pthread_mutex_lock(&frr_pthread_list_mtx); { - hash_iterate(frr_pthread_hash, frr_pthread_stop_all_iter, NULL); + struct listnode *n; + struct frr_pthread *fpt; + for (ALL_LIST_ELEMENTS_RO(frr_pthread_list, n, fpt)) + frr_pthread_stop(fpt, NULL); } - pthread_mutex_unlock(&frr_pthread_hash_mtx); -} - -uint32_t frr_pthread_get_id(void) -{ - _Atomic uint32_t nxid; - nxid = atomic_fetch_add_explicit(&next_id, 1, memory_order_seq_cst); - /* just a sanity check, this should never happen */ - assert(nxid <= (UINT32_MAX - 1)); - return nxid; + pthread_mutex_unlock(&frr_pthread_list_mtx); } void frr_pthread_yield(void) diff --git a/lib/frr_pthread.h b/lib/frr_pthread.h index 732e2925fe..2a41de6d47 100644 --- a/lib/frr_pthread.h +++ b/lib/frr_pthread.h @@ -34,7 +34,6 @@ struct frr_pthread; struct frr_pthread_attr; struct frr_pthread_attr { - _Atomic uint32_t id; void *(*start)(void *); int (*stop)(struct frr_pthread *, void **); }; @@ -154,13 +153,6 @@ int frr_pthread_set_name(struct frr_pthread *fpt, const char *name, */ void frr_pthread_destroy(struct frr_pthread *fpt); -/* - * Gets an existing frr_pthread by its id. - * - * @return frr_thread associated with the provided id, or NULL on error - */ -struct frr_pthread *frr_pthread_get(uint32_t id); - /* * Creates a new pthread and binds it to a frr_pthread. * @@ -221,19 +213,6 @@ void frr_pthread_stop_all(void); /* Yields the current thread of execution */ void frr_pthread_yield(void); -/* - * Returns a unique identifier for use with frr_pthread_new(). - * - * Internally, this is an integer that increments after each call to this - * function. Because the number of pthreads created should never exceed INT_MAX - * during the life of the program, there is no overflow protection. If by - * chance this function returns an ID which is already in use, - * frr_pthread_new() will fail when it is provided. - * - * @return unique identifier - */ -uint32_t frr_pthread_get_id(void); - #ifndef HAVE_PTHREAD_CONDATTR_SETCLOCK #define pthread_condattr_setclock(A, B) #endif diff --git a/tests/bgpd/test_aspath.c b/tests/bgpd/test_aspath.c index 247591580a..d2d960f278 100644 --- a/tests/bgpd/test_aspath.c +++ b/tests/bgpd/test_aspath.c @@ -1273,9 +1273,6 @@ static int handle_attr_test(struct aspath_tests *t) struct aspath *asp; size_t datalen; - bgp_pthreads_init(); - frr_pthread_get(PTHREAD_KEEPALIVES)->running = true; - asp = make_aspath(t->segment->asdata, t->segment->len, 0); peer.curr = stream_new(BGP_MAX_PACKET_SIZE); @@ -1382,6 +1379,9 @@ int main(void) i = 0; + bgp_pthreads_init(); + bgp_pth_ka->running = true; + while (aspath_tests[i].desc) { printf("aspath_attr test %d\n", i); attr_test(&aspath_tests[i++]); diff --git a/tests/bgpd/test_capability.c b/tests/bgpd/test_capability.c index 83af5e9c6d..968f9ac445 100644 --- a/tests/bgpd/test_capability.c +++ b/tests/bgpd/test_capability.c @@ -917,7 +917,7 @@ int main(void) bgp_option_set(BGP_OPT_NO_LISTEN); bgp_pthreads_init(); - frr_pthread_get(PTHREAD_KEEPALIVES)->running = true; + bgp_pth_ka->running = true; if (fileno(stdout) >= 0) tty = isatty(fileno(stdout)); diff --git a/zebra/zserv.c b/zebra/zserv.c index 2f4bb22ffe..8cc462577a 100644 --- a/zebra/zserv.c +++ b/zebra/zserv.c @@ -703,7 +703,6 @@ static struct zserv *zserv_client_create(int sock) listnode_add(zebrad.client_list, client); struct frr_pthread_attr zclient_pthr_attrs = { - .id = frr_pthread_get_id(), .start = frr_pthread_attr_default.start, .stop = frr_pthread_attr_default.stop }; From 64777f5d87e5e6a1b58e555f1e15849746808400 Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Wed, 12 Sep 2018 21:25:49 +0200 Subject: [PATCH 2/3] lib: remove unused frr_pthread_yield() OS-level yield is generally a bad and possibly dangerous idea. If the thread should be suspended, there should always be something to wait on, or it turns into busy waiting. And if it's "just giving something else the chance to run" - that's the kernel's job to determine, and the kernel will do so while considering priorities, cgroups, and whatnot. Let it do its job. Signed-off-by: David Lamparter --- lib/frr_pthread.c | 5 ----- lib/frr_pthread.h | 3 --- 2 files changed, 8 deletions(-) diff --git a/lib/frr_pthread.c b/lib/frr_pthread.c index 6541437f55..c1ce57e24e 100644 --- a/lib/frr_pthread.c +++ b/lib/frr_pthread.c @@ -205,11 +205,6 @@ void frr_pthread_stop_all() pthread_mutex_unlock(&frr_pthread_list_mtx); } -void frr_pthread_yield(void) -{ - (void)sched_yield(); -} - /* * ---------------------------------------------------------------------------- * Default Event Loop diff --git a/lib/frr_pthread.h b/lib/frr_pthread.h index 2a41de6d47..b9e60511d5 100644 --- a/lib/frr_pthread.h +++ b/lib/frr_pthread.h @@ -210,9 +210,6 @@ int frr_pthread_stop(struct frr_pthread *fpt, void **result); /* Stops all frr_pthread's. */ void frr_pthread_stop_all(void); -/* Yields the current thread of execution */ -void frr_pthread_yield(void); - #ifndef HAVE_PTHREAD_CONDATTR_SETCLOCK #define pthread_condattr_setclock(A, B) #endif From c5d580fbc76c41a3355aef8dcd4aa387ea9c58c2 Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Wed, 19 Sep 2018 22:05:32 +0200 Subject: [PATCH 3/3] tools/checkpatch.pl: recognize _Atomic as modifier checkpatch is slightly confused by _Atomic, causing some spurious warnings. Signed-off-by: David Lamparter --- tools/checkpatch.pl | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/checkpatch.pl b/tools/checkpatch.pl index 55b3e1e564..3b861e4f87 100755 --- a/tools/checkpatch.pl +++ b/tools/checkpatch.pl @@ -359,6 +359,7 @@ our $InitAttribute = qr{$InitAttributeData|$InitAttributeConst|$InitAttributeIni # We need \b after 'init' otherwise 'initconst' will cause a false positive in a check our $Attribute = qr{ const| + _Atomic| __percpu| __nocast| __safe|