pespin has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-uecups/+/40798?usp=email )
Change subject: cosmetic: Improve comments on cancellable thread safety ......................................................................
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(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-uecups refs/changes/98/40798/1
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);