From: JINMEI Tatuya Date: Sat, 18 Jan 2025 00:54:19 +0000 (-0800) Subject: Optimize database decref by avoiding locking with refs > 1 X-Git-Tag: v9.21.5~40^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=7f4471594db157b61218ee6377e6ba0887092a41;p=thirdparty%2Fbind9.git 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 Co-authored-by: Ondřej Surý --- diff --git a/lib/dns/qpcache.c b/lib/dns/qpcache.c index aadfaf2bb83..ae903f24e05 100644 --- 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 875c39920f9..cf8f55d7652 100644 --- 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); }