From 6bd201ccec1d5a11a42890e8b94a6920fcda97bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Fri, 14 Oct 2022 12:03:02 +0200 Subject: [PATCH 1/2] Remove one level of indirection from isc_rwlock [1/2] Instead of checking the PTHREAD_RUNTIME_CHECK from the header, move it to the pthread_rwlock implementation functions. The internal isc_rwlock actually cannot fail, so the checks in the header was useless anyway. --- lib/isc/include/isc/rwlock.h | 66 +++++++++------------------ lib/isc/rwlock.c | 86 ++++++++++++++++++++---------------- 2 files changed, 70 insertions(+), 82 deletions(-) diff --git a/lib/isc/include/isc/rwlock.h b/lib/isc/include/isc/rwlock.h index 68b91fc2b4..0984eed943 100644 --- a/lib/isc/include/isc/rwlock.h +++ b/lib/isc/include/isc/rwlock.h @@ -46,13 +46,13 @@ typedef pthread_rwlock_t isc__rwlock_t; isc__rwlock_init(*rwl, rq, wq); \ } #define isc_rwlock_lock(rwl, type) isc__rwlock_lock(*rwl, type) -#define isc_rwlock_trylock(rwl, type) isc___rwlock_trylock(*rwl, type) +#define isc_rwlock_trylock(rwl, type) isc__rwlock_trylock(*rwl, type) #define isc_rwlock_unlock(rwl, type) isc__rwlock_unlock(*rwl, type) -#define isc_rwlock_tryupgrade(rwl) isc___rwlock_tryupgrade(*rwl) -#define isc_rwlock_destroy(rwl) \ - { \ - isc___rwlock_destroy(*rwl); \ - free(*rwl); \ +#define isc_rwlock_tryupgrade(rwl) isc__rwlock_tryupgrade(*rwl) +#define isc_rwlock_destroy(rwl) \ + { \ + isc__rwlock_destroy(*rwl); \ + free(*rwl); \ } #else /* ISC_TRACK_PTHREADS_OBJECTS */ @@ -62,9 +62,9 @@ typedef pthread_rwlock_t isc__rwlock_t; #define isc_rwlock_init(rwl, rq, wq) isc__rwlock_init(rwl, rq, wq) #define isc_rwlock_lock(rwl, type) isc__rwlock_lock(rwl, type) -#define isc_rwlock_trylock(rwl, type) isc___rwlock_trylock(rwl, type) +#define isc_rwlock_trylock(rwl, type) isc__rwlock_trylock(rwl, type) #define isc_rwlock_unlock(rwl, type) isc__rwlock_unlock(rwl, type) -#define isc_rwlock_tryupgrade(rwl) isc___rwlock_tryupgrade(rwl) +#define isc_rwlock_tryupgrade(rwl) isc__rwlock_tryupgrade(rwl) #define isc_rwlock_destroy(rwl) isc__rwlock_destroy(rwl) #endif /* ISC_TRACK_PTHREADS_OBJECTS */ @@ -112,54 +112,30 @@ typedef struct isc_rwlock isc__rwlock_t; #define isc_rwlock_init(rwl, rq, wq) isc__rwlock_init(rwl, rq, wq) #define isc_rwlock_lock(rwl, type) isc__rwlock_lock(rwl, type) -#define isc_rwlock_trylock(rwl, type) isc___rwlock_trylock(rwl, type) +#define isc_rwlock_trylock(rwl, type) isc__rwlock_trylock(rwl, type) #define isc_rwlock_unlock(rwl, type) isc__rwlock_unlock(rwl, type) -#define isc_rwlock_tryupgrade(rwl) isc___rwlock_tryupgrade(rwl) +#define isc_rwlock_tryupgrade(rwl) isc__rwlock_tryupgrade(rwl) #define isc_rwlock_destroy(rwl) isc__rwlock_destroy(rwl) #endif /* USE_PTHREAD_RWLOCK */ -#define isc__rwlock_init(rwl, rq, wq) \ - { \ - int _ret = isc___rwlock_init(rwl, rq, wq); \ - PTHREADS_RUNTIME_CHECK(isc___rwlock_init, _ret); \ - } +void +isc__rwlock_init(isc__rwlock_t *rwl, unsigned int read_quota, + unsigned int write_quota); -#define isc__rwlock_lock(rwl, type) \ - { \ - int _ret = isc___rwlock_lock(rwl, type); \ - PTHREADS_RUNTIME_CHECK(isc___rwlock_lock, _ret); \ - } - -#define isc__rwlock_unlock(rwl, type) \ - { \ - int _ret = isc___rwlock_unlock(rwl, type); \ - PTHREADS_RUNTIME_CHECK(isc___rwlock_unlock, _ret); \ - } - -#define isc__rwlock_destroy(rwl) \ - { \ - int _ret = isc___rwlock_destroy(rwl); \ - PTHREADS_RUNTIME_CHECK(isc___rwlock_destroy, _ret); \ - } - -int -isc___rwlock_init(isc__rwlock_t *rwl, unsigned int read_quota, - unsigned int write_quota); - -int -isc___rwlock_lock(isc__rwlock_t *rwl, isc_rwlocktype_t type); +void +isc__rwlock_lock(isc__rwlock_t *rwl, isc_rwlocktype_t type); isc_result_t -isc___rwlock_trylock(isc__rwlock_t *rwl, isc_rwlocktype_t type); +isc__rwlock_trylock(isc__rwlock_t *rwl, isc_rwlocktype_t type); -int -isc___rwlock_unlock(isc__rwlock_t *rwl, isc_rwlocktype_t type); +void +isc__rwlock_unlock(isc__rwlock_t *rwl, isc_rwlocktype_t type); isc_result_t -isc___rwlock_tryupgrade(isc__rwlock_t *rwl); +isc__rwlock_tryupgrade(isc__rwlock_t *rwl); -int -isc___rwlock_destroy(isc__rwlock_t *rwl); +void +isc__rwlock_destroy(isc__rwlock_t *rwl); ISC_LANG_ENDDECLS diff --git a/lib/isc/rwlock.c b/lib/isc/rwlock.c index 739f9746b0..c04bae2571 100644 --- a/lib/isc/rwlock.c +++ b/lib/isc/rwlock.c @@ -32,32 +32,36 @@ #include #include -int -isc___rwlock_init(isc__rwlock_t *rwl, unsigned int read_quota, - unsigned int write_quota) { +void +isc__rwlock_init(isc__rwlock_t *rwl, unsigned int read_quota, + unsigned int write_quota) { int ret; UNUSED(read_quota); UNUSED(write_quota); ret = pthread_rwlock_init(rwl, NULL); - - return (ret); + PTHREADS_RUNTIME_CHECK(pthread_rwlock_init, ret); } -int -isc___rwlock_lock(isc__rwlock_t *rwl, isc_rwlocktype_t type) { +void +isc__rwlock_lock(isc__rwlock_t *rwl, isc_rwlocktype_t type) { + int ret; switch (type) { case isc_rwlocktype_read: - return (pthread_rwlock_rdlock(rwl)); + ret = pthread_rwlock_rdlock(rwl); + PTHREADS_RUNTIME_CHECK(pthread_rwlock_rdlock, ret); + break; case isc_rwlocktype_write: - return (pthread_rwlock_wrlock(rwl)); + ret = pthread_rwlock_wrlock(rwl); + PTHREADS_RUNTIME_CHECK(pthread_rwlock_rwlock, ret); + break; default: UNREACHABLE(); } } isc_result_t -isc___rwlock_trylock(isc__rwlock_t *rwl, isc_rwlocktype_t type) { +isc__rwlock_trylock(isc__rwlock_t *rwl, isc_rwlocktype_t type) { int ret = 0; switch (type) { case isc_rwlocktype_read: @@ -78,25 +82,41 @@ isc___rwlock_trylock(isc__rwlock_t *rwl, isc_rwlocktype_t type) { case EAGAIN: return (ISC_R_LOCKBUSY); default: - UNREACHABLE(); + break; } + + switch (type) { + case isc_rwlocktype_read: + PTHREADS_RUNTIME_CHECK(pthread_rwlock_tryrdlock, ret); + break; + case isc_rwlocktype_write: + PTHREADS_RUNTIME_CHECK(pthread_rwlock_trywrlock, ret); + break; + default: + break; + } + UNREACHABLE(); } -int -isc___rwlock_unlock(isc__rwlock_t *rwl, isc_rwlocktype_t type) { +void +isc__rwlock_unlock(isc__rwlock_t *rwl, isc_rwlocktype_t type) { + int ret; UNUSED(type); - return (pthread_rwlock_unlock(rwl)); + ret = pthread_rwlock_unlock(rwl); + PTHREADS_RUNTIME_CHECK(pthread_rwlock_rwlock, ret); } isc_result_t -isc___rwlock_tryupgrade(isc__rwlock_t *rwl) { +isc__rwlock_tryupgrade(isc__rwlock_t *rwl) { UNUSED(rwl); return (ISC_R_LOCKBUSY); } -int -isc___rwlock_destroy(isc__rwlock_t *rwl) { - return (pthread_rwlock_destroy(rwl)); +void +isc__rwlock_destroy(isc__rwlock_t *rwl) { + int ret = pthread_rwlock_destroy(rwl); + + PTHREADS_RUNTIME_CHECK(pthread_rwlock_destroy, ret); } #else /* if USE_PTHREAD_RWLOCK */ @@ -160,9 +180,9 @@ print_lock(const char *operation, isc__rwlock_t *rwl, isc_rwlocktype_t type) { } #endif /* ISC_RWLOCK_TRACE */ -int -isc___rwlock_init(isc__rwlock_t *rwl, unsigned int read_quota, - unsigned int write_quota) { +void +isc__rwlock_init(isc__rwlock_t *rwl, unsigned int read_quota, + unsigned int write_quota) { REQUIRE(rwl != NULL); /* @@ -191,12 +211,10 @@ isc___rwlock_init(isc__rwlock_t *rwl, unsigned int read_quota, isc_condition_init(&rwl->writeable); rwl->magic = RWLOCK_MAGIC; - - return (0); } -int -isc___rwlock_destroy(isc__rwlock_t *rwl) { +void +isc__rwlock_destroy(isc__rwlock_t *rwl) { REQUIRE(VALID_RWLOCK(rwl)); REQUIRE(atomic_load_acquire(&rwl->write_requests) == @@ -208,8 +226,6 @@ isc___rwlock_destroy(isc__rwlock_t *rwl) { isc_condition_destroy(&rwl->readable); isc_condition_destroy(&rwl->writeable); isc_mutex_destroy(&rwl->lock); - - return (0); } /* @@ -394,8 +410,8 @@ rwlock_lock(isc__rwlock_t *rwl, isc_rwlocktype_t type) { #endif /* ifdef ISC_RWLOCK_TRACE */ } -int -isc___rwlock_lock(isc__rwlock_t *rwl, isc_rwlocktype_t type) { +void +isc__rwlock_lock(isc__rwlock_t *rwl, isc_rwlocktype_t type) { int32_t cnt = 0; int32_t spins = atomic_load_acquire(&rwl->spins) * 2 + 10; int32_t max_cnt = ISC_MAX(spins, RWLOCK_MAX_ADAPTIVE_COUNT); @@ -409,12 +425,10 @@ isc___rwlock_lock(isc__rwlock_t *rwl, isc_rwlocktype_t type) { } while (isc_rwlock_trylock(rwl, type) != ISC_R_SUCCESS); atomic_fetch_add_release(&rwl->spins, (cnt - spins) / 8); - - return (0); } isc_result_t -isc___rwlock_trylock(isc__rwlock_t *rwl, isc_rwlocktype_t type) { +isc__rwlock_trylock(isc__rwlock_t *rwl, isc_rwlocktype_t type) { int32_t cntflag; REQUIRE(VALID_RWLOCK(rwl)); @@ -481,7 +495,7 @@ isc___rwlock_trylock(isc__rwlock_t *rwl, isc_rwlocktype_t type) { } isc_result_t -isc___rwlock_tryupgrade(isc__rwlock_t *rwl) { +isc__rwlock_tryupgrade(isc__rwlock_t *rwl) { REQUIRE(VALID_RWLOCK(rwl)); int_fast32_t reader_incr = READER_INCR; @@ -509,8 +523,8 @@ isc___rwlock_tryupgrade(isc__rwlock_t *rwl) { return (ISC_R_SUCCESS); } -int -isc___rwlock_unlock(isc__rwlock_t *rwl, isc_rwlocktype_t type) { +void +isc__rwlock_unlock(isc__rwlock_t *rwl, isc_rwlocktype_t type) { int32_t prev_cnt; REQUIRE(VALID_RWLOCK(rwl)); @@ -580,8 +594,6 @@ isc___rwlock_unlock(isc__rwlock_t *rwl, isc_rwlocktype_t type) { #ifdef ISC_RWLOCK_TRACE print_lock("postunlock", rwl, type); #endif /* ifdef ISC_RWLOCK_TRACE */ - - return (0); } #endif /* USE_PTHREAD_RWLOCK */ From 0492bbf590fa5c7e52f94d5a9220724e68052444 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Fri, 14 Oct 2022 12:54:57 +0200 Subject: [PATCH 2/2] Make the pthread_rwlock implementation header-only macros [2/2] While using mutrace, the phtread-rwlock based isc_rwlock implementation would be all tracked in the rwlock.c unit losing all useful information as all rwlocks would be traced in a single place. Rewrite the pthread_rwlock based implementation to be header-only macros, so we can use mutrace to properly track the rwlock contention without heavily patching mutrace to understand the libisc synchronization primitives. --- configure.ac | 1 + lib/isc/Makefile.am | 6 +- lib/isc/include/isc/condition.h | 6 ++ lib/isc/include/isc/mutex.h | 6 ++ lib/isc/include/isc/rwlock.h | 93 ++++++++++++++++++++++++++++++- lib/isc/rwlock.c | 98 +-------------------------------- 6 files changed, 110 insertions(+), 100 deletions(-) diff --git a/configure.ac b/configure.ac index e3cbb6a396..8cbd3e65d0 100644 --- a/configure.ac +++ b/configure.ac @@ -622,6 +622,7 @@ AS_IF([test "$enable_pthread_rwlock" = "yes"], [AC_MSG_ERROR([pthread_rwlock_rdlock requested but not found])]) AC_DEFINE([USE_PTHREAD_RWLOCK],[1],[Define if you want to use pthread rwlock implementation]) ]) +AM_CONDITIONAL([USE_ISC_RWLOCK], [test "$enable_pthread_rwlock" != "yes"]) CRYPTO=OpenSSL diff --git a/lib/isc/Makefile.am b/lib/isc/Makefile.am index e8ad720d03..4ff086b420 100644 --- a/lib/isc/Makefile.am +++ b/lib/isc/Makefile.am @@ -189,7 +189,6 @@ libisc_la_SOURCES = \ region.c \ resource.c \ result.c \ - rwlock.c \ safe.c \ serial.c \ signal.c \ @@ -216,6 +215,11 @@ libisc_la_SOURCES = \ xml.c \ work.c +if USE_ISC_RWLOCK +libisc_la_SOURCES += \ + rwlock.c +endif USE_ISC_RWLOCK + libisc_la_CPPFLAGS = \ $(AM_CPPFLAGS) \ $(LIBISC_CFLAGS) \ diff --git a/lib/isc/include/isc/condition.h b/lib/isc/include/isc/condition.h index 09302b35a9..4d820ab4b8 100644 --- a/lib/isc/include/isc/condition.h +++ b/lib/isc/include/isc/condition.h @@ -28,6 +28,12 @@ ISC_LANG_BEGINDECLS +/* + * We use macros instead of static inline functions so that the exact code + * location can be reported when PTHREADS_RUNTIME_CHECK() fails or when mutrace + * reports lock contention. + */ + #ifdef ISC_TRACK_PTHREADS_OBJECTS typedef pthread_cond_t *isc_condition_t; diff --git a/lib/isc/include/isc/mutex.h b/lib/isc/include/isc/mutex.h index 78aa153707..df24922336 100644 --- a/lib/isc/include/isc/mutex.h +++ b/lib/isc/include/isc/mutex.h @@ -25,6 +25,12 @@ ISC_LANG_BEGINDECLS +/* + * We use macros instead of static inline functions so that the exact code + * location can be reported when PTHREADS_RUNTIME_CHECK() fails or when mutrace + * reports lock contention. + */ + #ifdef ISC_TRACK_PTHREADS_OBJECTS typedef pthread_mutex_t *isc_mutex_t; diff --git a/lib/isc/include/isc/rwlock.h b/lib/isc/include/isc/rwlock.h index 0984eed943..44a44a86cc 100644 --- a/lib/isc/include/isc/rwlock.h +++ b/lib/isc/include/isc/rwlock.h @@ -35,6 +35,12 @@ typedef enum { #if USE_PTHREAD_RWLOCK #include +/* + * We use macros instead of static inline functions so that the exact code + * location can be reported when PTHREADS_RUNTIME_CHECK() fails or when mutrace + * reports lock contention. + */ + #if ISC_TRACK_PTHREADS_OBJECTS typedef pthread_rwlock_t *isc_rwlock_t; @@ -69,6 +75,89 @@ typedef pthread_rwlock_t isc__rwlock_t; #endif /* ISC_TRACK_PTHREADS_OBJECTS */ +#define isc__rwlock_init(rwl, read_quota, write_quote) \ + { \ + int _ret = pthread_rwlock_init(rwl, NULL); \ + PTHREADS_RUNTIME_CHECK(pthread_rwlock_init, _ret); \ + } + +#define isc__rwlock_lock(rwl, type) \ + { \ + int _ret; \ + switch (type) { \ + case isc_rwlocktype_read: \ + _ret = pthread_rwlock_rdlock(rwl); \ + PTHREADS_RUNTIME_CHECK(pthread_rwlock_rdlock, _ret); \ + break; \ + case isc_rwlocktype_write: \ + _ret = pthread_rwlock_wrlock(rwl); \ + PTHREADS_RUNTIME_CHECK(pthread_rwlock_rwlock, _ret); \ + break; \ + default: \ + UNREACHABLE(); \ + } \ + } + +#define isc__rwlock_trylock(rwl, type) \ + ({ \ + int _ret = 0; \ + isc_result_t _res = ISC_R_UNSET; \ + \ + switch (type) { \ + case isc_rwlocktype_read: \ + _ret = pthread_rwlock_tryrdlock(rwl); \ + break; \ + case isc_rwlocktype_write: \ + _ret = pthread_rwlock_trywrlock(rwl); \ + break; \ + default: \ + UNREACHABLE(); \ + } \ + \ + switch (_ret) { \ + case 0: \ + _res = ISC_R_SUCCESS; \ + break; \ + case EBUSY: \ + case EAGAIN: \ + _res = ISC_R_LOCKBUSY; \ + break; \ + default: \ + switch (type) { \ + case isc_rwlocktype_read: \ + PTHREADS_RUNTIME_CHECK( \ + pthread_rwlock_tryrdlock, _ret); \ + break; \ + case isc_rwlocktype_write: \ + PTHREADS_RUNTIME_CHECK( \ + pthread_rwlock_trywrlock, _ret); \ + break; \ + default: \ + UNREACHABLE(); \ + } \ + UNREACHABLE(); \ + } \ + _res; \ + }) + +#define isc__rwlock_unlock(rwl, type) \ + { \ + int _ret = pthread_rwlock_unlock(rwl); \ + PTHREADS_RUNTIME_CHECK(pthread_rwlock_rwlock, _ret); \ + } + +#define isc__rwlock_tryupgrade(rwl) \ + ({ \ + UNUSED(rwl); \ + ISC_R_LOCKBUSY; \ + }) + +#define isc__rwlock_destroy(rwl) \ + { \ + int _ret = pthread_rwlock_destroy(rwl); \ + PTHREADS_RUNTIME_CHECK(pthread_rwlock_destroy, _ret); \ + } + #else /* USE_PTHREAD_RWLOCK */ struct isc_rwlock { @@ -117,8 +206,6 @@ typedef struct isc_rwlock isc__rwlock_t; #define isc_rwlock_tryupgrade(rwl) isc__rwlock_tryupgrade(rwl) #define isc_rwlock_destroy(rwl) isc__rwlock_destroy(rwl) -#endif /* USE_PTHREAD_RWLOCK */ - void isc__rwlock_init(isc__rwlock_t *rwl, unsigned int read_quota, unsigned int write_quota); @@ -138,4 +225,6 @@ isc__rwlock_tryupgrade(isc__rwlock_t *rwl); void isc__rwlock_destroy(isc__rwlock_t *rwl); +#endif /* USE_PTHREAD_RWLOCK */ + ISC_LANG_ENDDECLS diff --git a/lib/isc/rwlock.c b/lib/isc/rwlock.c index c04bae2571..59ec0bffc2 100644 --- a/lib/isc/rwlock.c +++ b/lib/isc/rwlock.c @@ -27,100 +27,6 @@ #include #include -#if USE_PTHREAD_RWLOCK - -#include -#include - -void -isc__rwlock_init(isc__rwlock_t *rwl, unsigned int read_quota, - unsigned int write_quota) { - int ret; - UNUSED(read_quota); - UNUSED(write_quota); - - ret = pthread_rwlock_init(rwl, NULL); - PTHREADS_RUNTIME_CHECK(pthread_rwlock_init, ret); -} - -void -isc__rwlock_lock(isc__rwlock_t *rwl, isc_rwlocktype_t type) { - int ret; - switch (type) { - case isc_rwlocktype_read: - ret = pthread_rwlock_rdlock(rwl); - PTHREADS_RUNTIME_CHECK(pthread_rwlock_rdlock, ret); - break; - case isc_rwlocktype_write: - ret = pthread_rwlock_wrlock(rwl); - PTHREADS_RUNTIME_CHECK(pthread_rwlock_rwlock, ret); - break; - default: - UNREACHABLE(); - } -} - -isc_result_t -isc__rwlock_trylock(isc__rwlock_t *rwl, isc_rwlocktype_t type) { - int ret = 0; - switch (type) { - case isc_rwlocktype_read: - ret = pthread_rwlock_tryrdlock(rwl); - break; - case isc_rwlocktype_write: - ret = pthread_rwlock_trywrlock(rwl); - break; - default: - UNREACHABLE(); - } - - switch (ret) { - case 0: - return (ISC_R_SUCCESS); - case EBUSY: - return (ISC_R_LOCKBUSY); - case EAGAIN: - return (ISC_R_LOCKBUSY); - default: - break; - } - - switch (type) { - case isc_rwlocktype_read: - PTHREADS_RUNTIME_CHECK(pthread_rwlock_tryrdlock, ret); - break; - case isc_rwlocktype_write: - PTHREADS_RUNTIME_CHECK(pthread_rwlock_trywrlock, ret); - break; - default: - break; - } - UNREACHABLE(); -} - -void -isc__rwlock_unlock(isc__rwlock_t *rwl, isc_rwlocktype_t type) { - int ret; - UNUSED(type); - ret = pthread_rwlock_unlock(rwl); - PTHREADS_RUNTIME_CHECK(pthread_rwlock_rwlock, ret); -} - -isc_result_t -isc__rwlock_tryupgrade(isc__rwlock_t *rwl) { - UNUSED(rwl); - return (ISC_R_LOCKBUSY); -} - -void -isc__rwlock_destroy(isc__rwlock_t *rwl) { - int ret = pthread_rwlock_destroy(rwl); - - PTHREADS_RUNTIME_CHECK(pthread_rwlock_destroy, ret); -} - -#else /* if USE_PTHREAD_RWLOCK */ - #define RWLOCK_MAGIC ISC_MAGIC('R', 'W', 'L', 'k') #define VALID_RWLOCK(rwl) ISC_MAGIC_VALID(rwl, RWLOCK_MAGIC) @@ -178,7 +84,7 @@ print_lock(const char *operation, isc__rwlock_t *rwl, isc_rwlocktype_t type) { atomic_load_acquire(&rwl->cnt_and_flag), rwl->readers_waiting, atomic_load_acquire(&rwl->write_granted), rwl->write_quota); } -#endif /* ISC_RWLOCK_TRACE */ +#endif /* ISC_RWLOCK_TRACE */ void isc__rwlock_init(isc__rwlock_t *rwl, unsigned int read_quota, @@ -595,5 +501,3 @@ isc__rwlock_unlock(isc__rwlock_t *rwl, isc_rwlocktype_t type) { print_lock("postunlock", rwl, type); #endif /* ifdef ISC_RWLOCK_TRACE */ } - -#endif /* USE_PTHREAD_RWLOCK */