mir/bind
2
0
Fork 0
mirror of https://gitlab.isc.org/isc-projects/bind9 synced 2025-08-31 06:25:31 +00:00
Code Issues Releases Wiki Activity

rem: dev: Remove lock upgrading from the hot path in the QP cache

Browse Source 
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 clean up the header, and in `qpzonode_release()`, the tree lock would be optionally upgraded, so we can clean up the node directly if empty. These
optimizations are not needed and they have no effect on the performance.

Merge branch 'ondrej/no-lock-upgrade-in-check_stale_headers' into 'main'

See merge request isc-projects/bind9!10305
This commit is contained in:
Ondřej Surý
2025-03-25 09:58:09 +00:00
parent 7861042a27 e8a1949566
commit e1af47b829
2 changed files with 35 additions and 153 deletions
Download Patch File Download Diff File Expand all files Collapse all files

44
lib/dns/db_p.h
View File

@@ -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)

144
lib/dns/qpcache.c
View File

@@ -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);