Optimize database decref by avoiding locking with refs > 1

Previously, this function always acquires a node write lock if it
might need node cleanup in case the reference decrements to 0.  In
fact, the lock is unnecessary if the reference is larger than 1 and it
can be optimized as an "easy" case. This optimization could even be
"necessary". In some extreme cases, many worker threads could repeat
acquring and releasing the reference on the same node, resulting in
severe lock contention for nothing (as the ref wouldn't decrement to 0
in most cases). This change would prevent noticeable performance
drop like query timeout for such cases.

Co-authored-by: JINMEI Tatuya <jtatuya@infoblox.com>
Co-authored-by: Ondřej Surý <ondrej@isc.org>

diff --git a/lib/dns/qpcache.c b/lib/dns/qpcache.c
index aadfaf2bb832ba616c607c276a11ce3e9aefab72..ae903f24e054cb7d67cbd5087f40ce9159c05941 100644 (file)
--- a/lib/dns/qpcache.c
+++ b/lib/dns/qpcache.c
@@ -616,12 +616,9 @@ delete_node(qpcache_t *qpdb, qpcnode_t *node) {
  * then the caller must be holding at least one lock.
  */
 static void
-newref(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t nlocktype,
-       isc_rwlocktype_t tlocktype DNS__DB_FLARG) {
-       uint_fast32_t refs;
-
-       qpcnode_ref(node);
-       refs = isc_refcount_increment0(&node->erefs);
+qpcnode_newref(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t nlocktype,
+              isc_rwlocktype_t tlocktype DNS__DB_FLARG) {
+       uint_fast32_t refs = isc_refcount_increment0(&node->erefs);
 
 #if DNS_DB_NODETRACE
        fprintf(stderr, "incr:node:%s:%s:%u:%p->erefs = %" PRIuFAST32 "\n",
@@ -654,6 +651,13 @@ newref(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t nlocktype,
        }
 }
 
+static void
+newref(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t nlocktype,
+       isc_rwlocktype_t tlocktype DNS__DB_FLARG) {
+       qpcnode_ref(node);
+       qpcnode_newref(qpdb, node, nlocktype, tlocktype DNS__DB_FLARG_PASS);
+}
+
 static void
 cleanup_deadnodes(void *arg);
 
@@ -679,45 +683,45 @@ decref(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t *nlocktypep,
        isc_result_t result;
        bool locked = *tlocktypep != isc_rwlocktype_none;
        bool write_locked = false;
-       db_nodelock_t *nodelock = NULL;
        int bucket = node->locknum;
+       db_nodelock_t *nodelock = &qpdb->node_locks[bucket];
        uint_fast32_t refs;
 
        REQUIRE(*nlocktypep != isc_rwlocktype_none);
 
-       nodelock = &qpdb->node_locks[bucket];
-
-#define KEEP_NODE(n, r) ((n)->data != NULL || (n) == (r)->origin_node)
-
-       /* Handle easy and typical case first. */
-       if (!node->dirty && KEEP_NODE(node, qpdb)) {
-               bool no_reference = false;
-
-               refs = isc_refcount_decrement(&node->erefs);
+       refs = isc_refcount_decrement(&node->erefs);
 #if DNS_DB_NODETRACE
-               fprintf(stderr,
-                       "decr:node:%s:%s:%u:%p->erefs = %" PRIuFAST32 "\n",
-                       func, file, line, node, refs - 1);
-#else
-               UNUSED(refs);
+       fprintf(stderr, "decr:node:%s:%s:%u:%p->erefs = %" PRIuFAST32 "\n",
+               func, file, line, node, refs - 1);
 #endif
-               if (refs == 1) {
-                       refs = isc_refcount_decrement(&nodelock->references);
+       if (refs > 1) {
+               qpcnode_unref(node);
+               return false;
+       }
+
+       refs = isc_refcount_decrement(&nodelock->references);
 #if DNS_DB_NODETRACE
-                       fprintf(stderr,
-                               "decr:nodelock:%s:%s:%u:%p:%p->references = "
-                               "%" PRIuFAST32 "\n",
-                               func, file, line, node, nodelock, refs - 1);
+       fprintf(stderr,
+               "decr:nodelock:%s:%s:%u:%p:%p->references = "
+               "%" PRIuFAST32 "\n",
+               func, file, line, node, nodelock, refs - 1);
 #else
-                       UNUSED(refs);
+       UNUSED(refs);
 #endif
-                       no_reference = true;
-               }
 
+       /* Handle easy and typical case first. */
+       if (!node->dirty && (node->data != NULL || node == qpdb->origin_node)) {
                qpcnode_unref(node);
-               return no_reference;
+               return false;
        }
 
+       /*
+        * Node lock ref has decremented to 0 and we may need to clean up the
+        * node. To clean it up, the node ref needs to decrement to 0 under the
+        * node write lock, so we regain the ref and try again.
+        */
+       qpcnode_newref(qpdb, node, *nlocktypep, *tlocktypep DNS__DB_FLARG_PASS);
+
        /* Upgrade the lock? */
        if (*nlocktypep == isc_rwlocktype_read) {
                NODE_FORCEUPGRADE(&nodelock->lock, nlocktypep);
@@ -734,8 +738,6 @@ decref(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t *nlocktypep,
                return false;
        }
 
-       INSIST(refs == 1);
-
        if (node->dirty) {
                clean_cache_node(qpdb, node);
        }
@@ -782,7 +784,7 @@ decref(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t *nlocktypep,
        UNUSED(refs);
 #endif
 
-       if (KEEP_NODE(node, qpdb)) {
+       if (node->data != NULL || node == qpdb->origin_node) {
                goto restore_locks;
        }
 

diff --git a/lib/dns/qpzone.c b/lib/dns/qpzone.c
index 875c39920f9fc69749e199bdf4f03fde4b011da5..cf8f55d7652dfe8d0d33e563024e996d918b5d68 100644 (file)
--- a/lib/dns/qpzone.c
+++ b/lib/dns/qpzone.c
@@ -727,16 +727,11 @@ dns__qpzone_create(isc_mem_t *mctx, const dns_name_t *origin, dns_dbtype_t type,
 }
 
 static void
-newref(qpzonedb_t *qpdb, qpznode_t *node DNS__DB_FLARG) {
-       uint_fast32_t refs;
-
-       qpznode_ref(node);
-       refs = isc_refcount_increment0(&node->erefs);
+qpznode_newref(qpzonedb_t *qpdb, qpznode_t *node DNS__DB_FLARG) {
+       uint_fast32_t refs = isc_refcount_increment0(&node->erefs);
 #if DNS_DB_NODETRACE
        fprintf(stderr, "incr:node:%s:%s:%u:%p->erefs = %" PRIuFAST32 "\n",
                func, file, line, node, refs + 1);
-#else
-       UNUSED(refs);
 #endif
 
        if (refs == 0) {
@@ -755,6 +750,12 @@ newref(qpzonedb_t *qpdb, qpznode_t *node DNS__DB_FLARG) {
        }
 }
 
+static void
+newref(qpzonedb_t *qpdb, qpznode_t *node DNS__DB_FLARG) {
+       qpznode_ref(node);
+       qpznode_newref(qpdb, node DNS__DB_FLARG_PASS);
+}
+
 static void
 clean_zone_node(qpznode_t *node, uint32_t least_serial) {
        dns_slabheader_t *current = NULL, *dcurrent = NULL;
@@ -890,40 +891,47 @@ clean_zone_node(qpznode_t *node, uint32_t least_serial) {
 static void
 decref(qpzonedb_t *qpdb, qpznode_t *node, uint32_t least_serial,
        isc_rwlocktype_t *nlocktypep DNS__DB_FLARG) {
-       db_nodelock_t *nodelock = NULL;
        int bucket = node->locknum;
+       db_nodelock_t *nodelock = &qpdb->node_locks[bucket];
        uint_fast32_t refs;
 
        REQUIRE(*nlocktypep != isc_rwlocktype_none);
 
-       nodelock = &qpdb->node_locks[bucket];
-
-       /* Handle easy and typical case first. */
-       if (!node->dirty && (node->data != NULL || node == qpdb->origin ||
-                            node == qpdb->nsec3_origin))
-       {
-               refs = isc_refcount_decrement(&node->erefs);
+       refs = isc_refcount_decrement(&node->erefs);
 #if DNS_DB_NODETRACE
-               fprintf(stderr,
-                       "decr:node:%s:%s:%u:%p->erefs = %" PRIuFAST32 "\n",
-                       func, file, line, node, refs - 1);
-#else
-               UNUSED(refs);
+       fprintf(stderr, "decr:node:%s:%s:%u:%p->erefs = %" PRIuFAST32 "\n",
+               func, file, line, node, refs - 1);
 #endif
-               if (refs == 1) {
-                       refs = isc_refcount_decrement(&nodelock->references);
+       if (refs > 1) {
+               qpznode_unref(node);
+               return;
+       }
+
+       refs = isc_refcount_decrement(&nodelock->references);
 #if DNS_DB_NODETRACE
-                       fprintf(stderr,
-                               "decr:nodelock:%s:%s:%u:%p:%p->references = "
-                               "%" PRIuFAST32 "\n",
-                               func, file, line, node, nodelock, refs - 1);
+       fprintf(stderr,
+               "decr:nodelock:%s:%s:%u:%p:%p->references = "
+               "%" PRIuFAST32 "\n",
+               func, file, line, node, nodelock, refs - 1);
 #else
-                       UNUSED(refs);
+       UNUSED(refs);
 #endif
-               }
-               goto done;
+
+       /* Handle easy and typical case first. */
+       if (!node->dirty && (node->data != NULL || node == qpdb->origin ||
+                            node == qpdb->nsec3_origin))
+       {
+               qpznode_unref(node);
+               return;
        }
 
+       /*
+        * Node lock ref has decremented to 0 and we may need to clean up the
+        * node. To clean it up, the node ref needs to decrement to 0 under the
+        * node write lock, so we regain the ref and try again.
+        */
+       qpznode_newref(qpdb, node DNS__DB_FLARG_PASS);
+
        /* Upgrade the lock? */
        if (*nlocktypep == isc_rwlocktype_read) {
                NODE_FORCEUPGRADE(&nodelock->lock, nlocktypep);
@@ -934,32 +942,34 @@ decref(qpzonedb_t *qpdb, qpznode_t *node, uint32_t least_serial,
        fprintf(stderr, "decr:node:%s:%s:%u:%p->erefs = %" PRIuFAST32 "\n",
                func, file, line, node, refs - 1);
 #endif
-       if (refs == 1) {
-               if (node->dirty) {
-                       if (least_serial == 0) {
-                               /*
-                                * Caller doesn't know the least serial.
-                                * Get it.
-                                */
-                               RWLOCK(&qpdb->lock, isc_rwlocktype_read);
-                               least_serial = qpdb->least_serial;
-                               RWUNLOCK(&qpdb->lock, isc_rwlocktype_read);
-                       }
-                       clean_zone_node(node, least_serial);
+       if (refs > 1) {
+               qpznode_unref(node);
+               return;
+       }
+
+       if (node->dirty) {
+               if (least_serial == 0) {
+                       /*
+                        * Caller doesn't know the least serial.
+                        * Get it.
+                        */
+                       RWLOCK(&qpdb->lock, isc_rwlocktype_read);
+                       least_serial = qpdb->least_serial;
+                       RWUNLOCK(&qpdb->lock, isc_rwlocktype_read);
                }
+               clean_zone_node(node, least_serial);
+       }
 
-               refs = isc_refcount_decrement(&nodelock->references);
+       refs = isc_refcount_decrement(&nodelock->references);
 #if DNS_DB_NODETRACE
-               fprintf(stderr,
-                       "decr:nodelock:%s:%s:%u:%p:%p->references = "
-                       "%" PRIuFAST32 "\n",
-                       func, file, line, node, nodelock, refs - 1);
+       fprintf(stderr,
+               "decr:nodelock:%s:%s:%u:%p:%p->references = "
+               "%" PRIuFAST32 "\n",
+               func, file, line, node, nodelock, refs - 1);
 #else
-               UNUSED(refs);
+       UNUSED(refs);
 #endif
-       }
 
-done:
        qpznode_unref(node);
 }