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 f1e17ba6261b67f434cf8a2935e87a61dc407c58..63fcc3dd0ee152387b22a0d59253920a4b506a20 100644 (file)
--- 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 d8b9995040cca782d385187a8cd3832fb6658e24..428bc0d749f0112fe838e1535c70efe34f6056ce 100644 (file)
--- a/lib/dns/db.c
+++ b/lib/dns/db.c
@@ -21,12 +21,14 @@
 #include <stdbool.h>
 
 #include <isc/buffer.h>
+#include <isc/hash.h>
 #include <isc/mem.h>
 #include <isc/once.h>
 #include <isc/result.h>
 #include <isc/rwlock.h>
 #include <isc/string.h>
 #include <isc/tid.h>
+#include <isc/urcu.h>
 #include <isc/util.h>
 
 #include <dns/callbacks.h>
@@ -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 fa1713db84bde0a13c498c6d203f35d2bc6e85e0..373fa30384f24aaf29060c654e363e993e44fd95 100644 (file)
--- a/lib/dns/include/dns/db.h
+++ b/lib/dns/include/dns/db.h
@@ -58,6 +58,7 @@
 #include <isc/rwlock.h>
 #include <isc/stats.h>
 #include <isc/stdtime.h>
+#include <isc/urcu.h>
 
 #include <dns/clientinfo.h>
 #include <dns/fixedname.h>
@@ -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 d28d6e14b9312738a9ca4b19ada0a568c575b288..af909d1b1d2e2926ad4a2513dc14d37dd272af86 100644 (file)
--- 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 bb54d2cb907fb897ef274a27250c93712d655fbb..235b5fdcab5c5d65c2d13599e69eb6627dd17902 100644 (file)
--- 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 fd89f1aa9d5defd81991c522f609b8fb7c609d29..ee4ed0613a9fa75b2cafa3efbe97f577fe18cb4e 100644 (file)
--- 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 b7dfbc97bf98a27b75032eb82677e5562184154f..6fe31061b874401b07482a5d5bbf81858c5a7527 100644 (file)
--- 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]);
 }
 
 /*