lib: some frr_pthread fixes

* Use atomic fixed-width thread identifiers
* Add ability to change thread name at runtime

Signed-off-by: Quentin Young <qlyoung@cumulusnetworks.com>

5 files changed, 74 insertions, 20 deletions

diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c
index c06339aad9..ed0a12f052 100644
--- a/bgpd/bgpd.c
+++ b/bgpd/bgpd.c
@@ -7528,16 +7528,14 @@ static void bgp_pthreads_init()
 		.id = PTHREAD_IO,
 		.start = frr_pthread_attr_default.start,
 		.stop = frr_pthread_attr_default.stop,
-		.name = "BGP I/O thread",
 	};
 	struct frr_pthread_attr ka = {
 		.id = PTHREAD_KEEPALIVES,
 		.start = bgp_keepalives_start,
 		.stop = bgp_keepalives_stop,
-		.name = "BGP Keepalives thread",
 	};
-	frr_pthread_new(&io);
-	frr_pthread_new(&ka);
+	frr_pthread_new(&io, "BGP I/O thread");
+	frr_pthread_new(&ka, "BGP Keepalives thread");
 }
 
 void bgp_pthreads_run()
diff --git a/lib/frr_pthread.c b/lib/frr_pthread.c
index 72b47ae5c3..ba6d89429a 100644
--- a/lib/frr_pthread.c
+++ b/lib/frr_pthread.c
@@ -1,6 +1,6 @@
 /*
  * Utilities and interfaces for managing POSIX threads within FRR.
- * Copyright (C) 2017  Cumulus Networks
+ * Copyright (C) 2017  Cumulus Networks, Inc.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -29,7 +29,7 @@ DEFINE_MTYPE(LIB, FRR_PTHREAD, "FRR POSIX Thread");
 DEFINE_MTYPE(LIB, PTHREAD_PRIM, "POSIX synchronization primitives");
 
 /* id for next created pthread */
-static unsigned int next_id = 0;
+static _Atomic uint32_t next_id = 0;
 
 /* default frr_pthread start/stop routine prototypes */
 static void *fpt_run(void *arg);
@@ -40,7 +40,6 @@ struct frr_pthread_attr frr_pthread_attr_default = {
 	.id = 0,
 	.start = fpt_run,
 	.stop = fpt_halt,
-	.name = "Anonymous",
 };
 
 /* hash table to keep track of all frr_pthreads */
@@ -85,9 +84,10 @@ void frr_pthread_finish()
 	pthread_mutex_unlock(&frr_pthread_hash_mtx);
 }
 
-struct frr_pthread *frr_pthread_new(struct frr_pthread_attr *attr)
+struct frr_pthread *frr_pthread_new(struct frr_pthread_attr *attr,
+				    const char *name)
 {
-	static struct frr_pthread holder = {0};
+	static struct frr_pthread holder = {};
 	struct frr_pthread *fpt = NULL;
 
 	attr = attr ? attr : &frr_pthread_attr_default;
@@ -99,10 +99,14 @@ struct frr_pthread *frr_pthread_new(struct frr_pthread_attr *attr)
 		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(attr->name);
+			fpt->master = thread_master_create(name);
 			/* set attributes */
 			fpt->attr = *attr;
+			name = (name ? name : "Anonymous thread");
+			fpt->name = XSTRDUP(MTYPE_FRR_PTHREAD, name);
 			if (attr == &frr_pthread_attr_default)
 				fpt->attr.id = frr_pthread_get_id();
 			/* initialize startup synchronization primitives */
@@ -126,16 +130,31 @@ void frr_pthread_destroy(struct frr_pthread *fpt)
 {
 	thread_master_free(fpt->master);
 
+	pthread_mutex_destroy(&fpt->mtx);
 	pthread_mutex_destroy(fpt->running_cond_mtx);
 	pthread_cond_destroy(fpt->running_cond);
+	if (fpt->name)
+		XFREE(MTYPE_FRR_PTHREAD, fpt->name);
 	XFREE(MTYPE_PTHREAD_PRIM, fpt->running_cond_mtx);
 	XFREE(MTYPE_PTHREAD_PRIM, fpt->running_cond);
 	XFREE(MTYPE_FRR_PTHREAD, fpt);
 }
 
+void frr_pthread_set_name(struct frr_pthread *fpt, const char *name)
+{
+	pthread_mutex_lock(&fpt->mtx);
+	{
+		if (fpt->name)
+			XFREE(MTYPE_FRR_PTHREAD, fpt->name);
+		fpt->name = XSTRDUP(MTYPE_FRR_PTHREAD, name);
+	}
+	pthread_mutex_unlock(&fpt->mtx);
+	thread_master_set_name(fpt->master, name);
+}
+
 struct frr_pthread *frr_pthread_get(unsigned int id)
 {
-	static struct frr_pthread holder = {0};
+	static struct frr_pthread holder = {};
 	struct frr_pthread *fpt;
 
 	pthread_mutex_lock(&frr_pthread_hash_mtx);
@@ -210,11 +229,13 @@ void frr_pthread_stop_all()
 	pthread_mutex_unlock(&frr_pthread_hash_mtx);
 }
 
-unsigned int frr_pthread_get_id()
+uint32_t frr_pthread_get_id()
 {
+	_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(next_id <= INT_MAX - 1);
-	return next_id++;
+	assert(nxid <= (INT_MAX - 1));
+	return nxid;
 }
 
 void frr_pthread_yield(void)
diff --git a/lib/frr_pthread.h b/lib/frr_pthread.h
index 2cc50196a8..3990e2a4ce 100644
--- a/lib/frr_pthread.h
+++ b/lib/frr_pthread.h
@@ -1,6 +1,6 @@
 /*
  * Utilities and interfaces for managing POSIX threads within FRR.
- * Copyright (C) 2017  Cumulus Networks
+ * Copyright (C) 2017  Cumulus Networks, Inc.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -32,14 +32,19 @@ struct frr_pthread;
 struct frr_pthread_attr;
 
 struct frr_pthread_attr {
-	int id;
+	_Atomic uint32_t id;
 	void *(*start)(void *);
 	int (*stop)(struct frr_pthread *, void **);
-	const char *name;
 };
 
 struct frr_pthread {
 
+	/*
+	 * Mutex protecting this structure. Must be taken for reading some
+	 * fields, denoted by a 'Requires: mtx'.
+	 */
+	pthread_mutex_t mtx;
+
 	/* pthread id */
 	pthread_t thread;
 
@@ -73,8 +78,17 @@ struct frr_pthread {
 	 * Fake thread-specific storage. No constraints on usage. Helpful when
 	 * creating reentrant pthread implementations. Can be used to pass
 	 * argument to pthread entry function.
+	 *
+	 * Requires: mtx
 	 */
 	void *data;
+
+	/*
+	 * Human-readable thread name.
+	 *
+	 * Requires: mtx
+	 */
+	char *name;
 };
 
 extern struct frr_pthread_attr frr_pthread_attr_default;
@@ -107,9 +121,19 @@ void frr_pthread_finish(void);
  * frr_pthread will cause them to run on that pthread.
  *
  * @param attr - the thread attributes
+ * @param name - Human-readable name
  * @return the created frr_pthread upon success, or NULL upon failure
  */
-struct frr_pthread *frr_pthread_new(struct frr_pthread_attr *attr);
+struct frr_pthread *frr_pthread_new(struct frr_pthread_attr *attr,
+				    const char *name);
+
+/*
+ * Changes the name of the frr_pthread.
+ *
+ * @param fpt - the frr_pthread to operate on
+ * @param name - Human-readable name
+ */
+void frr_pthread_set_name(struct frr_pthread *fpt, const char *name);
 
 /*
  * Destroys an frr_pthread.
@@ -198,6 +222,6 @@ void frr_pthread_yield(void);
  *
  * @return unique identifier
  */
-unsigned int frr_pthread_get_id(void);
+uint32_t frr_pthread_get_id(void);
 
 #endif /* _FRR_PTHREAD_H */
diff --git a/lib/thread.c b/lib/thread.c
index 9d64663d9c..868bfd0e9b 100644
--- a/lib/thread.c
+++ b/lib/thread.c
@@ -343,7 +343,6 @@ static void initializer()
 	pthread_key_create(&thread_current, NULL);
 }
 
-/* Allocate new thread master.  */
 struct thread_master *thread_master_create(const char *name)
 {
 	struct thread_master *rv;
@@ -427,6 +426,17 @@ struct thread_master *thread_master_create(const char *name)
 	return rv;
 }
 
+void thread_master_set_name(struct thread_master *master, const char *name)
+{
+	pthread_mutex_lock(&master->mtx);
+	{
+		if (master->name)
+			XFREE(MTYPE_THREAD_MASTER, master->name);
+		master->name = XSTRDUP(MTYPE_THREAD_MASTER, name);
+	}
+	pthread_mutex_unlock(&master->mtx);
+}
+
 /* Add a new thread to the list.  */
 static void thread_list_add(struct thread_list *list, struct thread *thread)
 {
diff --git a/lib/thread.h b/lib/thread.h
index c830446e10..f7c110914d 100644
--- a/lib/thread.h
+++ b/lib/thread.h
@@ -174,6 +174,7 @@ struct cpu_thread_history {
 
 /* Prototypes. */
 extern struct thread_master *thread_master_create(const char *);
+void thread_master_set_name(struct thread_master *master, const char *name);
 extern void thread_master_free(struct thread_master *);
 extern void thread_master_free_unused(struct thread_master *);