helgrind: annotate false-positive race conditions

Fixes: #14163
Signed-off-by: Jason Dillaman <dillaman@redhat.com>

diff --git a/src/common/HeartbeatMap.cc b/src/common/HeartbeatMap.cc
index f2bf02d31f09fca228b6ce8351980964571b911c..51b7aa796e16d61f113b731f1aaf8169a19136fa 100644 (file)
--- 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 5e9b590a5395e56c7dcafc5e638f542d8a161143..05059b969917b2c666c4da989f94f8543cbc0271 100644 (file)
--- 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 47a8c87f500a2440e6c90528a99f016d15567716..282b69bdfe6f9075e34de5351e0e032b67b37519 100644 (file)
--- a/src/common/RWLock.h
+++ b/src/common/RWLock.h
@@ -22,6 +22,7 @@
 #include <include/assert.h>
 #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 3755018f801141628bc8a8fea5c2bc3c924a22ea..cd886f66f3757c25935ae0014162aa08ec49ce7d 100644 (file)
--- 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 10e01a00457d6330124b8ba78f825eb69482c7cd..904c02e39cc12b6b2758ccc7e26f3b38239b38ab 100644 (file)
--- 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 4ead871e9b41f7394cf73a8ddab890c9a47aa0d4..d1dfb26f7f7bdf61c121a7ee9b5d7455e7619e12 100644 (file)
--- 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 <iostream>
 #include <pthread.h>
@@ -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 ab187c56ea2686f09b29c881f228bfc6e060a2c1..99b2181b844e4ddcfef404847a9286205ff1cfb3 100644 (file)
--- 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 23c2e7ce7a19bc7484ea69e44f27ba1adb12f9bd..3303a7a1f104ae2150e96ecc347a1cd1c21bbbf2 100644 (file)
--- 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 c39dbf0e3364af84ef2e4f994e36c77d92b14bbe..e46540d5cd090628c991565ca3e787151553163f 100644 (file)
--- 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 0c86f130b5a29495ab95d928b6ac8668f6ed1fad..9bb500e2586e890f7b97fde79468616213d4557c 100644 (file)
--- 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 <errno.h>
 #include <map>
@@ -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 2aa3fb5380acd9aa59daab577c320d1e864aa48a..8efa60bf057e859796a83c1e3f95870ea3c3c6fc 100644 (file)
--- 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 <valgrind/helgrind.h>
 #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 860b2c7cdb96f8e53734538518bf69f054c3fe93..b1d391fb204135effd512721724815a873e9d324 100644 (file)
--- 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 33884c81cda4e68d5392234d48d19bc3e4556ea2..d91e6bb9e83b3cf05d9e48b4253438d7dde46cd8 100644 (file)
--- 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 fdb7278292d48a631c508d2966746e233fb455ab..721dc4c9177e4a8feb55ba9c49efc47ebcabfdf2 100644 (file)
--- 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;