From c319ccd4c9bb3f2d2b41ea7d67b0b79a33382fab Mon Sep 17 00:00:00 2001 From: Tony Finch Date: Mon, 15 May 2023 11:42:33 +0100 Subject: [PATCH] Fixes for liburcu-qsbr Move registration and deregistration of the main thread from `isc_loopmgr_run()` into `isc__initialize()` / `isc__shutdown()`: liburcu-qsbr fails an assertion if we try to use it from an unregistered thread, and we need to be able to use it when the event loops are not running. Use `rcu_assign_pointer()` and `rcu_dereference()` in qp-trie transactions so that they properly mark threads as online. The RCU-protected pointer is no longer declared atomic because liburcu does not (yet) use standard C atomics. Fix the definition of `isc_qsbr_rcu_dereference()` to return the referenced value, and to call the right function inside liburcu. Change the thread sanitizer suppressions to match any variant of `rcu_*_barrier()` --- .tsan-suppress | 2 +- lib/dns/qp.c | 6 ++---- lib/dns/qp_p.h | 4 ++-- lib/isc/include/isc/urcu.h | 6 +++--- lib/isc/lib.c | 4 ++++ lib/isc/loop.c | 4 ---- tests/libtest/qp.c | 2 +- 7 files changed, 13 insertions(+), 15 deletions(-) diff --git a/.tsan-suppress b/.tsan-suppress index c73eb7815f..77aa285e0a 100644 --- a/.tsan-suppress +++ b/.tsan-suppress @@ -1,4 +1,4 @@ # be more selective with liburcu race:rcu_barrier -race:rcu_memb_barrier +race:rcu_*_barrier thread:* diff --git a/lib/dns/qp.c b/lib/dns/qp.c index 7633c4d5aa..10d19e1b1b 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -1169,8 +1169,7 @@ dns_qpmulti_commit(dns_qpmulti_t *multi, dns_qp_t **qptp) { /* paired with chunk_free() */ isc_refcount_increment(&qp->base->refcount); - /* reader_open() below has the matching atomic_load_acquire() */ - atomic_store_release(&multi->reader, reader); /* COMMIT */ + rcu_assign_pointer(multi->reader, reader); /* COMMIT */ /* clean up what we can right now */ if (qp->transaction_mode == QP_UPDATE || QP_NEEDGC(qp)) { @@ -1249,8 +1248,7 @@ dns_qpmulti_rollback(dns_qpmulti_t *multi, dns_qp_t **qptp) { static dns_qpmulti_t * reader_open(dns_qpmulti_t *multi, dns_qpreadable_t qpr) { dns_qpreader_t *qp = dns_qpreader(qpr); - /* dns_qpmulti_commit() has the matching atomic_store_release() */ - qp_node_t *reader = atomic_load_acquire(&multi->reader); + qp_node_t *reader = rcu_dereference(multi->reader); if (reader == NULL) { QP_INIT(qp, multi->writer.methods, multi->writer.uctx); } else { diff --git a/lib/dns/qp_p.h b/lib/dns/qp_p.h index e9054da953..c0f69c3568 100644 --- a/lib/dns/qp_p.h +++ b/lib/dns/qp_p.h @@ -549,8 +549,8 @@ struct dns_qp { */ struct dns_qpmulti { uint32_t magic; - /*% pointer to current packed reader */ - atomic_ptr(qp_node_t) reader; + /*% RCU-protected pointer to current packed reader */ + qp_node_t *reader; /*% the mutex protects the rest of this structure */ isc_mutex_t mutex; /*% ref_ptr(writer, reader_ref) == reader */ diff --git a/lib/isc/include/isc/urcu.h b/lib/isc/include/isc/urcu.h index 0b828d92e6..48a1ac46da 100644 --- a/lib/isc/include/isc/urcu.h +++ b/lib/isc/include/isc/urcu.h @@ -109,12 +109,12 @@ #define synchronize_rcu() isc_qsbr_syncronize_rcu() #define isc_qsbr_rcu_dereference(ptr) \ - { \ + ({ \ if (!urcu_qsbr_read_ongoing()) { \ urcu_qsbr_thread_online(); \ } \ - urcu_qsbr_dereference(ptr); \ - } + _rcu_dereference(ptr); \ + }) #undef rcu_dereference #define rcu_dereference(ptr) isc_qsbr_rcu_dereference(ptr) diff --git a/lib/isc/lib.c b/lib/isc/lib.c index bef3e25558..aba5dc1db8 100644 --- a/lib/isc/lib.c +++ b/lib/isc/lib.c @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -51,6 +52,7 @@ isc__initialize(void) { isc__md_initialize(); isc__iterated_hash_initialize(); (void)isc_os_ncpus(); + rcu_register_thread(); } void @@ -63,4 +65,6 @@ isc__shutdown(void) { isc__mem_shutdown(); isc__mutex_shutdown(); isc__os_shutdown(); + /* should be after isc__mem_shutdown() which calls rcu_barrier() */ + rcu_unregister_thread(); } diff --git a/lib/isc/loop.c b/lib/isc/loop.c index ddbc83d37b..f9ebc49fe2 100644 --- a/lib/isc/loop.c +++ b/lib/isc/loop.c @@ -436,8 +436,6 @@ isc_loopmgr_run(isc_loopmgr_t *loopmgr) { bool free_call_rcu_data = !create_all_cpu_call_rcu_data(0); - rcu_register_thread(); - /* * The thread 0 is this one. */ @@ -453,8 +451,6 @@ isc_loopmgr_run(isc_loopmgr_t *loopmgr) { isc_thread_main(loop_thread, &loopmgr->loops[0]); - rcu_unregister_thread(); - rcu_barrier(); if (free_call_rcu_data) { diff --git a/tests/libtest/qp.c b/tests/libtest/qp.c index aff5eb3dc6..392297746a 100644 --- a/tests/libtest/qp.c +++ b/tests/libtest/qp.c @@ -232,7 +232,7 @@ qp_test_dumpqp(dns_qp_t *qp) { void qp_test_dumpmulti(dns_qpmulti_t *multi) { dns_qpreader_t qpr; - qp_node_t *reader = atomic_load(&multi->reader); + qp_node_t *reader = rcu_dereference(multi->reader); dns_qpmulti_t *whence = unpack_reader(&qpr, reader); dumpqp(&multi->writer, "qpmulti->writer"); printf("qpmulti->reader %p root_ref %u %u:%u base %p\n", reader,