From: Jason Dillaman Date: Fri, 8 Jan 2016 17:34:45 +0000 (-0500) Subject: helgrind: annotate false-positive race conditions X-Git-Tag: v10.0.3~46^2~1^2~7 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=8f8b4e2947db9314c60c31ccd08c0d3bb1805926;p=ceph.git helgrind: annotate false-positive race conditions Fixes: #14163 Signed-off-by: Jason Dillaman --- diff --git a/src/common/HeartbeatMap.cc b/src/common/HeartbeatMap.cc index f2bf02d31f09..51b7aa796e16 100644 --- a/src/common/HeartbeatMap.cc +++ b/src/common/HeartbeatMap.cc @@ -21,6 +21,7 @@ #include "HeartbeatMap.h" #include "ceph_context.h" #include "common/errno.h" +#include "common/valgrind.h" #include "debug.h" #define dout_subsys ceph_subsys_heartbeatmap @@ -48,6 +49,10 @@ heartbeat_handle_d *HeartbeatMap::add_worker(const string& name) m_rwlock.get_write(); ldout(m_cct, 10) << "add_worker '" << name << "'" << dendl; heartbeat_handle_d *h = new heartbeat_handle_d(name); + ANNOTATE_BENIGN_RACE_SIZED(&h->timeout, sizeof(h->timeout), + "heartbeat_handle_d timeout"); + ANNOTATE_BENIGN_RACE_SIZED(&h->suicide_timeout, sizeof(h->suicide_timeout), + "heartbeat_handle_d suicide_timeout"); m_workers.push_front(h); h->list_item = m_workers.begin(); m_rwlock.put_write(); diff --git a/src/common/Mutex.cc b/src/common/Mutex.cc index 5e9b590a5395..05059b969917 100644 --- a/src/common/Mutex.cc +++ b/src/common/Mutex.cc @@ -20,6 +20,7 @@ #include "include/stringify.h" #include "include/utime.h" #include "common/Clock.h" +#include "common/valgrind.h" Mutex::Mutex(const std::string &n, bool r, bool ld, bool bt, @@ -27,6 +28,9 @@ Mutex::Mutex(const std::string &n, bool r, bool ld, name(n), id(-1), recursive(r), lockdep(ld), backtrace(bt), nlock(0), locked_by(0), cct(cct), logger(0) { + ANNOTATE_BENIGN_RACE_SIZED(&id, sizeof(id), "Mutex lockdep id"); + ANNOTATE_BENIGN_RACE_SIZED(&nlock, sizeof(nlock), "Mutex nlock"); + ANNOTATE_BENIGN_RACE_SIZED(&locked_by, sizeof(locked_by), "Mutex locked_by"); if (cct) { PerfCountersBuilder b(cct, string("mutex-") + name, l_mutex_first, l_mutex_last); @@ -71,7 +75,11 @@ Mutex::Mutex(const std::string &n, bool r, bool ld, Mutex::~Mutex() { assert(nlock == 0); + + // helgrind gets confused by condition wakeups leading to mutex destruction + ANNOTATE_BENIGN_RACE_SIZED(&_m, sizeof(_m), "Mutex primitive"); pthread_mutex_destroy(&_m); + if (cct && logger) { cct->get_perfcounters_collection()->remove(logger); delete logger; diff --git a/src/common/RWLock.h b/src/common/RWLock.h index 47a8c87f500a..282b69bdfe6f 100644 --- a/src/common/RWLock.h +++ b/src/common/RWLock.h @@ -22,6 +22,7 @@ #include #include "lockdep.h" #include "include/atomic.h" +#include "common/valgrind.h" class RWLock { @@ -39,6 +40,9 @@ public: RWLock(const std::string &n, bool track_lock=true) : name(n), id(-1), nrlock(0), nwlock(0), track(track_lock) { pthread_rwlock_init(&L, NULL); + ANNOTATE_BENIGN_RACE_SIZED(&id, sizeof(id), "RWLock lockdep id"); + ANNOTATE_BENIGN_RACE_SIZED(&nrlock, sizeof(nrlock), "RWlock nrlock"); + ANNOTATE_BENIGN_RACE_SIZED(&nwlock, sizeof(nwlock), "RWlock nwlock"); if (g_lockdep) id = lockdep_register(name.c_str()); } diff --git a/src/common/RefCountedObj.h b/src/common/RefCountedObj.h index 3755018f8011..cd886f66f375 100644 --- a/src/common/RefCountedObj.h +++ b/src/common/RefCountedObj.h @@ -19,6 +19,7 @@ #include "common/Cond.h" #include "include/atomic.h" #include "common/ceph_context.h" +#include "common/valgrind.h" struct RefCountedObject { private: @@ -41,8 +42,13 @@ public: void put() { CephContext *local_cct = cct; int v = nref.dec(); - if (v == 0) + if (v == 0) { + ANNOTATE_HAPPENS_AFTER(&nref); + ANNOTATE_HAPPENS_BEFORE_FORGET_ALL(&nref); delete this; + } else { + ANNOTATE_HAPPENS_BEFORE(&nref); + } if (local_cct) lsubdout(local_cct, refs, 1) << "RefCountedObject::put " << this << " " << (v + 1) << " -> " << v diff --git a/src/common/buffer.cc b/src/common/buffer.cc index 10e01a00457d..904c02e39cc1 100644 --- a/src/common/buffer.cc +++ b/src/common/buffer.cc @@ -20,6 +20,7 @@ #include "common/simple_spin.h" #include "common/strtol.h" #include "common/likely.h" +#include "common/valgrind.h" #include "include/atomic.h" #include "common/RWLock.h" #include "include/types.h" @@ -746,7 +747,11 @@ static simple_spinlock_t buffer_debug_lock = SIMPLE_SPINLOCK_INITIALIZER; _raw = tr->clone(); _raw->nref.set(1); if (unlikely(tr->nref.dec() == 0)) { + ANNOTATE_HAPPENS_AFTER(&tr->nref); + ANNOTATE_HAPPENS_BEFORE_FORGET_ALL(&tr->nref); delete tr; + } else { + ANNOTATE_HAPPENS_BEFORE(&tr->nref); } } return *this; @@ -771,7 +776,11 @@ static simple_spinlock_t buffer_debug_lock = SIMPLE_SPINLOCK_INITIALIZER; bdout << "ptr " << this << " release " << _raw << bendl; if (_raw->nref.dec() == 0) { //cout << "hosing raw " << (void*)_raw << " len " << _raw->len << std::endl; + ANNOTATE_HAPPENS_AFTER(&_raw->nref); + ANNOTATE_HAPPENS_BEFORE_FORGET_ALL(&_raw->nref); delete _raw; // dealloc old (if any) + } else { + ANNOTATE_HAPPENS_BEFORE(&_raw->nref); } _raw = 0; } diff --git a/src/common/ceph_context.cc b/src/common/ceph_context.cc index 4ead871e9b41..d1dfb26f7f7b 100644 --- a/src/common/ceph_context.cc +++ b/src/common/ceph_context.cc @@ -33,6 +33,7 @@ #include "common/Mutex.h" #include "common/Cond.h" #include "common/PluginRegistry.h" +#include "common/valgrind.h" #include #include @@ -533,6 +534,16 @@ CephContext::~CephContext() ceph::crypto::shutdown(); } +void CephContext::put() { + if (nref.dec() == 0) { + ANNOTATE_HAPPENS_AFTER(&nref); + ANNOTATE_HAPPENS_BEFORE_FORGET_ALL(&nref); + delete this; + } else { + ANNOTATE_HAPPENS_BEFORE(&nref); + } +} + void CephContext::init_crypto() { ceph::crypto::init(this); diff --git a/src/common/ceph_context.h b/src/common/ceph_context.h index ab187c56ea26..99b2181b844e 100644 --- a/src/common/ceph_context.h +++ b/src/common/ceph_context.h @@ -67,10 +67,7 @@ public: nref.inc(); return this; } - void put() { - if (nref.dec() == 0) - delete this; - } + void put(); md_config_t *_conf; ceph::log::Log *_log; diff --git a/src/common/common_init.cc b/src/common/common_init.cc index 23c2e7ce7a19..3303a7a1f104 100644 --- a/src/common/common_init.cc +++ b/src/common/common_init.cc @@ -21,6 +21,7 @@ #include "common/dout.h" #include "common/errno.h" #include "common/safe_io.h" +#include "common/valgrind.h" #include "common/version.h" #include "include/color.h" @@ -36,6 +37,7 @@ CephContext *common_preinit(const CephInitParameters &iparams, enum code_environment_t code_env, int flags) { // set code environment + ANNOTATE_BENIGN_RACE_SIZED(&g_code_env, sizeof(g_code_env), "g_code_env"); g_code_env = code_env; // Create a configuration object diff --git a/src/common/lockdep.cc b/src/common/lockdep.cc index c39dbf0e3364..e46540d5cd09 100644 --- a/src/common/lockdep.cc +++ b/src/common/lockdep.cc @@ -15,6 +15,7 @@ #include "Clock.h" #include "common/dout.h" #include "common/environment.h" +#include "common/valgrind.h" #include "include/types.h" #include "lockdep.h" @@ -67,6 +68,10 @@ void lockdep_register_ceph_context(CephContext *cct) { pthread_mutex_lock(&lockdep_mutex); if (g_lockdep_ceph_ctx == NULL) { + ANNOTATE_BENIGN_RACE_SIZED(&g_lockdep_ceph_ctx, sizeof(g_lockdep_ceph_ctx), + "lockdep cct"); + ANNOTATE_BENIGN_RACE_SIZED(&g_lockdep, sizeof(g_lockdep), + "lockdep enabled"); g_lockdep = true; g_lockdep_ceph_ctx = cct; lockdep_dout(0) << "lockdep start" << dendl; diff --git a/src/common/perf_counters.cc b/src/common/perf_counters.cc index 0c86f130b5a2..9bb500e2586e 100644 --- a/src/common/perf_counters.cc +++ b/src/common/perf_counters.cc @@ -18,6 +18,7 @@ #include "common/dout.h" #include "common/errno.h" #include "common/Formatter.h" +#include "common/valgrind.h" #include #include @@ -180,6 +181,9 @@ void PerfCounters::set(int idx, uint64_t amt) perf_counter_data_any_d& data(m_data[idx - m_lower_bound - 1]); if (!(data.type & PERFCOUNTER_U64)) return; + + ANNOTATE_BENIGN_RACE_SIZED(&data.u64, sizeof(data.u64), + "perf counter atomic"); if (data.type & PERFCOUNTER_LONGRUNAVG) { data.avgcount.inc(); data.u64.set(amt); diff --git a/src/common/valgrind.h b/src/common/valgrind.h index 2aa3fb5380ac..8efa60bf057e 100644 --- a/src/common/valgrind.h +++ b/src/common/valgrind.h @@ -4,12 +4,16 @@ #ifndef CEPH_VALGRIND_H #define CEPH_VALGRIND_H +#include "acconfig.h" + #ifdef HAVE_VALGRIND_HELGRIND_H #include #else - #define ANNOTATE_HAPPENS_AFTER(x) do {} while (0) - #define ANNOTATE_HAPPENS_BEFORE_FORGET_ALL(x) ANNOTATE_HAPPENS_AFTER(x) - #define ANNOTATE_HAPPENS_BEFORE(x) ANNOTATE_HAPPENS_AFTER(x) + #define ANNOTATE_HAPPENS_AFTER(x) (void)0 + #define ANNOTATE_HAPPENS_BEFORE_FORGET_ALL(x) (void)0 + #define ANNOTATE_HAPPENS_BEFORE(x) (void)0 + + #define ANNOTATE_BENIGN_RACE_SIZED(address, size, description) (void)0 #endif #endif // CEPH_VALGRIND_H diff --git a/src/log/Log.cc b/src/log/Log.cc index 860b2c7cdb96..b1d391fb2041 100644 --- a/src/log/Log.cc +++ b/src/log/Log.cc @@ -12,6 +12,7 @@ #include "common/errno.h" #include "common/safe_io.h" #include "common/Clock.h" +#include "common/valgrind.h" #include "include/assert.h" #include "include/compat.h" #include "include/on_exit.h" @@ -189,6 +190,8 @@ Entry *Log::create_entry(int level, int subsys) Entry *Log::create_entry(int level, int subsys, size_t* expected_size) { if (true) { + ANNOTATE_BENIGN_RACE_SIZED(expected_size, sizeof(*expected_size), + "Log hint"); size_t size = __atomic_load_n(expected_size, __ATOMIC_RELAXED); void *ptr = ::operator new(sizeof(Entry) + size); return new(ptr) Entry(ceph_clock_now(NULL), diff --git a/src/msg/simple/Pipe.cc b/src/msg/simple/Pipe.cc index 33884c81cda4..d91e6bb9e83b 100644 --- a/src/msg/simple/Pipe.cc +++ b/src/msg/simple/Pipe.cc @@ -27,6 +27,7 @@ #include "common/debug.h" #include "common/errno.h" +#include "common/valgrind.h" // Below included to get encode_encrypt(); That probably should be in Crypto.h, instead @@ -83,6 +84,9 @@ Pipe::Pipe(SimpleMessenger *r, int st, PipeConnection *con) send_keepalive_ack(false), connect_seq(0), peer_global_seq(0), out_seq(0), in_seq(0), in_seq_acked(0) { + ANNOTATE_BENIGN_RACE_SIZED(&state, sizeof(state), "Pipe state"); + ANNOTATE_BENIGN_RACE_SIZED(&recv_len, sizeof(recv_len), "Pipe recv_len"); + ANNOTATE_BENIGN_RACE_SIZED(&recv_ofs, sizeof(recv_ofs), "Pipe recv_ofs"); if (con) { connection_state = con; connection_state->reset_pipe(this); diff --git a/src/msg/simple/SimpleMessenger.cc b/src/msg/simple/SimpleMessenger.cc index fdb7278292d4..721dc4c9177e 100644 --- a/src/msg/simple/SimpleMessenger.cc +++ b/src/msg/simple/SimpleMessenger.cc @@ -22,6 +22,7 @@ #include "common/config.h" #include "common/Timer.h" #include "common/errno.h" +#include "common/valgrind.h" #include "auth/Crypto.h" #include "include/Spinlock.h" @@ -53,6 +54,8 @@ SimpleMessenger::SimpleMessenger(CephContext *cct, entity_name_t name, timeout(0), local_connection(new PipeConnection(cct, this)) { + ANNOTATE_BENIGN_RACE_SIZED(&timeout, sizeof(timeout), + "SimpleMessenger read timeout"); ceph_spin_init(&global_seq_lock); local_features = features; init_local_connection(); @@ -701,6 +704,8 @@ void SimpleMessenger::learned_addr(const entity_addr_t &peer_addr_for_me) if (need_addr) { entity_addr_t t = peer_addr_for_me; t.set_port(my_inst.addr.get_port()); + ANNOTATE_BENIGN_RACE_SIZED(&my_inst.addr.addr, sizeof(my_inst.addr.addr), + "SimpleMessenger learned addr"); my_inst.addr.addr = t.addr; ldout(cct,1) << "learned my addr " << my_inst.addr << dendl; need_addr = false;