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 <equinox@diac24.net>

diff --git a/bgpd/bgp_io.c b/bgpd/bgp_io.c
index c3bfbe4a9037de573a4087fedb43a189431b36cd..95c3f15a66430b7d170835780451c6698400476e 100644 (file)
--- a/bgpd/bgp_io.c
+++ b/bgpd/bgp_io.c
@@ -23,7 +23,7 @@
 #include <zebra.h>
 #include <pthread.h>           // 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 aeb95f91ba4fcc983f44a1032c694347e31b6ec6..91fa8fa3736f89203fca68073a554886b87d3f53 100644 (file)
--- 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 7df8de55f03e5471981ae7694e58abc41591d285..552244b2f651e440efc8417736a68bd83749aeb9 100644 (file)
--- 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 0a8962b4c703dde608356e45adf19d02f8bae2c3..f644c0bd371af7ea305426cf83edc8bc45d5b0bd 100644 (file)
--- a/bgpd/bgpd.h
+++ b/bgpd/bgpd.h
@@ -24,6 +24,7 @@
 #include "qobj.h"
 #include <pthread.h>
 
+#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 d48b23f38a9a1804831cfa81f1541d03260adc9f..6541437f55a9c07efcb7518b81d0bd3c46ba05f9 100644 (file)
--- 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 732e2925fef67246b6e2b9a6184b2b223ea59ea5..2a41de6d474c35a2cadaaa876358c57c3144ae47 100644 (file)
--- 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 247591580aa99b408ca9e38267f7d36bec79e51e..d2d960f278d77f8219329011821dafba30f52c72 100644 (file)
--- 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 83af5e9c6dffd773a3609f976d6b5a77d8c84519..968f9ac445d25b610730574d9199aca94a5b189f 100644 (file)
--- 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 2f4bb22ffe2e48a8755b4f7e39a84fb06cfae650..8cc462577a2a5a317ab46b49105d0e91480d2f83 100644 (file)
--- 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
        };