From 70439e2494ba8625382753bae5aff0b1a778b3a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 9 Feb 2023 12:27:40 +0100 Subject: [PATCH] Add magic to fctxcount and replace the atomics with integers Add magic value to the fctxcount, to check for completely invalid counters, or counters that have been already destroyed. Improve the locking around the counters, and because of that we can drop the atomics and use simple integers - the counters were already locked and the tiny bits that used the atomics were not worth the extra effort. --- lib/dns/resolver.c | 98 ++++++++++++++++++++++------------------------ 1 file changed, 46 insertions(+), 52 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 32cb137f7a..cf1ac6e069 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -319,14 +319,18 @@ struct fctxkey { }; } __attribute__((__packed__)); +#define FCTXCOUNT_MAGIC ISC_MAGIC('F', 'C', 'n', 't') +#define VALID_FCTXCOUNT(counter) ISC_MAGIC_VALID(counter, FCTXCOUNT_MAGIC) + typedef struct fctxcount fctxcount_t; struct fctxcount { + unsigned int magic; dns_fixedname_t dfname; dns_name_t *domain; - atomic_uint_fast32_t count; - atomic_uint_fast32_t allowed; - atomic_uint_fast32_t dropped; - atomic_uint_fast32_t logged; + uint_fast32_t count; + uint_fast32_t allowed; + uint_fast32_t dropped; + isc_stdtime_t logged; }; struct fetchctx { @@ -1587,18 +1591,14 @@ fcount_logspill(fetchctx_t *fctx, fctxcount_t *counter, bool final) { return; } - uint_fast32_t allowed = atomic_load_relaxed(&counter->allowed); - uint_fast32_t dropped = atomic_load_relaxed(&counter->dropped); - isc_stdtime_t logged = atomic_load_relaxed(&counter->logged); - /* Do not log a message if there were no dropped fetches. */ - if (dropped == 0) { + if (counter->dropped == 0) { return; } /* Do not log the cumulative message if the previous log is recent. */ isc_stdtime_get(&now); - if (!final && logged > now - 60) { + if (!final && counter->logged > now - 60) { return; } @@ -1610,20 +1610,20 @@ fcount_logspill(fetchctx_t *fctx, fctxcount_t *counter, bool final) { "too many simultaneous fetches for %s " "(allowed %" PRIuFAST32 " spilled %" PRIuFAST32 "; %s)", - dbuf, allowed, dropped, - dropped == 1 ? "initial trigger event" - : "cumulative since " - "initial trigger event"); + dbuf, counter->allowed, counter->dropped, + counter->dropped == 1 ? "initial trigger event" + : "cumulative since " + "initial trigger event"); } else { isc_log_write(dns_lctx, DNS_LOGCATEGORY_SPILL, DNS_LOGMODULE_RESOLVER, ISC_LOG_INFO, "fetch counters for %s now being discarded " "(allowed %" PRIuFAST32 " spilled %" PRIuFAST32 "; cumulative since initial trigger event)", - dbuf, allowed, dropped); + dbuf, counter->allowed, counter->dropped); } - atomic_store_release(&counter->logged, now); + counter->logged = now; } static isc_result_t @@ -1632,7 +1632,6 @@ fcount_incr(fetchctx_t *fctx, bool force) { dns_resolver_t *res = NULL; fctxcount_t *counter = NULL; uint32_t hashval; - uint_fast32_t count; uint_fast32_t spill; REQUIRE(fctx != NULL); @@ -1640,6 +1639,11 @@ fcount_incr(fetchctx_t *fctx, bool force) { REQUIRE(res != NULL); INSIST(fctx->counter == NULL); + spill = atomic_load_acquire(&res->zspill); + if (force || spill == 0) { + return (ISC_R_SUCCESS); + } + hashval = isc_hashmap_hash(res->counters, fctx->domain->ndata, fctx->domain->length); @@ -1652,6 +1656,7 @@ fcount_incr(fetchctx_t *fctx, bool force) { case ISC_R_NOTFOUND: counter = isc_mem_get(fctx->mctx, sizeof(*counter)); *counter = (fctxcount_t){ + .magic = FCTXCOUNT_MAGIC, .count = 0, .allowed = 0, }; @@ -1666,22 +1671,22 @@ fcount_incr(fetchctx_t *fctx, bool force) { default: UNREACHABLE(); } - count = atomic_fetch_add_relaxed(&counter->count, 1) + 1; + INSIST(VALID_FCTXCOUNT(counter)); + + INSIST(spill > 0); + if (++counter->count > spill) { + counter->count--; + INSIST(counter->count > 0); + counter->dropped++; + fcount_logspill(fctx, counter, false); + result = ISC_R_QUOTA; + } else { + counter->allowed++; + fctx->counter = counter; + } UNLOCK(&res->counters_lock); - spill = atomic_load_acquire(&res->zspill); - if (!force && spill != 0 && count > spill) { - atomic_fetch_sub_release(&counter->count, 1); - atomic_fetch_add_relaxed(&counter->dropped, 1); - fcount_logspill(fctx, counter, false); - return (ISC_R_QUOTA); - } - - (void)atomic_fetch_add_relaxed(&counter->allowed, 1); - - fctx->counter = counter; - - return (ISC_R_SUCCESS); + return (result); } static void @@ -1694,14 +1699,10 @@ fcount_decr(fetchctx_t *fctx) { } fctx->counter = NULL; - uint_fast32_t count = atomic_fetch_sub_release(&counter->count, 1) - 1; - if (count == 0) { - LOCK(&fctx->res->counters_lock); - if (atomic_load_acquire(&counter->count) > 0) { - /* Other thread reacquired the counter */ - goto unlock; - } - + LOCK(&fctx->res->counters_lock); + INSIST(VALID_FCTXCOUNT(counter)); + INSIST(counter->count > 0); + if (--counter->count == 0) { isc_result_t result = isc_hashmap_delete( fctx->res->counters, NULL, counter->domain->ndata, counter->domain->length); @@ -1709,9 +1710,8 @@ fcount_decr(fetchctx_t *fctx) { fcount_logspill(fctx, counter, true); isc_mem_put(fctx->mctx, counter, sizeof(*counter)); - unlock: - UNLOCK(&fctx->res->counters_lock); } + UNLOCK(&fctx->res->counters_lock); } static void @@ -4408,7 +4408,7 @@ resume_qmin(isc_task_t *task, isc_event_t *event) { dns_name_copy(fname, fctx->domain); - result = fcount_incr(fctx, false); + result = fcount_incr(fctx, true); if (result != ISC_R_SUCCESS) { goto cleanup; } @@ -11395,15 +11395,12 @@ dns_resolver_dumpfetches(dns_resolver_t *res, isc_statsformat_t format, { fctxcount_t *counter = NULL; isc_hashmap_iter_current(it, (void **)&counter); - uint_fast32_t count = atomic_load_relaxed(&counter->count); - uint_fast32_t dropped = atomic_load_relaxed(&counter->dropped); - uint_fast32_t allowed = atomic_load_relaxed(&counter->allowed); dns_name_print(counter->domain, fp); fprintf(fp, ": %" PRIuFAST32 " active (%" PRIuFAST32 " spilled, %" PRIuFAST32 " allowed)\n", - count, dropped, allowed); + counter->count, counter->dropped, counter->allowed); } UNLOCK(&res->counters_lock); isc_hashmap_iter_destroy(&it); @@ -11432,11 +11429,7 @@ dns_resolver_dumpquota(dns_resolver_t *res, isc_buffer_t **buf) { char nb[DNS_NAME_FORMATSIZE], text[DNS_NAME_FORMATSIZE + BUFSIZ]; - uint_fast32_t count = atomic_load_relaxed(&counter->count); - uint_fast32_t allowed = atomic_load_relaxed(&counter->allowed); - uint_fast32_t dropped = atomic_load_relaxed(&counter->dropped); - - if (count < spill) { + if (counter->count < spill) { continue; } @@ -11444,7 +11437,8 @@ dns_resolver_dumpquota(dns_resolver_t *res, isc_buffer_t **buf) { snprintf(text, sizeof(text), "\n- %s: %" PRIuFAST32 " active (allowed %" PRIuFAST32 " spilled %" PRIuFAST32 ")", - nb, count, allowed, dropped); + nb, counter->count, counter->allowed, + counter->dropped); result = isc_buffer_reserve(*buf, strlen(text)); if (result != ISC_R_SUCCESS) {