From cb0db600e78e88ca6e804a95382b2dd99fbbbeb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 21 Sep 2023 11:59:01 +0200 Subject: [PATCH] Replace some ADB entry locking with atomics to reduce ADB contention Use atomics on couple of ADB entry members (.srtt, .flags, .expires, and .lastage) to remove ADB entry locking from couple of hot spots. The most prominent place is copy_namehook_lists() that gets called under ADB name lock and if the namehook list is long it acquires-releases quite a few ADB entry locks. Changing those ADB entry members to atomics allowed us to new_adbaddrinfo() not require locked ADB entry and since adbentry_overquota() already used atomics and handling lame information was dropped in the previous commit, we could not make the copy_namehook_lists() lockless. The other hotspot is dns_adb_adjustsrtt() and dns_adb_agesrtt() that can now use atomics because .srtt is already atomic_uint. And the last place that could now use atomics is dns_adb_changeflags(). --- lib/dns/adb.c | 100 +++++++++++++++++++++++--------------------------- 1 file changed, 46 insertions(+), 54 deletions(-) diff --git a/lib/dns/adb.c b/lib/dns/adb.c index 226b10e3ea..26f9885ba3 100644 --- a/lib/dns/adb.c +++ b/lib/dns/adb.c @@ -220,8 +220,8 @@ struct dns_adbentry { isc_refcount_t references; dns_adbnamehooklist_t nhs; - unsigned int flags; - unsigned int srtt; + atomic_uint flags; + atomic_uint srtt; unsigned int completed; unsigned int timeouts; unsigned char plain; @@ -239,8 +239,8 @@ struct dns_adbentry { unsigned char *cookie; uint16_t cookielen; - isc_stdtime_t expires; - isc_stdtime_t lastage; + _Atomic(isc_stdtime_t) expires; + _Atomic(isc_stdtime_t) lastage; /*%< * A nonzero 'expires' field indicates that the entry should * persist until that time. This allows entries found @@ -391,7 +391,7 @@ enum { enum { ENTRY_IS_DEAD = 1 << 31, }; -#define ENTRY_DEAD(e) (((e)->flags & ENTRY_IS_DEAD) != 0) +#define ENTRY_DEAD(e) ((atomic_load(&(e)->flags) & ENTRY_IS_DEAD) != 0) /* * To the name, address classes are all that really exist. If it has a @@ -1068,6 +1068,9 @@ new_adbentry(dns_adb_t *adb, const isc_sockaddr_t *addr) { .srtt = isc_random_uniform(0x1f) + 1, .sockaddr = *addr, .link = ISC_LINK_INITIALIZER, + .quota = adb->quota, + .references = ISC_REFCOUNT_INITIALIZER(1), + .adb = dns_adb_ref(adb), .magic = DNS_ADBENTRY_MAGIC, }; @@ -1075,14 +1078,8 @@ new_adbentry(dns_adb_t *adb, const isc_sockaddr_t *addr) { fprintf(stderr, "dns_adbentry__init:%s:%s:%d:%p->references = 1\n", __func__, __FILE__, __LINE__ + 1, entry); #endif - isc_refcount_init(&entry->references, 1); isc_mutex_init(&entry->lock); - atomic_init(&entry->active, 0); - atomic_init(&entry->quota, adb->quota); - - dns_adb_attach(adb, &entry->adb); - inc_adbstats(adb, dns_adbstats_entriescnt); return (entry); @@ -1210,8 +1207,8 @@ new_adbaddrinfo(dns_adb_t *adb, dns_adbentry_t *entry, in_port_t port) { ai = isc_mem_get(adb->mctx, sizeof(*ai)); *ai = (dns_adbaddrinfo_t){ - .srtt = entry->srtt, - .flags = entry->flags, + .srtt = atomic_load(&entry->srtt), + .flags = atomic_load(&entry->flags), .publink = ISC_LINK_INITIALIZER, .sockaddr = entry->sockaddr, .entry = dns_adbentry_ref(entry), @@ -1492,7 +1489,6 @@ copy_namehook_lists(dns_adb_t *adb, dns_adbfind_t *find, dns_adbname_t *name) { while (namehook != NULL) { dns_adbaddrinfo_t *addrinfo = NULL; entry = namehook->entry; - LOCK(&entry->lock); if (adbentry_overquota(entry)) { find->options |= DNS_ADBFIND_OVERQUOTA; @@ -1506,7 +1502,6 @@ copy_namehook_lists(dns_adb_t *adb, dns_adbfind_t *find, dns_adbname_t *name) { */ ISC_LIST_APPEND(find->list, addrinfo, publink); nextv4: - UNLOCK(&entry->lock); namehook = ISC_LIST_NEXT(namehook, name_link); } } @@ -1516,7 +1511,6 @@ copy_namehook_lists(dns_adb_t *adb, dns_adbfind_t *find, dns_adbname_t *name) { while (namehook != NULL) { dns_adbaddrinfo_t *addrinfo = NULL; entry = namehook->entry; - LOCK(&entry->lock); if (adbentry_overquota(entry)) { find->options |= DNS_ADBFIND_OVERQUOTA; @@ -1530,7 +1524,6 @@ copy_namehook_lists(dns_adb_t *adb, dns_adbfind_t *find, dns_adbname_t *name) { */ ISC_LIST_APPEND(find->list, addrinfo, publink); nextv6: - UNLOCK(&entry->lock); namehook = ISC_LIST_NEXT(namehook, name_link); } } @@ -1572,7 +1565,7 @@ expire_entry(dns_adbentry_t *adbentry) { dns_adb_t *adb = adbentry->adb; if (!ENTRY_DEAD(adbentry)) { - adbentry->flags |= ENTRY_IS_DEAD; + (void)atomic_fetch_or(&adbentry->flags, ENTRY_IS_DEAD); result = isc_hashmap_delete( adb->entries, @@ -1591,7 +1584,9 @@ entry_expired(dns_adbentry_t *adbentry, isc_stdtime_t now) { return (false); } - if (adbentry->expires == 0 || adbentry->expires > now) { + if (atomic_load(&adbentry->expires) == 0 || + atomic_load(&adbentry->expires) > now) + { return (false); } @@ -2452,8 +2447,8 @@ dump_entry(FILE *f, dns_adb_t *adb, dns_adbentry_t *entry, bool debug, fprintf(f, ";\t%s [srtt %u] [flags %08x] [edns %u/%u] " "[plain %u/%u]", - addrbuf, entry->srtt, entry->flags, entry->edns, entry->ednsto, - entry->plain, entry->plainto); + addrbuf, atomic_load(&entry->srtt), atomic_load(&entry->flags), + entry->edns, entry->ednsto, entry->plain, entry->plainto); if (entry->udpsize != 0U) { fprintf(f, " [udpsize %u]", entry->udpsize); } @@ -2465,8 +2460,9 @@ dump_entry(FILE *f, dns_adb_t *adb, dns_adbentry_t *entry, bool debug, } fprintf(f, "]"); } - if (entry->expires != 0) { - fprintf(f, " [ttl %d]", (int)(entry->expires - now)); + if (atomic_load(&entry->expires) != 0) { + fprintf(f, " [ttl %d]", + (int)(atomic_load(&entry->expires) - now)); } if (adb != NULL && adb->quota != 0 && adb->atr_freq != 0) { @@ -3031,16 +3027,14 @@ dns_adb_adjustsrtt(dns_adb_t *adb, dns_adbaddrinfo_t *addr, unsigned int rtt, REQUIRE(DNS_ADBADDRINFO_VALID(addr)); REQUIRE(factor <= 10); - isc_stdtime_t now = 0; dns_adbentry_t *entry = addr->entry; - LOCK(&entry->lock); - if (entry->expires == 0 || factor == DNS_ADB_RTTADJAGE) { + isc_stdtime_t now = 0; + if (atomic_load(&entry->expires) == 0 || factor == DNS_ADB_RTTADJAGE) { now = isc_stdtime_now(); } - adjustsrtt(addr, rtt, factor, now); - UNLOCK(&entry->lock); + adjustsrtt(addr, rtt, factor, now); } void @@ -3048,40 +3042,36 @@ dns_adb_agesrtt(dns_adb_t *adb, dns_adbaddrinfo_t *addr, isc_stdtime_t now) { REQUIRE(DNS_ADB_VALID(adb)); REQUIRE(DNS_ADBADDRINFO_VALID(addr)); - dns_adbentry_t *entry = addr->entry; - LOCK(&entry->lock); - adjustsrtt(addr, 0, DNS_ADB_RTTADJAGE, now); - - UNLOCK(&entry->lock); } static void adjustsrtt(dns_adbaddrinfo_t *addr, unsigned int rtt, unsigned int factor, isc_stdtime_t now) { - uint64_t new_srtt; + unsigned int new_srtt; if (factor == DNS_ADB_RTTADJAGE) { - if (addr->entry->lastage != now) { + if (atomic_load(&addr->entry->lastage) != now) { new_srtt = addr->entry->srtt; new_srtt <<= 9; new_srtt -= addr->entry->srtt; new_srtt >>= 9; - addr->entry->lastage = now; + atomic_store(&addr->entry->lastage, now); } else { - new_srtt = addr->entry->srtt; + new_srtt = atomic_load(&addr->entry->srtt) } } else { - new_srtt = ((uint64_t)addr->entry->srtt / 10 * factor) + + new_srtt = ((uint64_t)atomic_load(&addr->entry->srtt) / 10 * + factor) + ((uint64_t)rtt / 10 * (10 - factor)); } - addr->entry->srtt = (unsigned int)new_srtt; - addr->srtt = (unsigned int)new_srtt; + atomic_store(&addr->entry->srtt, new_srtt); + addr->srtt = new_srtt; - if (addr->entry->expires == 0) { - addr->entry->expires = now + ADB_ENTRY_WINDOW; - } + (void)atomic_compare_exchange_strong(&addr->entry->expires, + &(isc_stdtime_t){ 0 }, + now + ADB_ENTRY_WINDOW); } void @@ -3093,12 +3083,16 @@ dns_adb_changeflags(dns_adb_t *adb, dns_adbaddrinfo_t *addr, unsigned int bits, isc_stdtime_t now; dns_adbentry_t *entry = addr->entry; - LOCK(&entry->lock); + unsigned int flags = atomic_load(&entry->flags); + while (!atomic_compare_exchange_strong(&entry->flags, &flags, + (flags & ~mask) | (bits & mask))) + { + /* repeat */ + } - entry->flags = (entry->flags & ~mask) | (bits & mask); - if (entry->expires == 0) { + if (atomic_load(&entry->expires) == 0) { now = isc_stdtime_now(); - entry->expires = now + ADB_ENTRY_WINDOW; + atomic_store(&entry->expires, now + ADB_ENTRY_WINDOW); } /* @@ -3106,8 +3100,6 @@ dns_adb_changeflags(dns_adb_t *adb, dns_adbaddrinfo_t *addr, unsigned int bits, * the most recent values from addr->entry->flags. */ addr->flags = (addr->flags & ~mask) | (bits & mask); - - UNLOCK(&entry->lock); } /* @@ -3365,11 +3357,12 @@ dns_adb_findaddrinfo(dns_adb_t *adb, const isc_sockaddr_t *sa, entry = get_attached_and_locked_entry(adb, now, sa); INSIST(entry != NULL); + UNLOCK(&entry->lock); + port = isc_sockaddr_getport(sa); addr = new_adbaddrinfo(adb, entry, port); *addrp = addr; - UNLOCK(&entry->lock); dns_adbentry_detach(&entry); return (result); @@ -3393,10 +3386,9 @@ dns_adb_freeaddrinfo(dns_adb_t *adb, dns_adbaddrinfo_t **addrp) { REQUIRE(DNS_ADBENTRY_VALID(entry)); - if (entry->expires == 0) { - now = isc_stdtime_now(); - entry->expires = now + ADB_ENTRY_WINDOW; - } + now = isc_stdtime_now(); + (void)atomic_compare_exchange_strong( + &entry->expires, &(isc_stdtime_t){ 0 }, now + ADB_ENTRY_WINDOW); free_adbaddrinfo(adb, &addr); }