From 20670ee22d17e0836ee408b300890a3336810e4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ondr=CC=8Cej=20Sury=CC=81?= Date: Thu, 5 Jan 2023 22:18:40 +0100 Subject: [PATCH 1/3] Replace repetetive _TRYUPGRADE() with _FORCEUPGRADE() macros There was a repetetive pattern: if (NODE_TRYUPGRADE(&nodelock->lock, nlocktypep) != ISC_R_SUCCESS) { NODE_UNLOCK(&nodelock->lock, nlocktypep); NODE_WRLOCK(&nodelock->lock, nlocktypep); } Instead of doing that over again, introduce new NODE_FORCEUPGRADE() and TREE_FORCEUPGRADE() that does exactly this code, and simplify the aforementioned code with just: NODE_FORCEUPGRADE(&nodelock->lock, nlocktypep); --- lib/dns/rbtdb.c | 65 ++++++++++++++++++++++++++----------------------- 1 file changed, 34 insertions(+), 31 deletions(-) diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index f765518f11..a318b48247 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -150,6 +150,13 @@ typedef isc_rwlock_t nodelock_t; }; \ _result; \ }) +#define NODE_FORCEUPGRADE(l, tp) \ + { \ + if (NODE_TRYUPGRADE(l, tp) != ISC_R_SUCCESS) { \ + NODE_UNLOCK(l, tp); \ + NODE_WRLOCK(l, tp); \ + } \ + } typedef isc_rwlock_t treelock_t; @@ -189,6 +196,13 @@ typedef isc_rwlock_t treelock_t; }; \ _result; \ }) +#define TREE_FORCEUPGRADE(l, tp) \ + { \ + if (TREE_TRYUPGRADE(l, tp) != ISC_R_SUCCESS) { \ + TREE_UNLOCK(l, tp); \ + TREE_WRLOCK(l, tp); \ + } \ + } #else /* DNS_RBTDB_STRONG_RWLOCK_CHECK */ @@ -225,6 +239,13 @@ typedef isc_rwlock_t treelock_t; }; \ _result; \ }) +#define NODE_FORCEUPGRADE(l, tp) \ + { \ + if (NODE_TRYUPGRADE(l, tp) != ISC_R_SUCCESS) { \ + NODE_UNLOCK(l, tp); \ + NODE_WRLOCK(l, tp); \ + } \ + } typedef isc_rwlock_t treelock_t; @@ -266,6 +287,13 @@ typedef isc_rwlock_t treelock_t; }; \ _result; \ }) +#define TREE_FORCEUPGRADE(l, tp) \ + { \ + if (TREE_TRYUPGRADE(l, tp) != ISC_R_SUCCESS) { \ + TREE_UNLOCK(l, tp); \ + TREE_WRLOCK(l, tp); \ + } \ + } #endif @@ -2041,10 +2069,7 @@ reactivate_node(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, /* * Upgrade the lock and test if we still need to unlink. */ - if (NODE_TRYUPGRADE(nodelock, &nlocktype) != ISC_R_SUCCESS) { - NODE_UNLOCK(nodelock, &nlocktype); - NODE_WRLOCK(nodelock, &nlocktype); - } + NODE_FORCEUPGRADE(nodelock, &nlocktype); POST(nlocktype); if (ISC_LINK_LINKED(node, deadlink)) { ISC_LIST_UNLINK(rbtdb->deadnodes[node->locknum], node, @@ -2108,12 +2133,7 @@ decrement_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, /* Upgrade the lock? */ if (*nlocktypep == isc_rwlocktype_read) { - if (NODE_TRYUPGRADE(&nodelock->lock, nlocktypep) != - ISC_R_SUCCESS) - { - NODE_UNLOCK(&nodelock->lock, nlocktypep); - NODE_WRLOCK(&nodelock->lock, nlocktypep); - } + NODE_FORCEUPGRADE(&nodelock->lock, nlocktypep); } if (isc_refcount_decrement(&node->references) > 1) { @@ -2871,12 +2891,7 @@ findnodeintree(dns_rbtdb_t *rbtdb, dns_rbt_t *tree, const dns_name_t *name, /* * Try to upgrade the lock and if that fails unlock then relock. */ - if (TREE_TRYUPGRADE(&rbtdb->tree_lock, &tlocktype) != - ISC_R_SUCCESS) - { - TREE_UNLOCK(&rbtdb->tree_lock, &tlocktype); - TREE_WRLOCK(&rbtdb->tree_lock, &tlocktype); - } + TREE_FORCEUPGRADE(&rbtdb->tree_lock, &tlocktype); node = NULL; result = dns_rbt_addnode(tree, name, &node); if (result == ISC_R_SUCCESS) { @@ -4871,12 +4886,7 @@ find_deepest_zonecut(rbtdb_search_t *search, dns_rbtnode_t *node, need_headerupdate(foundsig, search->now))) { if (nlocktype != isc_rwlocktype_write) { - if (NODE_TRYUPGRADE(lock, &nlocktype) != - ISC_R_SUCCESS) - { - NODE_UNLOCK(lock, &nlocktype); - NODE_WRLOCK(lock, &nlocktype); - } + NODE_FORCEUPGRADE(lock, &nlocktype); POST(nlocktype); } if (need_headerupdate(found, search->now)) { @@ -5391,10 +5401,7 @@ node_exit: if ((update != NULL || updatesig != NULL) && nlocktype != isc_rwlocktype_write) { - if (NODE_TRYUPGRADE(lock, &nlocktype) != ISC_R_SUCCESS) { - NODE_UNLOCK(lock, &nlocktype); - NODE_WRLOCK(lock, &nlocktype); - } + NODE_FORCEUPGRADE(lock, &nlocktype); POST(nlocktype); } if (update != NULL && need_headerupdate(update, search.now)) { @@ -5574,11 +5581,7 @@ cache_findzonecut(dns_db_t *db, const dns_name_t *name, unsigned int options, (foundsig != NULL && need_headerupdate(foundsig, search.now))) { if (nlocktype != isc_rwlocktype_write) { - if (NODE_TRYUPGRADE(lock, &nlocktype) != ISC_R_SUCCESS) - { - NODE_UNLOCK(lock, &nlocktype); - NODE_WRLOCK(lock, &nlocktype); - } + NODE_FORCEUPGRADE(lock, &nlocktype); POST(nlocktype); } if (need_headerupdate(found, search.now)) { From d693c2e7a06d66dcb13255cf57dcec787099953c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ondr=CC=8Cej=20Sury=CC=81?= Date: Thu, 5 Jan 2023 22:26:23 +0100 Subject: [PATCH 2/3] Extend expire_header() to check node lock type Extend the expire_header() to accept the node lock type as one of the arguments and check whether the the node lock is always write locked + fix that bug. While doing that, it was found that expire_header() invocation in rdataset_expire() passes `false` as a type of tree lock instead of `isc_rwlocktype_none`. (Un)fortunately, both values mapped to 0, so no harm was done, but it has been fixed nevertheless. --- lib/dns/rbtdb.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index a318b48247..d1fa0a76a1 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -714,7 +714,8 @@ static void update_header(dns_rbtdb_t *rbtdb, rdatasetheader_t *header, isc_stdtime_t now); static void expire_header(dns_rbtdb_t *rbtdb, rdatasetheader_t *header, - isc_rwlocktype_t *tlocktypep, expire_t reason); + isc_rwlocktype_t *nlocktypep, isc_rwlocktype_t *tlocktypep, + expire_t reason); static void overmem_purge(dns_rbtdb_t *rbtdb, unsigned int locknum_start, isc_stdtime_t now, isc_rwlocktype_t *tlocktypep); @@ -7086,7 +7087,8 @@ addrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, header->rdh_ttl + STALE_TTL(header, rbtdb) < now - RBTDB_VIRTUAL) { - expire_header(rbtdb, header, &tlocktype, expire_ttl); + expire_header(rbtdb, header, &nlocktype, &tlocktype, + expire_ttl); } /* @@ -8802,11 +8804,13 @@ rdataset_expire(dns_rdataset_t *rdataset) { dns_rbtnode_t *rbtnode = rdataset->private2; rdatasetheader_t *header = rdataset->private3; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; + isc_rwlocktype_t tlocktype = isc_rwlocktype_none; header--; NODE_WRLOCK(&rbtdb->node_locks[rbtnode->locknum].lock, &nlocktype); - expire_header(rbtdb, header, false, expire_flush); + expire_header(rbtdb, header, &nlocktype, &tlocktype, expire_flush); NODE_UNLOCK(&rbtdb->node_locks[rbtnode->locknum].lock, &nlocktype); + INSIST(tlocktype == isc_rwlocktype_none); } static void @@ -10178,7 +10182,8 @@ overmem_purge(dns_rbtdb_t *rbtdb, unsigned int locknum_start, isc_stdtime_t now, header = isc_heap_element(rbtdb->heaps[locknum], 1); if (header && header->rdh_ttl < now - RBTDB_VIRTUAL) { - expire_header(rbtdb, header, tlocktypep, expire_ttl); + expire_header(rbtdb, header, &nlocktype, tlocktypep, + expire_ttl); purgecount--; } @@ -10195,7 +10200,8 @@ overmem_purge(dns_rbtdb_t *rbtdb, unsigned int locknum_start, isc_stdtime_t now, */ ISC_LIST_UNLINK(rbtdb->rdatasets[locknum], header, link); - expire_header(rbtdb, header, tlocktypep, expire_lru); + expire_header(rbtdb, header, &nlocktype, tlocktypep, + expire_lru); purgecount--; } @@ -10205,13 +10211,15 @@ overmem_purge(dns_rbtdb_t *rbtdb, unsigned int locknum_start, isc_stdtime_t now, static void expire_header(dns_rbtdb_t *rbtdb, rdatasetheader_t *header, - isc_rwlocktype_t *tlocktypep, expire_t reason) { + isc_rwlocktype_t *nlocktypep, isc_rwlocktype_t *tlocktypep, + expire_t reason) { set_ttl(rbtdb, header, 0); mark_header_ancient(rbtdb, header); /* * Caller must hold the node (write) lock. */ + INSIST(*nlocktypep == isc_rwlocktype_write); if (isc_refcount_current(&header->node->references) == 0) { isc_rwlocktype_t nlocktype = isc_rwlocktype_write; From 44135371df3762294964f14388288a2008b370e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ondr=CC=8Cej=20Sury=CC=81?= Date: Fri, 6 Jan 2023 08:49:11 +0100 Subject: [PATCH 3/3] Deduplicate DNS_RBTDB_STRONG_RWLOCK_CHECK macros There were couple of redundant macros on both sides of DNS_RBTDB_STRONG_RWLOCK_CHECK #ifdef block. Use a single set of macros, but disable the extra REQUIRES if the #define is not set. --- lib/dns/rbtdb.c | 174 +++++++++++------------------------------------- 1 file changed, 40 insertions(+), 134 deletions(-) diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index d1fa0a76a1..4850bc148d 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -109,30 +109,33 @@ typedef uint32_t rbtdb_rdatatype_t; #define RBTDB_LOCK(l, t) RWLOCK((l), (t)) #define RBTDB_UNLOCK(l, t) RWUNLOCK((l), (t)) -typedef isc_rwlock_t nodelock_t; - #ifdef DNS_RBTDB_STRONG_RWLOCK_CHECK +#define STRONG_RWLOCK_CHECK(cond) REQUIRE(cond) +#else +#define STRONG_RWLOCK_CHECK(cond) +#endif + +typedef isc_rwlock_t nodelock_t; #define NODE_INITLOCK(l) isc_rwlock_init((l), 0, 0) #define NODE_DESTROYLOCK(l) isc_rwlock_destroy(l) -#define NODE_LOCK(l, t, tp) \ - { \ - REQUIRE(*tp == isc_rwlocktype_none); \ - RWLOCK((l), (t)); \ - *tp = t; \ +#define NODE_LOCK(l, t, tp) \ + { \ + STRONG_RWLOCK_CHECK(*tp == isc_rwlocktype_none); \ + RWLOCK((l), (t)); \ + *tp = t; \ + } +#define NODE_UNLOCK(l, tp) \ + { \ + STRONG_RWLOCK_CHECK(*tp != isc_rwlocktype_none); \ + RWUNLOCK(l, *tp); \ + *tp = isc_rwlocktype_none; \ } #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_UNLOCK(l, tp) \ - { \ - REQUIRE(*tp != isc_rwlocktype_none); \ - RWUNLOCK(l, *tp); \ - *tp = isc_rwlocktype_none; \ - } #define NODE_TRYLOCK(l, t, tp) \ ({ \ - REQUIRE(*tp == isc_rwlocktype_none); \ + STRONG_RWLOCK_CHECK(*tp == isc_rwlocktype_none); \ isc_result_t _result = isc_rwlock_trylock(l, t); \ if (_result == ISC_R_SUCCESS) { \ *tp = t; \ @@ -143,42 +146,40 @@ typedef isc_rwlock_t nodelock_t; #define NODE_TRYWRLOCK(l, tp) NODE_TRYLOCK(l, isc_rwlocktype_write, tp) #define NODE_TRYUPGRADE(l, tp) \ ({ \ - REQUIRE(*tp == isc_rwlocktype_read); \ + STRONG_RWLOCK_CHECK(*tp == isc_rwlocktype_read); \ isc_result_t _result = isc_rwlock_tryupgrade(l); \ if (_result == ISC_R_SUCCESS) { \ *tp = isc_rwlocktype_write; \ }; \ _result; \ }) -#define NODE_FORCEUPGRADE(l, tp) \ - { \ - if (NODE_TRYUPGRADE(l, tp) != ISC_R_SUCCESS) { \ - NODE_UNLOCK(l, tp); \ - NODE_WRLOCK(l, tp); \ - } \ +#define NODE_FORCEUPGRADE(l, tp) \ + if (NODE_TRYUPGRADE(l, tp) != ISC_R_SUCCESS) { \ + NODE_UNLOCK(l, tp); \ + NODE_WRLOCK(l, tp); \ } typedef isc_rwlock_t treelock_t; #define TREE_INITLOCK(l) isc_rwlock_init(l, 0, 0) #define TREE_DESTROYLOCK(l) isc_rwlock_destroy(l) -#define TREE_LOCK(l, t, tp) \ - { \ - REQUIRE(*tp == isc_rwlocktype_none); \ - RWLOCK(l, t); \ - *tp = t; \ +#define TREE_LOCK(l, t, tp) \ + { \ + STRONG_RWLOCK_CHECK(*tp == isc_rwlocktype_none); \ + RWLOCK(l, t); \ + *tp = t; \ + } +#define TREE_UNLOCK(l, tp) \ + { \ + STRONG_RWLOCK_CHECK(*tp != isc_rwlocktype_none); \ + RWUNLOCK(l, *tp); \ + *tp = isc_rwlocktype_none; \ } #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_UNLOCK(l, tp) \ - { \ - REQUIRE(*tp != isc_rwlocktype_none); \ - RWUNLOCK(l, *tp); \ - *tp = isc_rwlocktype_none; \ - } #define TREE_TRYLOCK(l, t, tp) \ ({ \ - REQUIRE(*tp == isc_rwlocktype_none); \ + STRONG_RWLOCK_CHECK(*tp == isc_rwlocktype_none); \ isc_result_t _result = isc_rwlock_trylock(l, t); \ if (_result == ISC_R_SUCCESS) { \ *tp = t; \ @@ -189,114 +190,19 @@ typedef isc_rwlock_t treelock_t; #define TREE_TRYWRLOCK(l, tp) TREE_TRYLOCK(l, isc_rwlocktype_write, tp) #define TREE_TRYUPGRADE(l, tp) \ ({ \ - REQUIRE(*tp == isc_rwlocktype_read); \ + STRONG_RWLOCK_CHECK(*tp == isc_rwlocktype_read); \ isc_result_t _result = isc_rwlock_tryupgrade(l); \ if (_result == ISC_R_SUCCESS) { \ *tp = isc_rwlocktype_write; \ }; \ _result; \ }) -#define TREE_FORCEUPGRADE(l, tp) \ - { \ - if (TREE_TRYUPGRADE(l, tp) != ISC_R_SUCCESS) { \ - TREE_UNLOCK(l, tp); \ - TREE_WRLOCK(l, tp); \ - } \ +#define TREE_FORCEUPGRADE(l, tp) \ + if (TREE_TRYUPGRADE(l, tp) != ISC_R_SUCCESS) { \ + TREE_UNLOCK(l, tp); \ + TREE_WRLOCK(l, tp); \ } -#else /* DNS_RBTDB_STRONG_RWLOCK_CHECK */ - -#define NODE_INITLOCK(l) isc_rwlock_init((l), 0, 0) -#define NODE_DESTROYLOCK(l) isc_rwlock_destroy(l) -#define NODE_LOCK(l, t, tp) \ - { \ - RWLOCK((l), (t)); \ - *tp = t; \ - } -#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_UNLOCK(l, tp) \ - { \ - RWUNLOCK(l, *tp); \ - *tp = isc_rwlocktype_none; \ - } -#define NODE_TRYLOCK(l, t, tp) \ - ({ \ - 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) \ - ({ \ - isc_result_t _result = isc_rwlock_tryupgrade(l); \ - if (_result == ISC_R_SUCCESS) { \ - *tp = isc_rwlocktype_write; \ - }; \ - _result; \ - }) -#define NODE_FORCEUPGRADE(l, tp) \ - { \ - if (NODE_TRYUPGRADE(l, tp) != ISC_R_SUCCESS) { \ - NODE_UNLOCK(l, tp); \ - NODE_WRLOCK(l, tp); \ - } \ - } - -typedef isc_rwlock_t treelock_t; - -#define TREE_INITLOCK(l) isc_rwlock_init(l, 0, 0) -#define TREE_DESTROYLOCK(l) isc_rwlock_destroy(l) -#define TREE_LOCK(l, t, tp) \ - { \ - RWLOCK(l, t); \ - *tp = t; \ - } -#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_UNLOCK(l, tp) \ - { \ - RWUNLOCK(l, *tp); \ - *tp = isc_rwlocktype_none; \ - } -#define TREE_TRYLOCK(l, t, tp) \ - ({ \ - 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) \ - ({ \ - isc_result_t _result = isc_rwlock_tryupgrade(l); \ - if (_result == ISC_R_SUCCESS) { \ - *tp = isc_rwlocktype_write; \ - }; \ - _result; \ - }) -#define TREE_FORCEUPGRADE(l, tp) \ - { \ - if (TREE_TRYUPGRADE(l, tp) != ISC_R_SUCCESS) { \ - TREE_UNLOCK(l, tp); \ - TREE_WRLOCK(l, tp); \ - } \ - } - -#endif - /*% * Whether to rate-limit updating the LRU to avoid possible thread contention. * Updating LRU requires write locking, so we don't do it every time the