From d1d444d2ab1648ed0fac2785452bee6e4e1e2f02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Mon, 27 Jan 2025 18:06:17 +0100 Subject: [PATCH 1/6] Refactor decref() in both qpcache.c and qpzone.c Cleanup the pattern in the decref() functions in both qpcache.c and qpzone.c, so it follows the similar patter as we already have in newref() function. (cherry picked from commit 814b87da648b2471947e4f9ac25069ab83f38ea3) --- lib/dns/qpcache.c | 147 ++++++++++++++++++++++------------------------ lib/dns/qpzone.c | 106 ++++++++++++++++----------------- 2 files changed, 119 insertions(+), 134 deletions(-) diff --git a/lib/dns/qpcache.c b/lib/dns/qpcache.c index d3e86bf91a..8d3d93d8bd 100644 --- a/lib/dns/qpcache.c +++ b/lib/dns/qpcache.c @@ -626,30 +626,32 @@ qpcnode_newref(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t nlocktype, func, file, line, node, refs + 1); #endif - if (refs == 0) { - /* - * this is the first external reference to the node. - * - * we need to hold the node or tree lock to avoid - * incrementing the reference count while also deleting - * the node. delete_node() is always protected by both - * tree and node locks being write-locked. - */ - INSIST(nlocktype != isc_rwlocktype_none || - tlocktype != isc_rwlocktype_none); - - refs = isc_refcount_increment0( - &qpdb->node_locks[node->locknum].references); -#if DNS_DB_NODETRACE - fprintf(stderr, - "incr:nodelock:%s:%s:%u:%p:%p->references = " - "%" PRIuFAST32 "\n", - func, file, line, node, - &qpdb->node_locks[node->locknum], refs + 1); -#else - UNUSED(refs); -#endif + if (refs > 0) { + return; } + + /* + * this is the first external reference to the node. + * + * we need to hold the node or tree lock to avoid + * incrementing the reference count while also deleting + * the node. delete_node() is always protected by both + * tree and node locks being write-locked. + */ + INSIST(nlocktype != isc_rwlocktype_none || + tlocktype != isc_rwlocktype_none); + + refs = isc_refcount_increment0( + &qpdb->node_locks[node->locknum].references); +#if DNS_DB_NODETRACE + fprintf(stderr, + "incr:nodelock:%s:%s:%u:%p:%p->references = " + "%" PRIuFAST32 "\n", + func, file, line, node, &qpdb->node_locks[node->locknum], + refs + 1); +#else + UNUSED(refs); +#endif } static void @@ -662,6 +664,33 @@ newref(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t nlocktype, static void cleanup_deadnodes(void *arg); +static bool +qpcnode_decref(qpcache_t *qpdb, qpcnode_t *node DNS__DB_FLARG) { + uint_fast32_t refs = isc_refcount_decrement(&node->erefs); + +#if DNS_DB_NODETRACE + fprintf(stderr, "decr:node:%s:%s:%u:%p->erefs = %" PRIuFAST32 "\n", + func, file, line, node, refs - 1); +#endif + if (refs > 1) { + return false; + } + + refs = isc_refcount_decrement( + &qpdb->node_locks[node->locknum].references); +#if DNS_DB_NODETRACE + fprintf(stderr, + "decr:nodelock:%s:%s:%u:%p:%p->references = " + "%" PRIuFAST32 "\n", + func, file, line, node, &qpdb->node_locks[node->locknum], + refs - 1); +#else + UNUSED(refs); +#endif + + return true; +} + /* * Caller must be holding the node lock; either the read or write lock. * Note that the lock must be held even when node references are @@ -678,42 +707,22 @@ cleanup_deadnodes(void *arg); * to zero. (NOTE: Decrementing the reference count of a node to zero does * not mean it will be immediately freed.) */ -static bool +static void decref(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t *nlocktypep, isc_rwlocktype_t *tlocktypep, bool tryupgrade DNS__DB_FLARG) { isc_result_t result; bool locked = *tlocktypep != isc_rwlocktype_none; bool write_locked = false; - int bucket = node->locknum; - db_nodelock_t *nodelock = &qpdb->node_locks[bucket]; - uint_fast32_t refs; REQUIRE(*nlocktypep != isc_rwlocktype_none); - refs = isc_refcount_decrement(&node->erefs); -#if DNS_DB_NODETRACE - fprintf(stderr, "decr:node:%s:%s:%u:%p->erefs = %" PRIuFAST32 "\n", - func, file, line, node, refs - 1); -#endif - if (refs > 1) { - qpcnode_unref(node); - return false; + if (!qpcnode_decref(qpdb, node DNS__DB_FLARG_PASS)) { + goto unref; } - refs = isc_refcount_decrement(&nodelock->references); -#if DNS_DB_NODETRACE - fprintf(stderr, - "decr:nodelock:%s:%s:%u:%p:%p->references = " - "%" PRIuFAST32 "\n", - func, file, line, node, nodelock, refs - 1); -#else - UNUSED(refs); -#endif - /* Handle easy and typical case first. */ if (!node->dirty && (node->data != NULL || node == qpdb->origin_node)) { - qpcnode_unref(node); - return false; + goto unref; } /* @@ -725,18 +734,12 @@ decref(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t *nlocktypep, /* Upgrade the lock? */ if (*nlocktypep == isc_rwlocktype_read) { - NODE_FORCEUPGRADE(&nodelock->lock, nlocktypep); + NODE_FORCEUPGRADE(&qpdb->node_locks[node->locknum].lock, + nlocktypep); } - refs = isc_refcount_decrement(&node->erefs); -#if DNS_DB_NODETRACE - fprintf(stderr, "decr:node:%s:%s:%u:%p->erefs = %" PRIuFAST32 "\n", - func, file, line, node, refs - 1); -#endif - - if (refs > 1) { - qpcnode_unref(node); - return false; + if (!qpcnode_decref(qpdb, node DNS__DB_FLARG_PASS)) { + goto unref; } if (node->dirty) { @@ -776,21 +779,10 @@ decref(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t *nlocktypep, write_locked = true; } - refs = isc_refcount_decrement(&nodelock->references); -#if DNS_DB_NODETRACE - fprintf(stderr, - "decr:nodelock:%s:%s:%u:%p:%p->references = %" PRIuFAST32 "\n", - func, file, line, node, nodelock, refs - 1); -#else - UNUSED(refs); -#endif - if (node->data != NULL || node == qpdb->origin_node) { goto restore_locks; } -#undef KEEP_NODE - if (write_locked) { /* * We can now delete the node. @@ -800,11 +792,12 @@ decref(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t *nlocktypep, newref(qpdb, node, *nlocktypep, *tlocktypep DNS__DB_FLARG_PASS); isc_queue_node_init(&node->deadlink); - if (!isc_queue_enqueue_entry(&qpdb->deadnodes[bucket], node, - deadlink)) + if (!isc_queue_enqueue_entry(&qpdb->deadnodes[node->locknum], + node, deadlink)) { /* Queue was empty, trigger new cleaning */ - isc_loop_t *loop = isc_loop_get(qpdb->loopmgr, bucket); + isc_loop_t *loop = isc_loop_get(qpdb->loopmgr, + node->locknum); isc_async_run(loop, cleanup_deadnodes, qpdb); } @@ -818,8 +811,8 @@ restore_locks: TREE_UNLOCK(&qpdb->tree_lock, tlocktypep); } +unref: qpcnode_unref(node); - return true; } static void @@ -2752,13 +2745,11 @@ detachnode(dns_db_t *db, dns_dbnode_t **targetp DNS__DB_FLARG) { NODE_RDLOCK(&nodelock->lock, &nlocktype); - if (decref(qpdb, node, &nlocktype, &tlocktype, true DNS__DB_FLARG_PASS)) + decref(qpdb, node, &nlocktype, &tlocktype, true DNS__DB_FLARG_PASS); + if (isc_refcount_current(&nodelock->references) == 0 && + nodelock->exiting) { - if (isc_refcount_current(&nodelock->references) == 0 && - nodelock->exiting) - { - inactive = true; - } + inactive = true; } NODE_UNLOCK(&nodelock->lock, &nlocktype); diff --git a/lib/dns/qpzone.c b/lib/dns/qpzone.c index 243b8a74e9..20f0efdf66 100644 --- a/lib/dns/qpzone.c +++ b/lib/dns/qpzone.c @@ -665,20 +665,22 @@ qpznode_newref(qpzonedb_t *qpdb, qpznode_t *node DNS__DB_FLARG) { func, file, line, node, refs + 1); #endif - if (refs == 0) { - /* this is the first reference to the node */ - refs = isc_refcount_increment0( - &qpdb->node_locks[node->locknum].references); -#if DNS_DB_NODETRACE - fprintf(stderr, - "incr:nodelock:%s:%s:%u:%p:%p->references = " - "%" PRIuFAST32 "\n", - func, file, line, node, - &qpdb->node_locks[node->locknum], refs + 1); -#else - UNUSED(refs); -#endif + if (refs > 0) { + return; } + + /* this is the first reference to the node */ + refs = isc_refcount_increment0( + &qpdb->node_locks[node->locknum].references); +#if DNS_DB_NODETRACE + fprintf(stderr, + "incr:nodelock:%s:%s:%u:%p:%p->references = " + "%" PRIuFAST32 "\n", + func, file, line, node, &qpdb->node_locks[node->locknum], + refs + 1); +#else + UNUSED(refs); +#endif } static void @@ -804,6 +806,33 @@ clean_zone_node(qpznode_t *node, uint32_t least_serial) { } } +static bool +qpznode_decref(qpzonedb_t *qpdb, qpznode_t *node DNS__DB_FLARG) { + uint_fast32_t refs = isc_refcount_decrement(&node->erefs); + +#if DNS_DB_NODETRACE + fprintf(stderr, "decr:node:%s:%s:%u:%p->erefs = %" PRIuFAST32 "\n", + func, file, line, node, refs - 1); +#endif + if (refs > 1) { + return false; + } + + refs = isc_refcount_decrement( + &qpdb->node_locks[node->locknum].references); +#if DNS_DB_NODETRACE + fprintf(stderr, + "decr:nodelock:%s:%s:%u:%p:%p->references = " + "%" PRIuFAST32 "\n", + func, file, line, node, &qpdb->node_locks[node->locknum], + refs - 1); +#else + UNUSED(refs); +#endif + + return true; +} + /* * Caller must be holding the node lock; either the read or write lock. * Note that the lock must be held even when node references are @@ -822,38 +851,17 @@ clean_zone_node(qpznode_t *node, uint32_t least_serial) { static void decref(qpzonedb_t *qpdb, qpznode_t *node, uint32_t least_serial, isc_rwlocktype_t *nlocktypep DNS__DB_FLARG) { - int bucket = node->locknum; - db_nodelock_t *nodelock = &qpdb->node_locks[bucket]; - uint_fast32_t refs; - REQUIRE(*nlocktypep != isc_rwlocktype_none); - refs = isc_refcount_decrement(&node->erefs); -#if DNS_DB_NODETRACE - fprintf(stderr, "decr:node:%s:%s:%u:%p->erefs = %" PRIuFAST32 "\n", - func, file, line, node, refs - 1); -#endif - if (refs > 1) { - qpznode_unref(node); - return; + if (!qpznode_decref(qpdb, node DNS__DB_FLARG_PASS)) { + goto unref; } - refs = isc_refcount_decrement(&nodelock->references); -#if DNS_DB_NODETRACE - fprintf(stderr, - "decr:nodelock:%s:%s:%u:%p:%p->references = " - "%" PRIuFAST32 "\n", - func, file, line, node, nodelock, refs - 1); -#else - UNUSED(refs); -#endif - /* Handle easy and typical case first. */ if (!node->dirty && (node->data != NULL || node == qpdb->origin || node == qpdb->nsec3_origin)) { - qpznode_unref(node); - return; + goto unref; } /* @@ -865,17 +873,12 @@ decref(qpzonedb_t *qpdb, qpznode_t *node, uint32_t least_serial, /* Upgrade the lock? */ if (*nlocktypep == isc_rwlocktype_read) { - NODE_FORCEUPGRADE(&nodelock->lock, nlocktypep); + NODE_FORCEUPGRADE(&qpdb->node_locks[node->locknum].lock, + nlocktypep); } - refs = isc_refcount_decrement(&node->erefs); -#if DNS_DB_NODETRACE - fprintf(stderr, "decr:node:%s:%s:%u:%p->erefs = %" PRIuFAST32 "\n", - func, file, line, node, refs - 1); -#endif - if (refs > 1) { - qpznode_unref(node); - return; + if (!qpznode_decref(qpdb, node DNS__DB_FLARG_PASS)) { + goto unref; } if (node->dirty) { @@ -891,16 +894,7 @@ decref(qpzonedb_t *qpdb, qpznode_t *node, uint32_t least_serial, clean_zone_node(node, least_serial); } - refs = isc_refcount_decrement(&nodelock->references); -#if DNS_DB_NODETRACE - fprintf(stderr, - "decr:nodelock:%s:%s:%u:%p:%p->references = " - "%" PRIuFAST32 "\n", - func, file, line, node, nodelock, refs - 1); -#else - UNUSED(refs); -#endif - +unref: qpznode_unref(node); } From 082a54cc5d8e5c5490e990c772d78b81472c1d0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Mon, 27 Jan 2025 18:13:38 +0100 Subject: [PATCH 2/6] Remove origin_node from qpcache The origin_node in qpcache was always NULL, so we can remove the getoriginode() function and origin_node pointer as the dns_db_getoriginnode() correctly returns ISC_R_NOTFOUND when the function is not implemented. (cherry picked from commit 36a26bfa1a12f4711a1fd4e8aafe37fb0a355b1b) --- lib/dns/qpcache.c | 32 ++------------------------------ 1 file changed, 2 insertions(+), 30 deletions(-) diff --git a/lib/dns/qpcache.c b/lib/dns/qpcache.c index 8d3d93d8bd..26e373aeb7 100644 --- a/lib/dns/qpcache.c +++ b/lib/dns/qpcache.c @@ -208,7 +208,6 @@ struct qpcache { /* Locks for individual tree nodes */ unsigned int node_lock_count; db_nodelock_t *node_locks; - qpcnode_t *origin_node; dns_stats_t *rrsetstats; /* cache DB only */ isc_stats_t *cachestats; /* cache DB only */ isc_stats_t *gluecachestats; /* zone DB only */ @@ -721,7 +720,7 @@ decref(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t *nlocktypep, } /* Handle easy and typical case first. */ - if (!node->dirty && (node->data != NULL || node == qpdb->origin_node)) { + if (!node->dirty && node->data != NULL) { goto unref; } @@ -779,7 +778,7 @@ decref(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t *nlocktypep, write_locked = true; } - if (node->data != NULL || node == qpdb->origin_node) { + if (node->data != NULL) { goto restore_locks; } @@ -2555,10 +2554,6 @@ qpdb_destroy(dns_db_t *arg) { unsigned int i; unsigned int inactive = 0; - if (qpdb->origin_node != NULL) { - qpcnode_detach(&qpdb->origin_node); - } - /* * Even though there are no external direct references, there still * may be nodes in use. @@ -3620,28 +3615,6 @@ nodecount(dns_db_t *db, dns_dbtree_t tree) { return mu.leaves; } -static isc_result_t -getoriginnode(dns_db_t *db, dns_dbnode_t **nodep DNS__DB_FLARG) { - qpcache_t *qpdb = (qpcache_t *)db; - qpcnode_t *onode = NULL; - isc_result_t result = ISC_R_SUCCESS; - - REQUIRE(VALID_QPDB(qpdb)); - REQUIRE(nodep != NULL && *nodep == NULL); - - /* Note that the access to origin_node doesn't require a DB lock */ - onode = (qpcnode_t *)qpdb->origin_node; - if (onode != NULL) { - newref(qpdb, onode, isc_rwlocktype_none, - isc_rwlocktype_none DNS__DB_FLARG_PASS); - *nodep = qpdb->origin_node; - } else { - result = ISC_R_NOTFOUND; - } - - return result; -} - static void locknode(dns_db_t *db, dns_dbnode_t *node, isc_rwlocktype_t type) { qpcache_t *qpdb = (qpcache_t *)db; @@ -4385,7 +4358,6 @@ static dns_dbmethods_t qpdb_cachemethods = { .addrdataset = addrdataset, .deleterdataset = deleterdataset, .nodecount = nodecount, - .getoriginnode = getoriginnode, .getrrsetstats = getrrsetstats, .setcachestats = setcachestats, .setservestalettl = setservestalettl, From 7dab6cdfbc26bfa3aa37168c382563047d4dee95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Mon, 27 Jan 2025 21:07:11 +0100 Subject: [PATCH 3/6] Remove db_nodelock_t in favor of reference counted qpdb This removes the db_nodelock_t structure and changes the node_locks array to be composed only of isc_rwlock_t pointers. The .reference member has been moved to qpdb->references in addition to common.references that's external to dns_db API users. The .exiting members has been completely removed as it has no use when the reference counting is used correctly. (cherry picked from commit 431513d8b3cc595a7612479fd7811e959501e6dd) --- lib/dns/qpcache.c | 336 ++++++++++++++++--------------------- lib/dns/qpzone.c | 355 +++++++++++++++++----------------------- tests/dns/qpzone_test.c | 12 +- 3 files changed, 296 insertions(+), 407 deletions(-) diff --git a/lib/dns/qpcache.c b/lib/dns/qpcache.c index 26e373aeb7..23b566bcd6 100644 --- a/lib/dns/qpcache.c +++ b/lib/dns/qpcache.c @@ -207,12 +207,12 @@ struct qpcache { isc_rwlock_t tree_lock; /* Locks for individual tree nodes */ unsigned int node_lock_count; - db_nodelock_t *node_locks; dns_stats_t *rrsetstats; /* cache DB only */ isc_stats_t *cachestats; /* cache DB only */ isc_stats_t *gluecachestats; /* zone DB only */ - /* Locked by lock. */ - unsigned int active; + + isc_refcount_t references; + isc_rwlock_t *node_locks; uint32_t maxrrperset; /* Maximum RRs per RRset */ uint32_t maxtypepername; /* Maximum number of RR types per owner */ @@ -262,6 +262,17 @@ struct qpcache { dns_qp_t *nsec; }; +#ifdef DNS_DB_NODETRACE +#define qpcache_ref(ptr) qpcache__ref(ptr, __func__, __FILE__, __LINE__) +#define qpcache_unref(ptr) qpcache_unref(ptr, __func__, __FILE__, __LINE__) +#define qpcache_attach(ptr, ptrp) \ + qpcache__attach(ptr, ptrp, __func__, __FILE__, __LINE__) +#define qpcache_detach(ptrp) qpcache__detach(ptrp, __func__, __FILE__, __LINE__) +ISC_REFCOUNT_STATIC_TRACE_DECL(qpcache); +#else +ISC_REFCOUNT_STATIC_DECL(qpcache); +#endif + /*% * Search Context */ @@ -399,7 +410,7 @@ typedef struct qpc_dbit { } qpc_dbit_t; static void -free_qpdb(qpcache_t *qpdb, bool log); +qpcache__destroy(qpcache_t *qpdb); static dns_dbmethods_t qpdb_cachemethods; @@ -640,17 +651,7 @@ qpcnode_newref(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t nlocktype, INSIST(nlocktype != isc_rwlocktype_none || tlocktype != isc_rwlocktype_none); - refs = isc_refcount_increment0( - &qpdb->node_locks[node->locknum].references); -#if DNS_DB_NODETRACE - fprintf(stderr, - "incr:nodelock:%s:%s:%u:%p:%p->references = " - "%" PRIuFAST32 "\n", - func, file, line, node, &qpdb->node_locks[node->locknum], - refs + 1); -#else - UNUSED(refs); -#endif + qpcache_ref(qpdb); } static void @@ -675,17 +676,7 @@ qpcnode_decref(qpcache_t *qpdb, qpcnode_t *node DNS__DB_FLARG) { return false; } - refs = isc_refcount_decrement( - &qpdb->node_locks[node->locknum].references); -#if DNS_DB_NODETRACE - fprintf(stderr, - "decr:nodelock:%s:%s:%u:%p:%p->references = " - "%" PRIuFAST32 "\n", - func, file, line, node, &qpdb->node_locks[node->locknum], - refs - 1); -#else - UNUSED(refs); -#endif + qpcache_unref(qpdb); return true; } @@ -712,6 +703,7 @@ decref(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t *nlocktypep, isc_result_t result; bool locked = *tlocktypep != isc_rwlocktype_none; bool write_locked = false; + isc_rwlock_t *nlock = &qpdb->node_locks[node->locknum]; REQUIRE(*nlocktypep != isc_rwlocktype_none); @@ -733,8 +725,7 @@ decref(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t *nlocktypep, /* Upgrade the lock? */ if (*nlocktypep == isc_rwlocktype_read) { - NODE_FORCEUPGRADE(&qpdb->node_locks[node->locknum].lock, - nlocktypep); + NODE_FORCEUPGRADE(nlock, nlocktypep); } if (!qpcnode_decref(qpdb, node DNS__DB_FLARG_PASS)) { @@ -1114,8 +1105,8 @@ setup_delegation(qpc_search_t *search, dns_dbnode_t **nodep, } if (rdataset != NULL) { isc_rwlocktype_t nlocktype = isc_rwlocktype_none; - NODE_RDLOCK(&(search->qpdb->node_locks[node->locknum].lock), - &nlocktype); + isc_rwlock_t *nlock = &search->qpdb->node_locks[node->locknum]; + NODE_RDLOCK(nlock, &nlocktype); bindrdataset(search->qpdb, node, search->zonecut_header, search->now, nlocktype, tlocktype, rdataset DNS__DB_FLARG_PASS); @@ -1125,8 +1116,7 @@ setup_delegation(qpc_search_t *search, dns_dbnode_t **nodep, nlocktype, tlocktype, sigrdataset DNS__DB_FLARG_PASS); } - NODE_UNLOCK(&(search->qpdb->node_locks[node->locknum].lock), - &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); } if (type == dns_rdatatype_dname) { @@ -1137,7 +1127,7 @@ setup_delegation(qpc_search_t *search, dns_dbnode_t **nodep, static bool check_stale_header(qpcnode_t *node, dns_slabheader_t *header, - isc_rwlocktype_t *nlocktypep, isc_rwlock_t *lock, + isc_rwlocktype_t *nlocktypep, isc_rwlock_t *nlock, qpc_search_t *search, dns_slabheader_t **header_prev) { if (!ACTIVE(header, search->now)) { dns_ttl_t stale = header->ttl + STALE_TTL(header, search->qpdb); @@ -1201,7 +1191,7 @@ check_stale_header(qpcnode_t *node, dns_slabheader_t *header, */ if ((header->ttl < search->now - QPDB_VIRTUAL) && (*nlocktypep == isc_rwlocktype_write || - NODE_TRYUPGRADE(lock, nlocktypep) == ISC_R_SUCCESS)) + NODE_TRYUPGRADE(nlock, nlocktypep) == ISC_R_SUCCESS)) { /* * We update the node's status only when we can @@ -1248,20 +1238,20 @@ check_zonecut(qpcnode_t *node, void *arg DNS__DB_FLARG) { dns_slabheader_t *header_prev = NULL, *header_next = NULL; dns_slabheader_t *dname_header = NULL, *sigdname_header = NULL; isc_result_t result; - isc_rwlock_t *lock = NULL; + isc_rwlock_t *nlock = NULL; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; REQUIRE(search->zonecut == NULL); - lock = &(search->qpdb->node_locks[node->locknum].lock); - NODE_RDLOCK(lock, &nlocktype); + nlock = &search->qpdb->node_locks[node->locknum]; + NODE_RDLOCK(nlock, &nlocktype); /* * Look for a DNAME or RRSIG DNAME rdataset. */ for (header = node->data; header != NULL; header = header_next) { header_next = header->next; - if (check_stale_header(node, header, &nlocktype, lock, search, + if (check_stale_header(node, header, &nlocktype, nlock, search, &header_prev)) { /* Do nothing. */ @@ -1299,7 +1289,7 @@ check_zonecut(qpcnode_t *node, void *arg DNS__DB_FLARG) { result = DNS_R_CONTINUE; } - NODE_UNLOCK(lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); return result; } @@ -1322,13 +1312,13 @@ find_deepest_zonecut(qpc_search_t *search, qpcnode_t *node, dns_slabheader_t *header = NULL; dns_slabheader_t *header_prev = NULL, *header_next = NULL; dns_slabheader_t *found = NULL, *foundsig = NULL; - isc_rwlock_t *lock = NULL; + isc_rwlock_t *nlock = NULL; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; dns_qpchain_node(&search->chain, i, NULL, (void **)&node, NULL); - lock = &qpdb->node_locks[node->locknum].lock; + nlock = &qpdb->node_locks[node->locknum]; - NODE_RDLOCK(lock, &nlocktype); + NODE_RDLOCK(nlock, &nlocktype); /* * Look for NS and RRSIG NS rdatasets. @@ -1336,7 +1326,7 @@ find_deepest_zonecut(qpc_search_t *search, qpcnode_t *node, for (header = node->data; header != NULL; header = header_next) { header_next = header->next; - if (check_stale_header(node, header, &nlocktype, lock, + if (check_stale_header(node, header, &nlocktype, nlock, search, &header_prev)) { /* Do nothing. */ @@ -1392,7 +1382,7 @@ find_deepest_zonecut(qpc_search_t *search, qpcnode_t *node, need_headerupdate(foundsig, search->now))) { if (nlocktype != isc_rwlocktype_write) { - NODE_FORCEUPGRADE(lock, &nlocktype); + NODE_FORCEUPGRADE(nlock, &nlocktype); POST(nlocktype); } if (need_headerupdate(found, search->now)) { @@ -1408,7 +1398,7 @@ find_deepest_zonecut(qpc_search_t *search, qpcnode_t *node, } } - NODE_UNLOCK(lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); if (found != NULL) { break; @@ -1436,7 +1426,7 @@ find_coveringnsec(qpc_search_t *search, const dns_name_t *name, dns_qpiter_t iter; isc_result_t result; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; - isc_rwlock_t *lock = NULL; + isc_rwlock_t *nlock = NULL; dns_typepair_t matchtype, sigmatchtype; dns_slabheader_t *found = NULL, *foundsig = NULL; dns_slabheader_t *header = NULL; @@ -1475,11 +1465,11 @@ find_coveringnsec(qpc_search_t *search, const dns_name_t *name, } dns_name_copy(&node->name, fname); - lock = &(search->qpdb->node_locks[node->locknum].lock); - NODE_RDLOCK(lock, &nlocktype); + nlock = &search->qpdb->node_locks[node->locknum]; + NODE_RDLOCK(nlock, &nlocktype); for (header = node->data; header != NULL; header = header_next) { header_next = header->next; - if (check_stale_header(node, header, &nlocktype, lock, search, + if (check_stale_header(node, header, &nlocktype, nlock, search, &header_prev)) { continue; @@ -1520,7 +1510,7 @@ find_coveringnsec(qpc_search_t *search, const dns_name_t *name, } else { result = ISC_R_NOTFOUND; } - NODE_UNLOCK(lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); return result; } @@ -1536,7 +1526,7 @@ find(dns_db_t *db, const dns_name_t *name, dns_dbversion_t *version, bool found_noqname = false; bool all_negative = true; bool empty_node; - isc_rwlock_t *lock = NULL; + isc_rwlock_t *nlock = NULL; isc_rwlocktype_t tlocktype = isc_rwlocktype_none; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; dns_slabheader_t *header = NULL; @@ -1652,8 +1642,8 @@ find(dns_db_t *db, const dns_name_t *name, dns_dbversion_t *version, * We now go looking for rdata... */ - lock = &(search.qpdb->node_locks[node->locknum].lock); - NODE_RDLOCK(lock, &nlocktype); + nlock = &search.qpdb->node_locks[node->locknum]; + NODE_RDLOCK(nlock, &nlocktype); /* * These pointers need to be reset here in case we did @@ -1672,7 +1662,7 @@ find(dns_db_t *db, const dns_name_t *name, dns_dbversion_t *version, header_prev = NULL; for (header = node->data; header != NULL; header = header_next) { header_next = header->next; - if (check_stale_header(node, header, &nlocktype, lock, &search, + if (check_stale_header(node, header, &nlocktype, nlock, &search, &header_prev)) { /* Do nothing. */ @@ -1774,7 +1764,7 @@ find(dns_db_t *db, const dns_name_t *name, dns_dbversion_t *version, * extant rdatasets. That means that this node doesn't * meaningfully exist, and that we really have a partial match. */ - NODE_UNLOCK(lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); if ((search.options & DNS_DBFIND_COVERINGNSEC) != 0) { result = find_coveringnsec( &search, name, nodep, now, foundname, rdataset, @@ -1832,7 +1822,7 @@ find(dns_db_t *db, const dns_name_t *name, dns_dbversion_t *version, if (found == NULL && (found_noqname || all_negative) && (search.options & DNS_DBFIND_COVERINGNSEC) != 0) { - NODE_UNLOCK(lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); result = find_coveringnsec( &search, name, nodep, now, foundname, rdataset, sigrdataset DNS__DB_FLARG_PASS); @@ -1873,7 +1863,7 @@ find(dns_db_t *db, const dns_name_t *name, dns_dbversion_t *version, /* * Go find the deepest zone cut. */ - NODE_UNLOCK(lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); goto find_ns; } @@ -1934,7 +1924,7 @@ node_exit: if ((update != NULL || updatesig != NULL) && nlocktype != isc_rwlocktype_write) { - NODE_FORCEUPGRADE(lock, &nlocktype); + NODE_FORCEUPGRADE(nlock, &nlocktype); POST(nlocktype); } if (update != NULL && need_headerupdate(update, search.now)) { @@ -1944,7 +1934,7 @@ node_exit: update_header(search.qpdb, updatesig, search.now); } - NODE_UNLOCK(lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); tree_exit: TREE_UNLOCK(&search.qpdb->tree_lock, &tlocktype); @@ -1956,12 +1946,12 @@ tree_exit: if (search.need_cleanup) { node = search.zonecut; INSIST(node != NULL); - lock = &(search.qpdb->node_locks[node->locknum].lock); + nlock = &search.qpdb->node_locks[node->locknum]; - NODE_RDLOCK(lock, &nlocktype); + NODE_RDLOCK(nlock, &nlocktype); decref(search.qpdb, node, &nlocktype, &tlocktype, true DNS__DB_FLARG_PASS); - NODE_UNLOCK(lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); INSIST(tlocktype == isc_rwlocktype_none); } @@ -1975,7 +1965,7 @@ findzonecut(dns_db_t *db, const dns_name_t *name, unsigned int options, dns_name_t *dcname, dns_rdataset_t *rdataset, dns_rdataset_t *sigrdataset DNS__DB_FLARG) { qpcnode_t *node = NULL; - isc_rwlock_t *lock = NULL; + isc_rwlock_t *nlock = NULL; isc_result_t result; qpc_search_t search; dns_slabheader_t *header = NULL; @@ -2039,12 +2029,12 @@ findzonecut(dns_db_t *db, const dns_name_t *name, unsigned int options, * We now go looking for an NS rdataset at the node. */ - lock = &(search.qpdb->node_locks[node->locknum].lock); - NODE_RDLOCK(lock, &nlocktype); + nlock = &search.qpdb->node_locks[node->locknum]; + NODE_RDLOCK(nlock, &nlocktype); for (header = node->data; header != NULL; header = header_next) { header_next = header->next; - if (check_stale_header(node, header, &nlocktype, lock, &search, + if (check_stale_header(node, header, &nlocktype, nlock, &search, &header_prev)) { /* @@ -2057,7 +2047,7 @@ findzonecut(dns_db_t *db, const dns_name_t *name, unsigned int options, * zonecut we know about. If so, find the deepest * zonecut from this node up and return that instead. */ - NODE_UNLOCK(lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); result = find_deepest_zonecut( &search, node, nodep, foundname, rdataset, sigrdataset DNS__DB_FLARG_PASS); @@ -2094,7 +2084,7 @@ findzonecut(dns_db_t *db, const dns_name_t *name, unsigned int options, /* * No NS records here. */ - NODE_UNLOCK(lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); result = find_deepest_zonecut(&search, node, nodep, foundname, rdataset, sigrdataset DNS__DB_FLARG_PASS); @@ -2118,7 +2108,7 @@ findzonecut(dns_db_t *db, const dns_name_t *name, unsigned int options, (foundsig != NULL && need_headerupdate(foundsig, search.now))) { if (nlocktype != isc_rwlocktype_write) { - NODE_FORCEUPGRADE(lock, &nlocktype); + NODE_FORCEUPGRADE(nlock, &nlocktype); POST(nlocktype); } if (need_headerupdate(found, search.now)) { @@ -2130,7 +2120,7 @@ findzonecut(dns_db_t *db, const dns_name_t *name, unsigned int options, } } - NODE_UNLOCK(lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); tree_exit: TREE_UNLOCK(&search.qpdb->tree_lock, &tlocktype); @@ -2155,7 +2145,7 @@ findrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, dns_slabheader_t *found = NULL, *foundsig = NULL; dns_typepair_t matchtype, sigmatchtype, negtype; isc_result_t result; - isc_rwlock_t *lock = NULL; + isc_rwlock_t *nlock = NULL; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; REQUIRE(VALID_QPDB(qpdb)); @@ -2169,8 +2159,8 @@ findrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, now = isc_stdtime_now(); } - lock = &qpdb->node_locks[qpnode->locknum].lock; - NODE_RDLOCK(lock, &nlocktype); + nlock = &qpdb->node_locks[qpnode->locknum]; + NODE_RDLOCK(nlock, &nlocktype); matchtype = DNS_TYPEPAIR_VALUE(type, covers); negtype = DNS_TYPEPAIR_VALUE(0, type); @@ -2186,7 +2176,7 @@ findrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, if ((header->ttl + STALE_TTL(header, qpdb) < now - QPDB_VIRTUAL) && (nlocktype == isc_rwlocktype_write || - NODE_TRYUPGRADE(lock, &nlocktype) == + NODE_TRYUPGRADE(nlock, &nlocktype) == ISC_R_SUCCESS)) { /* @@ -2224,7 +2214,7 @@ findrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, } } - NODE_UNLOCK(lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); if (found == NULL) { return ISC_R_NOTFOUND; @@ -2315,11 +2305,12 @@ expiredata(dns_db_t *db, dns_dbnode_t *node, void *data) { dns_slabheader_t *header = data; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; isc_rwlocktype_t tlocktype = isc_rwlocktype_none; + isc_rwlock_t *nlock = &qpdb->node_locks[qpnode->locknum]; - NODE_WRLOCK(&qpdb->node_locks[qpnode->locknum].lock, &nlocktype); + NODE_WRLOCK(nlock, &nlocktype); expireheader(header, &nlocktype, &tlocktype, dns_expire_flush DNS__DB_FILELINE); - NODE_UNLOCK(&qpdb->node_locks[qpnode->locknum].lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); INSIST(tlocktype == isc_rwlocktype_none); } @@ -2396,7 +2387,8 @@ overmem(qpcache_t *qpdb, dns_slabheader_t *newheader, again: do { isc_rwlocktype_t nlocktype = isc_rwlocktype_none; - NODE_WRLOCK(&qpdb->node_locks[locknum].lock, &nlocktype); + isc_rwlock_t *nlock = &qpdb->node_locks[locknum]; + NODE_WRLOCK(nlock, &nlocktype); purged += expire_lru_headers( qpdb, locknum, &nlocktype, tlocktypep, @@ -2412,7 +2404,7 @@ again: { min_last_used = header->last_used; } - NODE_UNLOCK(&qpdb->node_locks[locknum].lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); locknum = (locknum + 1) % qpdb->node_lock_count; } while (locknum != locknum_start && purged <= purgesize); @@ -2453,7 +2445,7 @@ set_index(void *what, unsigned int idx) { } static void -free_qpdb(qpcache_t *qpdb, bool log) { +qpcache__destroy(qpcache_t *qpdb) { unsigned int i; char buf[DNS_NAME_FORMATSIZE]; dns_qp_t **treep = NULL; @@ -2474,22 +2466,19 @@ free_qpdb(qpcache_t *qpdb, bool log) { INSIST(*treep == NULL); } - if (log) { - if (dns_name_dynamic(&qpdb->common.origin)) { - dns_name_format(&qpdb->common.origin, buf, sizeof(buf)); - } else { - strlcpy(buf, "", sizeof(buf)); - } - isc_log_write(dns_lctx, DNS_LOGCATEGORY_DATABASE, - DNS_LOGMODULE_CACHE, ISC_LOG_DEBUG(1), - "done free_qpdb(%s)", buf); + if (dns_name_dynamic(&qpdb->common.origin)) { + dns_name_format(&qpdb->common.origin, buf, sizeof(buf)); + } else { + strlcpy(buf, "", sizeof(buf)); } + isc_log_write(dns_lctx, DNS_LOGCATEGORY_DATABASE, DNS_LOGMODULE_CACHE, + ISC_LOG_DEBUG(1), "done %s(%s)", __func__, buf); + if (dns_name_dynamic(&qpdb->common.origin)) { dns_name_free(&qpdb->common.origin, qpdb->common.mctx); } for (i = 0; i < qpdb->node_lock_count; i++) { - isc_refcount_destroy(&qpdb->node_locks[i].references); - NODE_DESTROYLOCK(&qpdb->node_locks[i].lock); + NODE_DESTROYLOCK(&qpdb->node_locks[i]); } /* @@ -2535,8 +2524,9 @@ free_qpdb(qpcache_t *qpdb, bool log) { } isc_mem_cput(qpdb->common.mctx, qpdb->node_locks, qpdb->node_lock_count, - sizeof(db_nodelock_t)); + sizeof(qpdb->node_locks[0])); TREE_DESTROYLOCK(&qpdb->tree_lock); + isc_refcount_destroy(&qpdb->references); isc_refcount_destroy(&qpdb->common.references); isc_rwlock_destroy(&qpdb->lock); @@ -2550,46 +2540,8 @@ free_qpdb(qpcache_t *qpdb, bool log) { static void qpdb_destroy(dns_db_t *arg) { qpcache_t *qpdb = (qpcache_t *)arg; - bool want_free = false; - unsigned int i; - unsigned int inactive = 0; - /* - * Even though there are no external direct references, there still - * may be nodes in use. - */ - for (i = 0; i < qpdb->node_lock_count; i++) { - isc_rwlocktype_t nodelock = isc_rwlocktype_none; - NODE_WRLOCK(&qpdb->node_locks[i].lock, &nodelock); - qpdb->node_locks[i].exiting = true; - if (isc_refcount_current(&qpdb->node_locks[i].references) == 0) - { - inactive++; - } - NODE_UNLOCK(&qpdb->node_locks[i].lock, &nodelock); - } - - if (inactive != 0) { - RWLOCK(&qpdb->lock, isc_rwlocktype_write); - qpdb->active -= inactive; - if (qpdb->active == 0) { - want_free = true; - } - RWUNLOCK(&qpdb->lock, isc_rwlocktype_write); - if (want_free) { - char buf[DNS_NAME_FORMATSIZE]; - if (dns_name_dynamic(&qpdb->common.origin)) { - dns_name_format(&qpdb->common.origin, buf, - sizeof(buf)); - } else { - strlcpy(buf, "", sizeof(buf)); - } - isc_log_write(dns_lctx, DNS_LOGCATEGORY_DATABASE, - DNS_LOGMODULE_CACHE, ISC_LOG_DEBUG(1), - "calling free_qpdb(%s)", buf); - free_qpdb(qpdb, true); - } - } + qpcache_detach(&qpdb); } static void @@ -2611,6 +2563,7 @@ cleanup_deadnodes(void *arg) { uint16_t locknum = isc_tid(); isc_rwlocktype_t tlocktype = isc_rwlocktype_none; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; + isc_rwlock_t *nlock = &qpdb->node_locks[locknum]; qpcnode_t *qpnode = NULL, *qpnext = NULL; isc_queue_t deadnodes; @@ -2619,14 +2572,14 @@ cleanup_deadnodes(void *arg) { isc_queue_init(&deadnodes); TREE_WRLOCK(&qpdb->tree_lock, &tlocktype); - NODE_WRLOCK(&qpdb->node_locks[locknum].lock, &nlocktype); + NODE_WRLOCK(nlock, &nlocktype); RUNTIME_CHECK(isc_queue_splice(&deadnodes, &qpdb->deadnodes[locknum])); isc_queue_for_each_entry_safe(&deadnodes, qpnode, qpnext, deadlink) { decref(qpdb, qpnode, &nlocktype, &tlocktype, false); } - NODE_UNLOCK(&qpdb->node_locks[locknum].lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); TREE_UNLOCK(&qpdb->tree_lock, &tlocktype); } @@ -2643,11 +2596,11 @@ static void reactivate_node(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t tlocktype ISC_ATTR_UNUSED DNS__DB_FLARG) { isc_rwlocktype_t nlocktype = isc_rwlocktype_none; - isc_rwlock_t *nodelock = &qpdb->node_locks[node->locknum].lock; + isc_rwlock_t *nlock = &qpdb->node_locks[node->locknum]; - NODE_RDLOCK(nodelock, &nlocktype); + NODE_RDLOCK(nlock, &nlocktype); newref(qpdb, node, nlocktype, tlocktype DNS__DB_FLARG_PASS); - NODE_UNLOCK(nodelock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); } static qpcnode_t * @@ -2723,56 +2676,31 @@ attachnode(dns_db_t *db, dns_dbnode_t *source, } static void -detachnode(dns_db_t *db, dns_dbnode_t **targetp DNS__DB_FLARG) { +detachnode(dns_db_t *db, dns_dbnode_t **nodep DNS__DB_FLARG) { qpcache_t *qpdb = (qpcache_t *)db; qpcnode_t *node = NULL; - bool want_free = false; - bool inactive = false; - db_nodelock_t *nodelock = NULL; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; isc_rwlocktype_t tlocktype = isc_rwlocktype_none; + isc_rwlock_t *nlock = NULL; REQUIRE(VALID_QPDB(qpdb)); - REQUIRE(targetp != NULL && *targetp != NULL); + REQUIRE(nodep != NULL && *nodep != NULL); - node = (qpcnode_t *)(*targetp); - nodelock = &qpdb->node_locks[node->locknum]; + node = (qpcnode_t *)(*nodep); + *nodep = NULL; + nlock = &qpdb->node_locks[node->locknum]; - NODE_RDLOCK(&nodelock->lock, &nlocktype); + /* + * We can't destroy qpcache under nodelock, so we need + * to reference it before acquiring the lock. + */ + qpcache_ref(qpdb); + NODE_RDLOCK(nlock, &nlocktype); decref(qpdb, node, &nlocktype, &tlocktype, true DNS__DB_FLARG_PASS); - if (isc_refcount_current(&nodelock->references) == 0 && - nodelock->exiting) - { - inactive = true; - } + NODE_UNLOCK(nlock, &nlocktype); - NODE_UNLOCK(&nodelock->lock, &nlocktype); - INSIST(tlocktype == isc_rwlocktype_none); - - *targetp = NULL; - - if (inactive) { - RWLOCK(&qpdb->lock, isc_rwlocktype_write); - qpdb->active--; - if (qpdb->active == 0) { - want_free = true; - } - RWUNLOCK(&qpdb->lock, isc_rwlocktype_write); - if (want_free) { - char buf[DNS_NAME_FORMATSIZE]; - if (dns_name_dynamic(&qpdb->common.origin)) { - dns_name_format(&qpdb->common.origin, buf, - sizeof(buf)); - } else { - strlcpy(buf, "", sizeof(buf)); - } - isc_log_write(dns_lctx, DNS_LOGCATEGORY_DATABASE, - DNS_LOGMODULE_CACHE, ISC_LOG_DEBUG(1), - "calling free_qpdb(%s)", buf); - free_qpdb(qpdb, true); - } - } + qpcache_detach(&qpdb); } static isc_result_t @@ -3388,6 +3316,7 @@ addrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, bool newnsec; isc_rwlocktype_t tlocktype = isc_rwlocktype_none; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; + isc_rwlock_t *nlock = NULL; bool cache_is_overmem = false; dns_fixedname_t fixed; dns_name_t *name = NULL; @@ -3499,7 +3428,9 @@ addrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, overmem(qpdb, newheader, &tlocktype DNS__DB_FLARG_PASS); } - NODE_WRLOCK(&qpdb->node_locks[qpnode->locknum].lock, &nlocktype); + nlock = &qpdb->node_locks[qpnode->locknum]; + + NODE_WRLOCK(nlock, &nlocktype); if (qpdb->rrsetstats != NULL) { DNS_SLABHEADER_SETATTR(newheader, DNS_SLABHEADERATTR_STATCOUNT); @@ -3548,7 +3479,7 @@ addrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, qpnode->delegating = 1; } - NODE_UNLOCK(&qpdb->node_locks[qpnode->locknum].lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); if (tlocktype != isc_rwlocktype_none) { TREE_UNLOCK(&qpdb->tree_lock, &tlocktype); @@ -3566,6 +3497,7 @@ deleterdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, isc_result_t result; dns_slabheader_t *newheader = NULL; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; + isc_rwlock_t *nlock = NULL; REQUIRE(VALID_QPDB(qpdb)); REQUIRE(version == NULL); @@ -3582,11 +3514,12 @@ deleterdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, setttl(newheader, 0); atomic_init(&newheader->attributes, DNS_SLABHEADERATTR_NONEXISTENT); - NODE_WRLOCK(&qpdb->node_locks[qpnode->locknum].lock, &nlocktype); + nlock = &qpdb->node_locks[qpnode->locknum]; + NODE_WRLOCK(nlock, &nlocktype); result = add(qpdb, qpnode, NULL, newheader, DNS_DBADD_FORCE, false, NULL, 0, nlocktype, isc_rwlocktype_none DNS__DB_FLARG_PASS); - NODE_UNLOCK(&qpdb->node_locks[qpnode->locknum].lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); return result; } @@ -3620,7 +3553,7 @@ locknode(dns_db_t *db, dns_dbnode_t *node, isc_rwlocktype_t type) { qpcache_t *qpdb = (qpcache_t *)db; qpcnode_t *qpnode = (qpcnode_t *)node; - RWLOCK(&qpdb->node_locks[qpnode->locknum].lock, type); + RWLOCK(&qpdb->node_locks[qpnode->locknum], type); } static void @@ -3628,7 +3561,7 @@ unlocknode(dns_db_t *db, dns_dbnode_t *node, isc_rwlocktype_t type) { qpcache_t *qpdb = (qpcache_t *)db; qpcnode_t *qpnode = (qpcnode_t *)node; - RWUNLOCK(&qpdb->node_locks[qpnode->locknum].lock, type); + RWUNLOCK(&qpdb->node_locks[qpnode->locknum], type); } isc_result_t @@ -3651,11 +3584,11 @@ dns__qpcache_create(isc_mem_t *mctx, const dns_name_t *origin, .common.origin = DNS_NAME_INITEMPTY, .common.rdclass = rdclass, .common.attributes = DNS_DBATTR_CACHE, + .common.references = 1, .loopmgr = isc_loop_getloopmgr(loop), + .references = 1, }; - isc_refcount_init(&qpdb->common.references, 1); - /* * If argv[0] exists, it points to a memory context to use for heap */ @@ -3668,7 +3601,7 @@ dns__qpcache_create(isc_mem_t *mctx, const dns_name_t *origin, qpdb->node_lock_count = isc_loopmgr_nloops(qpdb->loopmgr); qpdb->node_locks = isc_mem_cget(mctx, qpdb->node_lock_count, - sizeof(db_nodelock_t)); + sizeof(qpdb->node_locks[0])); dns_rdatasetstats_create(mctx, &qpdb->rrsetstats); qpdb->lru = isc_mem_cget(mctx, qpdb->node_lock_count, @@ -3696,12 +3629,8 @@ dns__qpcache_create(isc_mem_t *mctx, const dns_name_t *origin, isc_queue_init(&qpdb->deadnodes[i]); } - qpdb->active = qpdb->node_lock_count; - for (i = 0; i < (int)(qpdb->node_lock_count); i++) { - NODE_INITLOCK(&qpdb->node_locks[i].lock); - isc_refcount_init(&qpdb->node_locks[i].references, 0); - qpdb->node_locks[i].exiting = false; + NODE_INITLOCK(&qpdb->node_locks[i]); } /* @@ -3784,8 +3713,9 @@ rdatasetiter_first(dns_rdatasetiter_t *it DNS__DB_FLARG) { qpcnode_t *qpnode = iterator->common.node; dns_slabheader_t *header = NULL, *top_next = NULL; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; + isc_rwlock_t *nlock = &qpdb->node_locks[qpnode->locknum]; - NODE_RDLOCK(&qpdb->node_locks[qpnode->locknum].lock, &nlocktype); + NODE_RDLOCK(nlock, &nlocktype); for (header = qpnode->data; header != NULL; header = top_next) { top_next = header->next; @@ -3809,7 +3739,7 @@ rdatasetiter_first(dns_rdatasetiter_t *it DNS__DB_FLARG) { } } - NODE_UNLOCK(&qpdb->node_locks[qpnode->locknum].lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); iterator->current = header; @@ -3829,6 +3759,7 @@ rdatasetiter_next(dns_rdatasetiter_t *it DNS__DB_FLARG) { dns_typepair_t type, negtype; dns_rdatatype_t rdtype, covers; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; + isc_rwlock_t *nlock = &qpdb->node_locks[qpnode->locknum]; bool expiredok = EXPIREDOK(iterator); header = iterator->current; @@ -3836,7 +3767,7 @@ rdatasetiter_next(dns_rdatasetiter_t *it DNS__DB_FLARG) { return ISC_R_NOMORE; } - NODE_RDLOCK(&qpdb->node_locks[qpnode->locknum].lock, &nlocktype); + NODE_RDLOCK(nlock, &nlocktype); type = header->type; rdtype = DNS_TYPEPAIR_TYPE(header->type); @@ -3897,7 +3828,7 @@ rdatasetiter_next(dns_rdatasetiter_t *it DNS__DB_FLARG) { } } - NODE_UNLOCK(&qpdb->node_locks[qpnode->locknum].lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); iterator->current = header; @@ -3916,16 +3847,17 @@ rdatasetiter_current(dns_rdatasetiter_t *it, qpcnode_t *qpnode = iterator->common.node; dns_slabheader_t *header = NULL; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; + isc_rwlock_t *nlock = &qpdb->node_locks[qpnode->locknum]; header = iterator->current; REQUIRE(header != NULL); - NODE_RDLOCK(&qpdb->node_locks[qpnode->locknum].lock, &nlocktype); + NODE_RDLOCK(nlock, &nlocktype); bindrdataset(qpdb, qpnode, header, iterator->common.now, nlocktype, isc_rwlocktype_none, rdataset DNS__DB_FLARG_PASS); - NODE_UNLOCK(&qpdb->node_locks[qpnode->locknum].lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); } /* @@ -3949,7 +3881,7 @@ static void dereference_iter_node(qpc_dbit_t *qpdbiter DNS__DB_FLARG) { qpcache_t *qpdb = (qpcache_t *)qpdbiter->common.db; qpcnode_t *node = qpdbiter->node; - isc_rwlock_t *lock = NULL; + isc_rwlock_t *nlock = NULL; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; isc_rwlocktype_t tlocktype = qpdbiter->tree_locked; @@ -3959,11 +3891,11 @@ dereference_iter_node(qpc_dbit_t *qpdbiter DNS__DB_FLARG) { REQUIRE(tlocktype != isc_rwlocktype_write); - lock = &qpdb->node_locks[node->locknum].lock; - NODE_RDLOCK(lock, &nlocktype); + nlock = &qpdb->node_locks[node->locknum]; + NODE_RDLOCK(nlock, &nlocktype); decref(qpdb, node, &nlocktype, &qpdbiter->tree_locked, false DNS__DB_FLARG_PASS); - NODE_UNLOCK(lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); INSIST(qpdbiter->tree_locked == tlocktype); @@ -4398,3 +4330,9 @@ ISC_REFCOUNT_STATIC_TRACE_IMPL(qpcnode, qpcnode_destroy); #else ISC_REFCOUNT_STATIC_IMPL(qpcnode, qpcnode_destroy); #endif + +#ifdef DNS_DB_NODETRACE +ISC_REFCOUNT_STATIC_TRACE_IMPL(qpcache, qpcache__destroy); +#else +ISC_REFCOUNT_STATIC_IMPL(qpcache, qpcache__destroy); +#endif diff --git a/lib/dns/qpzone.c b/lib/dns/qpzone.c index 20f0efdf66..dc59100d61 100644 --- a/lib/dns/qpzone.c +++ b/lib/dns/qpzone.c @@ -168,7 +168,9 @@ struct qpzonedb { isc_rwlock_t lock; /* Locks for tree nodes */ int node_lock_count; - db_nodelock_t *node_locks; + isc_rwlock_t *node_locks; + isc_refcount_t references; + qpznode_t *origin; qpznode_t *nsec3_origin; isc_stats_t *gluecachestats; @@ -193,6 +195,18 @@ struct qpzonedb { dns_qpmulti_t *nsec3; /* NSEC3 nodes only */ }; +#ifdef DNS_DB_NODETRACE +#define qpzonedb_ref(ptr) qpzonedb__ref(ptr, __func__, __FILE__, __LINE__) +#define qpzonedb_unref(ptr) qpzonedb_unref(ptr, __func__, __FILE__, __LINE__) +#define qpzonedb_attach(ptr, ptrp) \ + qpzonedb__attach(ptr, ptrp, __func__, __FILE__, __LINE__) +#define qpzonedb_detach(ptrp) \ + qpzonedb__detach(ptrp, __func__, __FILE__, __LINE__) +ISC_REFCOUNT_STATIC_TRACE_DECL(qpzonedb); +#else +ISC_REFCOUNT_STATIC_DECL(qpzonedb); +#endif + /*% * Search Context */ @@ -377,8 +391,7 @@ free_db_rcu(struct rcu_head *rcu_head) { dns_name_free(&qpdb->common.origin, qpdb->common.mctx); } for (int i = 0; i < qpdb->node_lock_count; i++) { - isc_refcount_destroy(&qpdb->node_locks[i].references); - NODE_DESTROYLOCK(&qpdb->node_locks[i].lock); + NODE_DESTROYLOCK(&qpdb->node_locks[i]); } isc_heap_destroy(&qpdb->heap); @@ -388,13 +401,15 @@ free_db_rcu(struct rcu_head *rcu_head) { } isc_mem_cput(qpdb->common.mctx, qpdb->node_locks, qpdb->node_lock_count, - sizeof(db_nodelock_t)); - isc_refcount_destroy(&qpdb->common.references); + sizeof(qpdb->node_locks[0])); if (qpdb->loop != NULL) { isc_loop_detach(&qpdb->loop); } isc_rwlock_destroy(&qpdb->lock); + isc_refcount_destroy(&qpdb->references); + isc_refcount_destroy(&qpdb->common.references); + qpdb->common.magic = 0; qpdb->common.impmagic = 0; @@ -406,7 +421,7 @@ free_db_rcu(struct rcu_head *rcu_head) { } static void -free_qpdb(qpzonedb_t *qpdb, bool log) { +qpzone_destroy(qpzonedb_t *qpdb) { REQUIRE(qpdb->future_version == NULL); isc_refcount_decrementz(&qpdb->current_version->references); @@ -422,17 +437,14 @@ free_qpdb(qpzonedb_t *qpdb, bool log) { dns_qpmulti_destroy(&qpdb->nsec); dns_qpmulti_destroy(&qpdb->nsec3); - if (log) { - char buf[DNS_NAME_FORMATSIZE]; - if (dns_name_dynamic(&qpdb->common.origin)) { - dns_name_format(&qpdb->common.origin, buf, sizeof(buf)); - } else { - strlcpy(buf, "", sizeof(buf)); - } - isc_log_write(dns_lctx, DNS_LOGCATEGORY_DATABASE, - DNS_LOGMODULE_DB, ISC_LOG_DEBUG(1), - "called free_qpdb(%s)", buf); + char buf[DNS_NAME_FORMATSIZE]; + if (dns_name_dynamic(&qpdb->common.origin)) { + dns_name_format(&qpdb->common.origin, buf, sizeof(buf)); + } else { + strlcpy(buf, "", sizeof(buf)); } + isc_log_write(dns_lctx, DNS_LOGCATEGORY_DATABASE, DNS_LOGMODULE_DB, + ISC_LOG_DEBUG(1), "called %s(%s)", __func__, buf); call_rcu(&qpdb->rcu_head, free_db_rcu); } @@ -440,7 +452,6 @@ free_qpdb(qpzonedb_t *qpdb, bool log) { static void qpdb_destroy(dns_db_t *arg) { qpzonedb_t *qpdb = (qpzonedb_t *)arg; - unsigned int inactive = 0; if (qpdb->origin != NULL) { qpznode_detach(&qpdb->origin); @@ -458,45 +469,7 @@ qpdb_destroy(dns_db_t *arg) { dns__db_cleanup_gluelists(&qpdb->current_version->glue_stack); } - /* - * Even though there are no external direct references, there still - * may be nodes in use. - */ - for (int i = 0; i < qpdb->node_lock_count; i++) { - isc_rwlocktype_t nodelock = isc_rwlocktype_none; - NODE_WRLOCK(&qpdb->node_locks[i].lock, &nodelock); - qpdb->node_locks[i].exiting = true; - if (isc_refcount_current(&qpdb->node_locks[i].references) == 0) - { - inactive++; - } - NODE_UNLOCK(&qpdb->node_locks[i].lock, &nodelock); - } - - if (inactive != 0) { - bool want_free = false; - - RWLOCK(&qpdb->lock, isc_rwlocktype_write); - qpdb->active -= inactive; - if (qpdb->active == 0) { - want_free = true; - } - RWUNLOCK(&qpdb->lock, isc_rwlocktype_write); - - if (want_free) { - char buf[DNS_NAME_FORMATSIZE]; - if (dns_name_dynamic(&qpdb->common.origin)) { - dns_name_format(&qpdb->common.origin, buf, - sizeof(buf)); - } else { - strlcpy(buf, "", sizeof(buf)); - } - isc_log_write(dns_lctx, DNS_LOGCATEGORY_DATABASE, - DNS_LOGMODULE_DB, ISC_LOG_DEBUG(1), - "calling free_qpdb(%s)", buf); - free_qpdb(qpdb, true); - } - } + qpzonedb_detach(&qpdb); } static qpznode_t * @@ -527,11 +500,11 @@ allocate_version(isc_mem_t *mctx, uint32_t serial, unsigned int references, .changed_list = ISC_LIST_INITIALIZER, .resigned_list = ISC_LIST_INITIALIZER, .link = ISC_LINK_INITIALIZER, + .references = ISC_REFCOUNT_INITIALIZER(references), }; cds_wfs_init(&version->glue_stack); isc_rwlock_init(&version->rwlock); - isc_refcount_init(&version->references, references); return version; } @@ -549,15 +522,15 @@ dns__qpzone_create(isc_mem_t *mctx, const dns_name_t *origin, dns_dbtype_t type, *qpdb = (qpzonedb_t){ .common.origin = DNS_NAME_INITEMPTY, .common.rdclass = rdclass, + .common.references = ISC_REFCOUNT_INITIALIZER(1), .node_lock_count = DEFAULT_NODE_LOCK_COUNT, .current_serial = 1, .least_serial = 1, .next_serial = 2, .open_versions = ISC_LIST_INITIALIZER, + .references = ISC_REFCOUNT_INITIALIZER(1), }; - isc_refcount_init(&qpdb->common.references, 1); - qpdb->common.methods = &qpdb_zonemethods; if (type == dns_dbtype_stub) { qpdb->common.attributes |= DNS_DBATTR_STUB; @@ -566,7 +539,7 @@ dns__qpzone_create(isc_mem_t *mctx, const dns_name_t *origin, dns_dbtype_t type, isc_rwlock_init(&qpdb->lock); qpdb->node_locks = isc_mem_cget(mctx, qpdb->node_lock_count, - sizeof(db_nodelock_t)); + sizeof(qpdb->node_locks[0])); qpdb->common.update_listeners = cds_lfht_new(16, 16, 0, 0, NULL); @@ -575,9 +548,7 @@ dns__qpzone_create(isc_mem_t *mctx, const dns_name_t *origin, dns_dbtype_t type, qpdb->active = qpdb->node_lock_count; for (int i = 0; i < qpdb->node_lock_count; i++) { - NODE_INITLOCK(&qpdb->node_locks[i].lock); - isc_refcount_init(&qpdb->node_locks[i].references, 0); - qpdb->node_locks[i].exiting = false; + NODE_INITLOCK(&qpdb->node_locks[i]); } /* @@ -622,7 +593,7 @@ dns__qpzone_create(isc_mem_t *mctx, const dns_name_t *origin, dns_dbtype_t type, if (result != ISC_R_SUCCESS) { INSIST(result != ISC_R_EXISTS); - free_qpdb(qpdb, false); + qpzonedb_detach(&qpdb); return result; } @@ -639,7 +610,7 @@ dns__qpzone_create(isc_mem_t *mctx, const dns_name_t *origin, dns_dbtype_t type, if (result != ISC_R_SUCCESS) { INSIST(result != ISC_R_EXISTS); - free_qpdb(qpdb, false); + qpzonedb_detach(&qpdb); return result; } @@ -669,18 +640,7 @@ qpznode_newref(qpzonedb_t *qpdb, qpznode_t *node DNS__DB_FLARG) { return; } - /* this is the first reference to the node */ - refs = isc_refcount_increment0( - &qpdb->node_locks[node->locknum].references); -#if DNS_DB_NODETRACE - fprintf(stderr, - "incr:nodelock:%s:%s:%u:%p:%p->references = " - "%" PRIuFAST32 "\n", - func, file, line, node, &qpdb->node_locks[node->locknum], - refs + 1); -#else - UNUSED(refs); -#endif + qpzonedb_ref(qpdb); } static void @@ -818,17 +778,7 @@ qpznode_decref(qpzonedb_t *qpdb, qpznode_t *node DNS__DB_FLARG) { return false; } - refs = isc_refcount_decrement( - &qpdb->node_locks[node->locknum].references); -#if DNS_DB_NODETRACE - fprintf(stderr, - "decr:nodelock:%s:%s:%u:%p:%p->references = " - "%" PRIuFAST32 "\n", - func, file, line, node, &qpdb->node_locks[node->locknum], - refs - 1); -#else - UNUSED(refs); -#endif + qpzonedb_unref(qpdb); return true; } @@ -873,8 +823,8 @@ decref(qpzonedb_t *qpdb, qpznode_t *node, uint32_t least_serial, /* Upgrade the lock? */ if (*nlocktypep == isc_rwlocktype_read) { - NODE_FORCEUPGRADE(&qpdb->node_locks[node->locknum].lock, - nlocktypep); + isc_rwlock_t *nlock = &qpdb->node_locks[node->locknum]; + NODE_FORCEUPGRADE(nlock, nlocktypep); } if (!qpznode_decref(qpdb, node DNS__DB_FLARG_PASS)) { @@ -963,10 +913,12 @@ setnsec3parameters(dns_db_t *db, qpz_version_t *version) { unsigned int count, length; qpzonedb_t *qpdb = (qpzonedb_t *)db; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; + isc_rwlock_t *nlock = NULL; version->havensec3 = false; node = qpdb->origin; - NODE_RDLOCK(&(qpdb->node_locks[node->locknum].lock), &nlocktype); + nlock = &qpdb->node_locks[node->locknum]; + NODE_RDLOCK(nlock, &nlocktype); for (header = node->data; header != NULL; header = header_next) { header_next = header->next; do { @@ -1033,7 +985,7 @@ setnsec3parameters(dns_db_t *db, qpz_version_t *version) { } } unlock: - NODE_UNLOCK(&(qpdb->node_locks[node->locknum].lock), &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); } static void @@ -1434,19 +1386,19 @@ closeversion(dns_db_t *db, dns_dbversion_t **versionp, for (header = HEAD(resigned_list); header != NULL; header = HEAD(resigned_list)) { - isc_rwlock_t *lock = NULL; + isc_rwlock_t *nlock = NULL; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; ISC_LIST_UNLINK(resigned_list, header, link); - lock = &qpdb->node_locks[HEADERNODE(header)->locknum].lock; - NODE_WRLOCK(lock, &nlocktype); + nlock = &qpdb->node_locks[HEADERNODE(header)->locknum]; + NODE_WRLOCK(nlock, &nlocktype); if (rollback && !IGNORE(header)) { resigninsert(qpdb, header); } decref(qpdb, HEADERNODE(header), least_serial, &nlocktype DNS__DB_FLARG_PASS); - NODE_UNLOCK(lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); } if (EMPTY(cleanup_list)) { @@ -1457,20 +1409,20 @@ closeversion(dns_db_t *db, dns_dbversion_t **versionp, for (changed = HEAD(cleanup_list); changed != NULL; changed = next_changed) { - isc_rwlock_t *lock = NULL; + isc_rwlock_t *nlock = NULL; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; next_changed = NEXT(changed, link); node = changed->node; - lock = &qpdb->node_locks[node->locknum].lock; + nlock = &qpdb->node_locks[node->locknum]; - NODE_WRLOCK(lock, &nlocktype); + NODE_WRLOCK(nlock, &nlocktype); if (rollback) { rollback_node(node, serial); } decref(qpdb, node, least_serial, &nlocktype DNS__DB_FILELINE); - NODE_UNLOCK(lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); isc_mem_put(qpdb->common.mctx, changed, sizeof(*changed)); } @@ -1492,6 +1444,7 @@ findrdataset(dns_db_t *db, dns_dbnode_t *dbnode, dns_dbversion_t *dbversion, bool close_version = false; dns_typepair_t matchtype, sigmatchtype; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; + isc_rwlock_t *nlock = NULL; REQUIRE(VALID_QPZONE(qpdb)); REQUIRE(type != dns_rdatatype_any); @@ -1503,7 +1456,8 @@ findrdataset(dns_db_t *db, dns_dbnode_t *dbnode, dns_dbversion_t *dbversion, } serial = version->serial; - NODE_RDLOCK(&qpdb->node_locks[node->locknum].lock, &nlocktype); + nlock = &qpdb->node_locks[node->locknum]; + NODE_RDLOCK(nlock, &nlocktype); matchtype = DNS_TYPEPAIR_VALUE(type, covers); if (covers == 0) { @@ -1550,7 +1504,7 @@ findrdataset(dns_db_t *db, dns_dbnode_t *dbnode, dns_dbversion_t *dbversion, } } - NODE_UNLOCK(&qpdb->node_locks[node->locknum].lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); if (close_version) { closeversion(db, (dns_dbversion_t **)&version, @@ -2049,6 +2003,7 @@ loading_addrdataset(void *arg, const dns_name_t *name, isc_region_t region; dns_slabheader_t *newheader = NULL; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; + isc_rwlock_t *nlock = NULL; REQUIRE(rdataset->rdclass == qpdb->common.rdclass); @@ -2119,10 +2074,11 @@ loading_addrdataset(void *arg, const dns_name_t *name, newheader->resign_lsb = rdataset->resign & 0x1; } - NODE_WRLOCK(&qpdb->node_locks[node->locknum].lock, &nlocktype); + nlock = &qpdb->node_locks[node->locknum]; + NODE_WRLOCK(nlock, &nlocktype); result = add(qpdb, node, name, qpdb->current_version, newheader, DNS_DBADD_MERGE, true, NULL, 0 DNS__DB_FLARG_PASS); - NODE_UNLOCK(&qpdb->node_locks[node->locknum].lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); if (result == ISC_R_SUCCESS && delegating_type(qpdb, node, rdataset->type)) @@ -2319,6 +2275,7 @@ setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, isc_stdtime_t resign) { qpzonedb_t *qpdb = (qpzonedb_t *)db; dns_slabheader_t *header = NULL, oldheader; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; + isc_rwlock_t *nlock = NULL; REQUIRE(VALID_QPZONE(qpdb)); REQUIRE(rdataset != NULL); @@ -2326,8 +2283,8 @@ setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, isc_stdtime_t resign) { header = dns_slabheader_fromrdataset(rdataset); - NODE_WRLOCK(&qpdb->node_locks[HEADERNODE(header)->locknum].lock, - &nlocktype); + nlock = &qpdb->node_locks[HEADERNODE(header)->locknum]; + NODE_WRLOCK(nlock, &nlocktype); oldheader = *header; @@ -2358,8 +2315,7 @@ setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, isc_stdtime_t resign) { DNS_SLABHEADER_SETATTR(header, DNS_SLABHEADERATTR_RESIGN); resigninsert(qpdb, header); } - NODE_UNLOCK(&qpdb->node_locks[HEADERNODE(header)->locknum].lock, - &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); return ISC_R_SUCCESS; } @@ -2369,6 +2325,7 @@ getsigningtime(dns_db_t *db, isc_stdtime_t *resign, dns_name_t *foundname, qpzonedb_t *qpdb = (qpzonedb_t *)db; dns_slabheader_t *header = NULL; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; + isc_rwlock_t *nlock = NULL; uint16_t locknum; isc_result_t result = ISC_R_NOTFOUND; @@ -2387,13 +2344,15 @@ getsigningtime(dns_db_t *db, isc_stdtime_t *resign, dns_name_t *foundname, RWUNLOCK(&qpdb->lock, isc_rwlocktype_read); again: - NODE_RDLOCK(&qpdb->node_locks[locknum].lock, &nlocktype); + nlock = &qpdb->node_locks[locknum]; + + NODE_RDLOCK(nlock, &nlocktype); RWLOCK(&qpdb->lock, isc_rwlocktype_read); header = isc_heap_element(qpdb->heap, 1); if (header != NULL && HEADERNODE(header)->locknum != locknum) { RWUNLOCK(&qpdb->lock, isc_rwlocktype_read); - NODE_UNLOCK(&qpdb->node_locks[locknum].lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); locknum = HEADERNODE(header)->locknum; goto again; } @@ -2407,7 +2366,7 @@ again: result = ISC_R_SUCCESS; } RWUNLOCK(&qpdb->lock, isc_rwlocktype_read); - NODE_UNLOCK(&qpdb->node_locks[locknum].lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); return result; } @@ -2580,8 +2539,8 @@ setup_delegation(qpz_search_t *search, dns_dbnode_t **nodep, } if (rdataset != NULL) { isc_rwlocktype_t nlocktype = isc_rwlocktype_none; - NODE_RDLOCK(&(search->qpdb->node_locks[node->locknum].lock), - &nlocktype); + isc_rwlock_t *nlock = &search->qpdb->node_locks[node->locknum]; + NODE_RDLOCK(nlock, &nlocktype); bindrdataset(search->qpdb, node, search->zonecut_header, search->now, rdataset DNS__DB_FLARG_PASS); if (sigrdataset != NULL && search->zonecut_sigheader != NULL) { @@ -2589,8 +2548,7 @@ setup_delegation(qpz_search_t *search, dns_dbnode_t **nodep, search->zonecut_sigheader, search->now, sigrdataset DNS__DB_FLARG_PASS); } - NODE_UNLOCK(&(search->qpdb->node_locks[node->locknum].lock), - &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); } if (type == dns_rdatatype_dname) { @@ -2621,10 +2579,10 @@ step(qpz_search_t *search, dns_qpiter_t *it, direction_t direction, result = dns_qpiter_current(it, nodename, (void **)&node, NULL); while (result == ISC_R_SUCCESS) { - isc_rwlock_t *nodelock = &qpdb->node_locks[node->locknum].lock; + isc_rwlock_t *nlock = &qpdb->node_locks[node->locknum]; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; - NODE_RDLOCK(nodelock, &nlocktype); + NODE_RDLOCK(nlock, &nlocktype); for (header = node->data; header != NULL; header = header->next) { if (header->serial <= search->serial && @@ -2633,7 +2591,7 @@ step(qpz_search_t *search, dns_qpiter_t *it, direction_t direction, break; } } - NODE_UNLOCK(nodelock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); if (header != NULL) { break; } @@ -2768,14 +2726,14 @@ find_wildcard(qpz_search_t *search, qpznode_t **nodep, */ for (int i = dns_qpchain_length(&search->chain) - 1; i >= 0; i--) { qpznode_t *node = NULL; - isc_rwlock_t *lock = NULL; + isc_rwlock_t *nlock = NULL; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; bool wild, active; dns_qpchain_node(&search->chain, i, NULL, (void **)&node, NULL); - lock = &qpdb->node_locks[node->locknum].lock; - NODE_RDLOCK(lock, &nlocktype); + nlock = &qpdb->node_locks[node->locknum]; + NODE_RDLOCK(nlock, &nlocktype); /* * First we try to figure out if this node is active in * the search's version. We do this now, even though we @@ -2793,7 +2751,7 @@ find_wildcard(qpz_search_t *search, qpznode_t **nodep, active = (header != NULL); wild = node->wild; - NODE_UNLOCK(lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); if (wild) { qpznode_t *wnode = NULL; @@ -2818,8 +2776,8 @@ find_wildcard(qpz_search_t *search, qpznode_t **nodep, * is active in the search's version, we're * done. */ - lock = &qpdb->node_locks[wnode->locknum].lock; - NODE_RDLOCK(lock, &nlocktype); + nlock = &qpdb->node_locks[wnode->locknum]; + NODE_RDLOCK(nlock, &nlocktype); for (header = wnode->data; header != NULL; header = header->next) { @@ -2830,7 +2788,7 @@ find_wildcard(qpz_search_t *search, qpznode_t **nodep, break; } } - NODE_UNLOCK(lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); if (header != NULL || activeempty(search, &wit, wname)) { @@ -3001,8 +2959,8 @@ again: do { dns_slabheader_t *found = NULL, *foundsig = NULL; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; - NODE_RDLOCK(&(search->qpdb->node_locks[node->locknum].lock), - &nlocktype); + isc_rwlock_t *nlock = &search->qpdb->node_locks[node->locknum]; + NODE_RDLOCK(nlock, &nlocktype); empty_node = true; for (header = node->data; header != NULL; header = header_next) { @@ -3108,8 +3066,7 @@ again: &prevnode, &nseciter, &first); } - NODE_UNLOCK(&(search->qpdb->node_locks[node->locknum].lock), - &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); node = prevnode; prevnode = NULL; } while (empty_node && result == ISC_R_SUCCESS); @@ -3144,9 +3101,9 @@ check_zonecut(qpznode_t *node, void *arg DNS__DB_FLARG) { dns_slabheader_t *found = NULL; isc_result_t result = DNS_R_CONTINUE; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; + isc_rwlock_t *nlock = &search->qpdb->node_locks[node->locknum]; - NODE_RDLOCK(&(search->qpdb->node_locks[node->locknum].lock), - &nlocktype); + NODE_RDLOCK(nlock, &nlocktype); /* * Look for an NS or DNAME rdataset active in our version. @@ -3256,8 +3213,7 @@ check_zonecut(qpznode_t *node, void *arg DNS__DB_FLARG) { } } - NODE_UNLOCK(&(search->qpdb->node_locks[node->locknum].lock), - &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); return result; } @@ -3281,7 +3237,7 @@ find(dns_db_t *db, const dns_name_t *name, dns_dbversion_t *version, dns_slabheader_t *foundsig = NULL, *cnamesig = NULL, *nsecsig = NULL; dns_typepair_t sigtype; bool active; - isc_rwlock_t *lock = NULL; + isc_rwlock_t *nlock = NULL; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; REQUIRE(VALID_QPZONE((qpzonedb_t *)db)); @@ -3413,8 +3369,8 @@ found: * have matched a wildcard. */ - lock = &search.qpdb->node_locks[node->locknum].lock; - NODE_RDLOCK(lock, &nlocktype); + nlock = &search.qpdb->node_locks[node->locknum]; + NODE_RDLOCK(nlock, &nlocktype); if (search.zonecut != NULL) { /* @@ -3527,7 +3483,7 @@ found: if (header->type == dns_rdatatype_nsec3 && !matchparams(header, &search)) { - NODE_UNLOCK(lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); goto partial_match; } /* @@ -3616,7 +3572,7 @@ found: * we really have a partial match. */ if (!wild) { - NODE_UNLOCK(lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); goto partial_match; } } @@ -3632,7 +3588,7 @@ found: * * Return the delegation. */ - NODE_UNLOCK(lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); result = setup_delegation( &search, nodep, foundname, rdataset, sigrdataset DNS__DB_FLARG_PASS); @@ -3654,7 +3610,7 @@ found: goto node_exit; } - NODE_UNLOCK(lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); result = find_closest_nsec( &search, nodep, foundname, rdataset, sigrdataset, false, @@ -3750,7 +3706,7 @@ found: } node_exit: - NODE_UNLOCK(lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); tree_exit: if (nsec3) { @@ -3766,11 +3722,11 @@ tree_exit: if (search.need_cleanup) { node = search.zonecut; INSIST(node != NULL); - lock = &(search.qpdb->node_locks[node->locknum].lock); + nlock = &search.qpdb->node_locks[node->locknum]; - NODE_RDLOCK(lock, &nlocktype); + NODE_RDLOCK(nlock, &nlocktype); decref(search.qpdb, node, 0, &nlocktype DNS__DB_FLARG_PASS); - NODE_UNLOCK(lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); } if (close_version) { @@ -3829,52 +3785,26 @@ attachnode(dns_db_t *db, dns_dbnode_t *source, } static void -detachnode(dns_db_t *db, dns_dbnode_t **targetp DNS__DB_FLARG) { +detachnode(dns_db_t *db, dns_dbnode_t **nodep DNS__DB_FLARG) { qpzonedb_t *qpdb = (qpzonedb_t *)db; qpznode_t *node = NULL; - bool want_free = false; - bool inactive = false; - db_nodelock_t *nodelock = NULL; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; + isc_rwlock_t *nlock = NULL; REQUIRE(VALID_QPZONE(qpdb)); - REQUIRE(targetp != NULL && *targetp != NULL); + REQUIRE(nodep != NULL && *nodep != NULL); - node = (qpznode_t *)(*targetp); - nodelock = &qpdb->node_locks[node->locknum]; + node = (qpznode_t *)(*nodep); + *nodep = NULL; + nlock = &qpdb->node_locks[node->locknum]; - NODE_RDLOCK(&nodelock->lock, &nlocktype); + qpzonedb_ref(qpdb); + + NODE_RDLOCK(nlock, &nlocktype); decref(qpdb, node, 0, &nlocktype DNS__DB_FLARG_PASS); - if (isc_refcount_current(&nodelock->references) == 0 && - nodelock->exiting) - { - inactive = true; - } - NODE_UNLOCK(&nodelock->lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); - *targetp = NULL; - - if (inactive) { - RWLOCK(&qpdb->lock, isc_rwlocktype_write); - qpdb->active--; - if (qpdb->active == 0) { - want_free = true; - } - RWUNLOCK(&qpdb->lock, isc_rwlocktype_write); - if (want_free) { - char buf[DNS_NAME_FORMATSIZE]; - if (dns_name_dynamic(&qpdb->common.origin)) { - dns_name_format(&qpdb->common.origin, buf, - sizeof(buf)); - } else { - strlcpy(buf, "", sizeof(buf)); - } - isc_log_write(dns_lctx, DNS_LOGCATEGORY_DATABASE, - DNS_LOGMODULE_DB, ISC_LOG_DEBUG(1), - "calling free_qpdb(%s)", buf); - free_qpdb(qpdb, true); - } - } + qpzonedb_detach(&qpdb); } static unsigned int @@ -3943,7 +3873,7 @@ locknode(dns_db_t *db, dns_dbnode_t *dbnode, isc_rwlocktype_t type) { qpzonedb_t *qpdb = (qpzonedb_t *)db; qpznode_t *node = (qpznode_t *)dbnode; - RWLOCK(&qpdb->node_locks[node->locknum].lock, type); + RWLOCK(&qpdb->node_locks[node->locknum], type); } static void @@ -3951,7 +3881,7 @@ unlocknode(dns_db_t *db, dns_dbnode_t *dbnode, isc_rwlocktype_t type) { qpzonedb_t *qpdb = (qpzonedb_t *)db; qpznode_t *node = (qpznode_t *)dbnode; - RWUNLOCK(&qpdb->node_locks[node->locknum].lock, type); + RWUNLOCK(&qpdb->node_locks[node->locknum], type); } static void @@ -3997,8 +3927,9 @@ rdatasetiter_first(dns_rdatasetiter_t *iterator DNS__DB_FLARG) { qpz_version_t *version = qrditer->common.version; dns_slabheader_t *header = NULL, *top_next = NULL; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; + isc_rwlock_t *nlock = &qpdb->node_locks[node->locknum]; - NODE_RDLOCK(&qpdb->node_locks[node->locknum].lock, &nlocktype); + NODE_RDLOCK(nlock, &nlocktype); for (header = node->data; header != NULL; header = top_next) { top_next = header->next; @@ -4019,7 +3950,7 @@ rdatasetiter_first(dns_rdatasetiter_t *iterator DNS__DB_FLARG) { } } - NODE_UNLOCK(&qpdb->node_locks[node->locknum].lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); qrditer->current = header; @@ -4040,13 +3971,14 @@ rdatasetiter_next(dns_rdatasetiter_t *iterator DNS__DB_FLARG) { dns_typepair_t type, negtype; dns_rdatatype_t rdtype; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; + isc_rwlock_t *nlock = &qpdb->node_locks[node->locknum]; header = qrditer->current; if (header == NULL) { return ISC_R_NOMORE; } - NODE_RDLOCK(&qpdb->node_locks[node->locknum].lock, &nlocktype); + NODE_RDLOCK(nlock, &nlocktype); type = header->type; rdtype = DNS_TYPEPAIR_TYPE(header->type); @@ -4090,7 +4022,7 @@ rdatasetiter_next(dns_rdatasetiter_t *iterator DNS__DB_FLARG) { } } - NODE_UNLOCK(&qpdb->node_locks[node->locknum].lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); qrditer->current = header; @@ -4109,16 +4041,17 @@ rdatasetiter_current(dns_rdatasetiter_t *iterator, qpznode_t *node = qrditer->common.node; dns_slabheader_t *header = NULL; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; + isc_rwlock_t *nlock = &qpdb->node_locks[node->locknum]; header = qrditer->current; REQUIRE(header != NULL); - NODE_RDLOCK(&qpdb->node_locks[node->locknum].lock, &nlocktype); + NODE_RDLOCK(nlock, &nlocktype); bindrdataset(qpdb, node, header, qrditer->common.now, rdataset DNS__DB_FLARG_PASS); - NODE_UNLOCK(&qpdb->node_locks[node->locknum].lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); } /* @@ -4140,18 +4073,19 @@ static void dereference_iter_node(qpdb_dbiterator_t *iter DNS__DB_FLARG) { qpzonedb_t *qpdb = (qpzonedb_t *)iter->common.db; qpznode_t *node = iter->node; - isc_rwlock_t *lock = NULL; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; + isc_rwlock_t *nlock = NULL; if (node == NULL) { return; } - lock = &qpdb->node_locks[node->locknum].lock; - NODE_RDLOCK(lock, &nlocktype); - decref(qpdb, node, 0, &nlocktype DNS__DB_FLARG_PASS); - NODE_UNLOCK(lock, &nlocktype); iter->node = NULL; + nlock = &qpdb->node_locks[node->locknum]; + + NODE_RDLOCK(nlock, &nlocktype); + decref(qpdb, node, 0, &nlocktype DNS__DB_FLARG_PASS); + NODE_UNLOCK(nlock, &nlocktype); } static void @@ -4568,6 +4502,7 @@ addrdataset(dns_db_t *db, dns_dbnode_t *dbnode, dns_dbversion_t *dbversion, isc_region_t region; dns_slabheader_t *newheader = NULL; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; + isc_rwlock_t *nlock = NULL; dns_fixedname_t fn; dns_name_t *name = dns_fixedname_initname(&fn); dns_qp_t *nsec = NULL; @@ -4646,7 +4581,9 @@ addrdataset(dns_db_t *db, dns_dbnode_t *dbnode, dns_dbversion_t *dbversion, * (Note: node lock must be acquired after starting * the QPDB transaction and released before committing.) */ - NODE_WRLOCK(&qpdb->node_locks[node->locknum].lock, &nlocktype); + nlock = &qpdb->node_locks[node->locknum]; + + NODE_WRLOCK(nlock, &nlocktype); result = ISC_R_SUCCESS; if (nsec != NULL) { @@ -4677,7 +4614,7 @@ addrdataset(dns_db_t *db, dns_dbnode_t *dbnode, dns_dbversion_t *dbversion, node->delegating = true; } - NODE_UNLOCK(&qpdb->node_locks[node->locknum].lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); if (nsec != NULL) { dns_qpmulti_commit(qpdb->nsec, &nsec); @@ -4702,6 +4639,7 @@ subtractrdataset(dns_db_t *db, dns_dbnode_t *dbnode, dns_dbversion_t *dbversion, isc_result_t result; qpz_changed_t *changed = NULL; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; + isc_rwlock_t *nlock; REQUIRE(VALID_QPZONE(qpdb)); REQUIRE(version != NULL && version->qpdb == qpdb); @@ -4746,7 +4684,8 @@ subtractrdataset(dns_db_t *db, dns_dbnode_t *dbnode, dns_dbversion_t *dbversion, newheader->resign_lsb = 0; } - NODE_WRLOCK(&qpdb->node_locks[node->locknum].lock, &nlocktype); + nlock = &qpdb->node_locks[node->locknum]; + NODE_WRLOCK(nlock, &nlocktype); changed = add_changed(newheader, version DNS__DB_FLARG_PASS); for (topheader = node->data; topheader != NULL; @@ -4872,7 +4811,7 @@ subtractrdataset(dns_db_t *db, dns_dbnode_t *dbnode, dns_dbversion_t *dbversion, } unlock: - NODE_UNLOCK(&qpdb->node_locks[node->locknum].lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); return result; } @@ -4887,6 +4826,7 @@ deleterdataset(dns_db_t *db, dns_dbnode_t *dbnode, dns_dbversion_t *dbversion, isc_result_t result; dns_slabheader_t *newheader = NULL; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; + isc_rwlock_t *nlock = NULL; REQUIRE(VALID_QPZONE(qpdb)); REQUIRE(version != NULL && version->qpdb == qpdb); @@ -4906,10 +4846,11 @@ deleterdataset(dns_db_t *db, dns_dbnode_t *dbnode, dns_dbversion_t *dbversion, dns_name_copy(&node->name, nodename); - NODE_WRLOCK(&qpdb->node_locks[node->locknum].lock, &nlocktype); + nlock = &qpdb->node_locks[node->locknum]; + NODE_WRLOCK(nlock, &nlocktype); result = add(qpdb, node, nodename, version, newheader, DNS_DBADD_FORCE, false, NULL, 0 DNS__DB_FLARG_PASS); - NODE_UNLOCK(&qpdb->node_locks[node->locknum].lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); return result; } @@ -4918,14 +4859,18 @@ nodefullname(dns_db_t *db, dns_dbnode_t *node, dns_name_t *name) { qpzonedb_t *qpdb = (qpzonedb_t *)db; qpznode_t *qpnode = (qpznode_t *)node; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; + isc_rwlock_t *nlock = NULL; REQUIRE(VALID_QPZONE(qpdb)); REQUIRE(node != NULL); REQUIRE(name != NULL); - NODE_RDLOCK(&qpdb->node_locks[qpnode->locknum].lock, &nlocktype); + nlock = &qpdb->node_locks[qpnode->locknum]; + + NODE_RDLOCK(nlock, &nlocktype); dns_name_copy(&qpnode->name, name); - NODE_UNLOCK(&qpdb->node_locks[qpnode->locknum].lock, &nlocktype); + NODE_UNLOCK(nlock, &nlocktype); + return ISC_R_SUCCESS; } @@ -5165,6 +5110,12 @@ ISC_REFCOUNT_STATIC_TRACE_IMPL(qpznode, destroy_qpznode); ISC_REFCOUNT_STATIC_IMPL(qpznode, destroy_qpznode); #endif +#ifdef DNS_DB_NODETRACE +ISC_REFCOUNT_STATIC_TRACE_IMPL(qpzonedb, qpzone_destroy); +#else +ISC_REFCOUNT_STATIC_IMPL(qpzonedb, qpzone_destroy); +#endif + static void qp_attach(void *uctx ISC_ATTR_UNUSED, void *pval, uint32_t ival ISC_ATTR_UNUSED) { diff --git a/tests/dns/qpzone_test.c b/tests/dns/qpzone_test.c index a2d00814a3..eca1a70ed1 100644 --- a/tests/dns/qpzone_test.c +++ b/tests/dns/qpzone_test.c @@ -101,7 +101,7 @@ const char *ownercase_vectors[12][2] = { static bool ownercase_test_one(const char *str1, const char *str2) { isc_result_t result; - db_nodelock_t node_locks[1]; + isc_rwlock_t node_locks[1]; qpzonedb_t qpdb = { .common.methods = &qpdb_zonemethods, .common.mctx = mctx, @@ -125,7 +125,7 @@ ownercase_test_one(const char *str1, const char *str2) { memset(node_locks, 0, sizeof(node_locks)); /* Minimal initialization of the mock objects */ - NODE_INITLOCK(&qpdb.node_locks[0].lock); + NODE_INITLOCK(&qpdb.node_locks[0]); isc_buffer_constinit(&b, str1, strlen(str1)); isc_buffer_add(&b, strlen(str1)); @@ -145,7 +145,7 @@ ownercase_test_one(const char *str1, const char *str2) { /* Retrieve the case to name2 */ dns_rdataset_getownercase(&rdataset, name2); - NODE_DESTROYLOCK(&qpdb.node_locks[0].lock); + NODE_DESTROYLOCK(&qpdb.node_locks[0]); return dns_name_caseequal(name1, name2); } @@ -166,7 +166,7 @@ ISC_RUN_TEST_IMPL(ownercase) { ISC_RUN_TEST_IMPL(setownercase) { isc_result_t result; - db_nodelock_t node_locks[1]; + isc_rwlock_t node_locks[1]; qpzonedb_t qpdb = { .common.methods = &qpdb_zonemethods, .common.mctx = mctx, @@ -194,7 +194,7 @@ ISC_RUN_TEST_IMPL(setownercase) { /* Minimal initialization of the mock objects */ memset(node_locks, 0, sizeof(node_locks)); - NODE_INITLOCK(&qpdb.node_locks[0].lock); + NODE_INITLOCK(&qpdb.node_locks[0]); isc_buffer_constinit(&b, str1, strlen(str1)); isc_buffer_add(&b, strlen(str1)); @@ -211,7 +211,7 @@ ISC_RUN_TEST_IMPL(setownercase) { /* Retrieve the case to name2 */ dns_rdataset_getownercase(&rdataset, name2); - NODE_DESTROYLOCK(&qpdb.node_locks[0].lock); + NODE_DESTROYLOCK(&qpdb.node_locks[0]); assert_true(dns_name_caseequal(name1, name2)); } From 5300eebc9e68ab0f328831e5cd3849d9cf06ae2a Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Thu, 30 Jan 2025 14:42:57 -0800 Subject: [PATCH 4/6] Clarify reference counting in QP databases Change the names of the node reference counting functions and add comments to make the mechanism easier to understand: - newref() and decref() are now called qpcnode_acquire()/ qpznode_acquire() and qpcnode_release()/qpznode_release() respectively; this reflects the fact that they modify both the internal and external reference counters for a node. - qpcnode_newref() and qpznode_newref() are now called qpcnode_erefs_increment() and qpznode_erefs_increment(), and qpcnode_decref() and qpznode_decref() are now called qpcnode_erefs_decrement() and qpznode_erefs_decrement(), to reflect that they only increase and decrease the node's external reference counters, not internal. (cherry picked from commit d4f791793e2db06f63243eebb7c1a1de66d5792c) --- lib/dns/qpcache.c | 209 ++++++++++++++++++++++++++++------------------ lib/dns/qpzone.c | 155 +++++++++++++++++++++++----------- 2 files changed, 238 insertions(+), 126 deletions(-) diff --git a/lib/dns/qpcache.c b/lib/dns/qpcache.c index 23b566bcd6..9f02b1e395 100644 --- a/lib/dns/qpcache.c +++ b/lib/dns/qpcache.c @@ -172,6 +172,28 @@ struct qpcnode { uint16_t locknum; + /* + * 'erefs' counts external references held by a caller: for + * example, it could be incremented by dns_db_findnode(), + * and decremented by dns_db_detachnode(). + * + * 'references' counts internal references to the node object, + * including the one held by the QP trie so the node won't be + * deleted while it's quiescently stored in the database - even + * though 'erefs' may be zero because no external caller is + * using it at the time. + * + * Generally when 'erefs' is incremented or decremented, + * 'references' is too. When both go to zero (meaning callers + * and the database have both released the object) the object + * is freed. + * + * Whenever 'erefs' is incremented from zero, we also aquire a + * node use reference (see 'qpcache->references' below), and + * release it when 'erefs' goes back to zero. This prevents the + * database from being shut down until every caller has released + * all nodes. + */ isc_refcount_t references; isc_refcount_t erefs; void *data; @@ -205,15 +227,31 @@ struct qpcache { isc_rwlock_t lock; /* Locks the tree structure (prevents nodes appearing/disappearing) */ isc_rwlock_t tree_lock; + + /* + * NOTE: 'references' is NOT the global reference counter for + * the database object handled by dns_db_attach() and _detach(); + * that one is 'common.references'. + * + * Instead, 'references' counts the number of nodes being used by + * at least one external caller. (It's called 'references' to + * leverage the ISC_REFCOUNT_STATIC macros, but 'nodes_in_use' + * might be a clearer name.) + * + * One additional reference to this counter is held by the database + * object itself. When 'common.references' goes to zero, that + * reference is released. When in turn 'references' goes to zero, + * the database is shut down and freed. + */ + isc_refcount_t references; + /* Locks for individual tree nodes */ unsigned int node_lock_count; - dns_stats_t *rrsetstats; /* cache DB only */ - isc_stats_t *cachestats; /* cache DB only */ - isc_stats_t *gluecachestats; /* zone DB only */ - - isc_refcount_t references; isc_rwlock_t *node_locks; + dns_stats_t *rrsetstats; + isc_stats_t *cachestats; + uint32_t maxrrperset; /* Maximum RRs per RRset */ uint32_t maxtypepername; /* Maximum number of RR types per owner */ @@ -625,10 +663,18 @@ delete_node(qpcache_t *qpdb, qpcnode_t *node) { * It's okay for neither lock to be held if there are existing external * references to the node, but if this is the first external reference, * then the caller must be holding at least one lock. + * + * If incrementing erefs from zero, we also increment the node use counter + * in the qpcache object. + * + * This function is called from qpcnode_acquire(), so that internal + * and external references are acquired at the same time, and from + * qpcnode_release() when we only need to increase the internal references. */ static void -qpcnode_newref(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t nlocktype, - isc_rwlocktype_t tlocktype DNS__DB_FLARG) { +qpcnode_erefs_increment(qpcache_t *qpdb, qpcnode_t *node, + isc_rwlocktype_t nlocktype, + isc_rwlocktype_t tlocktype DNS__DB_FLARG) { uint_fast32_t refs = isc_refcount_increment0(&node->erefs); #if DNS_DB_NODETRACE @@ -655,17 +701,23 @@ qpcnode_newref(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t nlocktype, } static void -newref(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t nlocktype, - isc_rwlocktype_t tlocktype DNS__DB_FLARG) { +qpcnode_acquire(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t nlocktype, + isc_rwlocktype_t tlocktype DNS__DB_FLARG) { qpcnode_ref(node); - qpcnode_newref(qpdb, node, nlocktype, tlocktype DNS__DB_FLARG_PASS); + qpcnode_erefs_increment(qpdb, node, nlocktype, + tlocktype DNS__DB_FLARG_PASS); } static void cleanup_deadnodes(void *arg); +/* + * Decrement the external references to a node. If the counter + * goes to zero, decrement the node use counter in the qpcache object + * as well, and return true. Otherwise return false. + */ static bool -qpcnode_decref(qpcache_t *qpdb, qpcnode_t *node DNS__DB_FLARG) { +qpcnode_erefs_decrement(qpcache_t *qpdb, qpcnode_t *node DNS__DB_FLARG) { uint_fast32_t refs = isc_refcount_decrement(&node->erefs); #if DNS_DB_NODETRACE @@ -677,37 +729,32 @@ qpcnode_decref(qpcache_t *qpdb, qpcnode_t *node DNS__DB_FLARG) { } qpcache_unref(qpdb); - return true; } /* - * Caller must be holding the node lock; either the read or write lock. + * Caller must be holding a node lock, either read or write. + * * Note that the lock must be held even when node references are * atomically modified; in that case the decrement operation itself does not * have to be protected, but we must avoid a race condition where multiple * threads are decreasing the reference to zero simultaneously and at least * one of them is going to free the node. * - * This decrements both the internal and external node reference counters. - * If the external reference count drops to zero, then the node lock - * reference count is also decremented. - * - * This function returns true if and only if the node reference decreases - * to zero. (NOTE: Decrementing the reference count of a node to zero does - * not mean it will be immediately freed.) + * This calls dec_erefs() to decrement the external node reference counter, + * (and possibly the node use counter), cleans up and deletes the node + * if necessary, then decrements the internal reference counter as well. */ static void -decref(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t *nlocktypep, - isc_rwlocktype_t *tlocktypep, bool tryupgrade DNS__DB_FLARG) { +qpcnode_release(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t *nlocktypep, + isc_rwlocktype_t *tlocktypep, bool tryupgrade DNS__DB_FLARG) { + REQUIRE(*nlocktypep != isc_rwlocktype_none); + isc_result_t result; bool locked = *tlocktypep != isc_rwlocktype_none; bool write_locked = false; - isc_rwlock_t *nlock = &qpdb->node_locks[node->locknum]; - REQUIRE(*nlocktypep != isc_rwlocktype_none); - - if (!qpcnode_decref(qpdb, node DNS__DB_FLARG_PASS)) { + if (!qpcnode_erefs_decrement(qpdb, node DNS__DB_FLARG_PASS)) { goto unref; } @@ -716,20 +763,23 @@ decref(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t *nlocktypep, goto unref; } - /* - * Node lock ref has decremented to 0 and we may need to clean up the - * node. To clean it up, the node ref needs to decrement to 0 under the - * node write lock, so we regain the ref and try again. - */ - qpcnode_newref(qpdb, node, *nlocktypep, *tlocktypep DNS__DB_FLARG_PASS); - - /* Upgrade the lock? */ if (*nlocktypep == isc_rwlocktype_read) { + /* + * The external reference count went to zero and the node + * is dirty or has no data, so we might want to delete it. + * To do that, we'll need a write lock. If we don't already + * have one, we have to make sure nobody else has + * acquired a reference in the meantime, so we increment + * erefs (but NOT references!), upgrade the node lock, + * decrement erefs again, and see if it's still zero. + */ + isc_rwlock_t *nlock = &qpdb->node_locks[node->locknum]; + qpcnode_erefs_increment(qpdb, node, *nlocktypep, + *tlocktypep DNS__DB_FLARG_PASS); NODE_FORCEUPGRADE(nlock, nlocktypep); - } - - if (!qpcnode_decref(qpdb, node DNS__DB_FLARG_PASS)) { - goto unref; + if (!qpcnode_erefs_decrement(qpdb, node DNS__DB_FLARG_PASS)) { + goto unref; + } } if (node->dirty) { @@ -779,7 +829,8 @@ decref(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t *nlocktypep, */ delete_node(qpdb, node); } else { - newref(qpdb, node, *nlocktypep, *tlocktypep DNS__DB_FLARG_PASS); + qpcnode_acquire(qpdb, node, *nlocktypep, + *tlocktypep DNS__DB_FLARG_PASS); isc_queue_node_init(&node->deadlink); if (!isc_queue_enqueue_entry(&qpdb->deadnodes[node->locknum], @@ -795,7 +846,7 @@ decref(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t *nlocktypep, restore_locks: /* - * Relock a read lock, or unlock the write lock if no lock was held. + * Unlock the tree lock if it wasn't held previously. */ if (!locked && write_locked) { TREE_UNLOCK(&qpdb->tree_lock, tlocktypep); @@ -919,12 +970,12 @@ expireheader(dns_slabheader_t *header, isc_rwlocktype_t *nlocktypep, /* * If no one else is using the node, we can clean it up now. * We first need to gain a new reference to the node to meet a - * requirement of decref(). + * requirement of qpcnode_release(). */ - newref(qpdb, HEADERNODE(header), *nlocktypep, - *tlocktypep DNS__DB_FLARG_PASS); - decref(qpdb, HEADERNODE(header), nlocktypep, tlocktypep, - true DNS__DB_FLARG_PASS); + qpcnode_acquire(qpdb, HEADERNODE(header), *nlocktypep, + *tlocktypep DNS__DB_FLARG_PASS); + qpcnode_release(qpdb, HEADERNODE(header), nlocktypep, + tlocktypep, true DNS__DB_FLARG_PASS); if (qpdb->cachestats == NULL) { return; @@ -991,7 +1042,7 @@ bindrdataset(qpcache_t *qpdb, qpcnode_t *node, dns_slabheader_t *header, return; } - newref(qpdb, node, nlocktype, tlocktype DNS__DB_FLARG_PASS); + qpcnode_acquire(qpdb, node, nlocktype, tlocktype DNS__DB_FLARG_PASS); INSIST(rdataset->methods == NULL); /* We must be disassociated. */ @@ -1206,7 +1257,7 @@ check_stale_header(qpcnode_t *node, dns_slabheader_t *header, /* * header->down can be non-NULL if the * refcount has just decremented to 0 - * but decref() has not + * but qpcnode_release() has not * performed clean_cache_node(), in * which case we need to purge the stale * headers first. @@ -1278,8 +1329,8 @@ check_zonecut(qpcnode_t *node, void *arg DNS__DB_FLARG) { * We increment the reference count on node to ensure that * search->zonecut_header will still be valid later. */ - newref(search->qpdb, node, nlocktype, - isc_rwlocktype_none DNS__DB_FLARG_PASS); + qpcnode_acquire(search->qpdb, node, nlocktype, + isc_rwlocktype_none DNS__DB_FLARG_PASS); search->zonecut = node; search->zonecut_header = dname_header; search->zonecut_sigheader = sigdname_header; @@ -1364,8 +1415,9 @@ find_deepest_zonecut(qpc_search_t *search, qpcnode_t *node, } result = DNS_R_DELEGATION; if (nodep != NULL) { - newref(search->qpdb, node, nlocktype, - isc_rwlocktype_none DNS__DB_FLARG_PASS); + qpcnode_acquire( + search->qpdb, node, nlocktype, + isc_rwlocktype_none DNS__DB_FLARG_PASS); *nodep = node; } bindrdataset(search->qpdb, node, found, search->now, @@ -1500,8 +1552,8 @@ find_coveringnsec(qpc_search_t *search, const dns_name_t *name, nlocktype, isc_rwlocktype_none, sigrdataset DNS__DB_FLARG_PASS); } - newref(search->qpdb, node, nlocktype, - isc_rwlocktype_none DNS__DB_FLARG_PASS); + qpcnode_acquire(search->qpdb, node, nlocktype, + isc_rwlocktype_none DNS__DB_FLARG_PASS); dns_name_copy(fname, foundname); @@ -1794,8 +1846,8 @@ find(dns_db_t *db, const dns_name_t *name, dns_dbversion_t *version, nsecheader != NULL) { if (nodep != NULL) { - newref(search.qpdb, node, nlocktype, - tlocktype DNS__DB_FLARG_PASS); + qpcnode_acquire(search.qpdb, node, nlocktype, + tlocktype DNS__DB_FLARG_PASS); *nodep = node; } bindrdataset(search.qpdb, node, nsecheader, search.now, @@ -1838,8 +1890,8 @@ find(dns_db_t *db, const dns_name_t *name, dns_dbversion_t *version, */ if (nsheader != NULL) { if (nodep != NULL) { - newref(search.qpdb, node, nlocktype, - tlocktype DNS__DB_FLARG_PASS); + qpcnode_acquire(search.qpdb, node, nlocktype, + tlocktype DNS__DB_FLARG_PASS); *nodep = node; } bindrdataset(search.qpdb, node, nsheader, search.now, @@ -1872,8 +1924,8 @@ find(dns_db_t *db, const dns_name_t *name, dns_dbversion_t *version, */ if (nodep != NULL) { - newref(search.qpdb, node, nlocktype, - tlocktype DNS__DB_FLARG_PASS); + qpcnode_acquire(search.qpdb, node, nlocktype, + tlocktype DNS__DB_FLARG_PASS); *nodep = node; } @@ -1949,8 +2001,8 @@ tree_exit: nlock = &search.qpdb->node_locks[node->locknum]; NODE_RDLOCK(nlock, &nlocktype); - decref(search.qpdb, node, &nlocktype, &tlocktype, - true DNS__DB_FLARG_PASS); + qpcnode_release(search.qpdb, node, &nlocktype, &tlocktype, + true DNS__DB_FLARG_PASS); NODE_UNLOCK(nlock, &nlocktype); INSIST(tlocktype == isc_rwlocktype_none); } @@ -2092,8 +2144,8 @@ findzonecut(dns_db_t *db, const dns_name_t *name, unsigned int options, } if (nodep != NULL) { - newref(search.qpdb, node, nlocktype, - tlocktype DNS__DB_FLARG_PASS); + qpcnode_acquire(search.qpdb, node, nlocktype, + tlocktype DNS__DB_FLARG_PASS); *nodep = node; } @@ -2519,9 +2571,6 @@ qpcache__destroy(qpcache_t *qpdb) { if (qpdb->cachestats != NULL) { isc_stats_detach(&qpdb->cachestats); } - if (qpdb->gluecachestats != NULL) { - isc_stats_detach(&qpdb->gluecachestats); - } isc_mem_cput(qpdb->common.mctx, qpdb->node_locks, qpdb->node_lock_count, sizeof(qpdb->node_locks[0])); @@ -2576,7 +2625,7 @@ cleanup_deadnodes(void *arg) { RUNTIME_CHECK(isc_queue_splice(&deadnodes, &qpdb->deadnodes[locknum])); isc_queue_for_each_entry_safe(&deadnodes, qpnode, qpnext, deadlink) { - decref(qpdb, qpnode, &nlocktype, &tlocktype, false); + qpcnode_release(qpdb, qpnode, &nlocktype, &tlocktype, false); } NODE_UNLOCK(nlock, &nlocktype); @@ -2599,7 +2648,7 @@ reactivate_node(qpcache_t *qpdb, qpcnode_t *node, isc_rwlock_t *nlock = &qpdb->node_locks[node->locknum]; NODE_RDLOCK(nlock, &nlocktype); - newref(qpdb, node, nlocktype, tlocktype DNS__DB_FLARG_PASS); + qpcnode_acquire(qpdb, node, nlocktype, tlocktype DNS__DB_FLARG_PASS); NODE_UNLOCK(nlock, &nlocktype); } @@ -2669,8 +2718,8 @@ attachnode(dns_db_t *db, dns_dbnode_t *source, qpcache_t *qpdb = (qpcache_t *)db; qpcnode_t *node = (qpcnode_t *)source; - newref(qpdb, node, isc_rwlocktype_none, - isc_rwlocktype_none DNS__DB_FLARG_PASS); + qpcnode_acquire(qpdb, node, isc_rwlocktype_none, + isc_rwlocktype_none DNS__DB_FLARG_PASS); *targetp = source; } @@ -2691,13 +2740,15 @@ detachnode(dns_db_t *db, dns_dbnode_t **nodep DNS__DB_FLARG) { nlock = &qpdb->node_locks[node->locknum]; /* - * We can't destroy qpcache under nodelock, so we need - * to reference it before acquiring the lock. + * We can't destroy qpcache while holding a nodelock, so + * we need to reference it before acquiring the lock + * and release it afterward. */ qpcache_ref(qpdb); NODE_RDLOCK(nlock, &nlocktype); - decref(qpdb, node, &nlocktype, &tlocktype, true DNS__DB_FLARG_PASS); + qpcnode_release(qpdb, node, &nlocktype, &tlocktype, + true DNS__DB_FLARG_PASS); NODE_UNLOCK(nlock, &nlocktype); qpcache_detach(&qpdb); @@ -2753,8 +2804,8 @@ allrdatasets(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, iterator->common.now = now; iterator->current = NULL; - newref(qpdb, qpnode, isc_rwlocktype_none, - isc_rwlocktype_none DNS__DB_FLARG_PASS); + qpcnode_acquire(qpdb, qpnode, isc_rwlocktype_none, + isc_rwlocktype_none DNS__DB_FLARG_PASS); *iteratorp = (dns_rdatasetiter_t *)iterator; @@ -3893,8 +3944,8 @@ dereference_iter_node(qpc_dbit_t *qpdbiter DNS__DB_FLARG) { nlock = &qpdb->node_locks[node->locknum]; NODE_RDLOCK(nlock, &nlocktype); - decref(qpdb, node, &nlocktype, &qpdbiter->tree_locked, - false DNS__DB_FLARG_PASS); + qpcnode_release(qpdb, node, &nlocktype, &qpdbiter->tree_locked, + false DNS__DB_FLARG_PASS); NODE_UNLOCK(nlock, &nlocktype); INSIST(qpdbiter->tree_locked == tlocktype); @@ -4148,8 +4199,8 @@ dbiterator_current(dns_dbiterator_t *iterator, dns_dbnode_t **nodep, dns_name_copy(&node->name, name); } - newref(qpdb, node, isc_rwlocktype_none, - qpdbiter->tree_locked DNS__DB_FLARG_PASS); + qpcnode_acquire(qpdb, node, isc_rwlocktype_none, + qpdbiter->tree_locked DNS__DB_FLARG_PASS); *nodep = qpdbiter->node; return ISC_R_SUCCESS; diff --git a/lib/dns/qpzone.c b/lib/dns/qpzone.c index dc59100d61..25c52a2d2c 100644 --- a/lib/dns/qpzone.c +++ b/lib/dns/qpzone.c @@ -151,8 +151,32 @@ typedef ISC_LIST(qpz_version_t) qpz_versionlist_t; struct qpznode { dns_name_t name; isc_mem_t *mctx; + + /* + * 'erefs' counts external references held by a caller: for + * example, it could be incremented by dns_db_findnode(), + * and decremented by dns_db_detachnode(). + * + * 'references' counts internal references to the node object, + * including the one held by the QP trie so the node won't be + * deleted while it's quiescently stored in the database - even + * though 'erefs' may be zero because no external caller is + * using it at the time. + * + * Generally when 'erefs' is incremented or decremented, + * 'references' is too. When both go to zero (meaning callers + * and the database have both released the object) the object + * is freed. + * + * Whenever 'erefs' is incremented from zero, we also aquire a + * node use reference (see 'qpzonedb->references' below), and + * release it when 'erefs' goes back to zero. This prevents the + * database from being shut down until every caller has released + * all nodes. + */ isc_refcount_t references; isc_refcount_t erefs; + uint16_t locknum; atomic_uint_fast8_t nsec; atomic_bool wild; @@ -166,10 +190,27 @@ struct qpzonedb { dns_db_t common; /* Locks the data in this struct */ isc_rwlock_t lock; + + /* + * NOTE: 'references' is NOT the global reference counter for + * the database object handled by dns_db_attach() and _detach(); + * that one is 'common.references'. + * + * Instead, 'references' counts the number of nodes being used by + * at least one external caller. (It's called 'references' to + * leverage the ISC_REFCOUNT_STATIC macros, but 'nodes_in_use' + * might be a clearer name.) + * + * One additional reference to this counter is held by the database + * object itself. When 'common.references' goes to zero, that + * reference is released. When in turn 'references' goes to zero, + * the database is shut down and freed. + */ + isc_refcount_t references; + /* Locks for tree nodes */ int node_lock_count; isc_rwlock_t *node_locks; - isc_refcount_t references; qpznode_t *origin; qpznode_t *nsec3_origin; @@ -628,8 +669,16 @@ dns__qpzone_create(isc_mem_t *mctx, const dns_name_t *origin, dns_dbtype_t type, return ISC_R_SUCCESS; } +/* + * If incrementing erefs from zero, we also increment the node use counter + * in the qpzonedb object. + * + * This function is called from qpznode_acquire(), so that internal + * and external references are acquired at the same time, and from + * qpznode_release() when we only need to increase the internal references. + */ static void -qpznode_newref(qpzonedb_t *qpdb, qpznode_t *node DNS__DB_FLARG) { +qpznode_erefs_increment(qpzonedb_t *qpdb, qpznode_t *node DNS__DB_FLARG) { uint_fast32_t refs = isc_refcount_increment0(&node->erefs); #if DNS_DB_NODETRACE fprintf(stderr, "incr:node:%s:%s:%u:%p->erefs = %" PRIuFAST32 "\n", @@ -644,9 +693,9 @@ qpznode_newref(qpzonedb_t *qpdb, qpznode_t *node DNS__DB_FLARG) { } static void -newref(qpzonedb_t *qpdb, qpznode_t *node DNS__DB_FLARG) { +qpznode_acquire(qpzonedb_t *qpdb, qpznode_t *node DNS__DB_FLARG) { qpznode_ref(node); - qpznode_newref(qpdb, node DNS__DB_FLARG_PASS); + qpznode_erefs_increment(qpdb, node DNS__DB_FLARG_PASS); } static void @@ -766,8 +815,13 @@ clean_zone_node(qpznode_t *node, uint32_t least_serial) { } } +/* + * Decrement the external references to a node. If the counter + * goes to zero, decrement the node use counter in the qpzonedb object + * as well, and return true. Otherwise return false. + */ static bool -qpznode_decref(qpzonedb_t *qpdb, qpznode_t *node DNS__DB_FLARG) { +qpznode_erefs_decrement(qpzonedb_t *qpdb, qpznode_t *node DNS__DB_FLARG) { uint_fast32_t refs = isc_refcount_decrement(&node->erefs); #if DNS_DB_NODETRACE @@ -791,19 +845,16 @@ qpznode_decref(qpzonedb_t *qpdb, qpznode_t *node DNS__DB_FLARG) { * threads are decreasing the reference to zero simultaneously and at least * one of them is going to free the node. * - * This decrements both the internal and external node reference counters. - * If the external reference count drops to zero, then the node lock - * reference count is also decremented. - * - * (NOTE: Decrementing the reference count of a node to zero does - * not mean it will be immediately freed.) + * This calls dec_erefs() to decrement the external node reference counter, + * (and possibly the node use counter), cleans up and deletes the node + * if necessary, then decrements the internal reference counter as well. */ static void -decref(qpzonedb_t *qpdb, qpznode_t *node, uint32_t least_serial, - isc_rwlocktype_t *nlocktypep DNS__DB_FLARG) { +qpznode_release(qpzonedb_t *qpdb, qpznode_t *node, uint32_t least_serial, + isc_rwlocktype_t *nlocktypep DNS__DB_FLARG) { REQUIRE(*nlocktypep != isc_rwlocktype_none); - if (!qpznode_decref(qpdb, node DNS__DB_FLARG_PASS)) { + if (!qpznode_erefs_decrement(qpdb, node DNS__DB_FLARG_PASS)) { goto unref; } @@ -814,21 +865,22 @@ decref(qpzonedb_t *qpdb, qpznode_t *node, uint32_t least_serial, goto unref; } - /* - * Node lock ref has decremented to 0 and we may need to clean up the - * node. To clean it up, the node ref needs to decrement to 0 under the - * node write lock, so we regain the ref and try again. - */ - qpznode_newref(qpdb, node DNS__DB_FLARG_PASS); - - /* Upgrade the lock? */ if (*nlocktypep == isc_rwlocktype_read) { + /* + * The external reference count went to zero and the node + * is dirty or has no data, so we might want to delete it. + * To do that, we'll need a write lock. If we don't already + * have one, we have to make sure nobody else has + * acquired a reference in the meantime, so we increment + * erefs (but NOT references!), upgrade the node lock, + * decrement erefs again, and see if it's still zero. + */ isc_rwlock_t *nlock = &qpdb->node_locks[node->locknum]; + qpznode_erefs_increment(qpdb, node DNS__DB_FLARG_PASS); NODE_FORCEUPGRADE(nlock, nlocktypep); - } - - if (!qpznode_decref(qpdb, node DNS__DB_FLARG_PASS)) { - goto unref; + if (!qpznode_erefs_decrement(qpdb, node DNS__DB_FLARG_PASS)) { + goto unref; + } } if (node->dirty) { @@ -855,7 +907,7 @@ bindrdataset(qpzonedb_t *qpdb, qpznode_t *node, dns_slabheader_t *header, return; } - newref(qpdb, node DNS__DB_FLARG_PASS); + qpznode_acquire(qpdb, node DNS__DB_FLARG_PASS); INSIST(rdataset->methods == NULL); /* We must be disassociated. */ @@ -1158,7 +1210,7 @@ resigndelete(qpzonedb_t *qpdb, qpz_version_t *version, RWUNLOCK(&qpdb->lock, isc_rwlocktype_write); header->heap_index = 0; - newref(qpdb, HEADERNODE(header) DNS__DB_FLARG_PASS); + qpznode_acquire(qpdb, HEADERNODE(header) DNS__DB_FLARG_PASS); ISC_LIST_APPEND(version->resigned_list, header, link); } @@ -1396,8 +1448,8 @@ closeversion(dns_db_t *db, dns_dbversion_t **versionp, if (rollback && !IGNORE(header)) { resigninsert(qpdb, header); } - decref(qpdb, HEADERNODE(header), least_serial, - &nlocktype DNS__DB_FLARG_PASS); + qpznode_release(qpdb, HEADERNODE(header), least_serial, + &nlocktype DNS__DB_FLARG_PASS); NODE_UNLOCK(nlock, &nlocktype); } @@ -1420,7 +1472,8 @@ closeversion(dns_db_t *db, dns_dbversion_t **versionp, if (rollback) { rollback_node(node, serial); } - decref(qpdb, node, least_serial, &nlocktype DNS__DB_FILELINE); + qpznode_release(qpdb, node, least_serial, + &nlocktype DNS__DB_FILELINE); NODE_UNLOCK(nlock, &nlocktype); @@ -1663,7 +1716,7 @@ add_changed(dns_slabheader_t *header, qpz_version_t *version DNS__DB_FLARG) { *changed = (qpz_changed_t){ .node = node }; ISC_LIST_INITANDAPPEND(version->changed_list, changed, link); - newref(qpdb, node DNS__DB_FLARG_PASS); + qpznode_acquire(qpdb, node DNS__DB_FLARG_PASS); RWUNLOCK(&qpdb->lock, isc_rwlocktype_write); return changed; @@ -2424,7 +2477,7 @@ findnodeintree(qpzonedb_t *qpdb, const dns_name_t *name, bool create, INSIST(node->nsec == DNS_DB_NSEC_NSEC3 || !nsec3); } - newref(qpdb, node DNS__DB_FLARG_PASS); + qpznode_acquire(qpdb, node DNS__DB_FLARG_PASS); if (create) { dns_qp_compact(qp, DNS_QPGC_MAYBE); @@ -3024,8 +3077,9 @@ again: */ dns_name_copy(name, foundname); if (nodep != NULL) { - newref(search->qpdb, - node DNS__DB_FLARG_PASS); + qpznode_acquire( + search->qpdb, + node DNS__DB_FLARG_PASS); *nodep = node; } bindrdataset(search->qpdb, node, found, @@ -3169,7 +3223,7 @@ check_zonecut(qpznode_t *node, void *arg DNS__DB_FLARG) { * We increment the reference count on node to ensure that * search->zonecut_header will still be valid later. */ - newref(search->qpdb, node DNS__DB_FLARG_PASS); + qpznode_acquire(search->qpdb, node DNS__DB_FLARG_PASS); search->zonecut = node; search->zonecut_header = found; search->need_cleanup = true; @@ -3446,7 +3500,8 @@ found: * ensure that search->zonecut_header will * still be valid later. */ - newref(search.qpdb, node DNS__DB_FLARG_PASS); + qpznode_acquire(search.qpdb, + node DNS__DB_FLARG_PASS); search.zonecut = node; search.zonecut_header = header; search.zonecut_sigheader = NULL; @@ -3621,7 +3676,7 @@ found: goto tree_exit; } if (nodep != NULL) { - newref(search.qpdb, node DNS__DB_FLARG_PASS); + qpznode_acquire(search.qpdb, node DNS__DB_FLARG_PASS); *nodep = node; } if (search.version->secure && !search.version->havensec3) { @@ -3685,7 +3740,7 @@ found: if (nodep != NULL) { if (!at_zonecut) { - newref(search.qpdb, node DNS__DB_FLARG_PASS); + qpznode_acquire(search.qpdb, node DNS__DB_FLARG_PASS); } else { search.need_cleanup = false; } @@ -3725,7 +3780,8 @@ tree_exit: nlock = &search.qpdb->node_locks[node->locknum]; NODE_RDLOCK(nlock, &nlocktype); - decref(search.qpdb, node, 0, &nlocktype DNS__DB_FLARG_PASS); + qpznode_release(search.qpdb, node, 0, + &nlocktype DNS__DB_FLARG_PASS); NODE_UNLOCK(nlock, &nlocktype); } @@ -3764,7 +3820,7 @@ allrdatasets(dns_db_t *db, dns_dbnode_t *dbnode, dns_dbversion_t *dbversion, .common.magic = DNS_RDATASETITER_MAGIC, }; - newref(qpdb, node DNS__DB_FLARG_PASS); + qpznode_acquire(qpdb, node DNS__DB_FLARG_PASS); *iteratorp = (dns_rdatasetiter_t *)iterator; return ISC_R_SUCCESS; @@ -3779,7 +3835,7 @@ attachnode(dns_db_t *db, dns_dbnode_t *source, REQUIRE(VALID_QPZONE(qpdb)); REQUIRE(targetp != NULL && *targetp == NULL); - newref(qpdb, node DNS__DB_FLARG_PASS); + qpznode_acquire(qpdb, node DNS__DB_FLARG_PASS); *targetp = source; } @@ -3798,10 +3854,15 @@ detachnode(dns_db_t *db, dns_dbnode_t **nodep DNS__DB_FLARG) { *nodep = NULL; nlock = &qpdb->node_locks[node->locknum]; + /* + * We can't destroy qpzonedb while holding a nodelock, so + * we need to reference it before acquiring the lock + * and release it afterward. + */ qpzonedb_ref(qpdb); NODE_RDLOCK(nlock, &nlocktype); - decref(qpdb, node, 0, &nlocktype DNS__DB_FLARG_PASS); + qpznode_release(qpdb, node, 0, &nlocktype DNS__DB_FLARG_PASS); NODE_UNLOCK(nlock, &nlocktype); qpzonedb_detach(&qpdb); @@ -3862,7 +3923,7 @@ getoriginnode(dns_db_t *db, dns_dbnode_t **nodep DNS__DB_FLARG) { /* Note that the access to the origin node doesn't require a DB lock */ onode = (qpznode_t *)qpdb->origin; INSIST(onode != NULL); - newref(qpdb, onode DNS__DB_FLARG_PASS); + qpznode_acquire(qpdb, onode DNS__DB_FLARG_PASS); *nodep = onode; return ISC_R_SUCCESS; @@ -4066,7 +4127,7 @@ reference_iter_node(qpdb_dbiterator_t *iter DNS__DB_FLARG) { return; } - newref(qpdb, node DNS__DB_FLARG_PASS); + qpznode_acquire(qpdb, node DNS__DB_FLARG_PASS); } static void @@ -4084,7 +4145,7 @@ dereference_iter_node(qpdb_dbiterator_t *iter DNS__DB_FLARG) { nlock = &qpdb->node_locks[node->locknum]; NODE_RDLOCK(nlock, &nlocktype); - decref(qpdb, node, 0, &nlocktype DNS__DB_FLARG_PASS); + qpznode_release(qpdb, node, 0, &nlocktype DNS__DB_FLARG_PASS); NODE_UNLOCK(nlock, &nlocktype); } @@ -4428,7 +4489,7 @@ dbiterator_current(dns_dbiterator_t *iterator, dns_dbnode_t **nodep, dns_name_copy(&qpdbiter->node->name, name); } - newref(qpdb, node DNS__DB_FLARG_PASS); + qpznode_acquire(qpdb, node DNS__DB_FLARG_PASS); *nodep = qpdbiter->node; From 9c45de94736c0a0fe101eb0c2163208c00bab988 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 30 Jan 2025 17:48:34 +0100 Subject: [PATCH 5/6] Refactor node reference counting in rbtdb.c Refactor the pattern in the newref() and decref() functions in rbtdb.c following the pattern, so it follows the similar pattern we already have for QPDB. --- lib/dns/rbtdb.c | 119 +++++++++++++++++++++++------------------------- 1 file changed, 57 insertions(+), 62 deletions(-) diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index 247081905b..4080db246b 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -1115,14 +1115,39 @@ delete_node(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node) { } } +static void +rbtnode_newref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node DNS__DB_FLARG) { + uint_fast32_t refs = isc_refcount_increment0(&node->references); + +#if DNS_DB_NODETRACE + fprintf(stderr, "incr:node:%s:%s:%u:%p->references = %" PRIuFAST32 "\n", + func, file, line, node, refs + 1); +#endif + + if (refs > 0) { + return; + } + + /* this is the first reference to the node */ + refs = isc_refcount_increment0( + &rbtdb->node_locks[node->locknum].references); +#if DNS_DB_NODETRACE + fprintf(stderr, + "incr:nodelock:%s:%s:%u:%p:%p->references = " + "%" PRIuFAST32 "\n", + func, file, line, node, &rbtdb->node_locks[node->locknum], + refs + 1); +#else + UNUSED(refs); +#endif +} + /* * Caller must be holding the node lock. */ void dns__rbtdb_newref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, isc_rwlocktype_t nlocktype DNS__DB_FLARG) { - uint_fast32_t refs; - if (nlocktype == isc_rwlocktype_write && ISC_LINK_LINKED(node, deadlink)) { @@ -1130,28 +1155,7 @@ dns__rbtdb_newref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, deadlink); } - refs = isc_refcount_increment0(&node->references); -#if DNS_DB_NODETRACE - fprintf(stderr, "incr:node:%s:%s:%u:%p->references = %" PRIuFAST32 "\n", - func, file, line, node, refs + 1); -#else - UNUSED(refs); -#endif - - if (refs == 0) { - /* this is the first reference to the node */ - refs = isc_refcount_increment0( - &rbtdb->node_locks[node->locknum].references); -#if DNS_DB_NODETRACE - fprintf(stderr, - "incr:nodelock:%s:%s:%u:%p:%p->references = " - "%" PRIuFAST32 "\n", - func, file, line, node, - &rbtdb->node_locks[node->locknum], refs + 1); -#else - UNUSED(refs); -#endif - } + rbtnode_newref(rbtdb, node); } /*% @@ -1280,6 +1284,29 @@ reactivate_node(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, NODE_UNLOCK(nodelock, &nlocktype); } +static bool +rbtnode_decref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node DNS__DB_FLARG) { + db_nodelock_t *nodelock = &rbtdb->node_locks[node->locknum]; + uint_fast32_t refs = isc_refcount_decrement(&node->references); +#if DNS_DB_NODETRACE + fprintf(stderr, "decr:node:%s:%s:%u:%p->references = %" PRIuFAST32 "\n", + func, file, line, node, refs - 1); +#endif + if (refs > 1) { + return false; + } + refs = isc_refcount_decrement(&nodelock->references); +#if DNS_DB_NODETRACE + fprintf(stderr, + "decr:nodelock:%s:%s:%u:%p:%p->references = " + "%" PRIuFAST32 "\n", + func, file, line, node, nodelock, refs - 1); +#else + UNUSED(refs); +#endif + return true; +} + /* * Caller must be holding the node lock; either the read or write lock. * Note that the lock must be held even when node references are @@ -1302,10 +1329,8 @@ dns__rbtdb_decref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, isc_result_t result; bool locked = *tlocktypep != isc_rwlocktype_none; bool write_locked = false; - int bucket = node->locknum; - db_nodelock_t *nodelock = &rbtdb->node_locks[bucket]; + db_nodelock_t *nodelock = &rbtdb->node_locks[node->locknum]; bool no_reference = true; - uint_fast32_t refs; REQUIRE(*nlocktypep != isc_rwlocktype_none); @@ -1313,24 +1338,10 @@ dns__rbtdb_decref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, ((n)->data != NULL || ((l) && (n)->down != NULL) || \ (n) == (r)->origin_node || (n) == (r)->nsec3_origin_node) - /* Handle easy and/or typical case first. */ - refs = isc_refcount_decrement(&node->references); -#if DNS_DB_NODETRACE - fprintf(stderr, "decr:node:%s:%s:%u:%p->references = %" PRIuFAST32 "\n", - func, file, line, node, refs - 1); -#endif - if (refs > 1) { + if (!rbtnode_decref(rbtdb, node DNS__DB_FLARG_PASS)) { return false; } - refs = isc_refcount_decrement(&nodelock->references); -#if DNS_DB_NODETRACE - fprintf(stderr, - "decr:nodelock:%s:%s:%u:%p:%p->references = " - "%" PRIuFAST32 "\n", - func, file, line, node, nodelock, refs - 1); -#else - UNUSED(refs); -#endif + if (!node->dirty && KEEP_NODE(node, rbtdb, locked)) { return true; } @@ -1340,21 +1351,14 @@ dns__rbtdb_decref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, * node. To clean it up, the node ref needs to decrement to 0 under the * node write lock, so we regain the ref and try again. */ - dns__rbtdb_newref(rbtdb, node, *nlocktypep DNS__DB_FLARG_PASS); + rbtnode_newref(rbtdb, node DNS__DB_FLARG_PASS); /* Upgrade the lock? */ if (*nlocktypep == isc_rwlocktype_read) { NODE_FORCEUPGRADE(&nodelock->lock, nlocktypep); } - refs = isc_refcount_decrement(&node->references); -#if DNS_DB_NODETRACE - fprintf(stderr, "decr:node:%s:%s:%u:%p->references = %" PRIuFAST32 "\n", - func, file, line, node, refs - 1); -#else - UNUSED(refs); -#endif - if (refs > 1) { + if (!rbtnode_decref(rbtdb, node DNS__DB_FLARG_PASS)) { return false; } @@ -1407,15 +1411,6 @@ dns__rbtdb_decref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, write_locked = true; } - refs = isc_refcount_decrement(&nodelock->references); -#if DNS_DB_NODETRACE - fprintf(stderr, - "decr:nodelock:%s:%s:%u:%p:%p->references = %" PRIuFAST32 "\n", - func, file, line, node, nodelock, refs - 1); -#else - UNUSED(refs); -#endif - if (KEEP_NODE(node, rbtdb, locked || write_locked)) { goto restore_locks; } @@ -1455,7 +1450,7 @@ dns__rbtdb_decref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, } else { INSIST(node->data == NULL); if (!ISC_LINK_LINKED(node, deadlink)) { - ISC_LIST_APPEND(rbtdb->deadnodes[bucket], node, + ISC_LIST_APPEND(rbtdb->deadnodes[node->locknum], node, deadlink); } } From 857225aeb65497f979e459269f6425f88f0e3cd2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Fri, 31 Jan 2025 06:07:48 +0100 Subject: [PATCH 6/6] Clarify reference counting in RBTDB database Change the names of the node reference counting functions and add comments to make the mechanism easier to understand: - dns__rbtdb_newref() and dns__rbtdb_decref() are now called dns__rbtnode_acquire() and dns__rbtnode_release() respectively; this reflects the fact that they modify both the internal and external reference counters for a node. - rbtnode_newref() and rbtnode_decref are now called rbtnode_erefs_increment() and rbtnode_erefs_decrement(), to reflect that they only increase and decrease the node's external reference counters, not internal. --- lib/dns/rbt-cachedb.c | 50 ++++++++++++----------- lib/dns/rbt-zonedb.c | 26 ++++++------ lib/dns/rbtdb.c | 95 ++++++++++++++++++++++--------------------- lib/dns/rbtdb_p.h | 12 +++--- 4 files changed, 95 insertions(+), 88 deletions(-) diff --git a/lib/dns/rbt-cachedb.c b/lib/dns/rbt-cachedb.c index 72f3d2fa06..412e9d94cd 100644 --- a/lib/dns/rbt-cachedb.c +++ b/lib/dns/rbt-cachedb.c @@ -404,7 +404,7 @@ check_stale_header(dns_rbtnode_t *node, dns_slabheader_t *header, /* * header->down can be non-NULL if the * refcount has just decremented to 0 - * but dns__rbtdb_decref() has not + * but dns__rbtnode_release() has not * performed clean_cache_node(), in * which case we need to purge the stale * headers first. @@ -483,8 +483,8 @@ cache_zonecut_callback(dns_rbtnode_t *node, dns_name_t *name, * We increment the reference count on node to ensure that * search->zonecut_header will still be valid later. */ - dns__rbtdb_newref(search->rbtdb, node, - nlocktype DNS__DB_FLARG_PASS); + dns__rbtnode_acquire(search->rbtdb, node, + nlocktype DNS__DB_FLARG_PASS); search->zonecut = node; search->zonecut_header = dname_header; search->zonecut_sigheader = sigdname_header; @@ -591,8 +591,9 @@ find_deepest_zonecut(rbtdb_search_t *search, dns_rbtnode_t *node, } result = DNS_R_DELEGATION; if (nodep != NULL) { - dns__rbtdb_newref(search->rbtdb, node, - nlocktype DNS__DB_FLARG_PASS); + dns__rbtnode_acquire( + search->rbtdb, node, + nlocktype DNS__DB_FLARG_PASS); *nodep = node; } dns__rbtdb_bindrdataset(search->rbtdb, node, found, @@ -743,8 +744,8 @@ find_coveringnsec(rbtdb_search_t *search, const dns_name_t *name, now, nlocktype, sigrdataset DNS__DB_FLARG_PASS); } - dns__rbtdb_newref(search->rbtdb, node, - nlocktype DNS__DB_FLARG_PASS); + dns__rbtnode_acquire(search->rbtdb, node, + nlocktype DNS__DB_FLARG_PASS); dns_name_copy(fname, foundname); @@ -1009,8 +1010,9 @@ cache_find(dns_db_t *db, const dns_name_t *name, dns_dbversion_t *version, nsecheader != NULL) { if (nodep != NULL) { - dns__rbtdb_newref(search.rbtdb, node, - nlocktype DNS__DB_FLARG_PASS); + dns__rbtnode_acquire( + search.rbtdb, node, + nlocktype DNS__DB_FLARG_PASS); *nodep = node; } dns__rbtdb_bindrdataset(search.rbtdb, node, nsecheader, @@ -1054,8 +1056,9 @@ cache_find(dns_db_t *db, const dns_name_t *name, dns_dbversion_t *version, */ if (nsheader != NULL) { if (nodep != NULL) { - dns__rbtdb_newref(search.rbtdb, node, - nlocktype DNS__DB_FLARG_PASS); + dns__rbtnode_acquire( + search.rbtdb, node, + nlocktype DNS__DB_FLARG_PASS); *nodep = node; } dns__rbtdb_bindrdataset(search.rbtdb, node, nsheader, @@ -1089,8 +1092,8 @@ cache_find(dns_db_t *db, const dns_name_t *name, dns_dbversion_t *version, */ if (nodep != NULL) { - dns__rbtdb_newref(search.rbtdb, node, - nlocktype DNS__DB_FLARG_PASS); + dns__rbtnode_acquire(search.rbtdb, node, + nlocktype DNS__DB_FLARG_PASS); *nodep = node; } @@ -1166,8 +1169,9 @@ tree_exit: lock = &(search.rbtdb->node_locks[node->locknum].lock); NODE_RDLOCK(lock, &nlocktype); - dns__rbtdb_decref(search.rbtdb, node, 0, &nlocktype, &tlocktype, - true, false DNS__DB_FLARG_PASS); + dns__rbtnode_release(search.rbtdb, node, 0, &nlocktype, + &tlocktype, true, + false DNS__DB_FLARG_PASS); NODE_UNLOCK(lock, &nlocktype); INSIST(tlocktype == isc_rwlocktype_none); } @@ -1305,8 +1309,8 @@ cache_findzonecut(dns_db_t *db, const dns_name_t *name, unsigned int options, } if (nodep != NULL) { - dns__rbtdb_newref(search.rbtdb, node, - nlocktype DNS__DB_FLARG_PASS); + dns__rbtnode_acquire(search.rbtdb, node, + nlocktype DNS__DB_FLARG_PASS); *nodep = node; } @@ -1604,13 +1608,13 @@ dns__cacherbt_expireheader(dns_slabheader_t *header, /* * If no one else is using the node, we can clean it up now. * We first need to gain a new reference to the node to meet a - * requirement of dns__rbtdb_decref(). + * requirement of dns__rbtnode_release(). */ - dns__rbtdb_newref(rbtdb, RBTDB_HEADERNODE(header), - nlocktype DNS__DB_FLARG_PASS); - dns__rbtdb_decref(rbtdb, RBTDB_HEADERNODE(header), 0, - &nlocktype, tlocktypep, true, - false DNS__DB_FLARG_PASS); + dns__rbtnode_acquire(rbtdb, RBTDB_HEADERNODE(header), + nlocktype DNS__DB_FLARG_PASS); + dns__rbtnode_release(rbtdb, RBTDB_HEADERNODE(header), 0, + &nlocktype, tlocktypep, true, + false DNS__DB_FLARG_PASS); if (rbtdb->cachestats == NULL) { return; diff --git a/lib/dns/rbt-zonedb.c b/lib/dns/rbt-zonedb.c index 226603dea5..8c34beade1 100644 --- a/lib/dns/rbt-zonedb.c +++ b/lib/dns/rbt-zonedb.c @@ -199,8 +199,8 @@ zone_zonecut_callback(dns_rbtnode_t *node, dns_name_t *name, * We increment the reference count on node to ensure that * search->zonecut_header will still be valid later. */ - dns__rbtdb_newref(search->rbtdb, node, - isc_rwlocktype_read DNS__DB_FLARG_PASS); + dns__rbtnode_acquire(search->rbtdb, node, + isc_rwlocktype_read DNS__DB_FLARG_PASS); search->zonecut = node; search->zonecut_header = found; search->need_cleanup = true; @@ -902,7 +902,7 @@ again: foundname, NULL); if (result == ISC_R_SUCCESS) { if (nodep != NULL) { - dns__rbtdb_newref( + dns__rbtnode_acquire( search->rbtdb, node, isc_rwlocktype_read DNS__DB_FLARG_PASS); @@ -1195,8 +1195,9 @@ found: * ensure that search->zonecut_header will * still be valid later. */ - dns__rbtdb_newref(search.rbtdb, node, - nlocktype DNS__DB_FLARG_PASS); + dns__rbtnode_acquire( + search.rbtdb, node, + nlocktype DNS__DB_FLARG_PASS); search.zonecut = node; search.zonecut_header = header; search.zonecut_sigheader = NULL; @@ -1372,8 +1373,8 @@ found: goto tree_exit; } if (nodep != NULL) { - dns__rbtdb_newref(search.rbtdb, node, - nlocktype DNS__DB_FLARG_PASS); + dns__rbtnode_acquire(search.rbtdb, node, + nlocktype DNS__DB_FLARG_PASS); *nodep = node; } if (search.rbtversion->secure && !search.rbtversion->havensec3) @@ -1442,8 +1443,8 @@ found: if (nodep != NULL) { if (!at_zonecut) { - dns__rbtdb_newref(search.rbtdb, node, - nlocktype DNS__DB_FLARG_PASS); + dns__rbtnode_acquire(search.rbtdb, node, + nlocktype DNS__DB_FLARG_PASS); } else { search.need_cleanup = false; } @@ -1480,8 +1481,9 @@ tree_exit: lock = &(search.rbtdb->node_locks[node->locknum].lock); NODE_RDLOCK(lock, &nlocktype); - dns__rbtdb_decref(search.rbtdb, node, 0, &nlocktype, &tlocktype, - true, false DNS__DB_FLARG_PASS); + dns__rbtnode_release(search.rbtdb, node, 0, &nlocktype, + &tlocktype, true, + false DNS__DB_FLARG_PASS); NODE_UNLOCK(lock, &nlocktype); INSIST(tlocktype == isc_rwlocktype_none); } @@ -2311,7 +2313,7 @@ dns__zonerbt_resigndelete(dns_rbtdb_t *rbtdb, dns_rbtdb_version_t *version, header->heap_index); header->heap_index = 0; if (version != NULL) { - dns__rbtdb_newref( + dns__rbtnode_acquire( rbtdb, RBTDB_HEADERNODE(header), isc_rwlocktype_write DNS__DB_FLARG_PASS); ISC_LIST_APPEND(version->resigned_list, header, link); diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index 4080db246b..a708f7b7fe 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -1116,7 +1116,7 @@ delete_node(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node) { } static void -rbtnode_newref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node DNS__DB_FLARG) { +rbtnode_erefs_increment(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node DNS__DB_FLARG) { uint_fast32_t refs = isc_refcount_increment0(&node->references); #if DNS_DB_NODETRACE @@ -1146,8 +1146,8 @@ rbtnode_newref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node DNS__DB_FLARG) { * Caller must be holding the node lock. */ void -dns__rbtdb_newref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, - isc_rwlocktype_t nlocktype DNS__DB_FLARG) { +dns__rbtnode_acquire(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, + isc_rwlocktype_t nlocktype DNS__DB_FLARG) { if (nlocktype == isc_rwlocktype_write && ISC_LINK_LINKED(node, deadlink)) { @@ -1155,7 +1155,7 @@ dns__rbtdb_newref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, deadlink); } - rbtnode_newref(rbtdb, node); + rbtnode_erefs_increment(rbtdb, node); } /*% @@ -1174,7 +1174,7 @@ send_to_prune_tree(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, *prune = (rbtdb_prune_t){ .node = node }; dns_db_attach((dns_db_t *)rbtdb, &prune->db); - dns__rbtdb_newref(rbtdb, node, nlocktype DNS__DB_FLARG_PASS); + dns__rbtnode_acquire(rbtdb, node, nlocktype DNS__DB_FLARG_PASS); isc_async_run(rbtdb->loop, prune_tree, prune); } @@ -1279,13 +1279,13 @@ reactivate_node(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, } } - dns__rbtdb_newref(rbtdb, node, nlocktype DNS__DB_FLARG_PASS); + dns__rbtnode_acquire(rbtdb, node, nlocktype DNS__DB_FLARG_PASS); NODE_UNLOCK(nodelock, &nlocktype); } static bool -rbtnode_decref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node DNS__DB_FLARG) { +rbtnode_erefs_decrement(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node DNS__DB_FLARG) { db_nodelock_t *nodelock = &rbtdb->node_locks[node->locknum]; uint_fast32_t refs = isc_refcount_decrement(&node->references); #if DNS_DB_NODETRACE @@ -1322,10 +1322,10 @@ rbtnode_decref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node DNS__DB_FLARG) { * will be immediately freed. */ bool -dns__rbtdb_decref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, - uint32_t least_serial, isc_rwlocktype_t *nlocktypep, - isc_rwlocktype_t *tlocktypep, bool tryupgrade, - bool pruning DNS__DB_FLARG) { +dns__rbtnode_release(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, + uint32_t least_serial, isc_rwlocktype_t *nlocktypep, + isc_rwlocktype_t *tlocktypep, bool tryupgrade, + bool pruning DNS__DB_FLARG) { isc_result_t result; bool locked = *tlocktypep != isc_rwlocktype_none; bool write_locked = false; @@ -1338,7 +1338,7 @@ dns__rbtdb_decref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, ((n)->data != NULL || ((l) && (n)->down != NULL) || \ (n) == (r)->origin_node || (n) == (r)->nsec3_origin_node) - if (!rbtnode_decref(rbtdb, node DNS__DB_FLARG_PASS)) { + if (!rbtnode_erefs_decrement(rbtdb, node DNS__DB_FLARG_PASS)) { return false; } @@ -1346,20 +1346,19 @@ dns__rbtdb_decref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, return true; } - /* - * Node lock ref has decremented to 0 and we may need to clean up the - * node. To clean it up, the node ref needs to decrement to 0 under the - * node write lock, so we regain the ref and try again. - */ - rbtnode_newref(rbtdb, node DNS__DB_FLARG_PASS); - /* Upgrade the lock? */ if (*nlocktypep == isc_rwlocktype_read) { + /* + * Node lock ref has decremented to 0 and we may need to clean + * up the node. To clean it up, the node ref needs to decrement + * to 0 under the node write lock, so we regain the ref and try + * again. + */ + rbtnode_erefs_increment(rbtdb, node DNS__DB_FLARG_PASS); NODE_FORCEUPGRADE(&nodelock->lock, nlocktypep); - } - - if (!rbtnode_decref(rbtdb, node DNS__DB_FLARG_PASS)) { - return false; + if (!rbtnode_erefs_decrement(rbtdb, node DNS__DB_FLARG_PASS)) { + return false; + } } if (node->dirty) { @@ -1428,9 +1427,9 @@ dns__rbtdb_decref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, * different node buckets and we don't want to do juggle locks * right now. * - * Since prune_tree() also calls dns__rbtdb_decref(), check the - * value of the 'pruning' parameter (which is only set to - * 'true' in the dns__rbtdb_decref() call present in + * Since prune_tree() also calls dns__rbtnode_release(), check + * the value of the 'pruning' parameter (which is only set to + * 'true' in the dns__rbtnode_release() call present in * prune_tree()) to prevent an infinite loop and to allow a * node sent to prune_tree() to be deleted by the delete_node() * call in the code branch below. @@ -1469,7 +1468,7 @@ restore_locks: /* * Prune the RBTDB tree of trees. Start by attempting to delete a node that is * the only one left on its RBTDB level (see the send_to_prune_tree() call in - * dns__rbtdb_decref()). Then, if the node has a parent (which can either + * dns__rbtnode_release()). Then, if the node has a parent (which can either * exist on the same RBTDB level or on an upper RBTDB level), check whether the * latter is an interior node (i.e. a node with a non-NULL 'down' pointer). If * the parent node is not an interior node, attempt deleting the parent node as @@ -1505,12 +1504,12 @@ prune_tree(void *arg) { NODE_WRLOCK(&rbtdb->node_locks[locknum].lock, &nlocktype); do { parent = node->parent; - dns__rbtdb_decref(rbtdb, node, 0, &nlocktype, &tlocktype, true, - true DNS__DB_FILELINE); + dns__rbtnode_release(rbtdb, node, 0, &nlocktype, &tlocktype, + true, true DNS__DB_FILELINE); /* * Check whether the parent is an interior node. Note that it - * might have been one before the dns__rbtdb_decref() call on + * might have been one before the dns__rbtnode_release() call on * the previous line, but decrementing the reference count for * 'node' could have caused 'node->parent->down' to become * NULL. @@ -1532,8 +1531,8 @@ prune_tree(void *arg) { * We need to gain a reference to the parent node * before decrementing it in the next iteration. */ - dns__rbtdb_newref(rbtdb, parent, - nlocktype DNS__DB_FLARG_PASS); + dns__rbtnode_acquire(rbtdb, parent, + nlocktype DNS__DB_FLARG_PASS); } else { parent = NULL; } @@ -1968,9 +1967,9 @@ dns__rbtdb_closeversion(dns_db_t *db, dns_dbversion_t **versionp, rbtdb, RBTDB_HEADERNODE(header)->locknum, header); } - dns__rbtdb_decref(rbtdb, RBTDB_HEADERNODE(header), least_serial, - &nlocktype, &tlocktype, true, - false DNS__DB_FLARG_PASS); + dns__rbtnode_release(rbtdb, RBTDB_HEADERNODE(header), + least_serial, &nlocktype, &tlocktype, true, + false DNS__DB_FLARG_PASS); NODE_UNLOCK(lock, &nlocktype); INSIST(tlocktype == isc_rwlocktype_none); } @@ -1982,7 +1981,7 @@ dns__rbtdb_closeversion(dns_db_t *db, dns_dbversion_t **versionp, /* * We acquire a tree write lock here in order to make * sure that stale nodes will be removed in - * dns__rbtdb_decref(). If we didn't have the lock, + * dns__rbtnode_release(). If we didn't have the lock, * those nodes could miss the chance to be removed * until the server stops. The write lock is * expensive, but this should be rare enough @@ -2015,9 +2014,9 @@ dns__rbtdb_closeversion(dns_db_t *db, dns_dbversion_t **versionp, if (rollback) { rollback_node(rbtnode, serial); } - dns__rbtdb_decref(rbtdb, rbtnode, least_serial, - &nlocktype, &tlocktype, true, - false DNS__DB_FILELINE); + dns__rbtnode_release(rbtdb, rbtnode, least_serial, + &nlocktype, &tlocktype, true, + false DNS__DB_FILELINE); NODE_UNLOCK(lock, &nlocktype); @@ -2135,7 +2134,7 @@ dns__rbtdb_bindrdataset(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, return; } - dns__rbtdb_newref(rbtdb, node, locktype DNS__DB_FLARG_PASS); + dns__rbtnode_acquire(rbtdb, node, locktype DNS__DB_FLARG_PASS); INSIST(rdataset->methods == NULL); /* We must be disassociated. */ @@ -2266,8 +2265,8 @@ dns__rbtdb_detachnode(dns_db_t *db, dns_dbnode_t **targetp DNS__DB_FLARG) { NODE_RDLOCK(&nodelock->lock, &nlocktype); - if (dns__rbtdb_decref(rbtdb, node, 0, &nlocktype, &tlocktype, true, - false DNS__DB_FLARG_PASS)) + if (dns__rbtnode_release(rbtdb, node, 0, &nlocktype, &tlocktype, true, + false DNS__DB_FLARG_PASS)) { if (isc_refcount_current(&nodelock->references) == 0 && nodelock->exiting) @@ -3836,8 +3835,8 @@ dns__rbtdb_getoriginnode(dns_db_t *db, dns_dbnode_t **nodep DNS__DB_FLARG) { /* Note that the access to origin_node doesn't require a DB lock */ onode = (dns_rbtnode_t *)rbtdb->origin_node; if (onode != NULL) { - dns__rbtdb_newref(rbtdb, onode, - isc_rwlocktype_none DNS__DB_FLARG_PASS); + dns__rbtnode_acquire(rbtdb, onode, + isc_rwlocktype_none DNS__DB_FLARG_PASS); *nodep = rbtdb->origin_node; } else { INSIST(IS_CACHE(rbtdb)); @@ -4348,8 +4347,9 @@ dereference_iter_node(rbtdb_dbiterator_t *rbtdbiter DNS__DB_FLARG) { lock = &rbtdb->node_locks[node->locknum].lock; NODE_RDLOCK(lock, &nlocktype); - dns__rbtdb_decref(rbtdb, node, 0, &nlocktype, &rbtdbiter->tree_locked, - false, false DNS__DB_FLARG_PASS); + dns__rbtnode_release(rbtdb, node, 0, &nlocktype, + &rbtdbiter->tree_locked, false, + false DNS__DB_FLARG_PASS); NODE_UNLOCK(lock, &nlocktype); INSIST(rbtdbiter->tree_locked == tlocktype); @@ -4820,7 +4820,8 @@ dbiterator_current(dns_dbiterator_t *iterator, dns_dbnode_t **nodep, result = ISC_R_SUCCESS; } - dns__rbtdb_newref(rbtdb, node, isc_rwlocktype_none DNS__DB_FLARG_PASS); + dns__rbtnode_acquire(rbtdb, node, + isc_rwlocktype_none DNS__DB_FLARG_PASS); *nodep = rbtdbiter->node; diff --git a/lib/dns/rbtdb_p.h b/lib/dns/rbtdb_p.h index b28863eac7..45ab1bcc90 100644 --- a/lib/dns/rbtdb_p.h +++ b/lib/dns/rbtdb_p.h @@ -373,8 +373,8 @@ void dns__rbtdb_freeglue(dns_glue_t *glue_list); void -dns__rbtdb_newref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, - isc_rwlocktype_t locktype DNS__DB_FLARG); +dns__rbtnode_acquire(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, + isc_rwlocktype_t locktype DNS__DB_FLARG); /*%< * Increment the reference counter to a node in an RBT database. * If the caller holds a node lock then its lock type is specified @@ -384,10 +384,10 @@ dns__rbtdb_newref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, */ bool -dns__rbtdb_decref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, - uint32_t least_serial, isc_rwlocktype_t *nlocktypep, - isc_rwlocktype_t *tlocktypep, bool tryupgrade, - bool pruning DNS__DB_FLARG); +dns__rbtnode_release(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, + uint32_t least_serial, isc_rwlocktype_t *nlocktypep, + isc_rwlocktype_t *tlocktypep, bool tryupgrade, + bool pruning DNS__DB_FLARG); /*%< * Decrement the reference counter to a node in an RBT database. * 'nlocktypep' and 'tlocktypep' are pointers to the current status