From 756555dbcf8b813942420f8ec059c2df9e543308 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 20 Feb 2024 08:50:58 +0100 Subject: [PATCH 1/3] Remove expired rdataset headers from the heap It was discovered that an expired header could sit on top of the heap a little longer than desireable. Remove expired headers (headers with rdh_ttl set to 0) from the heap completely, so they don't block the next TTL-based cleaning. (cherry picked from commit a9383e4b95256a65f9f05e64a79b086a9a1ed035) --- lib/dns/rbtdb.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index 00bf3cef99..c992768923 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -907,6 +907,10 @@ set_ttl(dns_rbtdb_t *rbtdb, rdatasetheader_t *header, dns_ttl_t newttl) { } else { isc_heap_decreased(heap, header->heap_index); } + + if (newttl == 0) { + isc_heap_delete(heap, header->heap_index); + } } static bool From b4d9f1cbab554da071ffb02b52b07e44d09afa6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 20 Feb 2024 08:50:58 +0100 Subject: [PATCH 2/3] Make the TTL-based cleaning more aggressive It was discovered that the TTL-based cleaning could build up a significant backlog of the rdataset headers during the periods where the top of the TTL heap isn't expired yet. Make the TTL-based cleaning more aggressive by cleaning more headers from the heap when we are adding new header into the RBTDB. (cherry picked from commit d8220ca4ca45e0aadf1ad938ed6264c8f95c7e55) --- lib/dns/rbtdb.c | 65 +++++++++++++++++++++++++++++++++++++------------ 1 file changed, 50 insertions(+), 15 deletions(-) diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index c992768923..b622c20121 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -355,6 +355,14 @@ hash_32(uint32_t val, unsigned int bits) { #define DEFAULT_CACHE_NODE_LOCK_COUNT 17 #endif /* DNS_RBTDB_CACHE_NODE_LOCK_COUNT */ +/* + * This defines the number of headers that we try to expire each time the + * expire_ttl_headers() is run. The number should be small enough, so the + * TTL-based header expiration doesn't take too long, but it should be large + * enough, so we expire enough headers if their TTL is clustered. + */ +#define DNS_RBTDB_EXPIRE_TTL_COUNT 10 + typedef struct { nodelock_t lock; /* Protected in the refcount routines. */ @@ -6867,6 +6875,10 @@ rdataset_size(rdatasetheader_t *header) { return (sizeof(*header)); } +static void +expire_ttl_headers(dns_rbtdb_t *rbtdb, unsigned int locknum, bool tree_locked, + isc_stdtime_t now); + static isc_result_t addrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, isc_stdtime_t now, dns_rdataset_t *rdataset, unsigned int options, @@ -6876,7 +6888,6 @@ addrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, rbtdb_version_t *rbtversion = version; isc_region_t region; rdatasetheader_t *newheader; - rdatasetheader_t *header; isc_result_t result; bool delegating; bool newnsec; @@ -7049,20 +7060,7 @@ addrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, cleanup_dead_nodes(rbtdb, rbtnode->locknum); } - header = isc_heap_element(rbtdb->heaps[rbtnode->locknum], 1); - if (header != NULL) { - dns_ttl_t rdh_ttl = header->rdh_ttl; - - /* Only account for stale TTL if cache is not overmem */ - if (!cache_is_overmem) { - rdh_ttl += STALE_TTL(header, rbtdb); - } - - if (rdh_ttl < now - RBTDB_VIRTUAL) { - expire_header(rbtdb, header, tree_locked, - expire_ttl); - } - } + expire_ttl_headers(rbtdb, rbtnode->locknum, tree_locked, now); /* * If we've been holding a write lock on the tree just for @@ -10435,3 +10433,40 @@ expire_header(dns_rbtdb_t *rbtdb, rdatasetheader_t *header, bool tree_locked, } } } + +/* + * Caller must be holding the node write lock. + */ +static void +expire_ttl_headers(dns_rbtdb_t *rbtdb, unsigned int locknum, bool tree_locked, + isc_stdtime_t now) { + isc_heap_t *heap = rbtdb->heaps[locknum]; + + for (size_t i = 0; i < DNS_RBTDB_EXPIRE_TTL_COUNT; i++) { + rdatasetheader_t *header = isc_heap_element(heap, 1); + + if (header == NULL) { + /* No headers left on this TTL heap; exit cleaning */ + return; + } + + dns_ttl_t ttl = header->rdh_ttl; + + if (!isc_mem_isovermem(rbtdb->common.mctx)) { + /* Only account for stale TTL if cache is not overmem */ + ttl += STALE_TTL(header, rbtdb); + } + + if (ttl >= now - RBTDB_VIRTUAL) { + /* + * The header at the top of this TTL heap is not yet + * eligible for expiry, so none of the other headers on + * the same heap can be eligible for expiry, either; + * exit cleaning. + */ + return; + } + + expire_header(rbtdb, header, tree_locked, expire_ttl); + } +} From 9584c4338e26024d0900b4bf9931b920fd64bde4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 20 Feb 2024 13:27:05 +0100 Subject: [PATCH 3/3] Add CHANGES note for [GL #4591] (cherry picked from commit db69cc7891d966dfc767cb2da469771fbe4d0997) --- CHANGES | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGES b/CHANGES index d0dfc8d5b8..8c0f393a56 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,8 @@ +6353. [bug] Improve the TTL-based cleaning by removing the expired + headers from the heap, so they don't block the next + cleaning round and clean more than a single item for + each new addition to the RBTDB. [GL #4591] + 6352. [bug] Revert change 6319 and decrease lock contention during RBTDB tree pruning by not cleaning up nodes recursively within a single prune_tree() call. [GL #4596]