From 446c734edc92c57be6a4b60320f02cd8e4025ce5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beat=20K=C3=BCng?= Date: Thu, 16 Mar 2017 08:40:34 +0100 Subject: [PATCH] perf_counter: use a mutex to protect concurrent access to the perf_counters linked list perf_counters is read from and written to by different threads and thus requires synchronization. Without it we risk accessing invalid memory. There are still remaining issues (see comment in code), they will not lead to a crash however. --- src/modules/systemlib/perf_counter.c | 35 +++++++++++++++++++++++++--- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/src/modules/systemlib/perf_counter.c b/src/modules/systemlib/perf_counter.c index 1cb0ad2f5b..0e325146b2 100644 --- a/src/modules/systemlib/perf_counter.c +++ b/src/modules/systemlib/perf_counter.c @@ -43,8 +43,12 @@ #include #include #include +#include +#include + #include "perf_counter.h" + #ifdef __PX4_QURT // There is presumably no dprintf on QURT. Therefore use the usual output to mini-dm. #define dprintf(_fd, _text, ...) ((_fd) == 1 ? PX4_INFO((_text), ##__VA_ARGS__) : (void)(_fd)) @@ -99,7 +103,18 @@ struct perf_ctr_interval { /** * List of all known counters. */ -static sq_queue_t perf_counters; +static sq_queue_t perf_counters = { NULL, NULL }; + +/** + * mutex protecting access to the perf_counters linked list (which is read from & written to by different threads) + */ +pthread_mutex_t perf_counters_mutex = PTHREAD_MUTEX_INITIALIZER; +// FIXME: the mutex does **not** protect against access to/from the perf +// counter's data. It can still happen that a counter is updated while it is +// printed. This can lead to inconsistent output, or completely bogus values +// (especially the 64bit values which are in general not atomically updated). +// The same holds for shared perf counters (perf_alloc_once), that can be updated +// concurrently (this affects the 'ctrl_latency' counter). perf_counter_t @@ -128,7 +143,9 @@ perf_alloc(enum perf_counter_type type, const char *name) if (ctr != NULL) { ctr->type = type; ctr->name = name; + pthread_mutex_lock(&perf_counters_mutex); sq_addfirst(&ctr->link, &perf_counters); + pthread_mutex_unlock(&perf_counters_mutex); } return ctr; @@ -137,16 +154,19 @@ perf_alloc(enum perf_counter_type type, const char *name) perf_counter_t perf_alloc_once(enum perf_counter_type type, const char *name) { + pthread_mutex_lock(&perf_counters_mutex); perf_counter_t handle = (perf_counter_t)sq_peek(&perf_counters); while (handle != NULL) { if (!strcmp(handle->name, name)) { if (type == handle->type) { /* they are the same counter */ + pthread_mutex_unlock(&perf_counters_mutex); return handle; } else { /* same name but different type, assuming this is an error and not intended */ + pthread_mutex_unlock(&perf_counters_mutex); return NULL; } } @@ -154,6 +174,8 @@ perf_alloc_once(enum perf_counter_type type, const char *name) handle = (perf_counter_t)sq_next(&handle->link); } + pthread_mutex_unlock(&perf_counters_mutex); + /* if the execution reaches here, no existing counter of that name was found */ return perf_alloc(type, name); } @@ -165,7 +187,9 @@ perf_free(perf_counter_t handle) return; } + pthread_mutex_lock(&perf_counters_mutex); sq_rem(&handle->link, &perf_counters); + pthread_mutex_unlock(&perf_counters_mutex); free(handle); } @@ -290,8 +314,6 @@ perf_end(perf_counter_t handle) } } -#include "systemlib/err.h" - void perf_set_elapsed(perf_counter_t handle, int64_t elapsed) { @@ -497,14 +519,18 @@ perf_event_count(perf_counter_t handle) void perf_print_all(int fd) { + pthread_mutex_lock(&perf_counters_mutex); perf_counter_t handle = (perf_counter_t)sq_peek(&perf_counters); while (handle != NULL) { perf_print_counter_fd(fd, handle); handle = (perf_counter_t)sq_next(&handle->link); } + + pthread_mutex_unlock(&perf_counters_mutex); } +// these are defined in drv_hrt.c extern const uint16_t latency_bucket_count; extern uint32_t latency_counters[]; extern const uint16_t latency_buckets[]; @@ -525,6 +551,7 @@ perf_print_latency(int fd) void perf_reset_all(void) { + pthread_mutex_lock(&perf_counters_mutex); perf_counter_t handle = (perf_counter_t)sq_peek(&perf_counters); while (handle != NULL) { @@ -532,6 +559,8 @@ perf_reset_all(void) handle = (perf_counter_t)sq_next(&handle->link); } + pthread_mutex_unlock(&perf_counters_mutex); + for (int i = 0; i <= latency_bucket_count; i++) { latency_counters[i] = 0; }