From f7fa139fe71ece2623a77cddff5613f91b1a08c5 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Mon, 9 May 2022 12:55:07 -0700 Subject: [PATCH 01/11] maybe_cancel_validators() is always called locked there's no longer any need for a parameter to specify whether the function is called while holding the bucket lock, because all unlocked uses have been removed. --- lib/dns/resolver.c | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 2baa32cf51..c75505fa29 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -653,7 +653,7 @@ ncache_adderesult(dns_message_t *message, dns_db_t *cache, dns_dbnode_t *node, static void validated(isc_task_t *task, isc_event_t *event); static void -maybe_cancel_validators(fetchctx_t *fctx, bool locked); +maybe_cancel_validators(fetchctx_t *fctx); static void add_bad(fetchctx_t *fctx, dns_message_t *rmessage, dns_adbaddrinfo_t *addrinfo, isc_result_t reason, badnstype_t badtype); @@ -4247,7 +4247,7 @@ resume_qmin(isc_task_t *task, isc_event_t *event) { LOCK(&res->buckets[bucketnum].lock); if (SHUTTINGDOWN(fctx)) { - maybe_cancel_validators(fctx, true); + maybe_cancel_validators(fctx); UNLOCK(&res->buckets[bucketnum].lock); fctx_detach(&fctx); return; @@ -5237,27 +5237,21 @@ clone_results(fetchctx_t *fctx) { /* * Cancel validators associated with '*fctx' if it is ready to be * destroyed (i.e., no queries waiting for it and no pending ADB finds). + * Caller must hold fctx bucket lock. * * Requires: * '*fctx' is shutting down. */ static void -maybe_cancel_validators(fetchctx_t *fctx, bool locked) { - unsigned int bucketnum; - dns_resolver_t *res = fctx->res; - dns_validator_t *validator, *next_validator; - - bucketnum = fctx->bucketnum; - if (!locked) { - LOCK(&res->buckets[bucketnum].lock); - } +maybe_cancel_validators(fetchctx_t *fctx) { + dns_validator_t *validator = NULL, *next_validator = NULL; REQUIRE(SHUTTINGDOWN(fctx)); if (atomic_load_acquire(&fctx->pending) != 0 || atomic_load_acquire(&fctx->nqueries) != 0) { - goto unlock; + return; } for (validator = ISC_LIST_HEAD(fctx->validators); validator != NULL; @@ -5266,10 +5260,6 @@ maybe_cancel_validators(fetchctx_t *fctx, bool locked) { next_validator = ISC_LIST_NEXT(validator, link); dns_validator_cancel(validator); } -unlock: - if (!locked) { - UNLOCK(&res->buckets[bucketnum].lock); - } } /* @@ -5707,7 +5697,7 @@ validated(isc_task_t *task, isc_event_t *event) { */ dns_db_detachnode(fctx->cache, &node); if (SHUTTINGDOWN(fctx)) { - maybe_cancel_validators(fctx, true); + maybe_cancel_validators(fctx); } UNLOCK(&res->buckets[bucketnum].lock); fctx_detach(&fctx); @@ -7314,7 +7304,7 @@ resume_dslookup(isc_task_t *task, isc_event_t *event) { LOCK(&res->buckets[fctx->bucketnum].lock); if (SHUTTINGDOWN(fctx)) { - maybe_cancel_validators(fctx, true); + maybe_cancel_validators(fctx); UNLOCK(&res->buckets[fctx->bucketnum].lock); if (dns_rdataset_isassociated(frdataset)) { From fe1fa8dc88a6017921fe2237c9c62d385b595ef3 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 10 May 2022 12:48:31 -0700 Subject: [PATCH 02/11] minor view refactoring - eliminate dns_view_flushanddetach(), which was only called from one place; instead, we now call a function dns_view_flushonshutdown() which sets the view up to flush zones when it is detached normally with dns_view_detach(). - cleaned up code in dns_view_create(). --- bin/named/server.c | 7 +- lib/dns/include/dns/request.h | 2 - lib/dns/include/dns/view.h | 45 ++++--- lib/dns/view.c | 238 ++++++++++------------------------ 4 files changed, 99 insertions(+), 193 deletions(-) diff --git a/bin/named/server.c b/bin/named/server.c index b5a2118e20..043205b4fa 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -9917,11 +9917,8 @@ shutdown_server(isc_task_t *task, isc_event_t *event) { view = view_next) { view_next = ISC_LIST_NEXT(view, link); ISC_LIST_UNLINK(server->viewlist, view, link); - if (flush) { - dns_view_flushanddetach(&view); - } else { - dns_view_detach(&view); - } + dns_view_flushonshutdown(view, flush); + dns_view_detach(&view); } /* diff --git a/lib/dns/include/dns/request.h b/lib/dns/include/dns/request.h index bf17171edd..8dbee8c9ff 100644 --- a/lib/dns/include/dns/request.h +++ b/lib/dns/include/dns/request.h @@ -65,8 +65,6 @@ dns_requestmgr_create(isc_mem_t *mctx, isc_taskmgr_t *taskmgr, * *\li 'mctx' is a valid memory context. * - *\li 'socketmgr' is a valid socket manager. - * *\li 'taskmgr' is a valid task manager. * *\li 'dispatchv4' is a valid dispatcher with an IPv4 UDP socket, or is NULL. diff --git a/lib/dns/include/dns/view.h b/lib/dns/include/dns/view.h index 69edc41534..caa921ee8c 100644 --- a/lib/dns/include/dns/view.h +++ b/lib/dns/include/dns/view.h @@ -298,7 +298,8 @@ dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass, const char *name, void dns_view_attach(dns_view_t *source, dns_view_t **targetp); /*%< - * Attach '*targetp' to 'source'. + * Attach '*targetp' to 'source', incrementing the view's reference + * counter. * * Requires: * @@ -316,22 +317,12 @@ dns_view_attach(dns_view_t *source, dns_view_t **targetp); void dns_view_detach(dns_view_t **viewp); /*%< - * Detach '*viewp' from its view. - * - * Requires: - * - *\li 'viewp' points to a valid dns_view_t * - * - * Ensures: - * - *\li *viewp is NULL. - */ - -void -dns_view_flushanddetach(dns_view_t **viewp); -/*%< - * Detach '*viewp' from its view. If this was the last reference - * uncommitted changed in zones will be flushed to disk. + * Detach '*viewp' and decrement the view's reference counter. If this was + * the last reference, then the associated resolver, requestmgr, ADB and + * zones will be shut down; if dns_view_flushonshutdown() has been called + * with 'true', uncommitted changed in zones will also be flushed to disk. + * The view will not be fully destroyed, however, until the weak references + * (see below) reach zero as well. * * Requires: * @@ -345,7 +336,13 @@ dns_view_flushanddetach(dns_view_t **viewp); void dns_view_weakattach(dns_view_t *source, dns_view_t **targetp); /*%< - * Weakly attach '*targetp' to 'source'. + * Attach '*targetp' to 'source', incrementing the view's weak reference + * counter. + * + * Weak references are used by objects such as the resolver, requestmgr, + * ADB, and zones, which are subsidiary to the view; they need the view + * object to remain in existence as long as they persist, but they do + * not need to prevent it from being shut down. * * Requires: * @@ -363,7 +360,8 @@ dns_view_weakattach(dns_view_t *source, dns_view_t **targetp); void dns_view_weakdetach(dns_view_t **targetp); /*%< - * Detach '*viewp' from its view. + * Detach '*viewp' from its view. If this is the last weak reference, + * the view will be destroyed. * * Requires: * @@ -1359,4 +1357,13 @@ dns_view_staleanswerenabled(dns_view_t *view); *\li 'view' to be valid. */ +void +dns_view_flushonshutdown(dns_view_t *view, bool flush); +/*%< + * Inform the view that the zones should (or should not) be flushed to + * disk on shutdown. + * + * Requires: + *\li 'view' to be valid. + */ ISC_LANG_ENDDECLS diff --git a/lib/dns/view.c b/lib/dns/view.c index 3a08d42367..f7a3ac0400 100644 --- a/lib/dns/view.c +++ b/lib/dns/view.c @@ -61,6 +61,7 @@ #include #include #include +#include #include #include @@ -91,30 +92,66 @@ req_shutdown(isc_task_t *task, isc_event_t *event); isc_result_t dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass, const char *name, dns_view_t **viewp) { - dns_view_t *view; + dns_view_t *view = NULL; isc_result_t result; char buffer[1024]; - /* - * Create a view. - */ - REQUIRE(name != NULL); REQUIRE(viewp != NULL && *viewp == NULL); - view = isc_mem_get(mctx, sizeof(*view)); - - view->nta_file = NULL; - view->mctx = NULL; - isc_mem_attach(mctx, &view->mctx); - view->name = isc_mem_strdup(mctx, name); - - result = isc_file_sanitize(NULL, view->name, "nta", buffer, - sizeof(buffer)); + result = isc_file_sanitize(NULL, name, "nta", buffer, sizeof(buffer)); if (result != ISC_R_SUCCESS) { - goto cleanup_name; + return (result); } - view->nta_file = isc_mem_strdup(mctx, buffer); + + view = isc_mem_get(mctx, sizeof(*view)); + *view = (dns_view_t){ + .rdclass = rdclass, + .name = isc_mem_strdup(mctx, name), + .nta_file = isc_mem_strdup(mctx, buffer), + .recursion = true, + .enablevalidation = true, + .minimalresponses = dns_minimal_no, + .transfer_format = dns_one_answer, + .msgcompression = true, + .provideixfr = true, + .maxcachettl = 7 * 24 * 3600, + .maxncachettl = 3 * 3600, + .dstport = 53, + .staleanswerttl = 1, + .staleanswersok = dns_stale_answer_conf, + .sendcookie = true, + .synthfromdnssec = true, + .trust_anchor_telemetry = true, + .root_key_sentinel = true, + }; + + isc_refcount_init(&view->references, 1); + isc_refcount_init(&view->weakrefs, 1); + + dns_fixedname_init(&view->redirectfixed); + + ISC_LIST_INIT(view->dlz_searched); + ISC_LIST_INIT(view->dlz_unsearched); + ISC_LIST_INIT(view->dns64); + + ISC_LINK_INIT(view, link); + + ISC_EVENT_INIT(&view->resevent, sizeof(view->resevent), 0, NULL, + DNS_EVENT_VIEWRESSHUTDOWN, resolver_shutdown, view, NULL, + NULL, NULL); + ISC_EVENT_INIT(&view->adbevent, sizeof(view->adbevent), 0, NULL, + DNS_EVENT_VIEWADBSHUTDOWN, adb_shutdown, view, NULL, + NULL, NULL); + ISC_EVENT_INIT(&view->reqevent, sizeof(view->reqevent), 0, NULL, + DNS_EVENT_VIEWREQSHUTDOWN, req_shutdown, view, NULL, + NULL, NULL); + + atomic_init(&view->attributes, + (DNS_VIEWATTR_RESSHUTDOWN | DNS_VIEWATTR_ADBSHUTDOWN | + DNS_VIEWATTR_REQSHUTDOWN)); + + isc_mem_attach(mctx, &view->mctx); isc_mutex_init(&view->lock); @@ -128,9 +165,6 @@ dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass, const char *name, goto cleanup_mutex; } - view->secroots_priv = NULL; - view->ntatable_priv = NULL; - view->fwdtable = NULL; result = dns_fwdtable_create(mctx, &view->fwdtable); if (result != ISC_R_SUCCESS) { UNEXPECTED_ERROR(__FILE__, __LINE__, @@ -140,130 +174,16 @@ dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass, const char *name, goto cleanup_zt; } - view->cache = NULL; - view->cachedb = NULL; - ISC_LIST_INIT(view->dlz_searched); - ISC_LIST_INIT(view->dlz_unsearched); - view->hints = NULL; - view->resolver = NULL; - view->adb = NULL; - view->requestmgr = NULL; - view->rdclass = rdclass; - view->frozen = false; - view->task = NULL; - isc_refcount_init(&view->references, 1); - isc_refcount_init(&view->weakrefs, 1); - atomic_init(&view->attributes, - (DNS_VIEWATTR_RESSHUTDOWN | DNS_VIEWATTR_ADBSHUTDOWN | - DNS_VIEWATTR_REQSHUTDOWN)); - view->transports = NULL; - view->statickeys = NULL; - view->dynamickeys = NULL; - view->matchclients = NULL; - view->matchdestinations = NULL; - view->matchrecursiveonly = false; result = dns_tsigkeyring_create(view->mctx, &view->dynamickeys); if (result != ISC_R_SUCCESS) { goto cleanup_weakrefs; } - view->peers = NULL; - view->order = NULL; - view->delonly = NULL; - view->rootdelonly = false; - view->rootexclude = NULL; - view->adbstats = NULL; - view->resstats = NULL; - view->resquerystats = NULL; - view->cacheshared = false; - ISC_LIST_INIT(view->dns64); - view->dns64cnt = 0; - /* - * Initialize configuration data with default values. - */ - view->recursion = true; - view->qminimization = false; - view->qmin_strict = false; - view->auth_nxdomain = false; /* Was true in BIND 8 */ - view->enablevalidation = true; - view->acceptexpired = false; - view->use_glue_cache = false; - view->minimal_any = false; - view->minimalresponses = dns_minimal_no; - view->transfer_format = dns_one_answer; - view->cacheacl = NULL; - view->cacheonacl = NULL; - view->checknames = false; - view->queryacl = NULL; - view->queryonacl = NULL; - view->recursionacl = NULL; - view->recursiononacl = NULL; - view->sortlist = NULL; - view->transferacl = NULL; - view->notifyacl = NULL; - view->updateacl = NULL; - view->upfwdacl = NULL; - view->denyansweracl = NULL; - view->nocasecompress = NULL; - view->msgcompression = true; - view->answeracl_exclude = NULL; - view->denyanswernames = NULL; - view->answernames_exclude = NULL; - view->rrl = NULL; - view->provideixfr = true; - view->maxcachettl = 7 * 24 * 3600; - view->maxncachettl = 3 * 3600; - view->mincachettl = 0; - view->minncachettl = 0; - view->nta_lifetime = 0; - view->nta_recheck = 0; - view->prefetch_eligible = 0; - view->prefetch_trigger = 0; - view->dstport = 53; - view->preferred_glue = 0; - view->flush = false; - view->maxudp = 0; - view->staleanswerttl = 1; - view->staleanswersok = dns_stale_answer_conf; - view->staleanswersenable = false; - view->nocookieudp = 0; - view->padding = 0; - view->pad_acl = NULL; - view->maxbits = 0; - view->rpzs = NULL; - view->catzs = NULL; - view->managed_keys = NULL; - view->redirect = NULL; - view->redirectzone = NULL; - dns_fixedname_init(&view->redirectfixed); - view->requestnsid = false; - view->sendcookie = true; - view->requireservercookie = false; - view->synthfromdnssec = true; - view->trust_anchor_telemetry = true; - view->root_key_sentinel = true; - view->new_zone_dir = NULL; - view->new_zone_file = NULL; - view->new_zone_db = NULL; - view->new_zone_dbenv = NULL; - view->new_zone_mapsize = 0ULL; - view->new_zone_config = NULL; - view->cfg_destroy = NULL; - view->fail_ttl = 0; - view->failcache = NULL; result = dns_badcache_init(view->mctx, DNS_VIEW_FAILCACHESIZE, &view->failcache); if (result != ISC_R_SUCCESS) { goto cleanup_dynkeys; } - view->v6bias = 0; - view->dtenv = NULL; - view->dttypes = 0; - - view->plugins = NULL; - view->plugins_free = NULL; - view->hooktable = NULL; - view->hooktable_free = NULL; isc_mutex_init(&view->new_zone_lock); @@ -282,19 +202,7 @@ dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass, const char *name, goto cleanup_peerlist; } - ISC_LINK_INIT(view, link); - ISC_EVENT_INIT(&view->resevent, sizeof(view->resevent), 0, NULL, - DNS_EVENT_VIEWRESSHUTDOWN, resolver_shutdown, view, NULL, - NULL, NULL); - ISC_EVENT_INIT(&view->adbevent, sizeof(view->adbevent), 0, NULL, - DNS_EVENT_VIEWADBSHUTDOWN, adb_shutdown, view, NULL, - NULL, NULL); - ISC_EVENT_INIT(&view->reqevent, sizeof(view->reqevent), 0, NULL, - DNS_EVENT_VIEWREQSHUTDOWN, req_shutdown, view, NULL, - NULL, NULL); - view->viewlist = NULL; view->magic = DNS_VIEW_MAGIC; - *viewp = view; return (ISC_R_SUCCESS); @@ -311,7 +219,6 @@ cleanup_order: cleanup_new_zone_lock: isc_mutex_destroy(&view->new_zone_lock); - dns_badcache_destroy(&view->failcache); cleanup_dynkeys: @@ -342,7 +249,6 @@ cleanup_mutex: isc_mem_free(mctx, view->nta_file); } -cleanup_name: isc_mem_free(mctx, view->name); isc_mem_putanddetach(&view->mctx, view, sizeof(*view)); @@ -622,15 +528,14 @@ dns_view_attach(dns_view_t *source, dns_view_t **targetp) { *targetp = source; } -static void -view_flushanddetach(dns_view_t **viewp, bool flush) { - REQUIRE(viewp != NULL && DNS_VIEW_VALID(*viewp)); - dns_view_t *view = *viewp; - *viewp = NULL; +void +dns_view_detach(dns_view_t **viewp) { + dns_view_t *view = NULL; - if (flush) { - view->flush = flush; - } + REQUIRE(viewp != NULL && DNS_VIEW_VALID(*viewp)); + + view = *viewp; + *viewp = NULL; if (isc_refcount_decrement(&view->references) == 1) { dns_zone_t *mkzone = NULL, *rdzone = NULL; @@ -697,16 +602,6 @@ view_flushanddetach(dns_view_t **viewp, bool flush) { } } -void -dns_view_flushanddetach(dns_view_t **viewp) { - view_flushanddetach(viewp, true); -} - -void -dns_view_detach(dns_view_t **viewp) { - view_flushanddetach(viewp, false); -} - static isc_result_t dialup(dns_zone_t *zone, void *dummy) { UNUSED(dummy); @@ -734,11 +629,13 @@ dns_view_weakattach(dns_view_t *source, dns_view_t **targetp) { void dns_view_weakdetach(dns_view_t **viewp) { - dns_view_t *view; + dns_view_t *view = NULL; REQUIRE(viewp != NULL); + view = *viewp; *viewp = NULL; + REQUIRE(DNS_VIEW_VALID(view)); if (isc_refcount_decrement(&view->weakrefs) == 1) { @@ -813,7 +710,7 @@ dns_view_createresolver(dns_view_t *view, isc_taskmgr_t *taskmgr, dns_dispatch_t *dispatchv4, dns_dispatch_t *dispatchv6) { isc_result_t result; - isc_event_t *event; + isc_event_t *event = NULL; isc_mem_t *mctx = NULL; REQUIRE(DNS_VIEW_VALID(view)); @@ -2581,3 +2478,10 @@ dns_view_staleanswerenabled(dns_view_t *view) { return (result); } + +void +dns_view_flushonshutdown(dns_view_t *view, bool flush) { + REQUIRE(DNS_VIEW_VALID(view)); + + view->flush = flush; +} From e8ab719bc2f8794c914e849f82cf06ae128b34f3 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 10 May 2022 12:48:31 -0700 Subject: [PATCH 03/11] fixed a possible reference leak in dns_resolver_create() If an error occurred while creating the resolver, the mctx could remain attached. --- lib/dns/resolver.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index c75505fa29..fe909eb1bf 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -10234,10 +10234,6 @@ dns_resolver_create(dns_view_t *view, isc_taskmgr_t *taskmgr, res->quotaresp[dns_quotatype_zone] = DNS_R_DROP; res->quotaresp[dns_quotatype_server] = DNS_R_SERVFAIL; isc_refcount_init(&res->references, 1); - atomic_init(&res->exiting, false); - atomic_init(&res->priming, false); - atomic_init(&res->zspill, 0); - atomic_init(&res->nfctx, 0); ISC_LIST_INIT(res->whenshutdown); ISC_LIST_INIT(res->alternates); @@ -10343,8 +10339,7 @@ cleanup_buckets: dns_badcache_destroy(&res->badcache); cleanup_res: - isc_mem_put(view->mctx, res, sizeof(*res)); - + isc_mem_putanddetach(&res->mctx, res, sizeof(*res)); return (result); } From 435e29c3e021612b7994ef778d30b85410b6b040 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 11 May 2022 12:15:46 -0700 Subject: [PATCH 04/11] the validator can attach to the view normally dns_view_weakattach() and _weakdetach() are used by objects that are created by the view and need it to persist until they are destroyed, but don't need to prevent it from being shut down. the validator can use normal view references instead of weak references. --- lib/dns/validator.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/dns/validator.c b/lib/dns/validator.c index ddb08c481a..ca5000da77 100644 --- a/lib/dns/validator.c +++ b/lib/dns/validator.c @@ -3137,7 +3137,7 @@ dns_validator_create(dns_view_t *view, dns_name_t *name, dns_rdatatype_t type, .action = action, .arg = arg }; - dns_view_weakattach(view, &val->view); + dns_view_attach(view, &val->view); isc_mutex_init(&val->lock); result = dns_view_getsecroots(val->view, &val->keytable); @@ -3171,7 +3171,7 @@ cleanup: isc_task_detach(&tclone); isc_event_free(ISC_EVENT_PTR(&event)); - dns_view_weakdetach(&val->view); + dns_view_detach(&val->view); isc_mem_put(view->mctx, val, sizeof(*val)); return (result); @@ -3250,7 +3250,7 @@ destroy(dns_validator_t *val) { isc_mem_put(mctx, val->siginfo, sizeof(*val->siginfo)); } isc_mutex_destroy(&val->lock); - dns_view_weakdetach(&val->view); + dns_view_detach(&val->view); isc_mem_put(mctx, val, sizeof(*val)); } From e5e2d7814e814125727596584d64de93b5f7baff Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 11 May 2022 14:18:45 -0700 Subject: [PATCH 05/11] remove view->attributes field it's not necessary to use view attributes to determine whether the ADB, resolver and requestmgr need to be shut down; checking whether they're NULL is sufficient. --- lib/dns/include/dns/view.h | 5 ++--- lib/dns/view.c | 28 +++------------------------- 2 files changed, 5 insertions(+), 28 deletions(-) diff --git a/lib/dns/include/dns/view.h b/lib/dns/include/dns/view.h index caa921ee8c..b525583c89 100644 --- a/lib/dns/include/dns/view.h +++ b/lib/dns/include/dns/view.h @@ -199,9 +199,8 @@ struct dns_view { bool matchrecursiveonly; /* Locked by themselves. */ - isc_refcount_t references; - isc_refcount_t weakrefs; - atomic_uint_fast32_t attributes; + isc_refcount_t references; + isc_refcount_t weakrefs; /* Under owner's locking control. */ ISC_LINK(struct dns_view) link; diff --git a/lib/dns/view.c b/lib/dns/view.c index f7a3ac0400..969b153ea7 100644 --- a/lib/dns/view.c +++ b/lib/dns/view.c @@ -72,13 +72,6 @@ goto cleanup; \ } while (0) -#define RESSHUTDOWN(v) \ - ((atomic_load(&(v)->attributes) & DNS_VIEWATTR_RESSHUTDOWN) != 0) -#define ADBSHUTDOWN(v) \ - ((atomic_load(&(v)->attributes) & DNS_VIEWATTR_ADBSHUTDOWN) != 0) -#define REQSHUTDOWN(v) \ - ((atomic_load(&(v)->attributes) & DNS_VIEWATTR_REQSHUTDOWN) != 0) - #define DNS_VIEW_DELONLYHASH 111 #define DNS_VIEW_FAILCACHESIZE 1021 @@ -147,10 +140,6 @@ dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass, const char *name, DNS_EVENT_VIEWREQSHUTDOWN, req_shutdown, view, NULL, NULL, NULL); - atomic_init(&view->attributes, - (DNS_VIEWATTR_RESSHUTDOWN | DNS_VIEWATTR_ADBSHUTDOWN | - DNS_VIEWATTR_REQSHUTDOWN)); - isc_mem_attach(mctx, &view->mctx); isc_mutex_init(&view->lock); @@ -261,9 +250,6 @@ destroy(dns_view_t *view) { dns_dlzdb_t *dlzdb; REQUIRE(!ISC_LINK_LINKED(view, link)); - REQUIRE(RESSHUTDOWN(view)); - REQUIRE(ADBSHUTDOWN(view)); - REQUIRE(REQSHUTDOWN(view)); isc_refcount_destroy(&view->references); isc_refcount_destroy(&view->weakrefs); @@ -543,13 +529,13 @@ dns_view_detach(dns_view_t **viewp) { isc_refcount_destroy(&view->references); - if (!RESSHUTDOWN(view)) { + if (view->resolver != NULL) { dns_resolver_shutdown(view->resolver); } - if (!ADBSHUTDOWN(view)) { + if (view->adb != NULL) { dns_adb_shutdown(view->adb); } - if (!REQSHUTDOWN(view)) { + if (view->requestmgr != NULL) { dns_requestmgr_shutdown(view->requestmgr); } @@ -655,7 +641,6 @@ resolver_shutdown(isc_task_t *task, isc_event_t *event) { isc_event_free(&event); - atomic_fetch_or(&view->attributes, DNS_VIEWATTR_RESSHUTDOWN); dns_view_weakdetach(&view); } @@ -671,8 +656,6 @@ adb_shutdown(isc_task_t *task, isc_event_t *event) { isc_event_free(&event); - atomic_fetch_or(&view->attributes, DNS_VIEWATTR_ADBSHUTDOWN); - dns_view_weakdetach(&view); } @@ -688,8 +671,6 @@ req_shutdown(isc_task_t *task, isc_event_t *event) { isc_event_free(&event); - atomic_fetch_or(&view->attributes, DNS_VIEWATTR_REQSHUTDOWN); - dns_view_weakdetach(&view); } @@ -732,7 +713,6 @@ dns_view_createresolver(dns_view_t *view, isc_taskmgr_t *taskmgr, } event = &view->resevent; dns_resolver_whenshutdown(view->resolver, view->task, &event); - atomic_fetch_and(&view->attributes, ~DNS_VIEWATTR_RESSHUTDOWN); isc_refcount_increment(&view->weakrefs); isc_mem_create(&mctx); @@ -746,7 +726,6 @@ dns_view_createresolver(dns_view_t *view, isc_taskmgr_t *taskmgr, } event = &view->adbevent; dns_adb_whenshutdown(view->adb, view->task, &event); - atomic_fetch_and(&view->attributes, ~DNS_VIEWATTR_ADBSHUTDOWN); isc_refcount_increment(&view->weakrefs); result = dns_requestmgr_create( @@ -760,7 +739,6 @@ dns_view_createresolver(dns_view_t *view, isc_taskmgr_t *taskmgr, } event = &view->reqevent; dns_requestmgr_whenshutdown(view->requestmgr, view->task, &event); - atomic_fetch_and(&view->attributes, ~DNS_VIEWATTR_REQSHUTDOWN); isc_refcount_increment(&view->weakrefs); return (ISC_R_SUCCESS); From 3027f59f6f4c30fbaaa24a83ec5d0482c4a6dc32 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 11 May 2022 15:38:54 -0700 Subject: [PATCH 06/11] move ADB and resolver stats out of the view object for better object separation, ADB and resolver statistics counters are now stored in the ADB and resolver objects themsevles, rather than in the associated view. --- bin/named/server.c | 27 +++++----- bin/named/statschannel.c | 74 ++++++++++++++++----------- lib/dns/adb.c | 42 ++++++++------- lib/dns/include/dns/adb.h | 8 +++ lib/dns/include/dns/resolver.h | 60 ++++++++++++++++++++++ lib/dns/include/dns/view.h | 93 +++------------------------------- lib/dns/resolver.c | 79 +++++++++++++++++++++++++---- lib/dns/view.c | 66 ------------------------ 8 files changed, 226 insertions(+), 223 deletions(-) diff --git a/bin/named/server.c b/bin/named/server.c index 043205b4fa..8bdcc27aa2 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -4661,9 +4661,10 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist, cfg_obj_t *config, "reusing existing cache"); dns_cache_attach(pview->cache, &cache); } - dns_view_getresstats(pview, &resstats); - dns_view_getresquerystats(pview, - &resquerystats); + dns_resolver_getstats(pview->resolver, + &resstats); + dns_resolver_getquerystats(pview->resolver, + &resquerystats); dns_view_detach(&pview); } } @@ -4731,22 +4732,22 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist, cfg_obj_t *config, goto cleanup; } - if (resstats == NULL) { - CHECK(isc_stats_create(mctx, &resstats, - dns_resstatscounter_max)); - } - dns_view_setresstats(view, resstats); - if (resquerystats == NULL) { - CHECK(dns_rdatatypestats_create(mctx, &resquerystats)); - } - dns_view_setresquerystats(view, resquerystats); - ndisp = 4 * ISC_MIN(named_g_udpdisp, MAX_UDP_DISPATCH); CHECK(dns_view_createresolver( view, named_g_taskmgr, RESOLVER_NTASKS_PERCPU * named_g_cpus, ndisp, named_g_netmgr, named_g_timermgr, resopts, named_g_dispatchmgr, dispatch4, dispatch6)); + if (resstats == NULL) { + CHECK(isc_stats_create(mctx, &resstats, + dns_resstatscounter_max)); + } + dns_resolver_setstats(view->resolver, resstats); + if (resquerystats == NULL) { + CHECK(dns_rdatatypestats_create(mctx, &resquerystats)); + } + dns_resolver_setquerystats(view->resolver, resquerystats); + if (dscp4 == -1) { dscp4 = named_g_dscp; } diff --git a/bin/named/statschannel.c b/bin/named/statschannel.c index 1687d1f7ae..11e50e1689 100644 --- a/bin/named/statschannel.c +++ b/bin/named/statschannel.c @@ -25,6 +25,7 @@ #include #include +#include #include #include #include @@ -2239,6 +2240,9 @@ generatexml(named_server_t *server, uint32_t flags, int *buflen, TRY0(xmlTextWriterStartElement(writer, ISC_XMLCHAR "views")); while (view != NULL && ((flags & (STATS_XML_SERVER | STATS_XML_ZONES)) != 0)) { + isc_stats_t *istats = NULL; + dns_stats_t *dstats = NULL; + TRY0(xmlTextWriterStartElement(writer, ISC_XMLCHAR "view")); TRY0(xmlTextWriterWriteAttribute(writer, ISC_XMLCHAR "name", ISC_XMLCHAR view->name)); @@ -2261,25 +2265,29 @@ generatexml(named_server_t *server, uint32_t flags, int *buflen, TRY0(xmlTextWriterWriteAttribute(writer, ISC_XMLCHAR "type", ISC_XMLCHAR "resqtype")); - if (view->resquerystats != NULL) { + dns_resolver_getquerystats(view->resolver, &dstats); + if (dstats != NULL) { dumparg.result = ISC_R_SUCCESS; - dns_rdatatypestats_dump(view->resquerystats, - rdtypestat_dump, &dumparg, 0); + dns_rdatatypestats_dump(dstats, rdtypestat_dump, + &dumparg, 0); CHECK(dumparg.result); } + dns_stats_detach(&dstats); TRY0(xmlTextWriterEndElement(writer)); /* */ TRY0(xmlTextWriterStartElement(writer, ISC_XMLCHAR "counters")); TRY0(xmlTextWriterWriteAttribute(writer, ISC_XMLCHAR "type", ISC_XMLCHAR "resstats")); - if (view->resstats != NULL) { - CHECK(dump_counters(view->resstats, isc_statsformat_xml, - writer, NULL, resstats_xmldesc, + dns_resolver_getstats(view->resolver, &istats); + if (istats != NULL) { + CHECK(dump_counters(istats, isc_statsformat_xml, writer, + NULL, resstats_xmldesc, dns_resstatscounter_max, resstats_index, resstat_values, ISC_STATSDUMP_VERBOSE)); } + isc_stats_detach(&istats); TRY0(xmlTextWriterEndElement(writer)); /* */ cacherrstats = dns_db_getrrsetstats(view->cachedb); @@ -2300,13 +2308,10 @@ generatexml(named_server_t *server, uint32_t flags, int *buflen, TRY0(xmlTextWriterStartElement(writer, ISC_XMLCHAR "counters")); TRY0(xmlTextWriterWriteAttribute(writer, ISC_XMLCHAR "type", ISC_XMLCHAR "adbstat")); - if (view->adbstats != NULL) { - CHECK(dump_counters(view->adbstats, isc_statsformat_xml, - writer, NULL, adbstats_xmldesc, - dns_adbstats_max, adbstats_index, - adbstat_values, - ISC_STATSDUMP_VERBOSE)); - } + CHECK(dump_counters( + dns_adb_getstats(view->adb), isc_statsformat_xml, + writer, NULL, adbstats_xmldesc, dns_adbstats_max, + adbstats_index, adbstat_values, ISC_STATSDUMP_VERBOSE)); TRY0(xmlTextWriterEndElement(writer)); /* */ /* */ @@ -2993,15 +2998,15 @@ generatejson(named_server_t *server, size_t *msglen, const char **msg, } if ((flags & STATS_JSON_SERVER) != 0) { - json_object *res; - dns_stats_t *dstats; - isc_stats_t *istats; + json_object *res = NULL; + dns_stats_t *dstats = NULL; + isc_stats_t *istats = NULL; res = json_object_new_object(); CHECKMEM(res); json_object_object_add(v, "resolver", res); - istats = view->resstats; + dns_resolver_getstats(view->resolver, &istats); if (istats != NULL) { counters = json_object_new_object(); CHECKMEM(counters); @@ -3021,9 +3026,11 @@ generatejson(named_server_t *server, size_t *msglen, const char **msg, json_object_object_add(res, "stats", counters); + isc_stats_detach(&istats); } - dstats = view->resquerystats; + dns_resolver_getquerystats(view->resolver, + &dstats); if (dstats != NULL) { counters = json_object_new_object(); CHECKMEM(counters); @@ -3041,6 +3048,7 @@ generatejson(named_server_t *server, size_t *msglen, const char **msg, json_object_object_add(res, "qtypes", counters); + dns_stats_detach(&dstats); } dstats = dns_db_getrrsetstats(view->cachedb); @@ -3076,7 +3084,7 @@ generatejson(named_server_t *server, size_t *msglen, const char **msg, json_object_object_add(res, "cachestats", counters); - istats = view->adbstats; + istats = dns_adb_getstats(view->adb); if (istats != NULL) { counters = json_object_new_object(); CHECKMEM(counters); @@ -3926,7 +3934,9 @@ named_stats_dump(named_server_t *server, FILE *fp) { for (view = ISC_LIST_HEAD(server->viewlist); view != NULL; view = ISC_LIST_NEXT(view, link)) { - if (view->resquerystats == NULL) { + dns_stats_t *dstats = NULL; + dns_resolver_getquerystats(view->resolver, &dstats); + if (dstats == NULL) { continue; } if (strcmp(view->name, "_default") == 0) { @@ -3934,8 +3944,8 @@ named_stats_dump(named_server_t *server, FILE *fp) { } else { fprintf(fp, "[View: %s]\n", view->name); } - dns_rdatatypestats_dump(view->resquerystats, rdtypestat_dump, - &dumparg, 0); + dns_rdatatypestats_dump(dstats, rdtypestat_dump, &dumparg, 0); + dns_stats_detach(&dstats); } fprintf(fp, "++ Name Server Statistics ++\n"); @@ -3957,7 +3967,9 @@ named_stats_dump(named_server_t *server, FILE *fp) { for (view = ISC_LIST_HEAD(server->viewlist); view != NULL; view = ISC_LIST_NEXT(view, link)) { - if (view->resstats == NULL) { + isc_stats_t *istats = NULL; + dns_resolver_getstats(view->resolver, &istats); + if (istats == NULL) { continue; } if (strcmp(view->name, "_default") == 0) { @@ -3965,10 +3977,10 @@ named_stats_dump(named_server_t *server, FILE *fp) { } else { fprintf(fp, "[View: %s]\n", view->name); } - (void)dump_counters(view->resstats, isc_statsformat_file, fp, - NULL, resstats_desc, - dns_resstatscounter_max, resstats_index, - resstat_values, 0); + (void)dump_counters(istats, isc_statsformat_file, fp, NULL, + resstats_desc, dns_resstatscounter_max, + resstats_index, resstat_values, 0); + isc_stats_detach(&istats); } fprintf(fp, "++ Cache Statistics ++\n"); @@ -4021,7 +4033,9 @@ named_stats_dump(named_server_t *server, FILE *fp) { for (view = ISC_LIST_HEAD(server->viewlist); view != NULL; view = ISC_LIST_NEXT(view, link)) { - if (view->adbstats == NULL) { + isc_stats_t *adbstats = dns_adb_getstats(view->adb); + + if (adbstats == NULL) { continue; } if (strcmp(view->name, "_default") == 0) { @@ -4029,8 +4043,8 @@ named_stats_dump(named_server_t *server, FILE *fp) { } else { fprintf(fp, "[View: %s]\n", view->name); } - (void)dump_counters(view->adbstats, isc_statsformat_file, fp, - NULL, adbstats_desc, dns_adbstats_max, + (void)dump_counters(adbstats, isc_statsformat_file, fp, NULL, + adbstats_desc, dns_adbstats_max, adbstats_index, adbstat_values, 0); } diff --git a/lib/dns/adb.c b/lib/dns/adb.c index 99619a3d6a..52ebf9bdc8 100644 --- a/lib/dns/adb.c +++ b/lib/dns/adb.c @@ -115,6 +115,8 @@ struct dns_adb { isc_ht_t *entrybuckets; isc_rwlock_t entries_lock; + isc_stats_t *stats; + isc_event_t cevent; bool cevent_out; atomic_bool exiting; @@ -487,10 +489,8 @@ DP(int level, const char *format, ...) { * Increment resolver-related statistics counters. */ static void -inc_stats(dns_adb_t *adb, isc_statscounter_t counter) { - if (adb->view->resstats != NULL) { - isc_stats_increment(adb->view->resstats, counter); - } +inc_resstats(dns_adb_t *adb, isc_statscounter_t counter) { + dns_resolver_incstats(adb->view->resolver, counter); } /*% @@ -498,22 +498,22 @@ inc_stats(dns_adb_t *adb, isc_statscounter_t counter) { */ static void set_adbstat(dns_adb_t *adb, uint64_t val, isc_statscounter_t counter) { - if (adb->view->adbstats != NULL) { - isc_stats_set(adb->view->adbstats, val, counter); + if (adb->stats != NULL) { + isc_stats_set(adb->stats, val, counter); } } static void dec_adbstats(dns_adb_t *adb, isc_statscounter_t counter) { - if (adb->view->adbstats != NULL) { - isc_stats_decrement(adb->view->adbstats, counter); + if (adb->stats != NULL) { + isc_stats_decrement(adb->stats, counter); } } static void inc_adbstats(dns_adb_t *adb, isc_statscounter_t counter) { - if (adb->view->adbstats != NULL) { - isc_stats_increment(adb->view->adbstats, counter); + if (adb->stats != NULL) { + isc_stats_increment(adb->stats, counter); } } @@ -2075,6 +2075,7 @@ destroy(dns_adb_t *adb) { isc_rwlock_destroy(&adb->entries_lock); isc_mutex_destroy(&adb->lock); + isc_stats_detach(&adb->stats); isc_mem_putanddetach(&adb->mctx, adb, sizeof(dns_adb_t)); } @@ -2129,7 +2130,7 @@ dns_adb_create(isc_mem_t *mem, dns_view_t *view, isc_taskmgr_t *taskmgr, isc_task_setname(adb->task, "ADB", adb); - result = isc_stats_create(adb->mctx, &view->adbstats, dns_adbstats_max); + result = isc_stats_create(adb->mctx, &adb->stats, dns_adbstats_max); if (result != ISC_R_SUCCESS) { goto free_task; } @@ -3244,7 +3245,7 @@ fetch_callback(isc_task_t *task, isc_event_t *ev) { } else { name->fetch_err = FIND_ERR_NXRRSET; } - inc_stats(adb, dns_resstatscounter_gluefetchv4fail); + inc_resstats(adb, dns_resstatscounter_gluefetchv4fail); } else { DP(NCACHE_LEVEL, "adb fetch name %p: " @@ -3257,7 +3258,7 @@ fetch_callback(isc_task_t *task, isc_event_t *ev) { } else { name->fetch6_err = FIND_ERR_NXRRSET; } - inc_stats(adb, dns_resstatscounter_gluefetchv6fail); + inc_resstats(adb, dns_resstatscounter_gluefetchv6fail); } goto out; } @@ -3301,11 +3302,11 @@ fetch_callback(isc_task_t *task, isc_event_t *ev) { if (address_type == DNS_ADBFIND_INET) { name->expire_v4 = ISC_MIN(name->expire_v4, now + 10); name->fetch_err = FIND_ERR_FAILURE; - inc_stats(adb, dns_resstatscounter_gluefetchv4fail); + inc_resstats(adb, dns_resstatscounter_gluefetchv4fail); } else { name->expire_v6 = ISC_MIN(name->expire_v6, now + 10); name->fetch6_err = FIND_ERR_FAILURE; - inc_stats(adb, dns_resstatscounter_gluefetchv6fail); + inc_resstats(adb, dns_resstatscounter_gluefetchv6fail); } goto out; } @@ -3396,10 +3397,10 @@ fetch_name(dns_adbname_t *adbname, bool start_at_zone, unsigned int depth, if (type == dns_rdatatype_a) { adbname->fetch_a = fetch; - inc_stats(adb, dns_resstatscounter_gluefetchv4); + inc_resstats(adb, dns_resstatscounter_gluefetchv4); } else { adbname->fetch_aaaa = fetch; - inc_stats(adb, dns_resstatscounter_gluefetchv6); + inc_resstats(adb, dns_resstatscounter_gluefetchv6); } fetch = NULL; /* Keep us from cleaning this up below. */ @@ -4022,3 +4023,10 @@ dns_adb_endudpfetch(dns_adb_t *adb, dns_adbaddrinfo_t *addr) { REQUIRE(atomic_fetch_sub_release(&addr->entry->active, 1) != 0); } + +isc_stats_t * +dns_adb_getstats(dns_adb_t *adb) { + REQUIRE(DNS_ADB_VALID(adb)); + + return (adb->stats); +} diff --git a/lib/dns/include/dns/adb.h b/lib/dns/include/dns/adb.h index d6bb8ddb21..2163a2ce3f 100644 --- a/lib/dns/include/dns/adb.h +++ b/lib/dns/include/dns/adb.h @@ -807,4 +807,12 @@ dns_adb_endudpfetch(dns_adb_t *adb, dns_adbaddrinfo_t *addr); *\li addr be valid. */ +isc_stats_t * +dns_adb_getstats(dns_adb_t *adb); +/*%< + * Get the adb statistics counter set for 'adb'. + * + * Requires: + * \li 'adb' is valid. + */ ISC_LANG_ENDDECLS diff --git a/lib/dns/include/dns/resolver.h b/lib/dns/include/dns/resolver.h index d7d905dbb9..d17b28a177 100644 --- a/lib/dns/include/dns/resolver.h +++ b/lib/dns/include/dns/resolver.h @@ -736,4 +736,64 @@ void dns_resolver_setfuzzing(void); #endif /* ifdef ENABLE_AFL */ +void +dns_resolver_setstats(dns_resolver_t *res, isc_stats_t *stats); +/*%< + * Set a general resolver statistics counter set 'stats' for 'res'. + * + * Requires: + * \li 'res' is valid. + * + *\li stats is a valid statistics supporting resolver statistics counters + * (see dns/stats.h). + */ + +void +dns_resolver_getstats(dns_resolver_t *res, isc_stats_t **statsp); +/*%< + * Get the general statistics counter set for 'res'. If a statistics set is + * set '*statsp' will be attached to the set; otherwise, '*statsp' will be + * untouched. + * + * Requires: + * \li 'res' is valid. + * + *\li 'statsp' != NULL && '*statsp' != NULL + */ + +void +dns_resolver_incstats(dns_resolver_t *res, isc_statscounter_t counter); +/*%< + * Increment the specified statistics counter in res->stats, if res->stats + * is set. + * + * Requires: + * \li 'res' is valid. + */ + +void +dns_resolver_setquerystats(dns_resolver_t *res, dns_stats_t *stats); +/*%< + * Set a statistics counter set of rdata type, 'stats', for 'res'. Once the + * statistic set is installed, the resolver will count outgoing queries + * per rdata type. + * + * Requires: + * \li 'res' is valid. + * + *\li stats is a valid statistics created by dns_rdatatypestats_create(). + */ + +void +dns_resolver_getquerystats(dns_resolver_t *res, dns_stats_t **statsp); +/*%< + * Get the rdatatype statistics counter set for 'res'. If a statistics set is + * set '*statsp' will be attached to the set; otherwise, '*statsp' will be + * untouched. + * + * Requires: + * \li 'res' is valid. + * + *\li 'statsp' != NULL && '*statsp' != NULL + */ ISC_LANG_ENDDECLS diff --git a/lib/dns/include/dns/view.h b/lib/dns/include/dns/view.h index b525583c89..d590dfeaa3 100644 --- a/lib/dns/include/dns/view.h +++ b/lib/dns/include/dns/view.h @@ -101,16 +101,13 @@ struct dns_view { dns_keytable_t *secroots_priv; dns_ntatable_t *ntatable_priv; - isc_mutex_t lock; - bool frozen; - isc_task_t *task; - isc_event_t resevent; - isc_event_t adbevent; - isc_event_t reqevent; - isc_stats_t *adbstats; - isc_stats_t *resstats; - dns_stats_t *resquerystats; - bool cacheshared; + isc_mutex_t lock; + bool frozen; + isc_task_t *task; + isc_event_t resevent; + isc_event_t adbevent; + isc_event_t reqevent; + bool cacheshared; /* Configurable data. */ dns_transport_list_t *transports; @@ -1018,82 +1015,6 @@ dns_view_freezezones(dns_view_t *view, bool freeze); * \li 'view' is valid. */ -void -dns_view_setadbstats(dns_view_t *view, isc_stats_t *stats); -/*%< - * Set a adb statistics set 'stats' for 'view'. - * - * Requires: - * \li 'view' is valid and is not frozen. - * - *\li stats is a valid statistics supporting adb statistics - * (see dns/stats.h). - */ - -void -dns_view_getadbstats(dns_view_t *view, isc_stats_t **statsp); -/*%< - * Get the adb statistics counter set for 'view'. If a statistics set is - * set '*statsp' will be attached to the set; otherwise, '*statsp' will be - * untouched. - * - * Requires: - * \li 'view' is valid and is not frozen. - * - *\li 'statsp' != NULL && '*statsp' != NULL - */ - -void -dns_view_setresstats(dns_view_t *view, isc_stats_t *stats); -/*%< - * Set a general resolver statistics counter set 'stats' for 'view'. - * - * Requires: - * \li 'view' is valid and is not frozen. - * - *\li stats is a valid statistics supporting resolver statistics counters - * (see dns/stats.h). - */ - -void -dns_view_getresstats(dns_view_t *view, isc_stats_t **statsp); -/*%< - * Get the general statistics counter set for 'view'. If a statistics set is - * set '*statsp' will be attached to the set; otherwise, '*statsp' will be - * untouched. - * - * Requires: - * \li 'view' is valid and is not frozen. - * - *\li 'statsp' != NULL && '*statsp' != NULL - */ - -void -dns_view_setresquerystats(dns_view_t *view, dns_stats_t *stats); -/*%< - * Set a statistics counter set of rdata type, 'stats', for 'view'. Once the - * statistic set is installed, view's resolver will count outgoing queries - * per rdata type. - * - * Requires: - * \li 'view' is valid and is not frozen. - * - *\li stats is a valid statistics created by dns_rdatatypestats_create(). - */ - -void -dns_view_getresquerystats(dns_view_t *view, dns_stats_t **statsp); -/*%< - * Get the rdatatype statistics counter set for 'view'. If a statistics set is - * set '*statsp' will be attached to the set; otherwise, '*statsp' will be - * untouched. - * - * Requires: - * \li 'view' is valid and is not frozen. - * - *\li 'statsp' != NULL && '*statsp' != NULL - */ - bool dns_view_iscacheshared(dns_view_t *view); /*%< diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index fe909eb1bf..02bfb2a72e 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -556,6 +556,8 @@ struct dns_resolver { unsigned int maxdepth; unsigned int maxqueries; isc_result_t quotaresp[2]; + isc_stats_t *stats; + dns_stats_t *querystats; /* Additions for serve-stale feature. */ unsigned int retryinterval; /* in milliseconds */ @@ -911,15 +913,22 @@ rctx_ncache(respctx_t *rctx); */ static void inc_stats(dns_resolver_t *res, isc_statscounter_t counter) { - if (res->view->resstats != NULL) { - isc_stats_increment(res->view->resstats, counter); + if (res->stats != NULL) { + isc_stats_increment(res->stats, counter); } } static void dec_stats(dns_resolver_t *res, isc_statscounter_t counter) { - if (res->view->resstats != NULL) { - isc_stats_decrement(res->view->resstats, counter); + if (res->stats != NULL) { + isc_stats_decrement(res->stats, counter); + } +} + +static void +set_stats(dns_resolver_t *res, isc_statscounter_t counter, uint64_t val) { + if (res->stats != NULL) { + isc_stats_set(res->stats, val, counter); } } @@ -2933,8 +2942,8 @@ resquery_connected(isc_result_t eresult, isc_region_t *region, void *arg) { } else { inc_stats(res, dns_resstatscounter_queryv6); } - if (res->view->resquerystats != NULL) { - dns_rdatatypestats_increment(res->view->resquerystats, + if (res->querystats != NULL) { + dns_rdatatypestats_increment(res->querystats, fctx->type); } break; @@ -10092,6 +10101,13 @@ destroy(dns_resolver_t *res) { REQUIRE(atomic_load_acquire(&res->nfctx) == 0); + if (res->querystats != NULL) { + dns_stats_detach(&res->querystats); + } + if (res->stats != NULL) { + isc_stats_detach(&res->stats); + } + isc_mutex_destroy(&res->primelock); isc_mutex_destroy(&res->lock); for (i = 0; i < res->nbuckets; i++) { @@ -10243,11 +10259,6 @@ dns_resolver_create(dns_view_t *view, isc_taskmgr_t *taskmgr, goto cleanup_res; } - if (view->resstats != NULL) { - isc_stats_set(view->resstats, ntasks, - dns_resstatscounter_buckets); - } - res->buckets = isc_mem_get(view->mctx, ntasks * sizeof(res->buckets[0])); for (uint32_t i = 0; i < ntasks; i++) { @@ -11566,3 +11577,49 @@ dns_resolver_setnonbackofftries(dns_resolver_t *resolver, unsigned int tries) { resolver->nonbackofftries = tries; } + +void +dns_resolver_setstats(dns_resolver_t *res, isc_stats_t *stats) { + REQUIRE(VALID_RESOLVER(res)); + REQUIRE(res->stats == NULL); + + isc_stats_attach(stats, &res->stats); + + /* initialize the bucket "counter"; it's a static value */ + set_stats(res, dns_resstatscounter_buckets, res->nbuckets); +} + +void +dns_resolver_getstats(dns_resolver_t *res, isc_stats_t **statsp) { + REQUIRE(VALID_RESOLVER(res)); + REQUIRE(statsp != NULL && *statsp == NULL); + + if (res->stats != NULL) { + isc_stats_attach(res->stats, statsp); + } +} + +void +dns_resolver_incstats(dns_resolver_t *res, isc_statscounter_t counter) { + REQUIRE(VALID_RESOLVER(res)); + + isc_stats_increment(res->stats, counter); +} + +void +dns_resolver_setquerystats(dns_resolver_t *res, dns_stats_t *stats) { + REQUIRE(VALID_RESOLVER(res)); + REQUIRE(res->querystats == NULL); + + dns_stats_attach(stats, &res->querystats); +} + +void +dns_resolver_getquerystats(dns_resolver_t *res, dns_stats_t **statsp) { + REQUIRE(VALID_RESOLVER(res)); + REQUIRE(statsp != NULL && *statsp == NULL); + + if (res->querystats != NULL) { + dns_stats_attach(res->querystats, statsp); + } +} diff --git a/lib/dns/view.c b/lib/dns/view.c index 969b153ea7..13f55142b5 100644 --- a/lib/dns/view.c +++ b/lib/dns/view.c @@ -433,15 +433,6 @@ destroy(dns_view_t *view) { sizeof(dns_namelist_t) * DNS_VIEW_DELONLYHASH); view->rootexclude = NULL; } - if (view->adbstats != NULL) { - isc_stats_detach(&view->adbstats); - } - if (view->resstats != NULL) { - isc_stats_detach(&view->resstats); - } - if (view->resquerystats != NULL) { - dns_stats_detach(&view->resquerystats); - } if (view->secroots_priv != NULL) { dns_keytable_detach(&view->secroots_priv); } @@ -1741,63 +1732,6 @@ dns_view_freezezones(dns_view_t *view, bool value) { return (dns_zt_freezezones(view->zonetable, view, value)); } -void -dns_view_setadbstats(dns_view_t *view, isc_stats_t *stats) { - REQUIRE(DNS_VIEW_VALID(view)); - REQUIRE(!view->frozen); - REQUIRE(view->adbstats == NULL); - - isc_stats_attach(stats, &view->adbstats); -} - -void -dns_view_getadbstats(dns_view_t *view, isc_stats_t **statsp) { - REQUIRE(DNS_VIEW_VALID(view)); - REQUIRE(statsp != NULL && *statsp == NULL); - - if (view->adbstats != NULL) { - isc_stats_attach(view->adbstats, statsp); - } -} - -void -dns_view_setresstats(dns_view_t *view, isc_stats_t *stats) { - REQUIRE(DNS_VIEW_VALID(view)); - REQUIRE(!view->frozen); - REQUIRE(view->resstats == NULL); - - isc_stats_attach(stats, &view->resstats); -} - -void -dns_view_getresstats(dns_view_t *view, isc_stats_t **statsp) { - REQUIRE(DNS_VIEW_VALID(view)); - REQUIRE(statsp != NULL && *statsp == NULL); - - if (view->resstats != NULL) { - isc_stats_attach(view->resstats, statsp); - } -} - -void -dns_view_setresquerystats(dns_view_t *view, dns_stats_t *stats) { - REQUIRE(DNS_VIEW_VALID(view)); - REQUIRE(!view->frozen); - REQUIRE(view->resquerystats == NULL); - - dns_stats_attach(stats, &view->resquerystats); -} - -void -dns_view_getresquerystats(dns_view_t *view, dns_stats_t **statsp) { - REQUIRE(DNS_VIEW_VALID(view)); - REQUIRE(statsp != NULL && *statsp == NULL); - - if (view->resquerystats != NULL) { - dns_stats_attach(view->resquerystats, statsp); - } -} - isc_result_t dns_view_initntatable(dns_view_t *view, isc_taskmgr_t *taskmgr, isc_timermgr_t *timermgr) { From a2d491279af40ab4bd10db2ba400f471d11fcc54 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 11 May 2022 13:55:01 -0700 Subject: [PATCH 07/11] remove ADB whenshutdown events weakly attaching and detaching the view when creating or destroying the ADB obviates the need for a whenshutdown callback event to do the detaching. remove the dns_adb_whenshutdown() mechanism, since it is no longer needed. --- lib/dns/adb.c | 91 +++++++++--------------------------- lib/dns/include/dns/adb.h | 19 -------- lib/dns/include/dns/events.h | 8 ++-- lib/dns/include/dns/view.h | 1 - lib/dns/view.c | 32 +++---------- 5 files changed, 33 insertions(+), 118 deletions(-) diff --git a/lib/dns/adb.c b/lib/dns/adb.c index 52ebf9bdc8..b41994f710 100644 --- a/lib/dns/adb.c +++ b/lib/dns/adb.c @@ -117,10 +117,7 @@ struct dns_adb { isc_stats_t *stats; - isc_event_t cevent; - bool cevent_out; atomic_bool exiting; - isc_eventlist_t whenshutdown; uint32_t quota; uint32_t atr_freq; @@ -1237,7 +1234,9 @@ clean_finds_at_name(dns_adbname_t *name, isc_eventtype_t evtype, ev->ev_destroy = event_freefind; ev->ev_destroy_arg = find; - DP(DEF_LEVEL, "sending event %p to task %p for find %p", + DP(DEF_LEVEL, + "cfan: sending event %p " + "to task %p for find %p", ev, task, find); isc_task_sendanddetach(&task, (isc_event_t **)&ev); @@ -2045,8 +2044,7 @@ destroy(dns_adb_t *adb) { adb->magic = 0; - isc_task_detach(&adb->task); - + RWLOCK(&adb->names_lock, isc_rwlocktype_write); isc_ht_iter_create(adb->namebuckets, &it); for (result = isc_ht_iter_first(it); result == ISC_R_SUCCESS; result = isc_ht_iter_delcurrent_next(it)) @@ -2058,7 +2056,10 @@ destroy(dns_adb_t *adb) { } isc_ht_iter_destroy(&it); isc_ht_destroy(&adb->namebuckets); + RWUNLOCK(&adb->names_lock, isc_rwlocktype_write); + isc_rwlock_destroy(&adb->names_lock); + RWLOCK(&adb->entries_lock, isc_rwlocktype_write); isc_ht_iter_create(adb->entrybuckets, &it); for (result = isc_ht_iter_first(it); result == ISC_R_SUCCESS; result = isc_ht_iter_delcurrent_next(it)) @@ -2070,12 +2071,14 @@ destroy(dns_adb_t *adb) { } isc_ht_iter_destroy(&it); isc_ht_destroy(&adb->entrybuckets); - - isc_rwlock_destroy(&adb->names_lock); + RWUNLOCK(&adb->entries_lock, isc_rwlocktype_write); isc_rwlock_destroy(&adb->entries_lock); + isc_mutex_destroy(&adb->lock); + isc_task_detach(&adb->task); isc_stats_detach(&adb->stats); + dns_view_weakdetach(&adb->view); isc_mem_putanddetach(&adb->mctx, adb, sizeof(dns_adb_t)); } @@ -2096,7 +2099,6 @@ dns_adb_create(isc_mem_t *mem, dns_view_t *view, isc_taskmgr_t *taskmgr, adb = isc_mem_get(mem, sizeof(dns_adb_t)); *adb = (dns_adb_t){ - .view = view, .taskmgr = taskmgr, }; @@ -2105,11 +2107,7 @@ dns_adb_create(isc_mem_t *mem, dns_view_t *view, isc_taskmgr_t *taskmgr, * that must be NULL for the error return to work properly. */ isc_refcount_init(&adb->references, 1); - ISC_EVENT_INIT(&adb->cevent, sizeof(adb->cevent), 0, NULL, 0, NULL, - NULL, NULL, NULL, NULL); - ISC_LIST_INIT(adb->whenshutdown); - atomic_init(&adb->exiting, false); - + dns_view_weakattach(view, &adb->view); isc_mem_attach(mem, &adb->mctx); isc_ht_init(&adb->namebuckets, adb->mctx, 1, ISC_HT_CASE_INSENSITIVE); @@ -2158,6 +2156,7 @@ free_lock: isc_rwlock_destroy(&adb->names_lock); isc_ht_destroy(&adb->namebuckets); + dns_view_weakdetach(&adb->view); isc_mem_putanddetach(&adb->mctx, adb, sizeof(dns_adb_t)); return (result); @@ -2211,39 +2210,8 @@ dns__adb_detach(dns_adb_t **adbp, const char *func, const char *file, } } -void -dns_adb_whenshutdown(dns_adb_t *adb, isc_task_t *task, isc_event_t **eventp) { - isc_event_t *event = NULL; - - /* - * Send '*eventp' to 'task' when 'adb' has shutdown. - */ - - REQUIRE(DNS_ADB_VALID(adb)); - REQUIRE(eventp != NULL); - - event = *eventp; - *eventp = NULL; - - if (atomic_load(&adb->exiting)) { - /* - * We're already shutdown. Send the event. - */ - event->ev_sender = adb; - isc_task_send(task, &event); - } else { - LOCK(&adb->lock); - isc_task_attach(task, &(isc_task_t *){ NULL }); - event->ev_sender = task; - ISC_LIST_APPEND(adb->whenshutdown, event, ev_link); - UNLOCK(&adb->lock); - } -} - void dns_adb_shutdown(dns_adb_t *adb) { - isc_event_t *event = NULL; - if (!atomic_compare_exchange_strong(&adb->exiting, &(bool){ false }, true)) { return; @@ -2255,17 +2223,6 @@ dns_adb_shutdown(dns_adb_t *adb) { shutdown_names(adb); shutdown_entries(adb); - - LOCK(&adb->lock); - for (event = ISC_LIST_HEAD(adb->whenshutdown); event != NULL; - event = ISC_LIST_HEAD(adb->whenshutdown)) - { - isc_task_t *task = event->ev_sender; - event->ev_sender = adb; - ISC_LIST_UNLINK(adb->whenshutdown, event, ev_link); - isc_task_sendanddetach(&task, &event); - } - UNLOCK(&adb->lock); } /* @@ -3175,7 +3132,7 @@ fetch_callback(isc_task_t *task, isc_event_t *ev) { name = ev->ev_arg; REQUIRE(DNS_ADBNAME_VALID(name)); - adb = name->adb; + dns_adb_attach(name->adb, &adb); REQUIRE(DNS_ADB_VALID(adb)); @@ -3199,9 +3156,6 @@ fetch_callback(isc_task_t *task, isc_event_t *ev) { INSIST(address_type != 0 && fetch != NULL); - dns_resolver_destroyfetch(&fetch->fetch); - dev->fetch = NULL; - ev_status = DNS_EVENT_ADBNOMOREADDRESSES; /* @@ -3219,11 +3173,8 @@ fetch_callback(isc_task_t *task, isc_event_t *ev) { * potentially good data. */ if (NAME_DEAD(name)) { - free_adbfetch(adb, &fetch); - isc_event_free(&ev); - expire_name(&name, DNS_EVENT_ADBCANCELED); - UNLOCK(&nbucket->lock); - return; + ev_status = DNS_EVENT_ADBCANCELED; + goto out; } isc_stdtime_get(&now); @@ -3327,12 +3278,16 @@ check_result: } out: + dns_resolver_destroyfetch(&fetch->fetch); free_adbfetch(adb, &fetch); isc_event_free(&ev); - - clean_finds_at_name(name, ev_status, address_type); - + if (ev_status == DNS_EVENT_ADBCANCELED) { + expire_name(&name, ev_status); + } else { + clean_finds_at_name(name, ev_status, address_type); + } UNLOCK(&nbucket->lock); + dns_adb_detach(&adb); } static isc_result_t diff --git a/lib/dns/include/dns/adb.h b/lib/dns/include/dns/adb.h index 2163a2ce3f..643dfe7588 100644 --- a/lib/dns/include/dns/adb.h +++ b/lib/dns/include/dns/adb.h @@ -308,25 +308,6 @@ dns__adb_detach(dns_adb_t **adb, const char *func, const char *file, * dns_adb_create(). */ -void -dns_adb_whenshutdown(dns_adb_t *adb, isc_task_t *task, isc_event_t **eventp); -/*% - * Send '*eventp' to 'task' when 'adb' has shutdown. - * - * Requires: - * - *\li '*adb' is a valid dns_adb_t. - * - *\li eventp != NULL && *eventp is a valid event. - * - * Ensures: - * - *\li *eventp == NULL - * - *\li The event's sender field is set to the value of adb when the event - * is sent. - */ - void dns_adb_shutdown(dns_adb_t *adb); /*%< diff --git a/lib/dns/include/dns/events.h b/lib/dns/include/dns/events.h index b2ad2e83ef..58ce66df8f 100644 --- a/lib/dns/include/dns/events.h +++ b/lib/dns/include/dns/events.h @@ -20,10 +20,10 @@ * Registry of DNS event numbers. */ -#define DNS_EVENT_FETCHCONTROL (ISC_EVENTCLASS_DNS + 0) -#define DNS_EVENT_FETCHDONE (ISC_EVENTCLASS_DNS + 1) -#define DNS_EVENT_VIEWRESSHUTDOWN (ISC_EVENTCLASS_DNS + 2) -#define DNS_EVENT_VIEWADBSHUTDOWN (ISC_EVENTCLASS_DNS + 3) +#define DNS_EVENT_FETCHCONTROL (ISC_EVENTCLASS_DNS + 0) +#define DNS_EVENT_FETCHDONE (ISC_EVENTCLASS_DNS + 1) +#define DNS_EVENT_VIEWRESSHUTDOWN (ISC_EVENTCLASS_DNS + 2) +/* #define DNS_EVENT_VIEWADBSHUTDOWN (ISC_EVENTCLASS_DNS + 3) */ #define DNS_EVENT_UPDATE (ISC_EVENTCLASS_DNS + 4) #define DNS_EVENT_UPDATEDONE (ISC_EVENTCLASS_DNS + 5) #define DNS_EVENT_DISPATCH (ISC_EVENTCLASS_DNS + 6) diff --git a/lib/dns/include/dns/view.h b/lib/dns/include/dns/view.h index d590dfeaa3..9918e2c2be 100644 --- a/lib/dns/include/dns/view.h +++ b/lib/dns/include/dns/view.h @@ -105,7 +105,6 @@ struct dns_view { bool frozen; isc_task_t *task; isc_event_t resevent; - isc_event_t adbevent; isc_event_t reqevent; bool cacheshared; diff --git a/lib/dns/view.c b/lib/dns/view.c index 13f55142b5..8f2f4fbed2 100644 --- a/lib/dns/view.c +++ b/lib/dns/view.c @@ -78,8 +78,6 @@ static void resolver_shutdown(isc_task_t *task, isc_event_t *event); static void -adb_shutdown(isc_task_t *task, isc_event_t *event); -static void req_shutdown(isc_task_t *task, isc_event_t *event); isc_result_t @@ -133,9 +131,6 @@ dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass, const char *name, ISC_EVENT_INIT(&view->resevent, sizeof(view->resevent), 0, NULL, DNS_EVENT_VIEWRESSHUTDOWN, resolver_shutdown, view, NULL, NULL, NULL); - ISC_EVENT_INIT(&view->adbevent, sizeof(view->adbevent), 0, NULL, - DNS_EVENT_VIEWADBSHUTDOWN, adb_shutdown, view, NULL, - NULL, NULL); ISC_EVENT_INIT(&view->reqevent, sizeof(view->reqevent), 0, NULL, DNS_EVENT_VIEWREQSHUTDOWN, req_shutdown, view, NULL, NULL, NULL); @@ -525,6 +520,7 @@ dns_view_detach(dns_view_t **viewp) { } if (view->adb != NULL) { dns_adb_shutdown(view->adb); + dns_adb_detach(&view->adb); } if (view->requestmgr != NULL) { dns_requestmgr_shutdown(view->requestmgr); @@ -635,21 +631,6 @@ resolver_shutdown(isc_task_t *task, isc_event_t *event) { dns_view_weakdetach(&view); } -static void -adb_shutdown(isc_task_t *task, isc_event_t *event) { - dns_view_t *view = event->ev_arg; - - REQUIRE(event->ev_type == DNS_EVENT_VIEWADBSHUTDOWN); - REQUIRE(DNS_VIEW_VALID(view)); - REQUIRE(view->task == task); - - UNUSED(task); - - isc_event_free(&event); - - dns_view_weakdetach(&view); -} - static void req_shutdown(isc_task_t *task, isc_event_t *event) { dns_view_t *view = event->ev_arg; @@ -702,22 +683,19 @@ dns_view_createresolver(dns_view_t *view, isc_taskmgr_t *taskmgr, isc_task_detach(&view->task); return (result); } + + dns_view_weakattach(view, &(dns_view_t *){ NULL }); event = &view->resevent; dns_resolver_whenshutdown(view->resolver, view->task, &event); - isc_refcount_increment(&view->weakrefs); isc_mem_create(&mctx); isc_mem_setname(mctx, "ADB"); - result = dns_adb_create(mctx, view, taskmgr, &view->adb); isc_mem_detach(&mctx); if (result != ISC_R_SUCCESS) { dns_resolver_shutdown(view->resolver); return (result); } - event = &view->adbevent; - dns_adb_whenshutdown(view->adb, view->task, &event); - isc_refcount_increment(&view->weakrefs); result = dns_requestmgr_create( view->mctx, dns_resolver_taskmgr(view->resolver), @@ -725,12 +703,14 @@ dns_view_createresolver(dns_view_t *view, isc_taskmgr_t *taskmgr, dispatchv6, &view->requestmgr); if (result != ISC_R_SUCCESS) { dns_adb_shutdown(view->adb); + dns_adb_detach(&view->adb); dns_resolver_shutdown(view->resolver); return (result); } + + dns_view_weakattach(view, &(dns_view_t *){ NULL }); event = &view->reqevent; dns_requestmgr_whenshutdown(view->requestmgr, view->task, &event); - isc_refcount_increment(&view->weakrefs); return (ISC_R_SUCCESS); } From 51cd295c4bf7535ee7dd905dc14bd0ae92b2a128 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 11 May 2022 13:55:01 -0700 Subject: [PATCH 08/11] remove resolver whenshutdown events weakly attaching and detaching when creating and destroying the resolver obviates the need to have a callback event to do the weak detach. remove the dns_resolver_whenshutdown() mechanism, as it is now unused. --- lib/dns/adb.c | 4 +- lib/dns/include/dns/events.h | 6 +-- lib/dns/include/dns/resolver.h | 24 --------- lib/dns/include/dns/view.h | 1 - lib/dns/resolver.c | 91 +++++----------------------------- lib/dns/view.c | 43 +++++----------- 6 files changed, 32 insertions(+), 137 deletions(-) diff --git a/lib/dns/adb.c b/lib/dns/adb.c index b41994f710..66d82955c9 100644 --- a/lib/dns/adb.c +++ b/lib/dns/adb.c @@ -487,7 +487,9 @@ DP(int level, const char *format, ...) { */ static void inc_resstats(dns_adb_t *adb, isc_statscounter_t counter) { - dns_resolver_incstats(adb->view->resolver, counter); + if (adb->view != NULL && adb->view->resolver != NULL) { + dns_resolver_incstats(adb->view->resolver, counter); + } } /*% diff --git a/lib/dns/include/dns/events.h b/lib/dns/include/dns/events.h index 58ce66df8f..9be2f705cb 100644 --- a/lib/dns/include/dns/events.h +++ b/lib/dns/include/dns/events.h @@ -20,9 +20,9 @@ * Registry of DNS event numbers. */ -#define DNS_EVENT_FETCHCONTROL (ISC_EVENTCLASS_DNS + 0) -#define DNS_EVENT_FETCHDONE (ISC_EVENTCLASS_DNS + 1) -#define DNS_EVENT_VIEWRESSHUTDOWN (ISC_EVENTCLASS_DNS + 2) +#define DNS_EVENT_FETCHCONTROL (ISC_EVENTCLASS_DNS + 0) +#define DNS_EVENT_FETCHDONE (ISC_EVENTCLASS_DNS + 1) +/* #define DNS_EVENT_VIEWRESSHUTDOWN (ISC_EVENTCLASS_DNS + 2) */ /* #define DNS_EVENT_VIEWADBSHUTDOWN (ISC_EVENTCLASS_DNS + 3) */ #define DNS_EVENT_UPDATE (ISC_EVENTCLASS_DNS + 4) #define DNS_EVENT_UPDATEDONE (ISC_EVENTCLASS_DNS + 5) diff --git a/lib/dns/include/dns/resolver.h b/lib/dns/include/dns/resolver.h index d17b28a177..7264ff1ab6 100644 --- a/lib/dns/include/dns/resolver.h +++ b/lib/dns/include/dns/resolver.h @@ -241,30 +241,6 @@ dns_resolver_prime(dns_resolver_t *res); *\li 'res' is a valid, frozen resolver. */ -void -dns_resolver_whenshutdown(dns_resolver_t *res, isc_task_t *task, - isc_event_t **eventp); -/*%< - * Send '*eventp' to 'task' when 'res' has completed shutdown. - * - * Notes: - * - *\li It is not safe to detach the last reference to 'res' until - * shutdown is complete. - * - * Requires: - * - *\li 'res' is a valid resolver. - * - *\li 'task' is a valid task. - * - *\li *eventp is a valid event. - * - * Ensures: - * - *\li *eventp == NULL. - */ - void dns_resolver_shutdown(dns_resolver_t *res); /*%< diff --git a/lib/dns/include/dns/view.h b/lib/dns/include/dns/view.h index 9918e2c2be..1a620468b2 100644 --- a/lib/dns/include/dns/view.h +++ b/lib/dns/include/dns/view.h @@ -104,7 +104,6 @@ struct dns_view { isc_mutex_t lock; bool frozen; isc_task_t *task; - isc_event_t resevent; isc_event_t reqevent; bool cacheshared; diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 02bfb2a72e..1877dd36dc 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -570,7 +570,6 @@ struct dns_resolver { atomic_bool priming; /* Locked by lock. */ - isc_eventlist_t whenshutdown; isc_refcount_t activebuckets; unsigned int spillat; /* clients-per-query */ @@ -645,8 +644,6 @@ static isc_result_t fctx_minimize_qname(fetchctx_t *fctx); static void fctx_destroy(fetchctx_t *fctx, bool exiting); -static void -send_shutdown_events(dns_resolver_t *res); static isc_result_t ncache_adderesult(dns_message_t *message, dns_db_t *cache, dns_dbnode_t *node, dns_rdatatype_t covers, isc_stdtime_t now, dns_ttl_t minttl, @@ -4360,7 +4357,6 @@ fctx_destroy(fetchctx_t *fctx, bool exiting) { isc_sockaddr_t *sa = NULL, *next_sa = NULL; struct tried *tried = NULL; unsigned int bucketnum; - bool bucket_empty = false; REQUIRE(VALID_FCTX(fctx)); REQUIRE(ISC_LIST_EMPTY(fctx->events)); @@ -4377,27 +4373,19 @@ fctx_destroy(fetchctx_t *fctx, bool exiting) { LOCK(&res->buckets[bucketnum].lock); REQUIRE(fctx->state != fetchstate_active); - ISC_LIST_UNLINK(res->buckets[bucketnum].fctxs, fctx, link); INSIST(atomic_fetch_sub_release(&res->nfctx, 1) > 0); dec_stats(res, dns_resstatscounter_nfetch); - if (atomic_load_acquire(&res->buckets[bucketnum].exiting) && + if (exiting && atomic_load_acquire(&res->buckets[bucketnum].exiting) && ISC_LIST_EMPTY(res->buckets[bucketnum].fctxs)) { - bucket_empty = true; + isc_refcount_decrement(&res->activebuckets); } UNLOCK(&res->buckets[bucketnum].lock); - if (bucket_empty && exiting && - isc_refcount_decrement(&res->activebuckets) == 1) { - LOCK(&res->lock); - send_shutdown_events(res); - UNLOCK(&res->lock); - } - isc_refcount_destroy(&fctx->references); /* @@ -10101,6 +10089,13 @@ destroy(dns_resolver_t *res) { REQUIRE(atomic_load_acquire(&res->nfctx) == 0); + /* These must be run before zeroing the magic number */ + dns_resolver_reset_algorithms(res); + dns_resolver_reset_ds_digests(res); + dns_resolver_resetmustbesecure(res); + + res->magic = 0; + if (res->querystats != NULL) { dns_stats_detach(&res->querystats); } @@ -10136,35 +10131,12 @@ destroy(dns_resolver_t *res) { } isc_mem_put(res->mctx, a, sizeof(*a)); } - dns_resolver_reset_algorithms(res); - dns_resolver_reset_ds_digests(res); dns_badcache_destroy(&res->badcache); - dns_resolver_resetmustbesecure(res); isc_timer_destroy(&res->spillattimer); - res->magic = 0; + dns_view_weakdetach(&res->view); isc_mem_putanddetach(&res->mctx, res, sizeof(*res)); } -static void -send_shutdown_events(dns_resolver_t *res) { - isc_event_t *event, *next_event; - isc_task_t *etask; - - /* - * Caller must be holding the resolver lock. - */ - - for (event = ISC_LIST_HEAD(res->whenshutdown); event != NULL; - event = next_event) - { - next_event = ISC_LIST_NEXT(event, ev_link); - ISC_LIST_UNLINK(res->whenshutdown, event, ev_link); - etask = event->ev_sender; - event->ev_sender = res; - isc_task_sendanddetach(&etask, &event); - } -} - static void spillattimer_countdown(isc_task_t *task, isc_event_t *event) { dns_resolver_t *res = event->ev_arg; @@ -10227,7 +10199,6 @@ dns_resolver_create(dns_view_t *view, isc_taskmgr_t *taskmgr, .timermgr = timermgr, .taskmgr = taskmgr, .dispatchmgr = dispatchmgr, - .view = view, .options = options, .udpsize = DEFAULT_EDNS_BUFSIZE, .spillatmin = 10, @@ -10245,12 +10216,12 @@ dns_resolver_create(dns_view_t *view, isc_taskmgr_t *taskmgr, atomic_init(&res->activebuckets, ntasks); + dns_view_weakattach(view, &res->view); isc_mem_attach(view->mctx, &res->mctx); res->quotaresp[dns_quotatype_zone] = DNS_R_DROP; res->quotaresp[dns_quotatype_server] = DNS_R_SERVFAIL; isc_refcount_init(&res->references, 1); - ISC_LIST_INIT(res->whenshutdown); ISC_LIST_INIT(res->alternates); result = dns_badcache_init(res->mctx, DNS_RESOLVER_BADCACHESIZE, @@ -10350,6 +10321,7 @@ cleanup_buckets: dns_badcache_destroy(&res->badcache); cleanup_res: + dns_view_weakdetach(&res->view); isc_mem_putanddetach(&res->mctx, res, sizeof(*res)); return (result); } @@ -10478,43 +10450,12 @@ dns_resolver_attach(dns_resolver_t *source, dns_resolver_t **targetp) { *targetp = source; } -void -dns_resolver_whenshutdown(dns_resolver_t *res, isc_task_t *task, - isc_event_t **eventp) { - isc_event_t *event = NULL; - - REQUIRE(VALID_RESOLVER(res)); - REQUIRE(eventp != NULL); - - event = *eventp; - *eventp = NULL; - - LOCK(&res->lock); - - if (atomic_load_acquire(&res->exiting) && - atomic_load_acquire(&res->activebuckets) == 0) - { - /* - * We're already shutdown. Send the event. - */ - event->ev_sender = res; - isc_task_send(task, &event); - } else { - isc_task_attach(task, &(isc_task_t *){ NULL }); - event->ev_sender = task; - ISC_LIST_APPEND(res->whenshutdown, event, ev_link); - } - - UNLOCK(&res->lock); -} - void dns_resolver_shutdown(dns_resolver_t *res) { unsigned int i; fetchctx_t *fctx; isc_result_t result; bool is_false = false; - bool is_done = false; REQUIRE(VALID_RESOLVER(res)); @@ -10533,16 +10474,10 @@ dns_resolver_shutdown(dns_resolver_t *res) { } atomic_store(&res->buckets[i].exiting, true); if (ISC_LIST_EMPTY(res->buckets[i].fctxs)) { - if (isc_refcount_decrement( - &res->activebuckets) == 1) { - is_done = true; - } + isc_refcount_decrement(&res->activebuckets); } UNLOCK(&res->buckets[i].lock); } - if (is_done) { - send_shutdown_events(res); - } result = isc_timer_reset(res->spillattimer, isc_timertype_inactive, NULL, true); RUNTIME_CHECK(result == ISC_R_SUCCESS); diff --git a/lib/dns/view.c b/lib/dns/view.c index 8f2f4fbed2..8e15efeff9 100644 --- a/lib/dns/view.c +++ b/lib/dns/view.c @@ -75,8 +75,6 @@ #define DNS_VIEW_DELONLYHASH 111 #define DNS_VIEW_FAILCACHESIZE 1021 -static void -resolver_shutdown(isc_task_t *task, isc_event_t *event); static void req_shutdown(isc_task_t *task, isc_event_t *event); @@ -128,9 +126,6 @@ dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass, const char *name, ISC_LINK_INIT(view, link); - ISC_EVENT_INIT(&view->resevent, sizeof(view->resevent), 0, NULL, - DNS_EVENT_VIEWRESSHUTDOWN, resolver_shutdown, view, NULL, - NULL, NULL); ISC_EVENT_INIT(&view->reqevent, sizeof(view->reqevent), 0, NULL, DNS_EVENT_VIEWREQSHUTDOWN, req_shutdown, view, NULL, NULL, NULL); @@ -517,6 +512,7 @@ dns_view_detach(dns_view_t **viewp) { if (view->resolver != NULL) { dns_resolver_shutdown(view->resolver); + dns_resolver_detach(&view->resolver); } if (view->adb != NULL) { dns_adb_shutdown(view->adb); @@ -616,21 +612,6 @@ dns_view_weakdetach(dns_view_t **viewp) { } } -static void -resolver_shutdown(isc_task_t *task, isc_event_t *event) { - dns_view_t *view = event->ev_arg; - - REQUIRE(event->ev_type == DNS_EVENT_VIEWRESSHUTDOWN); - REQUIRE(DNS_VIEW_VALID(view)); - REQUIRE(view->task == task); - - UNUSED(task); - - isc_event_free(&event); - - dns_view_weakdetach(&view); -} - static void req_shutdown(isc_task_t *task, isc_event_t *event) { dns_view_t *view = event->ev_arg; @@ -684,17 +665,12 @@ dns_view_createresolver(dns_view_t *view, isc_taskmgr_t *taskmgr, return (result); } - dns_view_weakattach(view, &(dns_view_t *){ NULL }); - event = &view->resevent; - dns_resolver_whenshutdown(view->resolver, view->task, &event); - isc_mem_create(&mctx); isc_mem_setname(mctx, "ADB"); result = dns_adb_create(mctx, view, taskmgr, &view->adb); isc_mem_detach(&mctx); if (result != ISC_R_SUCCESS) { - dns_resolver_shutdown(view->resolver); - return (result); + goto cleanup_resolver; } result = dns_requestmgr_create( @@ -702,10 +678,7 @@ dns_view_createresolver(dns_view_t *view, isc_taskmgr_t *taskmgr, dns_resolver_dispatchmgr(view->resolver), dispatchv4, dispatchv6, &view->requestmgr); if (result != ISC_R_SUCCESS) { - dns_adb_shutdown(view->adb); - dns_adb_detach(&view->adb); - dns_resolver_shutdown(view->resolver); - return (result); + goto cleanup_adb; } dns_view_weakattach(view, &(dns_view_t *){ NULL }); @@ -713,6 +686,16 @@ dns_view_createresolver(dns_view_t *view, isc_taskmgr_t *taskmgr, dns_requestmgr_whenshutdown(view->requestmgr, view->task, &event); return (ISC_R_SUCCESS); + +cleanup_adb: + dns_adb_shutdown(view->adb); + dns_adb_detach(&view->adb); + +cleanup_resolver: + dns_resolver_shutdown(view->resolver); + dns_resolver_detach(&view->resolver); + + return (result); } void From d7ffd897ef61542a1d2e6763a61171b102b83639 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 11 May 2022 13:55:01 -0700 Subject: [PATCH 09/11] remove requestmgr whenshutdown events the request manager has no direct dependency on the view, so there's no need for a weak reference. remove the dns_requestmgr_whenshutdown() mechanism since it is no longer used. --- lib/dns/include/dns/events.h | 80 +++++++++++++++++------------------ lib/dns/include/dns/request.h | 24 ----------- lib/dns/include/dns/view.h | 1 - lib/dns/request.c | 79 +--------------------------------- lib/dns/view.c | 28 +----------- 5 files changed, 42 insertions(+), 170 deletions(-) diff --git a/lib/dns/include/dns/events.h b/lib/dns/include/dns/events.h index 9be2f705cb..7a514af530 100644 --- a/lib/dns/include/dns/events.h +++ b/lib/dns/include/dns/events.h @@ -42,44 +42,44 @@ #define DNS_EVENT_VALIDATORDONE (ISC_EVENTCLASS_DNS + 19) #define DNS_EVENT_REQUESTDONE (ISC_EVENTCLASS_DNS + 20) #define DNS_EVENT_VALIDATORSTART (ISC_EVENTCLASS_DNS + 21) -#define DNS_EVENT_VIEWREQSHUTDOWN (ISC_EVENTCLASS_DNS + 22) -#define DNS_EVENT_NOTIFYSENDTOADDR (ISC_EVENTCLASS_DNS + 23) -#define DNS_EVENT_ZONE (ISC_EVENTCLASS_DNS + 24) -#define DNS_EVENT_ZONESTARTXFRIN (ISC_EVENTCLASS_DNS + 25) -#define DNS_EVENT_MASTERQUANTUM (ISC_EVENTCLASS_DNS + 26) -#define DNS_EVENT_CACHEOVERMEM (ISC_EVENTCLASS_DNS + 27) -#define DNS_EVENT_MASTERNEXTZONE (ISC_EVENTCLASS_DNS + 28) -#define DNS_EVENT_IOREADY (ISC_EVENTCLASS_DNS + 29) -#define DNS_EVENT_LOOKUPDONE (ISC_EVENTCLASS_DNS + 30) -#define DNS_EVENT_RBTDEADNODES (ISC_EVENTCLASS_DNS + 31) -#define DNS_EVENT_DISPATCHCONTROL (ISC_EVENTCLASS_DNS + 32) -#define DNS_EVENT_REQUESTCONTROL (ISC_EVENTCLASS_DNS + 33) -#define DNS_EVENT_DUMPQUANTUM (ISC_EVENTCLASS_DNS + 34) +/* #define DNS_EVENT_VIEWREQSHUTDOWN (ISC_EVENTCLASS_DNS + 22) */ +#define DNS_EVENT_NOTIFYSENDTOADDR (ISC_EVENTCLASS_DNS + 23) +#define DNS_EVENT_ZONE (ISC_EVENTCLASS_DNS + 24) +#define DNS_EVENT_ZONESTARTXFRIN (ISC_EVENTCLASS_DNS + 25) +#define DNS_EVENT_MASTERQUANTUM (ISC_EVENTCLASS_DNS + 26) +#define DNS_EVENT_CACHEOVERMEM (ISC_EVENTCLASS_DNS + 27) +#define DNS_EVENT_MASTERNEXTZONE (ISC_EVENTCLASS_DNS + 28) +#define DNS_EVENT_IOREADY (ISC_EVENTCLASS_DNS + 29) +#define DNS_EVENT_LOOKUPDONE (ISC_EVENTCLASS_DNS + 30) +#define DNS_EVENT_RBTDEADNODES (ISC_EVENTCLASS_DNS + 31) +#define DNS_EVENT_DISPATCHCONTROL (ISC_EVENTCLASS_DNS + 32) +#define DNS_EVENT_REQUESTCONTROL (ISC_EVENTCLASS_DNS + 33) +#define DNS_EVENT_DUMPQUANTUM (ISC_EVENTCLASS_DNS + 34) /* #define DNS_EVENT_IMPORTRECVDONE (ISC_EVENTCLASS_DNS + 35) */ -#define DNS_EVENT_FREESTORAGE (ISC_EVENTCLASS_DNS + 36) -#define DNS_EVENT_VIEWACACHESHUTDOWN (ISC_EVENTCLASS_DNS + 37) -#define DNS_EVENT_ACACHECONTROL (ISC_EVENTCLASS_DNS + 38) -#define DNS_EVENT_ACACHECLEAN (ISC_EVENTCLASS_DNS + 39) -#define DNS_EVENT_ACACHEOVERMEM (ISC_EVENTCLASS_DNS + 40) -#define DNS_EVENT_RBTPRUNE (ISC_EVENTCLASS_DNS + 41) -#define DNS_EVENT_MANAGEKEYS (ISC_EVENTCLASS_DNS + 42) -#define DNS_EVENT_CLIENTRESDONE (ISC_EVENTCLASS_DNS + 43) -#define DNS_EVENT_CLIENTREQDONE (ISC_EVENTCLASS_DNS + 44) -#define DNS_EVENT_ADBGROWENTRIES (ISC_EVENTCLASS_DNS + 45) -#define DNS_EVENT_ADBGROWNAMES (ISC_EVENTCLASS_DNS + 46) -#define DNS_EVENT_ZONESECURESERIAL (ISC_EVENTCLASS_DNS + 47) -#define DNS_EVENT_ZONESECUREDB (ISC_EVENTCLASS_DNS + 48) -#define DNS_EVENT_ZONELOAD (ISC_EVENTCLASS_DNS + 49) -#define DNS_EVENT_KEYDONE (ISC_EVENTCLASS_DNS + 50) -#define DNS_EVENT_SETNSEC3PARAM (ISC_EVENTCLASS_DNS + 51) -#define DNS_EVENT_SETSERIAL (ISC_EVENTCLASS_DNS + 52) -#define DNS_EVENT_CATZUPDATED (ISC_EVENTCLASS_DNS + 53) -#define DNS_EVENT_CATZADDZONE (ISC_EVENTCLASS_DNS + 54) -#define DNS_EVENT_CATZMODZONE (ISC_EVENTCLASS_DNS + 55) -#define DNS_EVENT_CATZDELZONE (ISC_EVENTCLASS_DNS + 56) -#define DNS_EVENT_RPZUPDATED (ISC_EVENTCLASS_DNS + 57) -#define DNS_EVENT_STARTUPDATE (ISC_EVENTCLASS_DNS + 58) -#define DNS_EVENT_TRYSTALE (ISC_EVENTCLASS_DNS + 59) -#define DNS_EVENT_ZONEFLUSH (ISC_EVENTCLASS_DNS + 60) -#define DNS_EVENT_CHECKDSSENDTOADDR (ISC_EVENTCLASS_DNS + 61) -#define DNS_EVENT_CACHESHUTDOWN (ISC_EVENTCLASS_DNS + 62) +#define DNS_EVENT_FREESTORAGE (ISC_EVENTCLASS_DNS + 36) +/* #define DNS_EVENT_VIEWACACHESHUTDOWN (ISC_EVENTCLASS_DNS + 37) */ +#define DNS_EVENT_ACACHECONTROL (ISC_EVENTCLASS_DNS + 38) +#define DNS_EVENT_ACACHECLEAN (ISC_EVENTCLASS_DNS + 39) +#define DNS_EVENT_ACACHEOVERMEM (ISC_EVENTCLASS_DNS + 40) +#define DNS_EVENT_RBTPRUNE (ISC_EVENTCLASS_DNS + 41) +#define DNS_EVENT_MANAGEKEYS (ISC_EVENTCLASS_DNS + 42) +#define DNS_EVENT_CLIENTRESDONE (ISC_EVENTCLASS_DNS + 43) +#define DNS_EVENT_CLIENTREQDONE (ISC_EVENTCLASS_DNS + 44) +#define DNS_EVENT_ADBGROWENTRIES (ISC_EVENTCLASS_DNS + 45) +#define DNS_EVENT_ADBGROWNAMES (ISC_EVENTCLASS_DNS + 46) +#define DNS_EVENT_ZONESECURESERIAL (ISC_EVENTCLASS_DNS + 47) +#define DNS_EVENT_ZONESECUREDB (ISC_EVENTCLASS_DNS + 48) +#define DNS_EVENT_ZONELOAD (ISC_EVENTCLASS_DNS + 49) +#define DNS_EVENT_KEYDONE (ISC_EVENTCLASS_DNS + 50) +#define DNS_EVENT_SETNSEC3PARAM (ISC_EVENTCLASS_DNS + 51) +#define DNS_EVENT_SETSERIAL (ISC_EVENTCLASS_DNS + 52) +#define DNS_EVENT_CATZUPDATED (ISC_EVENTCLASS_DNS + 53) +#define DNS_EVENT_CATZADDZONE (ISC_EVENTCLASS_DNS + 54) +#define DNS_EVENT_CATZMODZONE (ISC_EVENTCLASS_DNS + 55) +#define DNS_EVENT_CATZDELZONE (ISC_EVENTCLASS_DNS + 56) +#define DNS_EVENT_RPZUPDATED (ISC_EVENTCLASS_DNS + 57) +#define DNS_EVENT_STARTUPDATE (ISC_EVENTCLASS_DNS + 58) +#define DNS_EVENT_TRYSTALE (ISC_EVENTCLASS_DNS + 59) +#define DNS_EVENT_ZONEFLUSH (ISC_EVENTCLASS_DNS + 60) +#define DNS_EVENT_CHECKDSSENDTOADDR (ISC_EVENTCLASS_DNS + 61) +#define DNS_EVENT_CACHESHUTDOWN (ISC_EVENTCLASS_DNS + 62) diff --git a/lib/dns/include/dns/request.h b/lib/dns/include/dns/request.h index 8dbee8c9ff..342277c9f2 100644 --- a/lib/dns/include/dns/request.h +++ b/lib/dns/include/dns/request.h @@ -84,30 +84,6 @@ dns_requestmgr_create(isc_mem_t *mctx, isc_taskmgr_t *taskmgr, *\li Any other result indicates failure. */ -void -dns_requestmgr_whenshutdown(dns_requestmgr_t *requestmgr, isc_task_t *task, - isc_event_t **eventp); -/*%< - * Send '*eventp' to 'task' when 'requestmgr' has completed shutdown. - * - * Notes: - * - *\li It is not safe to detach the last reference to 'requestmgr' until - * shutdown is complete. - * - * Requires: - * - *\li 'requestmgr' is a valid request manager. - * - *\li 'task' is a valid task. - * - *\li *eventp is a valid event. - * - * Ensures: - * - *\li *eventp == NULL. - */ - void dns_requestmgr_shutdown(dns_requestmgr_t *requestmgr); /*%< diff --git a/lib/dns/include/dns/view.h b/lib/dns/include/dns/view.h index 1a620468b2..b09230ba0a 100644 --- a/lib/dns/include/dns/view.h +++ b/lib/dns/include/dns/view.h @@ -104,7 +104,6 @@ struct dns_view { isc_mutex_t lock; bool frozen; isc_task_t *task; - isc_event_t reqevent; bool cacheshared; /* Configurable data. */ diff --git a/lib/dns/request.c b/lib/dns/request.c index 5a70ad6d89..54db330833 100644 --- a/lib/dns/request.c +++ b/lib/dns/request.c @@ -58,7 +58,6 @@ struct dns_requestmgr { dns_dispatch_t *dispatchv4; dns_dispatch_t *dispatchv6; atomic_bool exiting; - isc_eventlist_t whenshutdown; unsigned int hash; isc_mutex_t locks[DNS_REQUEST_NLOCKS]; dns_requestlist_t requests; @@ -103,9 +102,6 @@ static void mgr_destroy(dns_requestmgr_t *requestmgr); static unsigned int mgr_gethash(dns_requestmgr_t *requestmgr); -static void -send_shutdown_events(dns_requestmgr_t *requestmgr); - static isc_result_t req_render(dns_message_t *message, isc_buffer_t **buffer, unsigned int options, isc_mem_t *mctx); @@ -166,7 +162,6 @@ dns_requestmgr_create(isc_mem_t *mctx, isc_taskmgr_t *taskmgr, isc_refcount_init(&requestmgr->references, 1); - ISC_LIST_INIT(requestmgr->whenshutdown); ISC_LIST_INIT(requestmgr->requests); atomic_init(&requestmgr->exiting, false); @@ -179,37 +174,6 @@ dns_requestmgr_create(isc_mem_t *mctx, isc_taskmgr_t *taskmgr, return (ISC_R_SUCCESS); } -void -dns_requestmgr_whenshutdown(dns_requestmgr_t *requestmgr, isc_task_t *task, - isc_event_t **eventp) { - isc_task_t *tclone; - isc_event_t *event; - - req_log(ISC_LOG_DEBUG(3), "dns_requestmgr_whenshutdown"); - - REQUIRE(VALID_REQUESTMGR(requestmgr)); - REQUIRE(eventp != NULL); - - event = *eventp; - *eventp = NULL; - - LOCK(&requestmgr->lock); - - if (atomic_load_acquire(&requestmgr->exiting)) { - /* - * We're already shutdown. Send the event. - */ - event->ev_sender = requestmgr; - isc_task_send(task, &event); - } else { - tclone = NULL; - isc_task_attach(task, &tclone); - event->ev_sender = tclone; - ISC_LIST_APPEND(requestmgr->whenshutdown, event, ev_link); - } - UNLOCK(&requestmgr->lock); -} - void dns_requestmgr_shutdown(dns_requestmgr_t *requestmgr) { dns_request_t *request; @@ -230,11 +194,6 @@ dns_requestmgr_shutdown(dns_requestmgr_t *requestmgr) { { dns_request_cancel(request); } - - if (ISC_LIST_EMPTY(requestmgr->requests)) { - send_shutdown_events(requestmgr); - } - UNLOCK(&requestmgr->lock); } @@ -278,28 +237,6 @@ dns_requestmgr_detach(dns_requestmgr_t **requestmgrp) { } } -/* FIXME */ -static void -send_shutdown_events(dns_requestmgr_t *requestmgr) { - isc_event_t *event, *next_event; - isc_task_t *etask; - - req_log(ISC_LOG_DEBUG(3), "send_shutdown_events: %p", requestmgr); - - /* - * Caller must be holding the manager lock. - */ - for (event = ISC_LIST_HEAD(requestmgr->whenshutdown); event != NULL; - event = next_event) - { - next_event = ISC_LIST_NEXT(event, ev_link); - ISC_LIST_UNLINK(requestmgr->whenshutdown, event, ev_link); - etask = event->ev_sender; - event->ev_sender = requestmgr; - isc_task_sendanddetach(&etask, &event); - } -} - static void mgr_destroy(dns_requestmgr_t *requestmgr) { int i; @@ -1163,27 +1100,13 @@ req_attach(dns_request_t *source, dns_request_t **targetp) { static void req_detach(dns_request_t **requestp) { dns_request_t *request = NULL; - uint_fast32_t ref; REQUIRE(requestp != NULL && VALID_REQUEST(*requestp)); request = *requestp; *requestp = NULL; - ref = isc_refcount_decrement(&request->references); - - if (request->requestmgr != NULL && - atomic_load_acquire(&request->requestmgr->exiting)) - { - /* We are shutting down and this was last request */ - LOCK(&request->requestmgr->lock); - if (ISC_LIST_EMPTY(request->requestmgr->requests)) { - send_shutdown_events(request->requestmgr); - } - UNLOCK(&request->requestmgr->lock); - } - - if (ref == 1) { + if (isc_refcount_decrement(&request->references) == 1) { req_destroy(request); } } diff --git a/lib/dns/view.c b/lib/dns/view.c index 8e15efeff9..75516bae28 100644 --- a/lib/dns/view.c +++ b/lib/dns/view.c @@ -75,9 +75,6 @@ #define DNS_VIEW_DELONLYHASH 111 #define DNS_VIEW_FAILCACHESIZE 1021 -static void -req_shutdown(isc_task_t *task, isc_event_t *event); - isc_result_t dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass, const char *name, dns_view_t **viewp) { @@ -126,10 +123,6 @@ dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass, const char *name, ISC_LINK_INIT(view, link); - ISC_EVENT_INIT(&view->reqevent, sizeof(view->reqevent), 0, NULL, - DNS_EVENT_VIEWREQSHUTDOWN, req_shutdown, view, NULL, - NULL, NULL); - isc_mem_attach(mctx, &view->mctx); isc_mutex_init(&view->lock); @@ -520,6 +513,7 @@ dns_view_detach(dns_view_t **viewp) { } if (view->requestmgr != NULL) { dns_requestmgr_shutdown(view->requestmgr); + dns_requestmgr_detach(&view->requestmgr); } LOCK(&view->lock); @@ -612,21 +606,6 @@ dns_view_weakdetach(dns_view_t **viewp) { } } -static void -req_shutdown(isc_task_t *task, isc_event_t *event) { - dns_view_t *view = event->ev_arg; - - REQUIRE(event->ev_type == DNS_EVENT_VIEWREQSHUTDOWN); - REQUIRE(DNS_VIEW_VALID(view)); - REQUIRE(view->task == task); - - UNUSED(task); - - isc_event_free(&event); - - dns_view_weakdetach(&view); -} - isc_result_t dns_view_createzonetable(dns_view_t *view) { REQUIRE(DNS_VIEW_VALID(view)); @@ -644,7 +623,6 @@ dns_view_createresolver(dns_view_t *view, isc_taskmgr_t *taskmgr, dns_dispatch_t *dispatchv4, dns_dispatch_t *dispatchv6) { isc_result_t result; - isc_event_t *event = NULL; isc_mem_t *mctx = NULL; REQUIRE(DNS_VIEW_VALID(view)); @@ -681,10 +659,6 @@ dns_view_createresolver(dns_view_t *view, isc_taskmgr_t *taskmgr, goto cleanup_adb; } - dns_view_weakattach(view, &(dns_view_t *){ NULL }); - event = &view->reqevent; - dns_requestmgr_whenshutdown(view->requestmgr, view->task, &event); - return (ISC_R_SUCCESS); cleanup_adb: From 3b13edc5bd48e861005c5bd6a9aa1f6c66a604e6 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Thu, 12 May 2022 15:51:10 -0700 Subject: [PATCH 10/11] attach the resolver to the ADB the ADB depends on the resolver, but previously only accessed it via the view. as view->resolver may now be detached before the ADB finishes, a shutdown race was possible. attaching to the resolver directly prevents this. --- lib/dns/adb.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/lib/dns/adb.c b/lib/dns/adb.c index 66d82955c9..2cf2e85dcd 100644 --- a/lib/dns/adb.c +++ b/lib/dns/adb.c @@ -103,6 +103,7 @@ struct dns_adb { isc_mutex_t lock; isc_mem_t *mctx; dns_view_t *view; + dns_resolver_t *res; isc_taskmgr_t *taskmgr; isc_task_t *task; @@ -487,8 +488,8 @@ DP(int level, const char *format, ...) { */ static void inc_resstats(dns_adb_t *adb, isc_statscounter_t counter) { - if (adb->view != NULL && adb->view->resolver != NULL) { - dns_resolver_incstats(adb->view->resolver, counter); + if (adb->res != NULL) { + dns_resolver_incstats(adb->res, counter); } } @@ -2080,6 +2081,7 @@ destroy(dns_adb_t *adb) { isc_task_detach(&adb->task); isc_stats_detach(&adb->stats); + dns_resolver_detach(&adb->res); dns_view_weakdetach(&adb->view); isc_mem_putanddetach(&adb->mctx, adb, sizeof(dns_adb_t)); } @@ -2110,6 +2112,7 @@ dns_adb_create(isc_mem_t *mem, dns_view_t *view, isc_taskmgr_t *taskmgr, */ isc_refcount_init(&adb->references, 1); dns_view_weakattach(view, &adb->view); + dns_resolver_attach(view->resolver, &adb->res); isc_mem_attach(mem, &adb->mctx); isc_ht_init(&adb->namebuckets, adb->mctx, 1, ISC_HT_CASE_INSENSITIVE); @@ -2158,6 +2161,7 @@ free_lock: isc_rwlock_destroy(&adb->names_lock); isc_ht_destroy(&adb->namebuckets); + dns_resolver_detach(&adb->res); dns_view_weakdetach(&adb->view); isc_mem_putanddetach(&adb->mctx, adb, sizeof(dns_adb_t)); @@ -3343,9 +3347,9 @@ fetch_name(dns_adbname_t *adbname, bool start_at_zone, unsigned int depth, * domain and nameservers. */ result = dns_resolver_createfetch( - adb->view->resolver, &adbname->name, type, name, nameservers, - NULL, NULL, 0, options, depth, qc, adb->task, fetch_callback, - adbname, &fetch->rdataset, NULL, &fetch->fetch); + adb->res, &adbname->name, type, name, nameservers, NULL, NULL, + 0, options, depth, qc, adb->task, fetch_callback, adbname, + &fetch->rdataset, NULL, &fetch->fetch); if (result != ISC_R_SUCCESS) { DP(ENTER_LEVEL, "fetch_name: createfetch failed with %s", isc_result_totext(result)); From 578297936cd629ea6c5727b421d1b829a878452c Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Thu, 12 May 2022 16:57:43 -0700 Subject: [PATCH 11/11] CHANGES --- CHANGES | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGES b/CHANGES index 92a04312ee..400c185fdd 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,8 @@ +5889. [cleanup] Refactored and simplified the shutdown processes in + dns_view, dns_resolver, dns_requestmgr, and dns_adb + by reducing interdependencies between the objects. + [GL !6278] + 5888. [bug] Only write key files if the dnssec-policy keymgr has changed the metadata. [GL #3302]