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.
This commit is contained in:
Beat Küng 2017-03-16 08:40:34 +01:00 committed by Lorenz Meier
parent 4b01e5b6b6
commit 446c734edc

View File

@ -43,8 +43,12 @@
#include <sys/queue.h>
#include <drivers/drv_hrt.h>
#include <math.h>
#include <pthread.h>
#include <systemlib/err.h>
#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;
}