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.

diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c
index 32cb137f7ae1bcf556ee9d4132fd0bfc47de4e78..cf1ac6e069bfe6bb21b9ed35a2c428915511bd62 100644 (file)
--- 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;
-       UNLOCK(&res->counters_lock);
+       INSIST(VALID_FCTXCOUNT(counter));
 
-       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);
+       INSIST(spill > 0);
+       if (++counter->count > spill) {
+               counter->count--;
+               INSIST(counter->count > 0);
+               counter->dropped++;
                fcount_logspill(fctx, counter, false);
-               return (ISC_R_QUOTA);
+               result = ISC_R_QUOTA;
+       } else {
+               counter->allowed++;
+               fctx->counter = counter;
        }
+       UNLOCK(&res->counters_lock);
 
-       (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) {