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; }