simplify the isc_stat structure to take avantage of atomics

lib/isc/stats.c

@@ -21,7 +21,7 @@
 #include <isc/mem.h>
 #include <isc/platform.h>
 #include <isc/print.h>
-#include <isc/rwlock.h>
+#include <isc/refcount.h>
 #include <isc/stats.h>
 #include <isc/util.h>
 
@@ -31,78 +31,30 @@
 typedef atomic_int_fast64_t isc_stat_t;
 
 struct isc_stats {
-	/*% Unlocked */
 	unsigned int		magic;
 	isc_mem_t		*mctx;
+	isc_refcount_t		refs;
 	int			ncounters;
-
-	isc_mutex_t	lock;
-	unsigned int	references; /* locked by lock */
-
-	/*%
-	 * Locked by counterlock or unlocked if efficient rwlock is not
-	 * available.
-	 */
 	isc_stat_t		*counters;
-
-	/*%
-	 * We don't want to lock the counters while we are dumping, so we first
-	 * copy the current counter values into a local array.  This buffer
-	 * will be used as the copy destination.  It's allocated on creation
-	 * of the stats structure so that the dump operation won't fail due
-	 * to memory allocation failure.
-	 * XXX: this approach is weird for non-threaded build because the
-	 * additional memory and the copy overhead could be avoided.  We prefer
-	 * simplicity here, however, under the assumption that this function
-	 * should be only rarely called.
-	 */
-	uint64_t	*copiedcounters;
 };
 
 static isc_result_t
 create_stats(isc_mem_t *mctx, int ncounters, isc_stats_t **statsp) {
 	isc_stats_t *stats;
-	isc_result_t result = ISC_R_SUCCESS;
 
 	REQUIRE(statsp != NULL && *statsp == NULL);
 
 	stats = isc_mem_get(mctx, sizeof(*stats));
-	if (stats == NULL)
-		return (ISC_R_NOMEMORY);
-
-	isc_mutex_init(&stats->lock);
-
 	stats->counters = isc_mem_get(mctx, sizeof(isc_stat_t) * ncounters);
-	if (stats->counters == NULL) {
+	isc_refcount_init(&stats->refs, 1);
-		result = ISC_R_NOMEMORY;
-		goto clean_mutex;
-	}
-	stats->copiedcounters = isc_mem_get(mctx,
-					    sizeof(uint64_t) * ncounters);
-	if (stats->copiedcounters == NULL) {
-		result = ISC_R_NOMEMORY;
-		goto clean_counters;
-	}
-
-	stats->references = 1;
 	memset(stats->counters, 0, sizeof(isc_stat_t) * ncounters);
 	stats->mctx = NULL;
 	isc_mem_attach(mctx, &stats->mctx);
 	stats->ncounters = ncounters;
 	stats->magic = ISC_STATS_MAGIC;
-
 	*statsp = stats;
 
-	return (result);
+	return (ISC_R_SUCCESS);
-
-clean_counters:
-	isc_mem_put(mctx, stats->counters, sizeof(isc_stat_t) * ncounters);
-
-clean_mutex:
-	isc_mutex_destroy(&stats->lock);
-	isc_mem_put(mctx, stats, sizeof(*stats));
-
-	return (result);
 }
 
 void
@@ -110,10 +62,7 @@ isc_stats_attach(isc_stats_t *stats, isc_stats_t **statsp) {
 	REQUIRE(ISC_STATS_VALID(stats));
 	REQUIRE(statsp != NULL && *statsp == NULL);
 
-	LOCK(&stats->lock);
+	isc_refcount_increment(&stats->refs);
-	stats->references++;
-	UNLOCK(&stats->lock);
-
 	*statsp = stats;
 }
 
@@ -126,21 +75,11 @@ isc_stats_detach(isc_stats_t **statsp) {
 	stats = *statsp;
 	*statsp = NULL;
 
-	LOCK(&stats->lock);
+	if (isc_refcount_decrement(&stats->refs) == 1) {
-	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);
-		isc_mutex_destroy(&stats->lock);
 		isc_mem_putanddetach(&stats->mctx, stats, sizeof(*stats));
-		return;
 	}
-
-	UNLOCK(&stats->lock);
 }
 
 int
@@ -184,16 +123,12 @@ isc_stats_dump(isc_stats_t *stats, isc_stats_dumper_t dump_fn,
 	REQUIRE(ISC_STATS_VALID(stats));
 
 	for (i = 0; i < stats->ncounters; i++) {
-		stats->copiedcounters[i] =
+		uint32_t counter = atomic_load_explicit(&stats->counters[i],
-			atomic_load_explicit(&stats->counters[i],
 							memory_order_relaxed);
-	}
+		if ((options & ISC_STATSDUMP_VERBOSE) == 0 && counter == 0) {
-
-	for (i = 0; i < stats->ncounters; i++) {
-		if ((options & ISC_STATSDUMP_VERBOSE) == 0 &&
-		    stats->copiedcounters[i] == 0)
 			continue;
-		dump_fn((isc_statscounter_t)i, stats->copiedcounters[i], arg);
+		}
+		dump_fn((isc_statscounter_t)i, counter, arg);
 	}
 }