From e91fbd8dea0a44ebf9dc40247861764f7b84da97 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Thu, 25 Jan 2024 10:19:00 +0100 Subject: [PATCH] Improve node reference counting QP database node data is not reference counted the same way RBT nodes were: in the RBT, node->references could be zero if the node was in the tree but was not in use by any caller, whereas in the QP trie, the database itself uses reference counting of nodes internally. this caused some subtle errors. in RBTDB, when the newref() function is called and the node reference count was zero, the node lock reference counter would also be incremented. in the QP trie, this can never happen - because as long as the node is in the database its reference count cannot be zero - and so the node lock reference counter was never incremented. reference counting will probably need to be refactored in more detail later; the node lock reference count may not be needed at all. but for now, as a temporary measure, we add a third reference counter, 'erefs' (external references), to the dns_qpdata structure. this is counted separately from the main reference counter, and should match the node reference count as it would have been in RBTDB. this change revealed a number of places where the node reference counter was being incremented on behalf of a caller without newref() being called; those were cleaned up as well. This is an adaptation of commit 3dd686261d2c4bcd15a96ebfea10baffa277732b --- lib/dns/qp-cachedb.c | 2 +- lib/dns/qpdb.c | 75 +++++++++++++++++++++----------------------- lib/dns/qpdb_p.h | 1 + 3 files changed, 37 insertions(+), 41 deletions(-) diff --git a/lib/dns/qp-cachedb.c b/lib/dns/qp-cachedb.c index 1d8d03275e..e5c351453f 100644 --- a/lib/dns/qp-cachedb.c +++ b/lib/dns/qp-cachedb.c @@ -1562,7 +1562,7 @@ dns__qpcache_expireheader(dns_slabheader_t *header, dns__qpdb_mark(header, DNS_SLABHEADERATTR_ANCIENT); QPDB_HEADERNODE(header)->dirty = 1; - if (isc_refcount_current(&QPDB_HEADERNODE(header)->references) == 0) { + if (isc_refcount_current(&QPDB_HEADERNODE(header)->erefs) == 0) { isc_rwlocktype_t nlocktype = isc_rwlocktype_write; dns_qpdb_t *qpdb = (dns_qpdb_t *)header->db; diff --git a/lib/dns/qpdb.c b/lib/dns/qpdb.c index f41f4f7375..8b87c59c4d 100644 --- a/lib/dns/qpdb.c +++ b/lib/dns/qpdb.c @@ -775,14 +775,10 @@ add_changed(dns_slabheader_t *header, if (changed != NULL) { dns_qpdata_t *node = (dns_qpdata_t *)header->node; - uint_fast32_t refs = isc_refcount_increment(&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 + + dns__qpdb_newref(qpdb, node, + isc_rwlocktype_none DNS__DB_FLARG_PASS); + changed->node = node; changed->dirty = false; ISC_LIST_INITANDAPPEND(version->changed_list, changed, link); @@ -1105,16 +1101,18 @@ dns__qpdb_newref(dns_qpdb_t *qpdb, dns_qpdata_t *node, ISC_LIST_UNLINK(qpdb->deadnodes[node->locknum], node, deadlink); } - refs = isc_refcount_increment0(&node->references); + dns_qpdata_ref(node); + refs = isc_refcount_increment0(&node->erefs); + #if DNS_DB_NODETRACE - fprintf(stderr, "incr:node:%s:%s:%u:%p->references = %" PRIuFAST32 "\n", + fprintf(stderr, "incr:node:%s:%s:%u:%p->erefs = %" PRIuFAST32 "\n", func, file, line, node, refs + 1); #else UNUSED(refs); #endif if (refs == 0) { - /* this is the first reference to the node */ + /* this is the first external reference to the node */ refs = isc_refcount_increment0( &qpdb->node_locks[node->locknum].references); #if DNS_DB_NODETRACE @@ -1263,11 +1261,13 @@ reactivate_node(dns_qpdb_t *qpdb, dns_qpdata_t *node, * threads are decreasing the reference to zero simultaneously and at least * one of them is going to free the node. * - * This function returns true if and only if the node reference decreases - * to zero. + * 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 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.) */ bool dns__qpdb_decref(dns_qpdb_t *qpdb, dns_qpdata_t *node, uint32_t least_serial, @@ -1291,10 +1291,11 @@ dns__qpdb_decref(dns_qpdb_t *qpdb, dns_qpdata_t *node, uint32_t least_serial, /* Handle easy and typical case first. */ if (!node->dirty && KEEP_NODE(node, qpdb, locked)) { - refs = isc_refcount_decrement(&node->references); + refs = isc_refcount_decrement(&node->erefs); + #if DNS_DB_NODETRACE fprintf(stderr, - "decr:node:%s:%s:%u:%p->references = %" PRIuFAST32 "\n", + "decr:node:%s:%s:%u:%p->erefs = %" PRIuFAST32 "\n", func, file, line, node, refs - 1); #else UNUSED(refs); @@ -1309,10 +1310,13 @@ dns__qpdb_decref(dns_qpdb_t *qpdb, dns_qpdata_t *node, uint32_t least_serial, #else UNUSED(refs); #endif - return (true); + no_reference = true; } else { - return (false); + no_reference = false; } + + dns_qpdata_unref(node); + return (no_reference); } /* Upgrade the lock? */ @@ -1320,17 +1324,19 @@ dns__qpdb_decref(dns_qpdb_t *qpdb, dns_qpdata_t *node, uint32_t least_serial, NODE_FORCEUPGRADE(&nodelock->lock, nlocktypep); } - refs = isc_refcount_decrement(&node->references); + refs = isc_refcount_decrement(&node->erefs); #if DNS_DB_NODETRACE - fprintf(stderr, "decr:node:%s:%s:%u:%p->references = %" PRIuFAST32 "\n", + fprintf(stderr, "decr:node:%s:%s:%u:%p->erefs = %" PRIuFAST32 "\n", func, file, line, node, refs - 1); -#else - UNUSED(refs); #endif + if (refs > 1) { + dns_qpdata_unref(node); return (false); } + INSIST(refs == 1); + if (node->dirty) { if (IS_CACHE(qpdb)) { clean_cache_node(qpdb, node); @@ -1358,6 +1364,7 @@ dns__qpdb_decref(dns_qpdb_t *qpdb, dns_qpdata_t *node, uint32_t least_serial, * we only do a trylock. */ /* We are allowed to upgrade the tree lock */ + switch (*tlocktypep) { case isc_rwlocktype_write: result = ISC_R_SUCCESS; @@ -1438,6 +1445,7 @@ restore_locks: TREE_UNLOCK(&qpdb->tree_lock, tlocktypep); } + dns_qpdata_unref(node); return (no_reference); } @@ -2160,15 +2168,10 @@ dns__qpdb_attachnode(dns_db_t *db, dns_dbnode_t *source, REQUIRE(VALID_QPDB((dns_qpdb_t *)db)); REQUIRE(targetp != NULL && *targetp == NULL); + dns_qpdb_t *qpdb = (dns_qpdb_t *)db; dns_qpdata_t *node = (dns_qpdata_t *)source; - uint_fast32_t refs = isc_refcount_increment(&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 + dns__qpdb_newref(qpdb, node, isc_rwlocktype_none DNS__DB_FLARG_PASS); *targetp = source; } @@ -2282,7 +2285,6 @@ dns__qpdb_allrdatasets(dns_db_t *db, dns_dbnode_t *node, dns_qpdata_t *qpnode = (dns_qpdata_t *)node; dns_qpdb_version_t *rbtversion = version; qpdb_rdatasetiter_t *iterator = NULL; - uint_fast32_t refs; REQUIRE(VALID_QPDB(qpdb)); @@ -2312,17 +2314,10 @@ dns__qpdb_allrdatasets(dns_db_t *db, dns_dbnode_t *node, iterator->common.version = (dns_dbversion_t *)rbtversion; iterator->common.options = options; iterator->common.now = now; - - refs = isc_refcount_increment(&qpnode->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 - iterator->current = NULL; + dns__qpdb_newref(qpdb, qpnode, isc_rwlocktype_none DNS__DB_FLARG_PASS); + *iteratorp = (dns_rdatasetiter_t *)iterator; return (ISC_R_SUCCESS); diff --git a/lib/dns/qpdb_p.h b/lib/dns/qpdb_p.h index ab0d589f24..4239419cbe 100644 --- a/lib/dns/qpdb_p.h +++ b/lib/dns/qpdb_p.h @@ -135,6 +135,7 @@ struct dns_qpdata { uint8_t : 0; /* end of bitfields c/o node lock */ uint16_t locknum; /* note that this is not in the bitfield */ isc_refcount_t references; + isc_refcount_t erefs; /*@}*/ };