From 68eb9a7c6a0b03545c003d82f4258b9530d2f508 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Witold=20Kr=C4=99cicki?= Date: Wed, 5 Jun 2019 22:07:50 +0200 Subject: [PATCH 1/3] In dns_zonemgr_unreachable use atomics to avoid unnecessary write locking. Unreachable cache in zonemgr is realized as an static LRU list. When we 'use' an entry we need to update the last-used time, we can use atomics to do so without the necessity to upgrading read-lock to write-lock. --- lib/dns/zone.c | 106 ++++++++++++++++++++----------------------------- 1 file changed, 43 insertions(+), 63 deletions(-) diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 6681446a7a..f1ef2fc095 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -501,7 +501,7 @@ struct dns_zone { #define DNS_ZONELOADFLAG_THAW 0x00000002U /* Thaw the zone on successful load. */ -#define UNREACH_CHACHE_SIZE 10U +#define UNREACH_CACHE_SIZE 10U #define UNREACH_HOLD_TIME 600 /* 10 minutes */ #define CHECK(op) \ @@ -512,9 +512,9 @@ struct dns_zone { struct dns_unreachable { isc_sockaddr_t remote; isc_sockaddr_t local; - uint32_t expire; - uint32_t last; - uint32_t count; + atomic_uint_fast32_t expire; + atomic_uint_fast32_t last; + uint32_t count; }; struct dns_zonemgr { @@ -557,7 +557,7 @@ struct dns_zonemgr { /* Locked by urlock. */ /* LRU cache */ - struct dns_unreachable unreachable[UNREACH_CHACHE_SIZE]; + struct dns_unreachable unreachable[UNREACH_CACHE_SIZE]; }; /*% @@ -17339,30 +17339,24 @@ dns_zonemgr_unreachable(dns_zonemgr_t *zmgr, isc_sockaddr_t *remote, isc_sockaddr_t *local, isc_time_t *now) { unsigned int i; - isc_rwlocktype_t locktype; - isc_result_t result; uint32_t seconds = isc_time_seconds(now); uint32_t count = 0; REQUIRE(DNS_ZONEMGR_VALID(zmgr)); - locktype = isc_rwlocktype_read; - RWLOCK(&zmgr->urlock, locktype); - for (i = 0; i < UNREACH_CHACHE_SIZE; i++) { + RWLOCK(&zmgr->urlock, isc_rwlocktype_read); + for (i = 0; i < UNREACH_CACHE_SIZE; i++) { if (zmgr->unreachable[i].expire >= seconds && isc_sockaddr_equal(&zmgr->unreachable[i].remote, remote) && isc_sockaddr_equal(&zmgr->unreachable[i].local, local)) { - result = isc_rwlock_tryupgrade(&zmgr->urlock); - if (result == ISC_R_SUCCESS) { - locktype = isc_rwlocktype_write; - zmgr->unreachable[i].last = seconds; - count = zmgr->unreachable[i].count; - } + atomic_store_relaxed(&zmgr->unreachable[i].last, + seconds); + count = zmgr->unreachable[i].count; break; } } - RWUNLOCK(&zmgr->urlock, locktype); - return (i < UNREACH_CHACHE_SIZE && count > 1U); + RWUNLOCK(&zmgr->urlock, isc_rwlocktype_read); + return (i < UNREACH_CACHE_SIZE && count > 1U); } void @@ -17370,9 +17364,6 @@ dns_zonemgr_unreachabledel(dns_zonemgr_t *zmgr, isc_sockaddr_t *remote, isc_sockaddr_t *local) { unsigned int i; - isc_rwlocktype_t locktype; - isc_result_t result; - char master[ISC_SOCKADDR_FORMATSIZE]; char source[ISC_SOCKADDR_FORMATSIZE]; @@ -17381,27 +17372,15 @@ dns_zonemgr_unreachabledel(dns_zonemgr_t *zmgr, isc_sockaddr_t *remote, REQUIRE(DNS_ZONEMGR_VALID(zmgr)); - locktype = isc_rwlocktype_read; - RWLOCK(&zmgr->urlock, locktype); - for (i = 0; i < UNREACH_CHACHE_SIZE; i++) { + RWLOCK(&zmgr->urlock, isc_rwlocktype_read); + for (i = 0; i < UNREACH_CACHE_SIZE; i++) { if (isc_sockaddr_equal(&zmgr->unreachable[i].remote, remote) && isc_sockaddr_equal(&zmgr->unreachable[i].local, local)) { - if (zmgr->unreachable[i].expire == 0) - break; - result = isc_rwlock_tryupgrade(&zmgr->urlock); - if (result == ISC_R_SUCCESS) { - locktype = isc_rwlocktype_write; - zmgr->unreachable[i].expire = 0; - isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL, - DNS_LOGMODULE_ZONE, ISC_LOG_INFO, - "master %s (source %s) deleted " - "from unreachable cache", - master, source); - } + atomic_store_relaxed(&zmgr->unreachable[i].expire, 0); break; } } - RWUNLOCK(&zmgr->urlock, locktype); + RWUNLOCK(&zmgr->urlock, isc_rwlocktype_read); } void @@ -17410,54 +17389,55 @@ dns_zonemgr_unreachableadd(dns_zonemgr_t *zmgr, isc_sockaddr_t *remote, { uint32_t seconds = isc_time_seconds(now); uint32_t last = seconds; - unsigned int i, slot = UNREACH_CHACHE_SIZE, oldest = 0; - + unsigned int i, slot = UNREACH_CACHE_SIZE, oldest = 0; REQUIRE(DNS_ZONEMGR_VALID(zmgr)); RWLOCK(&zmgr->urlock, isc_rwlocktype_write); - for (i = 0; i < UNREACH_CHACHE_SIZE; i++) { + for (i = 0; i < UNREACH_CACHE_SIZE; i++) { /* Existing entry? */ if (isc_sockaddr_equal(&zmgr->unreachable[i].remote, remote) && isc_sockaddr_equal(&zmgr->unreachable[i].local, local)) break; /* Empty slot? */ - if (zmgr->unreachable[i].expire < seconds) + if (atomic_load_relaxed(&zmgr->unreachable[i].expire) < seconds) { slot = i; + } /* Least recently used slot? */ - if (zmgr->unreachable[i].last < last) { - last = zmgr->unreachable[i].last; + if (atomic_load_relaxed(&zmgr->unreachable[i].last) < last) { + last = atomic_load_relaxed(&zmgr->unreachable[i].last); oldest = i; } } - if (i < UNREACH_CHACHE_SIZE) { + if (i < UNREACH_CACHE_SIZE) { /* * Found a existing entry. Update the expire timer and * last usage timestamps. */ - zmgr->unreachable[i].expire = seconds + UNREACH_HOLD_TIME; - zmgr->unreachable[i].last = seconds; - if (zmgr->unreachable[i].expire < seconds) + if (atomic_load_relaxed(&zmgr->unreachable[i].expire) + < seconds) { zmgr->unreachable[i].count = 1; - else + } else { zmgr->unreachable[i].count++; - } else if (slot != UNREACH_CHACHE_SIZE) { - /* - * Found a empty slot. Add a new entry to the cache. - */ - zmgr->unreachable[slot].expire = seconds + UNREACH_HOLD_TIME; - zmgr->unreachable[slot].last = seconds; - zmgr->unreachable[slot].remote = *remote; - zmgr->unreachable[slot].local = *local; - zmgr->unreachable[slot].count = 1; + } + atomic_store_relaxed(&zmgr->unreachable[i].expire, + seconds + UNREACH_HOLD_TIME); + atomic_store_relaxed(&zmgr->unreachable[i].last, seconds); } else { /* - * Replace the least recently used entry in the cache. + * We need to write a new entry. */ - zmgr->unreachable[oldest].expire = seconds + UNREACH_HOLD_TIME; - zmgr->unreachable[oldest].last = seconds; - zmgr->unreachable[oldest].remote = *remote; - zmgr->unreachable[oldest].local = *local; - zmgr->unreachable[oldest].count = 1; + if (slot == UNREACH_CACHE_SIZE) { + /* + * We don't have an empty slot, use the oldest one. + */ + slot = oldest; + } + atomic_store_relaxed(&zmgr->unreachable[slot].expire, + seconds + UNREACH_HOLD_TIME); + atomic_store_relaxed(&zmgr->unreachable[slot].last, seconds); + zmgr->unreachable[slot].count = 1; + zmgr->unreachable[slot].remote = *remote; + zmgr->unreachable[slot].local = *local; } RWUNLOCK(&zmgr->urlock, isc_rwlocktype_write); } From 5adc0baaca29f25e28f0b4862c144c9c320d02ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Witold=20Kr=C4=99cicki?= Date: Fri, 7 Jun 2019 09:46:14 +0200 Subject: [PATCH 2/3] Add Gitlab CI job with pthread_rwlock --- .gitlab-ci.yml | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 14fe24b773..b71072a7bc 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -429,6 +429,26 @@ unit:asan:sid:amd64: dependencies: - asan:sid:amd64 +rwlock:sid:amd64: + variables: + CC: gcc + CFLAGS: "-Wall -Wextra -O2 -g -DISC_MEM_USE_INTERNAL_MALLOC=0" + EXTRA_CONFIGURE: "--with-libidn2 --enable-pthread-rwlock" + <<: *debian_sid_amd64_image + <<: *build_job + +system:rwlock:sid:amd64: + <<: *debian_sid_amd64_image + <<: *system_test_job + dependencies: + - rwlock:sid:amd64 + +unit:rwlock:sid:amd64: + <<: *debian_sid_amd64_image + <<: *unit_test_job + dependencies: + - rwlock:sid:amd64 + # Jobs for Clang builds on Debian Stretch (amd64) clang:stretch:amd64: From 3dcf121064400c885c3ae24e5adc5b2ea21c7352 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Fri, 7 Jun 2019 14:41:05 +0200 Subject: [PATCH 3/3] Put the items into unreachable LRU cache from first to last, not last to first --- lib/dns/zone.c | 60 +++++++++++++++++++++++--------------------------- 1 file changed, 28 insertions(+), 32 deletions(-) diff --git a/lib/dns/zone.c b/lib/dns/zone.c index f1ef2fc095..f4de17f0a0 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -17388,57 +17388,53 @@ dns_zonemgr_unreachableadd(dns_zonemgr_t *zmgr, isc_sockaddr_t *remote, isc_sockaddr_t *local, isc_time_t *now) { uint32_t seconds = isc_time_seconds(now); - uint32_t last = seconds; - unsigned int i, slot = UNREACH_CACHE_SIZE, oldest = 0; + uint32_t expire = 0, last = seconds; + unsigned int slot = UNREACH_CACHE_SIZE, oldest = 0; + bool update_entry = true; REQUIRE(DNS_ZONEMGR_VALID(zmgr)); RWLOCK(&zmgr->urlock, isc_rwlocktype_write); - for (i = 0; i < UNREACH_CACHE_SIZE; i++) { + for (unsigned int i = 0; i < UNREACH_CACHE_SIZE; i++) { /* Existing entry? */ if (isc_sockaddr_equal(&zmgr->unreachable[i].remote, remote) && isc_sockaddr_equal(&zmgr->unreachable[i].local, local)) + { + update_entry = false; + slot = i; + expire = atomic_load_relaxed(&zmgr->unreachable[i].expire); break; - /* Empty slot? */ + } + /* Pick first empty slot? */ if (atomic_load_relaxed(&zmgr->unreachable[i].expire) < seconds) { slot = i; + break; } - /* Least recently used slot? */ + /* The worst case, least recently used slot? */ if (atomic_load_relaxed(&zmgr->unreachable[i].last) < last) { last = atomic_load_relaxed(&zmgr->unreachable[i].last); oldest = i; } } - if (i < UNREACH_CACHE_SIZE) { - /* - * Found a existing entry. Update the expire timer and - * last usage timestamps. - */ - if (atomic_load_relaxed(&zmgr->unreachable[i].expire) - < seconds) { - zmgr->unreachable[i].count = 1; - } else { - zmgr->unreachable[i].count++; - } - atomic_store_relaxed(&zmgr->unreachable[i].expire, - seconds + UNREACH_HOLD_TIME); - atomic_store_relaxed(&zmgr->unreachable[i].last, seconds); - } else { - /* - * We need to write a new entry. - */ - if (slot == UNREACH_CACHE_SIZE) { - /* - * We don't have an empty slot, use the oldest one. - */ - slot = oldest; - } - atomic_store_relaxed(&zmgr->unreachable[slot].expire, - seconds + UNREACH_HOLD_TIME); - atomic_store_relaxed(&zmgr->unreachable[slot].last, seconds); + + /* We haven't found any existing or free slots, use the oldest */ + if (slot == UNREACH_CACHE_SIZE) { + slot = oldest; + } + + if (expire < seconds) { + /* Expired or new entry, reset count to 1 */ zmgr->unreachable[slot].count = 1; + } else { + zmgr->unreachable[slot].count++; + } + atomic_store_relaxed(&zmgr->unreachable[slot].expire, + seconds + UNREACH_HOLD_TIME); + atomic_store_relaxed(&zmgr->unreachable[slot].last, seconds); + if (update_entry) { zmgr->unreachable[slot].remote = *remote; zmgr->unreachable[slot].local = *local; } + RWUNLOCK(&zmgr->urlock, isc_rwlocktype_write); }