From 77e7eac54c51c5e23bf6e0349237442ea105d02a Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Fri, 17 Feb 2023 13:04:12 -0800 Subject: [PATCH] enable detailed db tracing move database attach/detach functions to db.c, instead of requiring them to be implemented for every database type. instead, they must implement a 'destroy' function that is called when references go to zero. this enables us to use ISC_REFCOUNT_IMPL for databases, with detailed tracing enabled by setting DNS_DB_TRACE to 1. --- bin/tests/system/dyndb/driver/db.c | 29 +++-------------- lib/dns/db.c | 40 ++++++++--------------- lib/dns/dnsrps.c | 46 +++++++-------------------- lib/dns/include/dns/db.h | 51 ++++++++++-------------------- lib/dns/rbtdb.c | 41 ++++++------------------ lib/dns/sdb.c | 41 ++++-------------------- lib/dns/sdlz.c | 40 ++++------------------- 7 files changed, 69 insertions(+), 219 deletions(-) diff --git a/bin/tests/system/dyndb/driver/db.c b/bin/tests/system/dyndb/driver/db.c index 05ae32b618..a712382100 100644 --- a/bin/tests/system/dyndb/driver/db.c +++ b/bin/tests/system/dyndb/driver/db.c @@ -64,7 +64,6 @@ struct sampledb { dns_db_t common; - isc_refcount_t refs; sample_instance_t *inst; /* @@ -93,36 +92,17 @@ sample_name_fromnode(dns_dbnode_t *node, dns_name_t *name) { } static void -attach(dns_db_t *source, dns_db_t **targetp) { - sampledb_t *sampledb = (sampledb_t *)source; +destroy(dns_db_t *db) { + sampledb_t *sampledb = (sampledb_t *)db; REQUIRE(VALID_SAMPLEDB(sampledb)); - isc_refcount_increment(&sampledb->refs); - *targetp = source; -} - -static void -free_sampledb(sampledb_t *sampledb) { - REQUIRE(VALID_SAMPLEDB(sampledb)); - dns_db_detach(&sampledb->rbtdb); dns_name_free(&sampledb->common.origin, sampledb->common.mctx); isc_mem_putanddetach(&sampledb->common.mctx, sampledb, sizeof(*sampledb)); } -static void -detach(dns_db_t **dbp) { - REQUIRE(dbp != NULL && VALID_SAMPLEDB((sampledb_t *)(*dbp))); - sampledb_t *sampledb = (sampledb_t *)(*dbp); - *dbp = NULL; - - if (isc_refcount_decrement(&sampledb->refs) == 1) { - free_sampledb(sampledb); - } -} - static void currentversion(dns_db_t *db, dns_dbversion_t **versionp) { sampledb_t *sampledb = (sampledb_t *)db; @@ -506,8 +486,7 @@ hashsize(dns_db_t *db) { * determine which implementation of dns_db_*() function to call. */ static dns_dbmethods_t sampledb_methods = { - .attach = attach, - .detach = detach, + .destroy = destroy, .currentversion = currentversion, .newversion = newversion, .attachversion = attachversion, @@ -699,7 +678,7 @@ create_db(isc_mem_t *mctx, const dns_name_t *origin, dns_dbtype_t type, CHECK(dns_name_dupwithoffsets(origin, mctx, &sampledb->common.origin)); - isc_refcount_init(&sampledb->refs, 1); + isc_refcount_init(&sampledb->common.references, 1); /* Translate instance name to instance pointer. */ sampledb->inst = driverarg; diff --git a/lib/dns/db.c b/lib/dns/db.c index c3e6204a30..de2e26a9e6 100644 --- a/lib/dns/db.c +++ b/lib/dns/db.c @@ -104,7 +104,7 @@ isc_result_t dns_db_create(isc_mem_t *mctx, const char *db_type, const dns_name_t *origin, dns_dbtype_t type, dns_rdataclass_t rdclass, unsigned int argc, char *argv[], dns_db_t **dbp) { - dns_dbimplementation_t *impinfo; + dns_dbimplementation_t *impinfo = NULL; isc_once_do(&once, initialize); @@ -122,6 +122,11 @@ dns_db_create(isc_mem_t *mctx, const char *db_type, const dns_name_t *origin, result = ((impinfo->create)(mctx, origin, type, rdclass, argc, argv, impinfo->driverarg, dbp)); RWUNLOCK(&implock, isc_rwlocktype_read); + +#if DNS_DB_TRACE + fprintf(stderr, "dns_db_create:%s:%s:%d:%p->references = 1\n", + __func__, __FILE__, __LINE__ + 1, *dbp); +#endif return (result); } @@ -133,33 +138,16 @@ dns_db_create(isc_mem_t *mctx, const char *db_type, const dns_name_t *origin, return (ISC_R_NOTFOUND); } -void -dns_db_attach(dns_db_t *source, dns_db_t **targetp) { - /* - * Attach *targetp to source. - */ - - REQUIRE(DNS_DB_VALID(source)); - REQUIRE(targetp != NULL && *targetp == NULL); - - (source->methods->attach)(source, targetp); - - ENSURE(*targetp == source); +static void +dns__db_destroy(dns_db_t *db) { + (db->methods->destroy)(db); } -void -dns_db_detach(dns_db_t **dbp) { - /* - * Detach *dbp from its database. - */ - - REQUIRE(dbp != NULL); - REQUIRE(DNS_DB_VALID(*dbp)); - - ((*dbp)->methods->detach)(dbp); - - ENSURE(*dbp == NULL); -} +#if DNS_DB_TRACE +ISC_REFCOUNT_TRACE_IMPL(dns_db, dns__db_destroy); +#else +ISC_REFCOUNT_IMPL(dns_db, dns__db_destroy); +#endif bool dns_db_iscache(dns_db_t *db) { diff --git a/lib/dns/dnsrps.c b/lib/dns/dnsrps.c index 5dede17450..1c3800bb98 100644 --- a/lib/dns/dnsrps.c +++ b/lib/dns/dnsrps.c @@ -255,7 +255,7 @@ isc_result_t dns_dnsrps_rewrite_init(librpz_emsg_t *emsg, dns_rpz_st_t *st, dns_rpz_zones_t *rpzs, const dns_name_t *qname, isc_mem_t *mctx, bool have_rd) { - rpsdb_t *rpsdb; + rpsdb_t *rpsdb = NULL; rpsdb = isc_mem_get(mctx, sizeof(*rpsdb)); *rpsdb = (rpsdb_t){ @@ -263,9 +263,9 @@ dns_dnsrps_rewrite_init(librpz_emsg_t *emsg, dns_rpz_st_t *st, .methods = &rpsdb_db_methods, .rdclass = dns_rdataclass_in, }, - .ref_cnt = 1, .qname = qname, }; + isc_refcount_init(&rpsdb->common.refcount, 1); if (!librpz->rsp_create(emsg, &rpsdb->rsp, NULL, rpzs->rps_client, have_rd, false)) @@ -362,35 +362,15 @@ dns_dnsrps_type2trig(dns_rpz_type_t type) { } static void -rpsdb_attach(dns_db_t *source, dns_db_t **targetp) { - rpsdb_t *rpsdb = (rpsdb_t *)source; +rpsdb_destroy(dns_db_t *db) { + rpsdb_t *rpsdb = (rpsdb_t *)db; REQUIRE(VALID_RPSDB(rpsdb)); - /* - * Use a simple count because only one thread uses any single rpsdb_t - */ - ++rpsdb->ref_cnt; - *targetp = source; -} - -static void -rpsdb_detach(dns_db_t **dbp) { - rpsdb_t *rpsdb = (rpsdb_t *)*dbp; - - REQUIRE(VALID_RPSDB(rpsdb)); - REQUIRE(rpsdb->ref_cnt > 0); - *dbp = NULL; - /* - * Simple count because only one thread uses a rpsdb_t. - */ - if (--rpsdb->ref_cnt != 0) { - return; - } - librpz->rsp_detach(&rpsdb->rsp); + isc_refcount_destroy(&rpsdb->common.refcount); rpsdb->common.impmagic = 0; isc_mem_putanddetach(&rpsdb->common.mctx, rpsdb, sizeof(*rpsdb)); } @@ -403,10 +383,7 @@ rpsdb_attachnode(dns_db_t *db, dns_dbnode_t *source, dns_dbnode_t **targetp) { REQUIRE(targetp != NULL && *targetp == NULL); REQUIRE(source == &rpsdb->origin_node || source == &rpsdb->data_node); - /* - * Simple count because only one thread uses a rpsdb_t. - */ - ++rpsdb->ref_cnt; + isc_refcount_increment(&rpsdb->common.refcount); *targetp = source; } @@ -419,14 +396,14 @@ rpsdb_detachnode(dns_db_t *db, dns_dbnode_t **targetp) { *targetp == &rpsdb->data_node); *targetp = NULL; - rpsdb_detach(&db); + dns_db_detach(&db); } static isc_result_t rpsdb_findnode(dns_db_t *db, const dns_name_t *name, bool create, dns_dbnode_t **nodep) { rpsdb_t *rpsdb = (rpsdb_t *)db; - dns_db_t *dbp; + dns_db_t *dbp = NULL; REQUIRE(VALID_RPSDB(rpsdb)); REQUIRE(nodep != NULL && *nodep == NULL); @@ -442,8 +419,8 @@ rpsdb_findnode(dns_db_t *db, const dns_name_t *name, bool create, } else { *nodep = &rpsdb->data_node; } - dbp = NULL; - rpsdb_attach(db, &dbp); + + dns_db_attach(db, &dbp); return (ISC_R_SUCCESS); } @@ -931,8 +908,7 @@ rpsdb_rdatasetiter_current(dns_rdatasetiter_t *iterator, } static dns_dbmethods_t rpsdb_db_methods = { - .attach = rpsdb_attach, - .detach = rpsdb_detach, + .destroy = rpsdb_destroy, .findnode = rpsdb_findnode, .find = rpsdb_finddb, .attachnode = rpsdb_attachnode, diff --git a/lib/dns/include/dns/db.h b/lib/dns/include/dns/db.h index 3e974cff10..de41b12212 100644 --- a/lib/dns/include/dns/db.h +++ b/lib/dns/include/dns/db.h @@ -47,6 +47,9 @@ ***** Imports *****/ +/* Define to 1 for detailed reference tracing */ +#undef DNS_DB_TRACE + #include #include @@ -74,8 +77,7 @@ extern unsigned int dns_pps; *****/ typedef struct dns_dbmethods { - void (*attach)(dns_db_t *source, dns_db_t **targetp); - void (*detach)(dns_db_t **dbp); + void (*destroy)(dns_db_t *db); isc_result_t (*beginload)(dns_db_t *db, dns_rdatacallbacks_t *callbacks); isc_result_t (*endload)(dns_db_t *db, dns_rdatacallbacks_t *callbacks); @@ -213,6 +215,7 @@ struct dns_db { dns_rdataclass_t rdclass; dns_name_t origin; isc_mem_t *mctx; + isc_refcount_t references; ISC_LIST(dns_dbonupdatelistener_t) update_listeners; }; @@ -310,6 +313,17 @@ struct dns_dbonupdatelistener { *** Basic DB Methods ***/ +#if DNS_DB_TRACE +#define dns_db_ref(ptr) dns_db__ref(ptr, __func__, __FILE__, __LINE__) +#define dns_db_unref(ptr) dns_db__unref(ptr, __func__, __FILE__, __LINE__) +#define dns_db_attach(ptr, ptrp) \ + dns_db__attach(ptr, ptrp, __func__, __FILE__, __LINE__) +#define dns_db_detach(ptrp) dns_db__detach(ptrp, __func__, __FILE__, __LINE__) +ISC_REFCOUNT_TRACE_DECL(dns_db); +#else +ISC_REFCOUNT_DECL(dns_db); +#endif + isc_result_t dns_db_create(isc_mem_t *mctx, const char *db_type, const dns_name_t *origin, dns_dbtype_t type, dns_rdataclass_t rdclass, unsigned int argc, @@ -349,39 +363,6 @@ dns_db_create(isc_mem_t *mctx, const char *db_type, const dns_name_t *origin, * specified. */ -void -dns_db_attach(dns_db_t *source, dns_db_t **targetp); -/*%< - * Attach *targetp to source. - * - * Requires: - * - * \li 'source' is a valid database. - * - * \li 'targetp' points to a NULL dns_db_t *. - * - * Ensures: - * - * \li *targetp is attached to source. - */ - -void -dns_db_detach(dns_db_t **dbp); -/*%< - * Detach *dbp from its database. - * - * Requires: - * - * \li 'dbp' points to a valid database. - * - * Ensures: - * - * \li *dbp is NULL. - * - * \li If '*dbp' is the last reference to the database, - * all resources used by the database will be freed - */ - bool dns_db_iscache(dns_db_t *db); /*%< diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index bb8dfe75eb..48267693b6 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -501,7 +501,6 @@ struct dns_rbtdb { isc_stats_t *gluecachestats; /* zone DB only */ /* Locked by lock. */ unsigned int active; - isc_refcount_t references; unsigned int attributes; rbtdb_serial_t current_serial; rbtdb_serial_t least_serial; @@ -811,17 +810,6 @@ static atomic_uint_fast16_t init_count = 0; * DB Routines */ -static void -attach(dns_db_t *source, dns_db_t **targetp) { - dns_rbtdb_t *rbtdb = (dns_rbtdb_t *)source; - - REQUIRE(VALID_RBTDB(rbtdb)); - - isc_refcount_increment(&rbtdb->references); - - *targetp = source; -} - static void free_rbtdb_callback(void *arg) { dns_rbtdb_t *rbtdb = (dns_rbtdb_t *)arg; @@ -1170,7 +1158,7 @@ free_rbtdb(dns_rbtdb_t *rbtdb, bool log) { isc_mem_put(rbtdb->common.mctx, rbtdb->node_locks, rbtdb->node_lock_count * sizeof(rbtdb_nodelock_t)); TREE_DESTROYLOCK(&rbtdb->tree_lock); - isc_refcount_destroy(&rbtdb->references); + isc_refcount_destroy(&rbtdb->common.references); if (rbtdb->loop != NULL) { isc_loop_detach(&rbtdb->loop); } @@ -1248,14 +1236,8 @@ maybe_free_rbtdb(dns_rbtdb_t *rbtdb) { } static void -detach(dns_db_t **dbp) { - REQUIRE(dbp != NULL && VALID_RBTDB((dns_rbtdb_t *)(*dbp))); - dns_rbtdb_t *rbtdb = (dns_rbtdb_t *)(*dbp); - *dbp = NULL; - - if (isc_refcount_decrement(&rbtdb->references) == 1) { - maybe_free_rbtdb(rbtdb); - } +destroy(dns_db_t *db) { + maybe_free_rbtdb((dns_rbtdb_t *)db); } static void @@ -1871,7 +1853,7 @@ send_to_prune_tree(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, prune_t *prune = isc_mem_get(rbtdb->common.mctx, sizeof(*prune)); *prune = (prune_t){ .node = node }; - attach((dns_db_t *)rbtdb, &prune->db); + dns_db_attach((dns_db_t *)rbtdb, &prune->db); new_reference(rbtdb, node, locktype); isc_async_run(rbtdb->loop, prune_tree, prune); @@ -2198,7 +2180,7 @@ prune_tree(void *arg) { NODE_UNLOCK(&rbtdb->node_locks[locknum].lock, &nlocktype); TREE_UNLOCK(&rbtdb->tree_lock, &tlocktype); - detach((dns_db_t **)&rbtdb); + dns_db_detach((dns_db_t **)&rbtdb); } static void @@ -2409,8 +2391,7 @@ cleanup_dead_nodes_callback(void *arg) { if (again) { isc_async_run(rbtdb->loop, cleanup_dead_nodes_callback, rbtdb); } else { - if (isc_refcount_decrement(&rbtdb->references) == 1) { - (void)isc_refcount_current(&rbtdb->references); + if (isc_refcount_decrement(&rbtdb->common.references) == 1) { maybe_free_rbtdb(rbtdb); } } @@ -2671,7 +2652,7 @@ closeversion(dns_db_t *db, dns_dbversion_t **versionp, bool commit) { sizeof(*changed)); } if (rbtdb->loop != NULL) { - isc_refcount_increment(&rbtdb->references); + isc_refcount_increment(&rbtdb->common.references); isc_async_run(rbtdb->loop, cleanup_dead_nodes_callback, rbtdb); } else { @@ -8065,8 +8046,7 @@ getservestalerefresh(dns_db_t *db, uint32_t *interval) { } static dns_dbmethods_t zone_methods = { - .attach = attach, - .detach = detach, + .destroy = destroy, .beginload = beginload, .endload = endload, .dump = dump, @@ -8104,8 +8084,7 @@ static dns_dbmethods_t zone_methods = { }; static dns_dbmethods_t cache_methods = { - .attach = attach, - .detach = detach, + .destroy = destroy, .beginload = beginload, .endload = endload, .dump = dump, @@ -8350,7 +8329,7 @@ dns_rbtdb_create(isc_mem_t *mctx, const dns_name_t *origin, dns_dbtype_t type, /* * Misc. Initialization. */ - isc_refcount_init(&rbtdb->references, 1); + isc_refcount_init(&rbtdb->common.references, 1); rbtdb->attributes = 0; rbtdb->loop = NULL; rbtdb->serve_stale_ttl = 0; diff --git a/lib/dns/sdb.c b/lib/dns/sdb.c index 7cbdbe6cfa..d67c27a14e 100644 --- a/lib/dns/sdb.c +++ b/lib/dns/sdb.c @@ -56,9 +56,6 @@ struct dns_sdb { char *zone; dns_sdbimplementation_t *implementation; void *dbdata; - - /* Atomic */ - isc_refcount_t references; }; struct dns_sdblookup { @@ -360,21 +357,11 @@ dns_sdb_putsoa(dns_sdblookup_t *lookup, const char *mname, const char *rname, */ static void -attach(dns_db_t *source, dns_db_t **targetp) { - dns_sdb_t *sdb = (dns_sdb_t *)source; - - REQUIRE(VALID_SDB(sdb)); - - isc_refcount_increment(&sdb->references); - - *targetp = source; -} - -static void -destroy(dns_sdb_t *sdb) { +destroy(dns_db_t *db) { + dns_sdb_t *sdb = (dns_sdb_t *)db; dns_sdbimplementation_t *imp = sdb->implementation; - isc_refcount_destroy(&sdb->references); + isc_refcount_destroy(&sdb->common.references); if (imp != NULL && imp->methods->destroy != NULL) { LOCK(&sdb->implementation->driverlock); @@ -392,19 +379,6 @@ destroy(dns_sdb_t *sdb) { isc_mem_putanddetach(&sdb->common.mctx, sdb, sizeof(dns_sdb_t)); } -static void -detach(dns_db_t **dbp) { - dns_sdb_t *sdb = (dns_sdb_t *)(*dbp); - - REQUIRE(VALID_SDB(sdb)); - - *dbp = NULL; - - if (isc_refcount_decrement(&sdb->references) == 1) { - destroy(sdb); - } -} - static void currentversion(dns_db_t *db, dns_dbversion_t **versionp) { REQUIRE(versionp != NULL && *versionp == NULL); @@ -444,7 +418,7 @@ createnode(dns_sdb_t *sdb, dns_sdbnode_t **nodep) { node = isc_mem_get(sdb->common.mctx, sizeof(dns_sdbnode_t)); node->sdb = NULL; - attach((dns_db_t *)sdb, (dns_db_t **)&node->sdb); + dns_db_attach((dns_db_t *)sdb, (dns_db_t **)&node->sdb); ISC_LIST_INIT(node->lists); ISC_LIST_INIT(node->buffers); ISC_LINK_INIT(node, link); @@ -494,7 +468,7 @@ destroynode(dns_sdbnode_t *node) { node->magic = 0; isc_mem_put(mctx, node, sizeof(dns_sdbnode_t)); - detach((dns_db_t **)(void *)&sdb); + dns_db_detach((dns_db_t **)(void *)&sdb); } static isc_result_t @@ -878,8 +852,7 @@ ispersistent(dns_db_t *db) { } static dns_dbmethods_t sdb_methods = { - .attach = attach, - .detach = detach, + .destroy = destroy, .currentversion = currentversion, .attachversion = attachversion, .closeversion = closeversion, @@ -945,7 +918,7 @@ create(isc_mem_t *mctx, const dns_name_t *origin, dns_dbtype_t type, } } - isc_refcount_init(&sdb->references, 1); + isc_refcount_init(&sdb->common.references, 1); sdb->common.magic = DNS_DB_MAGIC; sdb->common.impmagic = SDB_MAGIC; diff --git a/lib/dns/sdlz.c b/lib/dns/sdlz.c index 0c1031f113..870dad3522 100644 --- a/lib/dns/sdlz.c +++ b/lib/dns/sdlz.c @@ -99,9 +99,6 @@ struct dns_sdlz_db { void *dbdata; dns_sdlzimplementation_t *dlzimp; - /* Atomic */ - isc_refcount_t references; - /* Locked */ dns_dbversion_t *future_version; int dummy_version; @@ -300,40 +297,18 @@ static dns_rdatasetitermethods_t rdatasetiter_methods = { */ static void -attach(dns_db_t *source, dns_db_t **targetp) { - dns_sdlz_db_t *sdlz = (dns_sdlz_db_t *)source; +destroy(dns_db_t *db) { + dns_sdlz_db_t *sdlz = (dns_sdlz_db_t *)db; - REQUIRE(VALID_SDLZDB(sdlz)); - - isc_refcount_increment(&sdlz->references); - - *targetp = source; -} - -static void -destroy(dns_sdlz_db_t *sdlz) { sdlz->common.magic = 0; sdlz->common.impmagic = 0; dns_name_free(&sdlz->common.origin, sdlz->common.mctx); - isc_refcount_destroy(&sdlz->references); + isc_refcount_destroy(&sdlz->common.references); isc_mem_putanddetach(&sdlz->common.mctx, sdlz, sizeof(dns_sdlz_db_t)); } -static void -detach(dns_db_t **dbp) { - dns_sdlz_db_t *sdlz = (dns_sdlz_db_t *)(*dbp); - - REQUIRE(VALID_SDLZDB(sdlz)); - - *dbp = NULL; - - if (isc_refcount_decrement(&sdlz->references) == 1) { - destroy(sdlz); - } -} - static void currentversion(dns_db_t *db, dns_dbversion_t **versionp) { dns_sdlz_db_t *sdlz = (dns_sdlz_db_t *)db; @@ -418,7 +393,7 @@ createnode(dns_sdlz_db_t *sdlz, dns_sdlznode_t **nodep) { node = isc_mem_get(sdlz->common.mctx, sizeof(dns_sdlznode_t)); node->sdlz = NULL; - attach((dns_db_t *)sdlz, (dns_db_t **)&node->sdlz); + dns_db_attach((dns_db_t *)sdlz, (dns_db_t **)&node->sdlz); ISC_LIST_INIT(node->lists); ISC_LIST_INIT(node->buffers); ISC_LINK_INIT(node, link); @@ -471,7 +446,7 @@ destroynode(dns_sdlznode_t *node) { node->magic = 0; isc_mem_put(mctx, node, sizeof(dns_sdlznode_t)); db = &sdlz->common; - detach(&db); + dns_db_detach(&db); } static isc_result_t @@ -1179,8 +1154,7 @@ getoriginnode(dns_db_t *db, dns_dbnode_t **nodep) { } static dns_dbmethods_t sdlzdb_methods = { - .attach = attach, - .detach = detach, + .destroy = destroy, .currentversion = currentversion, .newversion = newversion, .attachversion = attachversion, @@ -1414,7 +1388,7 @@ dns_sdlzcreateDBP(isc_mem_t *mctx, void *driverarg, void *dbdata, goto mem_cleanup; } - isc_refcount_init(&sdlzdb->references, 1); + isc_refcount_init(&sdlzdb->common.references, 1); /* attach to the memory context */ isc_mem_attach(mctx, &sdlzdb->common.mctx);