From: Mukund Sivaraman Date: Sun, 18 Mar 2018 14:52:48 +0000 (+0530) Subject: Various fixes to lib/isc/stats.c X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=bcb011a306053092250464e9ea6707561f7e7784;p=thirdparty%2Fbind9.git 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 36c8f646a7c..4341442fd50 100644 --- 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 }