pespin submitted this change.

View Change

cosmetic: Improve comments on cancellable thread safety

* Move the comment explaining safety measures to the top of the thread
  function
* Document that special care must be taken in mutual exclusion zones
  within the thread code.

Change-Id: Ic1ffe68c1637cb06787d4193347cb4200c819154
---
M daemon/gtp_endpoint.c
M daemon/tun_device.c
2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/daemon/gtp_endpoint.c b/daemon/gtp_endpoint.c
index 7892d4e..87d333c 100644
--- a/daemon/gtp_endpoint.c
+++ b/daemon/gtp_endpoint.c
@@ -124,11 +124,16 @@
 	}
 }
 
-/* one thread for reading from each GTP/UDP socket (GTP decapsulation -> tun)
- * IMPORTANT!: All logging functions in this function block must be called with
- * PTHREAD_CANCEL_DISABLE set, otherwise the thread could be cancelled while
- * holding the logging mutex, hence causing deadlock with main (or other)
- * thread. */
+/* One thread for reading from each GTP/UDP socket (GTP decapsulation -> tun)
+ * IMPORTANT!: Since this thread is cancellable (deferred type):
+ * - All osmo logging functions in this thread must be called with PTHREAD_CANCEL_DISABLE set,
+ *   otherwise the thread could be cancelled while holding the libosmocore logging mutex, hence causing
+ *   deadlock with main (or other) thread.
+ * - Within pthread_rwlock_*(&d->rwlock) mutual exclusion zone, if we ever do any call considered
+ *   a cancellation point (see "man pthreads"), then make sure to do the call protected with
+ *   PTHREAD_CANCEL_DISABLE set, otherwise we may leave the d->rwlock held forever and cause a deadlock
+ *   with main (or other) thread.
+ */
 static void *gtp_endpoint_thread(void *arg)
 {
 	struct gtp_endpoint *ep = (struct gtp_endpoint *)arg;
diff --git a/daemon/tun_device.c b/daemon/tun_device.c
index ee4fbcc..40b6ec3 100644
--- a/daemon/tun_device.c
+++ b/daemon/tun_device.c
@@ -212,7 +212,16 @@
 	return rc;
 }
 
-/* one thread for reading from each TUN device (TUN -> GTP encapsulation) */
+/* One thread for reading from each TUN device (TUN -> GTP encapsulation)
+ * IMPORTANT!: Since this thread is cancellable (deferred type):
+ * - All osmo logging functions in this thread must be called with PTHREAD_CANCEL_DISABLE set,
+ *   otherwise the thread could be cancelled while holding the libosmocore logging mutex, hence causing
+ *   deadlock with main (or other) thread.
+ * - Within pthread_rwlock_*(&d->rwlock) mutual exclusion zone, if we ever do any call considered
+ *   a cancellation point (see "man pthreads"), then make sure to do the call protected with
+ *   PTHREAD_CANCEL_DISABLE set, otherwise we may leave the d->rwlock held forever and cause a deadlock
+ *   with main (or other) thread.
+ */
 static void *tun_device_thread(void *arg)
 {
 	struct tun_device *tun = (struct tun_device *)arg;
@@ -223,10 +232,6 @@
 	uint8_t base_buffer[payload_off_4byte_aligned + MAX_UDP_PACKET];
 
 	pthread_cleanup_push(tun_device_pthread_cleanup_routine, tun);
-	/* IMPORTANT!: All logging functions in this function block must be called with
-	 * PTHREAD_CANCEL_DISABLE set, otherwise the thread could be cancelled while
-	 * holding the logging mutex, hence causing deadlock with main (or other)
-	 * thread. */
 
 	snprintf(thread_name, sizeof(thread_name), "Rx%s", tun->devname);
 	pthread_setname_np(pthread_self(), thread_name);
@@ -269,6 +274,7 @@
 			continue;
 		}
 		rc = tx_gtp1u_pkt(t, base_buffer, buffer, nread);
+		/* pthread_rwlock_unlock() was called inside tx_gtp1u_pkt(). */
 		if (rc < 0) {
 			LOGTUN_NC(tun, LOGL_FATAL, "Error Writing to UDP socket: %s\n", strerror(errno));
 			exit(1);

To view, visit change 40798. To unsubscribe, or for help writing mail filters, visit settings.