upstream/bind9
1
0
Fork 0
mirror of https://github.com/isc-projects/bind9.git synced 2026-02-28 12:31:29 -05:00
Code Issues Projects Releases Packages Wiki Activity Actions

Improve node reference counting

Browse source 
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
This commit is contained in:
Matthijs Mekking 2024-01-25 10:19:00 +01:00
parent 815f54ec27
commit e91fbd8dea
3 changed files with 37 additions and 41 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
lib/dns/qp-cachedb.c
View file

@ -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;

75
lib/dns/qpdb.c
View file

@ -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);

1
lib/dns/qpdb_p.h
View file

@ -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;
/*@}*/
};