Various fixes to lib/isc/stats.c

* Re-introduce ISC_STATS_LOCKCOUNTERS so that it is an optional feature (it adds lock contention)
* Fix desired rwlock type in lock calls (they were opposite of what should be used)
* Add locking to isc_stats_set()
* Inline create_stats()

diff --git a/lib/isc/stats.c b/lib/isc/stats.c
index 36c8f646a7c8b56e132310c10c85cadd793ad00c..4341442fd50eb061724e2912c6811eb8b05c393e 100644 (file)
--- a/lib/isc/stats.c
+++ b/lib/isc/stats.c
@@ -29,6 +29,13 @@
 #define ISC_STATS_MAGIC                        ISC_MAGIC('S', 't', 'a', 't')
 #define ISC_STATS_VALID(x)             ISC_MAGIC_VALID(x, ISC_STATS_MAGIC)
 
+/*%
+ * `ISC_STATS_LOCKCOUNTERS` indicates if the complete set of statistics
+ * counters have to be locked before update, so that an instantaneous
+ * snapshot of them can be dumped.
+ */
+#define ISC_STATS_LOCKCOUNTERS 0
+
 typedef atomic_int_fast64_t isc_stat_t;
 
 struct isc_stats {
@@ -41,10 +48,11 @@ struct isc_stats {
        unsigned int    references; /* locked by lock */
 
        /*%
-        * Locked by counterlock or unlocked if efficient rwlock is not
-        * available.
+        * Locked by counterlock or unlocked.
         */
+#if ISC_STATS_LOCKCOUNTERS
        isc_rwlock_t    counterlock;
+#endif
        isc_stat_t      *counters;
 
        /*%
@@ -61,8 +69,56 @@ struct isc_stats {
        isc_uint64_t    *copiedcounters;
 };
 
-static isc_result_t
-create_stats(isc_mem_t *mctx, int ncounters, isc_stats_t **statsp) {
+void
+isc_stats_attach(isc_stats_t *stats, isc_stats_t **statsp) {
+       REQUIRE(ISC_STATS_VALID(stats));
+       REQUIRE(statsp != NULL && *statsp == NULL);
+
+       LOCK(&stats->lock);
+       stats->references++;
+       UNLOCK(&stats->lock);
+
+       *statsp = stats;
+}
+
+void
+isc_stats_detach(isc_stats_t **statsp) {
+       isc_stats_t *stats;
+
+       REQUIRE(statsp != NULL && ISC_STATS_VALID(*statsp));
+
+       stats = *statsp;
+       *statsp = NULL;
+
+       LOCK(&stats->lock);
+       stats->references--;
+
+       if (stats->references == 0) {
+               isc_mem_put(stats->mctx, stats->copiedcounters,
+                           sizeof(isc_stat_t) * stats->ncounters);
+               isc_mem_put(stats->mctx, stats->counters,
+                           sizeof(isc_stat_t) * stats->ncounters);
+               UNLOCK(&stats->lock);
+               DESTROYLOCK(&stats->lock);
+#if ISC_STATS_LOCKCOUNTERS
+               isc_rwlock_destroy(&stats->counterlock);
+#endif
+               isc_mem_putanddetach(&stats->mctx, stats, sizeof(*stats));
+               return;
+       }
+
+       UNLOCK(&stats->lock);
+}
+
+int
+isc_stats_ncounters(isc_stats_t *stats) {
+       REQUIRE(ISC_STATS_VALID(stats));
+
+       return (stats->ncounters);
+}
+
+isc_result_t
+isc_stats_create(isc_mem_t *mctx, isc_stats_t **statsp, int ncounters) {
        isc_stats_t *stats;
        isc_result_t result = ISC_R_SUCCESS;
 
@@ -88,12 +144,14 @@ create_stats(isc_mem_t *mctx, int ncounters, isc_stats_t **statsp) {
                result = ISC_R_NOMEMORY;
                goto clean_counters;
        }
-       
+
+#if ISC_STATS_LOCKCOUNTERS
        result = isc_rwlock_init(&stats->counterlock, 0, 0);
        if (result != ISC_R_SUCCESS) {
                goto clean_copiedcounters;
        }
-       
+#endif
+
        stats->references = 1;
        memset(stats->counters, 0, sizeof(isc_stat_t) * ncounters);
        stats->mctx = NULL;
@@ -105,10 +163,12 @@ create_stats(isc_mem_t *mctx, int ncounters, isc_stats_t **statsp) {
 
        return (result);
 
+#if ISC_STATS_LOCKCOUNTERS
 clean_copiedcounters:
        isc_mem_put(mctx, stats->copiedcounters,
                    sizeof(isc_stat_t) * ncounters);
-       
+#endif
+
 clean_counters:
        isc_mem_put(mctx, stats->counters, sizeof(isc_stat_t) * ncounters);
 
@@ -121,59 +181,6 @@ clean_stats:
        return (result);
 }
 
-void
-isc_stats_attach(isc_stats_t *stats, isc_stats_t **statsp) {
-       REQUIRE(ISC_STATS_VALID(stats));
-       REQUIRE(statsp != NULL && *statsp == NULL);
-
-       LOCK(&stats->lock);
-       stats->references++;
-       UNLOCK(&stats->lock);
-
-       *statsp = stats;
-}
-
-void
-isc_stats_detach(isc_stats_t **statsp) {
-       isc_stats_t *stats;
-
-       REQUIRE(statsp != NULL && ISC_STATS_VALID(*statsp));
-
-       stats = *statsp;
-       *statsp = NULL;
-
-       LOCK(&stats->lock);
-       stats->references--;
-
-       if (stats->references == 0) {
-               isc_mem_put(stats->mctx, stats->copiedcounters,
-                           sizeof(isc_stat_t) * stats->ncounters);
-               isc_mem_put(stats->mctx, stats->counters,
-                           sizeof(isc_stat_t) * stats->ncounters);
-               UNLOCK(&stats->lock);
-               DESTROYLOCK(&stats->lock);
-               isc_rwlock_destroy(&stats->counterlock);
-               isc_mem_putanddetach(&stats->mctx, stats, sizeof(*stats));
-               return;
-       }
-
-       UNLOCK(&stats->lock);
-}
-
-int
-isc_stats_ncounters(isc_stats_t *stats) {
-       REQUIRE(ISC_STATS_VALID(stats));
-
-       return (stats->ncounters);
-}
-
-isc_result_t
-isc_stats_create(isc_mem_t *mctx, isc_stats_t **statsp, int ncounters) {
-       REQUIRE(statsp != NULL && *statsp == NULL);
-
-       return (create_stats(mctx, ncounters, statsp));
-}
-
 void
 isc_stats_increment(isc_stats_t *stats, isc_statscounter_t counter) {
        REQUIRE(ISC_STATS_VALID(stats));
@@ -183,10 +190,16 @@ isc_stats_increment(isc_stats_t *stats, isc_statscounter_t counter) {
         * counter while we "writing" a counter field.  The write access itself
         * is protected by the atomic operation.
         */
-       isc_rwlock_lock(&stats->counterlock, isc_rwlocktype_read);
+#if ISC_STATS_LOCKCOUNTERS
+       isc_rwlock_lock(&stats->counterlock, isc_rwlocktype_write);
+#endif
+
        atomic_fetch_add_explicit(&stats->counters[counter], 1,
                                  memory_order_relaxed);
-       isc_rwlock_unlock(&stats->counterlock, isc_rwlocktype_read);
+
+#if ISC_STATS_LOCKCOUNTERS
+       isc_rwlock_unlock(&stats->counterlock, isc_rwlocktype_write);
+#endif
 }
 
 void
@@ -194,10 +207,16 @@ isc_stats_decrement(isc_stats_t *stats, isc_statscounter_t counter) {
        REQUIRE(ISC_STATS_VALID(stats));
        REQUIRE(counter < stats->ncounters);
 
-       isc_rwlock_lock(&stats->counterlock, isc_rwlocktype_read);
+#if ISC_STATS_LOCKCOUNTERS
+       isc_rwlock_lock(&stats->counterlock, isc_rwlocktype_write);
+#endif
+
        atomic_fetch_sub_explicit(&stats->counters[counter], 1,
                                  memory_order_relaxed);
-       isc_rwlock_unlock(&stats->counterlock, isc_rwlocktype_read);
+
+#if ISC_STATS_LOCKCOUNTERS
+       isc_rwlock_unlock(&stats->counterlock, isc_rwlocktype_write);
+#endif
 }
 
 void
@@ -212,13 +231,19 @@ isc_stats_dump(isc_stats_t *stats, isc_stats_dumper_t dump_fn,
         * We use a "write" lock before "reading" the statistics counters as
         * an exclusive lock.
         */
-       isc_rwlock_lock(&stats->counterlock, isc_rwlocktype_write);
+#if ISC_STATS_LOCKCOUNTERS
+       isc_rwlock_lock(&stats->counterlock, isc_rwlocktype_read);
+#endif
+
        for (i = 0; i < stats->ncounters; i++) {
                stats->copiedcounters[i] =
                        atomic_load_explicit(&stats->counters[i],
                                             memory_order_relaxed);
        }
-       isc_rwlock_unlock(&stats->counterlock, isc_rwlocktype_write);
+
+#if ISC_STATS_LOCKCOUNTERS
+       isc_rwlock_unlock(&stats->counterlock, isc_rwlocktype_read);
+#endif
 
        for (i = 0; i < stats->ncounters; i++) {
                if ((options & ISC_STATSDUMP_VERBOSE) == 0 &&
@@ -235,6 +260,14 @@ isc_stats_set(isc_stats_t *stats, isc_uint64_t val,
        REQUIRE(ISC_STATS_VALID(stats));
        REQUIRE(counter < stats->ncounters);
 
+#if ISC_STATS_LOCKCOUNTERS
+       isc_rwlock_lock(&stats->counterlock, isc_rwlocktype_write);
+#endif
+
        atomic_store_explicit(&stats->counters[counter], val,
                              memory_order_relaxed);
+
+#if ISC_STATS_LOCKCOUNTERS
+       isc_rwlock_unlock(&stats->counterlock, isc_rwlocktype_write);
+#endif
 }