From 8465e4516fbffc59e2d1666ad35a2db4c6a45731 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 1/2] 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. (cherry picked from commit 9c45de94736c0a0fe101eb0c2163208c00bab988) --- lib/dns/rbtdb.c | 67 ++++++++++++++++++++++++++++--------------------- 1 file changed, 38 insertions(+), 29 deletions(-) diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index 2d1b143b20..f3d2b1b54b 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -1878,6 +1878,15 @@ delete_node(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node) { } } +static void +rbtnode_newref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node) { + if (isc_refcount_increment0(&node->references) == 0) { + /* this is the first reference to the node */ + isc_refcount_increment0( + &rbtdb->node_locks[node->locknum].references); + } +} + /* * Caller must be holding the node lock. */ @@ -1890,11 +1899,8 @@ new_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, ISC_LIST_UNLINK(rbtdb->deadnodes[node->locknum], node, deadlink); } - if (isc_refcount_increment0(&node->references) == 0) { - /* this is the first reference to the node */ - isc_refcount_increment0( - &rbtdb->node_locks[node->locknum].references); - } + + rbtnode_newref(rbtdb, node); } /*% @@ -2025,6 +2031,22 @@ reactivate_node(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, NODE_UNLOCK(nodelock, locktype); } +static bool +rbtnode_decref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node) { + uint32_t refs; + rbtdb_nodelock_t *nodelock = &rbtdb->node_locks[node->locknum]; + + /* Handle easy and/or typical case first. */ + if (isc_refcount_decrement(&node->references) > 1) { + return false; + } + + refs = isc_refcount_decrement(&nodelock->references); + INSIST(refs > 0); + + return true; +} + /* * Caller must be holding the node lock; either the "strong", read or write * lock. Note that the lock must be held even when node references are @@ -2049,44 +2071,34 @@ decrement_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, int bucket = node->locknum; rbtdb_nodelock_t *nodelock = &rbtdb->node_locks[bucket]; bool no_reference = true; - uint_fast32_t refs; #define KEEP_NODE(n, r, l) \ ((n)->data != NULL || ((l) && (n)->down != NULL) || \ (n) == (r)->origin_node || (n) == (r)->nsec3_origin_node) - /* Handle easy and/or typical case first. */ - if (isc_refcount_decrement(&node->references) > 1) { + if (!rbtnode_decref(rbtdb, node)) { return false; } - refs = isc_refcount_decrement(&nodelock->references); - INSIST(refs > 0); - - /* Handle easy and typical case first. */ if (!node->dirty && KEEP_NODE(node, rbtdb, locked)) { 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. - */ - new_reference(rbtdb, node, nlock); - /* Upgrade the lock? */ if (nlock == 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_newref(rbtdb, node); NODE_UNLOCK(&nodelock->lock, isc_rwlocktype_read); NODE_LOCK(&nodelock->lock, isc_rwlocktype_write); - } - - if (isc_refcount_decrement(&node->references) > 1) { - /* Restore the lock? */ - if (nlock == isc_rwlocktype_read) { + if (!rbtnode_decref(rbtdb, node)) { NODE_DOWNGRADE(&nodelock->lock); + return false; } - return false; } if (node->dirty) { @@ -2131,9 +2143,6 @@ decrement_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, write_locked = true; } - refs = isc_refcount_decrement(&nodelock->references); - INSIST(refs > 0); - if (KEEP_NODE(node, rbtdb, locked || write_locked)) { goto restore_locks; } @@ -2172,7 +2181,7 @@ decrement_reference(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 2974bbba805ba2f3c145fb9116a6c21129186784 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 2/2] 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: - new_reference() and decrement_references() 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. (cherry picked from commit 857225aeb65497f979e459269f6425f88f0e3cd2) --- lib/dns/rbtdb.c | 132 +++++++++++++++++++++++++----------------------- 1 file changed, 70 insertions(+), 62 deletions(-) diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index f3d2b1b54b..1d60c14308 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -1879,7 +1879,7 @@ delete_node(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node) { } static void -rbtnode_newref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node) { +rbtnode_erefs_increment(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node) { if (isc_refcount_increment0(&node->references) == 0) { /* this is the first reference to the node */ isc_refcount_increment0( @@ -1891,8 +1891,8 @@ rbtnode_newref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node) { * Caller must be holding the node lock. */ static void -new_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, - isc_rwlocktype_t nlocktype) { +dns__rbtnode_acquire(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, + isc_rwlocktype_t nlocktype) { if (nlocktype == isc_rwlocktype_write && ISC_LINK_LINKED(node, deadlink)) { @@ -1900,7 +1900,7 @@ new_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, deadlink); } - rbtnode_newref(rbtdb, node); + rbtnode_erefs_increment(rbtdb, node); } /*% @@ -1920,7 +1920,7 @@ send_to_prune_tree(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, ev = isc_event_allocate(rbtdb->common.mctx, NULL, DNS_EVENT_RBTPRUNE, prune_tree, node, sizeof(isc_event_t)); - new_reference(rbtdb, node, nlocktype); + dns__rbtnode_acquire(rbtdb, node, nlocktype); db = NULL; attach((dns_db_t *)rbtdb, &db); ev->ev_sender = db; @@ -2026,13 +2026,13 @@ reactivate_node(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, } } - new_reference(rbtdb, node, locktype); + dns__rbtnode_acquire(rbtdb, node, locktype); NODE_UNLOCK(nodelock, locktype); } static bool -rbtnode_decref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node) { +rbtnode_erefs_decrement(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node) { uint32_t refs; rbtdb_nodelock_t *nodelock = &rbtdb->node_locks[node->locknum]; @@ -2062,9 +2062,9 @@ rbtnode_decref(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node) { * will be immediately freed. */ static bool -decrement_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, - rbtdb_serial_t least_serial, isc_rwlocktype_t nlock, - isc_rwlocktype_t tlock, bool pruning) { +dns__rbtnode_release(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, + rbtdb_serial_t least_serial, isc_rwlocktype_t nlock, + isc_rwlocktype_t tlock, bool pruning) { isc_result_t result; bool write_locked; bool locked = tlock != isc_rwlocktype_none; @@ -2076,7 +2076,7 @@ decrement_reference(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)) { + if (!rbtnode_erefs_decrement(rbtdb, node)) { return false; } @@ -2092,10 +2092,10 @@ decrement_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, * to 0 under the node write lock, so we regain the ref and try * again. */ - rbtnode_newref(rbtdb, node); + rbtnode_erefs_increment(rbtdb, node); NODE_UNLOCK(&nodelock->lock, isc_rwlocktype_read); NODE_LOCK(&nodelock->lock, isc_rwlocktype_write); - if (!rbtnode_decref(rbtdb, node)) { + if (!rbtnode_erefs_decrement(rbtdb, node)) { NODE_DOWNGRADE(&nodelock->lock); return false; } @@ -2160,9 +2160,9 @@ decrement_reference(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 decrement_reference(), check + * Since prune_tree() also calls dns__rbtnode_release(), check * the value of the 'pruning' parameter (which is only set to - * 'true' in the decrement_reference() call present in + * '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. @@ -2213,7 +2213,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 - * decrement_reference()). 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 @@ -2248,12 +2248,12 @@ prune_tree(isc_task_t *task, isc_event_t *event) { NODE_LOCK(&rbtdb->node_locks[locknum].lock, isc_rwlocktype_write); do { parent = node->parent; - decrement_reference(rbtdb, node, 0, isc_rwlocktype_write, - isc_rwlocktype_write, true); + dns__rbtnode_release(rbtdb, node, 0, isc_rwlocktype_write, + isc_rwlocktype_write, true); /* * Check whether the parent is an interior node. Note that it - * might have been one before the decrement_reference() 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. @@ -2275,7 +2275,8 @@ prune_tree(isc_task_t *task, isc_event_t *event) { * We need to gain a reference to the parent node * before decrementing it in the next iteration. */ - new_reference(rbtdb, parent, isc_rwlocktype_write); + dns__rbtnode_acquire(rbtdb, parent, + isc_rwlocktype_write); } else { parent = NULL; } @@ -2702,9 +2703,9 @@ closeversion(dns_db_t *db, dns_dbversion_t **versionp, bool commit) { if (rollback && !IGNORE(header)) { resign_insert(rbtdb, header->node->locknum, header); } - decrement_reference(rbtdb, header->node, least_serial, - isc_rwlocktype_write, isc_rwlocktype_none, - false); + dns__rbtnode_release(rbtdb, header->node, least_serial, + isc_rwlocktype_write, isc_rwlocktype_none, + false); NODE_UNLOCK(lock, isc_rwlocktype_write); } @@ -2722,7 +2723,7 @@ closeversion(dns_db_t *db, dns_dbversion_t **versionp, bool commit) { /* * We acquire a tree write lock here in order to make * sure that stale nodes will be removed in - * decrement_reference(). 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 event should be rare enough @@ -2753,8 +2754,9 @@ closeversion(dns_db_t *db, dns_dbversion_t **versionp, bool commit) { if (rollback) { rollback_node(rbtnode, serial); } - decrement_reference(rbtdb, rbtnode, least_serial, - isc_rwlocktype_write, tlock, false); + dns__rbtnode_release(rbtdb, rbtnode, least_serial, + isc_rwlocktype_write, tlock, + false); NODE_UNLOCK(lock, isc_rwlocktype_write); @@ -3047,7 +3049,7 @@ zone_zonecut_callback(dns_rbtnode_t *node, dns_name_t *name, void *arg) { * We increment the reference count on node to ensure that * search->zonecut_rdataset will still be valid later. */ - new_reference(search->rbtdb, node, isc_rwlocktype_read); + dns__rbtnode_acquire(search->rbtdb, node, isc_rwlocktype_read); search->zonecut = node; search->zonecut_rdataset = found; search->need_cleanup = true; @@ -3117,7 +3119,7 @@ bind_rdataset(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, rdatasetheader_t *header, return; } - new_reference(rbtdb, node, locktype); + dns__rbtnode_acquire(rbtdb, node, locktype); INSIST(rdataset->methods == NULL); /* We must be disassociated. */ @@ -3948,7 +3950,7 @@ again: foundname, NULL); if (result == ISC_R_SUCCESS) { if (nodep != NULL) { - new_reference( + dns__rbtnode_acquire( search->rbtdb, node, isc_rwlocktype_read); *nodep = node; @@ -4247,8 +4249,8 @@ found: * ensure that search->zonecut_rdataset will * still be valid later. */ - new_reference(search.rbtdb, node, - isc_rwlocktype_read); + dns__rbtnode_acquire(search.rbtdb, node, + isc_rwlocktype_read); search.zonecut = node; search.zonecut_rdataset = header; search.zonecut_sigrdataset = NULL; @@ -4431,7 +4433,8 @@ found: goto node_exit; } if (nodep != NULL) { - new_reference(search.rbtdb, node, isc_rwlocktype_read); + dns__rbtnode_acquire(search.rbtdb, node, + isc_rwlocktype_read); *nodep = node; } if ((search.rbtversion->secure == dns_db_secure && @@ -4515,7 +4518,8 @@ found: if (nodep != NULL) { if (!at_zonecut) { - new_reference(search.rbtdb, node, isc_rwlocktype_read); + dns__rbtnode_acquire(search.rbtdb, node, + isc_rwlocktype_read); } else { search.need_cleanup = false; } @@ -4551,8 +4555,8 @@ tree_exit: lock = &(search.rbtdb->node_locks[node->locknum].lock); NODE_LOCK(lock, isc_rwlocktype_read); - decrement_reference(search.rbtdb, node, 0, isc_rwlocktype_read, - isc_rwlocktype_none, false); + dns__rbtnode_release(search.rbtdb, node, 0, isc_rwlocktype_read, + isc_rwlocktype_none, false); NODE_UNLOCK(lock, isc_rwlocktype_read); } @@ -4670,7 +4674,7 @@ check_stale_header(dns_rbtnode_t *node, rdatasetheader_t *header, /* * header->down can be non-NULL if the * refcount has just decremented to 0 - * but decrement_reference() has not + * but dns__rbtnode_release() has not * performed clean_cache_node(), in * which case we need to purge the stale * headers first. @@ -4753,7 +4757,7 @@ cache_zonecut_callback(dns_rbtnode_t *node, dns_name_t *name, void *arg) { * We increment the reference count on node to ensure that * search->zonecut_rdataset will still be valid later. */ - new_reference(search->rbtdb, node, locktype); + dns__rbtnode_acquire(search->rbtdb, node, locktype); search->zonecut = node; search->zonecut_rdataset = dname_header; search->zonecut_sigrdataset = sigdname_header; @@ -4863,7 +4867,8 @@ find_deepest_zonecut(rbtdb_search_t *search, dns_rbtnode_t *node, } result = DNS_R_DELEGATION; if (nodep != NULL) { - new_reference(search->rbtdb, node, locktype); + dns__rbtnode_acquire(search->rbtdb, node, + locktype); *nodep = node; } bind_rdataset(search->rbtdb, node, found, search->now, @@ -5016,7 +5021,7 @@ find_coveringnsec(rbtdb_search_t *search, const dns_name_t *name, bind_rdataset(search->rbtdb, node, foundsig, now, locktype, sigrdataset); } - new_reference(search->rbtdb, node, locktype); + dns__rbtnode_acquire(search->rbtdb, node, locktype); dns_name_copy(fname, foundname); @@ -5277,7 +5282,8 @@ cache_find(dns_db_t *db, const dns_name_t *name, dns_dbversion_t *version, nsecheader != NULL) { if (nodep != NULL) { - new_reference(search.rbtdb, node, locktype); + dns__rbtnode_acquire(search.rbtdb, node, + locktype); *nodep = node; } bind_rdataset(search.rbtdb, node, nsecheader, @@ -5319,7 +5325,8 @@ cache_find(dns_db_t *db, const dns_name_t *name, dns_dbversion_t *version, */ if (nsheader != NULL) { if (nodep != NULL) { - new_reference(search.rbtdb, node, locktype); + dns__rbtnode_acquire(search.rbtdb, node, + locktype); *nodep = node; } bind_rdataset(search.rbtdb, node, nsheader, search.now, @@ -5351,7 +5358,7 @@ cache_find(dns_db_t *db, const dns_name_t *name, dns_dbversion_t *version, */ if (nodep != NULL) { - new_reference(search.rbtdb, node, locktype); + dns__rbtnode_acquire(search.rbtdb, node, locktype); *nodep = node; } @@ -5428,8 +5435,8 @@ tree_exit: lock = &(search.rbtdb->node_locks[node->locknum].lock); NODE_LOCK(lock, isc_rwlocktype_read); - decrement_reference(search.rbtdb, node, 0, isc_rwlocktype_read, - isc_rwlocktype_none, false); + dns__rbtnode_release(search.rbtdb, node, 0, isc_rwlocktype_read, + isc_rwlocktype_none, false); NODE_UNLOCK(lock, isc_rwlocktype_read); } @@ -5567,7 +5574,7 @@ cache_findzonecut(dns_db_t *db, const dns_name_t *name, unsigned int options, } if (nodep != NULL) { - new_reference(search.rbtdb, node, locktype); + dns__rbtnode_acquire(search.rbtdb, node, locktype); *nodep = node; } @@ -5641,8 +5648,8 @@ detachnode(dns_db_t *db, dns_dbnode_t **targetp) { NODE_LOCK(&nodelock->lock, isc_rwlocktype_read); - if (decrement_reference(rbtdb, node, 0, isc_rwlocktype_read, - isc_rwlocktype_none, false)) + if (dns__rbtnode_release(rbtdb, node, 0, isc_rwlocktype_read, + isc_rwlocktype_none, false)) { if (isc_refcount_current(&nodelock->references) == 0 && nodelock->exiting) @@ -6239,8 +6246,8 @@ resign_delete(dns_rbtdb_t *rbtdb, rbtdb_version_t *version, header->heap_index); header->heap_index = 0; if (version != NULL) { - new_reference(rbtdb, header->node, - isc_rwlocktype_write); + dns__rbtnode_acquire(rbtdb, header->node, + isc_rwlocktype_write); ISC_LIST_APPEND(version->resigned_list, header, link); } } @@ -7954,7 +7961,7 @@ getoriginnode(dns_db_t *db, dns_dbnode_t **nodep) { /* Note that the access to origin_node doesn't require a DB lock */ onode = (dns_rbtnode_t *)rbtdb->origin_node; if (onode != NULL) { - new_reference(rbtdb, onode, isc_rwlocktype_none); + dns__rbtnode_acquire(rbtdb, onode, isc_rwlocktype_none); *nodep = rbtdb->origin_node; } else { INSIST(IS_CACHE(rbtdb)); @@ -9198,8 +9205,8 @@ dereference_iter_node(rbtdb_dbiterator_t *rbtdbiter) { lock = &rbtdb->node_locks[node->locknum].lock; NODE_LOCK(lock, isc_rwlocktype_read); - decrement_reference(rbtdb, node, 0, isc_rwlocktype_read, - rbtdbiter->tree_locked, false); + dns__rbtnode_release(rbtdb, node, 0, isc_rwlocktype_read, + rbtdbiter->tree_locked, false); NODE_UNLOCK(lock, isc_rwlocktype_read); rbtdbiter->node = NULL; @@ -9238,8 +9245,9 @@ flush_deletions(rbtdb_dbiterator_t *rbtdbiter) { lock = &rbtdb->node_locks[node->locknum].lock; NODE_LOCK(lock, isc_rwlocktype_read); - decrement_reference(rbtdb, node, 0, isc_rwlocktype_read, - rbtdbiter->tree_locked, false); + dns__rbtnode_release(rbtdb, node, 0, + isc_rwlocktype_read, + rbtdbiter->tree_locked, false); NODE_UNLOCK(lock, isc_rwlocktype_read); } @@ -9722,7 +9730,7 @@ dbiterator_current(dns_dbiterator_t *iterator, dns_dbnode_t **nodep, result = ISC_R_SUCCESS; } - new_reference(rbtdb, node, isc_rwlocktype_none); + dns__rbtnode_acquire(rbtdb, node, isc_rwlocktype_none); *nodep = rbtdbiter->node; @@ -10567,14 +10575,14 @@ expire_header(dns_rbtdb_t *rbtdb, rdatasetheader_t *header, bool tree_locked, /* * 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 decrement_reference(). + * requirement of dns__rbtnode_release(). */ - new_reference(rbtdb, header->node, isc_rwlocktype_write); - decrement_reference(rbtdb, header->node, 0, - isc_rwlocktype_write, - tree_locked ? isc_rwlocktype_write - : isc_rwlocktype_none, - false); + dns__rbtnode_acquire(rbtdb, header->node, isc_rwlocktype_write); + dns__rbtnode_release(rbtdb, header->node, 0, + isc_rwlocktype_write, + tree_locked ? isc_rwlocktype_write + : isc_rwlocktype_none, + false); if (rbtdb->cachestats == NULL) { return;