From: Ondřej Surý Date: Fri, 21 Mar 2025 02:06:16 +0000 (+0100) Subject: Remove lock upgrading from the hot path in the cache X-Git-Tag: v9.21.7~27^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e8a1949566736106f61f3788d304fb4c7660b7fd;p=thirdparty%2Fbind9.git Remove lock upgrading from the hot path in the cache In QPcache, there were two places that tried to upgrade the lock. In clean_stale_header(), the code would try to upgrade the lock and cleanup the header, and in qpzonode_release(), the tree lock would be optionally upgraded, so we can cleanup the node directly if empty. These optimizations are not needed and they have no effect on the performance. --- diff --git a/lib/dns/db_p.h b/lib/dns/db_p.h index 2db152ec1f9..51b6943dabe 100644 --- a/lib/dns/db_p.h +++ b/lib/dns/db_p.h @@ -43,31 +43,17 @@ } #define NODE_RDLOCK(l, tp) NODE_LOCK(l, isc_rwlocktype_read, tp); #define NODE_WRLOCK(l, tp) NODE_LOCK(l, isc_rwlocktype_write, tp); -#define NODE_TRYLOCK(l, t, tp) \ - ({ \ - STRONG_RWLOCK_CHECK(*tp == isc_rwlocktype_none); \ - isc_result_t _result = isc_rwlock_trylock(l, t); \ - if (_result == ISC_R_SUCCESS) { \ - *tp = t; \ - }; \ - _result; \ - }) -#define NODE_TRYRDLOCK(l, tp) NODE_TRYLOCK(l, isc_rwlocktype_read, tp) -#define NODE_TRYWRLOCK(l, tp) NODE_TRYLOCK(l, isc_rwlocktype_write, tp) -#define NODE_TRYUPGRADE(l, tp) \ +#define NODE_FORCEUPGRADE(l, tp) \ ({ \ STRONG_RWLOCK_CHECK(*tp == isc_rwlocktype_read); \ isc_result_t _result = isc_rwlock_tryupgrade(l); \ if (_result == ISC_R_SUCCESS) { \ *tp = isc_rwlocktype_write; \ + } else { \ + NODE_UNLOCK(l, tp); \ + NODE_WRLOCK(l, tp); \ }; \ - _result; \ }) -#define NODE_FORCEUPGRADE(l, tp) \ - if (NODE_TRYUPGRADE(l, tp) != ISC_R_SUCCESS) { \ - NODE_UNLOCK(l, tp); \ - NODE_WRLOCK(l, tp); \ - } #define TREE_INITLOCK(l) isc_rwlock_init(l) #define TREE_DESTROYLOCK(l) isc_rwlock_destroy(l) @@ -85,31 +71,17 @@ } #define TREE_RDLOCK(l, tp) TREE_LOCK(l, isc_rwlocktype_read, tp); #define TREE_WRLOCK(l, tp) TREE_LOCK(l, isc_rwlocktype_write, tp); -#define TREE_TRYLOCK(l, t, tp) \ - ({ \ - STRONG_RWLOCK_CHECK(*tp == isc_rwlocktype_none); \ - isc_result_t _result = isc_rwlock_trylock(l, t); \ - if (_result == ISC_R_SUCCESS) { \ - *tp = t; \ - }; \ - _result; \ - }) -#define TREE_TRYRDLOCK(l, tp) TREE_TRYLOCK(l, isc_rwlocktype_read, tp) -#define TREE_TRYWRLOCK(l, tp) TREE_TRYLOCK(l, isc_rwlocktype_write, tp) -#define TREE_TRYUPGRADE(l, tp) \ +#define TREE_FORCEUPGRADE(l, tp) \ ({ \ STRONG_RWLOCK_CHECK(*tp == isc_rwlocktype_read); \ isc_result_t _result = isc_rwlock_tryupgrade(l); \ if (_result == ISC_R_SUCCESS) { \ *tp = isc_rwlocktype_write; \ + } else { \ + TREE_UNLOCK(l, tp); \ + TREE_WRLOCK(l, tp); \ }; \ - _result; \ }) -#define TREE_FORCEUPGRADE(l, tp) \ - if (TREE_TRYUPGRADE(l, tp) != ISC_R_SUCCESS) { \ - TREE_UNLOCK(l, tp); \ - TREE_WRLOCK(l, tp); \ - } #define IS_STUB(db) (((db)->common.attributes & DNS_DBATTR_STUB) != 0) #define IS_CACHE(db) (((db)->common.attributes & DNS_DBATTR_CACHE) != 0) diff --git a/lib/dns/qpcache.c b/lib/dns/qpcache.c index 44339525a16..1391945c023 100644 --- a/lib/dns/qpcache.c +++ b/lib/dns/qpcache.c @@ -772,13 +772,9 @@ qpcnode_erefs_decrement(qpcache_t *qpdb, qpcnode_t *node DNS__DB_FLARG) { */ static void qpcnode_release(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t *nlocktypep, - isc_rwlocktype_t *tlocktypep, bool tryupgrade DNS__DB_FLARG) { + isc_rwlocktype_t *tlocktypep DNS__DB_FLARG) { REQUIRE(*nlocktypep != isc_rwlocktype_none); - isc_result_t result; - bool locked = *tlocktypep != isc_rwlocktype_none; - bool write_locked = false; - if (!qpcnode_erefs_decrement(qpdb, node DNS__DB_FLARG_PASS)) { goto unref; } @@ -815,49 +811,21 @@ qpcnode_release(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t *nlocktypep, clean_cache_node(qpdb, node); } - /* - * Attempt to switch to a write lock on the tree. If this fails, - * we will add this node to a linked list of nodes in this locking - * bucket which we will free later. - * - * Locking hierarchy notwithstanding, we don't need to free - * the node lock before acquiring the tree write lock because - * we only do a trylock. - */ - /* We are allowed to upgrade the tree lock */ - - switch (*tlocktypep) { - case isc_rwlocktype_write: - result = ISC_R_SUCCESS; - break; - case isc_rwlocktype_read: - if (tryupgrade) { - result = TREE_TRYUPGRADE(&qpdb->tree_lock, tlocktypep); - } else { - result = ISC_R_LOCKBUSY; - } - break; - case isc_rwlocktype_none: - result = TREE_TRYWRLOCK(&qpdb->tree_lock, tlocktypep); - break; - default: - UNREACHABLE(); - } - RUNTIME_CHECK(result == ISC_R_SUCCESS || result == ISC_R_LOCKBUSY); - if (result == ISC_R_SUCCESS) { - write_locked = true; - } - if (node->data != NULL) { - goto restore_locks; + goto unref; } - if (write_locked) { + if (*tlocktypep == isc_rwlocktype_write) { /* - * We can now delete the node. + * We can delete the node if we have the tree write lock. */ delete_node(qpdb, node); } else { + /* + * If we don't have the tree lock, we will add this node to a + * linked list of nodes in this locking bucket which we will + * free later. + */ qpcnode_acquire(qpdb, node, *nlocktypep, *tlocktypep DNS__DB_FLARG_PASS); @@ -874,14 +842,6 @@ qpcnode_release(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t *nlocktypep, } } -restore_locks: - /* - * Unlock the tree lock if it wasn't held previously. - */ - if (!locked && write_locked) { - TREE_UNLOCK(&qpdb->tree_lock, tlocktypep); - } - unref: qpcnode_unref(node); } @@ -1009,7 +969,7 @@ expireheader(dns_slabheader_t *header, isc_rwlocktype_t *nlocktypep, qpcnode_acquire(qpdb, HEADERNODE(header), *nlocktypep, *tlocktypep DNS__DB_FLARG_PASS); qpcnode_release(qpdb, HEADERNODE(header), nlocktypep, - tlocktypep, true DNS__DB_FLARG_PASS); + tlocktypep DNS__DB_FLARG_PASS); if (qpdb->cachestats == NULL) { return; @@ -1225,9 +1185,8 @@ setup_delegation(qpc_search_t *search, dns_dbnode_t **nodep, } static bool -check_stale_header(qpcnode_t *node, dns_slabheader_t *header, - isc_rwlocktype_t *nlocktypep, isc_rwlock_t *nlock, - qpc_search_t *search, dns_slabheader_t **header_prev) { +check_stale_header(dns_slabheader_t *header, qpc_search_t *search, + dns_slabheader_t **header_prev) { if (ACTIVE(header, search->now)) { *header_prev = header; return false; @@ -1280,39 +1239,7 @@ check_stale_header(qpcnode_t *node, dns_slabheader_t *header, return (search->options & DNS_DBFIND_STALEOK) == 0; } - /* - * This rdataset is stale. If no one else is using the - * node, we can clean it up right now, otherwise we mark - * it as ancient, and the node as dirty, so it will get - * cleaned up later. - */ - if ((header->expire < search->now - QPDB_VIRTUAL) && - (*nlocktypep == isc_rwlocktype_write || - NODE_TRYUPGRADE(nlock, nlocktypep) == ISC_R_SUCCESS)) - { - /* - * We update the node's status only when we can - * get write access; otherwise, we leave others - * to this work. Periodical cleaning will - * eventually take the job as the last resort. - * We won't downgrade the lock, since other - * rdatasets are probably stale, too. - */ - if (isc_refcount_current(&node->references) == 0) { - clean_stale_headers(header); - if (*header_prev != NULL) { - (*header_prev)->next = header->next; - } else { - node->data = header->next; - } - dns_slabheader_destroy(&header); - } else { - mark_ancient(header); - *header_prev = header; - } - } else { - *header_prev = header; - } + *header_prev = header; return true; } @@ -1387,9 +1314,7 @@ check_zonecut(qpcnode_t *node, void *arg DNS__DB_FLARG) { */ for (header = node->data; header != NULL; header = header_next) { header_next = header->next; - if (check_stale_header(node, header, &nlocktype, nlock, search, - &header_prev)) - { + if (check_stale_header(header, search, &header_prev)) { continue; } @@ -1456,9 +1381,7 @@ find_deepest_zonecut(qpc_search_t *search, qpcnode_t *node, for (header = node->data; header != NULL; header = header_next) { header_next = header->next; - if (check_stale_header(node, header, &nlocktype, nlock, - search, &header_prev)) - { + if (check_stale_header(header, search, &header_prev)) { continue; } @@ -1560,9 +1483,7 @@ find_coveringnsec(qpc_search_t *search, const dns_name_t *name, NODE_RDLOCK(nlock, &nlocktype); for (header = node->data; header != NULL; header = header_next) { header_next = header->next; - if (check_stale_header(node, header, &nlocktype, nlock, search, - &header_prev)) - { + if (check_stale_header(header, search, &header_prev)) { continue; } @@ -1745,9 +1666,7 @@ qpcache_find(dns_db_t *db, const dns_name_t *name, dns_dbversion_t *version, header_prev = NULL; for (header = node->data; header != NULL; header = header_next) { header_next = header->next; - if (check_stale_header(node, header, &nlocktype, nlock, &search, - &header_prev)) - { + if (check_stale_header(header, &search, &header_prev)) { continue; } @@ -1986,8 +1905,8 @@ tree_exit: nlock = &search.qpdb->buckets[node->locknum].lock; NODE_RDLOCK(nlock, &nlocktype); - qpcnode_release(search.qpdb, node, &nlocktype, &tlocktype, - true DNS__DB_FLARG_PASS); + qpcnode_release(search.qpdb, node, &nlocktype, + &tlocktype DNS__DB_FLARG_PASS); NODE_UNLOCK(nlock, &nlocktype); INSIST(tlocktype == isc_rwlocktype_none); } @@ -2013,9 +1932,7 @@ seek_ns_headers(qpc_search_t *search, qpcnode_t *node, dns_dbnode_t **nodep, header_next = header->next; bool ns = (header->type == dns_rdatatype_ns || header->type == DNS_SIGTYPE(dns_rdatatype_ns)); - if (check_stale_header(node, header, &nlocktype, nlock, search, - &header_prev)) - { + if (check_stale_header(header, search, &header_prev)) { if (ns) { /* * We found a cached NS, but was either @@ -2179,18 +2096,12 @@ qpcache_findrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, matchtype = DNS_TYPEPAIR_VALUE(type, covers); negtype = DNS_TYPEPAIR_VALUE(0, type); - if (covers == 0) { - sigmatchtype = DNS_SIGTYPE(type); - } else { - sigmatchtype = 0; - } + sigmatchtype = (covers == 0) ? DNS_SIGTYPE(type) : 0; for (header = qpnode->data; header != NULL; header = header_next) { header_next = header->next; - if (check_stale_header(qpnode, header, &nlocktype, nlock, - &search, &header_prev)) - { + if (check_stale_header(header, &search, &header_prev)) { continue; } @@ -2536,8 +2447,8 @@ cleanup_deadnodes(void *arg) { RUNTIME_CHECK(isc_queue_splice(&deadnodes, &qpdb->buckets[locknum].deadnodes)); isc_queue_for_each_entry_safe(&deadnodes, qpnode, qpnext, deadlink) { - qpcnode_release(qpdb, qpnode, &nlocktype, &tlocktype, - false DNS__DB_FILELINE); + qpcnode_release(qpdb, qpnode, &nlocktype, + &tlocktype DNS__DB_FILELINE); } NODE_UNLOCK(nlock, &nlocktype); @@ -2659,8 +2570,7 @@ qpcache_detachnode(dns_db_t *db, dns_dbnode_t **nodep DNS__DB_FLARG) { rcu_read_lock(); NODE_RDLOCK(nlock, &nlocktype); - qpcnode_release(qpdb, node, &nlocktype, &tlocktype, - true DNS__DB_FLARG_PASS); + qpcnode_release(qpdb, node, &nlocktype, &tlocktype DNS__DB_FLARG_PASS); NODE_UNLOCK(nlock, &nlocktype); rcu_read_unlock(); @@ -3695,8 +3605,8 @@ dereference_iter_node(qpc_dbit_t *qpdbiter DNS__DB_FLARG) { nlock = &qpdb->buckets[node->locknum].lock; NODE_RDLOCK(nlock, &nlocktype); - qpcnode_release(qpdb, node, &nlocktype, &qpdbiter->tree_locked, - false DNS__DB_FLARG_PASS); + qpcnode_release(qpdb, node, &nlocktype, + &qpdbiter->tree_locked DNS__DB_FLARG_PASS); NODE_UNLOCK(nlock, &nlocktype); INSIST(qpdbiter->tree_locked == tlocktype);