From: Ondřej Surý Date: Mon, 10 Jul 2023 09:36:19 +0000 (+0200) Subject: Use cds_lfht for updatenotify mechanism in dns_db unit X-Git-Tag: v9.19.16~12^2~1 X-Git-Url: http://git.ipfire.org/gitweb/?a=commitdiff_plain;h=a1afa31a5a7d318508efe5a32001104d094be057;p=thirdparty%2Fbind9.git Use cds_lfht for updatenotify mechanism in dns_db unit The updatenotify mechanism in dns_db relied on unlocked ISC_LIST for adding and removing the "listeners". The mechanism relied on the exclusive mode - it should have been updated only during reconfiguration of the server. This turned not to be true anymore in the dns_catz - the updatenotify list could have been updated during offloaded work as the offloaded threads are not subject to the exclusive mode. Change the update_listeners to be cds_lfht (lock-free hash-table), and slightly refactor how register and unregister the callbacks - the calls are now idempotent (the register call already was and the return value of the unregister function was mostly ignored by the callers). --- diff --git a/lib/dns/catz.c b/lib/dns/catz.c index f1e17ba6261..63fcc3dd0ee 100644 --- a/lib/dns/catz.c +++ b/lib/dns/catz.c @@ -99,7 +99,6 @@ struct dns_catz_zone { isc_timer_t *updatetimer; bool active; - bool db_registered; bool broken; isc_refcount_t references; @@ -1005,14 +1004,12 @@ dns__catz_zone_destroy(dns_catz_zone_t *catz) { isc_timer_async_destroy(&catz->updatetimer); } - if (catz->db_registered) { + if (catz->db != NULL) { + if (catz->dbversion != NULL) { + dns_db_closeversion(catz->db, &catz->dbversion, false); + } dns_db_updatenotify_unregister( catz->db, dns_catz_dbupdate_callback, catz->catzs); - } - if (catz->dbversion != NULL) { - dns_db_closeversion(catz->db, &catz->dbversion, false); - } - if (catz->db != NULL) { dns_db_detach(&catz->db); } @@ -2144,16 +2141,12 @@ dns_catz_dbupdate_callback(dns_db_t *db, void *fn_arg) { dns_db_updatenotify_unregister( catz->db, dns_catz_dbupdate_callback, catz->catzs); dns_db_detach(&catz->db); - catz->db_registered = false; } if (catz->db == NULL) { /* New db registration. */ dns_db_attach(db, &catz->db); - result = dns_db_updatenotify_register( - db, dns_catz_dbupdate_callback, catz->catzs); - if (result == ISC_R_SUCCESS) { - catz->db_registered = true; - } + dns_db_updatenotify_register(db, dns_catz_dbupdate_callback, + catz->catzs); } if (!catz->updatepending && !catz->updaterunning) { @@ -2186,9 +2179,7 @@ dns_catz_dbupdate_unregister(dns_db_t *db, dns_catz_zones_t *catzs) { REQUIRE(DNS_DB_VALID(db)); REQUIRE(DNS_CATZ_ZONES_VALID(catzs)); - LOCK(&catzs->lock); dns_db_updatenotify_unregister(db, dns_catz_dbupdate_callback, catzs); - UNLOCK(&catzs->lock); } void @@ -2196,9 +2187,7 @@ dns_catz_dbupdate_register(dns_db_t *db, dns_catz_zones_t *catzs) { REQUIRE(DNS_DB_VALID(db)); REQUIRE(DNS_CATZ_ZONES_VALID(catzs)); - LOCK(&catzs->lock); dns_db_updatenotify_register(db, dns_catz_dbupdate_callback, catzs); - UNLOCK(&catzs->lock); } static bool @@ -2495,15 +2484,8 @@ dns__catz_update_cb(void *data) { * update callback in zone_startload or axfr_makedb, but we will * call onupdate() artificially so we can register the callback here. */ - LOCK(&catzs->lock); - if (!oldcatz->db_registered) { - result = dns_db_updatenotify_register( - updb, dns_catz_dbupdate_callback, oldcatz->catzs); - if (result == ISC_R_SUCCESS) { - oldcatz->db_registered = true; - } - } - UNLOCK(&catzs->lock); + dns_db_updatenotify_register(updb, dns_catz_dbupdate_callback, + oldcatz->catzs); exit: catz->updateresult = result; diff --git a/lib/dns/db.c b/lib/dns/db.c index d8b9995040c..428bc0d749f 100644 --- a/lib/dns/db.c +++ b/lib/dns/db.c @@ -21,12 +21,14 @@ #include #include +#include #include #include #include #include #include #include +#include #include #include @@ -97,6 +99,9 @@ impfind(const char *name) { return (NULL); } +static void +call_updatenotify(dns_db_t *db); + /*** *** Basic DB Methods ***/ @@ -265,8 +270,6 @@ dns_db_beginload(dns_db_t *db, dns_rdatacallbacks_t *callbacks) { isc_result_t dns_db_endload(dns_db_t *db, dns_rdatacallbacks_t *callbacks) { - dns_dbonupdatelistener_t *listener; - /* * Finish loading 'db'. */ @@ -280,11 +283,7 @@ dns_db_endload(dns_db_t *db, dns_rdatacallbacks_t *callbacks) { * for all registered listeners, regardless of whether the underlying * database has an 'endload' implementation. */ - for (listener = ISC_LIST_HEAD(db->update_listeners); listener != NULL; - listener = ISC_LIST_NEXT(listener, link)) - { - listener->onupdate(db, listener->onupdate_arg); - } + call_updatenotify(db); if (db->methods->endload != NULL) { return ((db->methods->endload)(db, callbacks)); @@ -385,8 +384,6 @@ dns_db_attachversion(dns_db_t *db, dns_dbversion_t *source, void dns__db_closeversion(dns_db_t *db, dns_dbversion_t **versionp, bool commit DNS__DB_FLARG) { - dns_dbonupdatelistener_t *listener; - /* * Close version '*versionp'. */ @@ -398,11 +395,7 @@ dns__db_closeversion(dns_db_t *db, dns_dbversion_t **versionp, (db->methods->closeversion)(db, versionp, commit DNS__DB_FLARG_PASS); if (commit) { - for (listener = ISC_LIST_HEAD(db->update_listeners); - listener != NULL; listener = ISC_LIST_NEXT(listener, link)) - { - listener->onupdate(db, listener->onupdate_arg); - } + call_updatenotify(db); } ENSURE(*versionp == NULL); @@ -951,59 +944,97 @@ dns__db_getsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, return (ISC_R_NOTFOUND); } +static void +call_updatenotify(dns_db_t *db) { + rcu_read_lock(); + struct cds_lfht *update_listeners = + rcu_dereference(db->update_listeners); + if (update_listeners != NULL) { + struct cds_lfht_iter iter; + dns_dbonupdatelistener_t *listener; + cds_lfht_for_each_entry(update_listeners, &iter, listener, + ht_node) { + if (!cds_lfht_is_node_deleted(&listener->ht_node)) { + listener->onupdate(db, listener->onupdate_arg); + } + } + } + rcu_read_unlock(); +} + +static void +updatenotify_free(struct rcu_head *rcu_head) { + dns_dbonupdatelistener_t *listener = + caa_container_of(rcu_head, dns_dbonupdatelistener_t, rcu_head); + isc_mem_putanddetach(&listener->mctx, listener, sizeof(*listener)); +} + +static int +updatenotify_match(struct cds_lfht_node *ht_node, const void *_key) { + const dns_dbonupdatelistener_t *listener = + caa_container_of(ht_node, dns_dbonupdatelistener_t, ht_node); + const dns_dbonupdatelistener_t *key = _key; + + return (listener->onupdate == key->onupdate && + listener->onupdate_arg == key->onupdate_arg); +} + /* * Attach a notify-on-update function the database */ -isc_result_t +void dns_db_updatenotify_register(dns_db_t *db, dns_dbupdate_callback_t fn, void *fn_arg) { - dns_dbonupdatelistener_t *listener; - REQUIRE(db != NULL); REQUIRE(fn != NULL); - for (listener = ISC_LIST_HEAD(db->update_listeners); listener != NULL; - listener = ISC_LIST_NEXT(listener, link)) - { - if ((listener->onupdate == fn) && - (listener->onupdate_arg == fn_arg)) - { - return (ISC_R_SUCCESS); - } + dns_dbonupdatelistener_t key = { .onupdate = fn, + .onupdate_arg = fn_arg }; + uint32_t hash = isc_hash32(&key, sizeof(key), true); + dns_dbonupdatelistener_t *listener = isc_mem_get(db->mctx, + sizeof(*listener)); + *listener = key; + + isc_mem_attach(db->mctx, &listener->mctx); + + rcu_read_lock(); + struct cds_lfht *update_listeners = + rcu_dereference(db->update_listeners); + INSIST(update_listeners != NULL); + struct cds_lfht_node *ht_node = + cds_lfht_add_unique(update_listeners, hash, updatenotify_match, + &key, &listener->ht_node); + rcu_read_unlock(); + + if (ht_node != &listener->ht_node) { + updatenotify_free(&listener->rcu_head); } - - listener = isc_mem_get(db->mctx, sizeof(dns_dbonupdatelistener_t)); - - listener->onupdate = fn; - listener->onupdate_arg = fn_arg; - - ISC_LINK_INIT(listener, link); - ISC_LIST_APPEND(db->update_listeners, listener, link); - - return (ISC_R_SUCCESS); } -isc_result_t +void dns_db_updatenotify_unregister(dns_db_t *db, dns_dbupdate_callback_t fn, void *fn_arg) { - dns_dbonupdatelistener_t *listener; - REQUIRE(db != NULL); - for (listener = ISC_LIST_HEAD(db->update_listeners); listener != NULL; - listener = ISC_LIST_NEXT(listener, link)) - { - if ((listener->onupdate == fn) && - (listener->onupdate_arg == fn_arg)) - { - ISC_LIST_UNLINK(db->update_listeners, listener, link); - isc_mem_put(db->mctx, listener, - sizeof(dns_dbonupdatelistener_t)); - return (ISC_R_SUCCESS); - } + dns_dbonupdatelistener_t key = { .onupdate = fn, + .onupdate_arg = fn_arg }; + uint32_t hash = isc_hash32(&key, sizeof(key), true); + struct cds_lfht_iter iter; + + rcu_read_lock(); + struct cds_lfht *update_listeners = + rcu_dereference(db->update_listeners); + INSIST(update_listeners != NULL); + cds_lfht_lookup(update_listeners, hash, updatenotify_match, &key, + &iter); + + struct cds_lfht_node *ht_node = cds_lfht_iter_get_node(&iter); + if (ht_node != NULL && !cds_lfht_del(update_listeners, ht_node)) { + dns_dbonupdatelistener_t *listener = caa_container_of( + ht_node, dns_dbonupdatelistener_t, ht_node); + call_rcu(&listener->rcu_head, updatenotify_free); } - - return (ISC_R_NOTFOUND); + rcu_read_unlock(); } isc_result_t diff --git a/lib/dns/include/dns/db.h b/lib/dns/include/dns/db.h index fa1713db84b..373fa30384f 100644 --- a/lib/dns/include/dns/db.h +++ b/lib/dns/include/dns/db.h @@ -58,6 +58,7 @@ #include #include #include +#include #include #include @@ -212,7 +213,7 @@ struct dns_db { dns_ttl_t serve_stale_ttl; /* for cache DB's only */ isc_mem_t *mctx; isc_refcount_t references; - ISC_LIST(dns_dbonupdatelistener_t) update_listeners; + struct cds_lfht *update_listeners; }; enum { @@ -221,9 +222,11 @@ enum { }; struct dns_dbonupdatelistener { + isc_mem_t *mctx; dns_dbupdate_callback_t onupdate; void *onupdate_arg; - ISC_LINK(dns_dbonupdatelistener_t) link; + struct cds_lfht_node ht_node; + struct rcu_head rcu_head; }; /*@{*/ @@ -1617,7 +1620,7 @@ dns_db_setcachestats(dns_db_t *db, isc_stats_t *stats); * dns_rdatasetstats_create(); otherwise NULL. */ -isc_result_t +void dns_db_updatenotify_register(dns_db_t *db, dns_dbupdate_callback_t fn, void *fn_arg); /*%< @@ -1631,7 +1634,7 @@ dns_db_updatenotify_register(dns_db_t *db, dns_dbupdate_callback_t fn, * */ -isc_result_t +void dns_db_updatenotify_unregister(dns_db_t *db, dns_dbupdate_callback_t fn, void *fn_arg); /*%< diff --git a/lib/dns/include/dns/rpz.h b/lib/dns/include/dns/rpz.h index d28d6e14b93..af909d1b1d2 100644 --- a/lib/dns/include/dns/rpz.h +++ b/lib/dns/include/dns/rpz.h @@ -402,6 +402,10 @@ dns_rpz_new_zone(dns_rpz_zones_t *rpzs, dns_rpz_zone_t **rpzp); isc_result_t dns_rpz_dbupdate_callback(dns_db_t *db, void *fn_arg); +void +dns_rpz_dbupdate_unregister(dns_db_t *db, dns_rpz_zone_t *rpz); +void +dns_rpz_dbupdate_register(dns_db_t *db, dns_rpz_zone_t *rpz); void dns_rpz_zones_shutdown(dns_rpz_zones_t *rpzs); diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index bb54d2cb907..235b5fdcab5 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -578,7 +578,9 @@ free_rbtdb(dns_rbtdb_t *rbtdb, bool log) { rbtdb->common.impmagic = 0; isc_mem_detach(&rbtdb->hmctx); - INSIST(ISC_LIST_EMPTY(rbtdb->common.update_listeners)); + if (rbtdb->common.update_listeners != NULL) { + INSIST(!cds_lfht_destroy(rbtdb->common.update_listeners, NULL)); + } isc_mem_putanddetach(&rbtdb->common.mctx, rbtdb, sizeof(*rbtdb)); } @@ -3750,7 +3752,6 @@ dns__rbtdb_create(isc_mem_t *mctx, const dns_name_t *origin, dns_dbtype_t type, *rbtdb = (dns_rbtdb_t){ .common.origin = DNS_NAME_INITEMPTY, .common.rdclass = rdclass, - .common.update_listeners = ISC_LIST_INITIALIZER, .current_serial = 1, .least_serial = 1, .next_serial = 2, @@ -3799,6 +3800,9 @@ dns__rbtdb_create(isc_mem_t *mctx, const dns_name_t *origin, dns_dbtype_t type, rbtdb->node_locks = isc_mem_get(mctx, rbtdb->node_lock_count * sizeof(rbtdb_nodelock_t)); + rbtdb->common.update_listeners = + cds_lfht_new(16, 16, 0, CDS_LFHT_ACCOUNTING, NULL); + if (IS_CACHE(rbtdb)) { dns_rdatasetstats_create(mctx, &rbtdb->rrsetstats); rbtdb->lru = isc_mem_get(mctx, diff --git a/lib/dns/rpz.c b/lib/dns/rpz.c index fd89f1aa9d5..ee4ed0613a9 100644 --- a/lib/dns/rpz.c +++ b/lib/dns/rpz.c @@ -1624,6 +1624,21 @@ unlock: return (result); } +void +dns_rpz_dbupdate_unregister(dns_db_t *db, dns_rpz_zone_t *rpz) { + REQUIRE(DNS_DB_VALID(db)); + REQUIRE(DNS_RPZ_ZONE_VALID(rpz)); + + dns_db_updatenotify_unregister(db, dns_rpz_dbupdate_callback, rpz); +} + +void +dns_rpz_dbupdate_register(dns_db_t *db, dns_rpz_zone_t *rpz) { + REQUIRE(DNS_DB_VALID(db)); + REQUIRE(DNS_RPZ_ZONE_VALID(rpz)); + + dns_db_updatenotify_register(db, dns_rpz_dbupdate_callback, rpz); +} static void dns__rpz_timer_start(dns_rpz_zone_t *rpz) { uint64_t tdiff; diff --git a/lib/dns/zone.c b/lib/dns/zone.c index b7dfbc97bf9..6fe31061b87 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -1896,14 +1896,11 @@ dns_zone_get_rpz_num(dns_zone_t *zone) { */ void dns_zone_rpz_enable_db(dns_zone_t *zone, dns_db_t *db) { - isc_result_t result; if (zone->rpz_num == DNS_RPZ_INVALID_NUM) { return; } REQUIRE(zone->rpzs != NULL); - result = dns_db_updatenotify_register(db, dns_rpz_dbupdate_callback, - zone->rpzs->zones[zone->rpz_num]); - REQUIRE(result == ISC_R_SUCCESS); + dns_rpz_dbupdate_register(db, zone->rpzs->zones[zone->rpz_num]); } static void @@ -1912,8 +1909,7 @@ dns_zone_rpz_disable_db(dns_zone_t *zone, dns_db_t *db) { return; } REQUIRE(zone->rpzs != NULL); - (void)dns_db_updatenotify_unregister(db, dns_rpz_dbupdate_callback, - zone->rpzs->zones[zone->rpz_num]); + dns_rpz_dbupdate_unregister(db, zone->rpzs->zones[zone->rpz_num]); } /*